Advertise browser_authcode flow in ldap idp discovery

To keep this backwards compatible, this PR changes how
the cli deals with ambiguous flows. Previously, if there
was more than one flow advertised, the cli would require users
to set the flag --upstream-identity-provider-flow. Now it
chooses the first one in the list.

Signed-off-by: Margo Crawford <margaretc@vmware.com>

cmd/pinniped/cmd/kubeconfig.go

@@ -233,7 +233,7 @@ func runGetKubeconfig(ctx context.Context, out io.Writer, deps kubeconfigDeps, f
 	// When all the upstream IDP flags are set by the user, then skip discovery and don't validate their input. Maybe they know something
 	// that we can't know, like the name of an IDP that they are going to define in the future.
 	if len(flags.oidc.issuer) > 0 && (flags.oidc.upstreamIDPType == "" || flags.oidc.upstreamIDPName == "" || flags.oidc.upstreamIDPFlow == "") {
-		if err := discoverSupervisorUpstreamIDP(ctx, &flags); err != nil {
+		if err := discoverSupervisorUpstreamIDP(ctx, &flags, deps.log); err != nil {
 			return err
 		}
 	}
@@ -726,7 +726,7 @@ func hasPendingStrategy(credentialIssuer *configv1alpha1.CredentialIssuer) bool
 	return false
 }
 
-func discoverSupervisorUpstreamIDP(ctx context.Context, flags *getKubeconfigParams) error {
+func discoverSupervisorUpstreamIDP(ctx context.Context, flags *getKubeconfigParams, log logr.Logger) error {
 	httpClient, err := newDiscoveryHTTPClient(flags.oidc.caBundle)
 	if err != nil {
 		return err
@@ -758,7 +758,7 @@ func discoverSupervisorUpstreamIDP(ctx context.Context, flags *getKubeconfigPara
 		return err
 	}
 
-	selectedIDPFlow, err := selectUpstreamIDPFlow(discoveredIDPFlows, selectedIDPName, selectedIDPType, flags.oidc.upstreamIDPFlow)
+	selectedIDPFlow, err := selectUpstreamIDPFlow(discoveredIDPFlows, selectedIDPName, selectedIDPType, flags.oidc.upstreamIDPFlow, log)
 	if err != nil {
 		return err
 	}
@@ -898,7 +898,7 @@ func selectUpstreamIDPNameAndType(pinnipedIDPs []idpdiscoveryv1alpha1.PinnipedID
 	}
 }
 
-func selectUpstreamIDPFlow(discoveredIDPFlows []idpdiscoveryv1alpha1.IDPFlow, selectedIDPName string, selectedIDPType idpdiscoveryv1alpha1.IDPType, specifiedFlow string) (idpdiscoveryv1alpha1.IDPFlow, error) {
+func selectUpstreamIDPFlow(discoveredIDPFlows []idpdiscoveryv1alpha1.IDPFlow, selectedIDPName string, selectedIDPType idpdiscoveryv1alpha1.IDPType, specifiedFlow string, log logr.Logger) (idpdiscoveryv1alpha1.IDPFlow, error) {
 	switch {
 	case len(discoveredIDPFlows) == 0:
 		// No flows listed by discovery means that we are talking to an old Supervisor from before this feature existed.
@@ -922,10 +922,7 @@ func selectUpstreamIDPFlow(discoveredIDPFlows []idpdiscoveryv1alpha1.IDPFlow, se
 		return discoveredIDPFlows[0], nil
 	default:
 		// The user did not specify a flow, and more than one was found.
-		return "", fmt.Errorf(
+		log.Info("multiple client flows found, selecting first value as default: "+discoveredIDPFlows[0].String(), "idpName", selectedIDPName, "idpType", selectedIDPType)
-			"multiple client flows for Supervisor upstream identity provider %q of type %q were found, "+
+		return discoveredIDPFlows[0], nil
-				"so the --upstream-identity-provider-flow flag must be specified. "+
-				"Found these flows: %v",
-			selectedIDPName, selectedIDPType, discoveredIDPFlows)
 	}
 }

cmd/pinniped/cmd/kubeconfig_test.go

@@ -1261,13 +1261,51 @@ func TestGetKubeconfig(t *testing.T) {
 			oidcDiscoveryResponse: happyOIDCDiscoveryResponse,
 			idpsDiscoveryResponse: here.Docf(`{
 				"pinniped_identity_providers": [
-					{"name": "some-oidc-idp", "type": "oidc", "flows": ["flow1", "flow2"]}
+					{"name": "some-ldap-idp", "type": "ldap", "flows": ["cli_password", "flow2"]}
 				]
 			}`),
-			wantError: true,
+			wantStdout: func(issuerCABundle string, issuerURL string) string {
-			wantStderr: func(issuerCABundle string, issuerURL string) string {
+				return here.Docf(`
-				return `Error: multiple client flows for Supervisor upstream identity provider "some-oidc-idp" of type "oidc" were found, so the --upstream-identity-provider-flow flag must be specified.` +
+					apiVersion: v1
-					` Found these flows: [flow1 flow2]` + "\n"
+					clusters:
+					- cluster:
+						certificate-authority-data: ZmFrZS1jZXJ0aWZpY2F0ZS1hdXRob3JpdHktZGF0YS12YWx1ZQ==
+						server: https://fake-server-url-value
+					  name: kind-cluster-pinniped
+					contexts:
+					- context:
+						cluster: kind-cluster-pinniped
+						user: kind-user-pinniped
+					  name: kind-context-pinniped
+					current-context: kind-context-pinniped
+					kind: Config
+					preferences: {}
+					users:
+					- name: kind-user-pinniped
+					  user:
+						exec:
+						  apiVersion: client.authentication.k8s.io/v1beta1
+						  args:
+						  - login
+						  - oidc
+						  - --issuer=%s
+						  - --client-id=pinniped-cli
+						  - --scopes=offline_access,openid,pinniped:request-audience
+						  - --ca-bundle-data=%s
+						  - --upstream-identity-provider-name=some-ldap-idp
+						  - --upstream-identity-provider-type=ldap
+						  - --upstream-identity-provider-flow=cli_password
+						  command: '.../path/to/pinniped'
+						  env: []
+						  installHint: The pinniped CLI does not appear to be installed.  See https://get.pinniped.dev/cli
+             for more details
+						  provideClusterInfo: true
+					`,
+					issuerURL,
+					base64.StdEncoding.EncodeToString([]byte(issuerCABundle)))
+			},
+			wantLogs: func(_ string, _ string) []string {
+				return []string{"\"level\"=0 \"msg\"=\"multiple client flows found, selecting first value as default: cli_password\"  \"idpName\"=\"some-ldap-idp\" \"idpType\"=\"ldap\""}
 			},
 		},
 		{

internal/oidc/idpdiscovery/idp_discovery_handler.go

@@ -1,4 +1,4 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package idpdiscovery provides a handler for the upstream IDP discovery endpoint.
@@ -44,14 +44,14 @@ func responseAsJSON(upstreamIDPs oidc.UpstreamIdentityProvidersLister) ([]byte,
 		r.PinnipedIDPs = append(r.PinnipedIDPs, v1alpha1.PinnipedIDP{
 			Name:  provider.GetName(),
 			Type:  v1alpha1.IDPTypeLDAP,
-			Flows: []v1alpha1.IDPFlow{v1alpha1.IDPFlowCLIPassword},
+			Flows: []v1alpha1.IDPFlow{v1alpha1.IDPFlowCLIPassword, v1alpha1.IDPFlowBrowserAuthcode},
 		})
 	}
 	for _, provider := range upstreamIDPs.GetActiveDirectoryIdentityProviders() {
 		r.PinnipedIDPs = append(r.PinnipedIDPs, v1alpha1.PinnipedIDP{
 			Name:  provider.GetName(),
 			Type:  v1alpha1.IDPTypeActiveDirectory,
-			Flows: []v1alpha1.IDPFlow{v1alpha1.IDPFlowCLIPassword},
+			Flows: []v1alpha1.IDPFlow{v1alpha1.IDPFlowCLIPassword, v1alpha1.IDPFlowBrowserAuthcode},
 		})
 	}
 	for _, provider := range upstreamIDPs.GetOIDCIdentityProviders() {

internal/oidc/idpdiscovery/idp_discovery_handler_test.go

@@ -1,4 +1,4 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package idpdiscovery
@@ -37,22 +37,22 @@ func TestIDPDiscovery(t *testing.T) {
 			wantContentType: "application/json",
 			wantFirstResponseBodyJSON: here.Doc(`{
 				"pinniped_identity_providers": [
-					{"name": "a-some-ldap-idp", "type": "ldap",            "flows": ["cli_password"]},
+					{"name": "a-some-ldap-idp", "type": "ldap",            "flows": ["cli_password", "browser_authcode"]},
 					{"name": "a-some-oidc-idp", "type": "oidc",            "flows": ["browser_authcode"]},
-					{"name": "x-some-idp",      "type": "ldap",            "flows": ["cli_password"]},
+					{"name": "x-some-idp",      "type": "ldap",            "flows": ["cli_password", "browser_authcode"]},
 					{"name": "x-some-idp",      "type": "oidc",            "flows": ["browser_authcode"]},
-					{"name": "y-some-ad-idp",   "type": "activedirectory", "flows": ["cli_password"]},
+					{"name": "y-some-ad-idp",   "type": "activedirectory", "flows": ["cli_password", "browser_authcode"]},
-					{"name": "z-some-ad-idp",   "type": "activedirectory", "flows": ["cli_password"]},
+					{"name": "z-some-ad-idp",   "type": "activedirectory", "flows": ["cli_password", "browser_authcode"]},
-					{"name": "z-some-ldap-idp", "type": "ldap",            "flows": ["cli_password"]},
+					{"name": "z-some-ldap-idp", "type": "ldap",            "flows": ["cli_password", "browser_authcode"]},
 					{"name": "z-some-oidc-idp", "type": "oidc",            "flows": ["browser_authcode", "cli_password"]}
 				]
 			}`),
 			wantSecondResponseBodyJSON: here.Doc(`{
 				"pinniped_identity_providers": [
-					{"name": "some-other-ad-idp-1",   "type": "activedirectory", "flows": ["cli_password"]},
+					{"name": "some-other-ad-idp-1",   "type": "activedirectory", "flows": ["cli_password", "browser_authcode"]},
-					{"name": "some-other-ad-idp-2",   "type": "activedirectory", "flows": ["cli_password"]},
+					{"name": "some-other-ad-idp-2",   "type": "activedirectory", "flows": ["cli_password", "browser_authcode"]},
-					{"name": "some-other-ldap-idp-1", "type": "ldap",            "flows": ["cli_password"]},
+					{"name": "some-other-ldap-idp-1", "type": "ldap",            "flows": ["cli_password", "browser_authcode"]},
-					{"name": "some-other-ldap-idp-2", "type": "ldap",            "flows": ["cli_password"]},
+					{"name": "some-other-ldap-idp-2", "type": "ldap",            "flows": ["cli_password", "browser_authcode"]},
 					{"name": "some-other-oidc-idp-1", "type": "oidc",            "flows": ["browser_authcode", "cli_password"]},
 					{"name": "some-other-oidc-idp-2", "type": "oidc",            "flows": ["browser_authcode"]}
 				]