Add more granular unit tests

.golangci.yaml

@@ -1,8 +1,7 @@
-# https://github.com/golangci/golangci-lint#config-file
+# https://golangci-lint.run/usage/configuration/
 run:
-  deadline: 1m
-  skip-dirs:
-    - generated
+  timeout: 1m
+
 
 linters:
   disable-all: true
@@ -47,6 +46,8 @@ linters:
     - whitespace
 
 issues:
+  exclude-dirs:
+    - generated
   exclude-rules:
     # exclude tests from some rules for things that are useful in a testing context.
     - path: _test\.go

pkg/oidcclient/login.go

@@ -475,14 +475,41 @@ func (h *handlerState) baseLogin() (*oidctypes.Token, error) {
 		h.pkce.Challenge(),
 		h.pkce.Method(),
 	}
-	if h.upstreamIdentityProviderName != "" {
-		authorizeOptions = append(authorizeOptions,
-			oauth2.SetAuthURLParam(oidcapi.AuthorizeUpstreamIDPNameParamName, h.upstreamIdentityProviderName),
-		)
-		// TODO Note that this is the only place where the requested IDP type is used in this file. It is sent as a custom param on the auth request.
-		authorizeOptions = append(authorizeOptions,
-			oauth2.SetAuthURLParam(oidcapi.AuthorizeUpstreamIDPTypeParamName, string(h.upstreamIdentityProviderType)),
-		)
+
+	pinnipedSupervisorOptions, err := h.maybePerformPinnipedSupervisorValidations()
+	if err != nil {
+		return nil, err
+	}
+	authorizeOptions = append(authorizeOptions, pinnipedSupervisorOptions...)
+
+	// Preserve the legacy behavior where browser-based auth is preferred
+	authFunc := h.webBrowserBasedAuth
+
+	// Choose the appropriate authorization and authcode exchange strategy.
+	// Use a switch so that lint will make sure we have full coverage.
+	switch h.loginFlow {
+	case idpdiscoveryv1alpha1.IDPFlowCLIPassword:
+		authFunc = h.cliBasedAuth
+	case idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode:
+		// NOOP
+	}
+
+	// Perform the authorize request and authcode exchange to get back OIDC tokens.
+	token, err := authFunc(&authorizeOptions)
+
+	// If we got tokens, put them in the cache.
+	if err == nil {
+		h.cache.PutToken(cacheKey, token)
+	}
+
+	return token, err
+}
+
+// maybePerformPinnipedSupervisorValidations will add some authorization options and perform additional validations if
+// the issuer is a Pinniped Supervisor and exposes IDP discovery.
+func (h *handlerState) maybePerformPinnipedSupervisorValidations() ([]oauth2.AuthCodeOption, error) {
+	if h.upstreamIdentityProviderName == "" {
+		return nil, nil
 	}
 
 	// TODO Right here is where we have already performed discovery and we are about to need a final choice of flow type.
@@ -499,53 +526,72 @@ func (h *handlerState) baseLogin() (*oidctypes.Token, error) {
 	//      and you may or may not be talking to a Supervisor in that case. We should continue to allow this to work
 	//      by omitting the name and type from the auth request and choosing the web browser flow.
 
-	if h.upstreamIdentityProviderName != "" && h.idpDiscovery != nil {
-		found := slices.ContainsFunc(h.idpDiscovery.PinnipedIDPs, func(idp idpdiscoveryv1alpha1.PinnipedIDP) bool {
-			return idp.Name == h.upstreamIdentityProviderName &&
-				idp.Type == h.upstreamIdentityProviderType
-		})
-		if !found {
-			return nil, fmt.Errorf(
-				"unable to login to IDP with name '%s' and type '%s'",
-				h.upstreamIdentityProviderName,
+	if h.idpDiscovery == nil {
+		return nil, fmt.Errorf("this Pinniped Supervisor does not have IDP discovery, please contact the system administrator to request an upgrade to the Pinniped Supervisor")
+	}
+
+	// Legacy Pinniped Supervisors do not provide this information
+	if len(h.idpDiscovery.PinnipedSupportedIDPTypes) > 0 {
+		var supportedIDPTypes []idpdiscoveryv1alpha1.IDPType
+		for _, idpType := range h.idpDiscovery.PinnipedSupportedIDPTypes {
+			supportedIDPTypes = append(supportedIDPTypes, idpType.Type)
+		}
+
+		// Sort by name for repeatability
+		slices.Sort(supportedIDPTypes)
+
+		if !slices.Contains(supportedIDPTypes, h.upstreamIdentityProviderType) {
+			return nil, fmt.Errorf("unable to login to IDP with type %q, this Pinniped Supervisor supports IDP types [%s]",
 				h.upstreamIdentityProviderType,
-			)
+				strings.Join(func() []string {
+					var temp []string
+					for _, idpType := range supportedIDPTypes {
+						temp = append(temp, fmt.Sprintf("%q", idpType))
+					}
+					return temp
+				}(), ", "))
 		}
 	}
 
-	// If the caller has not requested a specific flow, but has requested a specific IDP, and we're talking to a Pinniped Supervisor,
-	// infer the authentication flow.
-	if h.loginFlow == "" && h.upstreamIdentityProviderName != "" && h.idpDiscovery != nil {
-		for _, idp := range h.idpDiscovery.PinnipedIDPs {
-			if idp.Name == h.upstreamIdentityProviderName &&
-				idp.Type == h.upstreamIdentityProviderType && // TODO: not sure if this is explicitly tested
-				len(idp.Flows) > 0 { // TODO: we should test this even if the Supervisor always populates something here
-				h.loginFlow = idp.Flows[0]
-			}
+	foundIDPIndex := slices.IndexFunc(h.idpDiscovery.PinnipedIDPs, func(idp idpdiscoveryv1alpha1.PinnipedIDP) bool {
+		return idp.Name == h.upstreamIdentityProviderName &&
+			idp.Type == h.upstreamIdentityProviderType &&
+			(h.loginFlow == "" || slices.Contains(idp.Flows, h.loginFlow))
+	})
+
+	if foundIDPIndex < 0 {
+		if h.loginFlow == "" {
+			return nil, fmt.Errorf(
+				"unable to login to IDP with name %q and type %q",
+				h.upstreamIdentityProviderName,
+				h.upstreamIdentityProviderType,
+			)
 		}
+		return nil, fmt.Errorf(
+			"unable to login to IDP with name %q and type %q and flow %q",
+			h.upstreamIdentityProviderName,
+			h.upstreamIdentityProviderType,
+			h.loginFlow,
+		)
 	}
 
-	// Preserve the legacy behavior where browser-based auth is preferred
-	authFunc := h.webBrowserBasedAuth
+	foundIDP := h.idpDiscovery.PinnipedIDPs[foundIDPIndex]
 
-	// Choose the appropriate authorization and authcode exchange strategy.
-	// Use a switch so that lint will make sure we have full coverage.
-	switch h.loginFlow {
-	case idpdiscoveryv1alpha1.IDPFlowCLIPassword:
-		authFunc = h.cliBasedAuth
-	case idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode:
-		// NOOP
+	// If the caller has not requested a specific flow, but has requested a specific IDP, infer the authentication flow.
+	if h.loginFlow == "" {
+		h.loginFlow = foundIDP.Flows[0]
 	}
 
-	// Perform the authorize request and authcode exchange to get back OIDC tokens.
-	token, err := authFunc(&authorizeOptions)
+	var authorizeOptions []oauth2.AuthCodeOption
 
-	// If we got tokens, put them in the cache.
-	if err == nil {
-		h.cache.PutToken(cacheKey, token)
-	}
+	authorizeOptions = append(authorizeOptions,
+		oauth2.SetAuthURLParam(oidcapi.AuthorizeUpstreamIDPNameParamName, h.upstreamIdentityProviderName),
+	)
+	authorizeOptions = append(authorizeOptions,
+		oauth2.SetAuthURLParam(oidcapi.AuthorizeUpstreamIDPTypeParamName, string(h.upstreamIdentityProviderType)),
+	)
 
-	return token, err
+	return authorizeOptions, nil
 }
 
 // Make a direct call to the authorize endpoint, including the user's username and password on custom http headers,
@@ -909,54 +955,52 @@ func (h *handlerState) maybePerformPinnipedSupervisorIDPDiscovery() error {
 	// Note that this library can be used with OIDC IDPs other than Pinniped Supervisor, so
 	var pinnipedSupervisorClaims idpdiscoveryv1alpha1.OIDCDiscoveryResponse
 	if err := h.provider.Claims(&pinnipedSupervisorClaims); err != nil {
-		// TODO: Test this branch?
 		return fmt.Errorf("could not decode the Pinniped IDP discovery document URL in OIDC discovery from %q: %w", h.issuer, err)
 	}
 
-	if pinnipedSupervisorClaims.SupervisorDiscovery.PinnipedIDPsEndpoint != "" {
-		// This check confirms that the issuer is hosting the IDP discovery document, which would always be the case for
-		// Pinniped Supervisor. Since there are checks above to confirm that the issuer uses HTTPS, IDP discovery will
-		// always use HTTPS.
-		if !strings.HasPrefix(pinnipedSupervisorClaims.SupervisorDiscovery.PinnipedIDPsEndpoint, h.issuer) {
-			return fmt.Errorf("the Pinniped IDP discovery document must always be hosted by the issuer %q", h.issuer)
-		}
-
-		idpDiscoveryCtx, idpDiscoveryCtxCancelFunc := context.WithTimeout(h.ctx, httpRequestTimeout)
-		defer idpDiscoveryCtxCancelFunc()
-		idpDiscoveryReq, err := http.NewRequestWithContext(idpDiscoveryCtx, http.MethodGet, pinnipedSupervisorClaims.SupervisorDiscovery.PinnipedIDPsEndpoint, nil)
-		if err != nil {
-			// TODO: Test this branch?
-			return fmt.Errorf("could not build IDP Discovery request: %w", err)
-		}
-		idpDiscoveryRes, err := h.httpClient.Do(idpDiscoveryReq)
-		if err != nil {
-			// TODO: Test this branch?
-			return fmt.Errorf("IDP Discovery response error: %w", err)
-		}
-		defer func() {
-			_ = idpDiscoveryRes.Body.Close() // We can't do anything if this fails to close
-		}()
+	// This is not an error - it just means that this issuer is not a Pinniped Supervisor
+	if pinnipedSupervisorClaims.SupervisorDiscovery.PinnipedIDPsEndpoint == "" {
+		return nil
+	}
+	// This check confirms that the issuer is hosting the IDP discovery document, which would always be the case for
+	// Pinniped Supervisor. Since there are checks above to confirm that the issuer uses HTTPS, IDP discovery will
+	// always use HTTPS.
+	if !strings.HasPrefix(pinnipedSupervisorClaims.SupervisorDiscovery.PinnipedIDPsEndpoint, h.issuer) {
+		return fmt.Errorf("the Pinniped IDP discovery document must always be hosted by the issuer: %q", h.issuer)
+	}
 
-		if idpDiscoveryRes.StatusCode != http.StatusOK {
-			// TODO: Test this branch?
-			return fmt.Errorf("unable to fetch IDP discovery data from issuer: unexpected http response status: %s", idpDiscoveryRes.Status)
-		}
+	idpDiscoveryCtx, idpDiscoveryCtxCancelFunc := context.WithTimeout(h.ctx, httpRequestTimeout)
+	defer idpDiscoveryCtxCancelFunc()
+	idpDiscoveryReq, err := http.NewRequestWithContext(idpDiscoveryCtx, http.MethodGet, pinnipedSupervisorClaims.SupervisorDiscovery.PinnipedIDPsEndpoint, nil)
+	if err != nil { // untested
+		return fmt.Errorf("could not build IDP Discovery request: %w", err)
+	}
+	idpDiscoveryRes, err := h.httpClient.Do(idpDiscoveryReq)
+	if err != nil {
+		// TODO: Test this branch?
+		return fmt.Errorf("IDP Discovery response error: %w", err)
+	}
+	defer func() {
+		_ = idpDiscoveryRes.Body.Close() // We can't do anything if this fails to close
+	}()
 
-		rawBody, err := io.ReadAll(idpDiscoveryRes.Body)
-		if err != nil {
-			// TODO: Test this branch?
-			return fmt.Errorf("unable to fetch IDP discovery data from issuer: could not read response body: %w", err)
-		}
+	if idpDiscoveryRes.StatusCode != http.StatusOK {
+		// TODO: Test this branch?
+		return fmt.Errorf("unable to fetch IDP discovery data from issuer: unexpected http response status: %s", idpDiscoveryRes.Status)
+	}
 
-		var body idpdiscoveryv1alpha1.IDPDiscoveryResponse
-		err = json.Unmarshal(rawBody, &body)
-		if err != nil {
-			return fmt.Errorf("unable to fetch the Pinniped IDP discovery document: could not parse response JSON: %w", err)
-		}
+	rawBody, err := io.ReadAll(idpDiscoveryRes.Body)
+	if err != nil { // untested
+		return fmt.Errorf("unable to fetch IDP discovery data from issuer: could not read response body: %w", err)
+	}
 
-		h.idpDiscovery = &body
+	var body idpdiscoveryv1alpha1.IDPDiscoveryResponse
+	err = json.Unmarshal(rawBody, &body)
+	if err != nil {
+		return fmt.Errorf("unable to fetch the Pinniped IDP discovery document: could not parse response JSON: %w", err)
 	}
 
+	h.idpDiscovery = &body
 	return nil
 }