Merge pull request #1742 from vmware-tanzu/idp_chooser_ui

show interstitial web page to allow user to choose IDP when multiple IDPs are configured and authorize endpoint query param to choose IDP is not used

internal/federationdomain/endpoints/auth/auth_handler.go

@@ -89,6 +89,19 @@ func NewHandler(
 		// oidcapi.AuthorizeUpstreamIDPTypeParamName query (or form) params to request a certain upstream IDP.
 		// The Pinniped CLI has been sending these params since v0.9.0.
 		idpNameQueryParamValue := r.Form.Get(oidcapi.AuthorizeUpstreamIDPNameParamName)
+
+		// Check if we are in a special case where we should inject an interstitial page to ask the user
+		// which IDP they would like to use.
+		if shouldShowIDPChooser(idpFinder, idpNameQueryParamValue, requestedBrowserlessFlow) {
+			// Redirect to the IDP chooser page with all the same query/form params. When the user chooses an IDP,
+			// it will redirect back to here with all the same params again, with the pinniped_idp_name param added.
+			http.Redirect(w, r,
+				fmt.Sprintf("%s%s?%s", downstreamIssuer, oidc.ChooseIDPEndpointPath, r.Form.Encode()),
+				http.StatusSeeOther,
+			)
+			return nil
+		}
+
 		oidcUpstream, ldapUpstream, err := chooseUpstreamIDP(idpNameQueryParamValue, idpFinder)
 		if err != nil {
 			oidc.WriteAuthorizeError(r, w,
@@ -153,6 +166,20 @@ func NewHandler(
 	return securityheader.WrapWithCustomCSP(handler, formposthtml.ContentSecurityPolicy())
 }
 
+func shouldShowIDPChooser(
+	idpFinder federationdomainproviders.FederationDomainIdentityProvidersFinderI,
+	idpNameQueryParamValue string,
+	requestedBrowserlessFlow bool,
+) bool {
+	clientDidNotRequestSpecificIDP := len(idpNameQueryParamValue) == 0
+	clientRequestedBrowserBasedFlow := !requestedBrowserlessFlow
+	inBackwardsCompatMode := idpFinder.HasDefaultIDP()
+	federationDomainSpecHasSomeValidIDPs := idpFinder.IDPCount() > 0
+
+	return clientDidNotRequestSpecificIDP && clientRequestedBrowserBasedFlow &&
+		!inBackwardsCompatMode && federationDomainSpecHasSomeValidIDPs
+}
+
 func handleAuthRequestForLDAPUpstreamCLIFlow(
 	r *http.Request,
 	w http.ResponseWriter,

internal/federationdomain/endpoints/auth/auth_handler_test.go

@@ -731,6 +731,25 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo
 			wantUpstreamStateParamInLocationHeader: true,
 			wantBodyStringWithLocationInHref:       true,
 		},
+		{
+			name: "with multiple IDPs available, request does not choose which IDP to use",
+			idps: oidctestutil.NewUpstreamIDPListerBuilder().
+				WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()).
+				WithLDAP(upstreamLDAPIdentityProviderBuilder().Build()),
+			generateCSRF:                           happyCSRFGenerator,
+			generatePKCE:                           happyPKCEGenerator,
+			generateNonce:                          happyNonceGenerator,
+			stateEncoder:                           happyStateEncoder,
+			cookieEncoder:                          happyCookieEncoder,
+			method:                                 http.MethodGet,
+			path:                                   happyGetRequestPath, // does not include pinniped_idp_name param
+			wantStatus:                             http.StatusSeeOther,
+			wantContentType:                        htmlContentType,
+			wantCSRFValueInCookieHeader:            "", // there should not be a CSRF cookie set on the response
+			wantLocationHeader:                     urlWithQuery(downstreamIssuer+"/choose_identity_provider", happyGetRequestQueryMap),
+			wantUpstreamStateParamInLocationHeader: false, // it should copy the params of the original request, not add a new state param
+			wantBodyStringWithLocationInHref:       true,
+		},
 		{
 			name: "with multiple IDPs available, request chooses to use OIDC browser flow",
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().
@@ -3303,6 +3322,17 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo
 			wantContentType: plainContentType,
 			wantBodyString:  `{"error":"invalid_request","error_description":"The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed. 'pinniped_idp_name' param error: did not find IDP with name 'some-ldap-idp'"}`,
 		},
+		{
+			name:                 "with multiple IDPs, when using browserless flow, when pinniped_idp_name param is not specified, should be an error (browerless flows do not use IDP chooser page)",
+			idps:                 oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().WithAllowPasswordGrant(true).Build()),
+			method:               http.MethodGet,
+			path:                 happyGetRequestPath,
+			customUsernameHeader: ptr.To(oidcUpstreamUsername),
+			customPasswordHeader: ptr.To(oidcUpstreamPassword),
+			wantStatus:           http.StatusBadRequest,
+			wantContentType:      plainContentType,
+			wantBodyString:       `{"error":"invalid_request","error_description":"The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed. 'pinniped_idp_name' param error: identity provider not found: this federation domain does not have a default identity provider"}`,
+		},
 		{
 			name:            "post with invalid form in the body",
 			idps:            oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()),

internal/federationdomain/endpoints/chooseidp/choose_idp_handler.go

@@ -0,0 +1,77 @@
+// Copyright 2023 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package chooseidp
+
+import (
+	"fmt"
+	"net/http"
+	"net/url"
+	"sort"
+
+	"go.pinniped.dev/generated/latest/apis/supervisor/oidc"
+	"go.pinniped.dev/internal/federationdomain/endpoints/chooseidp/chooseidphtml"
+	"go.pinniped.dev/internal/federationdomain/federationdomainproviders"
+	"go.pinniped.dev/internal/httputil/httperr"
+	"go.pinniped.dev/internal/httputil/securityheader"
+)
+
+// NewHandler returns a http.Handler that serves an IDP chooser web page. The authorization endpoint may redirect
+// to this page, copying all the same parameters from the original authorization request. Each button on this page
+// simply adds the IDP's name as an additional request parameter to the original authorization request's parameters,
+// and sends the user back to the authorization endpoint, where the authorization flow can start from scratch using
+// the original params with the extra pinniped_idp_name param added.
+func NewHandler(authURL string, upstreamIDPs federationdomainproviders.FederationDomainIdentityProvidersListerI) http.Handler {
+	handler := httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error {
+		if r.Method != http.MethodGet {
+			return httperr.Newf(http.StatusMethodNotAllowed, "%s (try GET)", r.Method)
+		}
+
+		// This is just a sanity check that it appears to be an authorize request.
+		// Actual enforcement of parameters will happen at the authorization endpoint.
+		query := r.URL.Query()
+		if !(query.Has("client_id") && query.Has("redirect_uri") && query.Has("scope") && query.Has("response_type")) {
+			return httperr.New(http.StatusBadRequest, "missing required query params (must include client_id, redirect_uri, scope, and response_type)")
+		}
+
+		newIDPForPageData := func(displayName string) chooseidphtml.IdentityProvider {
+			return chooseidphtml.IdentityProvider{
+				DisplayName: displayName,
+				URL: fmt.Sprintf("%s?%s&%s=%s",
+					authURL, r.URL.Query().Encode(), oidc.AuthorizeUpstreamIDPNameParamName, url.QueryEscape(displayName)),
+			}
+		}
+
+		var idps []chooseidphtml.IdentityProvider
+		for _, p := range upstreamIDPs.GetOIDCIdentityProviders() {
+			idps = append(idps, newIDPForPageData(p.DisplayName))
+		}
+		for _, p := range upstreamIDPs.GetLDAPIdentityProviders() {
+			idps = append(idps, newIDPForPageData(p.DisplayName))
+		}
+		for _, p := range upstreamIDPs.GetActiveDirectoryIdentityProviders() {
+			idps = append(idps, newIDPForPageData(p.DisplayName))
+		}
+
+		sort.SliceStable(idps, func(i, j int) bool {
+			return idps[i].DisplayName < idps[j].DisplayName
+		})
+
+		if len(idps) == 0 {
+			// This shouldn't normally happen in practice because the auth endpoint would not have redirected to here.
+			return httperr.New(http.StatusInternalServerError,
+				"please check the server's configuration: no valid identity providers found for this FederationDomain")
+		}
+
+		return chooseidphtml.Template().Execute(w, &chooseidphtml.PageData{IdentityProviders: idps})
+	})
+
+	return wrapSecurityHeaders(handler)
+}
+
+func wrapSecurityHeaders(handler http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		wrapped := securityheader.WrapWithCustomCSP(handler, chooseidphtml.ContentSecurityPolicy())
+		wrapped.ServeHTTP(w, r)
+	})
+}

