Discussion on Authentication of users

[mjsalinger]

Stemming off a discussion from PR #203, the question on whether the library should include authentication via the Password grant by default. To me this presents several problems:

From the spec:

(B) The authorization server authenticates the resource owner (via the user-agent) and establishes whether the resource owner grants or denies the client's access request.

This implies that it is done via the user agent as a separate request. If the auth request is unauthenticated, then the library should redirect back to the calling program to retrieve credentials. To assume the password grant in this case is not a safe assumption.

Credentials should not be included in a grant request imo - that's beyond the scope of what the OAuth spec provides for the authorization request.

Unless I'm misunderstanding something. Any thoughts?

/cc: @nunofgs @lfk @thomseddon @ruimarinho @night

[lfk]

The entire point of the authentication code grant flow is not to expose the user's credentials to the client application, so naturally I agree with you on that point.

Quoting @nunofgs' previous response to my questions:

1. You have a webapp and you've configured it (with node-oauth2-server) with the password and authorization_code grants.
2. When the user submits the login/password, simply pass the username/password combination to the token-handler (by using the password grant).
3. The response from node-oauth2 is an access_token.
4. Now, if the user attempts to follow the authorization code flow on your webapp, he'll have an access_token.

So the auth code grant flow starts with a request to the authorize endpoint, where -- since the user is unknown -- we redirect to the alternative authentication mechanism, as per the spec. Now, rather than providing a "normal" (i.e. session-based) login, the concept here is that the login form itself utilizes the password grant flow, resulting in the token that the AuthenticationHandler.authorizeHandler is looking for. Again, I never implemented this; it'd be great if @nunofgs would weigh in on my interpretation so I don't end up misleading everybody. :)

Personally I opted for overriding with a custom auth handler (session user id lookup) since I didn't like the concept of issuing double token pairs for every auth code grant, but as I understand it, the two flows remain separate. I'm also not 100% clear on how one would go about distinguishing whether or not to redirect or to show the grant dialog when processing /authorize GETs.

[mjsalinger]

Yeah, I just don't see this as a common flow. It also isn't correct to include a bearer token in an auth grant request, at least not in any implementation I've seen and I think leads to confusion for the user - it confused both you and myself. To me the default should be the most common use case, which here would be to always pass the authenticationHandler.

The older version required a callback to the authorization method, which was used to validate the user. This seems the most common flow to me. I almost feel like the library should expect that, and return an error if the authenticationHandler isn't present, and allow an additional, settable option to use a password-grant flow as an alternate authentication mechanism. I think this will lead to less confusion overall. Thoughts?

[lfk]

IMO something like that'd be preferable, but it would be nice to get more input before proceeding with changes. With clear examples (primarily in the wrappers) it shouldn't be too hard to communicate how to bypass the current default.

[mjsalinger]

Same... Any input?

[nunofgs]

Hey guys!

Really happy about moving forward on node-oauth2-server, especially the change into an oauthjs org.

Again, I never implemented this; it'd be great if @nunofgs would weigh in on my interpretation so I don't end up misleading everybody. :)

I do have this implemented in a production app and I have to say it works great. Obviously I'm biased in this matter but I feel like using OAuth as an authentication solution (even during the authorization code flow) is a great alternative to simply requiring every user to bake in their own login mechanism.

Personally I opted for overriding with a custom auth handler (session user id lookup) since I didn't like the concept of issuing double token pairs for every auth code grant, but as I understand it, the two flows remain separate.

You nailed it. I seem them exactly as two separate flows and the OAuth spec says nothing about how you can authenticate a user during the authorization flow.

I also don't see an issue with "double token pairs". In fact, the user will most likely already possess an Authorization token (thus, they are already "logged in"). Even if they don't, when they login you can store that token for the regular password flow (ie: the token will not go to waste)

I'm also not 100% clear on how one would go about distinguishing whether or not to redirect or to show the grant dialog when processing /authorize GETs.

If I understand you correctly, it's simply a matter of redirecting the user to a /login page if they didn't present an "Authorization" header.

Yeah, I just don't see this as a common flow. It also isn't correct to include a bearer token in an auth grant request, at least not in any implementation I've seen and I think leads to confusion for the user - it confused both you and myself.

While I agree it's not very common I still believe it is at least useful enough to document as a starting point for any developer wanting a full OAuth-authentication solution.

To me the default should be the most common use case, which here would be to always pass the authenticationHandler.

I do agree that this is not the most common of authentication mechanisms (during authorization code flow) so if you prefer, we can simply remove the default and make the authenticationHandler a required parameter.

Agreed?

[mjsalinger]

Really happy about moving forward on node-oauth2-server, especially the change into an oauthjs org.

Yeah, I'm excited. Thanks for moving the express wrapper over there!

You nailed it. I seem them exactly as two separate flows and the OAuth spec says nothing about how you can authenticate a user during the authorization flow.

You have a point, it makes sense.

While I agree it's not very common I still believe it is at least useful enough to document as a starting point for any developer wanting a full OAuth-authentication solution.

I think you're right. Offering a login option will be good for users, I just don't think we want to confuse users in that they have to use our login option.

I do agree that this is not the most common of authentication mechanisms (during authorization code flow) so if you prefer, we can simply remove the default and make the authenticationHandler a required parameter.

Agreed?

100%. Let's remove the default, make authenticationHandler a required parameter, and document the option of having the library handle authentication/login. Works for me!

[steffansluis]

So, I agree with you on the authenticationHandler, I am trying to get it to work, and I'm running into an issue. I'm using express-oauth-server, which proxies the request and response object leaving only the essential information for node-oauth2-server. In the default AuthenicateHandler, a bearer token can be linked back to the user, which is passed in a request header. This request header is the link between the client and the user in this case, or the link between Authenticate and Authorize.

The issue I'm facing is that I already have the user before the authorization request, and I want to pass it directly to the handle function of my custom authenticateHandler. The express way to do this would be to put it on res.locals, but that property gets stripped off of the reponse by node-oauth2-server. This is forcing me to find a different way to communicate the user to the authenticateHandler, limiting me to the properties that aren't filtered out by the request and reponse of node-oauth2-server.

[steffansluis]

I just realized the response constructor takes an options arguments that can be used to achieve what I require, but it is ignored by express-oauth-server. I am still interested though in your opinion about the best way to get the information about the user to the authenticateHandler.

[lfk]

@steffansluis: I solved it by injecting the user ID into the request body right before passing along to my custom authenticateHandler. Here's an example.

Edit: This is using a custom wrapper for Koa, but it should essentially be the same.

[mjsalinger]

@maxtruxa What's your take here?

[maxtruxa]

I think the use case that the default handler covers is one of the rarer ones, as most OAuth2 servers probably implement user authentication using an existing (session-based) login mechanism. So, I'm all for removing the default handler and making authenticateHandler a required parameter.

On a related note, making the handler a simple function instead of an object with a handle method would be cleaner, especially if we're removing the default one. If this is done we could even go as far as moving the handler into the model instead of passing it as an additional argument. It could be called something like getAuthenticatedUser(request, response).

[knightcode]

how do you trigger the right redirect if you have no user to return from the authenticationHandler? It just throws an error.

[maxtruxa]

@knightcode If you don't have a user at that point something else is wrong and there is no sane way to recover from this condition.

@mjsalinger @lfk @nunofgs: I'm going to go ahead and create a PR removing the default handler.

Are we sure about keeping the name/signature the same? I still think that a solution similar to what I proposed earlier in this issue (simplifying the signature and possibly moving the handler to the model) would be the cleanest.

Sorry for the typos, my phone likes to auto-correct me...