Repository: oauth2-proxy/oauth2-proxy
Pull request #1920 (2022-12-05 20:16:21)

Fill empty UserIDClaim before assigning it to other values
the email claim gets overridden if userIDClaim is not set.

## Description

During login I am getting the error:

```
 Error creating session during OAuth2 callback: neither the id_token nor the profileURL set an email
```

## Motivation and Context

It seems to be related to:
https://github.com/oauth2-proxy/oauth2-proxy/issues/1623
https://github.com/oauth2-proxy/oauth2-proxy/issues/1732

The underlying problem seems to be the code that handles `Backwards Compatibility for Deprecated UserIDClaim option`. it overrides the EmailClaim if they are different. But at that point the UserIDClaim is still empty, if not explicitly set and so it clears EmailClaim. Later in the code it actually is explicitly set, so I just pulled this piece of code up.
As this seems to be a workaround I am unsure about the behavior it should have and potential additional side-effects.

## How Has This Been Tested?

I wrote a new test-case.

## Checklist:

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [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.


JoelSpeed: Apologies for the delay in getting to this, but it seems like a good fix, can you add a changelog entry and I'll look to get this merged

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 59bafc91..88e81b16 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,6 +14,7 @@
 - [#1906](https://github.com/oauth2-proxy/oauth2-proxy/pull/1906) Fix PKCE code verifier generation to never use UTF-8 characters
 - [#1839](https://github.com/oauth2-proxy/oauth2-proxy/pull/1839) Add readiness checks for deeper health checks (@kobim)
 - [#1927](https://github.com/oauth2-proxy/oauth2-proxy/pull/1927) Fix default scope settings for none oidc providers
+- [#1920](https://github.com/oauth2-proxy/oauth2-proxy/pull/1920) Make sure emailClaim is not overriden if userIDClaim is not set
 
 
 # V7.4.0
diff --git a/providers/providers.go b/providers/providers.go
index dbac1d03..1640c38e 100644
--- a/providers/providers.go
+++ b/providers/providers.go
@@ -145,6 +145,10 @@ func newProviderDataFromConfig(providerConfig options.Provider) (*ProviderData,
 		logger.Printf("Warning: Your provider supports PKCE methods %+q, but you have not enabled one with --code-challenge-method", p.SupportedCodeChallengeMethods)
 	}
 
+	if providerConfig.OIDCConfig.UserIDClaim == "" {
+		providerConfig.OIDCConfig.UserIDClaim = "email"
+	}
+
 	// TODO (@NickMeves) - Remove This
 	// Backwards Compatibility for Deprecated UserIDClaim option
 	if providerConfig.OIDCConfig.EmailClaim == options.OIDCEmailClaim &&
@@ -159,9 +163,6 @@ func newProviderDataFromConfig(providerConfig options.Provider) (*ProviderData,
 			p.Scope += " groups"
 		}
 	}
-	if providerConfig.OIDCConfig.UserIDClaim == "" {
-		providerConfig.OIDCConfig.UserIDClaim = "email"
-	}
 
 	p.setAllowedGroups(providerConfig.AllowedGroups)
 


Test patch:
diff --git a/providers/providers_test.go b/providers/providers_test.go
index 8d8aa297..5476cce7 100644
--- a/providers/providers_test.go
+++ b/providers/providers_test.go
@@ -221,3 +221,52 @@ func TestCanOverwriteS256(t *testing.T) {
 
 	g.Expect(method).To(Equal(CodeChallengeMethodPlain))
 }
+
+func TestEmailClaimCorrectlySet(t *testing.T) {
+	g := NewWithT(t)
+
+	testCases := []struct {
+		name               string
+		userIDClaim        string
+		emailClaim         string
+		expectedEmailClaim string
+	}{
+		{
+			name:               "do not override EmailClaim if UserIDClaim is empty",
+			userIDClaim:        "",
+			emailClaim:         "email",
+			expectedEmailClaim: "email",
+		},
+		{
+			name:               "set EmailClaim to UserIDClaim",
+			userIDClaim:        "user_id_claim",
+			emailClaim:         "email",
+			expectedEmailClaim: "user_id_claim",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			providerConfig := options.Provider{
+				ID:               providerID,
+				Type:             "oidc",
+				ClientID:         clientID,
+				ClientSecretFile: clientSecret,
+				LoginURL:         msAuthURL,
+				RedeemURL:        msTokenURL,
+				OIDCConfig: options.OIDCOptions{
+					IssuerURL:     msIssuerURL,
+					SkipDiscovery: true,
+					JwksURL:       msKeysURL,
+					UserIDClaim:   tc.userIDClaim,
+					EmailClaim:    tc.emailClaim,
+				},
+			}
+
+			pd, err := newProviderDataFromConfig(providerConfig)
+			g.Expect(err).ToNot(HaveOccurred())
+
+			g.Expect(pd.EmailClaim).To(Equal(tc.expectedEmailClaim))
+		})
+	}
+}