From f21b3b8b200bf972d920ccc75b298c91c8279944 Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Fri, 23 Oct 2020 20:53:38 -0700 Subject: [PATCH] Authorize in Redeem callback flow --- CHANGELOG.md | 2 +- oauthproxy.go | 8 ++++++-- providers/google.go | 28 ++++++++++++++++++---------- providers/google_test.go | 8 ++++---- 4 files changed, 29 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d89f2d92..36fae3e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ - [#905](https://github.com/oauth2-proxy/oauth2-proxy/pull/905) Existing sessions from v6.0.0 or earlier are no longer valid. They will trigger a reauthentication. - [#826](https://github.com/oauth2-proxy/oauth2-proxy/pull/826) `skip-auth-strip-headers` now applies to all requests, not just those where authentication would be skipped. - [#797](https://github.com/oauth2-proxy/oauth2-proxy/pull/797) The behavior of the Google provider Groups restriction changes with this - - Either `--google-group` or the new `--allowed-group` will work for Google now (`--google-group` will be used it both are set) + - Either `--google-group` or the new `--allowed-group` will work for Google now (`--google-group` will be used if both are set) - Group membership lists will be passed to the backend with the `X-Forwarded-Groups` header - If you change the list of allowed groups, existing sessions that now don't have a valid group will be logged out immediately. - Previously, group membership was only checked on session creation and refresh. diff --git a/oauthproxy.go b/oauthproxy.go index 98c82238..cb445a07 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -907,11 +907,15 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) { } // set cookie, or deny - if p.Validator(session.Email) { + authorized, err := p.provider.Authorize(req.Context(), session) + if err != nil { + logger.Errorf("Error with authorization: %v", err) + } + if p.Validator(session.Email) && authorized { logger.PrintAuthf(session.Email, req, logger.AuthSuccess, "Authenticated via OAuth2: %s", session) err := p.SaveSession(rw, req, session) if err != nil { - logger.Printf("Error saving session state for %s: %v", remoteAddr, err) + logger.Errorf("Error saving session state for %s: %v", remoteAddr, err) p.ErrorPage(rw, http.StatusInternalServerError, "Internal Server Error", err.Error()) return } diff --git a/providers/google.go b/providers/google.go index d355efca..3f643407 100644 --- a/providers/google.go +++ b/providers/google.go @@ -28,13 +28,13 @@ type GoogleProvider struct { RedeemRefreshURL *url.URL - // GroupValidator is a function that determines if the user in the passed + // groupValidator is a function that determines if the user in the passed // session is a member of any of the configured Google groups. // // This hits the Google API for each group, so it is called on Redeem & // Refresh. `Authorize` uses the results of this saved in `session.Groups` // Since it is called on every request. - GroupValidator func(*sessions.SessionState) bool + groupValidator func(*sessions.SessionState) bool } var _ Provider = (*GoogleProvider)(nil) @@ -90,9 +90,9 @@ func NewGoogleProvider(p *ProviderData) *GoogleProvider { }) return &GoogleProvider{ ProviderData: p, - // Set a default GroupValidator to just always return valid (true), it will + // Set a default groupValidator to just always return valid (true), it will // be overwritten if we configured a Google group restriction. - GroupValidator: func(*sessions.SessionState) bool { + groupValidator: func(*sessions.SessionState) bool { return true }, } @@ -165,7 +165,8 @@ func (p *GoogleProvider) Redeem(ctx context.Context, redirectURL, code string) ( created := time.Now() expires := time.Now().Add(time.Duration(jsonResponse.ExpiresIn) * time.Second).Truncate(time.Second) - s := &sessions.SessionState{ + + return &sessions.SessionState{ AccessToken: jsonResponse.AccessToken, IDToken: jsonResponse.IDToken, CreatedAt: &created, @@ -173,19 +174,26 @@ func (p *GoogleProvider) Redeem(ctx context.Context, redirectURL, code string) ( RefreshToken: jsonResponse.RefreshToken, Email: c.Email, User: c.Subject, - } - p.GroupValidator(s) + }, nil +} - return s, nil +// EnrichSessionState checks the listed Google Groups configured and adds any +// that the user is a member of to session.Groups. +func (p *GoogleProvider) EnrichSessionState(ctx context.Context, s *sessions.SessionState) error { + p.groupValidator(s) + + return nil } // SetGroupRestriction configures the GoogleProvider to restrict access to the // specified group(s). AdminEmail has to be an administrative email on the domain that is // checked. CredentialsFile is the path to a json file containing a Google service // account credentials. +// +// TODO (@NickMeves) - Unit Test this OR refactor away from groupValidator func func (p *GoogleProvider) SetGroupRestriction(groups []string, adminEmail string, credentialsReader io.Reader) { adminService := getAdminService(adminEmail, credentialsReader) - p.GroupValidator = func(s *sessions.SessionState) bool { + p.groupValidator = func(s *sessions.SessionState) bool { // Reset our saved Groups in case membership changed // This is used by `Authorize` on every request s.Groups = make([]string, 0, len(groups)) @@ -266,7 +274,7 @@ func (p *GoogleProvider) RefreshSessionIfNeeded(ctx context.Context, s *sessions } // re-check that the user is in the proper google group(s) - if !p.GroupValidator(s) { + if !p.groupValidator(s) { return false, fmt.Errorf("%s is no longer in the group(s)", s.Email) } diff --git a/providers/google_test.go b/providers/google_test.go index d2222790..458439d6 100644 --- a/providers/google_test.go +++ b/providers/google_test.go @@ -118,7 +118,7 @@ func TestGoogleProviderGroupValidator(t *testing.T) { validatorFunc func(*sessions.SessionState) bool expectedAuthZ bool }{ - "Email is authorized with GroupValidator": { + "Email is authorized with groupValidator": { session: &sessions.SessionState{ Email: sessionEmail, }, @@ -127,7 +127,7 @@ func TestGoogleProviderGroupValidator(t *testing.T) { }, expectedAuthZ: true, }, - "Email is denied with GroupValidator": { + "Email is denied with groupValidator": { session: &sessions.SessionState{ Email: sessionEmail, }, @@ -149,9 +149,9 @@ func TestGoogleProviderGroupValidator(t *testing.T) { g := NewWithT(t) p := newGoogleProvider() if tc.validatorFunc != nil { - p.GroupValidator = tc.validatorFunc + p.groupValidator = tc.validatorFunc } - g.Expect(p.GroupValidator(tc.session)).To(Equal(tc.expectedAuthZ)) + g.Expect(p.groupValidator(tc.session)).To(Equal(tc.expectedAuthZ)) }) } }