internal/federationdomain/endpoints/chooseidp/choose_idp_handler_test.go

@@ -0,0 +1,150 @@
+// Copyright 2023 the Pinniped contributors. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+package chooseidp
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"go.pinniped.dev/internal/federationdomain/endpoints/chooseidp/chooseidphtml"
+	"go.pinniped.dev/internal/federationdomain/federationdomainproviders"
+	"go.pinniped.dev/internal/federationdomain/oidc"
+	"go.pinniped.dev/internal/testutil"
+	"go.pinniped.dev/internal/testutil/oidctestutil"
+)
+
+func TestChooseIDPHandler(t *testing.T) {
+	const testIssuer = "https://pinniped.dev/issuer"
+
+	testReqQuery := url.Values{
+		"client_id":     []string{"foo"},
+		"redirect_uri":  []string{"bar"},
+		"scope":         []string{"baz"},
+		"response_type": []string{"bat"},
+	}
+	testIssuerWithTestReqQuery := testIssuer + "?" + testReqQuery.Encode()
+
+	tests := []struct {
+		name string
+
+		method    string
+		reqTarget string
+		idps      federationdomainproviders.FederationDomainIdentityProvidersListerI
+
+		wantStatus      int
+		wantContentType string
+		wantBodyString  string
+	}{
+		{
+			name:      "happy path",
+			method:    http.MethodGet,
+			reqTarget: "/some/path" + oidc.ChooseIDPEndpointPath + "?" + testReqQuery.Encode(),
+			idps: oidctestutil.NewUpstreamIDPListerBuilder().
+				WithOIDC(oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().WithName("oidc2").Build()).
+				WithLDAP(oidctestutil.NewTestUpstreamLDAPIdentityProviderBuilder().WithName("ldap1").Build()).
+				WithActiveDirectory(oidctestutil.NewTestUpstreamLDAPIdentityProviderBuilder().WithName("z-ad1").Build()).
+				WithLDAP(oidctestutil.NewTestUpstreamLDAPIdentityProviderBuilder().WithName("ldap2").Build()).
+				WithOIDC(oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().WithName("oidc1").Build()).
+				WithActiveDirectory(oidctestutil.NewTestUpstreamLDAPIdentityProviderBuilder().WithName("ad2").Build()).
+				BuildFederationDomainIdentityProvidersListerFinder(),
+			wantStatus:      http.StatusOK,
+			wantContentType: "text/html; charset=utf-8",
+			wantBodyString: testutil.ExpectedChooseIDPPageHTML(chooseidphtml.CSS(), chooseidphtml.JS(), []testutil.ChooseIDPPageExpectedValue{
+				// Should be sorted alphabetically by displayName.
+				{DisplayName: "ad2", URL: testIssuerWithTestReqQuery + "&pinniped_idp_name=ad2"},
+				{DisplayName: "ldap1", URL: testIssuerWithTestReqQuery + "&pinniped_idp_name=ldap1"},
+				{DisplayName: "ldap2", URL: testIssuerWithTestReqQuery + "&pinniped_idp_name=ldap2"},
+				{DisplayName: "oidc1", URL: testIssuerWithTestReqQuery + "&pinniped_idp_name=oidc1"},
+				{DisplayName: "oidc2", URL: testIssuerWithTestReqQuery + "&pinniped_idp_name=oidc2"},
+				{DisplayName: "z-ad1", URL: testIssuerWithTestReqQuery + "&pinniped_idp_name=z-ad1"},
+			}),
+		},
+		{
+			name:      "happy path when there are special characters in the IDP name",
+			method:    http.MethodGet,
+			reqTarget: "/some/path" + oidc.ChooseIDPEndpointPath + "?" + testReqQuery.Encode(),
+			idps: oidctestutil.NewUpstreamIDPListerBuilder().
+				WithOIDC(oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().WithName(`This is Ryan's IDP 👍\~!@#$%^&*()-+[]{}\|;'"<>,.?`).Build()).
+				WithLDAP(oidctestutil.NewTestUpstreamLDAPIdentityProviderBuilder().WithName(`This is Josh's IDP 🦭`).Build()).
+				BuildFederationDomainIdentityProvidersListerFinder(),
+			wantStatus:      http.StatusOK,
+			wantContentType: "text/html; charset=utf-8",
+			wantBodyString: testutil.ExpectedChooseIDPPageHTML(chooseidphtml.CSS(), chooseidphtml.JS(), []testutil.ChooseIDPPageExpectedValue{
+				// Should be sorted alphabetically by displayName.
+				{
+					DisplayName: `This is Josh's IDP 🦭`,
+					URL:         testIssuerWithTestReqQuery + `&pinniped_idp_name=` + url.QueryEscape(`This is Josh's IDP 🦭`),
+				},
+				{
+					DisplayName: `This is Ryan's IDP 👍\~!@#$%^&*()-+[]{}\|;'"<>,.?`,
+					URL:         testIssuerWithTestReqQuery + `&pinniped_idp_name=` + url.QueryEscape(`This is Ryan's IDP 👍\~!@#$%^&*()-+[]{}\|;'"<>,.?`),
+				},
+			}),
+		},
+		{
+			name:      "no valid IDPs are configured on the FederationDomain",
+			method:    http.MethodGet,
+			reqTarget: "/some/path" + oidc.ChooseIDPEndpointPath + "?" + testReqQuery.Encode(),
+			idps: oidctestutil.NewUpstreamIDPListerBuilder().
+				BuildFederationDomainIdentityProvidersListerFinder(),
+			wantStatus:      http.StatusInternalServerError,
+			wantContentType: "text/plain; charset=utf-8",
+			wantBodyString:  "Internal Server Error: please check the server's configuration: no valid identity providers found for this FederationDomain\n",
+		},
+		{
+			name:      "no query params on the request",
+			method:    http.MethodGet,
+			reqTarget: "/some/path" + oidc.ChooseIDPEndpointPath,
+			idps: oidctestutil.NewUpstreamIDPListerBuilder().
+				WithOIDC(oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().WithName("x-some-idp").Build()).
+				BuildFederationDomainIdentityProvidersListerFinder(),
+			wantStatus:      http.StatusBadRequest,
+			wantContentType: "text/plain; charset=utf-8",
+			wantBodyString:  "Bad Request: missing required query params (must include client_id, redirect_uri, scope, and response_type)\n",
+		},
+		{
+			name:      "missing required query param(s) on the request",
+			method:    http.MethodGet,
+			reqTarget: "/some/path" + oidc.ChooseIDPEndpointPath + "?client_id=foo",
+			idps: oidctestutil.NewUpstreamIDPListerBuilder().
+				WithOIDC(oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().WithName("x-some-idp").Build()).
+				BuildFederationDomainIdentityProvidersListerFinder(),
+			wantStatus:      http.StatusBadRequest,
+			wantContentType: "text/plain; charset=utf-8",
+			wantBodyString:  "Bad Request: missing required query params (must include client_id, redirect_uri, scope, and response_type)\n",
+		},
+		{
+			name:      "bad request method",
+			method:    http.MethodPost,
+			reqTarget: oidc.ChooseIDPEndpointPath,
+			idps: oidctestutil.NewUpstreamIDPListerBuilder().
+				WithOIDC(oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().WithName("x-some-idp").Build()).
+				BuildFederationDomainIdentityProvidersListerFinder(),
+			wantStatus:      http.StatusMethodNotAllowed,
+			wantContentType: "text/plain; charset=utf-8",
+			wantBodyString:  "Method Not Allowed: POST (try GET)\n",
+		},
+	}
+	for _, test := range tests {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			t.Parallel()
+
+			handler := NewHandler(testIssuer, test.idps)
+
+			req := httptest.NewRequest(test.method, test.reqTarget, nil)
+			rsp := httptest.NewRecorder()
+			handler.ServeHTTP(rsp, req)
+
+			require.Equal(t, test.wantStatus, rsp.Code)
+			require.Equal(t, test.wantContentType, rsp.Header().Get("Content-Type"))
+			require.Equal(t, test.wantBodyString, rsp.Body.String())
+			testutil.RequireSecurityHeadersWithIDPChooserPageCSPs(t, rsp)
+		})
+	}
+}

