Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Does this library support user approval dialog during authorization code grant? #180

Open
saschanaz opened this issue Mar 19, 2023 · 28 comments
Labels
discussion 🗨️ Discussion about a particular topic.

Comments

@saschanaz
Copy link
Contributor

It seems AuthenticationHandler is to see who the user is from the request, not to show an additional dialog for explicit approval. Is it a supported flow?

@jankapunkt
Copy link
Member

This is implementation detail, the allowed flag is set by client implementation and indicates approval if true or denied of false.

@saschanaz
Copy link
Contributor Author

saschanaz commented Mar 19, 2023

It indeed is implementation detail, but I see no way to:

  1. Call authorize() (in a route handler)
  2. After parameter validation, show an (implementation detail) prompt for the user
  3. Redirect when the user approves

@saschanaz
Copy link
Contributor Author

saschanaz commented Mar 19, 2023

if (request.query.allowed === 'false' || request.body.allowed === 'false') {
throw new AccessDeniedError('Access denied: user denied access to application');
}

I feel like the library expects the client to access the authorization endpoint after having approval from the user somehow, is that correct?

@jankapunkt
Copy link
Member

jankapunkt commented Mar 21, 2023

@saschanaz it's the second to third step in the Authorization Code Grant workflow: https://datatracker.ietf.org/doc/html/rfc6749.html#section-4.1 see Part B)

@saschanaz
Copy link
Contributor Author

saschanaz commented Mar 22, 2023

