Small refactors and comments for LDAP/AD UI

cmd/pinniped/cmd/kubeconfig.go

@@ -922,7 +922,9 @@ func selectUpstreamIDPFlow(discoveredIDPFlows []idpdiscoveryv1alpha1.IDPFlow, se
 		return discoveredIDPFlows[0], nil
 	default:
 		// The user did not specify a flow, and more than one was found.
-		log.Info("multiple client flows found, selecting first value as default: "+discoveredIDPFlows[0].String(), "idpName", selectedIDPName, "idpType", selectedIDPType)
+		log.Info("multiple client flows found, selecting first value as default",
+			"idpName", selectedIDPName, "idpType", selectedIDPType,
+			"selectedFlow", discoveredIDPFlows[0].String(), "availableFlows", discoveredIDPFlows)
 		return discoveredIDPFlows[0], nil
 	}
 }

cmd/pinniped/cmd/kubeconfig_test.go

@@ -1305,7 +1305,8 @@ func TestGetKubeconfig(t *testing.T) {
 					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\""}
+				return []string{`"level"=0 "msg"="multiple client flows found, selecting first value as default"  ` +
+					`"availableFlows"=["cli_password","flow2"] "idpName"="some-ldap-idp" "idpType"="ldap" "selectedFlow"="cli_password"`}
 			},
 		},
 		{

internal/oidc/auth/auth_handler.go

@@ -159,7 +159,7 @@ func handleAuthRequestForLDAPUpstreamBrowserFlow(
 	upstreamStateEncoder oidc.Encoder,
 	cookieCodec oidc.Codec,
 ) error {
-	encodedStateParamValue, _, _, err := handleBrowserAuthRequest(
+	authRequestState, err := handleBrowserFlowAuthRequest(
 		r,
 		w,
 		oauthHelper,
@@ -174,11 +174,12 @@ func handleAuthRequestForLDAPUpstreamBrowserFlow(
 	if err != nil {
 		return err
 	}
-	if encodedStateParamValue == "" {
+	if authRequestState == nil {
+		// There was an error but handleBrowserFlowAuthRequest() already took care of writing the response for it.
 		return nil
 	}
 
-	return login.RedirectToLoginPage(r, w, downstreamIssuer, encodedStateParamValue, login.ShowNoError)
+	return login.RedirectToLoginPage(r, w, downstreamIssuer, authRequestState.encodedStateParam, login.ShowNoError)
 }
 
 func handleAuthRequestForOIDCUpstreamPasswordGrant(
@@ -255,7 +256,7 @@ func handleAuthRequestForOIDCUpstreamBrowserFlow(
 	upstreamStateEncoder oidc.Encoder,
 	cookieCodec oidc.Codec,
 ) error {
-	encodedStateParamValue, pkceValue, nonceValue, err := handleBrowserAuthRequest(
+	authRequestState, err := handleBrowserFlowAuthRequest(
 		r,
 		w,
 		oauthHelper,
@@ -270,7 +271,8 @@ func handleAuthRequestForOIDCUpstreamBrowserFlow(
 	if err != nil {
 		return err
 	}
-	if encodedStateParamValue == "" {
+	if authRequestState == nil {
+		// There was an error but handleBrowserFlowAuthRequest() already took care of writing the response for it.
 		return nil
 	}
 
@@ -284,9 +286,9 @@ func handleAuthRequestForOIDCUpstreamBrowserFlow(
 	}
 
 	authCodeOptions := []oauth2.AuthCodeOption{
-		nonceValue.Param(),
-		pkceValue.Challenge(),
-		pkceValue.Method(),
+		authRequestState.nonce.Param(),
+		authRequestState.pkce.Challenge(),
+		authRequestState.pkce.Method(),
 	}
 
 	for key, val := range oidcUpstream.GetAdditionalAuthcodeParams() {
@@ -295,7 +297,7 @@ func handleAuthRequestForOIDCUpstreamBrowserFlow(
 
 	http.Redirect(w, r,
 		upstreamOAuthConfig.AuthCodeURL(
-			encodedStateParamValue,
+			authRequestState.encodedStateParam,
 			authCodeOptions...,
 		),
 		http.StatusSeeOther, // match fosite and https://tools.ietf.org/id/draft-ietf-oauth-security-topics-18.html#section-4.11
@@ -387,10 +389,21 @@ func chooseUpstreamIDP(idpLister oidc.UpstreamIdentityProvidersLister) (provider
 	}
 }
 
-// handleBrowserAuthRequest performs the shared validations and setup between browser based auth requests
-// regardless of IDP type-- LDAP, Active Directory and OIDC.
+type browserFlowAuthRequestState struct {
+	encodedStateParam string
+	pkce              pkce.Code
+	nonce             nonce.Nonce
+}
+
+// handleBrowserFlowAuthRequest performs the shared validations and setup between browser based
+// auth requests regardless of IDP type-- LDAP, Active Directory and OIDC.
 // It generates the state param, sets the CSRF cookie, and validates the prompt param.
-func handleBrowserAuthRequest(
+// It returns an error when it encounters an error without handling it, leaving it to
+// the caller to decide how to handle it.
+// It returns nil with no error when it encounters an error and also has already handled writing
+// the error response to the ResponseWriter, in which case the caller should not also try to
+// write the error response.
+func handleBrowserFlowAuthRequest(
 	r *http.Request,
 	w http.ResponseWriter,
 	oauthHelper fosite.OAuth2Provider,
@@ -401,10 +414,10 @@ func handleBrowserAuthRequest(
 	idpType psession.ProviderType,
 	cookieCodec oidc.Codec,
 	upstreamStateEncoder oidc.Encoder,
-) (string, pkce.Code, nonce.Nonce, error) {
+) (*browserFlowAuthRequestState, error) {
 	authorizeRequester, created := newAuthorizeRequest(r, w, oauthHelper, false)
 	if !created {
-		return "", "", "", nil
+		return nil, nil // already wrote the error response, don't return error
 	}
 
 	now := time.Now()
@@ -420,13 +433,13 @@ func handleBrowserAuthRequest(
 	})
 	if err != nil {
 		oidc.WriteAuthorizeError(w, oauthHelper, authorizeRequester, err, false)
-		return "", "", "", nil
+		return nil, nil // already wrote the error response, don't return error
 	}
 
 	csrfValue, nonceValue, pkceValue, err := generateValues(generateCSRF, generateNonce, generatePKCE)
 	if err != nil {
 		plog.Error("authorize generate error", err)
-		return "", "", "", err
+		return nil, err
 	}
 	csrfFromCookie := readCSRFCookie(r, cookieCodec)
 	if csrfFromCookie != "" {
@@ -444,13 +457,13 @@ func handleBrowserAuthRequest(
 	)
 	if err != nil {
 		plog.Error("authorize upstream state param error", err)
-		return "", "", "", err
+		return nil, err
 	}
 
 	promptParam := r.Form.Get(promptParamName)
 	if promptParam == promptParamNone && oidc.ScopeWasRequested(authorizeRequester, coreosoidc.ScopeOpenID) {
 		oidc.WriteAuthorizeError(w, oauthHelper, authorizeRequester, fosite.ErrLoginRequired, false)
-		return "", "", "", nil
+		return nil, nil // already wrote the error response, don't return error
 	}
 
 	if csrfFromCookie == "" {
@@ -458,10 +471,15 @@ func handleBrowserAuthRequest(
 		err = addCSRFSetCookieHeader(w, csrfValue, cookieCodec)
 		if err != nil {
 			plog.Error("error setting CSRF cookie", err)
-			return "", "", "", err
+			return nil, err
 		}
 	}
-	return encodedStateParamValue, pkceValue, nonceValue, nil
+
+	return &browserFlowAuthRequestState{
+		encodedStateParam: encodedStateParamValue,
+		pkce:              pkceValue,
+		nonce:             nonceValue,
+	}, nil
 }
 
 func generateValues(

internal/oidc/login/get_login_handler.go

@@ -15,12 +15,12 @@ const (
 	incorrectUsernameOrPasswordErrorMessage = "Incorrect username or password."
 )
 
-func NewGetHandler() HandlerFunc {
+func NewGetHandler(loginPath string) HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request, encodedState string, decodedState *oidc.UpstreamStateParamData) error {
 		alertMessage, hasAlert := getAlert(r)
 
 		pageInputs := &loginhtml.PageData{
-			PostPath:      r.URL.Path, // the path for POST is the same as for GET
+			PostPath:      loginPath,
 			State:         encodedState,
 			IDPName:       decodedState.UpstreamName,
 			HasAlertError: hasAlert,

internal/oidc/login/get_login_handler_test.go

@@ -96,7 +96,7 @@ func TestGetLogin(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
 
-			handler := NewGetHandler()
+			handler := NewGetHandler(testPath)
 			target := testPath + "?state=" + tt.encodedState
 			if tt.errParam != "" {
 				target += "&err=" + tt.errParam

internal/oidc/login/login_handler.go

@@ -94,6 +94,9 @@ func wrapSecurityHeaders(handler http.Handler) http.Handler {
 	})
 }
 
+// RedirectToLoginPage redirects to the GET /login page of the specified issuer.
+// The specified issuer should never end with a "/", which is validated by
+// provider.FederationDomainIssuer when the issuer string comes from that type.
 func RedirectToLoginPage(
 	r *http.Request,
 	w http.ResponseWriter,

internal/oidc/login/loginhtml/loginhtml.go

@@ -18,7 +18,7 @@ import (
 var (
 	//go:embed login_form.css
 	rawCSS      string
-	minifiedCSS = mustMinify(minify.CSS(rawCSS))
+	minifiedCSS = panicOnError(minify.CSS(rawCSS))
 
 	//go:embed login_form.gohtml
 	rawHTMLTemplate string
@@ -26,7 +26,7 @@ var (
 
 // Parse the Go templated HTML and inject functions providing the minified inline CSS and JS.
 var parsedHTMLTemplate = template.Must(template.New("login_form.gohtml").Funcs(template.FuncMap{
-	"minifiedCSS": func() template.CSS { return template.CSS(minifiedCSS) },
+	"minifiedCSS": func() template.CSS { return template.CSS(CSS()) },
 }).Parse(rawHTMLTemplate))
 
 // Generate the CSP header value once since it's effectively constant.
@@ -36,7 +36,7 @@ var cspValue = strings.Join([]string{
 	`frame-ancestors 'none'`,
 }, "; ")
 
-func mustMinify(s string, err error) string {
+func panicOnError(s string, err error) string {
 	if err != nil {
 		panic(err)
 	}

internal/oidc/login/loginhtml/loginhtml_test.go

@@ -63,6 +63,6 @@ func TestCSS(t *testing.T) {
 }
 
 func TestHelpers(t *testing.T) {
-	require.Equal(t, "test", mustMinify("test", nil))
-	require.PanicsWithError(t, "some error", func() { mustMinify("", fmt.Errorf("some error")) })
+	require.Equal(t, "test", panicOnError("test", nil))
+	require.PanicsWithError(t, "some error", func() { panicOnError("", fmt.Errorf("some error")) })
 }

internal/oidc/oidc.go

@@ -43,6 +43,9 @@ const (
 	// Just in case we need to make a breaking change to the format of the upstream state param,
 	// we are including a format version number. This gives the opportunity for a future version of Pinniped
 	// to have the consumer of this format decide to reject versions that it doesn't understand.
+	//
+	// Version 1 was the original version.
+	// Version 2 added the UpstreamType field to the UpstreamStateParamData struct.
 	UpstreamStateParamFormatVersion = "2"
 
 	// The `name` passed to the encoder for encoding the upstream state param value. This name is short

internal/oidc/provider/formposthtml/formposthtml.go

@@ -18,11 +18,11 @@ import (
 var (
 	//go:embed form_post.css
 	rawCSS      string
-	minifiedCSS = mustMinify(minify.CSS(rawCSS))
+	minifiedCSS = panicOnError(minify.CSS(rawCSS))
 
 	//go:embed form_post.js
 	rawJS      string
-	minifiedJS = mustMinify(minify.JS(rawJS))
+	minifiedJS = panicOnError(minify.JS(rawJS))
 
 	//go:embed form_post.gohtml
 	rawHTMLTemplate string
@@ -44,7 +44,7 @@ var cspValue = strings.Join([]string{
 	`frame-ancestors 'none'`,
 }, "; ")
 
-func mustMinify(s string, err error) string {
+func panicOnError(s string, err error) string {
 	if err != nil {
 		panic(err)
 	}

internal/oidc/provider/formposthtml/formposthtml_test.go

@@ -93,6 +93,6 @@ func TestContentSecurityPolicyHashes(t *testing.T) {
 }
 
 func TestHelpers(t *testing.T) {
-	require.Equal(t, "test", mustMinify("test", nil))
-	require.PanicsWithError(t, "some error", func() { mustMinify("", fmt.Errorf("some error")) })
+	require.Equal(t, "test", panicOnError("test", nil))
+	require.PanicsWithError(t, "some error", func() { panicOnError("", fmt.Errorf("some error")) })
 }

internal/oidc/provider/manager/manager.go

@@ -8,8 +8,6 @@ import (
 	"strings"
 	"sync"
 
-	"go.pinniped.dev/internal/oidc/login"
-
 	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
 
 	"go.pinniped.dev/internal/oidc"
@@ -20,6 +18,7 @@ import (
 	"go.pinniped.dev/internal/oidc/dynamiccodec"
 	"go.pinniped.dev/internal/oidc/idpdiscovery"
 	"go.pinniped.dev/internal/oidc/jwks"
+	"go.pinniped.dev/internal/oidc/login"
 	"go.pinniped.dev/internal/oidc/provider"
 	"go.pinniped.dev/internal/oidc/token"
 	"go.pinniped.dev/internal/plog"
@@ -139,7 +138,7 @@ func (m *Manager) SetProviders(federationDomains ...*provider.FederationDomainIs
 		m.providerHandlers[(issuerHostWithPath + oidc.PinnipedLoginPath)] = login.NewHandler(
 			upstreamStateEncoder,
 			csrfCookieEncoder,
-			login.NewGetHandler(),
+			login.NewGetHandler(incomingProvider.IssuerPath()+oidc.PinnipedLoginPath),
 			login.NewPostHandler(issuer, m.upstreamIDPs, oauthHelperWithKubeStorage),
 		)
 

site/content/docs/howto/configure-supervisor-with-workspace_one_access.md

@@ -118,10 +118,6 @@ stringData:
   clientSecret: "<your-client-secret>"
 ```
 
-Note that the `metadata.name` of the OIDCIdentityProvider resource may be visible to end users at login prompts
-if you choose to enable `allowPasswordGrant`, so choose a name which will be understood by your end users.
-For example, if you work at Acme Corp, choose something like `acme-corporate-workspace-one` over `my-idp`.
-
 The following claims are returned by Workspace ONE Access.  The `group` scope is required to use the
 `group_ids` and `group_names` claims.  The `email` scope is required to use the `email` claim.  The
 remaining claims are always available.