-
-
Notifications
You must be signed in to change notification settings - Fork 358
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
feat: Allow extra fields in introspect response #520
Conversation
3cefbfb
to
c2c7ba6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I've added a few comments with ideas how to improve the patch :)
@aeneasr I think I addressed all your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the long wait time, but it is finally possible to run oidc conformity tests in fosite which I wanted to address before continuing review here.
I added a few more comments which I think need to be addressed before merge!
@@ -55,7 +55,6 @@ var jwtValidCase = func(tokenType fosite.TokenType) *fosite.Request { | |||
JWTClaims: &jwt.JWTClaims{ | |||
Issuer: "fosite", | |||
Subject: "peter", | |||
Audience: []string{"group0"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this still work though? If so can we add a test to cover this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it cannot work because we are filtering out aud
from GetExtraClaims
. Before the test was in fact buggy, I would claim, and didn't really check if audience is set in the generated (and tested) token. So setting this value does nothing.
for name, value := range extraClaims { | ||
switch name { | ||
// We do not allow these to be set through extra claims. | ||
case "exp", "client_id", "scope", "iat", "sub", "aud", "username": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Must include iss
, jti
, nbf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not really possible to limit. Because currently regular claims from JWT tokens are passed through to introspection endpoint using GetExtraClaims
. I tried to not do it like that, but I get import cycle. See: 04cd625
I made a followup PR: #579 |
Related issue
Fixes #441.
Proposed changes
Sessions can now implement
GetExtraClaims
to control extra claims in token's introspect output.Checklist
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
Further comments
I am not completely satisfied with tests here. For example, there is no test which would check which all fields are really output from introspect for regular access token and for JWT access token. Any suggestion where to add it is welcome.