Address code review feedback

Fixed issues from code review of 94fe0dc:
- HIGH: Add ClientSecretExpiresAt to DCRCredentials so the Redis
  backend (sub-issue 2) can satisfy the TTL contract without re-touching
  the value type. Round-trip test asserts the new field is preserved.
- MEDIUM: Reject creds with empty Key.Issuer or empty Key.RedirectURI
  in StoreDCRCredentials, matching the validation pattern used by
  StoreUpstreamTokens. Empty ScopesHash is intentionally accepted —
  ScopesHash(nil) is a non-empty digest, so the empty-scope-registration
  path stays valid.
- MEDIUM: Drop runner-side TestScopesHash_* tests; the storage-side
  suite already exercises the canonical implementation. Replaced with
  a one-line comment pointing readers to the canonical tests.
- MEDIUM: Add DCRCredentials count to MemoryStorage.Stats for parity
  with every other in-memory map.
- MEDIUM: Reconcile the interface contract — couple the TTL clause to
  the new ClientSecretExpiresAt field, weaken MUST to SHOULD, and
  document that backends without native TTL retain the field verbatim
  for caller-side re-check.

Author: tgrunnagle

pkg/authserver/runner/dcr_store_test.go

@@ -168,75 +168,11 @@ func TestInMemoryDCRCredentialStore_GetReturnsDefensiveCopy(t *testing.T) {
 	assert.Equal(t, "orig", refetched.ClientID)
 }
 
-func TestScopesHash_StableAcrossPermutation(t *testing.T) {
-	t.Parallel()
-
-	tests := []struct {
-		name string
-		a, b []string
-	}{
-		{
-			name: "two-element permutation",
-			a:    []string{"openid", "profile"},
-			b:    []string{"profile", "openid"},
-		},
-		{
-			name: "three-element permutation",
-			a:    []string{"openid", "profile", "email"},
-			b:    []string{"email", "openid", "profile"},
-		},
-		{
-			// OAuth scope sets are sets, not multisets (RFC 6749 §3.3).
-			// scopesHash deduplicates before hashing so a caller who
-			// accidentally repeats a scope still hits the cache entry
-			// keyed under the canonical set.
-			name: "single element equals double element duplicate",
-			a:    []string{"openid"},
-			b:    []string{"openid", "openid"},
-		},
-		{
-			name: "three-element with duplicate equals two-element unique",
-			a:    []string{"openid", "profile", "openid"},
-			b:    []string{"openid", "profile"},
-		},
-	}
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			t.Parallel()
-			assert.Equal(t, scopesHash(tc.a), scopesHash(tc.b))
-		})
-	}
-}
-
-func TestScopesHash_DistinctForDistinctScopes(t *testing.T) {
-	t.Parallel()
-
-	a := scopesHash([]string{"openid"})
-	b := scopesHash([]string{"openid", "profile"})
-	c := scopesHash([]string{"profile"})
-	d := scopesHash(nil)
-	e := scopesHash([]string{})
-
-	// Non-empty distinct sets produce distinct hashes.
-	assert.NotEqual(t, a, b)
-	assert.NotEqual(t, a, c)
-	assert.NotEqual(t, b, c)
-	assert.NotEqual(t, a, d)
-	// nil and empty slice canonicalise to the same hash (both sort-then-join
-	// to the empty canonical form).
-	assert.Equal(t, d, e)
-}
-
-func TestScopesHash_NoCollisionFromBoundaryJoin(t *testing.T) {
-	t.Parallel()
-
-	// Without a delimiter that cannot appear inside a scope value,
-	// ["ab", "c"] and ["a", "bc"] would collide. This test exists to
-	// prevent a regression if the canonical form is ever simplified.
-	h1 := scopesHash([]string{"ab", "c"})
-	h2 := scopesHash([]string{"a", "bc"})
-	assert.NotEqual(t, h1, h2)
-}
+// Tests for the canonical scopes-hash form live next to the canonical
+// implementation in pkg/authserver/storage/memory_test.go (TestScopesHash_*).
+// The runner-package binding `scopesHash = storage.ScopesHash` would only
+// re-exercise the same code, so duplicating the suite here would be redundant
+// per .claude/rules/testing.md.
 
 // TestInMemoryDCRCredentialStore_ConcurrentAccess fans out N goroutines
 // performing alternating Put / Get against overlapping and disjoint keys,

