Skip to content

Fixed getUserFromClient not being awaited in client credentials grant. #218

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

Merged
merged 5 commits into from
Aug 15, 2023

Conversation

shrihari-prakash
Copy link
Contributor

Summary

getUserFromClient was not awaited in client credentials grant, this results in a promise being passed to the validateScope function in the model instead of the actual user.

Linked issue(s)

#217 217

Involved parts of the project

Client Credentials grant

Added tests?

NA

OAuth2 standard

Reproduction

@shrihari-prakash shrihari-prakash changed the title Release 5.0.0 Fixed getUserFromClient not being awaited in client credentials grant. Aug 15, 2023
@jankapunkt
Copy link
Member

Thanks a lot @shrihari-prakash !!!

Can you please update the following test in /test/integration/grant-types/client-credentials-grant-type_test.js at line 93:

it('should return a token', function() {
      const token = {};
      const model = {
        getUserFromClient: async function(client) {
          client.foo.should.equal('bar');
          return { id: '123'};
        },
        saveToken: async function(_token, client, user) {
          client.foo.should.equal('bar');
          user.id.should.equal('123');
          return token;
        },
        validateScope: function() { return 'foo'; }
      };
      const grantType = new ClientCredentialsGrantType({ accessTokenLifetime: 120, model: model });
      const request = new Request({ body: {}, headers: {}, method: {}, query: {} });

      return grantType.handle(request, { foo: 'bar' })
        .then(function(data) {
          data.should.equal(token);
        })
        .catch(should.fail);
    });

Please use lint:fix to format the code afterwards.

@jankapunkt jankapunkt linked an issue Aug 15, 2023 that may be closed by this pull request
@shrihari-prakash
Copy link
Contributor Author

@jankapunkt , lint:fix seems to be modifying every js file in the repository. Is this expected? I am not seeing any visible line changes, but possibly it is changing all crlf line endings to lf.

@shrihari-prakash
Copy link
Contributor Author

For now, I just added the test. Let me know if lint:fix is still required. Will remember to add respective tests in future :)

@jankapunkt
Copy link
Member

@shrihari-prakash no worries, it was just a hint, because sometimes pasting crashes the formatting. Looks good to me.

@shrihari-prakash
Copy link
Contributor Author

Integrations seem to be failing now...

@jankapunkt jankapunkt merged commit c299425 into node-oauth:release-5.0.0 Aug 15, 2023
@jankapunkt
Copy link
Member

@shrihari-prakash no worries this is a CI config issues, which is already fixed on the 5.0.0 branch, CI should complete there sucessdully

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.

Client Credentials broken in 5.0.0-rc.1
2 participants