fix: add scopes to token hook on authorization_code

[sousandrei]

Related issue(s)

#3969 #3891 #3969
Adding authorization_code to conditional of adding scopes to the requests for the token hook

Checklist

• I have read the contributing guidelines.
• I have referenced an issue containing the design document if my change
  introduces a new feature.
• I am following the
  contributing code guidelines.
• I have read the security policy.
• I confirm that this pull request does not address a security
  vulnerability. If this pull request addresses a security vulnerability, I
  confirm that I got the approval (please contact
  [email protected]) from the maintainers to push
  the changes.
• I have added tests that prove my fix is effective or that my feature
  works.
• I have added or changed the documentation.

Further Comments

[CLAassistant]

All committers have signed the CLA.

[tilgovi]

I think the reason that authorization code is left out of here is that a lot of this information is already determined when handling the authorization endpoint. I think the token endpoint should not be receiving the scope and audience parameters in the authorization code flow.

Line 1306 calls Handler.updateSessionWithRequest which should grant the scopes. If the scopes aren't available in the token hook, I think something else must be going on.

[tilgovi]

And I hope the drive-by is welcome! I'm not a maintainer here or anything, just someone who saw your note about this PR on Slack and was curious and has spent some time recently staring at some of this code.

[tilgovi]

I think what may be happening here is that fosite is supposed to handle this but it's called in two phases. First it's called to process the token request, then it's called to issue the token response. I think the token hook might be getting called in between these two phases. Moving some code between the two phases in fosite fixes the issue, I think.

I put up a draft fosite PR that seems to fix this for me, but haven't checked yet whether tests need to be updated or looked into whether there are other consequences to be aware of.

[tilgovi]

Just to explain that a little more clearly, maybe. The token endpoint handler does these things:

1. Calls fosite.OAuth2Provider.NewAccessRequest(), which calls the fosite code to handle the token request. This is just above your patch.
2. Runs the code around where you've patched here, to handle some things that fosite does not.
3. Calls all the hooks returned from Registry.AccessTokenHooks(), just below the code in your patch. That's where the token hook gets called.
4. Calls fosite.OAuth2Provider.NewAccessResponse(), which calls the fosite code to generate a response.

So, I think my patch in fosite is moving the code that reads the granted scope and audience, already set and persisted by the consent flow, and updates the token request. It moves this from step 4 to step 1 so that it's visible in the token hook in step 3.

[sousandrei]

I think you are on to something!
I'll keep this PR in draft until we have a resolution from your's as well if you don't mind

[tilgovi]

That sounds like the right move!

[aeneasr]

Hello, this definitely is not correct. This code part is only for machine interaction. User interaction has to be done differently.

[aeneasr]

Hello, this definitely is not correct. This code part is only for machine interaction. User interaction has to be done differently.