axa-group / oauth2-mock-server

A development and test oriented OAuth2 mock server
MIT License
173 stars 53 forks source link

why scope set to dummy for authorization_code and refresh_token #259

Open gravypower opened 1 year ago

gravypower commented 1 year ago

Summary

I am wondering why scope is set to dummy for authorization_code and refresh_token

https://github.com/axa-group/oauth2-mock-server/blob/752f93abf1bdf0828ab551fd57c44f124815957c/src/lib/oauth2-service.ts#L211

should this also just repeat back the requests scopes like client_credentials does?

nulltoken commented 12 months ago

@gravypower :wave: Sorry for the delay in getting back to you.

Could you please share a bit of context with regards to your question? Are you facing an issue or a possibly unexpected behavior?

gravypower commented 11 months ago

thanks for responding @nulltoken. I am wanting to generate a JWT with scopes using the auth code flow. I expected that this library would just echo the scopes back like what happens with the client id and audience but this is not the case. I am working on the logic that applies authorisation in an app but was not able to do this with out forking the code base and removing where the scope is set to dummy.

I think doing this was correct but wanted to understand why dummy was set in the first place.

nulltoken commented 11 months ago

@gravypower Hmmm. I'm a little bit puzzled.

The code below defines an xfn transformer that will set the sub and amr and copy along the scope properties in the token payload to be generated.

https://github.com/axa-group/oauth2-mock-server/blob/752f93abf1bdf0828ab551fd57c44f124815957c/src/lib/oauth2-service.ts#L212-L218

This transformer is later passed to buildToken

https://github.com/axa-group/oauth2-mock-server/blob/752f93abf1bdf0828ab551fd57c44f124815957c/src/lib/oauth2-service.ts#L236

This test showcases how an passed in scope is retrieved in the decoded token payload

https://github.com/axa-group/oauth2-mock-server/blob/master/test/oauth2-service.test.ts#L543-L573

In the end, I'm not really sure what doesn't work as you would expect and why you had to fork the code base.

Could you please help me understand better the problem you're facing? (Or if it's quicker for you, a failing unit test showing what should work and doesn't)

gravypower commented 11 months ago

I have altered that test you linked me to show the issue, see how when the grant type is authorization_code the scope is always dummy.

I think the issue is here: https://github.com/axa-group/oauth2-mock-server/blob/752f93abf1bdf0828ab551fd57c44f124815957c/src/lib/oauth2-service.ts#L211

it('should allow customizing the token response through a beforeTokenSigning event authorization_code', async () => {
    service.once('beforeTokenSigning', (token, req) => {
      expect(req).toBeInstanceOf(IncomingMessage);
      token.payload.custom_header = req.headers['custom-header'];
      token.payload.iss = "https://tada.com";
    });

    const res = await tokenRequest(service.requestHandler)
      .set('Custom-Header', 'custom-token-value')
      .send({
        grant_type: 'authorization_code',
        scope: 'a-test-scope',
      })
      .expect(200);

    const key = service.issuer.keys.get('test-rs256-key');
    expect(key).not.toBeNull();

    expect(res.body).toMatchObject({
      access_token: expect.any(String),
    });
    const resBody = res.body as { access_token: string };

    const decoded = await verifyTokenWithKey(service.issuer, resBody.access_token, 'test-rs256-key');

    expect(decoded.payload).toMatchObject({
      iss: "https://tada.com",
      scope: 'a-test-scope',
      custom_header: 'custom-token-value',
    });
  });
gravypower commented 11 months ago

I am happy to raise an PR to address this, just wanted to know if this was done for a reason?

nulltoken commented 11 months ago

just wanted to know if this was done for a reason?

paging @poveden that may have a better view on this

poveden commented 10 months ago

Hi @gravypower this was introduced about 4 years ago. I think (if my memory serves me well) that it was added because some client library complained about them missing. Again, this was 4 years ago, so probably this is no longer the case today.

So, yes, please, raise a PR. Thanks!

PetrasJaug commented 2 months ago

We would like to start using merged changes. What is the release process?