Merge pull request #1560 from oauth2-proxy/fix-provider-initialisation

Fix provider data initialisation

CHANGELOG.md

@@ -8,6 +8,7 @@
 
 ## Changes since v7.2.1
 
+- [#1560](https://github.com/oauth2-proxy/oauth2-proxy/pull/1560) Fix provider data initialisation (@JoelSpeed)
 - [#1555](https://github.com/oauth2-proxy/oauth2-proxy/pull/1555) Refactor provider configuration into providers package (@JoelSpeed)
 - [#1394](https://github.com/oauth2-proxy/oauth2-proxy/pull/1394) Add generic claim extractor to get claims from ID Tokens (@JoelSpeed)
 - [#1468](https://github.com/oauth2-proxy/oauth2-proxy/pull/1468) Implement session locking with session state lock (@JoelSpeed, @Bibob7)

providers/providers.go

@@ -101,17 +101,17 @@ func newProviderDataFromConfig(providerConfig options.Provider) (*ProviderData,
 
 	errs := []error{}
 	for name, u := range map[string]struct {
-		dst *url.URL
+		dst **url.URL
 		raw string
 	}{
-		"login":    {dst: p.LoginURL, raw: providerConfig.LoginURL},
-		"redeem":   {dst: p.RedeemURL, raw: providerConfig.RedeemURL},
-		"profile":  {dst: p.ProfileURL, raw: providerConfig.ProfileURL},
-		"validate": {dst: p.ValidateURL, raw: providerConfig.ValidateURL},
-		"resource": {dst: p.ProtectedResource, raw: providerConfig.ProtectedResource},
+		"login":    {dst: &p.LoginURL, raw: providerConfig.LoginURL},
+		"redeem":   {dst: &p.RedeemURL, raw: providerConfig.RedeemURL},
+		"profile":  {dst: &p.ProfileURL, raw: providerConfig.ProfileURL},
+		"validate": {dst: &p.ValidateURL, raw: providerConfig.ValidateURL},
+		"resource": {dst: &p.ProtectedResource, raw: providerConfig.ProtectedResource},
 	} {
 		var err error
-		u.dst, err = url.Parse(u.raw)
+		*u.dst, err = url.Parse(u.raw)
 		if err != nil {
 			errs = append(errs, fmt.Errorf("could not parse %s URL: %v", name, err))
 		}
@@ -132,11 +132,11 @@ func newProviderDataFromConfig(providerConfig options.Provider) (*ProviderData,
 		p.EmailClaim = providerConfig.OIDCConfig.UserIDClaim
 	}
 
-	if providerConfig.Scope == "" {
-		providerConfig.Scope = "openid email profile"
+	if p.Scope == "" {
+		p.Scope = "openid email profile"
 
 		if len(providerConfig.AllowedGroups) > 0 {
-			providerConfig.Scope += " groups"
+			p.Scope += " groups"
 		}
 	}
 	if providerConfig.OIDCConfig.UserIDClaim == "" {

providers/providers_test.go

@@ -13,6 +13,11 @@ const (
 	clientID     = "bazquux"
 	clientSecret = "xyzzyplugh"
 	providerID   = "providerID"
+
+	msIssuerURL = "https://login.microsoftonline.com/fabrikamb2c.onmicrosoft.com/v2.0/"
+	msKeysURL   = "https://login.microsoftonline.com/fabrikamb2c.onmicrosoft.com/discovery/v2.0/keys"
+	msAuthURL   = "https://login.microsoftonline.com/fabrikamb2c.onmicrosoft.com/oauth2/v2.0/authorize?p=b2c_1_sign_in"
+	msTokenURL  = "https://login.microsoftonline.com/fabrikamb2c.onmicrosoft.com/oauth2/v2.0/token?p=b2c_1_sign_in"
 )
 
 func TestClientSecretFileOptionFails(t *testing.T) {
@@ -76,7 +81,7 @@ func TestSkipOIDCDiscovery(t *testing.T) {
 		ClientID:         clientID,
 		ClientSecretFile: clientSecret,
 		OIDCConfig: options.OIDCOptions{
-			IssuerURL:     "https://login.microsoftonline.com/fabrikamb2c.onmicrosoft.com/v2.0/",
+			IssuerURL:     msIssuerURL,
 			SkipDiscovery: true,
 		},
 	}
@@ -84,10 +89,85 @@ func TestSkipOIDCDiscovery(t *testing.T) {
 	_, err := newProviderDataFromConfig(providerConfig)
 	g.Expect(err).To(MatchError("error setting OIDC configuration: [missing required setting: login-url, missing required setting: redeem-url, missing required setting: oidc-jwks-url]"))
 
-	providerConfig.LoginURL = "https://login.microsoftonline.com/fabrikamb2c.onmicrosoft.com/oauth2/v2.0/authorize?p=b2c_1_sign_in"
-	providerConfig.RedeemURL = "https://login.microsoftonline.com/fabrikamb2c.onmicrosoft.com/oauth2/v2.0/token?p=b2c_1_sign_in"
-	providerConfig.OIDCConfig.JwksURL = "https://login.microsoftonline.com/fabrikamb2c.onmicrosoft.com/discovery/v2.0/keys"
+	providerConfig.LoginURL = msAuthURL
+	providerConfig.RedeemURL = msTokenURL
+	providerConfig.OIDCConfig.JwksURL = msKeysURL
 
 	_, err = newProviderDataFromConfig(providerConfig)
 	g.Expect(err).ToNot(HaveOccurred())
 }
+
+func TestURLsCorrectlyParsed(t *testing.T) {
+	g := NewWithT(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,
+		},
+	}
+
+	pd, err := newProviderDataFromConfig(providerConfig)
+	g.Expect(err).ToNot(HaveOccurred())
+
+	g.Expect(pd.LoginURL.String()).To(Equal(msAuthURL))
+	g.Expect(pd.RedeemURL.String()).To(Equal(msTokenURL))
+}
+
+func TestScope(t *testing.T) {
+	g := NewWithT(t)
+
+	testCases := []struct {
+		name            string
+		configuredScope string
+		expectedScope   string
+		allowedGroups   []string
+	}{
+		{
+			name:            "with no scope provided",
+			configuredScope: "",
+			expectedScope:   "openid email profile",
+		},
+		{
+			name:            "with no scope provided and groups",
+			configuredScope: "",
+			expectedScope:   "openid email profile groups",
+			allowedGroups:   []string{"foo"},
+		},
+		{
+			name:            "with a configured scope provided",
+			configuredScope: "openid",
+			expectedScope:   "openid",
+		},
+	}
+
+	for _, tc := range testCases {
+		providerConfig := options.Provider{
+			ID:               providerID,
+			Type:             "oidc",
+			ClientID:         clientID,
+			ClientSecretFile: clientSecret,
+			LoginURL:         msAuthURL,
+			RedeemURL:        msTokenURL,
+			Scope:            tc.configuredScope,
+			AllowedGroups:    tc.allowedGroups,
+			OIDCConfig: options.OIDCOptions{
+				IssuerURL:     msIssuerURL,
+				SkipDiscovery: true,
+				JwksURL:       msKeysURL,
+			},
+		}
+
+		pd, err := newProviderDataFromConfig(providerConfig)
+		g.Expect(err).ToNot(HaveOccurred())
+
+		g.Expect(pd.Scope).To(Equal(tc.expectedScope))
+	}
+}