test(auth): cover remaining auth failure paths; fix invite re-send and sticky KMS signer errors

internal/auth/service_password.go

@@ -307,11 +307,24 @@ func (s *Service) ChangePassword(ctx context.Context, userID string, req ChangeP
 // token stays valid: this prevents a griefing attack where an adversary who
 // knows the victim's email repeatedly requests resets to perpetually
 // invalidate the victim's legitimate link.
+//
+// The stored token is either a reset token (PasswordResetExpiry) or an invite
+// token (PasswordSetupExpiry), and only its expiry is stored. A reset token
+// never has more than PasswordResetExpiry left, so a longer remaining window
+// identifies an invite; deriving an invite's age from the reset lifetime made
+// it negative for the whole week, silently dropping every "re-send my setup
+// link" request (audit finding A03-009). A remaining window beyond even the
+// invite lifetime still yields a negative age, so it stays rate-limited.
 func resetRecentlyIssued(user *User, email string) bool {
 	if user.PasswordResetExpiry == nil {
 		return false
 	}
-	tokenAge := PasswordResetExpiry - time.Until(*user.PasswordResetExpiry)
+	remaining := time.Until(*user.PasswordResetExpiry)
+	lifetime := PasswordResetExpiry
+	if remaining > PasswordResetExpiry {
+		lifetime = PasswordSetupExpiry
+	}
+	tokenAge := lifetime - remaining
 	if tokenAge < PasswordResetRateLimit {
 		logging.Debugf("Password reset rate-limited for %s (token age %s < %s)",
 			redactEmail(email), tokenAge.Round(time.Second), PasswordResetRateLimit)

internal/auth/service_password_invite_test.go

@@ -0,0 +1,109 @@
+package auth
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+)
+
+// Audit finding A03-009 (issue #263): an invite token is issued with
+// PasswordSetupExpiry (7d), but the forgot-password rate limit derived the
+// token's age as if every token had the 1h PasswordResetExpiry. For an invite
+// that age came out negative for almost the whole week, so the documented
+// "re-mail the setup link via Forgot Password" recovery returned success
+// without issuing a token or sending mail.
+//
+// The refusal case stubs NO write or email expectation: testify panics if
+// either lands, so the assertion cannot be vacuous.
+
+// invitedUser is an account created through the invite flow and not yet
+// activated: inactive, never deactivated, holding the invite token.
+func invitedUser(issuedAgo time.Duration) *User {
+	expiry := time.Now().Add(PasswordSetupExpiry - issuedAgo)
+	return &User{
+		ID:                  "user-invited",
+		Email:               "invited@example.com",
+		PasswordResetToken:  "invite-token-hash",
+		PasswordResetExpiry: &expiry,
+		Active:              false,
+	}
+}
+
+func TestService_RequestPasswordReset_InvitedUser(t *testing.T) {
+	ctx := context.Background()
+
+	t.Run("invite older than the rate limit gets a fresh link", func(t *testing.T) {
+		mockStore := new(MockStore)
+		mockEmail := new(MockEmailSender)
+		t.Cleanup(func() { mockStore.AssertExpectations(t) })
+		t.Cleanup(func() { mockEmail.AssertExpectations(t) })
+		svc := createTestService(mockStore, mockEmail)
+
+		user := invitedUser(time.Hour)
+		inviteExpiry := *user.PasswordResetExpiry
+
+		mockStore.On("GetUserByEmail", ctx, user.Email).Return(user, nil).Once()
+		mockStore.On("SetPasswordResetToken", ctx, mock.MatchedBy(func(u *User) bool {
+			// A normal 1h reset token replaces the invite token.
+			return u.ID == user.ID && u.PasswordResetToken != "invite-token-hash" &&
+				u.PasswordResetExpiry != nil &&
+				time.Until(*u.PasswordResetExpiry) <= PasswordResetExpiry
+		}), mock.MatchedBy(func(readExpiry *time.Time) bool {
+			// The write is conditioned on the invite row it read.
+			return readExpiry != nil && readExpiry.Equal(inviteExpiry)
+		})).Return(nil).Once()
+		mockEmail.On("SendPasswordResetEmail", ctx, user.Email, mock.AnythingOfType("string")).Return(nil).Once()
+
+		require.NoError(t, svc.RequestPasswordReset(ctx, user.Email))
+	})
+
+	t.Run("invite inside the rate limit window is not reissued", func(t *testing.T) {
+		mockStore := new(MockStore)
+		mockEmail := new(MockEmailSender)
+		t.Cleanup(func() { mockStore.AssertExpectations(t) })
+		t.Cleanup(func() { mockEmail.AssertExpectations(t) })
+		svc := createTestService(mockStore, mockEmail)
+
+		user := invitedUser(PasswordResetRateLimit / 2)
+		mockStore.On("GetUserByEmail", ctx, user.Email).Return(user, nil).Once()
+		// SetPasswordResetToken and SendPasswordResetEmail must NOT be called.
+
+		require.NoError(t, svc.RequestPasswordReset(ctx, user.Email))
+	})
+}
+
+// TestResetRecentlyIssued pins the age derivation for both token kinds at the
+// rate-limit boundary, so neither a reset token nor an invite token can drift
+// to "always limited" or "never limited".
+func TestResetRecentlyIssued(t *testing.T) {
+	cases := []struct {
+		name      string
+		lifetime  time.Duration
+		issuedAgo time.Duration
+		want      bool
+	}{
+		{"reset token just issued", PasswordResetExpiry, 5 * time.Second, true},
+		{"reset token past the limit", PasswordResetExpiry, PasswordResetRateLimit + 5*time.Second, false},
+		{"invite token just issued", PasswordSetupExpiry, 5 * time.Second, true},
+		{"invite token past the limit", PasswordSetupExpiry, PasswordResetRateLimit + 5*time.Second, false},
+		{"invite token a day old", PasswordSetupExpiry, 24 * time.Hour, false},
+		// Just over PasswordResetExpiry left is still an invite (aged ~7d), not a
+		// reset token issued in the future, so it is not limited.
+		{"invite token about to expire", PasswordSetupExpiry, PasswordSetupExpiry - PasswordResetExpiry - 30*time.Second, false},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			expiry := time.Now().Add(tc.lifetime - tc.issuedAgo)
+			user := &User{PasswordResetExpiry: &expiry}
+			assert.Equal(t, tc.want, resetRecentlyIssued(user, "u@example.com"))
+		})
+	}
+
+	t.Run("no token is never limited", func(t *testing.T) {
+		assert.False(t, resetRecentlyIssued(&User{}, "u@example.com"))
+	})
+}

