Check username claim is unchanged for oidc.

Also add integration tests for claims changing.

internal/oidc/token/token_handler.go

@@ -6,6 +6,7 @@ package token
 
 import (
 	"context"
+	"errors"
 	"net/http"
 
 	"github.com/ory/fosite"
@@ -142,12 +143,23 @@ func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession,
 		oldDownstreamSubject := session.Fosite.Claims.Subject
 		oldSub, err := upstreamoidc.ExtractUpstreamSubjectFromDownstream(oldDownstreamSubject)
 		if err != nil {
-			return errorsx.WithStack(errUpstreamRefreshError.WithHintf(
+			return errorsx.WithStack(errUpstreamRefreshError.WithHintf("Upstream refresh failed.").
-				"Could not verify upstream refresh subject against previous value").WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
+				WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
 		}
 		if oldSub != newSub {
 			return errorsx.WithStack(errUpstreamRefreshError.WithHintf(
-				"Subject in upstream refresh does not match previous value").WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
+				"Upstream refresh failed.").WithWrap(errors.New("subject in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
+		}
+		usernameClaim := p.GetUsernameClaim()
+		newUsername := claims[usernameClaim]
+		// its possible this won't be returned.
+		// but if it is, verify that it hasn't changed.
+		if newUsername != nil {
+			oldUsername := session.Fosite.Claims.Extra["username"]
+			if oldUsername != newUsername {
+				return errorsx.WithStack(errUpstreamRefreshError.WithHintf(
+					"Upstream refresh failed.").WithWrap(errors.New("username in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
+			}
 		}
 	}
 

internal/oidc/token/token_handler_test.go

@@ -22,8 +22,6 @@ import (
 	"testing"
 	"time"
 
-	"go.pinniped.dev/pkg/oidcclient/oidctypes"
-
 	"github.com/ory/fosite"
 	fositeoauth2 "github.com/ory/fosite/handler/oauth2"
 	"github.com/ory/fosite/handler/openid"
@@ -54,6 +52,7 @@ import (
 	"go.pinniped.dev/internal/psession"
 	"go.pinniped.dev/internal/testutil"
 	"go.pinniped.dev/internal/testutil/oidctestutil"
+	"go.pinniped.dev/pkg/oidcclient/oidctypes"
 )
 
 const (
@@ -1067,6 +1066,30 @@ func TestRefreshGrant(t *testing.T) {
 				),
 			},
 		},
+		{
+			name: "refresh grant with unchanged username claim",
+			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
+				upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedTokens(&oidctypes.Token{
+					IDToken: &oidctypes.IDToken{
+						Claims: map[string]interface{}{
+							"some-claim":     "some-value",
+							"sub":            "some-subject",
+							"username-claim": goodUsername,
+						},
+					},
+				}).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()),
+			authcodeExchange: authcodeExchangeInputs{
+				customSessionData: initialUpstreamOIDCCustomSessionData(),
+				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") },
+				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()),
+			},
+			refreshRequest: refreshRequestInputs{
+				want: happyRefreshTokenResponseForOpenIDAndOfflineAccess(
+					upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken),
+					refreshedUpstreamTokensWithIDAndRefreshTokens(),
+				),
+			},
+		},
 		{
 			name: "happy path refresh grant without openid scope granted (no id token returned)",
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
@@ -1617,7 +1640,67 @@ func TestRefreshGrant(t *testing.T) {
 					wantErrorResponseBody: here.Doc(`
 						{
 							"error":             "error",
-							"error_description": "Error during upstream refresh. Subject in upstream refresh does not match previous value"
+							"error_description": "Error during upstream refresh. Upstream refresh failed."
+						}
+					`),
+				},
+			},
+		},
+		{
+			name: "refresh grant with claims but not the subject claim",
+			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
+				upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{
+					IDToken: &oidctypes.IDToken{
+						Claims: map[string]interface{}{
+							"some-claim": "some-value",
+						},
+					},
+				}).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()),
+			authcodeExchange: authcodeExchangeInputs{
+				customSessionData: initialUpstreamOIDCCustomSessionData(),
+				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") },
+				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()),
+			},
+			refreshRequest: refreshRequestInputs{
+				want: tokenEndpointResponseExpectedValues{
+					wantUpstreamRefreshCall:           happyOIDCUpstreamRefreshCall(),
+					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()),
+					wantStatus:                        http.StatusUnauthorized,
+					wantErrorResponseBody: here.Doc(`
+						{
+							"error":             "error",
+							"error_description": "Error during upstream refresh. Upstream refresh failed."
+						}
+					`),
+				},
+			},
+		},
+		{
+			name: "refresh grant with changed username claim",
+			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
+				upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedTokens(&oidctypes.Token{
+					IDToken: &oidctypes.IDToken{
+						Claims: map[string]interface{}{
+							"some-claim":     "some-value",
+							"sub":            "some-subject",
+							"username-claim": "some-changed-username",
+						},
+					},
+				}).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()),
+			authcodeExchange: authcodeExchangeInputs{
+				customSessionData: initialUpstreamOIDCCustomSessionData(),
+				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") },
+				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()),
+			},
+			refreshRequest: refreshRequestInputs{
+				want: tokenEndpointResponseExpectedValues{
+					wantUpstreamRefreshCall:           happyOIDCUpstreamRefreshCall(),
+					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()),
+					wantStatus:                        http.StatusUnauthorized,
+					wantErrorResponseBody: here.Doc(`
+						{
+							"error":             "error",
+							"error_description": "Error during upstream refresh. Upstream refresh failed."
 						}
 					`),
 				},

