Skip to content

Client Credentials broken in 5.0.0-rc.1 #217

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

Closed
shrihari-prakash opened this issue Aug 15, 2023 · 12 comments · Fixed by #218
Closed

Client Credentials broken in 5.0.0-rc.1 #217

shrihari-prakash opened this issue Aug 15, 2023 · 12 comments · Fixed by #218
Labels
bug 🐛 Something isn't working bug resolved 😃 Bug has been resolved and fixed.

Comments

@shrihari-prakash
Copy link
Contributor

shrihari-prakash commented Aug 15, 2023

Specify your setup

Describe the bug

A clear and concise description of what the bug is.

To Reproduce

Steps to reproduce the behavior:

  1. In the token endpoint, supply grant type as client_credentials.
  2. Pass the access token from the previous step to a route that uses OAuthServer.server.authenticate.
  3. Result will be invalid_token: Invalid token: access token is invalid.

Alternatively, please add a link to a GitHub repo
that reproduces the error/s.

It is possible to do a quick check in a dev deployment of my project: https://liquid-pe2r.onrender.com (It is very slow on the first request, so give it a minute to load, and then do the API call).

  1. Send request to https://liquid-pe2r.onrender.com/oauth/token
  2. Sample client-credentials: client_id: application_client, client_secret: super-secure-client-secret
  3. Scope: system.client.all
  4. Now try to access http://localhost:2000/system/client-api/stats (Which can be accessed only by clients).

You should see an unauthorized.

Expected behavior

The server should accept the valid token.

Screenshots

If applicable, add screenshots to help explain your problem.

Additional context

The problem seems to be due to a different token passed to getAccessToken function in the model than the one that was returned by authorize function..

@shrihari-prakash
Copy link
Contributor Author

There also seem to be some missing pieces in validateScope method call. The user object seems to be a promise symbol:
image
Maybe getUserFromClient was not awaited?

@jankapunkt
Copy link
Member

jankapunkt commented Aug 15, 2023

hey @shrihari-prakash can you help to trace this issue further down to some part of the actual code? It's hard to "debug" this with a black-box setup as I can't trace the internal code-flow (which is of course wanted with authz setup 💯 )

Either there is a part, that causes a wrong token to be created or a part where a correctly created token is unexpectedly compared to some wrong token or there is a structural mismatch, like comparing a string to an object.

From what I read there may be multiple places that can cause this:

  • TokenHandler
  • ClientCredentialsGrantType
  • Model
    • saveToken
    • getClient
    • getAccessToken
    • revokeAuthorizationCode

Can you run this with the debugger and or place some console traces to actually find at which place exactly the program causes this?

@jankapunkt
Copy link
Member

jankapunkt commented Aug 15, 2023

Addition: please also make sure there is no unintended multiple request causing the codes to be revoked (see https://datatracker.ietf.org/doc/html/rfc6749.html#section-4.1.2)

@shrihari-prakash
Copy link
Contributor Author

Hello @jankapunkt ,

Sure, let me trace it down. Probably sometime today. I wanted to register the issues first so I don't lose to report them. Maybe in future I will make a minimal example of this framework so that I can submit future issues with a good code sample, but for now if you would want to explore, the codebase of the hosted API is here: https://github.com/shrihari-prakash/liquid. (Don't worry, I will be parallelly working on finding the root cause)

As of making unintended multiple requests, this system has been tested for several scenarios with 4.x. So something new in 5.0.0 causing this.

@shrihari-prakash
Copy link
Contributor Author

@jankapunkt , getUserFromClient is not being awaited in client credentials grant. I opened a PR to await it. Can you review #218?

This will solve the scope issue.

@shrihari-prakash
Copy link
Contributor Author

@jankapunkt , this seems to have resolved the other issue also 🙂

@jankapunkt
Copy link
Member

jankapunkt commented Aug 15, 2023

@shrihari-prakash FYI - please note the following issue #219

@jankapunkt jankapunkt added bug 🐛 Something isn't working bug resolved 😃 Bug has been resolved and fixed. and removed investigating 🔍 labels Aug 15, 2023
@shrihari-prakash
Copy link
Contributor Author

@jankapunkt what is the plan to release RC2? This would help to test the new changes and identify any other regressions that might be in 5.X.

@jankapunkt
Copy link
Member

Yes, RC.2 is going public the next hour

@jankapunkt
Copy link
Member

https://github.com/node-oauth/node-oauth2-server/releases/tag/v5.0.0-rc.2

@shrihari-prakash
Copy link
Contributor Author

Just an update. It's been 5 days since the upgrade of rc2 and so far, there are no new regressions in my applications that use this library. Can confirm that client credentials, auth code, refresh token are all working as intended.👍🏻

@jankapunkt
Copy link
Member

Hi @shrihari-prakash thanks for the testing! I currently work on #224 to cover potential similar cases and I hope to push coverage close to 100% so we can mitigate these kinds of issues in the future :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working bug resolved 😃 Bug has been resolved and fixed.
Projects
None yet
2 participants