From 318a9251bde4950e0bc4c565f699bef2929d6df9 Mon Sep 17 00:00:00 2001 From: Tom Wieczorek Date: Fri, 23 Aug 2019 02:41:36 +0200 Subject: [PATCH] Better detection for authorization_code OIDC response type (#2164) Currently, the authorization_code flow is only chosen if either a client secret is present, or if it is the only supported response type by the Identity Provider (which was a special case for dex). If a public OIDC client is used (i.e. a client without a secret) which supports more than just the 'code' flow, implicit mode is preferred. Change the flow detection to properly check if the 'code' flow is supported, and, if it's available, prefer it in any case over the implicit flow since one cannot obtain Refresh Tokens that way, which means that users need to re-authenticate every time the ID Token expires. --- cmd/argocd/commands/login.go | 2 +- util/oidc/oidc.go | 23 ++++++++------- util/oidc/oidc_test.go | 54 +++++++++++++----------------------- 3 files changed, 32 insertions(+), 47 deletions(-) diff --git a/cmd/argocd/commands/login.go b/cmd/argocd/commands/login.go index 94449f1eae..7aecaca335 100644 --- a/cmd/argocd/commands/login.go +++ b/cmd/argocd/commands/login.go @@ -243,7 +243,7 @@ func oauth2Login(ctx context.Context, port int, oauth2conf *oauth2.Config, provi fmt.Printf("Opening browser for authentication\n") var url string - grantType := oidcutil.InferGrantType(oauth2conf, oidcConf) + grantType := oidcutil.InferGrantType(oidcConf) switch grantType { case oidcutil.GrantTypeAuthorizationCode: url = oauth2conf.AuthCodeURL(stateNonce, oauth2.AccessTypeOffline) diff --git a/util/oidc/oidc.go b/util/oidc/oidc.go index 6c4657e53a..a8a5e9fcc3 100644 --- a/util/oidc/oidc.go +++ b/util/oidc/oidc.go @@ -175,7 +175,7 @@ func (a *ClientApp) HandleLogin(w http.ResponseWriter, r *http.Request) { } returnURL := r.FormValue("return_url") stateNonce := a.generateAppState(returnURL) - grantType := InferGrantType(oauth2Config, oidcConf) + grantType := InferGrantType(oidcConf) var url string switch grantType { case GrantTypeAuthorizationCode: @@ -358,17 +358,16 @@ func OfflineAccess(scopes []string) bool { // InferGrantType infers the proper grant flow depending on the OAuth2 client config and OIDC configuration. // Returns either: "authorization_code" or "implicit" -func InferGrantType(oauth2conf *oauth2.Config, oidcConf *OIDCConfiguration) string { - if oauth2conf.ClientSecret != "" { - // If we know the client secret, we are using the 'authorization_code' flow - return GrantTypeAuthorizationCode +func InferGrantType(oidcConf *OIDCConfiguration) string { + // Check the supported response types. If the list contains the response type 'code', + // then grant type is 'authorization_code'. This is preferred over the implicit + // grant type since refresh tokens cannot be issued that way. + for _, supportedType := range oidcConf.ResponseTypesSupported { + if supportedType == ResponseTypeCode { + return GrantTypeAuthorizationCode + } } - if len(oidcConf.ResponseTypesSupported) == 1 && oidcConf.ResponseTypesSupported[0] == ResponseTypeCode { - // If we don't have the secret, check the supported response types. If the list is a single - // response type of type 'code', then grant type is 'authorization_code'. This is the Dex - // case, which does not support implicit login flow (https://github.com/dexidp/dex/issues/1254) - return GrantTypeAuthorizationCode - } - // If we don't have the client secret (e.g. SPA app), we can assume to be implicit + + // Assume implicit otherwise return GrantTypeImplicit } diff --git a/util/oidc/oidc_test.go b/util/oidc/oidc_test.go index 47ed7cdedc..33ace0b85b 100644 --- a/util/oidc/oidc_test.go +++ b/util/oidc/oidc_test.go @@ -6,43 +6,29 @@ import ( "testing" "github.com/stretchr/testify/assert" - "golang.org/x/oauth2" -) - -var ( - spaOauth2Conf = &oauth2.Config{ - ClientID: "spa-id", - } - - webOauth2Conf = &oauth2.Config{ - ClientID: "spa-id", - ClientSecret: "my-super-secret", - } ) func TestInferGrantType(t *testing.T) { - var grantType string - dexRAW, err := ioutil.ReadFile("testdata/dex.json") - assert.NoError(t, err) - var dexConfig OIDCConfiguration - err = json.Unmarshal(dexRAW, &dexConfig) - assert.NoError(t, err) - grantType = InferGrantType(spaOauth2Conf, &dexConfig) - // Dex does not support implicit login flow (https://github.com/dexidp/dex/issues/1254) - assert.Equal(t, GrantTypeAuthorizationCode, grantType) - grantType = InferGrantType(webOauth2Conf, &dexConfig) - assert.Equal(t, GrantTypeAuthorizationCode, grantType) + for _, path := range []string{"dex", "okta", "auth0", "onelogin"} { + t.Run(path, func(t *testing.T) { + rawConfig, err := ioutil.ReadFile("testdata/" + path + ".json") + assert.NoError(t, err) + var config OIDCConfiguration + err = json.Unmarshal(rawConfig, &config) + assert.NoError(t, err) + grantType := InferGrantType(&config) + assert.Equal(t, GrantTypeAuthorizationCode, grantType) - testFiles := []string{"testdata/okta.json", "testdata/auth0.json", "testdata/onelogin.json"} - for _, path := range testFiles { - oktaRAW, err := ioutil.ReadFile(path) - assert.NoError(t, err) - var oktaConfig OIDCConfiguration - err = json.Unmarshal(oktaRAW, &oktaConfig) - assert.NoError(t, err) - grantType = InferGrantType(spaOauth2Conf, &oktaConfig) - assert.Equal(t, GrantTypeImplicit, grantType) - grantType = InferGrantType(webOauth2Conf, &oktaConfig) - assert.Equal(t, GrantTypeAuthorizationCode, grantType) + var noCodeResponseTypes []string + for _, supportedResponseType := range config.ResponseTypesSupported { + if supportedResponseType != ResponseTypeCode { + noCodeResponseTypes = append(noCodeResponseTypes, supportedResponseType) + } + } + + config.ResponseTypesSupported = noCodeResponseTypes + grantType = InferGrantType(&config) + assert.Equal(t, GrantTypeImplicit, grantType) + }) } }