From 4eb2a35aa88127c372be73466dd4d61f0430c403 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Wed, 16 Feb 2022 16:47:55 +0000 Subject: [PATCH] Fix provider data initialisation --- CHANGELOG.md | 1 + providers/providers.go | 20 ++++----- providers/providers_test.go | 88 +++++++++++++++++++++++++++++++++++-- 3 files changed, 95 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 33ebd727..21bf262e 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/providers/providers.go b/providers/providers.go index a4699e65..84dcc97f 100644 --- a/providers/providers.go +++ b/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 == "" { diff --git a/providers/providers_test.go b/providers/providers_test.go index dfa6a989..38917b70 100644 --- a/providers/providers_test.go +++ b/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)) + } +}