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

@@ -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)

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
 }

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)
+		})
 	}
 }