Skip to content

Conversation

@rub-sky
Copy link
Collaborator

@rub-sky rub-sky commented Aug 5, 2016

No description provided.

@coveralls
Copy link

coveralls commented Aug 5, 2016

Coverage Status

Changes Unknown when pulling d741140 on UnauthorizedAccess into * on development*.

@chances
Copy link
Collaborator

chances commented Aug 11, 2016

Why was sinon added?

lib/auth.js Outdated
isAuth = true;
}
req.session.isAuthenticated = isAuth; // Set the isAuthenticate flag
next(); // Continue logic flow as normal
Copy link
Collaborator

@chances chances Aug 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is superfluous. The next callback is an express paradigm

@chances
Copy link
Collaborator

chances commented Aug 11, 2016

The authentication status shouldn't be added to the session because this is middleware. If the middleware is added as ap-wide middleware, then the status is checked for every request and should be added to the request object. Conversely, if it is only route-specific middleware, the same thing applies because the check will be run anyway.

@chances
Copy link
Collaborator

chances commented Aug 11, 2016

The authentication check doesn't need to send a request to Jama. (Nor should it, it creates a large amount of overhead on requests that depend on the middleware.)

We know a user is authenticated if both a session exists with username, password, and project keys and the current session is not expired. The session is only populated with those keys upon successful authentication and is destroyed on logout.

All that is needed is to check that those keys exist on the session.

@rub-sky
Copy link
Collaborator Author

rub-sky commented Aug 16, 2016

@chances, the authentication status should be added as a global variable instead of session variable? Last time we talked about this you mentioned that the middleware should be route specific and each page will handle the check result.

@rub-sky
Copy link
Collaborator Author

rub-sky commented Aug 16, 2016

We used sinon for stubbing.

if (isValid && isServerAuthenticated(username, password, teamName)) {
isAuth = true;
}
req.session.isAuthenticated = isAuth; // Set the isAuthenticate flag
Copy link
Owner

@RickyV33 RickyV33 Aug 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You just need to check if the session fields aren't falsey here, you don't have to make a request to jama. teamName is stored in .env in so you don't need to check the req.session.teamName field for that.

Then you can store the actual result in the req object so we can check it in the routes.

req.isAuthenticated = isAuth;

@RickyV33
Copy link
Owner

Now that it's using the middleware isAuthenticated, it's setting the req.isAuthenticated to true or false. Check that value and only do the work if it's true in the routes that matter such as project/get or graph/get (the else statement)

so...

// in projects
router.get('/', auth.isAuthenticated.....) {
if (req.isAuthenitcated) {
 render.res...
}
}

etc.

"proxyquire": "^1.7.10",
"semistandard": "^8.0.0",
"sinon": "^1.17.4",
"sinon-chai": "^2.8.0",
Copy link
Collaborator

@chances chances Aug 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sinon-chai is unused, remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants