diff --git a/internal/api/handler_auth.go b/internal/api/handler_auth.go index 2deed046..045786c1 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 00000000..76794efa --- /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 9135bd34..454c21fb 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 ed155a53..20dc9825 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 7012a152..27c592b0 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 6fddd209..1fedbb2e 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 9570dca8..9be54f1f 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 b86d7425..ec762de2 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 00000000..18222573 --- /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 4a6f2310..50c19e9e 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