Do not infer username from email #146
Conversation
wonderhoss
force-pushed
the
no-infer-username
branch
from
1ac090a to
56da838
Compare
|
I've re-ran the CI jobs and they've successfully passed, so I'm happy with this if @JoelSpeed is. |
There was a problem hiding this comment.
Couple of bits inline, as discussed OOB yesterday, I'd also like a note added to the Changelog including description of why this is a breaking change. I guess we should add a Breaking Changes header to the Changelog section for the next release? Should we also think about a migration guide or just add migration instructions in the Breaking Changes header?
I believe the change now fixes the issue described
| @@ -148,7 +150,8 @@ func (p *GoogleProvider) Redeem(redirectURL, code string) (s *SessionState, err | |||
| IDToken: jsonResponse.IDToken, | |||
| ExpiresOn: time.Now().Add(time.Duration(jsonResponse.ExpiresIn) * time.Second).Truncate(time.Second), | |||
| RefreshToken: jsonResponse.RefreshToken, | |||
| Email: email, | |||
| Email: c.Email, | |||
| User: c.Subject, | |||
There was a problem hiding this comment.
generally for google the subject is meaningless garbage, and people expect to use their google email (which is verified), however given that both will go in the http headers that should be fine.
I think for this change a description is enough, but we can always add one later on in case anyone feels more information is required. |
Description
If the auth provider does not return a
usernameas part of the auth response, the proxy would infer a username by using the local part of the user's email (e.g.john.doeforjohn.doe@example.com).This change uses the entire email address instead to remove potential ambiguity between different users with the same email local part.
Note: This is a breaking change.
Motivation and Context
With the previous behaviour, the username stored in the session for users
john.doe@example.comandjohn.doe@server.invalidwould be identical even though both users are likely to be different individuals.How Has This Been Tested?
Test suites have been changed to reflect the new expected behaviour.
Checklist: