Extract DCR resolver into pkg/auth/dcr

pkg/auth/dcr/secret_test.go

@@ -0,0 +1,118 @@
+// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package dcr
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestResolveSecret pins the dcr-package copy of resolveSecret to the same
+// observable contract as the parallel runner-package copy
+// (pkg/authserver/runner/embeddedauthserver_test.go::TestResolveSecret*).
+// Two physically-distinct copies of this helper exist by design (the dcr
+// package must not reach back into pkg/authserver/runner per its
+// profile-agnostic charter); this test guards against silent drift between
+// them. If a future bug fix lands on one copy without being mirrored to the
+// other, this test or its runner-package twin will fail.
+//
+// Cases that take t.Setenv() are kept out of the parallel sub-suite because
+// t.Setenv requires a non-parallel test scope.
+func TestResolveSecret(t *testing.T) {
+	t.Parallel()
+
+	tmpDir := t.TempDir()
+	secretFile := filepath.Join(tmpDir, "secret")
+	require.NoError(t, os.WriteFile(secretFile, []byte("  secret-value  \n"), 0o600))
+
+	cases := []struct {
+		name        string
+		file        string
+		envVar      string
+		want        string
+		wantErr     bool
+		wantErrSubs []string
+	}{
+		{
+			name: "neither file nor env var set returns empty string and no error",
+			file: "", envVar: "",
+			want: "",
+		},
+		{
+			name: "file content is read and surrounding whitespace trimmed",
+			file: secretFile, envVar: "",
+			want: "secret-value",
+		},
+		{
+			name: "missing file returns wrapped read error",
+			file: "/nonexistent/file", envVar: "",
+			wantErr: true, wantErrSubs: []string{"failed to read secret file"},
+		},
+		{
+			name: "env var name is set but env var is empty returns explanatory error",
+			// Use a unique env var name that won't be set in the environment.
+			file: "", envVar: "TEST_SECRET_NOT_SET_DCR_PKG_12345",
+			wantErr: true, wantErrSubs: []string{"environment variable", "is not set"},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			got, err := resolveSecret(tc.file, tc.envVar)
+			if tc.wantErr {
+				require.Error(t, err)
+				for _, sub := range tc.wantErrSubs {
+					assert.Contains(t, err.Error(), sub)
+				}
+				assert.Empty(t, got)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, tc.want, got)
+		})
+	}
+}
+
+// TestResolveSecretWithEnvVar covers the env-var paths separately because
+// t.Setenv requires a non-parallel test scope. Mirrors the runner-package
+// twin (TestResolveSecretWithEnvVar in embeddedauthserver_test.go).
+func TestResolveSecretWithEnvVar(t *testing.T) {
+	tmpDir := t.TempDir()
+	secretFile := filepath.Join(tmpDir, "secret")
+	require.NoError(t, os.WriteFile(secretFile, []byte("secret-from-file"), 0o600))
+
+	t.Run("file takes precedence over env var when both are set", func(t *testing.T) {
+		envVar := "TEST_SECRET_FILE_PRECEDENCE_DCR_PKG"
+		t.Setenv(envVar, "secret-from-env")
+
+		got, err := resolveSecret(secretFile, envVar)
+		require.NoError(t, err)
+		assert.Equal(t, "secret-from-file", got)
+	})
+
+	t.Run("env var is read when file is empty", func(t *testing.T) {
+		envVar := "TEST_SECRET_ENV_ONLY_DCR_PKG"
+		t.Setenv(envVar, "secret-from-env")
+
+		got, err := resolveSecret("", envVar)
+		require.NoError(t, err)
+		assert.Equal(t, "secret-from-env", got)
+	})
+
+	t.Run("missing file does not silently fall back to env var", func(t *testing.T) {
+		envVar := "TEST_SECRET_NO_FALLBACK_DCR_PKG"
+		t.Setenv(envVar, "secret-from-env")
+
+		got, err := resolveSecret("/nonexistent/file", envVar)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "failed to read secret file")
+		assert.Empty(t, got)
+	})
+}

pkg/authserver/runner/dcr_store.go → pkg/auth/dcr/store.go

@@ -1,7 +1,7 @@
 // SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
 // SPDX-License-Identifier: Apache-2.0
 
-package runner
+package dcr
 
 import (
 	"context"
@@ -18,64 +18,72 @@ import (
 // long-lived and are only purged by explicit RFC 7592 deregistration.
 const dcrStaleAgeThreshold = 90 * 24 * time.Hour
 
-// DCRKey is a re-export of storage.DCRKey, kept as a package-local alias so
-// existing runner-side callers continue to compile against runner.DCRKey
-// while the canonical definition lives in pkg/authserver/storage. The
-// canonical form (and its ScopesHash constructor) MUST live in a single place
-// so any future Redis backend hashes keys identically to the in-memory
-// backend; see storage.DCRKey for the field documentation.
-type DCRKey = storage.DCRKey
+// Key is a re-export of storage.DCRKey, kept as a package-local alias so
+// callers in this package can reference the canonical cache key without an
+// explicit storage. qualifier on every call site, while the canonical
+// definition lives in pkg/authserver/storage. The canonical form (and its
+// ScopesHash constructor) MUST live in a single place so any future Redis
+// backend hashes keys identically to the in-memory backend; see
+// storage.DCRKey for the field documentation.
+type Key = storage.DCRKey
 
-// dcrResolutionCache caches RFC 7591 Dynamic Client Registration resolutions
+// CredentialStore caches RFC 7591 Dynamic Client Registration resolutions
 // keyed by the (Issuer, RedirectURI, ScopesHash) tuple. Implementations must
 // be safe for concurrent use.
 //
 // This is the runner-facing interface used by the DCR resolver. It is a
 // narrow re-projection of storage.DCRCredentialStore that exchanges
-// *DCRResolution values (the resolver's working type) instead of
+// *Resolution values (the resolver's working type) instead of
 // *storage.DCRCredentials so the resolver internals stay agnostic to the
 // persistence layer's exact field shape.
 //
 // Implementations in this package are thin adapters around a
 // storage.DCRCredentialStore — the durable map / Redis hash lives over
-// there, and this interface adds a per-call DCRResolution <-> DCRCredentials
+// there, and this interface adds a per-call Resolution <-> DCRCredentials
 // translation. There is exactly one persistence implementation per backend:
-// storage.MemoryStorage and storage.RedisStorage. See newStorageBackedStore
+// storage.MemoryStorage and storage.RedisStorage. See NewStorageBackedStore
 // for the adapter.
-type dcrResolutionCache interface {
+type CredentialStore interface {
 	// Get returns the cached resolution for key, or (nil, false, nil) if the
 	// key is not present. An error is returned only on backend failure.
-	Get(ctx context.Context, key DCRKey) (*DCRResolution, bool, error)
+	Get(ctx context.Context, key Key) (*Resolution, bool, error)
 
 	// Put stores the resolution for key, overwriting any existing entry.
 	// Implementations must reject a nil resolution with an error rather
 	// than silently succeeding — a no-op would leave callers with no
 	// debug trail for the subsequent Get miss.
-	Put(ctx context.Context, key DCRKey, resolution *DCRResolution) error
+	Put(ctx context.Context, key Key, resolution *Resolution) error
 }
 
-// newStorageBackedStore returns a dcrResolutionCache that delegates to a
+// NewStorageBackedStore returns a CredentialStore that delegates to a
 // storage.DCRCredentialStore for durable persistence and translates
-// DCRResolution values into DCRCredentials at the boundary. The returned
+// Resolution values into DCRCredentials at the boundary. The returned
 // store is safe for concurrent use because the underlying
 // storage.DCRCredentialStore must be (per its interface contract).
-func newStorageBackedStore(backend storage.DCRCredentialStore) dcrResolutionCache {
+//
+// Panics if backend is nil — a nil backend is unambiguously a programming
+// error and silent acceptance would only delay the eventual nil-pointer
+// dereference to the first Get/Put call, far from the constructor site.
+func NewStorageBackedStore(backend storage.DCRCredentialStore) CredentialStore {
+	if backend == nil {
+		panic("dcr: NewStorageBackedStore: backend must not be nil")
+	}
 	return &storageBackedStore{backend: backend}
 }
 
-// storageBackedStore is the runner-side dcrResolutionCache wrapping a
+// storageBackedStore is the dcr-package CredentialStore wrapping a
 // storage.DCRCredentialStore. Its methods are the only place that converts
-// between the resolver's *DCRResolution and the persisted
+// between the resolver's *Resolution and the persisted
 // *storage.DCRCredentials shapes.
 type storageBackedStore struct {
 	backend storage.DCRCredentialStore
 }
 
-// Get implements dcrResolutionCache.
+// Get implements CredentialStore.
 //
 // A storage-level ErrNotFound is translated into the (nil, false, nil)
 // miss-tuple advertised by the interface. Other errors propagate as-is.
-func (s *storageBackedStore) Get(ctx context.Context, key DCRKey) (*DCRResolution, bool, error) {
+func (s *storageBackedStore) Get(ctx context.Context, key Key) (*Resolution, bool, error) {
 	creds, err := s.backend.GetDCRCredentials(ctx, key)
 	if err != nil {
 		if errors.Is(err, storage.ErrNotFound) {
@@ -86,27 +94,27 @@ func (s *storageBackedStore) Get(ctx context.Context, key DCRKey) (*DCRResolutio
 	return credentialsToResolution(creds), true, nil
 }
 
-// Put implements dcrResolutionCache.
+// Put implements CredentialStore.
 //
 // A nil resolution is rejected rather than silently no-oped: a caller
 // passing nil would otherwise get a successful return, observe a miss on
 // the next Get, and have no error trail to debug from. Failing loudly at
 // the boundary makes such bugs visible at the first call.
-func (s *storageBackedStore) Put(ctx context.Context, key DCRKey, resolution *DCRResolution) error {
+func (s *storageBackedStore) Put(ctx context.Context, key Key, resolution *Resolution) error {
 	if resolution == nil {
 		return fmt.Errorf("dcr: resolution must not be nil")
 	}
 	creds := resolutionToCredentials(key, resolution)
 	return s.backend.StoreDCRCredentials(ctx, creds)
 }
 
-// resolutionToCredentials converts a resolver-side *DCRResolution into the
-// persisted *storage.DCRCredentials shape. The DCRKey is supplied separately
+// resolutionToCredentials converts a resolver-side *Resolution into the
+// persisted *storage.DCRCredentials shape. The Key is supplied separately
 // because storage.DCRCredentials carries the key as a struct field rather
 // than implicitly via a map key, so the persistence layer can round-trip it
 // across processes and backends.
 //
-// Fields that exist on DCRResolution but not on DCRCredentials are dropped:
+// Fields that exist on Resolution but not on DCRCredentials are dropped:
 //   - ClientIDIssuedAt: informational only per RFC 7591 §3.2.1; the resolver
 //     does not consult it for cache invalidation, so it does not need to
 //     survive a process restart.
@@ -116,7 +124,7 @@ func (s *storageBackedStore) Put(ctx context.Context, key DCRKey, resolution *DC
 // CreatedAt and ClientSecretExpiresAt are preserved so cache observers
 // (e.g. lookupCachedResolution's staleness Warn) and TTL-aware backends
 // (Redis) keep their existing behaviour after a restart.
-func resolutionToCredentials(key DCRKey, res *DCRResolution) *storage.DCRCredentials {
+func resolutionToCredentials(key Key, res *Resolution) *storage.DCRCredentials {
 	if res == nil {
 		return nil
 	}
@@ -136,17 +144,17 @@ func resolutionToCredentials(key DCRKey, res *DCRResolution) *storage.DCRCredent
 
 // credentialsToResolution is the inverse of resolutionToCredentials. The
 // RedirectURI is recovered from the persisted Key so consumers that read it
-// off the resolution (e.g. consumeResolution, which writes it back onto a
+// off the resolution (e.g. ConsumeResolution, which writes it back onto a
 // run-config copy when the caller left it empty) see the canonical value.
 //
 // ClientIDIssuedAt is left zero because it is not persisted. Callers that
 // care about it (none today) must read it directly from the live RFC 7591
 // response, not from a cached resolution.
-func credentialsToResolution(creds *storage.DCRCredentials) *DCRResolution {
+func credentialsToResolution(creds *storage.DCRCredentials) *Resolution {
 	if creds == nil {
 		return nil
 	}
-	return &DCRResolution{
+	return &Resolution{
 		ClientID:                creds.ClientID,
 		ClientSecret:            creds.ClientSecret,
 		AuthorizationEndpoint:   creds.AuthorizationEndpoint,