Authorize in Redeem callback flow

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.

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
 		}

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,
+	}, nil
 }
-	p.GroupValidator(s)
 
-	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)
 	}
 

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))
 		})
 	}
 }