From a53198725ea7c4127d1d0bf55b56fecc013d61b4 Mon Sep 17 00:00:00 2001
From: Nick Meves <nick.meves@greenhouse.io>
Date: Sat, 19 Jun 2021 16:06:58 -0700
Subject: [PATCH] Use `upn` as EmailClaim throughout ADFSProvider

By only overriding in the EnrichSession, any Refresh calls
would've overriden it with the `email` claim.
---
 CHANGELOG.md           |  1 +
 providers/adfs.go      | 52 +++++++++---------------------------------
 providers/adfs_test.go |  4 ++--
 3 files changed, 14 insertions(+), 43 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8defa4f8..b94310f4 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,6 +8,7 @@
 
 ## Changes since v7.2.0
 
+- [#1247](https://github.com/oauth2-proxy/oauth2-proxy/pull/1247) Use `upn` claim consistently in ADFSProvider (@NickMeves)
 - [#1447](https://github.com/oauth2-proxy/oauth2-proxy/pull/1447) Fix docker build/push issues found during last release (@JoelSpeed)
 - [#1433](https://github.com/oauth2-proxy/oauth2-proxy/pull/1433) Let authentication fail when session validation fails (@stippi2)
 - [#1445](https://github.com/oauth2-proxy/oauth2-proxy/pull/1445) Fix docker container multi arch build issue by passing GOARCH details to make build (@jkandasa)
diff --git a/providers/adfs.go b/providers/adfs.go
index c626bd09..89119c7a 100644
--- a/providers/adfs.go
+++ b/providers/adfs.go
@@ -1,36 +1,32 @@
 package providers
 
 import (
-	"context"
-	"errors"
-	"fmt"
 	"net/url"
 	"strings"
-
-	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions"
 )
 
 // ADFSProvider represents an ADFS based Identity Provider
 type ADFSProvider struct {
 	*OIDCProvider
-	SkipScope bool
+	skipScope bool
 }
 
 var _ Provider = (*ADFSProvider)(nil)
 
 const (
-	ADFSProviderName = "ADFS"
-	ADFSDefaultScope = "openid email profile"
-	ADFSSkipScope    = false
+	adfsProviderName = "ADFS"
+	adfsDefaultScope = "openid email profile"
+	adfsSkipScope    = false
+	adfsEmailClaim   = "upn"
 )
 
 // NewADFSProvider initiates a new ADFSProvider
 func NewADFSProvider(p *ProviderData) *ADFSProvider {
-
 	p.setProviderDefaults(providerDefaults{
-		name:  ADFSProviderName,
-		scope: ADFSDefaultScope,
+		name:  adfsProviderName,
+		scope: adfsDefaultScope,
 	})
+	p.EmailClaim = adfsEmailClaim
 
 	if p.ProtectedResource != nil && p.ProtectedResource.String() != "" {
 		resource := p.ProtectedResource.String()
@@ -48,13 +44,13 @@ func NewADFSProvider(p *ProviderData) *ADFSProvider {
 			ProviderData: p,
 			SkipNonce:    true,
 		},
-		SkipScope: ADFSSkipScope,
+		skipScope: adfsSkipScope,
 	}
 }
 
 // Configure defaults the ADFSProvider configuration options
 func (p *ADFSProvider) Configure(skipScope bool) {
-	p.SkipScope = skipScope
+	p.skipScope = skipScope
 }
 
 // GetLoginURL Override to double encode the state parameter. If not query params are lost
@@ -65,36 +61,10 @@ func (p *ADFSProvider) GetLoginURL(redirectURI, state, nonce string) string {
 		extraParams.Add("nonce", nonce)
 	}
 	loginURL := makeLoginURL(p.Data(), redirectURI, url.QueryEscape(state), extraParams)
-	if p.SkipScope {
+	if p.skipScope {
 		q := loginURL.Query()
 		q.Del("scope")
 		loginURL.RawQuery = q.Encode()
 	}
 	return loginURL.String()
 }
-
-// EnrichSession to add email
-func (p *ADFSProvider) EnrichSession(ctx context.Context, s *sessions.SessionState) error {
-	if s.Email != "" {
-		return nil
-	}
-
-	idToken, err := p.Verifier.Verify(ctx, s.IDToken)
-	if err != nil {
-		return err
-	}
-
-	p.EmailClaim = "upn"
-	c, err := p.getClaims(idToken)
-
-	if err != nil {
-		return fmt.Errorf("couldn't extract claims from id_token (%v)", err)
-	}
-	s.Email = c.Email
-
-	if s.Email == "" {
-		err = errors.New("email not set")
-	}
-
-	return err
-}
diff --git a/providers/adfs_test.go b/providers/adfs_test.go
index bd2a82ce..ae306cd1 100644
--- a/providers/adfs_test.go
+++ b/providers/adfs_test.go
@@ -134,8 +134,8 @@ var _ = Describe("ADFS Provider Tests", func() {
 			idToken, err := p.Verifier.Verify(context.Background(), rawIDToken)
 			Expect(err).To(BeNil())
 			session, err := p.buildSessionFromClaims(idToken)
-			session.IDToken = rawIDToken
 			Expect(err).To(BeNil())
+			session.IDToken = rawIDToken
 			err = p.EnrichSession(context.Background(), session)
 			Expect(session.Email).To(Equal("janed@me.com"))
 			Expect(err).To(BeNil())
@@ -149,7 +149,7 @@ var _ = Describe("ADFS Provider Tests", func() {
 				ProtectedResource: resource,
 				Scope:             "",
 			})
-			p.SkipScope = true
+			p.skipScope = true
 
 			result := p.GetLoginURL("https://example.com/adfs/oauth2/", "", "")
 			Expect(result).NotTo(ContainSubstring("scope="))