Pinniped CLI and the oidc-client package are now enhanced by pinniped_supported_identity_provider_types

Co-authored-by: Joshua Casey <joshuatcasey@gmail.com>

cmd/pinniped/cmd/kubeconfig_test.go

@@ -908,7 +908,7 @@ func TestGetKubeconfig(t *testing.T) {
 			idpsDiscoveryResponse: here.Docf(`{
 				"pinniped_identity_providers": [
 					{"name": "some-ldap-idp", "type": "ldap"},
-					{"name": "some-oidc-idp", "type": "oidc"}
+					{"name": "some-oidc-idp", "type": "oidc", "flows": ["flow1", "flow2"]}
 				]
 			}`),
 			wantLogs: func(issuerCABundle string, issuerURL string) []string {
@@ -927,7 +927,7 @@ func TestGetKubeconfig(t *testing.T) {
 			wantStderr: func(issuerCABundle string, issuerURL string) testutil.RequireErrorStringFunc {
 				return testutil.WantExactErrorString(`Error: multiple Supervisor upstream identity providers were found, ` +
 					`so the --upstream-identity-provider-name/--upstream-identity-provider-type flags must be specified. ` +
-					`Found these upstreams: [{"name":"some-ldap-idp","type":"ldap"},{"name":"some-oidc-idp","type":"oidc"}]` + "\n")
+					`Found these upstreams: [{"name":"some-ldap-idp","type":"ldap"},{"name":"some-oidc-idp","type":"oidc","flows":["flow1","flow2"]}]` + "\n")
 			},
 		},
 		{

cmd/pinniped/cmd/login_oidc.go

@@ -12,7 +12,6 @@ import (
 	"net/http"
 	"os"
 	"path/filepath"
-	"strings"
 	"time"
 
 	"github.com/spf13/cobra"
@@ -59,9 +58,10 @@ func init() {
 }
 
 type oidcLoginCommandDeps struct {
-	lookupEnv     func(string) (string, bool)
-	login         func(string, string, ...oidcclient.Option) (*oidctypes.Token, error)
-	exchangeToken func(context.Context, *conciergeclient.Client, string) (*clientauthv1beta1.ExecCredential, error)
+	lookupEnv      func(string) (string, bool)
+	login          func(string, string, ...oidcclient.Option) (*oidctypes.Token, error)
+	exchangeToken  func(context.Context, *conciergeclient.Client, string) (*clientauthv1beta1.ExecCredential, error)
+	optionsFactory OIDCClientOptions
 }
 
 func oidcLoginCommandRealDeps() oidcLoginCommandDeps {
@@ -71,6 +71,7 @@ func oidcLoginCommandRealDeps() oidcLoginCommandDeps {
 		exchangeToken: func(ctx context.Context, client *conciergeclient.Client, token string) (*clientauthv1beta1.ExecCredential, error) {
 			return client.ExchangeToken(ctx, token)
 		},
+		optionsFactory: &clientOptions{},
 	}
 }
 
@@ -175,39 +176,37 @@ func runOIDCLogin(cmd *cobra.Command, deps oidcLoginCommandDeps, flags oidcLogin
 
 	// Initialize the login handler.
 	opts := []oidcclient.Option{
-		oidcclient.WithContext(cmd.Context()),
-		oidcclient.WithLogger(plog.Logr()), //nolint:staticcheck // old code with lots of log statements
-		oidcclient.WithScopes(flags.scopes),
-		oidcclient.WithSessionCache(sessionCache),
+		deps.optionsFactory.WithContext(cmd.Context()),
+		deps.optionsFactory.WithLogger(plog.Logr()), //nolint:staticcheck // old code with lots of log statements
+		deps.optionsFactory.WithScopes(flags.scopes),
+		deps.optionsFactory.WithSessionCache(sessionCache),
 	}
 
 	skipPrintLoginURL, _ := deps.lookupEnv(skipPrintLoginURLEnvVarName)
 	if skipPrintLoginURL == envVarTruthyValue {
-		opts = append(opts, oidcclient.WithSkipPrintLoginURL())
+		opts = append(opts, deps.optionsFactory.WithSkipPrintLoginURL())
 	}
 
 	if flags.listenPort != 0 {
-		opts = append(opts, oidcclient.WithListenPort(flags.listenPort))
+		opts = append(opts, deps.optionsFactory.WithListenPort(flags.listenPort))
 	}
 
 	if flags.requestAudience != "" {
-		opts = append(opts, oidcclient.WithRequestAudience(flags.requestAudience))
+		opts = append(opts, deps.optionsFactory.WithRequestAudience(flags.requestAudience))
 	}
 
 	if flags.upstreamIdentityProviderName != "" {
-		opts = append(opts, oidcclient.WithUpstreamIdentityProvider(
+		opts = append(opts, deps.optionsFactory.WithUpstreamIdentityProvider(
 			flags.upstreamIdentityProviderName, flags.upstreamIdentityProviderType))
 	}
 
-	flowOpts, err := flowOptions(
-		idpdiscoveryv1alpha1.IDPType(flags.upstreamIdentityProviderType),
-		idpdiscoveryv1alpha1.IDPFlow(flags.upstreamIdentityProviderFlow),
-		deps,
-	)
-	if err != nil {
-		return err
+	requestedFlow, flowSource := idpdiscoveryv1alpha1.IDPFlow(flags.upstreamIdentityProviderFlow), "--upstream-identity-provider-flow"
+	if flowOverride, hasFlowOverride := deps.lookupEnv(upstreamIdentityProviderFlowEnvVarName); hasFlowOverride {
+		requestedFlow, flowSource = idpdiscoveryv1alpha1.IDPFlow(flowOverride), upstreamIdentityProviderFlowEnvVarName
+	}
+	if requestedFlow != "" {
+		opts = append(opts, deps.optionsFactory.WithLoginFlow(requestedFlow, flowSource))
 	}
-	opts = append(opts, flowOpts...)
 
 	var concierge *conciergeclient.Client
 	if flags.conciergeEnabled {
@@ -225,12 +224,12 @@ func runOIDCLogin(cmd *cobra.Command, deps oidcLoginCommandDeps, flags oidcLogin
 
 	// --skip-browser skips opening the browser.
 	if flags.skipBrowser {
-		opts = append(opts, oidcclient.WithSkipBrowserOpen())
+		opts = append(opts, deps.optionsFactory.WithSkipBrowserOpen())
 	}
 
 	// --skip-listen skips starting the localhost callback listener.
 	if flags.skipListen {
-		opts = append(opts, oidcclient.WithSkipListen())
+		opts = append(opts, deps.optionsFactory.WithSkipListen())
 	}
 
 	if len(flags.caBundlePaths) > 0 || len(flags.caBundleData) > 0 {
@@ -238,7 +237,7 @@ func runOIDCLogin(cmd *cobra.Command, deps oidcLoginCommandDeps, flags oidcLogin
 		if err != nil {
 			return err
 		}
-		opts = append(opts, oidcclient.WithClient(client))
+		opts = append(opts, deps.optionsFactory.WithClient(client))
 	}
 	// Look up cached credentials based on a hash of all the CLI arguments and the cluster info.
 	cacheKey := struct {
@@ -288,60 +287,6 @@ func runOIDCLogin(cmd *cobra.Command, deps oidcLoginCommandDeps, flags oidcLogin
 	return json.NewEncoder(cmd.OutOrStdout()).Encode(cred)
 }
 
-func flowOptions(
-	requestedIDPType idpdiscoveryv1alpha1.IDPType,
-	requestedFlow idpdiscoveryv1alpha1.IDPFlow,
-	deps oidcLoginCommandDeps,
-) ([]oidcclient.Option, error) {
-	useCLIFlow := []oidcclient.Option{oidcclient.WithCLISendingCredentials()}
-
-	// If the env var is set to override the --upstream-identity-provider-type flag, then override it.
-	flowOverride, hasFlowOverride := deps.lookupEnv(upstreamIdentityProviderFlowEnvVarName)
-	flowSource := "--upstream-identity-provider-flow"
-	if hasFlowOverride {
-		requestedFlow = idpdiscoveryv1alpha1.IDPFlow(flowOverride)
-		flowSource = upstreamIdentityProviderFlowEnvVarName
-	}
-
-	switch requestedIDPType {
-	case idpdiscoveryv1alpha1.IDPTypeOIDC:
-		switch requestedFlow {
-		case idpdiscoveryv1alpha1.IDPFlowCLIPassword:
-			return useCLIFlow, nil
-		case idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode, "":
-			return nil, nil // browser authcode flow is the default Option, so don't need to return an Option here
-		default:
-			return nil, fmt.Errorf(
-				"%s value not recognized for identity provider type %q: %s (supported values: %s)",
-				flowSource, requestedIDPType, requestedFlow,
-				strings.Join([]string{idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode.String(), idpdiscoveryv1alpha1.IDPFlowCLIPassword.String()}, ", "))
-		}
-	case idpdiscoveryv1alpha1.IDPTypeLDAP, idpdiscoveryv1alpha1.IDPTypeActiveDirectory:
-		switch requestedFlow {
-		case idpdiscoveryv1alpha1.IDPFlowCLIPassword, "":
-			return useCLIFlow, nil
-		case idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode:
-			return nil, nil // browser authcode flow is the default Option, so don't need to return an Option here
-		default:
-			return nil, fmt.Errorf(
-				"%s value not recognized for identity provider type %q: %s (supported values: %s)",
-				flowSource, requestedIDPType, requestedFlow,
-				strings.Join([]string{idpdiscoveryv1alpha1.IDPFlowCLIPassword.String(), idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode.String()}, ", "))
-		}
-	default:
-		// Surprisingly cobra does not support this kind of flag validation. See https://github.com/spf13/pflag/issues/236
-		return nil, fmt.Errorf(
-			"--upstream-identity-provider-type value not recognized: %s (supported values: %s)",
-			requestedIDPType,
-			strings.Join([]string{
-				idpdiscoveryv1alpha1.IDPTypeOIDC.String(),
-				idpdiscoveryv1alpha1.IDPTypeLDAP.String(),
-				idpdiscoveryv1alpha1.IDPTypeActiveDirectory.String(),
-			}, ", "),
-		)
-	}
-}
-
 func makeClient(caBundlePaths []string, caBundleData []string) (*http.Client, error) {
 	pool := x509.NewCertPool()
 	for _, p := range caBundlePaths {

cmd/pinniped/cmd/login_oidc_test.go

@@ -14,12 +14,16 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/require"
+	"go.uber.org/mock/gomock"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clientauthv1beta1 "k8s.io/client-go/pkg/apis/clientauthentication/v1beta1"
 	clocktesting "k8s.io/utils/clock/testing"
 
+	idpdiscoveryv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1"
+	oidcapi "go.pinniped.dev/generated/latest/apis/supervisor/oidc"
 	"go.pinniped.dev/internal/certauthority"
 	"go.pinniped.dev/internal/here"
+	"go.pinniped.dev/internal/mocks/mockoidcclientoptions"
 	"go.pinniped.dev/internal/plog"
 	"go.pinniped.dev/internal/testutil"
 	"go.pinniped.dev/pkg/conciergeclient"
@@ -42,6 +46,13 @@ func TestLoginOIDCCommand(t *testing.T) {
 	require.NoError(t, err)
 	nowStr := now.Local().Format(time.RFC1123)
 
+	defaultWantedOptions := func(f *mockoidcclientoptions.MockOIDCClientOptions) {
+		f.EXPECT().WithContext(gomock.Any())
+		f.EXPECT().WithLogger(gomock.Any())
+		f.EXPECT().WithScopes([]string{oidcapi.ScopeOfflineAccess, oidcapi.ScopeOpenID, oidcapi.ScopeRequestAudience, oidcapi.ScopeUsername, oidcapi.ScopeGroups})
+		f.EXPECT().WithSessionCache(gomock.Any())
+	}
+
 	tests := []struct {
 		name             string
 		args             []string
@@ -51,6 +62,7 @@ func TestLoginOIDCCommand(t *testing.T) {
 		wantError        bool
 		wantStdout       string
 		wantStderr       string
+		wantOptions      func(f *mockoidcclientoptions.MockOIDCClientOptions)
 		wantOptionsCount int
 		wantLogs         []string
 	}{
@@ -109,7 +121,8 @@ func TestLoginOIDCCommand(t *testing.T) {
 				"--issuer", "test-issuer",
 				"--enable-concierge",
 			},
-			wantError: true,
+			wantOptions: defaultWantedOptions,
+			wantError:   true,
 			wantStderr: here.Doc(`
 				Error: invalid Concierge parameters: endpoint must not be empty
 			`),
@@ -121,7 +134,8 @@ func TestLoginOIDCCommand(t *testing.T) {
 				"--issuer", "test-issuer",
 				"--ca-bundle", "./does/not/exist",
 			},
-			wantError: true,
+			wantOptions: defaultWantedOptions,
+			wantError:   true,
 			wantStderr: here.Doc(`
 				Error: could not read --ca-bundle: open ./does/not/exist: no such file or directory
 			`),
@@ -133,7 +147,8 @@ func TestLoginOIDCCommand(t *testing.T) {
 				"--issuer", "test-issuer",
 				"--ca-bundle-data", "invalid-base64",
 			},
-			wantError: true,
+			wantOptions: defaultWantedOptions,
+			wantError:   true,
 			wantStderr: here.Doc(`
 				Error: could not read --ca-bundle-data: illegal base64 data at input byte 7
 			`),
@@ -148,34 +163,12 @@ func TestLoginOIDCCommand(t *testing.T) {
 				"--concierge-authenticator-name", "test-authenticator",
 				"--concierge-endpoint", "https://127.0.0.1:1234/",
 			},
-			wantError: true,
+			wantOptions: defaultWantedOptions,
+			wantError:   true,
 			wantStderr: here.Doc(`
 				Error: invalid Concierge parameters: invalid API group suffix: a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')
 			`),
 		},
-		{
-			name: "invalid upstream type is an error",
-			args: []string{
-				"--issuer", "test-issuer",
-				"--upstream-identity-provider-type", "invalid",
-			},
-			wantError: true,
-			wantStderr: here.Doc(`
-				Error: --upstream-identity-provider-type value not recognized: invalid (supported values: oidc, ldap, activedirectory)
-			`),
-		},
-		{
-			name: "invalid upstream type when flow override env var is used is still an error",
-			args: []string{
-				"--issuer", "test-issuer",
-				"--upstream-identity-provider-type", "invalid",
-			},
-			env:       map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "browser_authcode"},
-			wantError: true,
-			wantStderr: here.Doc(`
-				Error: --upstream-identity-provider-type value not recognized: invalid (supported values: oidc, ldap, activedirectory)
-			`),
-		},
 		{
 			name: "oidc upstream type with default flow is allowed",
 			args: []string{
@@ -184,6 +177,7 @@ func TestLoginOIDCCommand(t *testing.T) {
 				"--upstream-identity-provider-type", "oidc",
 				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
 			},
+			wantOptions:      defaultWantedOptions,
 			wantOptionsCount: 4,
 			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n",
 		},
@@ -195,269 +189,45 @@ func TestLoginOIDCCommand(t *testing.T) {
 				"--upstream-identity-provider-type", "oidc",
 				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
 			},
-			env:              map[string]string{"PINNIPED_SKIP_PRINT_LOGIN_URL": "true"},
+			env: map[string]string{"PINNIPED_SKIP_PRINT_LOGIN_URL": "true"},
+			wantOptions: func(f *mockoidcclientoptions.MockOIDCClientOptions) {
+				defaultWantedOptions(f)
+				f.EXPECT().WithSkipPrintLoginURL()
+			},
 			wantOptionsCount: 5,
 			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n",
 		},
 		{
-			name: "oidc upstream type with CLI flow is allowed",
+			name: "--upstream-identity-provider-flow adds an option",
 			args: []string{
 				"--issuer", "test-issuer",
 				"--client-id", "test-client-id",
-				"--upstream-identity-provider-type", "oidc",
 				"--upstream-identity-provider-flow", "cli_password",
 				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
 			},
-			wantOptionsCount: 5,
-			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n",
-		},
-		{
-			name: "oidc upstream type with browser flow is allowed",
-			args: []string{
-				"--issuer", "test-issuer",
-				"--client-id", "test-client-id",
-				"--upstream-identity-provider-type", "oidc",
-				"--upstream-identity-provider-flow", "browser_authcode",
-				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
-			},
-			wantOptionsCount: 4,
-			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n",
-		},
-		{
-			name: "oidc upstream type with CLI flow in flow override env var is allowed",
-			args: []string{
-				"--issuer", "test-issuer",
-				"--client-id", "test-client-id",
-				"--upstream-identity-provider-type", "oidc",
-				"--upstream-identity-provider-flow", "browser_authcode",
-				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
-			},
-			env:              map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "cli_password"},
-			wantOptionsCount: 5,
-			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n",
-		},
-		{
-			name: "oidc upstream type with with browser flow in flow override env var is allowed",
-			args: []string{
-				"--issuer", "test-issuer",
-				"--client-id", "test-client-id",
-				"--upstream-identity-provider-type", "oidc",
-				"--upstream-identity-provider-flow", "cli_password",
-				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
-			},
-			env:              map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "browser_authcode"},
-			wantOptionsCount: 4,
-			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n",
-		},
-		{
-			name: "oidc upstream type with unsupported flow is an error",
-			args: []string{
-				"--issuer", "test-issuer",
-				"--client-id", "test-client-id",
-				"--upstream-identity-provider-type", "oidc",
-				"--upstream-identity-provider-flow", "foobar",
-				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
-			},
-			wantError: true,
-			wantStderr: here.Doc(`
-				Error: --upstream-identity-provider-flow value not recognized for identity provider type "oidc": foobar (supported values: browser_authcode, cli_password)
-			`),
-		},
-		{
-			name: "oidc upstream type with unsupported flow in flow override env var is an error",
-			args: []string{
-				"--issuer", "test-issuer",
-				"--client-id", "test-client-id",
-				"--upstream-identity-provider-type", "oidc",
-				"--upstream-identity-provider-flow", "browser_authcode",
-				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
-			},
-			env:       map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "foo"},
-			wantError: true,
-			wantStderr: here.Doc(`
-				Error: PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW value not recognized for identity provider type "oidc": foo (supported values: browser_authcode, cli_password)
-			`),
-		},
-		{
-			name: "ldap upstream type with default flow is allowed",
-			args: []string{
-				"--issuer", "test-issuer",
-				"--client-id", "test-client-id",
-				"--upstream-identity-provider-type", "ldap",
-				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
+			wantOptions: func(f *mockoidcclientoptions.MockOIDCClientOptions) {
+				defaultWantedOptions(f)
+				f.EXPECT().WithLoginFlow(idpdiscoveryv1alpha1.IDPFlowCLIPassword, "--upstream-identity-provider-flow")
 			},
 			wantOptionsCount: 5,
 			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n",
 		},
 		{
-			name: "activedirectory upstream type with default flow is allowed",
+			name: "PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW adds an option that overrides --upstream-identity-provider-flow",
 			args: []string{
 				"--issuer", "test-issuer",
 				"--client-id", "test-client-id",
-				"--upstream-identity-provider-type", "activedirectory",
+				"--upstream-identity-provider-flow", "ignored-value-from-param",
 				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
 			},
+			env: map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "actual-value-from-env"},
+			wantOptions: func(f *mockoidcclientoptions.MockOIDCClientOptions) {
+				defaultWantedOptions(f)
+				f.EXPECT().WithLoginFlow(idpdiscoveryv1alpha1.IDPFlow("actual-value-from-env"), "PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW")
+			},
 			wantOptionsCount: 5,
 			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n",
 		},
-		{
-			name: "ldap upstream type with CLI flow is allowed",
-			args: []string{
-				"--issuer", "test-issuer",
-				"--client-id", "test-client-id",
-				"--upstream-identity-provider-type", "ldap",
-				"--upstream-identity-provider-flow", "cli_password",
-				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
-			},
-			wantOptionsCount: 5,
-			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n",
-		},
-		{
-			name: "ldap upstream type with browser_authcode flow is allowed",
-			args: []string{
-				"--issuer", "test-issuer",
-				"--client-id", "test-client-id",
-				"--upstream-identity-provider-type", "ldap",
-				"--upstream-identity-provider-flow", "browser_authcode",
-				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
-			},
-			wantOptionsCount: 4,
-			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n",
-		},
-		{
-			name: "ldap upstream type with CLI flow in flow override env var is allowed",
-			args: []string{
-				"--issuer", "test-issuer",
-				"--client-id", "test-client-id",
-				"--upstream-identity-provider-type", "ldap",
-				"--upstream-identity-provider-flow", "browser_authcode",
-				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
-			},
-			env:              map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "cli_password"},
-			wantOptionsCount: 5,
-			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n",
-		},
-		{
-			name: "ldap upstream type with browser_authcode flow in flow override env var is allowed",
-			args: []string{
-				"--issuer", "test-issuer",
-				"--client-id", "test-client-id",
-				"--upstream-identity-provider-type", "ldap",
-				"--upstream-identity-provider-flow", "cli_password",
-				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
-			},
-			env:              map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "browser_authcode"},
-			wantOptionsCount: 4,
-			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n",
-		},
-		{
-			name: "ldap upstream type with unsupported flow is an error",
-			args: []string{
-				"--issuer", "test-issuer",
-				"--client-id", "test-client-id",
-				"--upstream-identity-provider-type", "ldap",
-				"--upstream-identity-provider-flow", "foo",
-				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
-			},
-			wantError: true,
-			wantStderr: here.Doc(`
-				Error: --upstream-identity-provider-flow value not recognized for identity provider type "ldap": foo (supported values: cli_password, browser_authcode)
-			`),
-		},
-		{
-			name: "ldap upstream type with unsupported flow in flow override env var is an error",
-			args: []string{
-				"--issuer", "test-issuer",
-				"--client-id", "test-client-id",
-				"--upstream-identity-provider-type", "ldap",
-				"--upstream-identity-provider-flow", "browser_authcode",
-				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
-			},
-			env:       map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "foo"},
-			wantError: true,
-			wantStderr: here.Doc(`
-				Error: PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW value not recognized for identity provider type "ldap": foo (supported values: cli_password, browser_authcode)
-			`),
-		},
-		{
-			name: "active directory upstream type with CLI flow is allowed",
-			args: []string{
-				"--issuer", "test-issuer",
-				"--client-id", "test-client-id",
-				"--upstream-identity-provider-type", "activedirectory",
-				"--upstream-identity-provider-flow", "cli_password",
-				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
-			},
-			wantOptionsCount: 5,
-			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n",
-		},
-		{
-			name: "active directory upstream type with browser_authcode is allowed",
-			args: []string{
-				"--issuer", "test-issuer",
-				"--client-id", "test-client-id",
-				"--upstream-identity-provider-type", "activedirectory",
-				"--upstream-identity-provider-flow", "browser_authcode",
-				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
-			},
-			wantOptionsCount: 4,
-			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n",
-		},
-		{
-			name: "active directory upstream type with CLI flow in flow override env var is allowed",
-			args: []string{
-				"--issuer", "test-issuer",
-				"--client-id", "test-client-id",
-				"--upstream-identity-provider-type", "activedirectory",
-				"--upstream-identity-provider-flow", "browser_authcode",
-				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
-			},
-			env:              map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "cli_password"},
-			wantOptionsCount: 5,
-			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n",
-		},
-		{
-			name: "active directory upstream type with browser_authcode in flow override env var is allowed",
-			args: []string{
-				"--issuer", "test-issuer",
-				"--client-id", "test-client-id",
-				"--upstream-identity-provider-type", "activedirectory",
-				"--upstream-identity-provider-flow", "cli_password",
-				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
-			},
-			env:              map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "browser_authcode"},
-			wantOptionsCount: 4,
-			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n",
-		},
-		{
-			name: "active directory upstream type with unsupported flow is an error",
-			args: []string{
-				"--issuer", "test-issuer",
-				"--client-id", "test-client-id",
-				"--upstream-identity-provider-type", "activedirectory",
-				"--upstream-identity-provider-flow", "foo",
-				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
-			},
-			wantError: true,
-			wantStderr: here.Doc(`
-				Error: --upstream-identity-provider-flow value not recognized for identity provider type "activedirectory": foo (supported values: cli_password, browser_authcode)
-			`),
-		},
-		{
-			name: "active directory upstream type with unsupported flow in flow override env var is an error",
-			args: []string{
-				"--issuer", "test-issuer",
-				"--client-id", "test-client-id",
-				"--upstream-identity-provider-type", "activedirectory",
-				"--upstream-identity-provider-flow", "browser_authcode",
-				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
-			},
-			env:       map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "foo"},
-			wantError: true,
-			wantStderr: here.Doc(`
-				Error: PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW value not recognized for identity provider type "activedirectory": foo (supported values: cli_password, browser_authcode)
-			`),
-		},
 		{
 			name: "login error",
 			args: []string{
@@ -466,6 +236,7 @@ func TestLoginOIDCCommand(t *testing.T) {
 				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
 			},
 			loginErr:         fmt.Errorf("some login error"),
+			wantOptions:      defaultWantedOptions,
 			wantOptionsCount: 4,
 			wantError:        true,
 			wantStderr: here.Doc(`
@@ -484,6 +255,7 @@ func TestLoginOIDCCommand(t *testing.T) {
 				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
 			},
 			conciergeErr:     fmt.Errorf("some concierge error"),
+			wantOptions:      defaultWantedOptions,
 			wantOptionsCount: 4,
 			wantError:        true,
 			wantStderr: here.Doc(`
@@ -498,11 +270,12 @@ func TestLoginOIDCCommand(t *testing.T) {
 				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
 			},
 			env:              map[string]string{"PINNIPED_DEBUG": "true"},
+			wantOptions:      defaultWantedOptions,
 			wantOptionsCount: 4,
 			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n",
 			wantLogs: []string{
-				nowStr + `  pinniped-login  cmd/login_oidc.go:260  Performing OIDC login  {"issuer": "test-issuer", "client id": "test-client-id"}`,
-				nowStr + `  pinniped-login  cmd/login_oidc.go:280  No concierge configured, skipping token credential exchange`,
+				nowStr + `  pinniped-login  cmd/login_oidc.go:259  Performing OIDC login  {"issuer": "test-issuer", "client id": "test-client-id"}`,
+				nowStr + `  pinniped-login  cmd/login_oidc.go:279  No concierge configured, skipping token credential exchange`,
 			},
 		},
 		{
@@ -526,15 +299,30 @@ func TestLoginOIDCCommand(t *testing.T) {
 				"--credential-cache", t.TempDir() + "/credentials.yaml", // must specify --credential-cache or else the cache file on disk causes test pollution
 				"--upstream-identity-provider-name", "some-upstream-name",
 				"--upstream-identity-provider-type", "ldap",
+				"--upstream-identity-provider-flow", "some-flow-type",
+			},
+			env: map[string]string{"PINNIPED_DEBUG": "true", "PINNIPED_SKIP_PRINT_LOGIN_URL": "true"},
+			wantOptions: func(f *mockoidcclientoptions.MockOIDCClientOptions) {
+				f.EXPECT().WithContext(gomock.Any())
+				f.EXPECT().WithLogger(gomock.Any())
+				f.EXPECT().WithScopes([]string{oidcapi.ScopeOfflineAccess, oidcapi.ScopeOpenID, oidcapi.ScopeRequestAudience, oidcapi.ScopeUsername, oidcapi.ScopeGroups})
+				f.EXPECT().WithSessionCache(gomock.Any())
+				f.EXPECT().WithListenPort(uint16(1234))
+				f.EXPECT().WithSkipBrowserOpen()
+				f.EXPECT().WithSkipListen()
+				f.EXPECT().WithSkipPrintLoginURL()
+				f.EXPECT().WithClient(gomock.Any())
+				f.EXPECT().WithRequestAudience("cluster-1234")
+				f.EXPECT().WithLoginFlow(idpdiscoveryv1alpha1.IDPFlow("some-flow-type"), "--upstream-identity-provider-flow")
+				f.EXPECT().WithUpstreamIdentityProvider("some-upstream-name", "ldap")
 			},
-			env:              map[string]string{"PINNIPED_DEBUG": "true", "PINNIPED_SKIP_PRINT_LOGIN_URL": "true"},
 			wantOptionsCount: 12,
 			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"token":"exchanged-token"}}` + "\n",
 			wantLogs: []string{
-				nowStr + `  pinniped-login  cmd/login_oidc.go:260  Performing OIDC login  {"issuer": "test-issuer", "client id": "test-client-id"}`,
-				nowStr + `  pinniped-login  cmd/login_oidc.go:270  Exchanging token for cluster credential  {"endpoint": "https://127.0.0.1:1234/", "authenticator type": "webhook", "authenticator name": "test-authenticator"}`,
-				nowStr + `  pinniped-login  cmd/login_oidc.go:278  Successfully exchanged token for cluster credential.`,
-				nowStr + `  pinniped-login  cmd/login_oidc.go:285  caching cluster credential for future use.`,
+				nowStr + `  pinniped-login  cmd/login_oidc.go:259  Performing OIDC login  {"issuer": "test-issuer", "client id": "test-client-id"}`,
+				nowStr + `  pinniped-login  cmd/login_oidc.go:269  Exchanging token for cluster credential  {"endpoint": "https://127.0.0.1:1234/", "authenticator type": "webhook", "authenticator name": "test-authenticator"}`,
+				nowStr + `  pinniped-login  cmd/login_oidc.go:277  Successfully exchanged token for cluster credential.`,
+				nowStr + `  pinniped-login  cmd/login_oidc.go:284  caching cluster credential for future use.`,
 			},
 		},
 	}
@@ -543,6 +331,13 @@ func TestLoginOIDCCommand(t *testing.T) {
 			var buf bytes.Buffer
 			ctx := plog.AddZapOverridesToContext(context.Background(), t, &buf, nil, clocktesting.NewFakeClock(now))
 
+			ctrl := gomock.NewController(t)
+			t.Cleanup(ctrl.Finish)
+			optionsFactory := mockoidcclientoptions.NewMockOIDCClientOptions(ctrl)
+			if tt.wantOptions != nil {
+				tt.wantOptions(optionsFactory)
+			}
+
 			var gotOptions []oidcclient.Option
 			cmd := oidcLoginCommand(oidcLoginCommandDeps{
 				lookupEnv: func(s string) (string, bool) {
@@ -578,6 +373,7 @@ func TestLoginOIDCCommand(t *testing.T) {
 						},
 					}, nil
 				},
+				optionsFactory: optionsFactory,
 			})
 			require.NotNil(t, cmd)
 

cmd/pinniped/cmd/oidc_client_options.go

@@ -0,0 +1,85 @@
+// Copyright 2024 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package cmd
+
+import (
+	"context"
+	"net/http"
+
+	"github.com/go-logr/logr"
+
+	"go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1"
+	"go.pinniped.dev/pkg/oidcclient"
+)
+
+// OIDCClientOptions is an interface that wraps the creation of Options for the purpose of making them
+// more friendly to unit tests. Because the Option type refers to a private struct type, it is hard
+// to create mocks for them in tests of other packages. This provides a seam that can be mocked.
+type OIDCClientOptions interface {
+	WithContext(ctx context.Context) oidcclient.Option
+	WithLogger(logger logr.Logger) oidcclient.Option
+	WithListenPort(port uint16) oidcclient.Option
+	WithSkipBrowserOpen() oidcclient.Option
+	WithSkipListen() oidcclient.Option
+	WithSkipPrintLoginURL() oidcclient.Option
+	WithSessionCache(cache oidcclient.SessionCache) oidcclient.Option
+	WithClient(httpClient *http.Client) oidcclient.Option
+	WithScopes(scopes []string) oidcclient.Option
+	WithRequestAudience(audience string) oidcclient.Option
+	WithLoginFlow(loginFlow v1alpha1.IDPFlow, flowSource string) oidcclient.Option
+	WithUpstreamIdentityProvider(upstreamName, upstreamType string) oidcclient.Option
+}
+
+// clientOptions implements OIDCClientOptions for production use.
+type clientOptions struct{}
+
+var _ OIDCClientOptions = (*clientOptions)(nil)
+
+func (o *clientOptions) WithContext(ctx context.Context) oidcclient.Option {
+	return oidcclient.WithContext(ctx)
+}
+
+func (o *clientOptions) WithLogger(logger logr.Logger) oidcclient.Option {
+	return oidcclient.WithLogger(logger)
+}
+
+func (o *clientOptions) WithListenPort(port uint16) oidcclient.Option {
+	return oidcclient.WithListenPort(port)
+}
+
+func (o *clientOptions) WithSkipBrowserOpen() oidcclient.Option {
+	return oidcclient.WithSkipBrowserOpen()
+}
+
+func (o *clientOptions) WithSkipListen() oidcclient.Option {
+	return oidcclient.WithSkipListen()
+}
+
+func (o *clientOptions) WithSkipPrintLoginURL() oidcclient.Option {
+	return oidcclient.WithSkipPrintLoginURL()
+}
+
+func (o *clientOptions) WithSessionCache(cache oidcclient.SessionCache) oidcclient.Option {
+	return oidcclient.WithSessionCache(cache)
+}
+
+func (o *clientOptions) WithClient(httpClient *http.Client) oidcclient.Option {
+	return oidcclient.WithClient(httpClient)
+}
+
+func (o *clientOptions) WithScopes(scopes []string) oidcclient.Option {
+	return oidcclient.WithScopes(scopes)
+}
+
+func (o *clientOptions) WithRequestAudience(audience string) oidcclient.Option {
+	return oidcclient.WithRequestAudience(audience)
+}
+
+func (o *clientOptions) WithLoginFlow(loginFlow v1alpha1.IDPFlow, flowSource string) oidcclient.Option {
+	return oidcclient.WithLoginFlow(loginFlow, flowSource)
+}
+
+func (o *clientOptions) WithUpstreamIdentityProvider(upstreamName, upstreamType string) oidcclient.Option {
+	return oidcclient.WithUpstreamIdentityProvider(upstreamName, upstreamType)
+}