Repository: oauth2-proxy/oauth2-proxy
Pull request #2001 (2023-02-04 21:23:48)

fix typo in comment for oidc/provider.go
## Description

just a typo fix - no functional changes

## 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! -->

- [ ] My change requires a change to the documentation or CHANGELOG.
- [ ] I have updated the documentation/CHANGELOG accordingly.
- [x] I have created a feature (non-master) branch for my PR - github web editor created patch-1



Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 59bafc91..6fe5ec52 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,7 +14,8 @@
 - [#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
-
+- [#1951](https://github.com/oauth2-proxy/oauth2-proxy/pull/1951) Fix validate URL, check if query string marker (?) or separator (&) needs to be appended (@miguelborges99)
+- [#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/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md
index e36544e0..ea398506 100644
--- a/docs/docs/configuration/overview.md
+++ b/docs/docs/configuration/overview.md
@@ -99,7 +99,7 @@ An example [oauth2-proxy.cfg](https://github.com/oauth2-proxy/oauth2-proxy/blob/
 | `--cookie-csrf-per-request` | bool | Enable having different CSRF cookies per request, making it possible to have parallel requests. | false |
 | `--cookie-csrf-expire` | duration | expire timeframe for CSRF cookie | 15m |
 | `--custom-templates-dir` | string | path to custom html templates | |
-| `--custom-sign-in-logo` | string | path or a URL to an custom image for the sign_in page logo. Use \"-\" to disable default logo. |
+| `--custom-sign-in-logo` | string | path or a URL to an custom image for the sign_in page logo. Use `"-"` to disable default logo. |
 | `--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 | |
diff --git a/go.mod b/go.mod
index 14a578e8..0d176d02 100644
--- a/go.mod
+++ b/go.mod
@@ -31,7 +31,7 @@ require (
 	github.com/vmihailenco/msgpack/v5 v5.3.5
 	golang.org/x/crypto v0.1.0
 	golang.org/x/exp v0.0.0-20221019170559-20944726eadf
-	golang.org/x/net v0.1.0
+	golang.org/x/net v0.5.0
 	golang.org/x/oauth2 v0.1.0
 	golang.org/x/sync v0.1.0
 	google.golang.org/api v0.100.0
@@ -69,8 +69,8 @@ require (
 	github.com/vmihailenco/tagparser/v2 v2.0.0 // indirect
 	github.com/yuin/gopher-lua v0.0.0-20210529063254-f4c35e4016d9 // indirect
 	go.opencensus.io v0.23.0 // indirect
-	golang.org/x/sys v0.1.0 // indirect
-	golang.org/x/text v0.4.0 // indirect
+	golang.org/x/sys v0.4.0 // indirect
+	golang.org/x/text v0.6.0 // indirect
 	google.golang.org/appengine v1.6.7 // indirect
 	google.golang.org/genproto v0.0.0-20221014213838-99cd37c6964a // indirect
 	google.golang.org/grpc v1.50.1 // indirect
diff --git a/go.sum b/go.sum
index f61ab4e9..388c3d79 100644
--- a/go.sum
+++ b/go.sum
@@ -536,8 +536,8 @@ golang.org/x/net v0.0.0-20220425223048-2871e0cb64e4/go.mod h1:CfG3xpIq0wQ8r1q4Su
 golang.org/x/net v0.0.0-20220607020251-c690dde0001d/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c=
 golang.org/x/net v0.0.0-20220624214902-1bab6f366d9e/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c=
 golang.org/x/net v0.0.0-20220826154423-83b083e8dc8b/go.mod h1:YDH+HFinaLZZlnHAfSS6ZXJJ9M9t4Dl22yv3iI2vPwk=
-golang.org/x/net v0.1.0 h1:hZ/3BUoy5aId7sCpA/Tc5lt8DkFgdVS2onTpJsZ/fl0=
-golang.org/x/net v0.1.0/go.mod h1:Cx3nUiGt4eDBEyega/BKRp+/AlGL8hYe7U9odMt2Cco=
+golang.org/x/net v0.5.0 h1:GyT4nK/YDHSqa1c4753ouYCDajOYKTja9Xb/OHtgvSw=
+golang.org/x/net v0.5.0/go.mod h1:DivGGAXEgPSlEBzxGzZI+ZLohi+xUj054jfeKui00ws=
 golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U=
 golang.org/x/oauth2 v0.0.0-20190226205417-e64efc72b421/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw=
 golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw=
@@ -651,8 +651,8 @@ golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBc
 golang.org/x/sys v0.0.0-20220610221304-9f5ed59c137d/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220728004956-3c1f35247d10/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220908164124-27713097b956/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.1.0 h1:kunALQeHf1/185U1i0GOB/fy1IPRDDpuoOOqRReG57U=
-golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
+golang.org/x/sys v0.4.0 h1:Zr2JFtRQNX3BCZ8YtxRE9hNJYC8J6I1MVbMg6owUp18=
+golang.org/x/sys v0.4.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
 golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8=
 golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
@@ -664,8 +664,8 @@ golang.org/x/text v0.3.4/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
 golang.org/x/text v0.3.5/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
 golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
 golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ=
-golang.org/x/text v0.4.0 h1:BrVqGRd7+k1DiOgtnFvAkoQEWQvBc25ouMJM6429SFg=
-golang.org/x/text v0.4.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8=
+golang.org/x/text v0.6.0 h1:3XmdazWV+ubf7QgHSTWeykHOci5oeekaGJBLkrkaw4k=
+golang.org/x/text v0.6.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8=
 golang.org/x/time v0.0.0-20181108054448-85acf8d2951c/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
 golang.org/x/time v0.0.0-20190308202827-9d24e82272b4/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
 golang.org/x/time v0.0.0-20191024005414-555d28b269f0/go.mod h1:tRJNPiyCQ0inRvYxbN9jk5I+vvW/OXSQhTDSoE431IQ=
diff --git a/oauthproxy.go b/oauthproxy.go
index a0d4bf7b..1e371e6a 100644
--- a/oauthproxy.go
+++ b/oauthproxy.go
@@ -124,7 +124,7 @@ func NewOAuthProxy(opts *options.Options, validator func(string) bool) (*OAuthPr
 
 	provider, err := providers.NewProvider(opts.Providers[0])
 	if err != nil {
-		return nil, fmt.Errorf("error intiailising provider: %v", err)
+		return nil, fmt.Errorf("error initialising provider: %v", err)
 	}
 
 	pageWriter, err := pagewriter.NewWriter(pagewriter.Opts{
diff --git a/pkg/providers/oidc/provider.go b/pkg/providers/oidc/provider.go
index 894c3263..427f79a6 100644
--- a/pkg/providers/oidc/provider.go
+++ b/pkg/providers/oidc/provider.go
@@ -9,7 +9,7 @@ import (
 	"github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests"
 )
 
-// providerJSON resresents the information we need from an OIDC discovery
+// providerJSON represents the information we need from an OIDC discovery
 type providerJSON struct {
 	Issuer               string   `json:"issuer"`
 	AuthURL              string   `json:"authorization_endpoint"`
diff --git a/providers/internal_util.go b/providers/internal_util.go
index 346ee80d..52cfd0a7 100644
--- a/providers/internal_util.go
+++ b/providers/internal_util.go
@@ -53,7 +53,11 @@ func validateToken(ctx context.Context, p Provider, accessToken string, header h
 	endpoint := p.Data().ValidateURL.String()
 	if len(header) == 0 {
 		params := url.Values{"access_token": {accessToken}}
-		endpoint = endpoint + "?" + params.Encode()
+		if hasQueryParams(endpoint) {
+			endpoint = endpoint + "&" + params.Encode()
+		} else {
+			endpoint = endpoint + "?" + params.Encode()
+		}
 	}
 
 	result := requests.New(endpoint).
@@ -74,3 +78,13 @@ func validateToken(ctx context.Context, p Provider, accessToken string, header h
 	logger.Errorf("token validation request failed: status %d - %s", result.StatusCode(), result.Body())
 	return false
 }
+
+// hasQueryParams check if URL has query parameters
+func hasQueryParams(endpoint string) bool {
+	endpointURL, err := url.Parse(endpoint)
+	if err != nil {
+		return false
+	}
+
+	return len(endpointURL.RawQuery) != 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/internal_util_test.go b/providers/internal_util_test.go
index 53070017..c8ab3f4f 100644
--- a/providers/internal_util_test.go
+++ b/providers/internal_util_test.go
@@ -132,6 +132,13 @@ func TestValidateSessionExpiredToken(t *testing.T) {
 	assert.Equal(t, false, validateToken(context.Background(), vtTest.provider, "foobar", nil))
 }
 
+func TestValidateSessionValidateURLWithQueryParams(t *testing.T) {
+	vtTest := NewValidateSessionTest()
+	defer vtTest.Close()
+	vtTest.provider.Data().ValidateURL, _ = url.Parse(vtTest.provider.Data().ValidateURL.String() + "?query_param1=true&query_param2=test")
+	assert.Equal(t, true, validateToken(context.Background(), vtTest.provider, "foobar", nil))
+}
+
 func TestStripTokenNotPresent(t *testing.T) {
 	test := "http://local.test/api/test?a=1&b=2"
 	assert.Equal(t, test, stripToken(test))
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))
+		})
+	}
+}