internal/federationdomain/endpoints/chooseidp/chooseidphtml/choose_idp.css

@@ -0,0 +1,71 @@
+/* Copyright 2023 the Pinniped contributors. All Rights Reserved. */
+/* SPDX-License-Identifier: Apache-2.0 */
+
+html {
+    height: 100%;
+}
+
+/* The form for this page is styled to be the same as the form from login_form.css */
+body {
+    font-family: "Metropolis-Light", Helvetica, sans-serif;
+    display: flex;
+    flex-flow: column wrap;
+    justify-content: flex-start;
+    align-items: center;
+    /* subtle gradient make the login box stand out */
+    background: linear-gradient(to top, #f8f8f8, white);
+    min-height: 100%;
+}
+
+h1 {
+    font-size: 20px;
+    margin: 0;
+}
+
+.box {
+    display: flex;
+    flex-direction: column;
+    flex-wrap: nowrap;
+    border-radius: 4px;
+    border-color: #ddd;
+    border-width: 1px;
+    border-style: solid;
+    width: 400px;
+    padding:30px 30px 0;
+    margin: 60px 20px 0;
+    background: white;
+    font-size: 14px;
+}
+
+/* Buttons for this page are styled to be the same as the form submit button in login_form.css */
+button {
+    color: inherit;
+    font: inherit;
+    border: 0;
+    margin: 0;
+    outline: 0;
+    padding: 0;
+}
+
+.form-field {
+    display: flex;
+    margin-bottom: 30px;
+}
+
+.form-field button {
+    width: 100%;
+    padding: 1em;
+    background-color: #218fcf; /* this is a color from the Pinniped logo :) */
+    color: #eee;
+    font-weight: bold;
+    cursor: pointer;
+    transition: all .3s;
+}
+
+.form-field button:focus, .form-field button:hover {
+    background-color: #1abfd3; /* this is a color from the Pinniped logo :) */
+}
+
+.form-field button:active {
+    transform: scale(.99);
+}