-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add preferred_username support (OIDC provider) #420
Add preferred_username support (OIDC provider) #420
Conversation
@@ -350,6 +351,10 @@ func (p *OAuthProxy) redeemCode(host, code string) (s *sessionsapi.SessionState, | |||
s.Email, err = p.provider.GetEmailAddress(s) | |||
} | |||
|
|||
if s.PreferredUsername == "" { | |||
s.PreferredUsername, err = p.provider.GetPreferredUsername(s) |
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 error is currently unchecked, and, for most providers, will always error, can we add a check on this error similar to the one below for s.User
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.
Sure! I've added it in ae34f54.
ae34f54
to
9545890
Compare
(Rebased to resolve conflicts.) |
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.
lgtm, thanks @ffdybuster
@JoelSpeed thanks a lot for reviewing and merging! :) |
Description
This PR adds a new claim,
preferred_username
. I added code for the OIDC provider to support it and tested it with dex (dexidp/dex#1566).Motivation and Context
Since #282 doesn't look like it will get done anytime soon, and since I really need
preferred_username
, I created this PR to add this specific claim. I don't really know the other connectors next to OIDC (and can't test them), so I only added support in there. I don't think the PR will break any of the other providers (the claim is returned empty for them and thus the headers will not be added).How Has This Been Tested?
I've build the docker container and ran it in our system for almost a day without problems. Our system uses dex via OIDC provider, and is used from Nginx via
auth_request
.Checklist: