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
Refactor redeemCode and support a Provider-wide EnrichSessionState method #796
Conversation
3919d0e
to
ea8f57e
Compare
b := testGitHubBackend(map[string][]string{ | ||
"/user/emails": {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`}, | ||
"/user/emails": {`[ {"email": "michael.bland@gsa.gov", "verified": false, "primary": true} ]`}, |
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.
I'm assuming the test wanted to test NotVerified
like the name implies, hence the change in test behavior.
assert.NotEqual(t, nil, err) | ||
assert.Equal(t, "", email) | ||
err := p.getEmail(context.Background(), session) | ||
assert.NoError(t, err) |
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 test was broken before -- The error catch was catching an error in the testGitHubBackend
by having an empty []string{}
for /repo/oauth2-proxy/oauth2-proxy
.
I disagree with the underlying behavior of hasRepo
-- getEmail
raises an error and doesn't set an Email on an error in this method or a false return. In the false
case, the email that used to be returned was ""
and nil
error was raised.
This refactor now at least makes it so ""
won't be returned and clobber a session.Email
that might already be set. But I wonder if it is best to return an actual error? (I imagine this worked in the past to restrict access accidentally by setting the session.Email
to "" which failed the email domain validation.
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.
To clarify, if hasRepo
fails, ie the user has failed the authorization check, what happens presently?
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.
oauth2-proxy/providers/github.go
Line 402 in 8be97f2
if ok, err := p.hasRepo(ctx, s.AccessToken); err != nil || !ok { |
It looks like it will return "", err
is either hasRepo
errors or returns false
. This is fine at the moment, but not great -- and got accidental security from from our email validator not allowing empty emails. But that will change with this PR: #770
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.
Even though it goes beyond a refactor, I'm thinking we need to make hasRepo
returning false return an error so the security logic is completely contained here appropriately.
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 pattern is all over the place in this provider:
err != nil || !ok {
return err
}
A lot of nil
errors are potentially returned in auth codepaths. Punting for now, but this will need to be refactored later once we have the better Authorization
scheme.
The I'll wait until a later PR to take on that refactoring. |
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.
Looks good, left a couple of comments for you when you're back from vacation!
func (p *GitLabProvider) verifyEmailDomain(userInfo *gitlabUserInfo) error { | ||
if len(p.EmailDomains) == 0 || p.EmailDomains[0] == "*" { | ||
return nil | ||
} | ||
|
||
for _, domain := range p.EmailDomains { | ||
if strings.HasSuffix(userInfo.Email, domain) { | ||
return nil | ||
} | ||
} | ||
|
||
return fmt.Errorf("user email is not one of the valid domains '%v'", p.EmailDomains) | ||
} | ||
|
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.
Is this covered by the main email verification?
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.
Yup - this code seemed completely redundant to our email validator check that happens right after the redeemCode
section that used to perform this check.
func (p *ProviderData) GetUserName(ctx context.Context, s *sessions.SessionState) (string, error) { | ||
return "", errors.New("not implemented") | ||
// DEPRECATED: Migrate to EnrichSessionState | ||
func (p *ProviderData) GetEmailAddress(_ context.Context, _ *sessions.SessionState) (string, error) { |
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.
Does this PR remove all use of GetEmailAddress
? Do we need to keep this in?
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.
Still a bunch of GetEmailAddress
uses unfortunately. There were only 2 GetUserName
uses, so I refactored those now while still keeping the PR small.
My goal is to go GetEmailAddress
over time and have the few authorization related PRs for Keycloak and Gitlab coming in clean this up as a start.
assert.NotEqual(t, nil, err) | ||
assert.Equal(t, "", email) | ||
err := p.getEmail(context.Background(), session) | ||
assert.NoError(t, err) |
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.
To clarify, if hasRepo
fails, ie the user has failed the authorization check, what happens presently?
c947c5e
to
38501f6
Compare
38501f6
to
e9ab047
Compare
@JoelSpeed Bump on this one when you are available. I want to get this and #797 soon since we have a few Provider refactor PRs from the community for Keycloak & Gitlab waiting on these 2 to merge. |
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.
If we can get the changelog fixed I'm happy for this to merge
CHANGELOG.md
Outdated
@@ -26,9 +26,11 @@ | |||
## Changes since v6.1.1 | |||
|
|||
- [#825](https://github.com/oauth2-proxy/oauth2-proxy/pull/825) Fix code coverage reporting on GitHub actions(@JoelSpeed) | |||
- [#767](https://github.com/oauth2-proxy/oauth2-proxy/pull/796) Deprecate GetUserName & GetEmailAdress for EnrichSessionState (@NickMeves) |
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.
We have duplicate entries here by the looks of it
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.
Good catch - I even had the wrong issue # as the link text 😅
I rebased and fixed it
This is handled globally
e9ab047
to
4a54c94
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.
LGTM, thanks @NickMeves
Split out session enrichment from OAuth Redeem functionality in
redeemCode
Add EnrichSessionState provider method to unify use cases previous done with
GetUserName
&GetEmailAddress
. Begin refactoring migrating fewGetUserName
providers toEnrichSessionState
as a starting point.Motivation and Context
Codeclimate refactoring.
Simplify the provider interface and give an appropriate method to enrich sessions after Oauth2 redeem step. (A lot of providers seem to have hacked in AuthZ logic unrelated to emails into
GetEmailAddress
since it was the only viable hook).How Has This Been Tested?
New unit tests added.
GitHub provider test suite cleaned up to fix test bugs.
Checklist: