Repository: oauth2-proxy/oauth2-proxy
Pull request #796 (2020-09-26 21:08:39)

Refactor redeemCode and support a Provider-wide EnrichSessionState method
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 few `GetUserName` providers to `EnrichSessionState` 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:

- [x] My change requires a change to the documentation or CHANGELOG.
- [x] I have updated the documentation/CHANGELOG accordingly.
- [x] I have created a feature (non-master) branch for my PR.


NickMeves: The `GetEmailAddress` method I renamed as `getEmail` is the source of the CodeClimate issues.  It is a giant monster and needs refactoring.  But it should wait until `Authorize` in #797 is merged -- alot of the bloat is from it doing all the authorization logic that didn't belong in `GetEmailAddress`.

I'll wait until a later PR to take on that refactoring.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index eaca5c86..cb496b3a 100644
--- a/CHANGELOG.md
+++ b/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)
diff --git a/oauthproxy.go b/oauthproxy.go
index e64ffe91..4fd03a40 100644
--- a/oauthproxy.go
+++ b/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")
diff --git a/pkg/validation/options.go b/pkg/validation/options.go
index 2ea35f9f..b54df330 100644
--- a/pkg/validation/options.go
+++ b/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()
diff --git a/providers/github.go b/providers/github.go
index 40d30799..d1be571f 100644
--- a/providers/github.go
+++ b/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,11 +393,11 @@ 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
@@ -391,16 +405,16 @@ func (p *GitHubProvider) GetEmailAddress(ctx context.Context, s *sessions.Sessio
 		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
diff --git a/providers/gitlab.go b/providers/gitlab.go
index ee15a486..a04beca6 100644
--- a/providers/gitlab.go
+++ b/providers/gitlab.go
@@ -3,7 +3,6 @@ package providers
 import (
 	"context"
 	"fmt"
-	"strings"
 	"time"
 
 	oidc "github.com/coreos/go-oidc"
@@ -16,9 +15,7 @@ import (
 type GitLabProvider struct {
 	*ProviderData
 
-	Groups       []string
-	EmailDomains []string
-
+	Groups               []string
 	Verifier             *oidc.IDTokenVerifier
 	AllowUnverifiedEmail bool
 }
@@ -168,20 +165,6 @@ func (p *GitLabProvider) verifyGroupMembership(userInfo *gitlabUserInfo) error {
 	return fmt.Errorf("user is not a member of '%s'", p.Groups)
 }
 
-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)
-}
-
 func (p *GitLabProvider) createSessionState(ctx context.Context, token *oauth2.Token) (*sessions.SessionState, error) {
 	rawIDToken, ok := token.Extra("id_token").(string)
 	if !ok {
@@ -211,39 +194,27 @@ func (p *GitLabProvider) ValidateSessionState(ctx context.Context, s *sessions.S
 }
 
 // GetEmailAddress returns the Account email address
-func (p *GitLabProvider) GetEmailAddress(ctx context.Context, s *sessions.SessionState) (string, error) {
+func (p *GitLabProvider) EnrichSessionState(ctx context.Context, s *sessions.SessionState) error {
 	// Retrieve user info
 	userInfo, err := p.getUserInfo(ctx, s)
 	if err != nil {
-		return "", fmt.Errorf("failed to retrieve user info: %v", err)
+		return fmt.Errorf("failed to retrieve user info: %v", err)
 	}
 
 	// Check if email is verified
 	if !p.AllowUnverifiedEmail && !userInfo.EmailVerified {
-		return "", fmt.Errorf("user email is not verified")
-	}
-
-	// Check if email has valid domain
-	err = p.verifyEmailDomain(userInfo)
-	if err != nil {
-		return "", fmt.Errorf("email domain check failed: %v", err)
+		return fmt.Errorf("user email is not verified")
 	}
 
 	// Check group membership
+	// TODO (@NickMeves) - Refactor to Authorize
 	err = p.verifyGroupMembership(userInfo)
 	if err != nil {
-		return "", fmt.Errorf("group membership check failed: %v", err)
+		return fmt.Errorf("group membership check failed: %v", err)
 	}
 
-	return userInfo.Email, nil
-}
-
-// GetUserName returns the Account user name
-func (p *GitLabProvider) GetUserName(ctx context.Context, s *sessions.SessionState) (string, error) {
-	userInfo, err := p.getUserInfo(ctx, s)
-	if err != nil {
-		return "", fmt.Errorf("failed to retrieve user info: %v", err)
-	}
+	s.User = userInfo.Username
+	s.Email = userInfo.Email
 
-	return userInfo.Username, nil
+	return nil
 }
diff --git a/providers/provider_default.go b/providers/provider_default.go
index 8a5a98ac..479e52c0 100644
--- a/providers/provider_default.go
+++ b/providers/provider_default.go
@@ -14,7 +14,13 @@ import (
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests"
 )
 
-var _ Provider = (*ProviderData)(nil)
+var (
+	// ErrNotImplemented is returned when a provider did not override a default
+	// implementation method that doesn't have sensible defaults
+	ErrNotImplemented = errors.New("not implemented")
+
+	_ Provider = (*ProviderData)(nil)
+)
 
 // Redeem provides a default implementation of the OAuth2 token redemption process
 func (p *ProviderData) Redeem(ctx context.Context, redirectURL, code string) (s *sessions.SessionState, err error) {
@@ -81,21 +87,23 @@ func (p *ProviderData) GetLoginURL(redirectURI, state string) string {
 }
 
 // GetEmailAddress returns the Account email address
-func (p *ProviderData) GetEmailAddress(ctx context.Context, s *sessions.SessionState) (string, error) {
-	return "", errors.New("not implemented")
-}
-
-// GetUserName returns the Account username
-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) {
+	return "", ErrNotImplemented
 }
 
 // ValidateGroup validates that the provided email exists in the configured provider
 // email group(s).
-func (p *ProviderData) ValidateGroup(email string) bool {
+func (p *ProviderData) ValidateGroup(_ string) bool {
 	return true
 }
 
+// EnrichSessionState is called after Redeem to allow providers to enrich session fields
+// such as User, Email, Groups with provider specific API calls.
+func (p *ProviderData) EnrichSessionState(_ context.Context, _ *sessions.SessionState) error {
+	return nil
+}
+
 // ValidateSessionState validates the AccessToken
 func (p *ProviderData) ValidateSessionState(ctx context.Context, s *sessions.SessionState) bool {
 	return validateToken(ctx, p, s.AccessToken, nil)
@@ -103,12 +111,12 @@ func (p *ProviderData) ValidateSessionState(ctx context.Context, s *sessions.Ses
 
 // RefreshSessionIfNeeded should refresh the user's session if required and
 // do nothing if a refresh is not required
-func (p *ProviderData) RefreshSessionIfNeeded(ctx context.Context, s *sessions.SessionState) (bool, error) {
+func (p *ProviderData) RefreshSessionIfNeeded(_ context.Context, _ *sessions.SessionState) (bool, error) {
 	return false, nil
 }
 
 // CreateSessionStateFromBearerToken should be implemented to allow providers
 // to convert ID tokens into sessions
-func (p *ProviderData) CreateSessionStateFromBearerToken(ctx context.Context, rawIDToken string, idToken *oidc.IDToken) (*sessions.SessionState, error) {
-	return nil, errors.New("not implemented")
+func (p *ProviderData) CreateSessionStateFromBearerToken(_ context.Context, _ string, _ *oidc.IDToken) (*sessions.SessionState, error) {
+	return nil, ErrNotImplemented
 }
diff --git a/providers/providers.go b/providers/providers.go
index e92b3293..6987cf6b 100644
--- a/providers/providers.go
+++ b/providers/providers.go
@@ -10,10 +10,11 @@ import (
 // Provider represents an upstream identity provider implementation
 type Provider interface {
 	Data() *ProviderData
+	// DEPRECATED: Migrate to EnrichSessionState
 	GetEmailAddress(ctx context.Context, s *sessions.SessionState) (string, error)
-	GetUserName(ctx context.Context, s *sessions.SessionState) (string, error)
 	Redeem(ctx context.Context, redirectURI, code string) (*sessions.SessionState, error)
 	ValidateGroup(string) bool
+	EnrichSessionState(ctx context.Context, s *sessions.SessionState) error
 	ValidateSessionState(ctx context.Context, s *sessions.SessionState) bool
 	GetLoginURL(redirectURI, finalRedirect string) string
 	RefreshSessionIfNeeded(ctx context.Context, s *sessions.SessionState) (bool, error)


Test patch:
diff --git a/oauthproxy_test.go b/oauthproxy_test.go
index d0fd9481..46ee24b8 100644
--- a/oauthproxy_test.go
+++ b/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)
diff --git a/providers/github_test.go b/providers/github_test.go
index dba4bcf6..d19aaa37 100644
--- a/providers/github_test.go
+++ b/providers/github_test.go
@@ -107,7 +107,7 @@ func TestGitHubProviderOverrides(t *testing.T) {
 	assert.Equal(t, "profile", p.Data().Scope)
 }
 
-func TestGitHubProviderGetEmailAddress(t *testing.T) {
+func TestGitHubProvider_getEmail(t *testing.T) {
 	b := testGitHubBackend(map[string][]string{
 		"/user/emails": {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},
 	})
@@ -117,14 +117,14 @@ func TestGitHubProviderGetEmailAddress(t *testing.T) {
 	p := testGitHubProvider(bURL.Host)
 
 	session := CreateAuthorizedSession()
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.Equal(t, nil, err)
-	assert.Equal(t, "michael.bland@gsa.gov", email)
+	err := p.getEmail(context.Background(), session)
+	assert.NoError(t, err)
+	assert.Equal(t, "michael.bland@gsa.gov", session.Email)
 }
 
-func TestGitHubProviderGetEmailAddressNotVerified(t *testing.T) {
+func TestGitHubProvider_getEmailNotVerified(t *testing.T) {
 	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} ]`},
 	})
 	defer b.Close()
 
@@ -132,12 +132,12 @@ func TestGitHubProviderGetEmailAddressNotVerified(t *testing.T) {
 	p := testGitHubProvider(bURL.Host)
 
 	session := CreateAuthorizedSession()
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.Equal(t, nil, err)
-	assert.Empty(t, "", email)
+	err := p.getEmail(context.Background(), session)
+	assert.NoError(t, err)
+	assert.Empty(t, session.Email)
 }
 
-func TestGitHubProviderGetEmailAddressWithOrg(t *testing.T) {
+func TestGitHubProvider_getEmailWithOrg(t *testing.T) {
 	b := testGitHubBackend(map[string][]string{
 		"/user/emails": {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},
 		"/user/orgs": {
@@ -153,12 +153,12 @@ func TestGitHubProviderGetEmailAddressWithOrg(t *testing.T) {
 	p.Org = "testorg1"
 
 	session := CreateAuthorizedSession()
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.Equal(t, nil, err)
-	assert.Equal(t, "michael.bland@gsa.gov", email)
+	err := p.getEmail(context.Background(), session)
+	assert.NoError(t, err)
+	assert.Equal(t, "michael.bland@gsa.gov", session.Email)
 }
 
-func TestGitHubProviderGetEmailAddressWithWriteAccessToPublicRepo(t *testing.T) {
+func TestGitHubProvider_getEmailWithWriteAccessToPublicRepo(t *testing.T) {
 	b := testGitHubBackend(map[string][]string{
 		"/repo/oauth2-proxy/oauth2-proxy": {`{"permissions": {"pull": true, "push": true}, "private": false}`},
 		"/user/emails":                    {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},
@@ -170,12 +170,12 @@ func TestGitHubProviderGetEmailAddressWithWriteAccessToPublicRepo(t *testing.T)
 	p.SetRepo("oauth2-proxy/oauth2-proxy", "")
 
 	session := CreateAuthorizedSession()
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.Equal(t, nil, err)
-	assert.Equal(t, "michael.bland@gsa.gov", email)
+	err := p.getEmail(context.Background(), session)
+	assert.NoError(t, err)
+	assert.Equal(t, "michael.bland@gsa.gov", session.Email)
 }
 
-func TestGitHubProviderGetEmailAddressWithReadOnlyAccessToPrivateRepo(t *testing.T) {
+func TestGitHubProvider_getEmailWithReadOnlyAccessToPrivateRepo(t *testing.T) {
 	b := testGitHubBackend(map[string][]string{
 		"/repo/oauth2-proxy/oauth2-proxy": {`{"permissions": {"pull": true, "push": false}, "private": true}`},
 		"/user/emails":                    {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},
@@ -187,12 +187,12 @@ func TestGitHubProviderGetEmailAddressWithReadOnlyAccessToPrivateRepo(t *testing
 	p.SetRepo("oauth2-proxy/oauth2-proxy", "")
 
 	session := CreateAuthorizedSession()
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.Equal(t, nil, err)
-	assert.Equal(t, "michael.bland@gsa.gov", email)
+	err := p.getEmail(context.Background(), session)
+	assert.NoError(t, err)
+	assert.Equal(t, "michael.bland@gsa.gov", session.Email)
 }
 
-func TestGitHubProviderGetEmailAddressWithWriteAccessToPrivateRepo(t *testing.T) {
+func TestGitHubProvider_getEmailWithWriteAccessToPrivateRepo(t *testing.T) {
 	b := testGitHubBackend(map[string][]string{
 		"/repo/oauth2-proxy/oauth2-proxy": {`{"permissions": {"pull": true, "push": true}, "private": true}`},
 		"/user/emails":                    {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},
@@ -204,14 +204,14 @@ func TestGitHubProviderGetEmailAddressWithWriteAccessToPrivateRepo(t *testing.T)
 	p.SetRepo("oauth2-proxy/oauth2-proxy", "")
 
 	session := CreateAuthorizedSession()
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.Equal(t, nil, err)
-	assert.Equal(t, "michael.bland@gsa.gov", email)
+	err := p.getEmail(context.Background(), session)
+	assert.NoError(t, err)
+	assert.Equal(t, "michael.bland@gsa.gov", session.Email)
 }
 
-func TestGitHubProviderGetEmailAddressWithNoAccessToPrivateRepo(t *testing.T) {
+func TestGitHubProvider_getEmailWithNoAccessToPrivateRepo(t *testing.T) {
 	b := testGitHubBackend(map[string][]string{
-		"/repo/oauth2-proxy/oauth2-proxy": {},
+		"/repo/oauth2-proxy/oauth2-proxy": {`{}`},
 	})
 	defer b.Close()
 
@@ -220,12 +220,12 @@ func TestGitHubProviderGetEmailAddressWithNoAccessToPrivateRepo(t *testing.T) {
 	p.SetRepo("oauth2-proxy/oauth2-proxy", "")
 
 	session := CreateAuthorizedSession()
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.NotEqual(t, nil, err)
-	assert.Equal(t, "", email)
+	err := p.getEmail(context.Background(), session)
+	assert.NoError(t, err)
+	assert.Empty(t, session.Email)
 }
 
-func TestGitHubProviderGetEmailAddressWithToken(t *testing.T) {
+func TestGitHubProvider_getEmailWithToken(t *testing.T) {
 	b := testGitHubBackend(map[string][]string{
 		"/user/emails": {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},
 	})
@@ -236,14 +236,14 @@ func TestGitHubProviderGetEmailAddressWithToken(t *testing.T) {
 	p.SetRepo("oauth2-proxy/oauth2-proxy", "token")
 
 	session := CreateAuthorizedSession()
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.Equal(t, nil, err)
-	assert.Equal(t, "michael.bland@gsa.gov", email)
+	err := p.getEmail(context.Background(), session)
+	assert.NoError(t, err)
+	assert.Equal(t, "michael.bland@gsa.gov", session.Email)
 }
 
 // Note that trying to trigger the "failed building request" case is not
 // practical, since the only way it can fail is if the URL fails to parse.
-func TestGitHubProviderGetEmailAddressFailedRequest(t *testing.T) {
+func TestGitHubProvider_getEmailFailedRequest(t *testing.T) {
 	b := testGitHubBackend(map[string][]string{})
 	defer b.Close()
 
@@ -254,12 +254,12 @@ func TestGitHubProviderGetEmailAddressFailedRequest(t *testing.T) {
 	// token. Alternatively, we could allow the parsing of the payload as
 	// JSON to fail.
 	session := &sessions.SessionState{AccessToken: "unexpected_access_token"}
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.NotEqual(t, nil, err)
-	assert.Equal(t, "", email)
+	err := p.getEmail(context.Background(), session)
+	assert.Error(t, err)
+	assert.Empty(t, session.Email)
 }
 
-func TestGitHubProviderGetEmailAddressEmailNotPresentInPayload(t *testing.T) {
+func TestGitHubProvider_getEmailNotPresentInPayload(t *testing.T) {
 	b := testGitHubBackend(map[string][]string{
 		"/user/emails": {`{"foo": "bar"}`},
 	})
@@ -269,12 +269,12 @@ func TestGitHubProviderGetEmailAddressEmailNotPresentInPayload(t *testing.T) {
 	p := testGitHubProvider(bURL.Host)
 
 	session := CreateAuthorizedSession()
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.NotEqual(t, nil, err)
-	assert.Equal(t, "", email)
+	err := p.getEmail(context.Background(), session)
+	assert.Error(t, err)
+	assert.Empty(t, session.Email)
 }
 
-func TestGitHubProviderGetUserName(t *testing.T) {
+func TestGitHubProvider_getUser(t *testing.T) {
 	b := testGitHubBackend(map[string][]string{
 		"/user": {`{"email": "michael.bland@gsa.gov", "login": "mbland"}`},
 	})
@@ -284,12 +284,12 @@ func TestGitHubProviderGetUserName(t *testing.T) {
 	p := testGitHubProvider(bURL.Host)
 
 	session := CreateAuthorizedSession()
-	email, err := p.GetUserName(context.Background(), session)
-	assert.Equal(t, nil, err)
-	assert.Equal(t, "mbland", email)
+	err := p.getUser(context.Background(), session)
+	assert.NoError(t, err)
+	assert.Equal(t, "mbland", session.User)
 }
 
-func TestGitHubProviderGetUserNameWithRepoAndToken(t *testing.T) {
+func TestGitHubProvider_getUserWithRepoAndToken(t *testing.T) {
 	b := testGitHubBackend(map[string][]string{
 		"/user": {`{"email": "michael.bland@gsa.gov", "login": "mbland"}`},
 		"/repos/oauth2-proxy/oauth2-proxy/collaborators/mbland": {""},
@@ -301,12 +301,12 @@ func TestGitHubProviderGetUserNameWithRepoAndToken(t *testing.T) {
 	p.SetRepo("oauth2-proxy/oauth2-proxy", "token")
 
 	session := CreateAuthorizedSession()
-	email, err := p.GetUserName(context.Background(), session)
-	assert.Equal(t, nil, err)
-	assert.Equal(t, "mbland", email)
+	err := p.getUser(context.Background(), session)
+	assert.NoError(t, err)
+	assert.Equal(t, "mbland", session.User)
 }
 
-func TestGitHubProviderGetUserNameWithRepoAndTokenWithoutPushAccess(t *testing.T) {
+func TestGitHubProvider_getUserWithRepoAndTokenWithoutPushAccess(t *testing.T) {
 	b := testGitHubBackend(map[string][]string{})
 	defer b.Close()
 
@@ -315,12 +315,12 @@ func TestGitHubProviderGetUserNameWithRepoAndTokenWithoutPushAccess(t *testing.T
 	p.SetRepo("oauth2-proxy/oauth2-proxy", "token")
 
 	session := CreateAuthorizedSession()
-	email, err := p.GetUserName(context.Background(), session)
-	assert.NotEqual(t, nil, err)
-	assert.Equal(t, "", email)
+	err := p.getUser(context.Background(), session)
+	assert.Error(t, err)
+	assert.Empty(t, session.User)
 }
 
-func TestGitHubProviderGetEmailAddressWithUsername(t *testing.T) {
+func TestGitHubProvider_getEmailWithUsername(t *testing.T) {
 	b := testGitHubBackend(map[string][]string{
 		"/user":        {`{"email": "michael.bland@gsa.gov", "login": "mbland"}`},
 		"/user/emails": {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},
@@ -332,12 +332,12 @@ func TestGitHubProviderGetEmailAddressWithUsername(t *testing.T) {
 	p.SetUsers([]string{"mbland", "octocat"})
 
 	session := CreateAuthorizedSession()
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.Equal(t, nil, err)
-	assert.Equal(t, "michael.bland@gsa.gov", email)
+	err := p.getEmail(context.Background(), session)
+	assert.NoError(t, err)
+	assert.Equal(t, "michael.bland@gsa.gov", session.Email)
 }
 
-func TestGitHubProviderGetEmailAddressWithNotAllowedUsername(t *testing.T) {
+func TestGitHubProvider_getEmailWithNotAllowedUsername(t *testing.T) {
 	b := testGitHubBackend(map[string][]string{
 		"/user":        {`{"email": "michael.bland@gsa.gov", "login": "mbland"}`},
 		"/user/emails": {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},
@@ -349,12 +349,12 @@ func TestGitHubProviderGetEmailAddressWithNotAllowedUsername(t *testing.T) {
 	p.SetUsers([]string{"octocat"})
 
 	session := CreateAuthorizedSession()
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.NotEqual(t, nil, err)
-	assert.Equal(t, "", email)
+	err := p.getEmail(context.Background(), session)
+	assert.Error(t, err)
+	assert.Empty(t, session.Email)
 }
 
-func TestGitHubProviderGetEmailAddressWithUsernameAndNotBelongToOrg(t *testing.T) {
+func TestGitHubProvider_getEmailWithUsernameAndNotBelongToOrg(t *testing.T) {
 	b := testGitHubBackend(map[string][]string{
 		"/user":        {`{"email": "michael.bland@gsa.gov", "login": "mbland"}`},
 		"/user/emails": {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},
@@ -371,16 +371,16 @@ func TestGitHubProviderGetEmailAddressWithUsernameAndNotBelongToOrg(t *testing.T
 	p.SetUsers([]string{"mbland"})
 
 	session := CreateAuthorizedSession()
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.Equal(t, nil, err)
-	assert.Equal(t, "michael.bland@gsa.gov", email)
+	err := p.getEmail(context.Background(), session)
+	assert.NoError(t, err)
+	assert.Equal(t, "michael.bland@gsa.gov", session.Email)
 }
 
-func TestGitHubProviderGetEmailAddressWithUsernameAndNoAccessToPrivateRepo(t *testing.T) {
+func TestGitHubProvider_getEmailWithUsernameAndNoAccessToPrivateRepo(t *testing.T) {
 	b := testGitHubBackend(map[string][]string{
 		"/user":                           {`{"email": "michael.bland@gsa.gov", "login": "mbland"}`},
 		"/user/emails":                    {`[ {"email": "michael.bland@gsa.gov", "verified": true, "primary": true} ]`},
-		"/repo/oauth2-proxy/oauth2-proxy": {},
+		"/repo/oauth2-proxy/oauth2-proxy": {`{}`},
 	})
 	defer b.Close()
 
@@ -390,7 +390,7 @@ func TestGitHubProviderGetEmailAddressWithUsernameAndNoAccessToPrivateRepo(t *te
 	p.SetUsers([]string{"mbland"})
 
 	session := CreateAuthorizedSession()
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.Equal(t, nil, err)
-	assert.Equal(t, "michael.bland@gsa.gov", email)
+	err := p.getEmail(context.Background(), session)
+	assert.NoError(t, err)
+	assert.Equal(t, "michael.bland@gsa.gov", session.Email)
 }
diff --git a/providers/gitlab_test.go b/providers/gitlab_test.go
index 21bd04f7..12b9d6f4 100644
--- a/providers/gitlab_test.go
+++ b/providers/gitlab_test.go
@@ -64,8 +64,8 @@ func TestGitLabProviderBadToken(t *testing.T) {
 	p := testGitLabProvider(bURL.Host)
 
 	session := &sessions.SessionState{AccessToken: "unexpected_gitlab_access_token"}
-	_, err := p.GetEmailAddress(context.Background(), session)
-	assert.NotEqual(t, nil, err)
+	err := p.EnrichSessionState(context.Background(), session)
+	assert.Error(t, err)
 }
 
 func TestGitLabProviderUnverifiedEmailDenied(t *testing.T) {
@@ -76,8 +76,8 @@ func TestGitLabProviderUnverifiedEmailDenied(t *testing.T) {
 	p := testGitLabProvider(bURL.Host)
 
 	session := &sessions.SessionState{AccessToken: "gitlab_access_token"}
-	_, err := p.GetEmailAddress(context.Background(), session)
-	assert.NotEqual(t, nil, err)
+	err := p.EnrichSessionState(context.Background(), session)
+	assert.Error(t, err)
 }
 
 func TestGitLabProviderUnverifiedEmailAllowed(t *testing.T) {
@@ -89,9 +89,9 @@ func TestGitLabProviderUnverifiedEmailAllowed(t *testing.T) {
 	p.AllowUnverifiedEmail = true
 
 	session := &sessions.SessionState{AccessToken: "gitlab_access_token"}
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.Equal(t, nil, err)
-	assert.Equal(t, "foo@bar.com", email)
+	err := p.EnrichSessionState(context.Background(), session)
+	assert.NoError(t, err)
+	assert.Equal(t, "foo@bar.com", session.Email)
 }
 
 func TestGitLabProviderUsername(t *testing.T) {
@@ -103,9 +103,9 @@ func TestGitLabProviderUsername(t *testing.T) {
 	p.AllowUnverifiedEmail = true
 
 	session := &sessions.SessionState{AccessToken: "gitlab_access_token"}
-	username, err := p.GetUserName(context.Background(), session)
-	assert.Equal(t, nil, err)
-	assert.Equal(t, "FooBar", username)
+	err := p.EnrichSessionState(context.Background(), session)
+	assert.NoError(t, err)
+	assert.Equal(t, "FooBar", session.User)
 }
 
 func TestGitLabProviderGroupMembershipValid(t *testing.T) {
@@ -118,9 +118,9 @@ func TestGitLabProviderGroupMembershipValid(t *testing.T) {
 	p.Groups = []string{"foo"}
 
 	session := &sessions.SessionState{AccessToken: "gitlab_access_token"}
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.Equal(t, nil, err)
-	assert.Equal(t, "foo@bar.com", email)
+	err := p.EnrichSessionState(context.Background(), session)
+	assert.NoError(t, err)
+	assert.Equal(t, "FooBar", session.User)
 }
 
 func TestGitLabProviderGroupMembershipMissing(t *testing.T) {
@@ -133,35 +133,6 @@ func TestGitLabProviderGroupMembershipMissing(t *testing.T) {
 	p.Groups = []string{"baz"}
 
 	session := &sessions.SessionState{AccessToken: "gitlab_access_token"}
-	_, err := p.GetEmailAddress(context.Background(), session)
-	assert.NotEqual(t, nil, err)
-}
-
-func TestGitLabProviderEmailDomainValid(t *testing.T) {
-	b := testGitLabBackend()
-	defer b.Close()
-
-	bURL, _ := url.Parse(b.URL)
-	p := testGitLabProvider(bURL.Host)
-	p.AllowUnverifiedEmail = true
-	p.EmailDomains = []string{"bar.com"}
-
-	session := &sessions.SessionState{AccessToken: "gitlab_access_token"}
-	email, err := p.GetEmailAddress(context.Background(), session)
-	assert.Equal(t, nil, err)
-	assert.Equal(t, "foo@bar.com", email)
-}
-
-func TestGitLabProviderEmailDomainInvalid(t *testing.T) {
-	b := testGitLabBackend()
-	defer b.Close()
-
-	bURL, _ := url.Parse(b.URL)
-	p := testGitLabProvider(bURL.Host)
-	p.AllowUnverifiedEmail = true
-	p.EmailDomains = []string{"baz.com"}
-
-	session := &sessions.SessionState{AccessToken: "gitlab_access_token"}
-	_, err := p.GetEmailAddress(context.Background(), session)
-	assert.NotEqual(t, nil, err)
+	err := p.EnrichSessionState(context.Background(), session)
+	assert.Error(t, err)
 }
diff --git a/providers/provider_default_test.go b/providers/provider_default_test.go
index 8597ac66..f04fe607 100644
--- a/providers/provider_default_test.go
+++ b/providers/provider_default_test.go
@@ -47,3 +47,9 @@ func TestAcrValuesConfigured(t *testing.T) {
 	result := p.GetLoginURL("https://my.test.app/oauth", "")
 	assert.Contains(t, result, "acr_values=testValue")
 }
+
+func TestEnrichSessionState(t *testing.T) {
+	p := &ProviderData{}
+	s := &sessions.SessionState{}
+	assert.NoError(t, p.EnrichSessionState(context.Background(), s))
+}