+
-
+
API key will automatically become invalid on this date
diff --git a/internal/api/handler_apikeys.go b/internal/api/handler_apikeys.go
index 7f7839c3d..cef22d56e 100644
--- a/internal/api/handler_apikeys.go
+++ b/internal/api/handler_apikeys.go
@@ -3,11 +3,13 @@ package api
import (
"context"
"encoding/json"
+ "errors"
"fmt"
"strings"
"time"
"github.com/LeanerCloud/cloud-commitments-go/pkg/logging"
+ "github.com/LeanerCloud/cloud-commitments-platform/internal/auth"
"github.com/aws/aws-lambda-go/events"
)
@@ -62,14 +64,43 @@ func (h *Handler) createAPIKey(ctx context.Context, req *events.LambdaFunctionUR
return nil, NewClientError(400, "invalid request body")
}
+ // Password re-verification, base64-encoded like login/MFA (issue #102).
+ password, err := decodeBase64Password(createReq.Password)
+ if err != nil {
+ return nil, err
+ }
+ createReq.Password = password
+
result, err := h.auth.CreateAPIKeyAPI(ctx, session.UserID, createReq)
if err != nil {
- return nil, fmt.Errorf("failed to create API key: %w", err)
+ return nil, mapCreateAPIKeyError(err)
}
return result, nil
}
+// mapCreateAPIKeyError converts CreateAPIKey service errors to the correct
+// HTTP status code and a user-facing message.
+//
+// - ErrAPIKeyInvalidPassword -> 401: the acting user is re-verifying their
+// own credential, so a precise message is safe (same treatment as
+// ErrCurrentPasswordIncorrect).
+// - ErrAPIKeyExpiresAtRequired / ErrAPIKeyExpiresAtTooFar / ErrAPIKeyExpiresAtInPast
+// -> 400: caller-correctable request validation.
+// - All other errors pass through unchanged for handleRequestError to
+// render as 500.
+func mapCreateAPIKeyError(err error) error {
+ switch {
+ case errors.Is(err, auth.ErrAPIKeyInvalidPassword):
+ return NewClientError(401, err.Error())
+ case errors.Is(err, auth.ErrAPIKeyExpiresAtRequired),
+ errors.Is(err, auth.ErrAPIKeyExpiresAtTooFar),
+ errors.Is(err, auth.ErrAPIKeyExpiresAtInPast):
+ return NewClientError(400, err.Error())
+ }
+ return fmt.Errorf("failed to create API key: %w", err)
+}
+
// deleteAPIKey handles DELETE /api/api-keys/{id}.
func (h *Handler) deleteAPIKey(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) {
session, err := h.requirePermission(ctx, req, "delete", "api-keys")
diff --git a/internal/api/handler_apikeys_test.go b/internal/api/handler_apikeys_test.go
index 9326ff474..22b31b8e0 100644
--- a/internal/api/handler_apikeys_test.go
+++ b/internal/api/handler_apikeys_test.go
@@ -2,10 +2,14 @@ package api
import (
"context"
+ "encoding/base64"
+ "encoding/json"
"errors"
+ "fmt"
"testing"
"time"
+ "github.com/LeanerCloud/cloud-commitments-platform/internal/auth"
"github.com/LeanerCloud/cloud-commitments-platform/internal/config"
"github.com/aws/aws-lambda-go/events"
"github.com/stretchr/testify/assert"
@@ -272,6 +276,157 @@ func TestHandler_createAPIKey_ServiceError(t *testing.T) {
assert.Contains(t, err.Error(), "failed to create API key")
}
+// TestHandler_createAPIKey_WrongPassword is the regression test for issue
+// #102 property (2): CreateAPIKey previously required no password
+// re-verification, unlike the comparable sensitive-action pattern in
+// MFASetup/MFADisable. Mirrors TestHandler_mfaSetup_WrongPassword: the mock
+// simulates the real service's wrapped sentinel and asserts the handler maps
+// it to 401, the same treatment as ErrCurrentPasswordIncorrect.
+//
+// Pre-fix, CreateAPIKey never checked the password at all, so this sentinel
+// and its handler mapping did not exist; this test could not have been
+// written against the pre-fix code because ErrAPIKeyInvalidPassword is a
+// fix artifact. See TestService_CreateAPIKey/fail_when_password_is_wrong in
+// internal/auth for the service-level proof that the real (unmocked)
+// verification logic rejects a wrong password, which is what makes this
+// handler-level sentinel reachable in production.
+func TestHandler_createAPIKey_WrongPassword(t *testing.T) {
+ ctx := context.Background()
+ mockAuth := new(MockAuthService)
+ mockRateLimiter := new(MockRateLimiter)
+
+ session := &Session{UserID: "user-123", Email: "user@example.com"}
+ mockAuth.On("ValidateSession", ctx, "test-token").Return(session, nil)
+ mockAuth.grantAdmin()
+ mockRateLimiter.On("AllowWithUser", ctx, "user-123", "admin").Return(true, nil)
+ mockAuth.On("CreateAPIKeyAPI", ctx, "user-123", mock.Anything).
+ Return(nil, fmt.Errorf("%w", auth.ErrAPIKeyInvalidPassword))
+
+ handler := &Handler{auth: mockAuth, rateLimiter: mockRateLimiter}
+
+ req := &events.LambdaFunctionURLRequest{
+ Headers: map[string]string{
+ "Authorization": "Bearer test-token",
+ },
+ Body: `{"name": "My API Key", "password": "` + base64.StdEncoding.EncodeToString([]byte("wrong")) + `"}`,
+ }
+
+ _, err := handler.createAPIKey(ctx, req)
+ require.Error(t, err)
+ ce, ok := IsClientError(err)
+ require.True(t, ok)
+ assert.Equal(t, 401, ce.code)
+ assert.Contains(t, ce.Error(), "invalid password")
+}
+
+// TestHandler_createAPIKey_PasswordDecodedBeforeForwarding proves the
+// handler decodes the base64-encoded password field (same convention as
+// login/MFA) before handing it to the service, rather than forwarding the
+// encoded value or dropping it.
+func TestHandler_createAPIKey_PasswordDecodedBeforeForwarding(t *testing.T) {
+ ctx := context.Background()
+ mockAuth := new(MockAuthService)
+ mockRateLimiter := new(MockRateLimiter)
+
+ session := &Session{UserID: "user-123", Email: "user@example.com"}
+ mockAuth.On("ValidateSession", ctx, "test-token").Return(session, nil)
+ mockAuth.grantAdmin()
+ mockRateLimiter.On("AllowWithUser", ctx, "user-123", "admin").Return(true, nil)
+ mockAuth.On("CreateAPIKeyAPI", ctx, "user-123", mock.MatchedBy(func(req any) bool {
+ ckReq, ok := req.(CreateAPIKeyRequest)
+ return ok && ckReq.Password == "correct-horse"
+ })).Return(map[string]string{"api_key": "new-key-value", "key_id": "key-123"}, nil)
+
+ handler := &Handler{auth: mockAuth, rateLimiter: mockRateLimiter}
+
+ req := &events.LambdaFunctionURLRequest{
+ Headers: map[string]string{
+ "Authorization": "Bearer test-token",
+ },
+ Body: `{"name": "My API Key", "password": "` + base64.StdEncoding.EncodeToString([]byte("correct-horse")) + `"}`,
+ }
+
+ _, err := handler.createAPIKey(ctx, req)
+ require.NoError(t, err)
+ mockAuth.AssertExpectations(t)
+}
+
+// TestHandler_createAPIKey_ExpiresAtRequired and
+// TestHandler_createAPIKey_ExpiresAtTooFar are the regression tests for
+// issue #102 property (3): ExpiresAt used to be optional and uncapped (nil
+// meant "never expires"). Mirrors the mapMFAServiceError pattern: the mock
+// simulates the real service's wrapped sentinel and asserts the handler maps
+// it to 400. See TestService_CreateAPIKey/fail_when_expiresAt_is_nil and
+// .../fail_when_expiresAt_exceeds_MaxAPIKeyLifetime in internal/auth for the
+// service-level proof against the real (unmocked) validation.
+func TestHandler_createAPIKey_ExpiresAtRequired(t *testing.T) {
+ ctx := context.Background()
+ mockAuth := new(MockAuthService)
+ mockRateLimiter := new(MockRateLimiter)
+
+ session := &Session{UserID: "user-123", Email: "user@example.com"}
+ mockAuth.On("ValidateSession", ctx, "test-token").Return(session, nil)
+ mockAuth.grantAdmin()
+ mockRateLimiter.On("AllowWithUser", ctx, "user-123", "admin").Return(true, nil)
+ mockAuth.On("CreateAPIKeyAPI", ctx, "user-123", mock.Anything).
+ Return(nil, fmt.Errorf("%w", auth.ErrAPIKeyExpiresAtRequired))
+
+ handler := &Handler{auth: mockAuth, rateLimiter: mockRateLimiter}
+
+ req := &events.LambdaFunctionURLRequest{
+ Headers: map[string]string{
+ "Authorization": "Bearer test-token",
+ },
+ // No expires_at field at all -- the pre-fix behavior treated this as
+ // "never expires".
+ Body: `{"name": "My API Key", "password": "` + base64.StdEncoding.EncodeToString([]byte("pw")) + `"}`,
+ }
+
+ _, err := handler.createAPIKey(ctx, req)
+ require.Error(t, err)
+ ce, ok := IsClientError(err)
+ require.True(t, ok)
+ assert.Equal(t, 400, ce.code)
+ assert.Contains(t, ce.Error(), "expires_at is required")
+}
+
+func TestHandler_createAPIKey_ExpiresAtTooFar(t *testing.T) {
+ ctx := context.Background()
+ mockAuth := new(MockAuthService)
+ mockRateLimiter := new(MockRateLimiter)
+
+ session := &Session{UserID: "user-123", Email: "user@example.com"}
+ mockAuth.On("ValidateSession", ctx, "test-token").Return(session, nil)
+ mockAuth.grantAdmin()
+ mockRateLimiter.On("AllowWithUser", ctx, "user-123", "admin").Return(true, nil)
+ mockAuth.On("CreateAPIKeyAPI", ctx, "user-123", mock.Anything).
+ Return(nil, fmt.Errorf("%w", auth.ErrAPIKeyExpiresAtTooFar))
+
+ handler := &Handler{auth: mockAuth, rateLimiter: mockRateLimiter}
+
+ farFuture := time.Now().Add(10 * 365 * 24 * time.Hour)
+ body, err := json.Marshal(map[string]string{
+ "name": "My API Key",
+ "password": base64.StdEncoding.EncodeToString([]byte("pw")),
+ "expires_at": farFuture.Format(time.RFC3339),
+ })
+ require.NoError(t, err)
+
+ req := &events.LambdaFunctionURLRequest{
+ Headers: map[string]string{
+ "Authorization": "Bearer test-token",
+ },
+ Body: string(body),
+ }
+
+ _, err = handler.createAPIKey(ctx, req)
+ require.Error(t, err)
+ ce, ok := IsClientError(err)
+ require.True(t, ok)
+ assert.Equal(t, 400, ce.code)
+ assert.Contains(t, ce.Error(), "exceeds the maximum API key lifetime")
+}
+
func TestHandler_deleteAPIKey_Success(t *testing.T) {
ctx := context.Background()
mockAuth := new(MockAuthService)
diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml
index ca2dcde5f..76581421a 100644
--- a/internal/api/openapi.yaml
+++ b/internal/api/openapi.yaml
@@ -2290,10 +2290,13 @@ components:
# -- API Keys -----------------------------------------------------------
CreateAPIKeyRequest:
type: object
- required: [name]
+ required: [name, password, expires_at]
properties:
name:
type: string
+ password:
+ type: string
+ description: Base64-encoded password, re-verified before minting the key
permissions:
type: array
items:
@@ -2301,6 +2304,7 @@ components:
expires_at:
type: string
format: date-time
+ description: Required and capped at the server-side maximum API key lifetime
CreateAPIKeyResponse:
type: object
diff --git a/internal/api/types_apikeys.go b/internal/api/types_apikeys.go
index f59371430..c2b7ec90c 100644
--- a/internal/api/types_apikeys.go
+++ b/internal/api/types_apikeys.go
@@ -6,6 +6,7 @@ import "time"
type CreateAPIKeyRequest struct {
ExpiresAt *time.Time `json:"expires_at,omitempty"`
Name string `json:"name"`
+ Password string `json:"password"` //nolint:gosec // G117: intentional credential field in request struct -- base64-encoded like the login/MFA password fields, decoded by the handler and never re-stored
Permissions []Permission `json:"permissions,omitempty"`
}
diff --git a/internal/auth/carveout_wildcard_test.go b/internal/auth/carveout_wildcard_test.go
index 4eb4d24ba..d92825ce6 100644
--- a/internal/auth/carveout_wildcard_test.go
+++ b/internal/auth/carveout_wildcard_test.go
@@ -3,6 +3,7 @@ package auth
import (
"context"
"testing"
+ "time"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
@@ -122,18 +123,23 @@ func TestCreateAPIKey_AdminCannotMintWildcardMoneyVerb(t *testing.T) {
ctx := context.Background()
adminGrp := &Group{ID: DefaultAdminGroupID, Permissions: adminOnly}
+ const password = "CorrectHorse@123"
+ passwordHash, err := newTestService().hashPassword(password)
+ require.NoError(t, err)
+ expiresAt := time.Now().Add(24 * time.Hour)
+
for _, perm := range wildcardMoneyVerbs {
t.Run(perm.Action+":"+perm.Resource, func(t *testing.T) {
mockStore := new(MockStore)
t.Cleanup(func() { mockStore.AssertExpectations(t) })
service := &Service{store: mockStore}
- user := &User{ID: "user-123", Active: true, GroupIDs: []string{DefaultAdminGroupID}}
+ user := &User{ID: "user-123", Active: true, PasswordHash: passwordHash, GroupIDs: []string{DefaultAdminGroupID}}
mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil)
mockStore.On("GetGroup", ctx, DefaultAdminGroupID).Return(adminGrp, nil)
- _, _, err := service.CreateAPIKey(ctx, "user-123", "spender",
- []Permission{{Action: perm.Action, Resource: perm.Resource}}, nil)
+ _, _, err := service.CreateAPIKey(ctx, "user-123", "spender", password,
+ []Permission{{Action: perm.Action, Resource: perm.Resource}}, &expiresAt)
require.Error(t, err)
assert.Contains(t, err.Error(), "user does not have permission")
@@ -150,7 +156,7 @@ func TestCreateAPIKey_AdminCannotMintWildcardMoneyVerb(t *testing.T) {
service := &Service{store: mockStore}
grpID := "99999999-9999-4999-8999-999999999999"
- user := &User{ID: "user-123", Active: true, GroupIDs: []string{grpID}}
+ user := &User{ID: "user-123", Active: true, PasswordHash: passwordHash, GroupIDs: []string{grpID}}
mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil)
mockStore.On("GetGroup", ctx, grpID).Return(&Group{
ID: grpID,
@@ -158,8 +164,8 @@ func TestCreateAPIKey_AdminCannotMintWildcardMoneyVerb(t *testing.T) {
}, nil)
mockStore.On("CreateAPIKey", ctx, mock.AnythingOfType("*auth.UserAPIKey")).Return(nil).Once()
- _, _, err := service.CreateAPIKey(ctx, "user-123", "spender",
- []Permission{{Action: ActionExecute, Resource: ResourceAll}}, nil)
+ _, _, err := service.CreateAPIKey(ctx, "user-123", "spender", password,
+ []Permission{{Action: ActionExecute, Resource: ResourceAll}}, &expiresAt)
require.NoError(t, err)
})
}
diff --git a/internal/auth/errors.go b/internal/auth/errors.go
index c9732cd3f..5d3fc72c7 100644
--- a/internal/auth/errors.go
+++ b/internal/auth/errors.go
@@ -100,4 +100,21 @@ var (
ErrMFAEnrollmentExpired = errors.New("MFA enrollment expired")
ErrMFANotEnabled = errors.New("MFA is not enabled")
ErrMFAAuthFailed = errors.New("authentication failed")
+
+ // API key creation sentinels — returned (wrapped via fmt.Errorf "%w") by
+ // CreateAPIKey so the API handler can map each to the right HTTP status
+ // via errors.Is rather than substring matching. See issue #102.
+ //
+ // ErrAPIKeyInvalidPassword — wrong current password on key creation
+ // (same defense-in-depth re-verification as
+ // MFASetup/MFADisable).
+ // ErrAPIKeyExpiresAtRequired — no expires_at supplied; unscoped-lifetime
+ // keys are no longer allowed.
+ // ErrAPIKeyExpiresAtTooFar — expires_at exceeds MaxAPIKeyLifetime.
+ // ErrAPIKeyExpiresAtInPast — expires_at is not in the future, which would
+ // mint a key that is already expired.
+ ErrAPIKeyInvalidPassword = errors.New("invalid password")
+ ErrAPIKeyExpiresAtRequired = errors.New("expires_at is required")
+ ErrAPIKeyExpiresAtTooFar = errors.New("expires_at exceeds the maximum API key lifetime")
+ ErrAPIKeyExpiresAtInPast = errors.New("expires_at must be in the future")
)
diff --git a/internal/auth/service_api_test.go b/internal/auth/service_api_test.go
index b351547cd..2bd0d0146 100644
--- a/internal/auth/service_api_test.go
+++ b/internal/auth/service_api_test.go
@@ -347,6 +347,7 @@ func TestService_ChangePasswordAPI(t *testing.T) {
mockStore.On("GetUserByID", ctx, "user-123").Return(testUser, nil).Once()
mockStore.On("DeleteUserSessions", ctx, "user-123").Return(nil).Once()
+ mockStore.On("ListAPIKeysByUser", ctx, "user-123").Return([]*UserAPIKey{}, nil).Once()
mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Once()
err := service.ChangePasswordAPI(ctx, "user-123", "OldPassword123", "SecureTest@456")
diff --git a/internal/auth/service_apikeys.go b/internal/auth/service_apikeys.go
index ad03a5727..06cc7022c 100644
--- a/internal/auth/service_apikeys.go
+++ b/internal/auth/service_apikeys.go
@@ -35,11 +35,23 @@ const (
// load would keep a single goroutine writing forever; with it, the
// leftover count is simply picked up by the next request's flush.
maxAPIKeyUsageFlushRounds = 16
+
+ // MaxAPIKeyLifetime caps how far in the future ExpiresAt may be set on
+ // creation. A key that never expires (nil ExpiresAt) survived
+ // indefinitely, including past a password rotation intended to lock out
+ // an attacker holding it (issue #102); one year bounds the blast radius
+ // of a leaked key to a re-issuance cycle without forcing short-lived
+ // CI/CD credentials to be re-minted more often than that.
+ MaxAPIKeyLifetime = 365 * 24 * time.Hour
)
-// CreateAPIKey creates a new user API key with scoped permissions
+// CreateAPIKey creates a new user API key with scoped permissions. The
+// caller's current password must be supplied and is re-verified against the
+// stored hash before minting a key: a stolen session token alone must not be
+// enough to create a durable, potentially wildcard-scoped credential, the
+// same defense-in-depth MFASetup/MFADisable already apply (issue #102).
// Returns the full API key (shown only once), key info, and error.
-func (s *Service) CreateAPIKey(ctx context.Context, userID, name string, permissions []Permission, expiresAt *time.Time) (string, *UserAPIKey, error) {
+func (s *Service) CreateAPIKey(ctx context.Context, userID, name, password string, permissions []Permission, expiresAt *time.Time) (string, *UserAPIKey, error) {
// Validate user exists and is active
user, err := s.store.GetUserByID(ctx, userID)
if err != nil {
@@ -55,9 +67,8 @@ func (s *Service) CreateAPIKey(ctx context.Context, userID, name string, permiss
return "", nil, fmt.Errorf("user account is not active")
}
- // Validate name
- if name == "" {
- return "", nil, fmt.Errorf("API key name is required")
+ if err := s.validateCreateAPIKeyRequest(user, password, name, expiresAt); err != nil {
+ return "", nil, err
}
// Generate a secure random key (32 bytes = 256 bits)
@@ -113,6 +124,32 @@ func (s *Service) CreateAPIKey(ctx context.Context, userID, name string, permiss
return apiKey, userAPIKey, nil
}
+// validateCreateAPIKeyRequest checks the caller-supplied fields of a
+// CreateAPIKey call: password re-verification, key name, and the mandatory,
+// capped expiry (issue #102). Extracted out of CreateAPIKey to keep its
+// cyclomatic complexity under the repo's gocyclo gate.
+func (s *Service) validateCreateAPIKeyRequest(user *User, password, name string, expiresAt *time.Time) error {
+ if !s.verifyPassword(password, user.PasswordHash) {
+ return fmt.Errorf("%w", ErrAPIKeyInvalidPassword)
+ }
+ if name == "" {
+ return fmt.Errorf("API key name is required")
+ }
+ // ExpiresAt is mandatory and capped: a nil value used to mean "never
+ // expires", which let a single leaked key outlive any credential rotation.
+ if expiresAt == nil {
+ return fmt.Errorf("%w", ErrAPIKeyExpiresAtRequired)
+ }
+ now := time.Now()
+ if !expiresAt.After(now) {
+ return fmt.Errorf("%w", ErrAPIKeyExpiresAtInPast)
+ }
+ if expiresAt.After(now.Add(MaxAPIKeyLifetime)) {
+ return fmt.Errorf("%w", ErrAPIKeyExpiresAtTooFar)
+ }
+ return nil
+}
+
// validateAPIKeyPermissions ensures the key's permissions don't exceed the
// user's permissions. Administrators-group members hold {admin, *}, so the
// per-permission HasPermission check below already passes for every requested
@@ -246,6 +283,32 @@ func (s *Service) DeleteAPIKey(ctx context.Context, userID, keyID string) error
return nil
}
+// RevokeAllUserAPIKeys deactivates every still-active API key owned by a
+// user. Called by ChangePassword and ConfirmPasswordReset alongside
+// DeleteUserSessions: a password rotation meant to lock out an attacker
+// otherwise left any API key they had minted fully valid, since keys are
+// presented via X-API-Key and never touch the session store (issue #102).
+//
+// Best-effort like the sibling DeleteUserSessions call: a transient store
+// failure here is logged by the caller and does not block the password
+// change itself, since the new password has already taken effect.
+func (s *Service) RevokeAllUserAPIKeys(ctx context.Context, userID string) error {
+ keys, err := s.store.ListAPIKeysByUser(ctx, userID)
+ if err != nil {
+ return fmt.Errorf("failed to list API keys for user %s: %w", userID, err)
+ }
+ for _, key := range keys {
+ if !key.IsActive {
+ continue
+ }
+ key.IsActive = false
+ if err := s.store.UpdateAPIKey(ctx, key); err != nil {
+ return fmt.Errorf("failed to revoke API key %s for user %s: %w", key.ID, userID, err)
+ }
+ }
+ return nil
+}
+
// validateAPIKeyStatus checks that the key is active and not expired.
func validateAPIKeyStatus(key *UserAPIKey) error {
if !key.IsActive {
diff --git a/internal/auth/service_apikeys_api.go b/internal/auth/service_apikeys_api.go
index fa89e227a..8e9336f16 100644
--- a/internal/auth/service_apikeys_api.go
+++ b/internal/auth/service_apikeys_api.go
@@ -79,6 +79,7 @@ func effectiveLifetimeUsage(key *UserAPIKey) *int64 {
type APICreateAPIKeyRequest struct {
ExpiresAt *time.Time `json:"expires_at,omitempty"`
Name string `json:"name"`
+ Password string `json:"password"` //nolint:gosec // G117: intentional credential field in request struct -- decoded by the handler and verified against the caller's own stored hash, never re-stored
Permissions []Permission `json:"permissions,omitempty"`
}
@@ -186,7 +187,7 @@ func (s *Service) CreateAPIKeyAPI(ctx context.Context, userID string, req any) (
}
// Create the API key
- apiKey, keyInfo, err := s.CreateAPIKey(ctx, userID, createReq.Name, createReq.Permissions, createReq.ExpiresAt)
+ apiKey, keyInfo, err := s.CreateAPIKey(ctx, userID, createReq.Name, createReq.Password, createReq.Permissions, createReq.ExpiresAt)
if err != nil {
return nil, err
}
diff --git a/internal/auth/service_apikeys_api_test.go b/internal/auth/service_apikeys_api_test.go
index 925494bc6..c7dd2322a 100644
--- a/internal/auth/service_apikeys_api_test.go
+++ b/internal/auth/service_apikeys_api_test.go
@@ -20,20 +20,23 @@ func TestService_CreateAPIKeyAPI(t *testing.T) {
service := &Service{store: mockStore}
user := &User{
- ID: "user-123",
- Email: "test@example.com",
- Active: true,
- GroupIDs: []string{DefaultAdminGroupID},
+ ID: "user-123",
+ Email: "test@example.com",
+ Active: true,
+ PasswordHash: testAPIKeyPasswordHash,
+ GroupIDs: []string{DefaultAdminGroupID},
}
permissions := []Permission{
{Action: ActionView, Resource: ResourceRecommendations},
}
+ expiresAt := time.Now().Add(24 * time.Hour)
req := APICreateAPIKeyRequest{
Name: "Test API Key",
+ Password: testAPIKeyPassword,
Permissions: permissions,
- ExpiresAt: nil,
+ ExpiresAt: &expiresAt,
}
mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil)
@@ -60,15 +63,17 @@ func TestService_CreateAPIKeyAPI(t *testing.T) {
service := &Service{store: mockStore}
user := &User{
- ID: "user-123",
- Email: "test@example.com",
- Active: true,
- GroupIDs: []string{DefaultAdminGroupID},
+ ID: "user-123",
+ Email: "test@example.com",
+ Active: true,
+ PasswordHash: testAPIKeyPasswordHash,
+ GroupIDs: []string{DefaultAdminGroupID},
}
expiresAt := time.Now().Add(30 * 24 * time.Hour)
req := APICreateAPIKeyRequest{
Name: "Test API Key",
+ Password: testAPIKeyPassword,
Permissions: []Permission{{Action: ActionView, Resource: ResourceRecommendations}},
ExpiresAt: &expiresAt,
}
@@ -128,10 +133,11 @@ func TestService_CreateAPIKeyAPI_CrossPackageType(t *testing.T) {
service := &Service{store: mockStore}
user := &User{
- ID: "user-123",
- Email: "test@example.com",
- Active: true,
- GroupIDs: []string{DefaultAdminGroupID},
+ ID: "user-123",
+ Email: "test@example.com",
+ Active: true,
+ PasswordHash: testAPIKeyPasswordHash,
+ GroupIDs: []string{DefaultAdminGroupID},
}
mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil)
mockStore.On("GetGroup", ctx, DefaultAdminGroupID).Return(&Group{
@@ -143,15 +149,19 @@ func TestService_CreateAPIKeyAPI_CrossPackageType(t *testing.T) {
// Simulate what the api.Handler does: an anonymous struct with the same
// json field names but a different Go type than APICreateAPIKeyRequest.
// This is exactly the value passed by the handler in production (issue #1440).
+ expiresAt := time.Now().Add(24 * time.Hour)
crossPkgReq := struct {
Name string `json:"name"`
+ Password string `json:"password"`
ExpiresAt *time.Time `json:"expires_at,omitempty"`
Permissions []struct {
Action string `json:"action"`
Resource string `json:"resource"`
} `json:"permissions,omitempty"`
}{
- Name: "My API Key",
+ Name: "My API Key",
+ Password: testAPIKeyPassword,
+ ExpiresAt: &expiresAt,
}
result, err := service.CreateAPIKeyAPI(ctx, "user-123", crossPkgReq)
diff --git a/internal/auth/service_apikeys_test.go b/internal/auth/service_apikeys_test.go
index 2366b98c3..51e4efcb2 100644
--- a/internal/auth/service_apikeys_test.go
+++ b/internal/auth/service_apikeys_test.go
@@ -14,18 +14,36 @@ import (
"github.com/stretchr/testify/require"
)
+// testAPIKeyPassword and testAPIKeyPasswordHash back every TestService_CreateAPIKey
+// fixture: CreateAPIKey now re-verifies the caller's password before minting a
+// key (issue #102), so every user fixture in this file needs a matching hash.
+// Hashed once at MinCost and reused across subtests to keep bcrypt off the hot
+// path of each assertion.
+var (
+ testAPIKeyPassword = "CorrectHorse@123"
+ testAPIKeyPasswordHash = func() string {
+ hash, err := newTestService().hashPassword(testAPIKeyPassword)
+ if err != nil {
+ panic(err)
+ }
+ return hash
+ }()
+)
+
func TestService_CreateAPIKey(t *testing.T) {
ctx := context.Background()
+ validExpiry := time.Now().Add(24 * time.Hour)
t.Run("successfully create API key", func(t *testing.T) {
mockStore := new(MockStore)
service := &Service{store: mockStore}
user := &User{
- ID: "user-123",
- Email: "test@example.com",
- Active: true,
- GroupIDs: []string{DefaultAdminGroupID},
+ ID: "user-123",
+ Email: "test@example.com",
+ Active: true,
+ PasswordHash: testAPIKeyPasswordHash,
+ GroupIDs: []string{DefaultAdminGroupID},
}
permissions := []Permission{
@@ -39,7 +57,7 @@ func TestService_CreateAPIKey(t *testing.T) {
}, nil)
mockStore.On("CreateAPIKey", ctx, mock.AnythingOfType("*auth.UserAPIKey")).Return(nil)
- apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", permissions, nil)
+ apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", testAPIKeyPassword, permissions, &validExpiry)
require.NoError(t, err)
require.NotNil(t, keyInfo)
@@ -58,7 +76,7 @@ func TestService_CreateAPIKey(t *testing.T) {
mockStore.On("GetUserByID", ctx, "user-123").Return(nil, assert.AnError)
- apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", []Permission{}, nil)
+ apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", testAPIKeyPassword, []Permission{}, &validExpiry)
assert.Error(t, err)
assert.Empty(t, apiKey)
@@ -78,7 +96,7 @@ func TestService_CreateAPIKey(t *testing.T) {
mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil)
- apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", []Permission{}, nil)
+ apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", testAPIKeyPassword, []Permission{}, &validExpiry)
assert.Error(t, err)
assert.Contains(t, err.Error(), "not active")
@@ -87,19 +105,42 @@ func TestService_CreateAPIKey(t *testing.T) {
mockStore.AssertExpectations(t)
})
+ t.Run("fail when password is wrong", func(t *testing.T) {
+ mockStore := new(MockStore)
+ service := &Service{store: mockStore}
+
+ user := &User{
+ ID: "user-123",
+ Email: "test@example.com",
+ Active: true,
+ PasswordHash: testAPIKeyPasswordHash,
+ }
+
+ mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil)
+
+ apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", "wrong-password", []Permission{}, &validExpiry)
+
+ assert.Error(t, err)
+ assert.ErrorIs(t, err, ErrAPIKeyInvalidPassword)
+ assert.Empty(t, apiKey)
+ assert.Nil(t, keyInfo)
+ mockStore.AssertExpectations(t)
+ })
+
t.Run("fail when name is empty", func(t *testing.T) {
mockStore := new(MockStore)
service := &Service{store: mockStore}
user := &User{
- ID: "user-123",
- Email: "test@example.com",
- Active: true,
+ ID: "user-123",
+ Email: "test@example.com",
+ Active: true,
+ PasswordHash: testAPIKeyPasswordHash,
}
mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil)
- apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "", []Permission{}, nil)
+ apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "", testAPIKeyPassword, []Permission{}, &validExpiry)
assert.Error(t, err)
assert.Contains(t, err.Error(), "name is required")
@@ -108,15 +149,87 @@ func TestService_CreateAPIKey(t *testing.T) {
mockStore.AssertExpectations(t)
})
+ t.Run("fail when expiresAt is nil", func(t *testing.T) {
+ mockStore := new(MockStore)
+ service := &Service{store: mockStore}
+
+ user := &User{
+ ID: "user-123",
+ Email: "test@example.com",
+ Active: true,
+ PasswordHash: testAPIKeyPasswordHash,
+ }
+
+ mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil)
+
+ apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", testAPIKeyPassword, []Permission{}, nil)
+
+ assert.Error(t, err)
+ assert.ErrorIs(t, err, ErrAPIKeyExpiresAtRequired)
+ assert.Empty(t, apiKey)
+ assert.Nil(t, keyInfo)
+ mockStore.AssertExpectations(t)
+ })
+
+ t.Run("fail when expiresAt exceeds MaxAPIKeyLifetime", func(t *testing.T) {
+ mockStore := new(MockStore)
+ service := &Service{store: mockStore}
+
+ user := &User{
+ ID: "user-123",
+ Email: "test@example.com",
+ Active: true,
+ PasswordHash: testAPIKeyPasswordHash,
+ }
+ tooFar := time.Now().Add(MaxAPIKeyLifetime + 24*time.Hour)
+
+ mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil)
+
+ apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", testAPIKeyPassword, []Permission{}, &tooFar)
+
+ assert.Error(t, err)
+ assert.ErrorIs(t, err, ErrAPIKeyExpiresAtTooFar)
+ assert.Empty(t, apiKey)
+ assert.Nil(t, keyInfo)
+ mockStore.AssertExpectations(t)
+ })
+
+ // CodeRabbit finding on this PR: a past expiresAt slipped through the
+ // pre-fix version of this check (which only rejected values too far in
+ // the future), minting a key that was already expired.
+ t.Run("fail when expiresAt is in the past", func(t *testing.T) {
+ mockStore := new(MockStore)
+ service := &Service{store: mockStore}
+
+ user := &User{
+ ID: "user-123",
+ Email: "test@example.com",
+ Active: true,
+ PasswordHash: testAPIKeyPasswordHash,
+ }
+ pastExpiry := time.Now().Add(-1 * time.Hour)
+
+ mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil)
+
+ apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", testAPIKeyPassword, []Permission{}, &pastExpiry)
+
+ assert.Error(t, err)
+ assert.ErrorIs(t, err, ErrAPIKeyExpiresAtInPast)
+ assert.Empty(t, apiKey)
+ assert.Nil(t, keyInfo)
+ mockStore.AssertExpectations(t)
+ })
+
t.Run("successfully create API key with expiration", func(t *testing.T) {
mockStore := new(MockStore)
service := &Service{store: mockStore}
user := &User{
- ID: "user-123",
- Email: "test@example.com",
- Active: true,
- GroupIDs: []string{DefaultAdminGroupID},
+ ID: "user-123",
+ Email: "test@example.com",
+ Active: true,
+ PasswordHash: testAPIKeyPasswordHash,
+ GroupIDs: []string{DefaultAdminGroupID},
}
expiresAt := time.Now().Add(30 * 24 * time.Hour)
@@ -131,7 +244,7 @@ func TestService_CreateAPIKey(t *testing.T) {
}, nil)
mockStore.On("CreateAPIKey", ctx, mock.AnythingOfType("*auth.UserAPIKey")).Return(nil)
- apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", permissions, &expiresAt)
+ apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", testAPIKeyPassword, permissions, &expiresAt)
require.NoError(t, err)
require.NotNil(t, keyInfo)
@@ -146,15 +259,16 @@ func TestService_CreateAPIKey(t *testing.T) {
service := &Service{store: mockStore}
user := &User{
- ID: "user-123",
- Email: "test@example.com",
- Active: true,
+ ID: "user-123",
+ Email: "test@example.com",
+ Active: true,
+ PasswordHash: testAPIKeyPasswordHash,
}
mockStore.On("GetUserByID", ctx, "user-123").Return(user, nil)
mockStore.On("CreateAPIKey", ctx, mock.AnythingOfType("*auth.UserAPIKey")).Return(assert.AnError)
- apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", []Permission{}, nil)
+ apiKey, keyInfo, err := service.CreateAPIKey(ctx, "user-123", "Test Key", testAPIKeyPassword, []Permission{}, &validExpiry)
assert.Error(t, err)
assert.Empty(t, apiKey)
diff --git a/internal/auth/service_password.go b/internal/auth/service_password.go
index 70469b055..83fd0d5c8 100644
--- a/internal/auth/service_password.go
+++ b/internal/auth/service_password.go
@@ -208,6 +208,25 @@ func (s *Service) checkCommonPasswords(password string) error {
return nil
}
+// invalidateUserCredentialsBestEffort revokes every credential surface a
+// password rotation must close: sessions and API keys. A leaked API key
+// authenticates via X-API-Key and never touches the session store, so it
+// would otherwise survive a password change or reset meant to lock an
+// attacker out (issue #102).
+//
+// Both cleanups are best-effort: failures are logged, not returned, because
+// the new password has already taken effect by the time this runs. Shared
+// by ChangePassword, ConfirmPasswordReset and UpdateUserProfile so the two
+// checks live in one place instead of three.
+func (s *Service) invalidateUserCredentialsBestEffort(ctx context.Context, userID, opName string) {
+ if err := s.store.DeleteUserSessions(ctx, userID); err != nil {
+ logging.Warnf("Failed to delete sessions for user %s during %s: %v", userID, opName, err)
+ }
+ if err := s.RevokeAllUserAPIKeys(ctx, userID); err != nil {
+ logging.Warnf("Failed to revoke API keys for user %s during %s: %v", userID, opName, err)
+ }
+}
+
// ChangePassword allows a user to change their password.
func (s *Service) ChangePassword(ctx context.Context, userID string, req ChangePasswordRequest) error {
user, err := s.store.GetUserByID(ctx, userID)
@@ -251,14 +270,17 @@ func (s *Service) ChangePassword(ctx context.Context, userID string, req ChangeP
user.Salt = "" // Not used anymore
user.PasswordHash = passwordHash
- // Invalidate all sessions (non-critical, log error but continue)
- if err := s.store.DeleteUserSessions(ctx, userID); err != nil {
- logging.Warnf("Failed to delete sessions for user %s during password change: %v", userID, err)
- }
-
if err := s.store.UpdateUser(ctx, user); err != nil {
return err
}
+
+ // Invalidate sessions and API keys only after the new password is
+ // persisted (non-critical, best-effort -- issue #102). Invalidating
+ // first would mean a failed UpdateUser leaves the caller with revoked
+ // credentials for a password that never actually changed (CodeRabbit
+ // finding on #392).
+ s.invalidateUserCredentialsBestEffort(ctx, userID, "password change")
+
s.notifyPasswordChange(ctx, userID, req.NewPassword)
return nil
}
@@ -356,13 +378,14 @@ func (s *Service) ConfirmPasswordReset(ctx context.Context, req PasswordResetCon
user.Active = true
}
- if err := s.store.DeleteUserSessions(ctx, user.ID); err != nil {
- logging.Warnf("Failed to delete sessions for user %s during password reset: %v", user.ID, err)
- }
-
if err := s.store.UpdateUser(ctx, user); err != nil {
return err
}
+
+ // See the matching comment in ChangePassword: invalidate only after the
+ // new password is persisted, or a failed UpdateUser leaves the caller
+ // with revoked credentials for a password that never actually changed.
+ s.invalidateUserCredentialsBestEffort(ctx, user.ID, "password reset")
s.notifyPasswordChange(ctx, user.ID, req.NewPassword)
return nil
}
diff --git a/internal/auth/service_password_callback_test.go b/internal/auth/service_password_callback_test.go
index b3d649370..7e76c10bd 100644
--- a/internal/auth/service_password_callback_test.go
+++ b/internal/auth/service_password_callback_test.go
@@ -34,6 +34,7 @@ func TestService_OnPasswordChange_ChangePassword(t *testing.T) {
mockStore.On("GetUserByID", ctx, "user-123").Return(testUser, nil).Once()
mockStore.On("DeleteUserSessions", ctx, "user-123").Return(nil).Once()
+ mockStore.On("ListAPIKeysByUser", ctx, "user-123").Return([]*UserAPIKey{}, nil).Once()
mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Once()
err := service.ChangePassword(ctx, "user-123", ChangePasswordRequest{
@@ -55,6 +56,7 @@ func TestService_OnPasswordChange_ChangePassword(t *testing.T) {
mockStore.On("GetUserByID", ctx, "user-123").Return(testUser, nil).Once()
mockStore.On("DeleteUserSessions", ctx, "user-123").Return(nil).Once()
+ mockStore.On("ListAPIKeysByUser", ctx, "user-123").Return([]*UserAPIKey{}, nil).Once()
mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Once()
err := service.ChangePassword(ctx, "user-123", ChangePasswordRequest{
@@ -82,8 +84,11 @@ func TestService_OnPasswordChange_ChangePassword(t *testing.T) {
testUser := createTestUser(t, "OldSecure123!")
+ // Invalidation now runs only after UpdateUser succeeds (issue #102
+ // CodeRabbit finding on #392: invalidating first would revoke
+ // credentials for a password that never actually changed), so a
+ // failed UpdateUser must never reach DeleteUserSessions/ListAPIKeysByUser.
mockStore.On("GetUserByID", ctx, "user-123").Return(testUser, nil).Once()
- mockStore.On("DeleteUserSessions", ctx, "user-123").Return(nil).Once()
mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(assert.AnError).Once()
err := service.ChangePassword(ctx, "user-123", ChangePasswordRequest{
@@ -127,6 +132,7 @@ func TestService_OnPasswordChange_ConfirmPasswordReset(t *testing.T) {
mockStore.On("GetUserByResetToken", ctx, mock.AnythingOfType("string")).Return(testUser, nil).Once()
mockStore.On("DeleteUserSessions", ctx, "user-456").Return(nil).Once()
+ mockStore.On("ListAPIKeysByUser", ctx, "user-456").Return([]*UserAPIKey{}, nil).Once()
mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Once()
err := service.ConfirmPasswordReset(ctx, PasswordResetConfirm{
@@ -164,6 +170,7 @@ func TestService_OnPasswordChange_UpdateUserProfile(t *testing.T) {
mockStore.On("GetUserByID", ctx, "user-123").Return(testUser, nil).Once()
mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Once()
mockStore.On("DeleteUserSessions", ctx, "user-123").Return(nil).Once()
+ mockStore.On("ListAPIKeysByUser", ctx, "user-123").Return([]*UserAPIKey{}, nil).Once()
err := service.UpdateUserProfile(ctx, "user-123", "", "OldSecure123!", "NewSecure@456")
require.NoError(t, err)
diff --git a/internal/auth/service_password_test.go b/internal/auth/service_password_test.go
index b325ae4ab..c5f84df2c 100644
--- a/internal/auth/service_password_test.go
+++ b/internal/auth/service_password_test.go
@@ -2,6 +2,8 @@ package auth
import (
"context"
+ "crypto/sha256"
+ "encoding/base64"
"testing"
"time"
@@ -23,6 +25,7 @@ func TestService_ChangePassword(t *testing.T) {
mockStore.On("GetUserByID", ctx, "user-123").Return(testUser, nil).Once()
mockStore.On("DeleteUserSessions", ctx, "user-123").Return(nil).Once()
+ mockStore.On("ListAPIKeysByUser", ctx, "user-123").Return([]*UserAPIKey{}, nil).Once()
mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Once()
req := ChangePasswordRequest{
@@ -136,6 +139,7 @@ func TestService_ChangePassword(t *testing.T) {
mockStore.On("GetUserByID", ctx, "user-123").Return(testUser, nil).Once()
mockStore.On("DeleteUserSessions", ctx, "user-123").Return(nil).Once()
+ mockStore.On("ListAPIKeysByUser", ctx, "user-123").Return([]*UserAPIKey{}, nil).Once()
mockStore.On("UpdateUser", ctx, mock.MatchedBy(func(u *User) bool {
// Verify password history includes old password and maintains limit
// Should have: original current password (newly added to history) + 2 existing = 3 total
@@ -182,6 +186,66 @@ func TestService_ChangePassword(t *testing.T) {
mockStore.AssertExpectations(t)
})
+
+ // Regression test for issue #102 property (4): ChangePassword called
+ // DeleteUserSessions and nothing else, so an API key minted before the
+ // change stayed fully valid afterward -- a password rotation meant to
+ // lock out an attacker did not lock out a key they held. This proves the
+ // real (unmocked) revocation end to end: after ChangePassword, the same
+ // key that ValidateUserAPIKey previously accepted -- the function the
+ // X-API-Key middleware calls on every protected request via
+ // HasAPIKeyPermissionAPI (internal/api/handler.go) -- is rejected.
+ //
+ // apiKeyRecord is shared by pointer with the mocked store: RevokeAllUserAPIKeys
+ // flips IsActive on the same object ListAPIKeysByUser returned, so the
+ // second ValidateUserAPIKey call (mocked to return that same pointer)
+ // observes the mutation exactly as a real database round-trip would.
+ t.Run("revokes API keys so a previously valid key is rejected afterward", func(t *testing.T) {
+ mockStore := new(MockStore)
+ mockEmail := new(MockEmailSender)
+ service := createTestService(mockStore, mockEmail)
+
+ testUser := createTestUser(t, "OldSecure123!")
+
+ plaintextAPIKey := "user-api-key-abc123"
+ hash := sha256.Sum256([]byte(plaintextAPIKey))
+ keyHash := base64.RawURLEncoding.EncodeToString(hash[:])
+ apiKeyRecord := &UserAPIKey{
+ ID: "key-1",
+ UserID: "user-123",
+ Name: "CI key",
+ KeyHash: keyHash,
+ IsActive: true,
+ }
+
+ mockStore.On("GetAPIKeyByHash", ctx, keyHash).Return(apiKeyRecord, nil)
+ mockStore.On("GetUserByID", ctx, "user-123").Return(testUser, nil)
+
+ // Precondition: the key is valid before the password change.
+ _, _, err := service.ValidateUserAPIKey(ctx, plaintextAPIKey)
+ require.NoError(t, err, "key must be valid before ChangePassword")
+
+ mockStore.On("DeleteUserSessions", ctx, "user-123").Return(nil).Once()
+ mockStore.On("ListAPIKeysByUser", ctx, "user-123").Return([]*UserAPIKey{apiKeyRecord}, nil).Once()
+ mockStore.On("UpdateAPIKey", ctx, apiKeyRecord).Return(nil).Once()
+ mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Once()
+
+ err = service.ChangePassword(ctx, "user-123", ChangePasswordRequest{
+ CurrentPassword: "OldSecure123!",
+ NewPassword: "NewSecure@456",
+ })
+ require.NoError(t, err)
+ assert.False(t, apiKeyRecord.IsActive, "ChangePassword must revoke the user's API keys")
+
+ // Postcondition: the same key used with X-API-Key must now be
+ // rejected -- this is exactly what HasAPIKeyPermissionAPI enforces
+ // on every protected request.
+ _, _, err = service.ValidateUserAPIKey(ctx, plaintextAPIKey)
+ require.Error(t, err, "a key that survived ChangePassword defeats the purpose of the password rotation")
+ assert.Contains(t, err.Error(), "revoked")
+
+ mockStore.AssertExpectations(t)
+ })
}
func TestService_RequestPasswordReset(t *testing.T) {
@@ -317,6 +381,7 @@ func TestService_ConfirmPasswordReset(t *testing.T) {
// Token is hashed before lookup, use mock.Anything to match the hash
mockStore.On("GetUserByResetToken", ctx, mock.AnythingOfType("string")).Return(testUser, nil).Once()
mockStore.On("DeleteUserSessions", ctx, "user-123").Return(nil).Once()
+ mockStore.On("ListAPIKeysByUser", ctx, "user-123").Return([]*UserAPIKey{}, nil).Once()
// UpdateUser is called once: password change + token invalidation in single call
mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Once()
@@ -331,6 +396,59 @@ func TestService_ConfirmPasswordReset(t *testing.T) {
mockStore.AssertExpectations(t)
})
+ // Regression test for issue #102 property (4), reset-flow half: same
+ // gap as ChangePassword above, but for the "forgot password" flow.
+ t.Run("revokes API keys so a previously valid key is rejected afterward", func(t *testing.T) {
+ mockStore := new(MockStore)
+ mockEmail := new(MockEmailSender)
+ service := createTestService(mockStore, mockEmail)
+
+ expiry := time.Now().Add(time.Hour)
+ testUser := &User{
+ ID: "user-123",
+ Email: "test@example.com",
+ PasswordResetToken: hashSessionToken("valid-reset-token"),
+ PasswordResetExpiry: &expiry,
+ Active: true,
+ }
+
+ plaintextAPIKey := "user-api-key-def456"
+ hash := sha256.Sum256([]byte(plaintextAPIKey))
+ keyHash := base64.RawURLEncoding.EncodeToString(hash[:])
+ apiKeyRecord := &UserAPIKey{
+ ID: "key-2",
+ UserID: "user-123",
+ Name: "CI key",
+ KeyHash: keyHash,
+ IsActive: true,
+ }
+
+ mockStore.On("GetAPIKeyByHash", ctx, keyHash).Return(apiKeyRecord, nil)
+ mockStore.On("GetUserByID", ctx, "user-123").Return(testUser, nil)
+
+ _, _, err := service.ValidateUserAPIKey(ctx, plaintextAPIKey)
+ require.NoError(t, err, "key must be valid before ConfirmPasswordReset")
+
+ mockStore.On("GetUserByResetToken", ctx, mock.AnythingOfType("string")).Return(testUser, nil).Once()
+ mockStore.On("DeleteUserSessions", ctx, "user-123").Return(nil).Once()
+ mockStore.On("ListAPIKeysByUser", ctx, "user-123").Return([]*UserAPIKey{apiKeyRecord}, nil).Once()
+ mockStore.On("UpdateAPIKey", ctx, apiKeyRecord).Return(nil).Once()
+ mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Once()
+
+ err = service.ConfirmPasswordReset(ctx, PasswordResetConfirm{
+ Token: "valid-reset-token",
+ NewPassword: "SecureT3st@789",
+ })
+ require.NoError(t, err)
+ assert.False(t, apiKeyRecord.IsActive, "ConfirmPasswordReset must revoke the user's API keys")
+
+ _, _, err = service.ValidateUserAPIKey(ctx, plaintextAPIKey)
+ require.Error(t, err, "a key that survived ConfirmPasswordReset defeats the purpose of the reset")
+ assert.Contains(t, err.Error(), "revoked")
+
+ mockStore.AssertExpectations(t)
+ })
+
t.Run("invalid token", func(t *testing.T) {
mockStore := new(MockStore)
mockEmail := new(MockEmailSender)
diff --git a/internal/auth/service_test.go b/internal/auth/service_test.go
index 88e330954..2ccbff31f 100644
--- a/internal/auth/service_test.go
+++ b/internal/auth/service_test.go
@@ -636,6 +636,7 @@ func TestService_ErrorPaths(t *testing.T) {
mockStore.On("GetUserByID", ctx, "user-123").Return(testUser, nil).Once()
mockStore.On("DeleteUserSessions", ctx, "user-123").Return(fmt.Errorf("session error")).Once()
+ mockStore.On("ListAPIKeysByUser", ctx, "user-123").Return([]*UserAPIKey{}, nil).Once()
mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Once()
req := ChangePasswordRequest{
@@ -666,6 +667,7 @@ func TestService_ErrorPaths(t *testing.T) {
mockStore.On("GetUserByResetToken", ctx, mock.AnythingOfType("string")).Return(testUser, nil).Once()
mockStore.On("DeleteUserSessions", ctx, "user-123").Return(fmt.Errorf("session error")).Once()
+ mockStore.On("ListAPIKeysByUser", ctx, "user-123").Return([]*UserAPIKey{}, nil).Once()
// UpdateUser is called once: password change + token invalidation in single call
mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Once()
diff --git a/internal/auth/service_user.go b/internal/auth/service_user.go
index 2628f612b..232515873 100644
--- a/internal/auth/service_user.go
+++ b/internal/auth/service_user.go
@@ -687,11 +687,13 @@ func (s *Service) UpdateUserProfile(ctx context.Context, userID, email, currentP
return fmt.Errorf("failed to update user: %w", err)
}
- // Invalidate sessions when password changes
+ // Invalidate sessions and API keys when password changes. This is a
+ // third self-service password-rotation path with the same gap as
+ // ChangePassword/ConfirmPasswordReset (issue #102): an API key
+ // authenticates via X-API-Key rather than the session store, so it
+ // would otherwise survive a profile-driven password change too.
if passwordChanged {
- if err := s.store.DeleteUserSessions(ctx, userID); err != nil {
- logging.Warnf("Failed to delete sessions for user %s during profile update: %v", userID, err)
- }
+ s.invalidateUserCredentialsBestEffort(ctx, userID, "profile update")
s.notifyPasswordChange(ctx, userID, newPassword)
}
diff --git a/internal/auth/service_user_test.go b/internal/auth/service_user_test.go
index bfde8c913..ef01bba1b 100644
--- a/internal/auth/service_user_test.go
+++ b/internal/auth/service_user_test.go
@@ -1092,6 +1092,7 @@ func TestService_UpdateUserProfile(t *testing.T) {
mockStore.On("GetUserByEmail", ctx, "new@example.com").Return(nil, nil).Once()
mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Once()
mockStore.On("DeleteUserSessions", ctx, "user-123").Return(nil).Once()
+ mockStore.On("ListAPIKeysByUser", ctx, "user-123").Return([]*UserAPIKey{}, nil).Once()
err := service.UpdateUserProfile(ctx, "user-123", "new@example.com", "OldPassword123", "SecureTest@456")
require.NoError(t, err)
diff --git a/internal/auth/test_helpers.go b/internal/auth/test_helpers.go
index 8f519ad1e..f670eaf17 100644
--- a/internal/auth/test_helpers.go
+++ b/internal/auth/test_helpers.go
@@ -274,6 +274,19 @@ func TestCSRFKey() []byte {
return key
}
+// TestPasswordHash returns a bcrypt hash of password at bcrypt.MinCost, for
+// test fixtures in this and other packages that need a User.PasswordHash
+// verifyPassword will accept (issue #102: CreateAPIKey and the password-
+// rotation paths now re-verify the caller's password). MinCost keeps the
+// hash cheap to compute since tests only need a hash that round-trips
+// through bcrypt.CompareHashAndPassword, not the production work factor.
+func TestPasswordHash(t testing.TB, password string) string {
+ t.Helper()
+ hash, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.MinCost)
+ require.NoError(t, err)
+ return string(hash)
+}
+
// DeriveTestCSRFToken returns the CSRF token a service configured with
// TestCSRFKey expects for the given raw session token. It reuses the
// production derivation (HMAC-SHA256(key, rawSessionToken)) so tests assert
diff --git a/internal/server/adapter_test.go b/internal/server/adapter_test.go
index 0d7b15758..91307c218 100644
--- a/internal/server/adapter_test.go
+++ b/internal/server/adapter_test.go
@@ -377,13 +377,17 @@ func TestAuthServiceAdapter_CreateAPIKeyAPI(t *testing.T) {
ctx := context.Background()
t.Cleanup(func() { mockStore.AssertExpectations(t) })
- // CreateAPIKey calls GetUserByID then validateAPIKeyPermissions (GetGroup)
- // then CreateAPIKey. All three stores must be mocked for the happy path.
+ // CreateAPIKey calls GetUserByID then validateCreateAPIKeyRequest
+ // (password re-verification, issue #102) then validateAPIKeyPermissions
+ // (GetGroup) then CreateAPIKey. All three stores must be mocked for the
+ // happy path.
+ const password = "CorrectHorse@123"
mockStore.On("GetUserByID", ctx, "user-1").Return(&auth.User{
- ID: "user-1",
- Email: "test@example.com",
- Active: true,
- GroupIDs: []string{auth.DefaultAdminGroupID},
+ ID: "user-1",
+ Email: "test@example.com",
+ Active: true,
+ PasswordHash: auth.TestPasswordHash(t, password),
+ GroupIDs: []string{auth.DefaultAdminGroupID},
}, nil)
mockStore.On("GetGroup", ctx, auth.DefaultAdminGroupID).Return(&auth.Group{
ID: auth.DefaultAdminGroupID,
@@ -391,8 +395,11 @@ func TestAuthServiceAdapter_CreateAPIKeyAPI(t *testing.T) {
}, nil)
mockStore.On("CreateAPIKey", ctx, mock.AnythingOfType("*auth.UserAPIKey")).Return(nil)
+ expiresAt := time.Now().Add(24 * time.Hour)
result, err := adapter.CreateAPIKeyAPI(ctx, "user-1", auth.APICreateAPIKeyRequest{
- Name: "my-key",
+ Name: "my-key",
+ Password: password,
+ ExpiresAt: &expiresAt,
})
require.NoError(t, err)
require.NotNil(t, result)