pkg/authserver/storage/memory.go

@@ -1219,12 +1219,25 @@ func cloneDCRCredentials(c *DCRCredentials) *DCRCredentials {
 // do not affect persisted state.
 //
 // Overwrites any existing entry for the same Key. The in-memory backend
-// applies no TTL — DCR registrations are long-lived and bounded by the
-// operator-configured upstream count.
+// applies no native TTL — DCR registrations are long-lived and bounded by
+// the operator-configured upstream count, and ClientSecretExpiresAt is
+// retained verbatim for callers to re-check on read (see the interface
+// docstring's "TTL handling" section).
+//
+// Rejects nil creds and an unpopulated Key (empty Issuer or empty
+// RedirectURI). An empty ScopesHash is permitted because ScopesHash("")
+// produces a non-empty canonical digest, and the empty-scope case
+// (no scopes configured or discovered) is a real, valid registration.
 func (s *MemoryStorage) StoreDCRCredentials(_ context.Context, creds *DCRCredentials) error {
 	if creds == nil {
 		return fosite.ErrInvalidRequest.WithHint("dcr credentials cannot be nil")
 	}
+	if creds.Key.Issuer == "" {
+		return fosite.ErrInvalidRequest.WithHint("dcr credentials key issuer cannot be empty")
+	}
+	if creds.Key.RedirectURI == "" {
+		return fosite.ErrInvalidRequest.WithHint("dcr credentials key redirect_uri cannot be empty")
+	}
 
 	s.mu.Lock()
 	defer s.mu.Unlock()
@@ -1268,6 +1281,7 @@ type Stats struct {
 	ClientAssertionJWTs   int
 	Users                 int
 	ProviderIdentities    int
+	DCRCredentials        int
 }
 
 // Stats returns current statistics about storage contents.
@@ -1288,6 +1302,7 @@ func (s *MemoryStorage) Stats() Stats {
 		ClientAssertionJWTs:   len(s.clientAssertionJWTs),
 		Users:                 len(s.users),
 		ProviderIdentities:    len(s.providerIdentities),
+		DCRCredentials:        len(s.dcrCredentials),
 	}
 }
 

pkg/authserver/storage/memory_test.go

@@ -1736,6 +1736,7 @@ func TestMemoryStorage_DCRCredentials_RoundTrip(t *testing.T) {
 			AuthorizationEndpoint:   "https://idp.example.com/authorize",
 			TokenEndpoint:           "https://idp.example.com/token",
 			CreatedAt:               time.Date(2025, 5, 1, 12, 0, 0, 0, time.UTC),
+			ClientSecretExpiresAt:   time.Date(2026, 5, 1, 12, 0, 0, 0, time.UTC),
 		}
 
 		require.NoError(t, s.StoreDCRCredentials(ctx, creds))
@@ -1797,12 +1798,48 @@ func TestMemoryStorage_DCRCredentials_NotFound(t *testing.T) {
 	})
 }
 
