From 7861a707cd7871e71517ebf603641a00b8c0e8c7 Mon Sep 17 00:00:00 2001
From: Joel Speed <joel.speed@hotmail.co.uk>
Date: Wed, 7 Oct 2020 18:08:16 +0100
Subject: [PATCH 01/15] Fix name of test report ID variable

---
 test.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/test.sh b/test.sh
index c4f6ef85..114940b1 100755
--- a/test.sh
+++ b/test.sh
@@ -2,8 +2,8 @@
 # manually exiting from script, because after-build needs to run always
 set +e
 
-if [ -z $CC_TEST_REPORT_ID ]; then
-  echo "1. CC_TEST_REPORT_ID is unset, skipping"
+if [ -z $CC_TEST_REPORTER_ID ]; then
+  echo "1. CC_TEST_REPORTER_ID is unset, skipping"
 else
   echo "1. Running before-build"
   ./cc-test-reporter before-build
@@ -14,8 +14,8 @@ make test
 TEST_STATUS=$?
 echo "TEST_STATUS: ${TEST_STATUS}"
 
-if [ -z $CC_TEST_REPORT_ID ]; then
-  echo "3. CC_TEST_REPORT_ID is unset, skipping"
+if [ -z $CC_TEST_REPORTER_ID ]; then
+  echo "3. CC_TEST_REPORTER_ID is unset, skipping"
 else
   echo "3. Running after-build"
   ./cc-test-reporter after-build --exit-code $TEST_STATUS -t gocov

From b848663a3d74fd33dbce1aa8a2ec5ec65bdcc7bd Mon Sep 17 00:00:00 2001
From: Joel Speed <joel.speed@hotmail.co.uk>
Date: Wed, 7 Oct 2020 18:14:41 +0100
Subject: [PATCH 02/15] Move test script to workflows folder

---
 .github/workflows/ci.yaml            | 2 +-
 test.sh => .github/workflows/test.sh | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename test.sh => .github/workflows/test.sh (100%)

diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
index 1b0d26c4..9aab298c 100644
--- a/.github/workflows/ci.yaml
+++ b/.github/workflows/ci.yaml
@@ -45,7 +45,7 @@ jobs:
       env:
         CC_TEST_REPORTER_ID: ${{ secrets.CC_TEST_REPORTER_ID }}
       run: |
-        ./test.sh
+        ./.github/workflows/test.sh
 
   docker:
     runs-on: ubuntu-18.04
diff --git a/test.sh b/.github/workflows/test.sh
similarity index 100%
rename from test.sh
rename to .github/workflows/test.sh

From 70c585812e338a9ac48bfcdbbda5b13c01a9e2dc Mon Sep 17 00:00:00 2001
From: Joel Speed <joel.speed@hotmail.co.uk>
Date: Wed, 7 Oct 2020 18:22:38 +0100
Subject: [PATCH 03/15] Fix coverage file path recognition

---
 .github/workflows/test.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/.github/workflows/test.sh b/.github/workflows/test.sh
index 114940b1..e17aa713 100755
--- a/.github/workflows/test.sh
+++ b/.github/workflows/test.sh
@@ -12,13 +12,12 @@ fi
 echo "2. Running test"
 make test
 TEST_STATUS=$?
-echo "TEST_STATUS: ${TEST_STATUS}"
 
 if [ -z $CC_TEST_REPORTER_ID ]; then
   echo "3. CC_TEST_REPORTER_ID is unset, skipping"
 else
   echo "3. Running after-build"
-  ./cc-test-reporter after-build --exit-code $TEST_STATUS -t gocov
+  ./cc-test-reporter after-build --exit-code $TEST_STATUS -t gocov --prefix $(basename $(go list -m))
 fi
 
 if [ "$TEST_STATUS" -ne 0 ]; then

