Skip to content

fix: add scopes to token hook on authorization_code #3970

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.

Sign up for GitHub

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

Draft
sousandrei wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

fix: add scopes to token hook on authorization_code #3970

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion oauth2/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1178,7 +1178,8 @@ func (h *Handler) oauth2TokenExchange(w http.ResponseWriter, r *http.Request) {

if accessRequest.GetGrantTypes().ExactOne(string(fosite.GrantTypeClientCredentials)) ||
accessRequest.GetGrantTypes().ExactOne(string(fosite.GrantTypeJWTBearer)) ||
accessRequest.GetGrantTypes().ExactOne(string(fosite.GrantTypePassword)) {
accessRequest.GetGrantTypes().ExactOne(string(fosite.GrantTypePassword)) ||
accessRequest.GetGrantTypes().ExactOne(string(fosite.GrantTypeAuthorizationCode)) {
Copy link

@tilgovi tilgovi Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@tilgovi tilgovi Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@tilgovi tilgovi Apr 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@tilgovi tilgovi Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@sousandrei sousandrei Apr 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@tilgovi tilgovi Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like the right move!

Copy link
Member

@aeneasr aeneasr Apr 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var accessTokenKeyID string
if h.c.AccessTokenStrategy(ctx, client.AccessTokenStrategySource(accessRequest.GetClient())) == "jwt" {
accessTokenKeyID, err = h.r.AccessTokenJWTStrategy().GetPublicKeyID(ctx)
Expand Down
Loading