From c6594dfba5ef9d12b903a0c9ff42b40811b4fbd7 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 22:11:20 +0100 Subject: [PATCH] fix(api/auth): report a user API key's effective permissions on /me/permissions GET /api/auth/me/permissions resolved a user-API-key caller to its owning user and returned the owner's full group permissions, so a view-only key owned by an administrator was told it held {admin, *} with is_admin=true. Enforcement already used the key's effective set (key permissions intersected with the owner's), so the endpoint advertised a surface the key could not use. For a user-API-key principal the handler now returns the key's effective permissions through a new auth.Service.GetAPIKeyEffectivePermissionsAPI (the same ComputeEffectivePermissions path HasAPIKeyPermissionAPI enforces), and derives is_admin from that set. The key is re-checked (owner match, revocation, expiry, owner active, password rotation) and any lookup failure refuses the request instead of falling back to the owner's permissions. Bearer sessions and the admin API key are unchanged. Closes #62 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_auth.go | 76 ++++- ...handler_auth_me_permissions_apikey_test.go | 284 ++++++++++++++++++ internal/api/handler_auth_test.go | 32 +- internal/api/handler_ri_exchange_test.go | 3 + internal/api/mocks_test.go | 6 + internal/api/router_authuser_test.go | 4 +- internal/api/types.go | 14 +- internal/auth/service_apikeys_api.go | 42 +++ .../auth/service_apikeys_effective_test.go | 142 +++++++++ internal/server/app.go | 4 + 10 files changed, 592 insertions(+), 15 deletions(-) create mode 100644 internal/api/handler_auth_me_permissions_apikey_test.go create mode 100644 internal/auth/service_apikeys_effective_test.go diff --git a/internal/api/handler_auth.go b/internal/api/handler_auth.go index 2deed0467..045786c11 100644 --- a/internal/api/handler_auth.go +++ b/internal/api/handler_auth.go @@ -105,13 +105,20 @@ func (h *Handler) getCurrentUser(ctx context.Context, req *events.LambdaFunction } // getCurrentUserPermissions handles GET /api/auth/me/permissions. -// It returns the effective permission set for the authenticated user, -// derived from the union of all their group permissions. The same path -// the backend uses for enforcement (GetUserPermissionsAPI -> GetUserPermissions). +// It returns the effective permission set of the authenticated caller, the +// same set the backend enforces for that caller: +// - bearer-token session: the union of the user's group permissions +// (GetUserPermissionsAPI -> GetUserPermissions); +// - user API key: the key's permissions intersected with its owner's +// (GetAPIKeyEffectivePermissionsAPI), never the owner's full set, so a +// view-only key owned by an admin is not advertised as admin (issue #62); +// - admin API key: {admin, *}. +// +// IsAdmin is derived from that same set. // // Auth: the route is AuthUser, which admits admin API key, user API // key, OR a bearer-token session (see router.go AuthLevel doc). The -// handler resolves the authenticated user from any of those three +// handler resolves the authenticated caller from any of those three // credentials rather than re-requiring a bearer session — otherwise a // caller that authenticated upstream with an API key would still get a // 401 here. @@ -120,6 +127,14 @@ func (h *Handler) getCurrentUserPermissions(ctx context.Context, req *events.Lam return nil, fmt.Errorf("authentication service not configured") } + keyID, keyOwnerID, isUserAPIKey, err := h.userAPIKeyCaller(ctx, req) + if err != nil { + return nil, err + } + if isUserAPIKey { + return h.userAPIKeyPermissions(ctx, keyID, keyOwnerID) + } + userID, err := h.resolveAuthenticatedUserID(ctx, req) if err != nil { return nil, err @@ -151,6 +166,56 @@ func (h *Handler) getCurrentUserPermissions(ctx context.Context, req *events.Lam logging.Errorf("getCurrentUserPermissions: GetUserPermissionsAPI returned %T, want []auth.APIPermission", raw) return nil, fmt.Errorf("GetUserPermissionsAPI returned unexpected payload type %T", raw) } + return permissionsResponse(apiPerms), nil +} + +// userAPIKeyCaller reports whether the request authenticated with a user API +// key and, if so, returns the key's database ID and its owner's user ID. +// With a principal in context (every routed request) it reads the principal: +// a user-API-key principal wins over a bearer session sent alongside it, +// exactly as requirePermission evaluates it. Without one it validates the +// x-api-key header itself; a key that does not validate is not a key caller +// (the bearer session is tried next, as resolveAuthenticatedUserID does), but +// a key that validates without a usable key/owner record is refused rather +// than described with its owner's permissions. +func (h *Handler) userAPIKeyCaller(ctx context.Context, req *events.LambdaFunctionURLRequest) (keyID, userID string, isUserAPIKey bool, err error) { + if principal, ok := principalFromContext(ctx); ok { + if principal.Kind != PrincipalUserAPIKey { + return "", "", false, nil + } + return principal.APIKeyID, principal.UserID, true, nil + } + apiKey := extractAPIKey(req) + if apiKey == "" || h.checkAdminAPIKey(apiKey) { + return "", "", false, nil + } + keyRaw, userRaw, validateErr := h.auth.ValidateUserAPIKeyAPI(ctx, apiKey) + if validateErr != nil { + return "", "", false, nil + } + key, keyOK := keyRaw.(*auth.UserAPIKey) + user, userOK := userRaw.(*auth.User) + if !keyOK || !userOK || key == nil || user == nil { + return "", "", false, NewClientError(401, "API key permission validation failed") + } + return key.ID, user.ID, true, nil +} + +// userAPIKeyPermissions builds the /me/permissions response for a user API +// key from the key's effective permissions. Fail closed: a lookup error +// refuses the request instead of falling back to the owner's permissions. +func (h *Handler) userAPIKeyPermissions(ctx context.Context, keyID, userID string) (*UserPermissionsResponse, error) { + apiPerms, err := h.auth.GetAPIKeyEffectivePermissionsAPI(ctx, keyID, userID) + if err != nil { + logging.Errorf("getCurrentUserPermissions: resolving API key %s effective permissions: %v", keyID, err) + return nil, fmt.Errorf("failed to resolve API key permissions: %w", err) + } + return permissionsResponse(apiPerms), nil +} + +// permissionsResponse converts an effective permission set to the wire +// shape, setting IsAdmin when the set holds the {admin, *} wildcard. +func permissionsResponse(apiPerms []auth.APIPermission) *UserPermissionsResponse { entries := make([]PermissionEntry, len(apiPerms)) isAdmin := false for i, p := range apiPerms { @@ -159,11 +224,10 @@ func (h *Handler) getCurrentUserPermissions(ctx context.Context, req *events.Lam isAdmin = true } } - return &UserPermissionsResponse{ Permissions: entries, IsAdmin: isAdmin, - }, nil + } } // resolveAuthenticatedUserID resolves the calling user's ID from any of diff --git a/internal/api/handler_auth_me_permissions_apikey_test.go b/internal/api/handler_auth_me_permissions_apikey_test.go new file mode 100644 index 000000000..76794efa1 --- /dev/null +++ b/internal/api/handler_auth_me_permissions_apikey_test.go @@ -0,0 +1,284 @@ +package api //nolint:revive // var-naming: package name "api" is intentional for handler package + +import ( + "context" + "crypto/sha256" + "encoding/base64" + "encoding/json" + "errors" + "net/http" + "testing" + "time" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" +) + +// Issue #62: GET /api/auth/me/permissions must describe a user API key by its +// EFFECTIVE permissions (key ∩ owner), not by its owner's full set. These +// tests drive HandleRequest (middleware, router, handler) into a real +// auth.Service backed by auth.MockStore, so the reported set comes from the +// same ComputeEffectivePermissions path that enforcement uses. + +const ( + mePermsOwnerID = "aaaa0062-0000-4000-8000-000000000062" + mePermsKeyID = "key-62" + mePermsRawKey = "raw-user-key-62" + mePermsToken = "owner-session-62" + mePermsGroupID = "group-62" + mePermsAdmin = "admin-api-key-62" +) + +var adminAll = auth.Permission{Action: auth.ActionAdmin, Resource: auth.ResourceAll} + +// mePermsAuth sends API-key validation, permission listing and permission +// decisions to a real auth.Service; only bearer-session validation is mocked. +type mePermsAuth struct { + *MockAuthService + service *auth.Service + store *auth.MockStore +} + +func (a *mePermsAuth) ValidateUserAPIKeyAPI(ctx context.Context, apiKey string) (any, any, error) { + key, user, err := a.service.ValidateUserAPIKeyAPI(ctx, apiKey) + if err != nil { + return nil, nil, err + } + return key, user, nil +} + +func (a *mePermsAuth) GetAPIKeyEffectivePermissionsAPI(ctx context.Context, keyID, userID string) ([]auth.APIPermission, error) { + return a.service.GetAPIKeyEffectivePermissionsAPI(ctx, keyID, userID) +} + +func (a *mePermsAuth) GetUserPermissionsAPI(ctx context.Context, userID string) (any, error) { + return a.service.GetUserPermissionsAPI(ctx, userID) +} + +func (a *mePermsAuth) HasAPIKeyPermissionAPI(ctx context.Context, apiKey, action, resource string) (userID, keyID string, allowed bool, err error) { + return a.service.HasAPIKeyPermissionAPI(ctx, apiKey, action, resource) +} + +// keyLookup is what the store's GetAPIKeyByID answers. +type keyLookup struct { + key *auth.UserAPIKey + err error +} + +// newMePermsAuth builds an owner whose group holds ownerPerms and a key +// scoped to keyPerms. keyByID, when non-nil, overrides what GetAPIKeyByID +// returns, so a test can make the key go bad after the middleware accepted it. +func newMePermsAuth(ownerPerms, keyPerms []auth.Permission, keyByID func(*auth.UserAPIKey) keyLookup) *mePermsAuth { + sum := sha256.Sum256([]byte(mePermsRawKey)) + expires := time.Now().Add(24 * time.Hour) + key := &auth.UserAPIKey{ + ID: mePermsKeyID, UserID: mePermsOwnerID, KeyHash: base64.RawURLEncoding.EncodeToString(sum[:]), + IsActive: true, ExpiresAt: &expires, Permissions: keyPerms, + } + store := new(auth.MockStore) + store.On("GetAPIKeyByHash", mock.Anything, key.KeyHash).Return(key, nil).Maybe() + if keyByID == nil { + store.On("GetAPIKeyByID", mock.Anything, mePermsKeyID).Return(key, nil).Maybe() + } else { + l := keyByID(key) + store.On("GetAPIKeyByID", mock.Anything, mePermsKeyID).Return(l.key, l.err).Maybe() + } + store.On("GetUserByID", mock.Anything, mePermsOwnerID).Return(&auth.User{ + ID: mePermsOwnerID, Email: "owner@example.com", Active: true, GroupIDs: []string{mePermsGroupID}, + }, nil).Maybe() + store.On("GetGroup", mock.Anything, mePermsGroupID).Return(&auth.Group{ + ID: mePermsGroupID, Permissions: ownerPerms, + }, nil).Maybe() + + m := new(MockAuthService) + m.On("ValidateSession", mock.Anything, mePermsToken).Return(&Session{UserID: mePermsOwnerID, Email: "owner@example.com"}, nil).Maybe() + m.On("ValidateCSRFToken", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + m.On("GetAllowedAccountsAPI", mock.Anything, mePermsOwnerID).Return([]string{}, nil).Maybe() + return &mePermsAuth{ + MockAuthService: m, + service: auth.NewService(auth.ServiceConfig{Store: store, CSRFKey: auth.TestCSRFKey()}), + store: store, + } +} + +func getMePermissions(t *testing.T, a AuthServiceInterface, headers map[string]string) *events.LambdaFunctionURLResponse { + t.Helper() + h := &Handler{auth: a, apiKey: mePermsAdmin} + resp, err := h.HandleRequest(context.Background(), &events.LambdaFunctionURLRequest{ + Headers: headers, + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{Method: http.MethodGet, Path: "/api/auth/me/permissions"}, + }, + }) + require.NoError(t, err) + require.NotNil(t, resp) + return resp +} + +func decodeMePermissions(t *testing.T, resp *events.LambdaFunctionURLResponse) UserPermissionsResponse { + t.Helper() + require.Equal(t, http.StatusOK, resp.StatusCode, "body: %s", resp.Body) + var out UserPermissionsResponse + require.NoError(t, json.Unmarshal([]byte(resp.Body), &out), "body: %s", resp.Body) + return out +} + +// storeCalled reports whether the auth store recorded a call to method. +func storeCalled(store *auth.MockStore, method string) bool { + for _, c := range store.Calls { + if c.Method == method { + return true + } + } + return false +} + +func meKeyHeaders() map[string]string { return map[string]string{"X-API-Key": mePermsRawKey} } + +func meOwnerHeaders() map[string]string { + return map[string]string{"Authorization": "Bearer " + mePermsToken} +} + +// assertReportedMatchesEnforced checks that every reported entry is one the +// key is actually allowed on the request path, and that admin is held only +// when reported, so the advertised and enforced sets cannot drift (#61/#62). +func assertReportedMatchesEnforced(t *testing.T, a *mePermsAuth, got UserPermissionsResponse) { + t.Helper() + for _, p := range got.Permissions { + _, _, allowed, err := a.HasAPIKeyPermissionAPI(context.Background(), mePermsRawKey, p.Action, p.Resource) + require.NoError(t, err) + assert.True(t, allowed, "reported %s:%s must be enforced as allowed", p.Action, p.Resource) + } + _, _, adminAllowed, err := a.HasAPIKeyPermissionAPI(context.Background(), mePermsRawKey, auth.ActionAdmin, auth.ResourceAll) + require.NoError(t, err) + assert.Equal(t, adminAllowed, got.IsAdmin, "is_admin must match what enforcement grants the key") +} + +// TestGetCurrentUserPermissions_UserAPIKey_ReturnsEffectiveScope is the #62 +// regression: an admin's view-only key is reported as view-only, not admin. +func TestGetCurrentUserPermissions_UserAPIKey_ReturnsEffectiveScope(t *testing.T) { + view := auth.Permission{Action: auth.ActionView, Resource: auth.ResourceRecommendations} + a := newMePermsAuth([]auth.Permission{adminAll}, []auth.Permission{view}, nil) + + got := decodeMePermissions(t, getMePermissions(t, a, meKeyHeaders())) + + assert.Equal(t, []PermissionEntry{{Action: auth.ActionView, Resource: auth.ResourceRecommendations}}, got.Permissions) + assert.False(t, got.IsAdmin, "a view-only key must not be reported as admin") + assertReportedMatchesEnforced(t, a, got) +} + +// A key sent together with its owner's bearer session is still evaluated as +// the key by requirePermission, so it must be reported as the key too. +func TestGetCurrentUserPermissions_UserAPIKeyWithOwnerSession_ReportsKey(t *testing.T) { + view := auth.Permission{Action: auth.ActionView, Resource: auth.ResourceRecommendations} + a := newMePermsAuth([]auth.Permission{adminAll}, []auth.Permission{view}, nil) + headers := meKeyHeaders() + for k, v := range meOwnerHeaders() { + headers[k] = v + } + + got := decodeMePermissions(t, getMePermissions(t, a, headers)) + + assert.Equal(t, []PermissionEntry{{Action: auth.ActionView, Resource: auth.ResourceRecommendations}}, got.Permissions) + assert.False(t, got.IsAdmin) +} + +// The owner's own session is unchanged: it still reports the owner's groups. +func TestGetCurrentUserPermissions_OwnerSession_Unchanged(t *testing.T) { + view := auth.Permission{Action: auth.ActionView, Resource: auth.ResourceRecommendations} + a := newMePermsAuth([]auth.Permission{adminAll}, []auth.Permission{view}, nil) + + got := decodeMePermissions(t, getMePermissions(t, a, meOwnerHeaders())) + + assert.Equal(t, []PermissionEntry{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}}, got.Permissions) + assert.True(t, got.IsAdmin) + assert.False(t, storeCalled(a.store, "GetAPIKeyByID"), "a session must not be described through a key") +} + +// The stateless admin API key is unchanged: {admin, *} with no store lookup. +func TestGetCurrentUserPermissions_AdminAPIKey_Unchanged(t *testing.T) { + a := newMePermsAuth([]auth.Permission{adminAll}, nil, nil) + + got := decodeMePermissions(t, getMePermissions(t, a, map[string]string{"X-API-Key": mePermsAdmin})) + + assert.Equal(t, []PermissionEntry{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}}, got.Permissions) + assert.True(t, got.IsAdmin) + assert.False(t, storeCalled(a.store, "GetAPIKeyByID"), "the admin API key must not look up a key row") + assert.False(t, storeCalled(a.store, "GetUserByID"), "the admin API key must not look up a user row") +} + +// A deliberately admin-scoped key on an admin owner keeps admin, so is_admin +// follows the effective set rather than being forced off for every key. +func TestGetCurrentUserPermissions_AdminScopedKey_ReportsAdmin(t *testing.T) { + a := newMePermsAuth([]auth.Permission{adminAll}, []auth.Permission{adminAll}, nil) + + got := decodeMePermissions(t, getMePermissions(t, a, meKeyHeaders())) + + assert.Equal(t, []PermissionEntry{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}}, got.Permissions) + assert.True(t, got.IsAdmin) + assertReportedMatchesEnforced(t, a, got) +} + +// A constrained key is reported by its own entries: key permissions the +// owner lacks are dropped, the owner's extra permissions are not added, and +// the surviving entry is the key's (its constraints are the key's; the wire +// shape carries action/resource only, see the auth-level test for the +// constraint values). +func TestGetCurrentUserPermissions_ConstrainedKey_ReportsKeyEntries(t *testing.T) { + ownerPerms := []auth.Permission{ + {Action: auth.ActionExecute, Resource: auth.ResourcePurchases, Constraints: &auth.PermissionConstraints{MaxPurchaseAmount: 1000}}, + {Action: auth.ActionView, Resource: auth.ResourceRecommendations}, + {Action: auth.ActionView, Resource: auth.ResourcePurchases}, + } + keyPerms := []auth.Permission{ + {Action: auth.ActionExecute, Resource: auth.ResourcePurchases, + Constraints: &auth.PermissionConstraints{MaxPurchaseAmount: 100, Providers: []string{"aws"}}}, + {Action: auth.ActionDelete, Resource: auth.ResourceUsers}, // owner lacks it + } + a := newMePermsAuth(ownerPerms, keyPerms, nil) + + got := decodeMePermissions(t, getMePermissions(t, a, meKeyHeaders())) + + assert.Equal(t, []PermissionEntry{{Action: auth.ActionExecute, Resource: auth.ResourcePurchases}}, got.Permissions) + assert.False(t, got.IsAdmin) + assertReportedMatchesEnforced(t, a, got) + + eff, err := a.GetAPIKeyEffectivePermissionsAPI(context.Background(), mePermsKeyID, mePermsOwnerID) + require.NoError(t, err) + require.Len(t, eff, 1) + require.NotNil(t, eff[0].Constraints) + assert.InDelta(t, 100, eff[0].Constraints.MaxAmount, 0, "the key's cap, not the owner's") + assert.Equal(t, []string{"aws"}, eff[0].Constraints.Providers) +} + +// When the key's effective permissions cannot be established after the +// middleware accepted it, the request is refused; it never falls back to +// the owner's permissions. +func TestGetCurrentUserPermissions_UserAPIKeyLookupFailureRefuses(t *testing.T) { + view := auth.Permission{Action: auth.ActionView, Resource: auth.ResourceRecommendations} + cases := map[string]func(*auth.UserAPIKey) keyLookup{ + "store error": func(*auth.UserAPIKey) keyLookup { + return keyLookup{err: errors.New("db down")} + }, + "revoked mid-request": func(k *auth.UserAPIKey) keyLookup { + revoked := *k + revoked.IsActive = false + return keyLookup{key: &revoked} + }, + } + for name, keyByID := range cases { + t.Run(name, func(t *testing.T) { + a := newMePermsAuth([]auth.Permission{adminAll}, []auth.Permission{view}, keyByID) + + resp := getMePermissions(t, a, meKeyHeaders()) + + assert.Equal(t, http.StatusInternalServerError, resp.StatusCode, "body: %s", resp.Body) + assert.NotContains(t, resp.Body, `"is_admin"`) + assert.NotContains(t, resp.Body, auth.ActionAdmin) + }) + } +} diff --git a/internal/api/handler_auth_test.go b/internal/api/handler_auth_test.go index 9135bd34e..454c21fbd 100644 --- a/internal/api/handler_auth_test.go +++ b/internal/api/handler_auth_test.go @@ -1530,21 +1530,22 @@ func TestHandler_getCurrentUserPermissions_AdminAPIKey(t *testing.T) { } // TestHandler_getCurrentUserPermissions_UserAPIKey guards CR #922 F2: -// a user API key resolves to the owning user, and the handler must look -// up that user's effective permissions instead of returning 401 because -// the request has no bearer token. +// a user API key resolves without a bearer token instead of returning 401. +// Since issue #62 the handler describes the KEY's effective permissions, +// never the owning user's (GetUserPermissionsAPI must not be consulted). func TestHandler_getCurrentUserPermissions_UserAPIKey(t *testing.T) { ctx := context.Background() mockAuth := new(MockAuthService) t.Cleanup(func() { mockAuth.AssertExpectations(t) }) user := &auth.User{ID: "user-42"} - mockAuth.On("ValidateUserAPIKeyAPI", ctx, "user-key").Return((any)(nil), any(user), nil) + key := &auth.UserAPIKey{ID: "key-42", UserID: "user-42"} + mockAuth.On("ValidateUserAPIKeyAPI", ctx, "user-key").Return(any(key), any(user), nil) perms := []auth.APIPermission{ {Action: "view", Resource: "recommendations"}, } - mockAuth.On("GetUserPermissionsAPI", ctx, "user-42").Return(perms, nil) + mockAuth.On("GetAPIKeyEffectivePermissionsAPI", ctx, "key-42", "user-42").Return(perms, nil) handler := &Handler{auth: mockAuth} req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"X-API-Key": "user-key"}} @@ -1553,6 +1554,27 @@ func TestHandler_getCurrentUserPermissions_UserAPIKey(t *testing.T) { assert.False(t, result.IsAdmin) require.Len(t, result.Permissions, 1) assert.Equal(t, PermissionEntry{Action: "view", Resource: "recommendations"}, result.Permissions[0]) + mockAuth.AssertNotCalled(t, "GetUserPermissionsAPI", mock.Anything, mock.Anything) +} + +// TestHandler_getCurrentUserPermissions_UserAPIKeyWithoutKeyRecordRefuses: +// a key that validates without a usable key record cannot be described, so +// the handler refuses instead of falling back to the owner's permissions. +func TestHandler_getCurrentUserPermissions_UserAPIKeyWithoutKeyRecordRefuses(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + mockAuth.On("ValidateUserAPIKeyAPI", ctx, "user-key").Return((any)(nil), any(&auth.User{ID: "user-42"}), nil) + + handler := &Handler{auth: mockAuth} + req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"X-API-Key": "user-key"}} + _, err := handler.getCurrentUserPermissions(ctx, req) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 401, ce.code) + mockAuth.AssertNotCalled(t, "GetUserPermissionsAPI", mock.Anything, mock.Anything) } // TestHandler_login_ErrorEquivalence verifies that two failed login attempts -- diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index ed155a530..20dc98258 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -1650,6 +1650,9 @@ func (m *mockAuthForExchange) ValidateUserAPIKeyAPI(_ context.Context, _ string) return nil, nil, nil } func (m *mockAuthForExchange) RecordAPIKeyUsageAsync(_ string) {} +func (m *mockAuthForExchange) GetAPIKeyEffectivePermissionsAPI(_ context.Context, _, _ string) ([]auth.APIPermission, error) { + return nil, nil +} func (m *mockAuthForExchange) HasAPIKeyPermissionAPI(_ context.Context, _, _, _ string) (string, string, bool, error) { return "admin", "", true, nil } diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index 7012a152d..27c592b0c 100644 --- a/internal/api/mocks_test.go +++ b/internal/api/mocks_test.go @@ -346,6 +346,12 @@ func (m *MockAuthService) GetUserPermissionsAPI(ctx context.Context, userID stri return args.Get(0), args.Error(1) } +func (m *MockAuthService) GetAPIKeyEffectivePermissionsAPI(ctx context.Context, keyID, userID string) ([]auth.APIPermission, error) { + args := m.Called(ctx, keyID, userID) + perms, _ := args.Get(0).([]auth.APIPermission) + return perms, args.Error(1) +} + // allowConstraintChecks stubs the SEC-01 execution-time permission // constraint check (HasPermissionForConstraintsAPI and the user-API-key // variant HasAPIKeyPermissionForConstraintsAPI) to succeed for any request, diff --git a/internal/api/router_authuser_test.go b/internal/api/router_authuser_test.go index 6fddd2092..1fedbb2e4 100644 --- a/internal/api/router_authuser_test.go +++ b/internal/api/router_authuser_test.go @@ -107,7 +107,7 @@ func TestRouterAuthUser_UserAPIKeyPrincipalAvoidsSecondValidation(t *testing.T) mockAuth := new(MockAuthService) user := &auth.User{ID: "api-key-user", Email: "user@example.com"} mockAuth.On("ValidateUserAPIKeyAPI", ctx, "user-key").Return(&auth.UserAPIKey{ID: "key-1"}, user, nil).Once() - mockAuth.On("GetUserPermissionsAPI", mock.Anything, "api-key-user").Return([]auth.APIPermission{}, nil).Once() + mockAuth.On("GetAPIKeyEffectivePermissionsAPI", mock.Anything, "key-1", "api-key-user").Return([]auth.APIPermission{}, nil).Once() h := &Handler{auth: mockAuth} r := NewRouter(h) req := &events.LambdaFunctionURLRequest{Headers: map[string]string{"X-API-Key": "user-key"}} @@ -163,7 +163,7 @@ func TestHandleRequest_UserAPIKeyPrincipalValidatedOnce(t *testing.T) { mockAuth := new(MockAuthService) user := &auth.User{ID: "api-key-user", Email: "user@example.com"} mockAuth.On("ValidateUserAPIKeyAPI", ctx, "user-key").Return(&auth.UserAPIKey{ID: "key-1"}, user, nil).Once() - mockAuth.On("GetUserPermissionsAPI", mock.Anything, "api-key-user").Return([]auth.APIPermission{}, nil).Once() + mockAuth.On("GetAPIKeyEffectivePermissionsAPI", mock.Anything, "key-1", "api-key-user").Return([]auth.APIPermission{}, nil).Once() h := &Handler{auth: mockAuth} req := &events.LambdaFunctionURLRequest{ Headers: map[string]string{"X-API-Key": "user-key"}, diff --git a/internal/api/types.go b/internal/api/types.go index 9570dca8d..9be54f1f4 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -222,6 +222,14 @@ type AuthServiceInterface interface { // (union of all group permissions). Used by GET /api/auth/me/permissions. // Returns []auth.APIPermission converted to []PermissionEntry by the handler. GetUserPermissionsAPI(ctx context.Context, userID string) (any, error) + // GetAPIKeyEffectivePermissionsAPI returns a user API key's effective + // permission set (the key's permissions intersected with its owner's, + // carrying the key's constraints): the set HasAPIKeyPermissionAPI + // enforces. Used by GET /api/auth/me/permissions for user-API-key + // principals so the key, not its owner, is described (issue #62). A + // non-nil error means the key or its owner could not be confirmed, and + // callers must refuse rather than fall back to the owner's permissions. + GetAPIKeyEffectivePermissionsAPI(ctx context.Context, keyID, userID string) ([]auth.APIPermission, error) // Account access - returns the union of allowed_accounts from all user groups (empty = all access) GetAllowedAccountsAPI(ctx context.Context, userID string) ([]string, error) // API Key management @@ -528,8 +536,10 @@ type PermissionEntry struct { } // UserPermissionsResponse is the response shape for GET /api/auth/me/permissions. -// Permissions is the effective set derived from the union of the user's groups. -// IsAdmin mirrors whether the effective set contains the {admin, *} wildcard. +// Permissions is the caller's effective set: for a user session, the union of +// the user's groups; for a user API key, the key's permissions intersected +// with its owner's (issue #62); for the admin API key, {admin, *}. +// IsAdmin mirrors whether that effective set contains the {admin, *} wildcard. type UserPermissionsResponse struct { Permissions []PermissionEntry `json:"permissions"` IsAdmin bool `json:"is_admin"` diff --git a/internal/auth/service_apikeys_api.go b/internal/auth/service_apikeys_api.go index b86d74252..ec762de2c 100644 --- a/internal/auth/service_apikeys_api.go +++ b/internal/auth/service_apikeys_api.go @@ -448,3 +448,45 @@ func (s *Service) HasAPIKeyPermissionForConstraintsAPI(ctx context.Context, keyI } return true, nil } + +// GetAPIKeyEffectivePermissionsAPI returns the effective permission set of a +// user API key: the key's own permissions intersected with its owner's +// group-derived permissions (ComputeEffectivePermissions), the same set +// HasAPIKeyPermissionAPI enforces. Each entry carries the key's constraints, +// not the owner's. GET /api/auth/me/permissions uses it so a scoped key is +// told what it can do rather than what its owner can (issue #62). +// +// keyID and userID come from the request's already-authenticated principal. +// The key is re-checked here (owner match, revocation, expiry, owner active, +// password rotation) so a key that stopped being valid mid-request is +// refused. Fail closed: any lookup failure returns an error, never the +// owner's permissions. +func (s *Service) GetAPIKeyEffectivePermissionsAPI(ctx context.Context, keyID, userID string) ([]APIPermission, error) { + key, err := s.store.GetAPIKeyByID(ctx, keyID) + if err != nil { + return nil, fmt.Errorf("failed to load API key: %w", err) + } + if key == nil || key.UserID != userID { + return nil, fmt.Errorf("API key not found") + } + err = validateAPIKeyStatus(key) + if err != nil { + return nil, err + } + user, err := s.lookupAPIKeyUser(ctx, userID) + if err != nil { + return nil, err + } + if key.PasswordVersion != user.PasswordVersion { + return nil, ErrAPIKeyPasswordRotated + } + perms, err := s.ComputeEffectivePermissions(ctx, key, user) + if err != nil { + return nil, fmt.Errorf("computing effective API key permissions: %w", err) + } + result := make([]APIPermission, len(perms)) + for i, p := range perms { + result[i] = permissionToAPIPermission(p) + } + return result, nil +} diff --git a/internal/auth/service_apikeys_effective_test.go b/internal/auth/service_apikeys_effective_test.go new file mode 100644 index 000000000..18222573e --- /dev/null +++ b/internal/auth/service_apikeys_effective_test.go @@ -0,0 +1,142 @@ +package auth + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Issue #62: GET /api/auth/me/permissions describes a user API key with +// GetAPIKeyEffectivePermissionsAPI. It must return the set +// HasAPIKeyPermissionAPI enforces (key ∩ owner, key's constraints) and fail +// closed on every lookup problem. + +const ( + effKeyID = "key-62" + effOwnerID = "owner-62" + effGroupID = "group-62" +) + +func effStore(t *testing.T, key *UserAPIKey, owner *User, ownerPerms []Permission) *MockStore { + t.Helper() + store := new(MockStore) + store.On("GetAPIKeyByID", mock.Anything, effKeyID).Return(key, nil).Maybe() + store.On("GetUserByID", mock.Anything, effOwnerID).Return(owner, nil).Maybe() + store.On("GetGroup", mock.Anything, effGroupID).Return(&Group{ID: effGroupID, Permissions: ownerPerms}, nil).Maybe() + return store +} + +func effOwner() *User { + return &User{ID: effOwnerID, Email: "owner@example.com", Active: true, GroupIDs: []string{effGroupID}} +} + +func effKey(perms ...Permission) *UserAPIKey { + return &UserAPIKey{ID: effKeyID, UserID: effOwnerID, IsActive: true, Permissions: perms} +} + +func TestGetAPIKeyEffectivePermissionsAPI_AdminOwnerViewOnlyKey(t *testing.T) { + key := effKey(Permission{Action: ActionView, Resource: ResourceRecommendations}) + s := &Service{store: effStore(t, key, effOwner(), []Permission{{Action: ActionAdmin, Resource: ResourceAll}})} + + got, err := s.GetAPIKeyEffectivePermissionsAPI(context.Background(), effKeyID, effOwnerID) + require.NoError(t, err) + assert.Equal(t, []APIPermission{{Action: ActionView, Resource: ResourceRecommendations}}, got) +} + +// The intersection drops key permissions the owner lacks and keeps the KEY's +// constraints on what survives, not the owner's. +func TestGetAPIKeyEffectivePermissionsAPI_KeyConstraintsAndIntersection(t *testing.T) { + key := effKey( + Permission{Action: ActionExecute, Resource: ResourcePurchases, + Constraints: &PermissionConstraints{MaxPurchaseAmount: 100, Providers: []string{"aws"}}}, + Permission{Action: ActionDelete, Resource: ResourceUsers}, + ) + ownerPerms := []Permission{ + {Action: ActionExecute, Resource: ResourcePurchases, Constraints: &PermissionConstraints{MaxPurchaseAmount: 1000}}, + {Action: ActionView, Resource: ResourceRecommendations}, + } + s := &Service{store: effStore(t, key, effOwner(), ownerPerms)} + + got, err := s.GetAPIKeyEffectivePermissionsAPI(context.Background(), effKeyID, effOwnerID) + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, ActionExecute, got[0].Action) + assert.Equal(t, ResourcePurchases, got[0].Resource) + require.NotNil(t, got[0].Constraints) + assert.InDelta(t, 100, got[0].Constraints.MaxAmount, 0) + assert.Equal(t, []string{"aws"}, got[0].Constraints.Providers) +} + +func TestGetAPIKeyEffectivePermissionsAPI_UnscopedKeyGrantsNothing(t *testing.T) { + s := &Service{store: effStore(t, effKey(), effOwner(), []Permission{{Action: ActionAdmin, Resource: ResourceAll}})} + + got, err := s.GetAPIKeyEffectivePermissionsAPI(context.Background(), effKeyID, effOwnerID) + require.NoError(t, err) + assert.Empty(t, got) +} + +func TestGetAPIKeyEffectivePermissionsAPI_FailsClosed(t *testing.T) { + view := Permission{Action: ActionView, Resource: ResourceRecommendations} + past := time.Now().Add(-time.Hour) + admin := []Permission{{Action: ActionAdmin, Resource: ResourceAll}} + cases := []struct { + store func(t *testing.T) *MockStore + name string + }{ + {name: "key lookup error", store: func(t *testing.T) *MockStore { + s := new(MockStore) + s.On("GetAPIKeyByID", mock.Anything, effKeyID).Return((*UserAPIKey)(nil), errors.New("db down")) + return s + }}, + {name: "key not found", store: func(t *testing.T) *MockStore { + s := new(MockStore) + s.On("GetAPIKeyByID", mock.Anything, effKeyID).Return((*UserAPIKey)(nil), nil) + return s + }}, + {name: "key owned by someone else", store: func(t *testing.T) *MockStore { + k := effKey(view) + k.UserID = "someone-else" + return effStore(t, k, effOwner(), admin) + }}, + {name: "revoked key", store: func(t *testing.T) *MockStore { + k := effKey(view) + k.IsActive = false + return effStore(t, k, effOwner(), admin) + }}, + {name: "expired key", store: func(t *testing.T) *MockStore { + k := effKey(view) + k.ExpiresAt = &past + return effStore(t, k, effOwner(), admin) + }}, + {name: "inactive owner", store: func(t *testing.T) *MockStore { + o := effOwner() + o.Active = false + return effStore(t, effKey(view), o, admin) + }}, + {name: "owner password rotated", store: func(t *testing.T) *MockStore { + o := effOwner() + o.PasswordVersion = 7 + return effStore(t, effKey(view), o, admin) + }}, + {name: "owner group lookup error", store: func(t *testing.T) *MockStore { + s := new(MockStore) + s.On("GetAPIKeyByID", mock.Anything, effKeyID).Return(effKey(view), nil) + s.On("GetUserByID", mock.Anything, effOwnerID).Return(effOwner(), nil) + s.On("GetGroup", mock.Anything, effGroupID).Return((*Group)(nil), errors.New("db down")) + return s + }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + s := &Service{store: tc.store(t)} + got, err := s.GetAPIKeyEffectivePermissionsAPI(context.Background(), effKeyID, effOwnerID) + require.Error(t, err) + assert.Nil(t, got) + }) + } +} diff --git a/internal/server/app.go b/internal/server/app.go index 4a6f23103..50c19e9e4 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -1204,6 +1204,10 @@ func (a *authServiceAdapter) GetUserPermissionsAPI(ctx context.Context, userID s return a.service.GetUserPermissionsAPI(ctx, userID) } +func (a *authServiceAdapter) GetAPIKeyEffectivePermissionsAPI(ctx context.Context, keyID, userID string) ([]auth.APIPermission, error) { + return a.service.GetAPIKeyEffectivePermissionsAPI(ctx, keyID, userID) +} + // Account access. func (a *authServiceAdapter) GetAllowedAccountsAPI(ctx context.Context, userID string) ([]string, error) { // ResolveAllowedAccounts rather than BuildAuthContext: it fails closed when