node-oauth / node-oauth2-server

🚀 The successor to oauthjs/oauth2-server. 🔒 Complete, compliant, maintained and well tested OAuth2 Server for node.js. Includes native async await and PKCE.
https://www.npmjs.com/package/@node-oauth/oauth2-server
MIT License
313 stars 46 forks source link

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

Closed shrihari-prakash closed 1 year ago

shrihari-prakash commented 1 year ago

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

jankapunkt commented 1 year ago

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.

shrihari-prakash commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

shrihari-prakash commented 1 year ago

Integrations seem to be failing now...

jankapunkt commented 1 year ago

@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