Fix auth_handler_test.go

internal/oidc/auth/auth_handler.go

@@ -56,23 +56,54 @@ func NewHandler(
 			return httperr.Newf(http.StatusMethodNotAllowed, "%s (try GET or POST)", r.Method)
 		}
 
+		requestedBrowserlessFlow := len(r.Header.Values(oidcapi.AuthorizeUsernameHeaderName)) > 0 ||
+			len(r.Header.Values(oidcapi.AuthorizePasswordHeaderName)) > 0
+
+		// Need to parse the request params so we can get the IDP name. The style and text of the error is inspired by
+		// fosite's implementation of NewAuthorizeRequest(). Fosite only calls ParseMultipartForm() there. However,
+		// although ParseMultipartForm() calls ParseForm(), it swallows errors from ParseForm() sometimes. To avoid
+		// having any errors swallowed, we call both. When fosite calls ParseMultipartForm() later, it will be a noop.
+		if err := r.ParseForm(); err != nil {
+			oidc.WriteAuthorizeError(r, w,
+				oauthHelperWithoutStorage,
+				fosite.NewAuthorizeRequest(),
+				fosite.ErrInvalidRequest.
+					WithHint("Unable to parse form params, make sure to send a properly formatted query params or form request body.").
+					WithWrap(err).WithDebug(err.Error()),
+				requestedBrowserlessFlow)
+			return nil
+		}
+		if err := r.ParseMultipartForm(1 << 20); err != nil && err != http.ErrNotMultipart {
+			oidc.WriteAuthorizeError(r, w,
+				oauthHelperWithoutStorage,
+				fosite.NewAuthorizeRequest(),
+				fosite.ErrInvalidRequest.
+					WithHint("Unable to parse multipart HTTP body, make sure to send a properly formatted form request body.").
+					WithWrap(err).WithDebug(err.Error()),
+				requestedBrowserlessFlow)
+			return nil
+		}
+
 		// Note that the client might have used oidcapi.AuthorizeUpstreamIDPNameParamName and
-		// oidcapi.AuthorizeUpstreamIDPTypeParamName query params to request a certain upstream IDP.
+		// 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.URL.Query().Get(oidcapi.AuthorizeUpstreamIDPNameParamName)
+		idpNameQueryParamValue := r.Form.Get(oidcapi.AuthorizeUpstreamIDPNameParamName)
 		oidcUpstream, ldapUpstream, err := chooseUpstreamIDP(idpNameQueryParamValue, idpFinder)
 		if err != nil {
-			plog.WarningErr("authorize upstream config", err)
+			oidc.WriteAuthorizeError(r, w,
-			return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err)
+				oauthHelperWithoutStorage,
+				fosite.NewAuthorizeRequest(),
+				fosite.ErrInvalidRequest.
+					WithHintf("%q param error: %s", oidcapi.AuthorizeUpstreamIDPNameParamName, err.Error()).
+					WithWrap(err).WithDebug(err.Error()),
+				requestedBrowserlessFlow)
+			return nil
 		}
 
 		if oidcUpstream != nil {
-			if len(r.Header.Values(oidcapi.AuthorizeUsernameHeaderName)) > 0 ||
+			if requestedBrowserlessFlow {
-				len(r.Header.Values(oidcapi.AuthorizePasswordHeaderName)) > 0 {
 				// The client set a username header, so they are trying to log in with a username/password.
-				return handleAuthRequestForOIDCUpstreamPasswordGrant(
+				return handleAuthRequestForOIDCUpstreamPasswordGrant(r, w,
-					r,
-					w,
 					oauthHelperWithStorage,
 					oidcUpstream.Provider,
 					oidcUpstream.Transforms,
@@ -91,8 +122,7 @@ func NewHandler(
 		}
 
 		// We know it's an AD/LDAP upstream.
-		if len(r.Header.Values(oidcapi.AuthorizeUsernameHeaderName)) > 0 ||
+		if requestedBrowserlessFlow {
-			len(r.Header.Values(oidcapi.AuthorizePasswordHeaderName)) > 0 {
 			// The client set a username header, so they are trying to log in with a username/password.
 			return handleAuthRequestForLDAPUpstreamCLIFlow(r, w,
 				oauthHelperWithStorage,
@@ -102,13 +132,9 @@ func NewHandler(
 				idpNameQueryParamValue,
 			)
 		}
-		return handleAuthRequestForLDAPUpstreamBrowserFlow(
+		return handleAuthRequestForLDAPUpstreamBrowserFlow(r, w,
-			r,
-			w,
 			oauthHelperWithoutStorage,
-			generateCSRF,
+			generateCSRF, generateNonce, generatePKCE,
-			generateNonce,
-			generatePKCE,
 			ldapUpstream,
 			ldapUpstream.SessionProviderType,
 			downstreamIssuer,

internal/testutil/oidctestutil/oidctestutil.go

@@ -463,6 +463,11 @@ type TestFederationDomainIdentityProvidersListerFinder struct {
 	upstreamOIDCIdentityProviders            []*TestUpstreamOIDCIdentityProvider
 	upstreamLDAPIdentityProviders            []*TestUpstreamLDAPIdentityProvider
 	upstreamActiveDirectoryIdentityProviders []*TestUpstreamLDAPIdentityProvider
+	defaultIDPDisplayName                    string
+}
+
+func (t *TestFederationDomainIdentityProvidersListerFinder) SetDefaultIDPDisplayName(displayName string) {
+	t.defaultIDPDisplayName = displayName
 }
 
 func (t *TestFederationDomainIdentityProvidersListerFinder) GetOIDCIdentityProviders() []*provider.FederationDomainResolvedOIDCIdentityProvider {
@@ -508,7 +513,10 @@ func (t *TestFederationDomainIdentityProvidersListerFinder) GetActiveDirectoryId
 }
 
 func (t *TestFederationDomainIdentityProvidersListerFinder) FindDefaultIDP() (*provider.FederationDomainResolvedOIDCIdentityProvider, *provider.FederationDomainResolvedLDAPIdentityProvider, error) {
-	return nil, nil, fmt.Errorf("TODO: implement me") // TODO
+	if t.defaultIDPDisplayName == "" {
+		return nil, nil, fmt.Errorf("identity provider not found: this federation domain does not have a default identity provider")
+	}
+	return t.FindUpstreamIDPByDisplayName(t.defaultIDPDisplayName)
 }
 
 func (t *TestFederationDomainIdentityProvidersListerFinder) FindUpstreamIDPByDisplayName(upstreamIDPDisplayName string) (*provider.FederationDomainResolvedOIDCIdentityProvider, *provider.FederationDomainResolvedLDAPIdentityProvider, error) {