From 132c2cb2103940468bd62d6c6e49cd3ee2b7d631 Mon Sep 17 00:00:00 2001
From: Joel Speed <joel.speed@hotmail.co.uk>
Date: Wed, 7 Oct 2020 18:49:32 +0100
Subject: [PATCH 04/15] Add changelog for fixing test reporting in github
 actions

---
 CHANGELOG.md | 1 +
 1 file changed, 1 insertion(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 62a9c5ac..eaca5c86 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -25,6 +25,7 @@
 
 ## Changes since v6.1.1
 
+- [#825](https://github.com/oauth2-proxy/oauth2-proxy/pull/825) Fix code coverage reporting on GitHub actions(@JoelSpeed)
 - [#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)

From 420a34f8146c3f93268d3cdee0ad8606b69d30c1 Mon Sep 17 00:00:00 2001
From: Philippe Pepiot <phil@philpep.org>
Date: Wed, 14 Oct 2020 18:17:55 +0200
Subject: [PATCH 05/15] Document set_xauthrequest with pass_access_token (#829)

* Document set_xauthrequest with pass_access_token

Document feature implemented in https://github.com/oauth2-proxy/oauth2-proxy/pull/68

The feature is already decribed in in the nginx example but not clearly
on each respective parameters documentation.

* Update docs/configuration/configuration.md

Co-authored-by: Nick Meves <nick.meves@greenhouse.io>

Co-authored-by: Nick Meves <nick.meves@greenhouse.io>
---
 docs/configuration/configuration.md | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/configuration/configuration.md b/docs/configuration/configuration.md
index 8fbc406d..6f0c4333 100644
--- a/docs/configuration/configuration.md
+++ b/docs/configuration/configuration.md
@@ -79,7 +79,7 @@ An example [oauth2-proxy.cfg]({{ site.gitweb }}/contrib/oauth2-proxy.cfg.example
 | `--oidc-issuer-url` | string | the OpenID Connect issuer URL, e.g. `"https://accounts.google.com"` | |
 | `--oidc-jwks-url` | string | OIDC JWKS URI for token verification; required if OIDC discovery is disabled | |
 | `--oidc-groups-claim` | string | which claim contains the user groups | `"groups"` |
-| `--pass-access-token` | bool | pass OAuth access_token to upstream via X-Forwarded-Access-Token header | false |
+| `--pass-access-token` | bool | pass OAuth access_token to upstream via X-Forwarded-Access-Token header. When used with `--set-xauthrequest` this adds the X-Auth-Request-Access-Token header to the response | false |
 | `--pass-authorization-header` | bool | pass OIDC IDToken to upstream via Authorization Bearer header | false |
 | `--pass-basic-auth` | bool | pass HTTP Basic Auth, X-Forwarded-User, X-Forwarded-Email and X-Forwarded-Preferred-Username information to upstream | true |
 | `--prefer-email-to-user` | bool | Prefer to use the Email address as the Username when passing information to upstream. Will only use Username if Email is unavailable, e.g. htaccess authentication. Used in conjunction with `--pass-basic-auth` and `--pass-user-headers` | false |
@@ -113,7 +113,7 @@ An example [oauth2-proxy.cfg]({{ site.gitweb }}/contrib/oauth2-proxy.cfg.example
 | `--scope` | string | OAuth scope specification | |
 | `--session-cookie-minimal` | bool | strip OAuth tokens from cookie session stores if they aren't needed (cookie session store only) | false |
 | `--session-store-type` | string | [Session data storage backend](configuration/sessions); redis or cookie | cookie |
-| `--set-xauthrequest` | bool | set X-Auth-Request-User, X-Auth-Request-Groups, X-Auth-Request-Email and X-Auth-Request-Preferred-Username response headers (useful in Nginx auth_request mode) | false |
+| `--set-xauthrequest` | bool | set X-Auth-Request-User, X-Auth-Request-Groups, X-Auth-Request-Email and X-Auth-Request-Preferred-Username response headers (useful in Nginx auth_request mode). When used with `--pass-access-token`, X-Auth-Request-Access-Token is added to response headers.  | false |
 | `--set-authorization-header` | bool | set Authorization Bearer response header (useful in Nginx auth_request mode) | false |
 | `--set-basic-auth` | bool | set HTTP Basic Auth information in response (useful in Nginx auth_request mode) | false |
 | `--signature-key` | string | GAP-Signature request signature key (algorithm:secretkey) | |

From 8b44ddd547fb28df0165fb184212fb4dd8fa851e Mon Sep 17 00:00:00 2001
From: Jakub Holy <jakub.holy@telia.no>
Date: Mon, 19 Oct 2020 20:14:50 +0200
Subject: [PATCH 06/15] config: clarify skip-jwt-bearer-tokens needs (#851)

i.e. that the token must have a recognized `aud` field.
---
 docs/configuration/configuration.md | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/docs/configuration/configuration.md b/docs/configuration/configuration.md
index 6f0c4333..436e316b 100644
--- a/docs/configuration/configuration.md
+++ b/docs/configuration/configuration.md
@@ -46,7 +46,7 @@ An example [oauth2-proxy.cfg]({{ site.gitweb }}/contrib/oauth2-proxy.cfg.example
 | `--display-htpasswd-form` | bool | display username / password login form if an htpasswd file is provided | true |
 | `--email-domain` | string \| list  | authenticate emails with the specified domain (may be given multiple times). Use `*` to authenticate any email | |
 | `--errors-to-info-log` | bool | redirects error-level logging to default log channel instead of stderr | |
-| `--extra-jwt-issuers` | string | if `--skip-jwt-bearer-tokens` is set, a list of extra JWT `issuer=audience` pairs (where the issuer URL has a `.well-known/openid-configuration` or a `.well-known/jwks.json`) | |
+| `--extra-jwt-issuers` | string | if `--skip-jwt-bearer-tokens` is set, a list of extra JWT `issuer=audience` (see a token's `iss`, `aud` fields) pairs (where the issuer URL has a `.well-known/openid-configuration` or a `.well-known/jwks.json`) | |
 | `--exclude-logging-paths` | string | comma separated list of paths to exclude from logging, e.g. `"/ping,/path2"` |`""` (no paths excluded) |
 | `--flush-interval` | duration | period between flushing response buffers when streaming responses | `"1s"` |
 | `--force-https` | bool | enforce https redirect | `false` |
@@ -122,7 +122,7 @@ An example [oauth2-proxy.cfg]({{ site.gitweb }}/contrib/oauth2-proxy.cfg.example
 | `--skip-auth-regex` | string \| list | (DEPRECATED for `--skip-auth-route`) bypass authentication for requests paths that match (may be given multiple times) | |
 | `--skip-auth-route` | string \| list | bypass authentication for requests that match the method & path. Format: method=path_regex OR path_regex alone for all methods | |
 | `--skip-auth-strip-headers` | bool | strips `X-Forwarded-*` style authentication headers & `Authorization` header if they would be set by oauth2-proxy for allowlisted requests (`--skip-auth-route`, `--skip-auth-regex`, `--skip-auth-preflight`, `--trusted-ip`) | false |
-| `--skip-jwt-bearer-tokens` | bool | will skip requests that have verified JWT bearer tokens | false |
+| `--skip-jwt-bearer-tokens` | bool | will skip requests that have verified JWT bearer tokens (the token must have [`aud`](https://en.wikipedia.org/wiki/JSON_Web_Token#Standard_fields) that matches this client id or one of the extras from `extra-jwt-issuers`) | false |
 | `--skip-oidc-discovery` | bool | bypass OIDC endpoint discovery. `--login-url`, `--redeem-url` and `--oidc-jwks-url` must be configured in this case | false |
 | `--skip-provider-button` | bool | will skip sign-in-page to directly reach the next step: oauth/start | false |
 | `--ssl-insecure-skip-verify` | bool | skip validation of certificates presented when using HTTPS providers | false |

From add45c360ceeb6b12702f4f95dc447df72653393 Mon Sep 17 00:00:00 2001
From: Nick Meves <nick.meves@greenhouse.io>
Date: Sat, 26 Sep 2020 13:19:08 -0700
Subject: [PATCH 07/15] Split session enrichment from code redemption

---
 oauthproxy.go | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/oauthproxy.go b/oauthproxy.go
index e64ffe91..653701d9 100644
--- a/oauthproxy.go
+++ b/oauthproxy.go
@@ -357,22 +357,24 @@ func (p *OAuthProxy) redeemCode(ctx context.Context, host, code string) (*sessio
 	if err != nil {
 		return nil, err
 	}
+	return s, nil
+}
 
+func (p *OAuthProxy) enrichSession(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
+			return err
 		}
 	}
-
 	if s.User == "" {
 		s.User, err = p.provider.GetUserName(ctx, s)
 		if err != nil && err.Error() != "not implemented" {
-			return nil, err
+			return err
 		}
 	}
-
-	return s, nil
+	return nil
 }
 
 // MakeCSRFCookie creates a cookie for CSRF
@@ -829,14 +831,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.enrichSession(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")

From 0bd8eb31910bb09a765eadae84bb103b744bda77 Mon Sep 17 00:00:00 2001
From: Nick Meves <nick.meves@greenhouse.io>
Date: Sat, 26 Sep 2020 14:06:52 -0700
Subject: [PATCH 08/15] Setup provider.ErrNotImplemented sentinel error

---
 oauthproxy.go                 |  4 ++--
 providers/provider_default.go | 14 ++++++++++----
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/oauthproxy.go b/oauthproxy.go
index 653701d9..9f80f643 100644
--- a/oauthproxy.go
+++ b/oauthproxy.go
@@ -364,13 +364,13 @@ func (p *OAuthProxy) enrichSession(ctx context.Context, s *sessionsapi.SessionSt
 	var err error
 	if s.Email == "" {
 		s.Email, err = p.provider.GetEmailAddress(ctx, s)
-		if err != nil && err.Error() != "not implemented" {
+		if err != nil && !errors.Is(err, providers.ErrNotImplemented) {
 			return err
 		}
 	}
 	if s.User == "" {
 		s.User, err = p.provider.GetUserName(ctx, s)
-		if err != nil && err.Error() != "not implemented" {
+		if err != nil && !errors.Is(err, providers.ErrNotImplemented) {
 			return err
 		}
 	}
diff --git a/providers/provider_default.go b/providers/provider_default.go
index 8a5a98ac..4e96f0e0 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) {
@@ -82,12 +88,12 @@ 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")
+	return "", ErrNotImplemented
 }
 
 // GetUserName returns the Account username
 func (p *ProviderData) GetUserName(ctx context.Context, s *sessions.SessionState) (string, error) {
-	return "", errors.New("not implemented")
+	return "", ErrNotImplemented
 }
 
 // ValidateGroup validates that the provided email exists in the configured provider
@@ -110,5 +116,5 @@ func (p *ProviderData) RefreshSessionIfNeeded(ctx context.Context, s *sessions.S
 // 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")
+	return nil, ErrNotImplemented
 }

From b6061f08036fee813556ad4981a5e2ed0d03409d Mon Sep 17 00:00:00 2001
From: Nick Meves <nick.meves@greenhouse.io>
Date: Sat, 26 Sep 2020 15:17:26 -0700
Subject: [PATCH 09/15] Adds tests for redeemCode and enrichSession

---
 oauthproxy_test.go | 72 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/oauthproxy_test.go b/oauthproxy_test.go
index d0fd9481..9f60439b 100644
--- a/oauthproxy_test.go
+++ b/oauthproxy_test.go
@@ -404,6 +404,78 @@ func (tp *TestProvider) ValidateSessionState(ctx context.Context, session *sessi
 	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.enrichSession(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)

From 2b9e1bbba0998095cea6cbb8dd4625d93fc3d6fc Mon Sep 17 00:00:00 2001
From: Nick Meves <nick.meves@greenhouse.io>
Date: Sun, 27 Sep 2020 11:46:29 -0700
Subject: [PATCH 10/15] Add EnrichSessionState as main post-Redeem session
 updater

---
 CHANGELOG.md                       |  2 ++
 oauthproxy.go                      |  7 ++++---
 oauthproxy_test.go                 |  8 ++++----
 providers/provider_default.go      | 18 +++++++++++++-----
 providers/provider_default_test.go |  6 ++++++
 providers/providers.go             |  3 +++
 6 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index eaca5c86..5dfc03e4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -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)
 - [#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)
+- [#767](https://github.com/oauth2-proxy/oauth2-proxy/pull/796) Deprecate GetUserName & GetEmailAdress for EnrichSessionState (@NickMeves)
 - [#575](https://github.com/oauth2-proxy/oauth2-proxy/pull/575) Stop accepting legacy SHA1 signed cookies (@NickMeves)
 - [#722](https://github.com/oauth2-proxy/oauth2-proxy/pull/722) Validate Redis configuration options at startup (@NickMeves)
 - [#791](https://github.com/oauth2-proxy/oauth2-proxy/pull/791) Remove GetPreferredUsername method from provider interface (@NickMeves)
diff --git a/oauthproxy.go b/oauthproxy.go
index 9f80f643..db18e660 100644
--- a/oauthproxy.go
+++ b/oauthproxy.go
@@ -360,7 +360,7 @@ func (p *OAuthProxy) redeemCode(ctx context.Context, host, code string) (*sessio
 	return s, nil
 }
 
-func (p *OAuthProxy) enrichSession(ctx context.Context, s *sessionsapi.SessionState) error {
+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)
@@ -374,7 +374,8 @@ func (p *OAuthProxy) enrichSession(ctx context.Context, s *sessionsapi.SessionSt
 			return err
 		}
 	}
-	return nil
+
+	return p.provider.EnrichSessionState(ctx, s)
 }
 
 // MakeCSRFCookie creates a cookie for CSRF
@@ -831,7 +832,7 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {
 		return
 	}
 
-	err = p.enrichSession(req.Context(), session)
+	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")
diff --git a/oauthproxy_test.go b/oauthproxy_test.go
index 9f60439b..46ee24b8 100644
--- a/oauthproxy_test.go
+++ b/oauthproxy_test.go
@@ -396,11 +396,11 @@ 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
 }
 
@@ -468,7 +468,7 @@ func Test_enrichSession(t *testing.T) {
 				t.Fatal(err)
 			}
 
-			err = proxy.enrichSession(context.Background(), tc.session)
+			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)
@@ -1955,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/provider_default.go b/providers/provider_default.go
index 4e96f0e0..87bef08e 100644
--- a/providers/provider_default.go
+++ b/providers/provider_default.go
@@ -87,21 +87,29 @@ 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) {
+// DEPRECATED: Migrate to EnrichSessionState
+func (p *ProviderData) GetEmailAddress(_ context.Context, _ *sessions.SessionState) (string, error) {
 	return "", ErrNotImplemented
 }
 
 // GetUserName returns the Account username
-func (p *ProviderData) GetUserName(ctx context.Context, s *sessions.SessionState) (string, error) {
+// DEPRECATED: Migrate to EnrichSessionState
+func (p *ProviderData) GetUserName(_ 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)
@@ -109,12 +117,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) {
+func (p *ProviderData) CreateSessionStateFromBearerToken(_ context.Context, _ string, _ *oidc.IDToken) (*sessions.SessionState, error) {
 	return nil, ErrNotImplemented
 }
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))
+}
diff --git a/providers/providers.go b/providers/providers.go
index e92b3293..5e43613e 100644
--- a/providers/providers.go
+++ b/providers/providers.go
@@ -10,10 +10,13 @@ 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)
+	// DEPRECATED: Migrate to EnrichSessionState
 	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)

From e51f5fe7c9b939a06abad4f32cd846322a40e068 Mon Sep 17 00:00:00 2001
From: Nick Meves <nick.meves@greenhouse.io>
Date: Sun, 27 Sep 2020 13:35:06 -0700
Subject: [PATCH 11/15] Refactor GitHub to EnrichSessionState

---
 providers/github.go      |  53 +++++++++------
 providers/github_test.go | 142 +++++++++++++++++++--------------------
 2 files changed, 102 insertions(+), 93 deletions(-)

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

From 0da45e97e163c2dd901a4d336c4fa14412c19406 Mon Sep 17 00:00:00 2001
From: Nick Meves <nick.meves@greenhouse.io>
Date: Sun, 27 Sep 2020 15:15:33 -0700
Subject: [PATCH 12/15] Refactor GitLab to EnrichSessionState

---
 providers/gitlab.go      | 43 ++++++-----------------------
 providers/gitlab_test.go | 59 ++++++++++------------------------------
 2 files changed, 23 insertions(+), 79 deletions(-)

diff --git a/providers/gitlab.go b/providers/gitlab.go
index ee15a486..eb673a3d 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"
@@ -168,20 +167,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 +196,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
-}
+	s.User = userInfo.Username
+	s.Email = userInfo.Email
 
-// 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)
-	}
-
-	return userInfo.Username, nil
+	return nil
 }
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)
 }

From d9c141ae7c88af6e095c513dfcebeede52e95c27 Mon Sep 17 00:00:00 2001
From: Nick Meves <nick.meves@greenhouse.io>
Date: Sun, 27 Sep 2020 15:18:12 -0700
Subject: [PATCH 13/15] Remove GetUserName method from Provider

---
 oauthproxy.go                 | 6 ------
 providers/provider_default.go | 6 ------
 providers/providers.go        | 2 --
 3 files changed, 14 deletions(-)

diff --git a/oauthproxy.go b/oauthproxy.go
index db18e660..4fd03a40 100644
--- a/oauthproxy.go
+++ b/oauthproxy.go
@@ -368,12 +368,6 @@ func (p *OAuthProxy) enrichSessionState(ctx context.Context, s *sessionsapi.Sess
 			return err
 		}
 	}
-	if s.User == "" {
-		s.User, err = p.provider.GetUserName(ctx, s)
-		if err != nil && !errors.Is(err, providers.ErrNotImplemented) {
-			return err
-		}
-	}
 
 	return p.provider.EnrichSessionState(ctx, s)
 }
diff --git a/providers/provider_default.go b/providers/provider_default.go
index 87bef08e..479e52c0 100644
--- a/providers/provider_default.go
+++ b/providers/provider_default.go
@@ -92,12 +92,6 @@ func (p *ProviderData) GetEmailAddress(_ context.Context, _ *sessions.SessionSta
 	return "", ErrNotImplemented
 }
 
-// GetUserName returns the Account username
-// DEPRECATED: Migrate to EnrichSessionState
-func (p *ProviderData) GetUserName(_ 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(_ string) bool {
diff --git a/providers/providers.go b/providers/providers.go
index 5e43613e..6987cf6b 100644
--- a/providers/providers.go
+++ b/providers/providers.go
@@ -12,8 +12,6 @@ type Provider interface {
 	Data() *ProviderData
 	// DEPRECATED: Migrate to EnrichSessionState
 	GetEmailAddress(ctx context.Context, s *sessions.SessionState) (string, error)
-	// DEPRECATED: Migrate to EnrichSessionState
-	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

From 4a54c9421c505446168b2463aa9a5a0b24b752ad Mon Sep 17 00:00:00 2001
From: Nick Meves <nick.meves@greenhouse.io>
Date: Sun, 27 Sep 2020 15:30:45 -0700
Subject: [PATCH 14/15] Remove EmailDomain verification from GitLab provider

This is handled globally
---
 CHANGELOG.md              | 3 +--
 pkg/validation/options.go | 1 -
 providers/gitlab.go       | 4 +---
 3 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 5dfc03e4..cb496b3a 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -26,11 +26,10 @@
 ## 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)
+- [#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)
-- [#767](https://github.com/oauth2-proxy/oauth2-proxy/pull/796) Deprecate GetUserName & GetEmailAdress for EnrichSessionState (@NickMeves)
 - [#575](https://github.com/oauth2-proxy/oauth2-proxy/pull/575) Stop accepting legacy SHA1 signed cookies (@NickMeves)
 - [#722](https://github.com/oauth2-proxy/oauth2-proxy/pull/722) Validate Redis configuration options at startup (@NickMeves)
 - [#791](https://github.com/oauth2-proxy/oauth2-proxy/pull/791) Remove GetPreferredUsername method from provider interface (@NickMeves)
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/gitlab.go b/providers/gitlab.go
index eb673a3d..a04beca6 100644
--- a/providers/gitlab.go
+++ b/providers/gitlab.go
@@ -15,9 +15,7 @@ import (
 type GitLabProvider struct {
 	*ProviderData
 
-	Groups       []string
-	EmailDomains []string
-
+	Groups               []string
 	Verifier             *oidc.IDTokenVerifier
 	AllowUnverifiedEmail bool
 }

From 8abc4e6d876b2e4a5c348e05927b2c472fdfef9b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Halvard=20M=C3=B8rstad?=
 <33620089+halvardssm@users.noreply.github.com>
Date: Wed, 21 Oct 2020 18:36:17 +0200
Subject: [PATCH 15/15] Updated Gitlab docs (#859)

---
 docs/2_auth.md | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/docs/2_auth.md b/docs/2_auth.md
index d8230289..db910567 100644
--- a/docs/2_auth.md
+++ b/docs/2_auth.md
@@ -151,15 +151,25 @@ The group management in keycloak is using a tree. If you create a group named ad
 
 ### GitLab Auth Provider
 
-Whether you are using GitLab.com or self-hosting GitLab, follow [these steps to add an application](https://docs.gitlab.com/ce/integration/oauth_provider.html). Make sure to enable at least the `openid`, `profile` and `email` scopes.
+Whether you are using GitLab.com or self-hosting GitLab, follow [these steps to add an application](https://docs.gitlab.com/ce/integration/oauth_provider.html). Make sure to enable at least the `openid`, `profile` and `email` scopes, and set the redirect url to your application url e.g. https://myapp.com/oauth2/callback.
 
+The following config should be set to ensure that the oauth will work properly. To get a cookie secret follow [these steps](https://github.com/oauth2-proxy/oauth2-proxy/blob/master/docs/configuration/configuration.md#configuration)
+
+```
+    --provider="gitlab"
+    --redirect-url="https://myapp.com/oauth2/callback" // Should be the same as the redirect url for the application in gitlab
+    --client-id=GITLAB_CLIENT_ID
+    --client-secret=GITLAB_CLIENT_SECRET
+    --cookie-secret=COOKIE_SECRET
+```
+    
 Restricting by group membership is possible with the following option:
 
-    -gitlab-group="": restrict logins to members of any of these groups (slug), separated by a comma
+    --gitlab-group="mygroup,myothergroup": restrict logins to members of any of these groups (slug), separated by a comma
 
 If you are using self-hosted GitLab, make sure you set the following to the appropriate URL:
 
-    -oidc-issuer-url="<your gitlab url>"
+    --oidc-issuer-url="<your gitlab url>"
 
 ### LinkedIn Auth Provider