internal/auth/service_reset_invalidation_db_test.go

@@ -3,7 +3,11 @@
 package auth
 
 import (
+	"context"
+	"errors"
+	"net/url"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -75,3 +79,78 @@ func TestIntegration_CompletePasswordResetEnforcesExpiryInSQL(t *testing.T) {
 		require.ErrorIs(t, err, ErrUserChanged)
 	})
 }
+
+// inviteMailSink fails every invite send, the case CreateUserResult answers
+// by pointing the admin at Forgot Password, and records each reset link.
+type inviteMailSink struct {
+	resetURLs []string
+}
+
+func (m *inviteMailSink) SendPasswordResetEmail(_ context.Context, _, resetURL string) error {
+	m.resetURLs = append(m.resetURLs, resetURL)
+	return nil
+}
+func (m *inviteMailSink) SendWelcomeEmail(context.Context, string, string, string) error {
+	return nil
+}
+func (m *inviteMailSink) SendUserInviteEmail(context.Context, string, string) error {
+	return errors.New("smtp unavailable")
+}
+
+// TestIntegration_InvitedUserForgotPasswordReissuesLink is audit finding
+// A03-009 on real PostgreSQL: an invite whose email failed is recovered via
+// Forgot Password, even while the 7-day invite token is still live, and two
+// concurrent requests still store and mail exactly one token.
+func TestIntegration_InvitedUserForgotPasswordReissuesLink(t *testing.T) {
+	db := setupAuthTestDB(t)
+	store := NewPostgresStore(db)
+	ctx := t.Context()
+	newSvc := func(st StoreInterface, mail EmailSenderInterface) *Service {
+		svc := NewService(ServiceConfig{Store: st, EmailSender: mail, DashboardURL: "https://dashboard.example.com"})
+		svc.bcryptCostOverride = 4
+		return svc
+	}
+
+	barrier := &credentialReadBarrier{StoreInterface: store}
+	mail := &inviteMailSink{}
+	svc := newSvc(barrier, mail)
+	created, err := svc.CreateUser(ctx, "", CreateUserRequest{Email: "invite-forgot@example.com", GroupIDs: []string{DefaultPurchaserGroupID}})
+	require.NoError(t, err)
+	require.NotNil(t, created.InviteEmailSent)
+	require.False(t, *created.InviteEmailSent, "fixture: the invite email must have failed")
+	invited := created.User
+
+	// The invite went out an hour ago: well past the rate limit, with almost
+	// all of its 7-day window left.
+	_, err = db.Exec(ctx, "UPDATE users SET password_reset_expiry = password_reset_expiry - interval '1 hour' WHERE id = $1", invited.ID)
+	require.NoError(t, err)
+	inviteHash := invited.PasswordResetToken
+
+	// A second request lands between this one's read and its write.
+	otherMail := &inviteMailSink{}
+	barrier.afterRead = func(ctx context.Context, _ *User) {
+		require.NoError(t, newSvc(store, otherMail).RequestPasswordReset(ctx, invited.Email))
+	}
+	require.NoError(t, svc.RequestPasswordReset(ctx, invited.Email))
+
+	require.Len(t, otherMail.resetURLs, 1, "the first committed request must issue and mail a link")
+	assert.Empty(t, mail.resetURLs, "the losing request must not mail a token it did not store")
+
+	stored, err := store.GetUserByID(ctx, invited.ID)
+	require.NoError(t, err)
+	assert.False(t, stored.Active)
+	assert.NotEqual(t, inviteHash, stored.PasswordResetToken, "the invite token must be replaced")
+	require.NotNil(t, stored.PasswordResetExpiry)
+	assert.LessOrEqual(t, time.Until(*stored.PasswordResetExpiry), PasswordResetExpiry)
+
+	link, err := url.Parse(otherMail.resetURLs[0])
+	require.NoError(t, err)
+	token := link.Query().Get("token")
+	require.Equal(t, hashSessionToken(token), stored.PasswordResetToken, "the mailed link must be the stored token")
+
+	require.NoError(t, svc.ConfirmPasswordReset(ctx, PasswordResetConfirm{Token: token, NewPassword: credentialRaceNew}))
+	activated, err := store.GetUserByID(ctx, invited.ID)
+	require.NoError(t, err)
+	assert.True(t, activated.Active, "completing the re-sent link must activate the invited account")
+	assert.True(t, svc.verifyPassword(credentialRaceNew, activated.PasswordHash))
+}

