Repository: oauth2-proxy/oauth2-proxy
Pull request #1927 (2022-12-14 11:24:23)

Fix default scope settings for none oidc providers like GitHub
<!--- Provide a general summary of your changes in the Title above -->

## Description
As described in detail in bug #1903 and related to #1724.
The current implementation overrides the default scope for all providers with the default oidc scopes when no scope is set in the configuration file. Which is obviously not the expected behaviour.

As the config file values are loaded before the default values of the providers:
https://github.com/oauth2-proxy/oauth2-proxy/blob/fd2807c091686a2a23f41123e3470a1076e877a0/providers/providers.go#L34-L71

https://github.com/oauth2-proxy/oauth2-proxy/blob/fd2807c091686a2a23f41123e3470a1076e877a0/providers/providers.go#L155-L169

The scope is overwritten with "openid email profile" which leads to issues later on in the setProviderDefaults method. As the p.Scope will never be empty, therefore all provider default scopes are ignored.
https://github.com/oauth2-proxy/oauth2-proxy/blob/fd2807c091686a2a23f41123e3470a1076e877a0/providers/provider_data.go#L197-L199

<!--- Describe your changes in detail -->

I added a check for the providerName as an additional condition in the default setter while loading the data from config file.

## How Has This Been Tested?

I tested it locally with GitHub and dex, to ensure both implementations still work and I extended the unit test cases.

## 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.


tuunit: @JoelSpeed as this seems to be blocking quite a few people and it has been a bug for nearly a year now. I would like to ask you directly to have a look at this PR, if anything is missing to get it merged.
tuunit: Resolves #1903, resolves #1724 

JoelSpeed: Looks like the test updates aren't quite right as one of the scope tests failed, can you please look into that?
tuunit: > Looks like the test updates aren't quite right as one of the scope tests failed, can you please look into that?

And had another look at it and it should now work as expected.
tuunit: @JoelSpeed can you retrigger the build and tests? :)

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 54a5d607..f9900432 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,6 +12,7 @@
 - [#1882](https://github.com/oauth2-proxy/oauth2-proxy/pull/1882) Make `htpasswd.GetUsers` racecondition safe
 - [#1883](https://github.com/oauth2-proxy/oauth2-proxy/pull/1883) Ensure v8 manifest variant is set on docker images
 - [#1906](https://github.com/oauth2-proxy/oauth2-proxy/pull/1906) Fix PKCE code verifier generation to never use UTF-8 characters
+- [#1927](https://github.com/oauth2-proxy/oauth2-proxy/pull/1927) Fix default scope settings for none oidc providers
 
 # V7.4.0
 
diff --git a/providers/providers.go b/providers/providers.go
index d2570081..dbac1d03 100644
--- a/providers/providers.go
+++ b/providers/providers.go
@@ -152,7 +152,7 @@ func newProviderDataFromConfig(providerConfig options.Provider) (*ProviderData,
 		p.EmailClaim = providerConfig.OIDCConfig.UserIDClaim
 	}
 
-	if p.Scope == "" {
+	if providerConfig.Type == "oidc" && p.Scope == "" {
 		p.Scope = "openid email profile"
 
 		if len(providerConfig.AllowedGroups) > 0 {


Test patch:
diff --git a/providers/providers_test.go b/providers/providers_test.go
index a69a0641..8d8aa297 100644
--- a/providers/providers_test.go
+++ b/providers/providers_test.go
@@ -125,32 +125,48 @@ func TestScope(t *testing.T) {
 
 	testCases := []struct {
 		name            string
+		configuredType  options.ProviderType
 		configuredScope string
 		expectedScope   string
 		allowedGroups   []string
 	}{
 		{
-			name:            "with no scope provided",
+			name:            "oidc: with no scope provided",
+			configuredType:  "oidc",
 			configuredScope: "",
 			expectedScope:   "openid email profile",
 		},
 		{
-			name:            "with no scope provided and groups",
+			name:            "oidc: with no scope provided and groups",
+			configuredType:  "oidc",
 			configuredScope: "",
 			expectedScope:   "openid email profile groups",
 			allowedGroups:   []string{"foo"},
 		},
 		{
-			name:            "with a configured scope provided",
+			name:            "oidc: with a configured scope provided",
+			configuredType:  "oidc",
 			configuredScope: "openid",
 			expectedScope:   "openid",
 		},
+		{
+			name:            "github: with no scope provided",
+			configuredType:  "github",
+			configuredScope: "",
+			expectedScope:   "user:email",
+		},
+		{
+			name:            "github: with a configured scope provided",
+			configuredType:  "github",
+			configuredScope: "user:email org:read",
+			expectedScope:   "user:email org:read",
+		},
 	}
 
 	for _, tc := range testCases {
 		providerConfig := options.Provider{
 			ID:               providerID,
-			Type:             "oidc",
+			Type:             tc.configuredType,
 			ClientID:         clientID,
 			ClientSecretFile: clientSecret,
 			LoginURL:         msAuthURL,
@@ -164,10 +180,10 @@ func TestScope(t *testing.T) {
 			},
 		}
 
-		pd, err := newProviderDataFromConfig(providerConfig)
+		pd, err := NewProvider(providerConfig)
 		g.Expect(err).ToNot(HaveOccurred())
 
-		g.Expect(pd.Scope).To(Equal(tc.expectedScope))
+		g.Expect(pd.Data().Scope).To(Equal(tc.expectedScope))
 	}
 }