Feature/configurable userid claim minimal #499
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fix oauth2-proxy#431 - This is a minimal change to allow the user to configure which claim is the source of the "user ID". - Add the option `user-id-claim` (defaults to email) - OIDC extracts this claim into session.Email (to be renamed later) - providers: add `CreateSessionStateFromBearerToken` with a default impl taken from `GetJwtSession` and overridden by oidc to respect `user-id-claim` Once oauth2-proxy#466 is merged, I can continue to rename SessionState.Email to .UserID and add HTTP headers with a corresponding name.
Co-Authored-By: Joel Speed <Joel.speed@hotmail.co.uk>
Instead, parse them twice - it might be sligtly slower but less bug-prone as the code evolves.
holyjak
requested review from
JoelSpeed,
steakunderscore and
loshz
as code owners
holyjak
force-pushed
the
feature/configurable-userid-claim-minimal
branch
from
037818d
to
ae04c95
Compare
4 tasks
holyjak
force-pushed
the
feature/configurable-userid-claim-minimal
branch
from
ae04c95
to
a6ab338
Compare
JoelSpeed
reviewed
Apr 25, 2020
Comment on lines
+154
to
+162
| var newSession *sessions.SessionState | ||
|
|
||
| if idToken != nil { | ||
| claims, err := findClaimsFromIDToken(idToken, token.AccessToken, p.ProfileURL.String()) | ||
| if idToken == nil { | ||
| newSession = &sessions.SessionState{} | ||
| } else { | ||
| var err error | ||
| newSession, err = p.createSessionStateInternal(token.Extra("id_token").(string), idToken, token) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("couldn't extract claims from id_token (%e)", err) | ||
| } | ||
|
|
||
| if claims != nil { | ||
|
|
||
| if !p.AllowUnverifiedEmail && claims.Verified != nil && !*claims.Verified { | ||
| return nil, fmt.Errorf("email in id_token (%s) isn't verified", claims.Email) | ||
| } | ||
|
|
||
| newSession.IDToken = token.Extra("id_token").(string) | ||
| newSession.Email = claims.Email | ||
| newSession.User = claims.Subject | ||
| newSession.PreferredUsername = claims.PreferredUsername | ||
| return nil, err |
There was a problem hiding this comment.
I don't think this if else block adds anything, the same logic is capture in createSessionStateInternal right, could potentially clean this up
steakunderscore
approved these changes
Apr 28, 2020
This was referenced May 17, 2020
NickMeves
pushed a commit
to grnhse/oauth2-proxy
that referenced
this pull request
Jul 27, 2020
This reverts to functionality before oauth2-proxy#499 where an OIDC provider could be used with `--skip-jwt-bearer-tokens` and tokens without an email or profileURL would still be valid. This logic mirrors `middleware.createSessionStateFromBearerToken` which used to be the universal logic before oauth2-proxy#499.
3 tasks
NickMeves
pushed a commit
to grnhse/oauth2-proxy
that referenced
this pull request
Jul 27, 2020
This reverts to functionality before oauth2-proxy#499 where an OIDC provider could be used with `--skip-jwt-bearer-tokens` and tokens without an email or profileURL would still be valid. This logic mirrors `middleware.createSessionStateFromBearerToken` which used to be the universal logic before oauth2-proxy#499.
NickMeves
pushed a commit
to grnhse/oauth2-proxy
that referenced
this pull request
Aug 10, 2020
This reverts to functionality before oauth2-proxy#499 where an OIDC provider could be used with `--skip-jwt-bearer-tokens` and tokens without an email or profileURL would still be valid. This logic mirrors `middleware.createSessionStateFromBearerToken` which used to be the universal logic before oauth2-proxy#499.
NickMeves
pushed a commit
to grnhse/oauth2-proxy
that referenced
this pull request
Aug 10, 2020
This reverts to functionality before oauth2-proxy#499 where an OIDC provider could be used with `--skip-jwt-bearer-tokens` and tokens without an email or profileURL would still be valid. This logic mirrors `middleware.createSessionStateFromBearerToken` which used to be the universal logic before oauth2-proxy#499.
NickMeves
pushed a commit
to grnhse/oauth2-proxy
that referenced
this pull request
Aug 11, 2020
This reverts to functionality before oauth2-proxy#499 where an OIDC provider could be used with `--skip-jwt-bearer-tokens` and tokens without an email or profileURL would still be valid. This logic mirrors `middleware.createSessionStateFromBearerToken` which used to be the universal logic before oauth2-proxy#499.
NickMeves
pushed a commit
to grnhse/oauth2-proxy
that referenced
this pull request
Aug 14, 2020
This reverts to functionality before oauth2-proxy#499 where an OIDC provider could be used with `--skip-jwt-bearer-tokens` and tokens without an email or profileURL would still be valid. This logic mirrors `middleware.createSessionStateFromBearerToken` which used to be the universal logic before oauth2-proxy#499.
3 tasks
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replace #469 with a PR from a personal repo so that maintainers can fix merge issues
Fix #431 - This is a minimal change to allow the user to configure which claim is
the source of the "user ID".
It is kept minimal to make to minimize conflicts with other ongoing changes.
Description
user-id-claim(defaults to email)CreateSessionStateFromBearerTokenwith a default impl taken fromGetJwtSessionand overridden by oidc to respectuser-id-claimOnce #466 is merged, I can continue to port other work from #448, namely to rename SessionState.Email to .UserID, adjust (de)serialization accordingly, add HTTP headers with a corresponding name.
Motivation and Context
Fix #431
How Has This Been Tested?
Tested on OS X:
-user-id-claimand a user with an email, verify logged in-user-id-claim phone_numberand-user-id-claim sub, verify it works-user-id-claimand verify it fails-skip-jwt-bearer-tokens true -user-id-claim phone_numberand verify that requests that includeAuthorization: <the auth. header forwarded by the proxy to the upstream>go directly to the upstreamPS: I'd love to add a unit test for
CreateSessionStateFromBearerTokenbut couldn't figure out a reasonable way to create anoidc.IDToken- any tips?Checklist: