Merge pull request #796 from grnhse/refactor-redeem-code

Refactor redeemCode and support a Provider-wide EnrichSessionState method

CHANGELOG.md

@@ -26,6 +26,7 @@
 ## Changes since v6.1.1
 
 - [#825](https://github.com/oauth2-proxy/oauth2-proxy/pull/825) Fix code coverage reporting on GitHub actions(@JoelSpeed)
+- [#796](https://github.com/oauth2-proxy/oauth2-proxy/pull/796) Deprecate GetUserName & GetEmailAdress for EnrichSessionState (@NickMeves)
 - [#705](https://github.com/oauth2-proxy/oauth2-proxy/pull/705) Add generic Header injectors for upstream request and response headers (@JoelSpeed)
 - [#753](https://github.com/oauth2-proxy/oauth2-proxy/pull/753) Pass resource parameter in login url (@codablock)
 - [#789](https://github.com/oauth2-proxy/oauth2-proxy/pull/789) Add `--skip-auth-route` configuration option for `METHOD=pathRegex` based allowlists (@NickMeves)

oauthproxy.go

@@ -357,22 +357,19 @@ func (p *OAuthProxy) redeemCode(ctx context.Context, host, code string) (*sessio
 	if err != nil {
 		return nil, err
 	}
+	return s, nil
+}
 
+func (p *OAuthProxy) enrichSessionState(ctx context.Context, s *sessionsapi.SessionState) error {
+	var err error
 	if s.Email == "" {
 		s.Email, err = p.provider.GetEmailAddress(ctx, s)
-		if err != nil && err.Error() != "not implemented" {
-			return nil, err
-		}
-	}
-
-	if s.User == "" {
-		s.User, err = p.provider.GetUserName(ctx, s)
-		if err != nil && err.Error() != "not implemented" {
-			return nil, err
+		if err != nil && !errors.Is(err, providers.ErrNotImplemented) {
+			return err
 		}
 	}
 
-	return s, nil
+	return p.provider.EnrichSessionState(ctx, s)
 }
 
 // MakeCSRFCookie creates a cookie for CSRF
@@ -829,14 +826,21 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {
 		return
 	}
 
-	s := strings.SplitN(req.Form.Get("state"), ":", 2)
-	if len(s) != 2 {
+	err = p.enrichSessionState(req.Context(), session)
+	if err != nil {
+		logger.Errorf("Error creating session during OAuth2 callback: %v", err)
+		p.ErrorPage(rw, http.StatusInternalServerError, "Internal Server Error", "Internal Error")
+		return
+	}
+
+	state := strings.SplitN(req.Form.Get("state"), ":", 2)
+	if len(state) != 2 {
 		logger.Error("Error while parsing OAuth2 state: invalid length")
 		p.ErrorPage(rw, http.StatusInternalServerError, "Internal Server Error", "Invalid State")
 		return
 	}
-	nonce := s[0]
-	redirect := s[1]
+	nonce := state[0]
+	redirect := state[1]
 	c, err := req.Cookie(p.CSRFCookieName)
 	if err != nil {
 		logger.PrintAuthf(session.Email, req, logger.AuthFailure, "Invalid authentication via OAuth2: unable to obtain CSRF cookie")

oauthproxy_test.go

@@ -396,14 +396,86 @@ func NewTestProvider(providerURL *url.URL, emailAddress string) *TestProvider {
 	}
 }
 
-func (tp *TestProvider) GetEmailAddress(ctx context.Context, session *sessions.SessionState) (string, error) {
+func (tp *TestProvider) GetEmailAddress(_ context.Context, _ *sessions.SessionState) (string, error) {
 	return tp.EmailAddress, nil
 }
 
-func (tp *TestProvider) ValidateSessionState(ctx context.Context, session *sessions.SessionState) bool {
+func (tp *TestProvider) ValidateSessionState(_ context.Context, _ *sessions.SessionState) bool {
 	return tp.ValidToken
 }
 
+func Test_redeemCode(t *testing.T) {
+	opts := baseTestOptions()
+	err := validation.Validate(opts)
+	assert.NoError(t, err)
+
+	proxy, err := NewOAuthProxy(opts, func(string) bool { return true })
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	_, err = proxy.redeemCode(context.Background(), "www.example.com", "")
+	assert.Error(t, err)
+}
+
+func Test_enrichSession(t *testing.T) {
+	const (
+		sessionUser   = "Mr Session"
+		sessionEmail  = "session@example.com"
+		providerEmail = "provider@example.com"
+	)
+
+	testCases := map[string]struct {
+		session       *sessions.SessionState
+		expectedUser  string
+		expectedEmail string
+	}{
+		"Session already has enrichable fields": {
+			session: &sessions.SessionState{
+				User:  sessionUser,
+				Email: sessionEmail,
+			},
+			expectedUser:  sessionUser,
+			expectedEmail: sessionEmail,
+		},
+		"Session is missing Email and GetEmailAddress is implemented": {
+			session: &sessions.SessionState{
+				User: sessionUser,
+			},
+			expectedUser:  sessionUser,
+			expectedEmail: providerEmail,
+		},
+		"Session is missing User and GetUserName is not implemented": {
+			session: &sessions.SessionState{
+				Email: sessionEmail,
+			},
+			expectedUser:  "",
+			expectedEmail: sessionEmail,
+		},
+	}
+
+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {
+			opts := baseTestOptions()
+			err := validation.Validate(opts)
+			assert.NoError(t, err)
+
+			// intentionally set after validation.Validate(opts) since it will clobber
+			// our TestProvider and call `providers.New` defaulting to `providers.GoogleProvider`
+			opts.SetProvider(NewTestProvider(&url.URL{Host: "www.example.com"}, providerEmail))
+			proxy, err := NewOAuthProxy(opts, func(string) bool { return true })
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			err = proxy.enrichSessionState(context.Background(), tc.session)
+			assert.NoError(t, err)
+			assert.Equal(t, tc.expectedUser, tc.session.User)
+			assert.Equal(t, tc.expectedEmail, tc.session.Email)
+		})
+	}
+}
+
 func TestBasicAuthPassword(t *testing.T) {
 	providerServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		logger.Printf("%#v", r)
@@ -1883,7 +1955,7 @@ func TestClearSingleCookie(t *testing.T) {
 type NoOpKeySet struct {
 }
 
-func (NoOpKeySet) VerifySignature(ctx context.Context, jwt string) (payload []byte, err error) {
+func (NoOpKeySet) VerifySignature(_ context.Context, jwt string) (payload []byte, err error) {
 	splitStrings := strings.Split(jwt, ".")
 	payloadString := splitStrings[1]
 	return base64.RawURLEncoding.DecodeString(payloadString)

pkg/validation/options.go

@@ -273,7 +273,6 @@ func parseProviderInfo(o *options.Options, msgs []string) []string {
 	case *providers.GitLabProvider:
 		p.AllowUnverifiedEmail = o.InsecureOIDCAllowUnverifiedEmail
 		p.Groups = o.GitLabGroup
-		p.EmailDomains = o.EmailDomains
 
 		if o.GetOIDCVerifier() != nil {
 			p.Verifier = o.GetOIDCVerifier()

providers/github.go

@@ -102,6 +102,20 @@ func (p *GitHubProvider) SetUsers(users []string) {
 	p.Users = users
 }
 
+// EnrichSessionState updates the User & Email after the initial Redeem
+func (p *GitHubProvider) EnrichSessionState(ctx context.Context, s *sessions.SessionState) error {
+	err := p.getEmail(ctx, s)
+	if err != nil {
+		return err
+	}
+	return p.getUser(ctx, s)
+}
+
+// ValidateSessionState validates the AccessToken
+func (p *GitHubProvider) ValidateSessionState(ctx context.Context, s *sessions.SessionState) bool {
+	return validateToken(ctx, p, s.AccessToken, makeGitHubHeader(s.AccessToken))
+}
+
 func (p *GitHubProvider) hasOrg(ctx context.Context, accessToken string) (bool, error) {
 	// https://developer.github.com/v3/orgs/#list-your-organizations
 
@@ -364,8 +378,8 @@ func (p *GitHubProvider) isCollaborator(ctx context.Context, username, accessTok
 	return true, nil
 }
 
-// GetEmailAddress returns the Account email address
-func (p *GitHubProvider) GetEmailAddress(ctx context.Context, s *sessions.SessionState) (string, error) {
+// getEmail updates the SessionState Email
+func (p *GitHubProvider) getEmail(ctx context.Context, s *sessions.SessionState) error {
 
 	var emails []struct {
 		Email    string `json:"email"`
@@ -379,28 +393,28 @@ func (p *GitHubProvider) GetEmailAddress(ctx context.Context, s *sessions.Sessio
 		var err error
 		verifiedUser, err = p.hasUser(ctx, s.AccessToken)
 		if err != nil {
-			return "", err
+			return err
 		}
 		// org and repository options are not configured
 		if !verifiedUser && p.Org == "" && p.Repo == "" {
-			return "", errors.New("missing github user")
+			return errors.New("missing github user")
 		}
 	}
 	// If a user is verified by username options, skip the following restrictions
 	if !verifiedUser {
 		if p.Org != "" {
 			if p.Team != "" {
 				if ok, err := p.hasOrgAndTeam(ctx, s.AccessToken); err != nil || !ok {
-					return "", err
+					return err
 				}
 			} else {
 				if ok, err := p.hasOrg(ctx, s.AccessToken); err != nil || !ok {
-					return "", err
+					return err
 				}
 			}
 		} else if p.Repo != "" && p.Token == "" { // If we have a token we'll do the collaborator check in GetUserName
 			if ok, err := p.hasRepo(ctx, s.AccessToken); err != nil || !ok {
-				return "", err
+				return err
 			}
 		}
 	}
@@ -416,24 +430,23 @@ func (p *GitHubProvider) GetEmailAddress(ctx context.Context, s *sessions.Sessio
 		Do().
 		UnmarshalInto(&emails)
 	if err != nil {
-		return "", err
+		return err
 	}
 
-	returnEmail := ""
 	for _, email := range emails {
 		if email.Verified {
-			returnEmail = email.Email
 			if email.Primary {
-				return returnEmail, nil
+				s.Email = email.Email
+				return nil
 			}
 		}
 	}
 
-	return returnEmail, nil
+	return nil
 }
 
-// GetUserName returns the Account user name
-func (p *GitHubProvider) GetUserName(ctx context.Context, s *sessions.SessionState) (string, error) {
+// getUser updates the SessionState User
+func (p *GitHubProvider) getUser(ctx context.Context, s *sessions.SessionState) error {
 	var user struct {
 		Login string `json:"login"`
 		Email string `json:"email"`
@@ -451,22 +464,18 @@ func (p *GitHubProvider) GetUserName(ctx context.Context, s *sessions.SessionSta
 		Do().
 		UnmarshalInto(&user)
 	if err != nil {
-		return "", err
+		return err
 	}
 
 	// Now that we have the username we can check collaborator status
 	if !p.isVerifiedUser(user.Login) && p.Org == "" && p.Repo != "" && p.Token != "" {
 		if ok, err := p.isCollaborator(ctx, user.Login, p.Token); err != nil || !ok {
-			return "", err
+			return err
 		}
 	}
 
-	return user.Login, nil
-}
-
-// ValidateSessionState validates the AccessToken
-func (p *GitHubProvider) ValidateSessionState(ctx context.Context, s *sessions.SessionState) bool {
-	return validateToken(ctx, p, s.AccessToken, makeGitHubHeader(s.AccessToken))
+	s.User = user.Login
+	return nil
 }
 
 // isVerifiedUser