internal/upstreamoidc/upstreamoidc.go

@@ -242,7 +242,7 @@ func ExtractUpstreamSubjectFromDownstream(downstreamSubject string) (string, err
 	if !strings.Contains(downstreamSubject, "?sub=") {
 		return "", errors.New("downstream subject did not contain original upstream subject")
 	}
-	return strings.SplitN(downstreamSubject, "?sub=", 2)[1], nil // TODO test for ?sub= occurring twice (imagine if you ran the supervisor with another supervisor as the upstream idp...)
+	return strings.SplitN(downstreamSubject, "?sub=", 2)[1], nil
 }
 
 // ValidateToken will validate the ID token. It will also merge the claims from the userinfo endpoint response,

internal/upstreamoidc/upstreamoidc_test.go

@@ -910,6 +910,45 @@ func TestProviderConfig(t *testing.T) {
 		}
 	})
 
+	t.Run("ExtractUpstreamSubjectFromDownstream", func(t *testing.T) {
+		tests := []struct {
+			name                string
+			downstreamSubject   string
+			wantUpstreamSubject string
+			wantErr             string
+		}{
+			{
+				name:                "happy path",
+				downstreamSubject:   "https://some-issuer?sub=some-subject",
+				wantUpstreamSubject: "some-subject",
+			},
+			{
+				name:                "subject in a subject",
+				downstreamSubject:   "https://some-other-issuer?sub=https://some-issuer?sub=some-subject",
+				wantUpstreamSubject: "https://some-issuer?sub=some-subject",
+			},
+			{
+				name:              "doesn't contain sub=",
+				downstreamSubject: "something-invalid",
+				wantErr:           "downstream subject did not contain original upstream subject",
+			},
+		}
+		for _, tt := range tests {
+			tt := tt
+			t.Run(tt.name, func(t *testing.T) {
+				actualUpstreamSubject, err := ExtractUpstreamSubjectFromDownstream(tt.downstreamSubject)
+				if tt.wantErr != "" {
+					require.Error(t, err)
+					require.Equal(t, tt.wantErr, err.Error())
+				} else {
+					require.NoError(t, err)
+					require.Equal(t, tt.wantUpstreamSubject, actualUpstreamSubject)
+				}
+			})
+		}
+
+	})
+
 	t.Run("ExchangeAuthcodeAndValidateTokens", func(t *testing.T) {
 		tests := []struct {
 			name        string

test/integration/supervisor_login_test.go

@@ -87,10 +87,8 @@ func TestSupervisorLogin(t *testing.T) {
 			},
 			requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlow,
 			breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _, _ string) {
-				customSessionData := pinnipedSession.Custom
+				fositeSessionData := pinnipedSession.Fosite
-				require.Equal(t, psession.ProviderTypeOIDC, customSessionData.ProviderType)
+				fositeSessionData.Claims.Subject = "wrong-subject"
-				require.NotEmpty(t, customSessionData.OIDC.UpstreamRefreshToken)
-				customSessionData.OIDC.UpstreamRefreshToken = "invalid-updated-refresh-token"
 			},
 			// the ID token Subject should include the upstream user ID after the upstream issuer name
 			wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+",
@@ -124,10 +122,8 @@ func TestSupervisorLogin(t *testing.T) {
 			},
 			requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlow,
 			breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _, _ string) {
-				customSessionData := pinnipedSession.Custom
+				fositeSessionData := pinnipedSession.Fosite
-				require.Equal(t, psession.ProviderTypeOIDC, customSessionData.ProviderType)
+				fositeSessionData.Claims.Extra["username"] = "some-incorrect-username"
-				require.NotEmpty(t, customSessionData.OIDC.UpstreamRefreshToken)
-				customSessionData.OIDC.UpstreamRefreshToken = "invalid-updated-refresh-token"
 			},
 			wantDownstreamIDTokenSubjectToMatch:  "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+",
 			wantDownstreamIDTokenUsernameToMatch: func(_ string) string { return "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Username) + "$" },