Yes I want to do that, but how can implement that using this library? oauth2orize for example allows rendering a page after the parameter verification with a transaction ID (https://github.com/jaredhanson/oauth2orize#implement-authorization-endpoint).

Edit: to be more specific:

(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.

For this step, in many cases the server renders a page for user with buttons to grant/deny, or login if not yet. I don't immediately see such step in this library, to render an (implementation detail) page before granting code. AFAICT this library just proceeds to grant, doesn't it?

@jankapunkt
Copy link
Member

@saschanaz do you use express? We could create a minimal example project with express + vanilla JavaScript + in memory DB that shows how this could work. We have an examples repo under https://github.com/node-oauth/node-oauth2-server-examples and this would be a perfect example. What do you think?

@saschanaz
Copy link
Contributor Author

I use Fastify but can use fastify-express if needed. Yes, an example would be great.

@jankapunkt
Copy link
Member

@saschanaz I added you as collaborator to the examples repo and created a branch there incl. draft PR: node-oauth/node-oauth2-server-examples#1

Let's continue discussion here: node-oauth/node-oauth2-server-examples#2

@jfstephe
Copy link

jfstephe commented Jun 21, 2023

I had/am having similar issues. I can't believe this is so difficult to get going (that is not a criticism of the project maintainers, just a comment on the project itself).

After hours digging into the source code I found I needed to implement:

....authorize({
  authenticateHandler: {
    handle: function(req, res) {
      // get authenticated user or return falsy value, e.g.:
      return req.session.user;
    }
  }

taken from oauthjs/node-oauth2-server#314

A good, and complete, example would be a great first step though.

To the maintainers: Thanks for keeping this project alive and doing what you do! 👍

@saschanaz
Copy link
Contributor Author

Is it related to showing the dialog page during the authorization process, though? It's only to extract the user info from the request, right?

@jfstephe
Copy link

Well yeah sort of... I was trying to work out how to get the auth code. You can't get an auth code without having an authorised user (I'm still working through this so if this isn't 100% please correct me). If you don't override that function an exception will be thrown as it expects a token to be present.... ...but there can't be a token as you are trying to access this resource for the first time (as per the related issue in the original oauthjs project).

@saschanaz
Copy link
Contributor Author

but there can't be a token as you are trying to access this resource for the first time (as per the related issue in the original oauthjs project).

Exactly, and it should be perfectly fine to not have a token since the client may be a native app and the browser may not have any user info yet when opening oauth/authorize endpoint.

@jankapunkt
Copy link
Member

@saschanaz @jfstephe just to clarify: we are all talking about the authorization code grant workflow here, right?

@saschanaz saschanaz changed the title Does this library support user approval dialog during authorization? Does this library support user approval dialog during authorization code grant? Jun 22, 2023
@saschanaz
Copy link
Contributor Author

Yes, and see https://datatracker.ietf.org/doc/html/rfc8252#section-4.1 for example (as IMO it's clearer than the one in https://datatracker.ietf.org/doc/html/rfc6749.html#section-4.1).

(1) Client app opens a browser tab with the authorization request.

(2) Authorization endpoint receives the authorization request,
authenticates the user, and obtains authorization.
Authenticating the user may involve chaining to other
authentication systems.

A client in the step 1 will, for example, open https://host.example/oauth/authorize?... via a web browser. And this request should be validated and then render something in the browser to show accept/reject buttons. So far I see no way, and oauth-example skips the whole validation and goes straight to show the form, which seems wrong to me.

https://datatracker.ietf.org/doc/html/rfc6749.html#section-4.1.1

The authorization server validates the request to ensure that all
required parameters are present and valid. If the request is valid,
the authorization server authenticates the resource owner and obtains
an authorization decision (by asking the resource owner or by
establishing approval via other means).

Validate and then obtain the decision, not the other way around, right?

@jankapunkt
Copy link
Member

@saschanaz if you are still interested then please let's continue this in the examples repository. Closing this for now.

@sergeyshilin
Copy link

I'll describe the issue as I understood it. Let's see if this is what @saschanaz meant.

The flow right now:

app.use("/authorize", ensureLoggedIn(), oauth.authorize());

Need to prompt scope for user's verification before issuing an authorization code. My current options right now are:

app.use("/authorize", ensureLoggedIn(), promptScope(), oauth.authorize());

or

app.use("/authorize", ensureLoggedIn(), oauth.authorize(), promptScope());

Let's talk about each of the options.

First option

We prompt scope before authorizing the code request:

app.use("/authorize", ensureLoggedIn(), promptScope(), oauth.authorize());

In this case, we have user's session -- which is required before we prompt the user a dialogue to allow/reject the scope -- but we don't have the client yet, as it is only validated later, during authorize() call. Therefore, the user would have to approve the scope without knowing what client they are approving it for. Alternatively, a developer would need to copy-paste the client validation functionality into promptScope. Not the end of the world, but it causes code duplication.

Second option

We have user session, and we validate both the user and the client:

app.use("/authorize", ensureLoggedIn(), oauth.authorize(), promptScope());

In this case, the authorization code is already granted. Prompting the user at this point is possible, but a developer would need to remove the issued code from the data store in case of scope rejection by the user. Again, ok but not perfect.

What we want:

app.use("/authorize", ensureLoggedIn(), oauth.authorize());
                                                  ^ 
                                     scope validation prompt here

It should go in the place where both the user session and the client are validated, but the authoriztion code hasn't been issued yet -- and won't be if the user rejected the scope request prompt.

@jankapunkt I would be happy to learn that what I am trying to do is possible with the current server implementation, but seems like this hasn't been thought through. Any guidance is much appreciated.

@shrihari-prakash
Copy link
Contributor

shrihari-prakash commented Feb 5, 2024

Hello @sergeyshilin ,

The first option is actually a viable option. On the statement "but we don't have the client yet, as it is only validated later", I think the problem is more with using a single endpoint for prompting and authorize. On my implementations, I usually have separate APIs for these with a dedicated frontend. For instance, consider the following flow:

  1. User enters a login screen with a client_id in the URL params or even have a default client_id set in your frontend.
  2. They login and you decide to show the consent screen.
  3. On the consent screen, you hit a get-client API that will return a very minimal info about the client like scopes it requires, it's display name, etc.
  4. When user clicks on "Accept", you call the authorize API.

As you might have noticed, consent screen is a completely isolated from the core working of the library and is an implementation detail. The authorize API is called only after the user has consented.

Of course, this is just my implementation, but maybe @jankapunkt has a better way of doing this.

@sergeyshilin
Copy link

Thanks @shrihari-prakash, that makes sense to me! Although it feels a bit like a workaround due to client information being validated twice, it is still a working solution indeed.

@shrihari-prakash
Copy link
Contributor

shrihari-prakash commented Feb 5, 2024

Thanks @shrihari-prakash, that makes sense to me! Although it feels a bit like a workaround due to client information being validated twice, it is still a working solution indeed.

I see where you're coming from. I imagine you could also do away without using an API to fetch the client details as well. Except that you send scopes along in the login page and show that list on the consent screen as well. In any case if the scope is not valid, OAuth server takes care of rejecting the request in authorize call subsequently.

The main thing I was trying to convey is that anything that happens before authorize, say login or a consent screen is outside of the library's responsibilities. It's not something I would closely couple with the authorize API.

@sergeyshilin
Copy link

say login or a consent screen is outside of the library's responsibilities

I disagree with this statement. Approving/rejecting scopes is part of the OAuth2 specification, so it would make sense to have it supported within the library too.

https://oauth.net/2/scope/

An application can request one or more scopes, this information is then presented to the user in the consent screen, and the access token issued to the application will be limited to the scopes granted.

For example, as @saschanaz mentioned earlier, it is part of oauth2orize -- another node oauth server library -- by design: https://github.com/jaredhanson/oauth2orize#implement-authorization-endpoint

@jankapunkt
Copy link
Member

approving and rejecting scopes is partially implemented to the point where the standard demands it. Beyond that we allow users to have fine grained control over scopes using model implementation.

Everything beyond that, especially any involvement of UI layers is up to the developers.

Regarding allow/deny - I think we can talk about a better integration of passing the values on to the model but I personally don't see an implementation that might ease up the process a lot. What do you currently imagine for a helpful behavior?

@saschanaz
Copy link
Contributor Author

The main thing I was trying to convey is that anything that happens before authorize, say login or a consent screen is outside of the library's responsibilities. It's not something I would closely couple with the authorize API.

There is no point to show any screen to user before validation. Say, the server allows scope A and B and somehow the app requests A/B/C. My expectation is that the request fails without prompting anything to user, which is currently impossible without duplicating the validation steps. Perhaps the validation part should be split from authorize()?

@jankapunkt
Copy link
Member

@saschanaz what do you mean by "duplicating"? You mean because of verifyScope and validateScope?

@saschanaz
Copy link
Contributor Author

That and maybe all the checks here that should ideally happen before showing any screen:

async getClient (request) {
const self = this;
const clientId = request.body.client_id || request.query.client_id;
if (!clientId) {
throw new InvalidRequestError('Missing parameter: `client_id`');
}
if (!isFormat.vschar(clientId)) {
throw new InvalidRequestError('Invalid parameter: `client_id`');
}
const redirectUri = request.body.redirect_uri || request.query.redirect_uri;
if (redirectUri && !isFormat.uri(redirectUri)) {
throw new InvalidRequestError('Invalid request: `redirect_uri` is not a valid URI');
}
const client = await this.model.getClient(clientId, null);
if (!client) {
throw new InvalidClientError('Invalid client: client credentials are invalid');
}
if (!client.grants) {
throw new InvalidClientError('Invalid client: missing client `grants`');
}
if (!Array.isArray(client.grants) || !client.grants.includes('authorization_code')) {
throw new UnauthorizedClientError('Unauthorized client: `grant_type` is invalid');
}
if (!client.redirectUris || 0 === client.redirectUris.length) {
throw new InvalidClientError('Invalid client: missing client `redirectUri`');
}
if (redirectUri) {
const valid = await self.validateRedirectUri(redirectUri, client);
if (!valid) {
throw new InvalidClientError('Invalid client: `redirect_uri` does not match client value');
}
}
return client;
}

@jankapunkt jankapunkt reopened this Feb 5, 2024
@sergeyshilin
Copy link

@jankapunkt duplication of validation comes from validating twice both the scope (the requested scope is in the list of server's accepted scopes), and the client (the client_id a valid string, and the client itself exists in the DB).

Twice because the first time is for prompting the scope to a user, as per the picture

image

and second time when calling authorize().

Instead, an expected flow would be

  1. validate user
  2. validate client
  3. validate scope
  4. prompt scope dialogue to a user
  5. call authorize()

@Maia-Everett
Copy link

In my implementation of an OAuth server using node-oauth2-server, I did it this way:

  • The /oauth/authorize endpoint is implemented by the frontend, which takes the query parameters and calls an internal API via Ajax, which in turn calls authorize() to do all the validation before the consent screen is even shown.
  • The model's implementation of saveAuthorizationCode saves the generated authorization code in the database, marks the request as "needing user consent", and returns data needed to render the consent screen.
  • When the user clicks the approve button, the frontend calls another internal API endpoint to actually authorize the request. This endpoint loads the unconfirmed request, marks it as approved, and returns the redirect URI that authorize() would have returned to in the HTTP Location header.
  • The frontend redirects the browser using window.location.replace.

Rube Goldberg would be proud. Unfortunately, this is the most unopinionated maintained implementation of OAuth2 in Node that I could find, and the most well-documented one. oauth2orize expects to hook into Express and run before my own server logic, whereas I was looking for a library that I can call from my code, and trying to do something with its other than "hook it as Express middleware exactly as described" requires studying its code, which is headache-inducing callback hell.

Ideally the library should support passing custom data from the caller of authorize() to the model. Currently I have to tuck it into objects like user, which is hackish. For example, I want to record the IP from which the authorization request was made, but since the library doesn't give the model direct access to the request object, I have to pass it in the user object instead so saveAuthorizationCode can read it.

@jankapunkt
Copy link
Member

Thank you @Maia-Everett for the help and explanation.

If you think there are valid use-cases for passing custom data beyond the current structures then feel free to open this as an idea in the discussion section as we are always looking to improve the library, while keeping it as unopinionated as possible.

@shrihari-prakash
Copy link
Contributor

In my implementation of an OAuth server using node-oauth2-server, I did it this way:

  • The /oauth/authorize endpoint is implemented by the frontend, which takes the query parameters and calls an internal API via Ajax, which in turn calls authorize() to do all the validation before the consent screen is even shown.
  • The model's implementation of saveAuthorizationCode saves the generated authorization code in the database, marks the request as "needing user consent", and returns data needed to render the consent screen.
  • When the user clicks the approve button, the frontend calls another internal API endpoint to actually authorize the request. This endpoint loads the unconfirmed request, marks it as approved, and returns the redirect URI that authorize() would have returned to in the HTTP Location header.
  • The frontend redirects the browser using window.location.replace.

Rube Goldberg would be proud. Unfortunately, this is the most unopinionated maintained implementation of OAuth2 in Node that I could find, and the most well-documented one. oauth2orize expects to hook into Express and run before my own server logic, whereas I was looking for a library that I can call from my code, and trying to do something with its other than "hook it as Express middleware exactly as described" requires studying its code, which is headache-inducing callback hell.

Ideally the library should support passing custom data from the caller of authorize() to the model. Currently I have to tuck it into objects like user, which is hackish. For example, I want to record the IP from which the authorization request was made, but since the library doesn't give the model direct access to the request object, I have to pass it in the user object instead so saveAuthorizationCode can read it.

This is precisely what I am doing also. Honestly, this method does not make any assumptions about the UI / server setup in any way and that is why I like it.

@jankapunkt jankapunkt added the discussion 🗨️ Discussion about a particular topic. label Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion 🗨️ Discussion about a particular topic.
Projects
None yet
Development

No branches or pull requests

6 participants