-func TestMemoryStorage_DCRCredentials_StoreNilRejected(t *testing.T) {
-	withStorage(t, func(ctx context.Context, s *MemoryStorage) {
-		err := s.StoreDCRCredentials(ctx, nil)
-		require.Error(t, err)
-		assert.ErrorIs(t, err, fosite.ErrInvalidRequest)
-	})
+// TestMemoryStorage_DCRCredentials_StoreInvalidInputRejected pins the
+// fail-loud-on-invalid-input contract: nil creds, an empty Key.Issuer, or
+// an empty Key.RedirectURI must be rejected with fosite.ErrInvalidRequest
+// rather than producing a working-looking write under a partially-populated
+// key.
+func TestMemoryStorage_DCRCredentials_StoreInvalidInputRejected(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name  string
+		creds *DCRCredentials
+	}{
+		{
+			name:  "nil creds",
+			creds: nil,
+		},
+		{
+			name: "empty issuer",
+			creds: &DCRCredentials{
+				Key: DCRKey{Issuer: "", RedirectURI: "https://x/cb"},
+			},
+		},
+		{
+			name: "empty redirect_uri",
+			creds: &DCRCredentials{
+				Key: DCRKey{Issuer: "https://idp.example.com", RedirectURI: ""},
+			},
+		},
+	}
+	for _, tc := range tests {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			withStorage(t, func(ctx context.Context, s *MemoryStorage) {
+				err := s.StoreDCRCredentials(ctx, tc.creds)
+				require.Error(t, err)
+				assert.ErrorIs(t, err, fosite.ErrInvalidRequest)
+				// Confirm the rejection did not partially populate the store.
+				assert.Equal(t, 0, s.Stats().DCRCredentials,
+					"rejected Store must not leave any entry behind")
+			})
+		})
+	}
 }
 
 // TestMemoryStorage_DCRCredentials_GetReturnsDefensiveCopy pins the

pkg/authserver/storage/types.go

@@ -217,6 +217,20 @@ type DCRCredentials struct {
 	// not expire entries based on age (see ClientSecretExpiresAt for the
 	// authoritative expiry signal).
 	CreatedAt time.Time
+
+	// ClientSecretExpiresAt is the RFC 7591 §3.2.1 client_secret_expires_at
+	// value converted from int64 epoch seconds to time.Time. The wire
+	// convention is that 0 means "the secret does not expire"; this struct
+	// represents that as the zero time.Time so callers can use IsZero()
+	// rather than special-casing 0.
+	//
+	// When non-zero, this is the authoritative signal a backend uses to TTL
+	// the persisted entry: the Redis backend (sub-issue 2) plumbs it through
+	// SetEX so the row evicts before the upstream rejects the secret at the
+	// token endpoint. The in-memory backend ignores this field — entries
+	// persist for the process lifetime and the resolver re-checks the
+	// expiry on read.
+	ClientSecretExpiresAt time.Time
 }
 
 // DCRCredentialStore is a narrow, segregated interface for persisting
@@ -235,17 +249,26 @@ type DCRCredentials struct {
 // Implementations MUST defensively copy on both Store and Get so caller
 // mutations cannot reach persisted state and vice versa, mirroring the
 // UpstreamTokens contract.
+//
+// # TTL handling
+//
+// Implementations SHOULD honor a non-zero DCRCredentials.ClientSecretExpiresAt
+// as a backend-level TTL when the underlying store supports one (e.g. Redis
+// SetEX) so an entry evicts before the upstream rejects the secret at the
+// token endpoint. Backends without a native TTL (e.g. the in-memory backend)
+// retain the field verbatim and rely on the caller — typically the runner's
+// resolver — to re-check expiry on read; see MemoryStorage.GetDCRCredentials.
+// A zero ClientSecretExpiresAt means the upstream did not assert an expiry
+// and no TTL is applied.
 type DCRCredentialStore interface {
 	// GetDCRCredentials returns the credentials for the given key.
 	// Returns ErrNotFound (wrapped) if no entry exists for the key.
 	// The returned value is a defensive copy.
 	GetDCRCredentials(ctx context.Context, key DCRKey) (*DCRCredentials, error)
 
 	// StoreDCRCredentials persists the credentials, overwriting any existing
-	// entry for the same Key. Implementations MUST honor RFC 7591
-	// client_secret_expires_at as a TTL when the source DCR response
-	// provided it (e.g. propagated through a higher-level resolution type).
-	// When absent or zero, no TTL is applied.
+	// entry for the same Key. See the interface-level "TTL handling" section
+	// for the contract on ClientSecretExpiresAt.
 	StoreDCRCredentials(ctx context.Context, creds *DCRCredentials) error
 }