internal/oidc/aws_signer.go

@@ -7,7 +7,6 @@ import (
 	"crypto/elliptic"
 	"crypto/x509"
 	"fmt"
-	"sync"
 
 	awsconfig "github.com/aws/aws-sdk-go-v2/config"
 	"github.com/aws/aws-sdk-go-v2/service/kms"
@@ -25,11 +24,8 @@ type AWSKMSClient interface {
 // The private key never leaves KMS.
 type AWSKMSSigner struct {
 	client AWSKMSClient
-	err    error
-	pubKey *ecdsa.PublicKey
 	keyID  string
-	kid    string
-	once   sync.Once
+	keys   publicKeyCache
 }
 
 // NewAWSKMSSigner constructs a signer bound to the given KMS key. The
@@ -70,46 +66,32 @@ func (s *AWSKMSSigner) Sign(ctx context.Context, digest []byte) ([]byte, error)
 	return derToRawECDSASignature(out.Signature)
 }
 
-// PublicKey fetches the public half of the KMS key once and caches it.
-// Subsequent calls return the cached value.
+// PublicKey fetches the public half of the KMS key and caches it once a
+// fetch succeeds. A failed fetch is returned and retried on the next call.
 func (s *AWSKMSSigner) PublicKey(ctx context.Context) (crypto.PublicKey, error) {
-	s.resolveOnce(ctx)
-	return s.pubKey, s.err
+	return s.keys.publicKey(ctx, s.fetchPublicKey)
 }
 
 // KeyID returns the JWK kid derived from the cached public key.
 func (s *AWSKMSSigner) KeyID(ctx context.Context) (string, error) {
-	s.resolveOnce(ctx)
-	return s.kid, s.err
+	return s.keys.keyID(ctx, s.fetchPublicKey)
 }
 
-func (s *AWSKMSSigner) resolveOnce(ctx context.Context) {
-	s.once.Do(func() {
-		out, err := s.client.GetPublicKey(ctx, &kms.GetPublicKeyInput{KeyId: &s.keyID})
-		if err != nil {
-			s.err = fmt.Errorf("oidc: kms:GetPublicKey: %w", err)
-			return
-		}
-		pub, err := x509.ParsePKIXPublicKey(out.PublicKey)
-		if err != nil {
-			s.err = fmt.Errorf("oidc: parse kms public key: %w", err)
-			return
-		}
-		ecPub, ok := pub.(*ecdsa.PublicKey)
-		if !ok {
-			s.err = fmt.Errorf("oidc: kms key is not ECDSA (got %T); key must be ECC_NIST_P256", pub)
-			return
-		}
-		if ecPub.Curve != elliptic.P256() {
-			s.err = fmt.Errorf("oidc: kms key curve is %s; key must be ECC_NIST_P256 (ES256)", ecPub.Curve.Params().Name)
-			return
-		}
-		kid, err := ComputeKeyID(ecPub)
-		if err != nil {
-			s.err = err
-			return
-		}
-		s.pubKey = ecPub
-		s.kid = kid
-	})
+func (s *AWSKMSSigner) fetchPublicKey(ctx context.Context) (*ecdsa.PublicKey, error) {
+	out, err := s.client.GetPublicKey(ctx, &kms.GetPublicKeyInput{KeyId: &s.keyID})
+	if err != nil {
+		return nil, fmt.Errorf("oidc: kms:GetPublicKey: %w", err)
+	}
+	pub, err := x509.ParsePKIXPublicKey(out.PublicKey)
+	if err != nil {
+		return nil, fmt.Errorf("oidc: parse kms public key: %w", err)
+	}
+	ecPub, ok := pub.(*ecdsa.PublicKey)
+	if !ok {
+		return nil, fmt.Errorf("oidc: kms key is not ECDSA (got %T); key must be ECC_NIST_P256", pub)
+	}
+	if ecPub.Curve != elliptic.P256() {
+		return nil, fmt.Errorf("oidc: kms key curve is %s; key must be ECC_NIST_P256 (ES256)", ecPub.Curve.Params().Name)
+	}
+	return ecPub, nil
 }

internal/oidc/aws_signer_test.go

@@ -78,7 +78,7 @@ func TestAWSKMSSignerRoundTrip(t *testing.T) {
 
 // TestAWSKMSSignerRejectsWrongCurve guards against a misconfigured KMS
 // key (e.g. ECC_NIST_P384 instead of the required ECC_NIST_P256) being
-// accepted silently. resolveOnce must fail fast with a clear error
+// accepted silently. fetchPublicKey must fail fast with a clear error
 // instead of caching a wrong-curve public key that only breaks later
 // at JWKS-publish or signing time.
 func TestAWSKMSSignerRejectsWrongCurve(t *testing.T) {

internal/oidc/azure_factory_test.go

@@ -13,7 +13,7 @@ import (
 )
 
 // fakeAzureKeyVaultClient is a minimal AzureKeyVaultClient backed by an
-// in-process EC key. Used to exercise resolveOnce without a real Key Vault.
+// in-process EC key. Used to exercise fetchPublicKey without a real Key Vault.
 type fakeAzureKeyVaultClient struct {
 	signErr error
 	key     *ecdsa.PublicKey
@@ -107,7 +107,7 @@ func (f *fakeAzureKeyVaultClient) jwkCrv() *azkeys.CurveName {
 // M6 -- Azure EC key completeness guard (replaces RSA exponent-range check)
 // ---------------------------------------------------------------------------
 
-// TestAzureSigner_ECKeyCompleteness verifies that resolveOnce rejects Key
+// TestAzureSigner_ECKeyCompleteness verifies that fetchPublicKey rejects Key
 // Vault responses that are missing the EC public-key coordinates, which
 // would otherwise produce a zero-valued *ecdsa.PublicKey and sign invalid
 // tokens. A valid response with both X and Y must be accepted.