Skip to content

Conversation

@wilzbach
Copy link
Contributor

@wilzbach wilzbach commented Jun 3, 2017

So the main problem turned out to be that I use a different method to check for the login state, which means that the session is never added to req.context. The failure manifested as nasty segfault due to Session being null. Thus, the second commit will add the session to req.context as fallback if settings are supplied to oauthSession. In theory, login should also have a check for this - maybe in userAuthUri?

The first commit allows to use the result of the login function, s.t. instead of

webapp.login(req, res, googleOAuthSettings, null, googleScopes))
if (!res.headerWritten)

We can write:

if (webapp.login(req, res, googleOAuthSettings, null, googleScopes))
{

}

Copy link
Owner

@thaven thaven left a comment

Choose a reason for hiding this comment

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

I understand the inconvenience in situations where oauth is not being used to keep track of user logins. Seems sensible to reconsider the naming, and possibly usage, of isLoggedIn. This commit does not seem to solve the problem correctly.

+/
final
OAuthSession oauthSession(scope HTTPServerRequest req) nothrow @trusted
OAuthSession oauthSession(scope HTTPServerRequest req, immutable OAuthSettings settings = null) nothrow @trusted
Copy link
Owner

Choose a reason for hiding this comment

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

The reason for oauthSession to not take a settings parameter, is that it should be possible for multi-provider apps to get the OAuthSession regardless of the settings used to login.

As mentioned in the documentation comment, oauthSession is just a getter to be used only after checking for session availability and validity.

if (auto pCM = "oauth.session" in req.context)
return pCM.get!OAuthSession;
else
return loadSessionToContext(req, settings);
Copy link
Owner

Choose a reason for hiding this comment

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

Even with this, there's still the possibility of oauthSession returning null, i.e. when the user did not login with OAuth. So the need for a null-check will always exist, unless a session is known to be available (e.g. isLoggedIn is known to have returned true for this request).

} ();

loadSessionToContext(req, settings);
return true;
Copy link
Owner

Choose a reason for hiding this comment

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

This would introduce a serious bug. Should only return true if a session is available.

@thaven
Copy link
Owner

thaven commented Jun 17, 2017

Added commit 88e11ac to master.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants