Merge branch 'main' into upstream_access_revocation_during_gc

internal/upstreamoidc/upstreamoidc.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package upstreamoidc implements an abstraction of upstream OIDC provider interactions.
@@ -12,6 +12,7 @@ import (
 	"net/http"
 	"net/url"
 	"strings"
+	"time"
 
 	coreosoidc "github.com/coreos/go-oidc/v3/oidc"
 	"golang.org/x/oauth2"
@@ -60,6 +61,19 @@ func (p *ProviderConfig) GetRevocationURL() *url.URL {
 	return p.RevocationURL
 }
 
+func (p *ProviderConfig) HasUserInfoURL() bool {
+	providerJSON := &struct {
+		UserInfoURL string `json:"userinfo_endpoint"`
+	}{}
+	if err := p.Provider.Claims(providerJSON); err != nil {
+		// This should never happen in practice because we should have already successfully
+		// parsed these claims when p.Provider was created.
+		return false
+	}
+
+	return len(providerJSON.UserInfoURL) > 0
+}
+
 func (p *ProviderConfig) GetAdditionalAuthcodeParams() map[string]string {
 	return p.AdditionalAuthcodeParams
 }
@@ -112,7 +126,7 @@ func (p *ProviderConfig) PasswordCredentialsGrantAndValidateTokens(ctx context.C
 	// There is no nonce to validate for a resource owner password credentials grant because it skips using
 	// the authorize endpoint and goes straight to the token endpoint.
 	const skipNonceValidation nonce.Nonce = ""
-	return p.ValidateToken(ctx, tok, skipNonceValidation)
+	return p.ValidateTokenAndMergeWithUserInfo(ctx, tok, skipNonceValidation, true, false)
 }
 
 func (p *ProviderConfig) ExchangeAuthcodeAndValidateTokens(ctx context.Context, authcode string, pkceCodeVerifier pkce.Code, expectedIDTokenNonce nonce.Nonce, redirectURI string) (*oidctypes.Token, error) {
@@ -126,7 +140,7 @@ func (p *ProviderConfig) ExchangeAuthcodeAndValidateTokens(ctx context.Context,
 		return nil, err
 	}
 
-	return p.ValidateToken(ctx, tok, expectedIDTokenNonce)
+	return p.ValidateTokenAndMergeWithUserInfo(ctx, tok, expectedIDTokenNonce, true, false)
 }
 
 func (p *ProviderConfig) PerformRefresh(ctx context.Context, refreshToken string) (*oauth2.Token, error) {
@@ -259,36 +273,27 @@ func (p *ProviderConfig) tryRevokeToken(
 	}
 }
 
-// ValidateToken will validate the ID token. It will also merge the claims from the userinfo endpoint response,
+// ValidateTokenAndMergeWithUserInfo will validate the ID token. It will also merge the claims from the userinfo endpoint response,
 // if the provider offers the userinfo endpoint.
-func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) {
-	idTok, hasIDTok := tok.Extra("id_token").(string)
-	if !hasIDTok {
-		return nil, httperr.New(http.StatusBadRequest, "received response missing ID token")
-	}
-	validated, err := p.Provider.Verifier(&coreosoidc.Config{ClientID: p.GetClientID()}).Verify(coreosoidc.ClientContext(ctx, p.Client), idTok)
+func (p *ProviderConfig) ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool, requireUserInfo bool) (*oidctypes.Token, error) {
+	var validatedClaims = make(map[string]interface{})
+
+	var idTokenExpiry time.Time
+	// if we require the id token, make sure we have it.
+	// also, if it exists but wasn't required, still make sure it passes these checks.
+	idTokenExpiry, idTok, err := p.validateIDToken(ctx, tok, expectedIDTokenNonce, validatedClaims, requireIDToken)
 	if err != nil {
-		return nil, httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err)
-	}
-	if validated.AccessTokenHash != "" {
-		if err := validated.VerifyAccessToken(tok.AccessToken); err != nil {
-			return nil, httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err)
-		}
-	}
-	if expectedIDTokenNonce != "" {
-		if err := expectedIDTokenNonce.Validate(validated); err != nil {
-			return nil, httperr.Wrap(http.StatusBadRequest, "received ID token with invalid nonce", err)
-		}
+		return nil, err
 	}
 
-	var validatedClaims map[string]interface{}
-	if err := validated.Claims(&validatedClaims); err != nil {
-		return nil, httperr.Wrap(http.StatusInternalServerError, "could not unmarshal id token claims", err)
-	}
-	maybeLogClaims("claims from ID token", p.Name, validatedClaims)
+	idTokenSubject, _ := validatedClaims[oidc.IDTokenSubjectClaim].(string)
 
-	if err := p.maybeFetchUserInfoAndMergeClaims(ctx, tok, validatedClaims); err != nil {
-		return nil, httperr.Wrap(http.StatusInternalServerError, "could not fetch user info claims", err)
+	if len(idTokenSubject) > 0 || !requireIDToken {
+		// only fetch userinfo if the ID token has a subject or if we are ignoring the id token completely.
+		// otherwise, defer to existing ID token validation
+		if err := p.maybeFetchUserInfoAndMergeClaims(ctx, tok, validatedClaims, requireIDToken, requireUserInfo); err != nil {
+			return nil, httperr.Wrap(http.StatusInternalServerError, "could not fetch user info claims", err)
+		}
 	}
 
 	return &oidctypes.Token{
@@ -302,58 +307,107 @@ func (p *ProviderConfig) ValidateToken(ctx context.Context, tok *oauth2.Token, e
 		},
 		IDToken: &oidctypes.IDToken{
 			Token:  idTok,
-			Expiry: metav1.NewTime(validated.Expiry),
+			Expiry: metav1.NewTime(idTokenExpiry),
 			Claims: validatedClaims,
 		},
 	}, nil
 }
 
-func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, tok *oauth2.Token, claims map[string]interface{}) error {
+func (p *ProviderConfig) validateIDToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, validatedClaims map[string]interface{}, requireIDToken bool) (time.Time, string, error) {
+	idTok, hasIDTok := tok.Extra("id_token").(string)
+	if !hasIDTok && !requireIDToken {
+		return time.Time{}, "", nil // exit early
+	}
+
+	var idTokenExpiry time.Time
+	if !hasIDTok {
+		return time.Time{}, "", httperr.New(http.StatusBadRequest, "received response missing ID token")
+	}
+	validated, err := p.Provider.Verifier(&coreosoidc.Config{ClientID: p.GetClientID()}).Verify(coreosoidc.ClientContext(ctx, p.Client), idTok)
+	if err != nil {
+		return time.Time{}, "", httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err)
+	}
+	if validated.AccessTokenHash != "" {
+		if err := validated.VerifyAccessToken(tok.AccessToken); err != nil {
+			return time.Time{}, "", httperr.Wrap(http.StatusBadRequest, "received invalid ID token", err)
+		}
+	}
+	if expectedIDTokenNonce != "" {
+		if err := expectedIDTokenNonce.Validate(validated); err != nil {
+			return time.Time{}, "", httperr.Wrap(http.StatusBadRequest, "received ID token with invalid nonce", err)
+		}
+	}
+	if err := validated.Claims(&validatedClaims); err != nil {
+		return time.Time{}, "", httperr.Wrap(http.StatusInternalServerError, "could not unmarshal id token claims", err)
+	}
+	maybeLogClaims("claims from ID token", p.Name, validatedClaims)
+	idTokenExpiry = validated.Expiry // keep track of the id token expiry if we have an id token. Otherwise, it'll just be the zero value.
+	return idTokenExpiry, idTok, nil
+}
+
+func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, tok *oauth2.Token, claims map[string]interface{}, requireIDToken bool, requireUserInfo bool) error {
 	idTokenSubject, _ := claims[oidc.IDTokenSubjectClaim].(string)
-	if len(idTokenSubject) == 0 {
-		return nil // defer to existing ID token validation
-	}
 
-	providerJSON := &struct {
-		UserInfoURL string `json:"userinfo_endpoint"`
-	}{}
-	if err := p.Provider.Claims(providerJSON); err != nil {
-		// this should never happen because we should have already parsed these claims at an earlier stage
-		return httperr.Wrap(http.StatusInternalServerError, "could not unmarshal discovery JSON", err)
+	userInfo, err := p.maybeFetchUserInfo(ctx, tok, requireUserInfo)
+	if err != nil {
+		return err
 	}
-
-	// implementing the user info endpoint is not required, skip this logic when it is absent
-	if len(providerJSON.UserInfoURL) == 0 {
+	if userInfo == nil {
 		return nil
 	}
 
-	userInfo, err := p.Provider.UserInfo(coreosoidc.ClientContext(ctx, p.Client), oauth2.StaticTokenSource(tok))
-	if err != nil {
-		return httperr.Wrap(http.StatusInternalServerError, "could not get user info", err)
-	}
-
 	// The sub (subject) Claim MUST always be returned in the UserInfo Response.
-	//
 	// NOTE: Due to the possibility of token substitution attacks (see Section 16.11), the UserInfo Response is not
 	// guaranteed to be about the End-User identified by the sub (subject) element of the ID Token. The sub Claim in
 	// the UserInfo Response MUST be verified to exactly match the sub Claim in the ID Token; if they do not match,
 	// the UserInfo Response values MUST NOT be used.
 	//
 	// http://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse
-	if len(userInfo.Subject) == 0 || userInfo.Subject != idTokenSubject {
+	// If there is no ID token and it is not required, we must assume that the caller is performing other checks
+	// to ensure the subject is correct.
+	checkIDToken := requireIDToken || len(idTokenSubject) > 0
+	if checkIDToken && (len(userInfo.Subject) == 0 || userInfo.Subject != idTokenSubject) {
 		return httperr.Newf(http.StatusUnprocessableEntity, "userinfo 'sub' claim (%s) did not match id_token 'sub' claim (%s)", userInfo.Subject, idTokenSubject)
 	}
 
+	// keep track of the issuer from the ID token
+	idTokenIssuer := claims["iss"]
+
 	// merge existing claims with user info claims
 	if err := userInfo.Claims(&claims); err != nil {
 		return httperr.Wrap(http.StatusInternalServerError, "could not unmarshal user info claims", err)
 	}
+	//  The OIDC spec for the UserInfo response does not make any guarantees about the iss claim's existence or validity:
+	//  "If signed, the UserInfo Response SHOULD contain the Claims iss (issuer) and aud (audience) as members. The iss value SHOULD be the OP's Issuer Identifier URL."
+	//  See https://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse
+	//  So we just ignore it and use it the version from the id token, which has stronger guarantees.
+	delete(claims, "iss")
+	if idTokenIssuer != nil {
+		claims["iss"] = idTokenIssuer
+	}
 
 	maybeLogClaims("claims from ID token and userinfo", p.Name, claims)
 
 	return nil
 }
 
+func (p *ProviderConfig) maybeFetchUserInfo(ctx context.Context, tok *oauth2.Token, requireUserInfo bool) (*coreosoidc.UserInfo, error) {
+	// implementing the user info endpoint is not required by the OIDC spec, but we may require it in certain situations.
+	if !p.HasUserInfoURL() {
+		if requireUserInfo {
+			// TODO should these all be http errors?
+			return nil, httperr.New(http.StatusInternalServerError, "userinfo endpoint not found, but is required")
+		}
+		return nil, nil
+	}
+
+	userInfo, err := p.Provider.UserInfo(coreosoidc.ClientContext(ctx, p.Client), oauth2.StaticTokenSource(tok))
+	if err != nil {
+		return nil, httperr.Wrap(http.StatusInternalServerError, "could not get user info", err)
+	}
+	return userInfo, nil
+}
+
 func maybeLogClaims(msg, name string, claims map[string]interface{}) {
 	if plog.Enabled(plog.LevelAll) { // log keys and values at all level
 		data, _ := json.Marshal(claims) // nothing we can do if it fails, but it really never should

internal/upstreamoidc/upstreamoidc_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package upstreamoidc
@@ -41,6 +41,9 @@ func TestProviderConfig(t *testing.T) {
 				Endpoint: oauth2.Endpoint{AuthURL: "https://example.com"},
 				Scopes:   []string{"scope1", "scope2"},
 			},
+			Provider: &mockProvider{
+				rawClaims: []byte(`{"userinfo_endpoint": "https://example.com/userinfo"}`),
+			},
 		}
 		require.Equal(t, "test-name", p.GetName())
 		require.Equal(t, "test-client-id", p.GetClientID())
@@ -55,6 +58,16 @@ func TestProviderConfig(t *testing.T) {
 		require.True(t, p.AllowsPasswordGrant())
 		p.AllowPasswordGrant = false
 		require.False(t, p.AllowsPasswordGrant())
+
+		require.True(t, p.HasUserInfoURL())
+		p.Provider = &mockProvider{
+			rawClaims: []byte(`{"some_other_endpoint": "https://example.com/blah"}`),
+		}
+		require.False(t, p.HasUserInfoURL())
+		p.Provider = &mockProvider{
+			rawClaims: []byte(`{`),
+		}
+		require.False(t, p.HasUserInfoURL())
 	})
 
 	const (
@@ -707,6 +720,399 @@ func TestProviderConfig(t *testing.T) {
 		}
 	})
 
+	t.Run("ValidateTokenAndMergeWithUserInfo", func(t *testing.T) {
+		expiryTime := time.Now().Add(42 * time.Second)
+		testTokenWithoutIDToken := &oauth2.Token{
+			AccessToken: "test-access-token",
+			// the library sets the original refresh token into the result, even though the server did not return that
+			RefreshToken: "test-initial-refresh-token",
+			TokenType:    "test-token-type",
+			Expiry:       expiryTime,
+		}
+		// generated from jwt.io
+		// sub: some-subject
+		// iss: some-issuer
+		// nonce: some-nonce
+		goodIDToken := "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJub25jZSI6InNvbWUtbm9uY2UiLCJpc3MiOiJzb21lLWlzc3VlciJ9.eGvzOihLUqzn3M4k6fHsToedgy7Fu89_Xu_u4mwMgRlIyRWZqmEMV76RVLnZd9Ihm9j_VpvrpirIkaj4JM9eRNfLX1n328cmBivBwnTKAzHuTm17dUKO5EvdTmQzmwnN0WZ8nWk4GfR7RzcvE1V8G9tIiWD8FkO3Dr-NR_zTun3N37onAazVLCmF0SDtATDfUH1ETqviHEp8xGx5HD5mv5T3HEjOuer5gxTEnfncef0LurBH3po-C0tXHKu74PD8x88CMJ1DLsRdCalnctwa850slKPkBSTP-ssh0JVg7cdMXoosVpwiXtKYaBkrhu8VS018aFP-cBbW0mYwsHmt3g" //nolint:gosec
+
+		tests := []struct {
+			name             string
+			tok              *oauth2.Token
+			nonce            nonce.Nonce
+			requireIDToken   bool
+			requireUserInfo  bool
+			userInfo         *oidc.UserInfo
+			rawClaims        []byte
+			userInfoErr      error
+			wantErr          string
+			wantMergedTokens *oidctypes.Token
+		}{
+			{
+				name:           "token with id, access and refresh tokens, valid nonce, and no userinfo",
+				tok:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}),
+				nonce:          "some-nonce",
+				requireIDToken: true,
+				rawClaims:      []byte(`{"userinfo_endpoint": "not-empty"}`),
+				wantMergedTokens: &oidctypes.Token{
+					AccessToken: &oidctypes.AccessToken{
+						Token:  "test-access-token",
+						Type:   "test-token-type",
+						Expiry: metav1.NewTime(expiryTime),
+					},
+					RefreshToken: &oidctypes.RefreshToken{
+						Token: "test-initial-refresh-token",
+					},
+					IDToken: &oidctypes.IDToken{
+						Token: goodIDToken,
+						Claims: map[string]interface{}{
+							"iss":   "some-issuer",
+							"nonce": "some-nonce",
+							"sub":   "some-subject",
+						},
+					},
+				},
+			},
+			{
+				name:           "id token not required but is provided",
+				tok:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}),
+				nonce:          "some-nonce",
+				requireIDToken: false,
+				rawClaims:      []byte(`{"userinfo_endpoint": "not-empty"}`),
+				userInfo:       forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`),
+				wantMergedTokens: &oidctypes.Token{
+					AccessToken: &oidctypes.AccessToken{
+						Token:  "test-access-token",
+						Type:   "test-token-type",
+						Expiry: metav1.NewTime(expiryTime),
+					},
+					RefreshToken: &oidctypes.RefreshToken{
+						Token: "test-initial-refresh-token",
+					},
+					IDToken: &oidctypes.IDToken{
+						Token: goodIDToken,
+						Claims: map[string]interface{}{
+							"iss":   "some-issuer",
+							"nonce": "some-nonce",
+							"sub":   "some-subject",
+							"name":  "Pinny TheSeal",
+						},
+					},
+				},
+			},
+			{
+				name:           "token with id, access and refresh tokens, valid nonce, and userinfo with a value that doesn't exist in the id token",
+				tok:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}),
+				nonce:          "some-nonce",
+				requireIDToken: true,
+				rawClaims:      []byte(`{"userinfo_endpoint": "not-empty"}`),
+				userInfo:       forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`),
+				wantMergedTokens: &oidctypes.Token{
+					AccessToken: &oidctypes.AccessToken{
+						Token:  "test-access-token",
+						Type:   "test-token-type",
+						Expiry: metav1.NewTime(expiryTime),
+					},
+					RefreshToken: &oidctypes.RefreshToken{
+						Token: "test-initial-refresh-token",
+					},
+					IDToken: &oidctypes.IDToken{
+						Token: goodIDToken,
+						Claims: map[string]interface{}{
+							"iss":   "some-issuer",
+							"nonce": "some-nonce",
+							"sub":   "some-subject",
+							"name":  "Pinny TheSeal",
+						},
+					},
+				},
+			},
+			{
+				name:            "userinfo is required, token with id, access and refresh tokens, valid nonce, and userinfo with a value that doesn't exist in the id token",
+				tok:             testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}),
+				nonce:           "some-nonce",
+				requireIDToken:  true,
+				requireUserInfo: true,
+				rawClaims:       []byte(`{"userinfo_endpoint": "not-empty"}`),
+				userInfo:        forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`),
+				wantMergedTokens: &oidctypes.Token{
+					AccessToken: &oidctypes.AccessToken{
+						Token:  "test-access-token",
+						Type:   "test-token-type",
+						Expiry: metav1.NewTime(expiryTime),
+					},
+					RefreshToken: &oidctypes.RefreshToken{
+						Token: "test-initial-refresh-token",
+					},
+					IDToken: &oidctypes.IDToken{
+						Token: goodIDToken,
+						Claims: map[string]interface{}{
+							"iss":   "some-issuer",
+							"nonce": "some-nonce",
+							"sub":   "some-subject",
+							"name":  "Pinny TheSeal",
+						},
+					},
+				},
+			},
+			{
+				name:           "claims from userinfo override id token claims",
+				tok:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiSm9obiBEb2UiLCJpc3MiOiJzb21lLWlzc3VlciIsIm5vbmNlIjoic29tZS1ub25jZSJ9.sBWi3_4cfGwrmMFZWkCghw4uvCnHN35h9xNX1gkwOtj6Oz_yKqpj7wfO4AqeWsRyrDGnkmIZbVuhAAJqPSi4GlNzN4NU8zh53PGDUpFlpDI1dvqDjIRb9iIEJpRIj34--Sz41H0ooxviIzvUdZFvQlaSzLOqgjR3ddHe2urhbtUuz_DsabP84AWo2DSg0y3ull6DRvk_DvzC6HNN8JwVi08fFvvV9BVq8kjdVeob7gajJkuGSTjsxNZGs5rbBuxBx0MZTQ8boR1fDNdG70GoIb4SsCoBSs7pZxtmGZPHInteY1SilHDDDmpQuE-LvSmvvPN_Cyk1d3eS-IR7hBbCAA"}),
+				nonce:          "some-nonce",
+				requireIDToken: true,
+				rawClaims:      []byte(`{"userinfo_endpoint": "not-empty"}`),
+				userInfo:       forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`),
+				wantMergedTokens: &oidctypes.Token{
+					AccessToken: &oidctypes.AccessToken{
+						Token:  "test-access-token",
+						Type:   "test-token-type",
+						Expiry: metav1.NewTime(expiryTime),
+					},
+					RefreshToken: &oidctypes.RefreshToken{
+						Token: "test-initial-refresh-token",
+					},
+					IDToken: &oidctypes.IDToken{
+						Token: "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiSm9obiBEb2UiLCJpc3MiOiJzb21lLWlzc3VlciIsIm5vbmNlIjoic29tZS1ub25jZSJ9.sBWi3_4cfGwrmMFZWkCghw4uvCnHN35h9xNX1gkwOtj6Oz_yKqpj7wfO4AqeWsRyrDGnkmIZbVuhAAJqPSi4GlNzN4NU8zh53PGDUpFlpDI1dvqDjIRb9iIEJpRIj34--Sz41H0ooxviIzvUdZFvQlaSzLOqgjR3ddHe2urhbtUuz_DsabP84AWo2DSg0y3ull6DRvk_DvzC6HNN8JwVi08fFvvV9BVq8kjdVeob7gajJkuGSTjsxNZGs5rbBuxBx0MZTQ8boR1fDNdG70GoIb4SsCoBSs7pZxtmGZPHInteY1SilHDDDmpQuE-LvSmvvPN_Cyk1d3eS-IR7hBbCAA",
+						Claims: map[string]interface{}{
+							"iss":   "some-issuer", // takes the issuer from the ID token, since the userinfo one is unreliable.
+							"nonce": "some-nonce",
+							"sub":   "some-subject",
+							"name":  "Pinny TheSeal",
+						},
+					},
+				},
+			},
+			{
+				name:           "token with id, access and refresh tokens and valid nonce, but userinfo has a different issuer",
+				tok:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}),
+				nonce:          "some-nonce",
+				requireIDToken: true,
+				rawClaims:      []byte(`{"userinfo_endpoint": "not-empty"}`),
+				userInfo:       forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "iss": "some-other-issuer", "sub": "some-subject"}`),
+				wantMergedTokens: &oidctypes.Token{
+					AccessToken: &oidctypes.AccessToken{
+						Token:  "test-access-token",
+						Type:   "test-token-type",
+						Expiry: metav1.NewTime(expiryTime),
+					},
+					RefreshToken: &oidctypes.RefreshToken{
+						Token: "test-initial-refresh-token",
+					},
+					IDToken: &oidctypes.IDToken{
+						Token: goodIDToken,
+						Claims: map[string]interface{}{
+							"iss":   "some-issuer", // takes the issuer from the ID token, since the userinfo one is unreliable.
+							"nonce": "some-nonce",
+							"sub":   "some-subject",
+							"name":  "Pinny TheSeal",
+						},
+					},
+				},
+			},
+			{
+				name:            "token with id, access and refresh tokens and valid nonce, but no userinfo endpoint from discovery and it's not required",
+				tok:             testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}),
+				nonce:           "some-nonce",
+				requireIDToken:  true,
+				requireUserInfo: false,
+				rawClaims:       []byte(`{"not_the_userinfo_endpoint": "some-other-endpoint"}`),
+				wantMergedTokens: &oidctypes.Token{
+					AccessToken: &oidctypes.AccessToken{
+						Token:  "test-access-token",
+						Type:   "test-token-type",
+						Expiry: metav1.NewTime(expiryTime),
+					},
+					RefreshToken: &oidctypes.RefreshToken{
+						Token: "test-initial-refresh-token",
+					},
+					IDToken: &oidctypes.IDToken{
+						Token: goodIDToken,
+						Claims: map[string]interface{}{
+							"iss":   "some-issuer",
+							"nonce": "some-nonce",
+							"sub":   "some-subject",
+						},
+					},
+				},
+			},
+			{
+				name:           "token with no id token but valid userinfo",
+				tok:            testTokenWithoutIDToken,
+				nonce:          "",
+				requireIDToken: false,
+				rawClaims:      []byte(`{"userinfo_endpoint": "not-empty"}`),
+				userInfo:       forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "iss": "some-other-issuer", "sub": "some-subject"}`),
+				wantMergedTokens: &oidctypes.Token{
+					AccessToken: &oidctypes.AccessToken{
+						Token:  "test-access-token",
+						Type:   "test-token-type",
+						Expiry: metav1.NewTime(expiryTime),
+					},
+					RefreshToken: &oidctypes.RefreshToken{
+						Token: "test-initial-refresh-token",
+					},
+					IDToken: &oidctypes.IDToken{
+						Token: "",
+						Claims: map[string]interface{}{
+							"sub":  "some-subject",
+							"name": "Pinny TheSeal",
+						},
+					},
+				},
+			},
+			{
+				name:           "token with neither id token nor userinfo",
+				tok:            testTokenWithoutIDToken,
+				nonce:          "",
+				requireIDToken: false,
+				rawClaims:      []byte(`{"userinfo_endpoint": "not-empty"}`),
+				wantMergedTokens: &oidctypes.Token{
+					AccessToken: &oidctypes.AccessToken{
+						Token:  "test-access-token",
+						Type:   "test-token-type",
+						Expiry: metav1.NewTime(expiryTime),
+					},
+					RefreshToken: &oidctypes.RefreshToken{
+						Token: "test-initial-refresh-token",
+					},
+					IDToken: &oidctypes.IDToken{
+						Claims: map[string]interface{}{},
+					},
+				},
+			},
+			{
+				name:           "token with id, access and refresh tokens, valid nonce, and userinfo subject that doesn't match",
+				tok:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}),
+				nonce:          "some-nonce",
+				requireIDToken: true,
+				rawClaims:      []byte(`{"userinfo_endpoint": "not-empty"}`),
+				userInfo:       forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal", "sub": "some-other-subject"}`),
+				wantErr:        "could not fetch user info claims: userinfo 'sub' claim (some-other-subject) did not match id_token 'sub' claim (some-subject)",
+			},
+			{
+				name:           "id token not required but is provided, and subjects don't match",
+				tok:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}),
+				nonce:          "some-nonce",
+				requireIDToken: false,
+				rawClaims:      []byte(`{"userinfo_endpoint": "not-empty"}`),
+				userInfo:       forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal", "sub": "some-other-subject"}`),
+				wantErr:        "could not fetch user info claims: userinfo 'sub' claim (some-other-subject) did not match id_token 'sub' claim (some-subject)",
+			},
+			{
+				name:           "invalid id token",
+				tok:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "not-an-id-token"}),
+				nonce:          "some-nonce",
+				requireIDToken: true,
+				rawClaims:      []byte(`{"userinfo_endpoint": "not-empty"}`),
+				userInfo:       forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal", "sub": "some-other-subject"}`),
+				wantErr:        "received invalid ID token: oidc: malformed jwt: square/go-jose: compact JWS format must have three parts",
+			},
+			{
+				name:           "invalid nonce",
+				tok:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}),
+				nonce:          "some-other-nonce",
+				requireIDToken: true,
+				rawClaims:      []byte(`{"userinfo_endpoint": "not-empty"}`),
+				userInfo:       forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal", "sub": "some-other-subject"}`),
+				wantErr:        "received ID token with invalid nonce: invalid nonce (expected \"some-other-nonce\", got \"some-nonce\")",
+			},
+			{
+				name:           "expected to have id token, but doesn't",
+				tok:            testTokenWithoutIDToken,
+				nonce:          "some-other-nonce",
+				requireIDToken: true,
+				rawClaims:      []byte(`{"userinfo_endpoint": "not-empty"}`),
+				userInfo:       forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`),
+				wantErr:        "received response missing ID token",
+			},
+			{
+				name:            "expected to have userinfo, but doesn't",
+				tok:             testTokenWithoutIDToken,
+				nonce:           "some-other-nonce",
+				requireUserInfo: true,
+				rawClaims:       []byte(`{}`),
+				wantErr:         "could not fetch user info claims: userinfo endpoint not found, but is required",
+			},
+			{
+				name:            "expected to have id token and userinfo, but doesn't have either",
+				tok:             testTokenWithoutIDToken,
+				nonce:           "some-other-nonce",
+				requireUserInfo: true,
+				requireIDToken:  true,
+				rawClaims:       []byte(`{}`),
+				wantErr:         "received response missing ID token",
+			},
+			{
+				name:           "mismatched access token hash",
+				tok:            testTokenWithoutIDToken,
+				nonce:          "some-other-nonce",
+				requireIDToken: true,
+				rawClaims:      []byte(`{"userinfo_endpoint": "not-empty"}`),
+				userInfo:       forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`),
+				wantErr:        "received response missing ID token",
+			},
+			{
+				name:           "id token missing subject, skip userinfo check",
+				tok:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoiSm9obiBEb2UiLCJpc3MiOiJzb21lLWlzc3VlciIsIm5vbmNlIjoic29tZS1ub25jZSJ9.aIhrhikAnQ4Mb1g6RAT08qqflT2LLLi2yj4F2S4zud8nYad4tfEd2ITVJ4Njdjf70ubqyzZ6XxojtC4OqaWbDaQOcd95sd3PW58SYrf4NMvEStFkcMG0HMhJEZLVGnuJQstuq3G9h5Z5bFCkx4mFNo5ho_isBWyHpk-uF14duXXlIDB10SnyZ9dRbcmu-3mMOq0g4oCUPEDiHWkv-Rf70Mk0harL2xvcpxlSMLK4glDfiiki5gl6IReIo4rTVosXAqv3JmjLDeVLtJQRG6F8YcIlDCIfUEUfk0GeYacBVjoDIO570ywVJy1LGvyUuvgXNQUjq2JgzCfb8HWGp7iJdQ"}),
+				nonce:          "some-nonce",
+				requireIDToken: true,
+				rawClaims:      []byte(`{"userinfo_endpoint": "not-empty"}`),
+				userInfo:       forceUserInfoWithClaims("some-other-subject", `{"name": "Pinny TheSeal", "sub": "some-other-subject"}`),
+				wantMergedTokens: &oidctypes.Token{
+					AccessToken: &oidctypes.AccessToken{
+						Token:  "test-access-token",
+						Type:   "test-token-type",
+						Expiry: metav1.NewTime(expiryTime),
+					},
+					RefreshToken: &oidctypes.RefreshToken{
+						Token: "test-initial-refresh-token",
+					},
+					IDToken: &oidctypes.IDToken{
+						Token: "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJuYW1lIjoiSm9obiBEb2UiLCJpc3MiOiJzb21lLWlzc3VlciIsIm5vbmNlIjoic29tZS1ub25jZSJ9.aIhrhikAnQ4Mb1g6RAT08qqflT2LLLi2yj4F2S4zud8nYad4tfEd2ITVJ4Njdjf70ubqyzZ6XxojtC4OqaWbDaQOcd95sd3PW58SYrf4NMvEStFkcMG0HMhJEZLVGnuJQstuq3G9h5Z5bFCkx4mFNo5ho_isBWyHpk-uF14duXXlIDB10SnyZ9dRbcmu-3mMOq0g4oCUPEDiHWkv-Rf70Mk0harL2xvcpxlSMLK4glDfiiki5gl6IReIo4rTVosXAqv3JmjLDeVLtJQRG6F8YcIlDCIfUEUfk0GeYacBVjoDIO570ywVJy1LGvyUuvgXNQUjq2JgzCfb8HWGp7iJdQ",
+						Claims: map[string]interface{}{
+							"iss":   "some-issuer",
+							"name":  "John Doe",
+							"nonce": "some-nonce",
+						},
+					},
+				},
+			},
+		}
+
+		for _, tt := range tests {
+			tt := tt
+			t.Run(tt.name, func(t *testing.T) {
+				p := ProviderConfig{
+					Name:          "test-name",
+					UsernameClaim: "test-username-claim",
+					GroupsClaim:   "test-groups-claim",
+					Config: &oauth2.Config{
+						ClientID:     "test-client-id",
+						ClientSecret: "test-client-secret",
+						Endpoint: oauth2.Endpoint{
+							AuthURL:   "https://example.com",
+							TokenURL:  "https://example.com",
+							AuthStyle: oauth2.AuthStyleInParams,
+						},
+						Scopes: []string{"scope1", "scope2"},
+					},
+					Provider: &mockProvider{
+						rawClaims:   tt.rawClaims,
+						userInfo:    tt.userInfo,
+						userInfoErr: tt.userInfoErr,
+					},
+				}
+				gotTok, err := p.ValidateTokenAndMergeWithUserInfo(context.Background(), tt.tok, tt.nonce, tt.requireIDToken, tt.requireUserInfo)
+				if tt.wantErr != "" {
+					require.Error(t, err)
+					require.Equal(t, tt.wantErr, err.Error())
+				} else {
+					require.NoError(t, err)
+					require.Equal(t, tt.wantMergedTokens, gotTok)
+				}
+			})
+		}
+	})
+
 	t.Run("ExchangeAuthcodeAndValidateTokens", func(t *testing.T) {
 		tests := []struct {
 			name        string
@@ -779,6 +1185,36 @@ func TestProviderConfig(t *testing.T) {
 				rawClaims:          []byte(`{}`), // user info not supported
 				wantUserInfoCalled: false,
 			},
+			{
+				name:        "valid but userinfo endpoint could not be found due to parse error",
+				authCode:    "valid",
+				returnIDTok: validIDToken,
+				wantToken: oidctypes.Token{
+					AccessToken: &oidctypes.AccessToken{
+						Token:  "test-access-token",
+						Expiry: metav1.Time{},
+					},
+					RefreshToken: &oidctypes.RefreshToken{
+						Token: "test-refresh-token",
+					},
+					IDToken: &oidctypes.IDToken{
+						Token:  validIDToken,
+						Expiry: metav1.Time{},
+						Claims: map[string]interface{}{
+							"foo": "bar",
+							"bat": "baz",
+							"aud": "test-client-id",
+							"iat": 1.606768593e+09,
+							"jti": "test-jti",
+							"nbf": 1.606768593e+09,
+							"sub": "test-user",
+						},
+					},
+				},
+				// cannot be parsed as json, but note that in this case constructing a real provider would have failed
+				rawClaims:          []byte(`{`),
+				wantUserInfoCalled: false,
+			},
 			{
 				name:        "valid",
 				authCode:    "valid",
@@ -808,13 +1244,6 @@ func TestProviderConfig(t *testing.T) {
 				rawClaims:          []byte(`{}`), // user info not supported
 				wantUserInfoCalled: false,
 			},
-			{
-				name:        "user info discovery parse error",
-				authCode:    "valid",
-				returnIDTok: validIDToken,
-				rawClaims:   []byte(`junk`), // user info discovery fails
-				wantErr:     "could not fetch user info claims: could not unmarshal discovery JSON: invalid character 'j' looking for beginning of value",
-			},
 			{
 				name:        "user info fetch error",
 				authCode:    "valid",