From f5e45eb431ea615feb67c9eda0522cfe1cb5e1c1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 01:52:49 +0200 Subject: [PATCH 1/8] sec(auth): enforce a grant ceiling and system-managed guard on group writes CreateGroupAPI / UpdateGroupAPI wrote the client-supplied permission list onto a group verbatim, with no check that the caller may grant what they are granting and no consultation of the system_managed column. Because update:groups is not one of the pairs carved out of the admin:* wildcard, any admin could void the #923 money separation-of-duties control tenant-wide in a single request: PUT /api/groups/ {"permissions":[{admin,*},{execute,purchases},{approve-any,purchases}, {retry-any,purchases}]} Two rules now gate every group-permission write: 1. Ceiling: a caller may only grant permissions their own effective set already holds, matched through the same carve-out-aware logic used at enforcement time, and at constraints no broader than their own (a holder capped at $100 cannot hand out an uncapped grant). 2. Non-grantable: the three money verbs in adminCarvedOuts may never be ADDED to a group, whoever the caller is. This is load-bearing rather than belt-and-braces: migrations 000059/000064 backfill every Administrators member into the Purchaser group, so a default-deployment admin explicitly holds those verbs and rule 1 alone would let them relay the verbs onto the Administrators group. A carved-out permission already stored on the target group may be carried through an unrelated edit, but not widened, so a rename is not forced to strip it. Refusals fail closed and name the offending permission; the list is never silently narrowed to the allowed subset, which is the corruption mode #1629 reports on the frontend side. An unidentified actor, or any error resolving the actor's permissions, refuses the write. The stateless admin API key has no user row, so the ceiling measures it against a bare {admin, *} holding. It can still seed ordinary groups but is now subject to the same money carve-out as a human admin, closing the third vector in the report. system_managed is enforced on update and on delete. Delete is the third write path to a group's permissions and was named in neither issue: dropping the seeded Purchaser group destroys the only holder of the carved-out verbs, which nothing can then re-grant, so the purchase path would be dead tenant-wide. CreateGroupAPI / UpdateGroupAPI now take the acting principal, matching UpdateUserAPI's existing shape. updateGroup previously discarded its session. Refs #1550, #1629. --- internal/api/handler.go | 5 +- internal/api/handler_coverage_test.go | 4 +- internal/api/handler_groups.go | 31 +- internal/api/handler_groups_ceiling_test.go | 140 ++++++ internal/api/handler_groups_test.go | 4 +- internal/api/handler_ri_exchange_test.go | 6 +- internal/api/handler_router_test.go | 4 +- internal/api/mocks_test.go | 8 +- internal/api/types.go | 9 +- internal/auth/errors.go | 20 + internal/auth/group_ceiling.go | 202 ++++++++ internal/auth/group_ceiling_test.go | 517 ++++++++++++++++++++ internal/auth/service_api.go | 82 +++- internal/auth/service_api_test.go | 11 +- internal/auth/service_group.go | 17 + internal/auth/service_group_test.go | 4 + internal/server/adapter_test.go | 6 +- internal/server/app.go | 8 +- 18 files changed, 1022 insertions(+), 56 deletions(-) create mode 100644 internal/api/handler_groups_ceiling_test.go create mode 100644 internal/auth/group_ceiling.go create mode 100644 internal/auth/group_ceiling_test.go diff --git a/internal/api/handler.go b/internal/api/handler.go index d61f74986..b68d9bd0e 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -222,7 +222,10 @@ func NewHandler(cfg HandlerConfig) *Handler { // that would otherwise resolve group-derived permissions for it must treat it // as full-access up front (the API key is an infrastructure credential, not a // user). See requirePermission / requireAdmin / getAllowedAccounts. -const apiKeyAdminUserID = "admin-api-key" +// It is also the actor identity threaded into the group-write grant ceiling, +// which recognizes it and measures the key against a bare {admin, *} holding +// (auth.AdminAPIKeyActorID) rather than failing the user lookup. +const apiKeyAdminUserID = auth.AdminAPIKeyActorID // requirePermission validates authentication and checks if the user holds the // specified permission. The stateless admin API key bypasses the per-user diff --git a/internal/api/handler_coverage_test.go b/internal/api/handler_coverage_test.go index e2de3a9b0..b64feff96 100644 --- a/internal/api/handler_coverage_test.go +++ b/internal/api/handler_coverage_test.go @@ -502,7 +502,7 @@ func TestRouter_Handlers_Coverage(t *testing.T) { mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", ctx, "admin-token").Return(&Session{UserID: "admin"}, nil) mockAuth.grantAdmin() - mockAuth.On("CreateGroupAPI", ctx, mock.Anything).Return(map[string]string{"id": "new-group"}, nil) + mockAuth.On("CreateGroupAPI", ctx, "admin", mock.Anything).Return(map[string]string{"id": "new-group"}, nil) h := &Handler{auth: mockAuth} router := NewRouter(h) @@ -540,7 +540,7 @@ func TestRouter_Handlers_Coverage(t *testing.T) { mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", ctx, "admin-token").Return(&Session{UserID: "admin"}, nil) mockAuth.grantAdmin() - mockAuth.On("UpdateGroupAPI", ctx, "11111111-1111-1111-1111-111111111111", mock.Anything).Return(map[string]string{}, nil) + mockAuth.On("UpdateGroupAPI", ctx, "admin", "11111111-1111-1111-1111-111111111111", mock.Anything).Return(map[string]string{}, nil) h := &Handler{auth: mockAuth} router := NewRouter(h) diff --git a/internal/api/handler_groups.go b/internal/api/handler_groups.go index 320cd1b5c..ffc840b11 100644 --- a/internal/api/handler_groups.go +++ b/internal/api/handler_groups.go @@ -4,6 +4,7 @@ package api import ( "context" "encoding/json" + "errors" "github.com/LeanerCloud/CUDly/internal/auth" "github.com/LeanerCloud/CUDly/pkg/logging" @@ -49,14 +50,29 @@ func (h *Handler) createGroup(ctx context.Context, req *events.LambdaFunctionURL return nil, NewClientError(400, "invalid request body") } - group, err := h.auth.CreateGroupAPI(ctx, createReq) + group, err := h.auth.CreateGroupAPI(ctx, session.UserID, createReq) if err != nil { - return nil, err + return nil, mapGroupAuthError(err) } return group, nil } +// mapGroupAuthError maps the group-write sentinels to 403 ClientErrors so a +// refused grant surfaces the specific permission that was refused instead of +// a generic 500. Kept separate from mapAuthError (handler_users.go) because +// that switch is already at the cyclomatic limit and the two sentinel sets +// are disjoint. Unrecognized errors pass through for handleRequestError. +func mapGroupAuthError(err error) error { + switch { + case errors.Is(err, auth.ErrPermissionCeiling), + errors.Is(err, auth.ErrPermissionNotGrantable), + errors.Is(err, auth.ErrSystemManagedGroup): + return NewClientError(403, err.Error()) + } + return err +} + // getGroup handles GET /api/groups/{id}. func (h *Handler) getGroup(ctx context.Context, req *events.LambdaFunctionURLRequest, groupID string) (any, error) { // Validate UUID format to prevent injection attacks @@ -83,18 +99,19 @@ func (h *Handler) updateGroup(ctx context.Context, req *events.LambdaFunctionURL return nil, err } - if _, err := h.requirePermission(ctx, req, "update", "groups"); err != nil { + session, err := h.requirePermission(ctx, req, "update", "groups") + if err != nil { return nil, err } var updateReq auth.APIUpdateGroupRequest - if err := json.Unmarshal([]byte(req.Body), &updateReq); err != nil { + if unmarshalErr := json.Unmarshal([]byte(req.Body), &updateReq); unmarshalErr != nil { return nil, NewClientError(400, "invalid request body") } - group, err := h.auth.UpdateGroupAPI(ctx, groupID, updateReq) + group, err := h.auth.UpdateGroupAPI(ctx, session.UserID, groupID, updateReq) if err != nil { - return nil, err + return nil, mapGroupAuthError(err) } return group, nil @@ -112,7 +129,7 @@ func (h *Handler) deleteGroup(ctx context.Context, req *events.LambdaFunctionURL } if err := h.auth.DeleteGroup(ctx, groupID); err != nil { - return nil, err + return nil, mapGroupAuthError(err) } return map[string]string{"status": "group deleted"}, nil diff --git a/internal/api/handler_groups_ceiling_test.go b/internal/api/handler_groups_ceiling_test.go new file mode 100644 index 000000000..4b0502707 --- /dev/null +++ b/internal/api/handler_groups_ceiling_test.go @@ -0,0 +1,140 @@ +package api + +import ( + "context" + "fmt" + "testing" + + "github.com/LeanerCloud/CUDly/internal/auth" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Handler-level coverage for the group grant ceiling (issues #1550, #1629). +// +// The ceiling itself is enforced in internal/auth; what these tests pin is +// the wiring the auth package cannot see: that the AUTHENTICATED session's +// user ID reaches the service as the actor (a ceiling measured against the +// wrong principal is no ceiling), and that a refusal surfaces as a 403 +// naming the permission rather than a generic 500. + +const ceilingGroupID = "11111111-1111-1111-1111-111111111111" + +func TestUpdateGroup_ThreadsSessionActorAndMaps403(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(session, nil) + mockAuth.grantAdmin() + + refusal := fmt.Errorf("%w: execute:purchases is reserved for separation of duties (issue #923) and cannot be granted through the API", + auth.ErrPermissionNotGrantable) + // The actor argument is asserted EXACTLY: if updateGroup passed anything + // other than the validated session's user ID, this expectation would not + // match and the mock would fail the test. + mockAuth.On("UpdateGroupAPI", ctx, session.UserID, ceilingGroupID, mock.Anything). + Return(nil, refusal).Once() + + h := &Handler{auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"permissions":[{"action":"execute","resource":"purchases"}]}`, + } + + result, err := h.updateGroup(ctx, req, ceilingGroupID) + + require.Error(t, err) + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok, "a refused grant must map to a client error, not a 500") + assert.Equal(t, 403, ce.code) + assert.Contains(t, err.Error(), "execute:purchases") +} + +func TestCreateGroup_ThreadsSessionActorAndMaps403(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(session, nil) + mockAuth.grantAdmin() + + refusal := fmt.Errorf("%w: cannot grant delete:accounts because your own permissions do not include it (or not at the requested scope)", + auth.ErrPermissionCeiling) + mockAuth.On("CreateGroupAPI", ctx, session.UserID, mock.Anything). + Return(nil, refusal).Once() + + h := &Handler{auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"name":"Escalated","permissions":[{"action":"delete","resource":"accounts"}]}`, + } + + result, err := h.createGroup(ctx, req) + + require.Error(t, err) + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok, "a refused grant must map to a client error, not a 500") + assert.Equal(t, 403, ce.code) + assert.Contains(t, err.Error(), "delete:accounts") +} + +func TestDeleteGroup_SystemManagedMaps403(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(session, nil) + mockAuth.grantAdmin() + + refusal := fmt.Errorf("%w: %q is seeded and maintained by migrations", auth.ErrSystemManagedGroup, "Purchaser") + mockAuth.On("DeleteGroup", ctx, ceilingGroupID).Return(refusal).Once() + + h := &Handler{auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + } + + result, err := h.deleteGroup(ctx, req, ceilingGroupID) + + require.Error(t, err) + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok, "a system-managed refusal must map to a client error, not a 500") + assert.Equal(t, 403, ce.code) + assert.Contains(t, err.Error(), "Purchaser") +} + +// TestUpdateGroup_AdminAPIKeyActorIsSentinel pins the admin-API-key branch: +// the key has no user row, so the handler must hand the auth service the +// sentinel the ceiling recognizes. Passing an empty string here would make +// the ceiling fail closed on every admin-key group write instead. +func TestUpdateGroup_AdminAPIKeyActorIsSentinel(t *testing.T) { + assert.Equal(t, auth.AdminAPIKeyActorID, apiKeyAdminUserID, + "the admin-API-key session identity must be the sentinel auth.grantCeilingPermissions recognizes") + + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + updated := map[string]any{"id": ceilingGroupID} + mockAuth.On("UpdateGroupAPI", ctx, apiKeyAdminUserID, ceilingGroupID, mock.Anything). + Return(updated, nil).Once() + + h := &Handler{auth: mockAuth, apiKey: "super-secret-admin-key"} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"x-api-key": "super-secret-admin-key"}, + Body: `{"name":"Renamed"}`, + } + + result, err := h.updateGroup(ctx, req, ceilingGroupID) + require.NoError(t, err) + assert.Equal(t, updated, result) +} diff --git a/internal/api/handler_groups_test.go b/internal/api/handler_groups_test.go index 1a49d4969..e2aa18ed6 100644 --- a/internal/api/handler_groups_test.go +++ b/internal/api/handler_groups_test.go @@ -59,7 +59,7 @@ func TestHandler_createGroup_Success(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockAuth.On("CreateGroupAPI", ctx, mock.Anything).Return(createdGroup, nil) + mockAuth.On("CreateGroupAPI", ctx, adminSession.UserID, mock.Anything).Return(createdGroup, nil) handler := &Handler{auth: mockAuth} @@ -122,7 +122,7 @@ func TestHandler_updateGroup_Success(t *testing.T) { mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockAuth.On("UpdateGroupAPI", ctx, "11111111-1111-1111-1111-111111111111", mock.Anything).Return(updatedGroup, nil) + mockAuth.On("UpdateGroupAPI", ctx, adminSession.UserID, "11111111-1111-1111-1111-111111111111", mock.Anything).Return(updatedGroup, nil) handler := &Handler{auth: mockAuth} diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 6359041df..a2b0da82d 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -1218,8 +1218,10 @@ func (m *mockAuthForExchange) UpdateUserAPI(_ context.Context, _, _ string, _ an func (m *mockAuthForExchange) DeleteUser(_ context.Context, _ string) error { return nil } func (m *mockAuthForExchange) ListUsersAPI(_ context.Context) (any, error) { return nil, nil } func (m *mockAuthForExchange) ChangePasswordAPI(_ context.Context, _, _, _ string) error { return nil } -func (m *mockAuthForExchange) CreateGroupAPI(_ context.Context, _ any) (any, error) { return nil, nil } -func (m *mockAuthForExchange) UpdateGroupAPI(_ context.Context, _ string, _ any) (any, error) { +func (m *mockAuthForExchange) CreateGroupAPI(_ context.Context, _ string, _ any) (any, error) { + return nil, nil +} +func (m *mockAuthForExchange) UpdateGroupAPI(_ context.Context, _, _ string, _ any) (any, error) { return nil, nil } func (m *mockAuthForExchange) DeleteGroup(_ context.Context, _ string) error { return nil } diff --git a/internal/api/handler_router_test.go b/internal/api/handler_router_test.go index b57f6d357..10060d794 100644 --- a/internal/api/handler_router_test.go +++ b/internal/api/handler_router_test.go @@ -38,7 +38,7 @@ func TestHandler_updateGroup_Error(t *testing.T) { adminSession := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"} mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockAuth.On("UpdateGroupAPI", ctx, "11111111-1111-1111-1111-111111111111", mock.Anything).Return(nil, assert.AnError) + mockAuth.On("UpdateGroupAPI", ctx, adminSession.UserID, "11111111-1111-1111-1111-111111111111", mock.Anything).Return(nil, assert.AnError) handler := &Handler{auth: mockAuth} @@ -79,7 +79,7 @@ func TestHandler_createGroup_Error(t *testing.T) { adminSession := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"} mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) mockAuth.grantAdmin() - mockAuth.On("CreateGroupAPI", ctx, mock.Anything).Return(nil, assert.AnError) + mockAuth.On("CreateGroupAPI", ctx, adminSession.UserID, mock.Anything).Return(nil, assert.AnError) handler := &Handler{auth: mockAuth} diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index 081a5f92c..9f1be1c2f 100644 --- a/internal/api/mocks_test.go +++ b/internal/api/mocks_test.go @@ -250,13 +250,13 @@ func (m *MockAuthService) MFARegenerateRecoveryCodesAPI(ctx context.Context, use } // Group management mock methods. -func (m *MockAuthService) CreateGroupAPI(ctx context.Context, req interface{}) (interface{}, error) { - args := m.Called(ctx, req) +func (m *MockAuthService) CreateGroupAPI(ctx context.Context, actorUserID string, req interface{}) (interface{}, error) { + args := m.Called(ctx, actorUserID, req) return args.Get(0), args.Error(1) } -func (m *MockAuthService) UpdateGroupAPI(ctx context.Context, groupID string, req interface{}) (interface{}, error) { - args := m.Called(ctx, groupID, req) +func (m *MockAuthService) UpdateGroupAPI(ctx context.Context, actorUserID, groupID string, req interface{}) (interface{}, error) { + args := m.Called(ctx, actorUserID, groupID, req) return args.Get(0), args.Error(1) } diff --git a/internal/api/types.go b/internal/api/types.go index 08a282341..a6676e278 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -186,8 +186,13 @@ type AuthServiceInterface interface { MFADisableAPI(ctx context.Context, userID, password, codeOrRecovery string) error MFARegenerateRecoveryCodesAPI(ctx context.Context, userID, code string) (recoveryCodes []string, err error) // Group management - uses auth.API* types - CreateGroupAPI(ctx context.Context, req any) (any, error) - UpdateGroupAPI(ctx context.Context, groupID string, req any) (any, error) + // CreateGroupAPI / UpdateGroupAPI take the acting principal so the + // service can enforce the grant ceiling: a caller may not write a + // permission onto a group that their own effective permissions do not + // hold, and the money verbs carved out of admin:* are not grantable at + // all (issues #1550, #1629). + CreateGroupAPI(ctx context.Context, actorUserID string, req any) (any, error) + UpdateGroupAPI(ctx context.Context, actorUserID, groupID string, req any) (any, error) DeleteGroup(ctx context.Context, groupID string) error GetGroupAPI(ctx context.Context, groupID string) (any, error) ListGroupsAPI(ctx context.Context) (any, error) diff --git a/internal/auth/errors.go b/internal/auth/errors.go index 6f3dcda24..0f6326226 100644 --- a/internal/auth/errors.go +++ b/internal/auth/errors.go @@ -34,6 +34,26 @@ var ( // permission. Mapped to 403 (issue #907). ErrSelfEscalation = errors.New("cannot escalate your own group membership") + // ErrPermissionCeiling is returned by a group write that would grant a + // permission the acting user does not themselves hold (or holds only at + // a narrower constraint scope), and by the fail-closed paths where the + // acting user's own permissions cannot be resolved at all. Mapped to 403 + // (issue #1550). + ErrPermissionCeiling = errors.New("permission ceiling exceeded") + + // ErrPermissionNotGrantable is returned by a group write that would ADD + // one of the money-spending verbs carved out of the admin:* wildcard + // (adminCarvedOuts). Those are reserved for separation of duties and are + // provisioned by migration, never through the API. Mapped to 403 + // (issues #923, #1550). + ErrPermissionNotGrantable = errors.New("permission not grantable") + + // ErrSystemManagedGroup is returned when a write targets one of the + // seeded, system-managed groups. Their contents are owned by migrations; + // an API edit that reshapes them can silently disable a whole capability + // tenant-wide. Mapped to 403 (issue #1629). + ErrSystemManagedGroup = errors.New("system-managed group cannot be modified") + // ErrCurrentPasswordIncorrect is returned by UpdateUserProfile when the // caller-supplied current password does not match the stored hash. Mapped // to 401 at the API layer (the acting user is verifying their own diff --git a/internal/auth/group_ceiling.go b/internal/auth/group_ceiling.go new file mode 100644 index 000000000..bcddb3171 --- /dev/null +++ b/internal/auth/group_ceiling.go @@ -0,0 +1,202 @@ +package auth + +import ( + "context" + "fmt" + "strings" +) + +// Grant ceiling for group-permission writes (issues #1550, #1629). +// +// Before this, CreateGroupAPI / UpdateGroupAPI wrote the client-supplied +// permission list onto a group verbatim. Because update:groups is NOT one of +// the pairs carved out of the admin:* wildcard, any admin could, in a single +// request, write execute:purchases / approve-any:purchases / +// retry-any:purchases onto their own group and void the #923 money +// separation-of-duties control tenant-wide. +// +// Two rules close that, and both are enforced here: +// +// 1. Ceiling: a caller may only grant permissions their own effective set +// already holds, at constraints no broader than their own. +// 2. Non-grantable: the money verbs in adminCarvedOuts may never be ADDED to +// a group at all, whoever the caller is. This second rule is load-bearing +// rather than belt-and-braces: migrations 000059/000064 backfill every +// Administrators member into the Purchaser group, so in a default +// deployment a typical admin DOES explicitly hold the money verbs and +// rule 1 alone would let them relay those verbs onto the Administrators +// group. The only group granting these verbs today is seeded by SQL and +// is system-managed, so nothing legitimate needs the API to grant them. + +// AdminAPIKeyActorID is the sentinel actor identifier carried by the +// stateless admin API-key session. The key is an infrastructure credential +// with no backing user row, so a group-derived permission lookup cannot +// resolve it. The ceiling treats it as holding exactly {admin, *} -- what it +// is authorized as everywhere else -- which subjects it to the same +// carve-out as a human admin: it can seed ordinary groups but cannot grant +// the money-spending verbs (issue #1550's third vector). +// +// internal/api's apiKeyAdminUserID aliases this constant; keep them equal. +const AdminAPIKeyActorID = "admin-api-key" + +// grantCeilingPermissions returns the permission set a group write is +// measured against. Fails closed: an unidentified actor, or any error +// resolving the actor's groups, refuses the write rather than falling +// through to "allow". +func (s *Service) grantCeilingPermissions(ctx context.Context, actorUserID string) ([]Permission, error) { + if actorUserID == "" { + return nil, fmt.Errorf("%w: the acting user could not be identified", ErrPermissionCeiling) + } + if actorUserID == AdminAPIKeyActorID { + return []Permission{{Action: ActionAdmin, Resource: ResourceAll}}, nil + } + perms, err := s.GetUserPermissions(ctx, actorUserID) + if err != nil { + return nil, fmt.Errorf("%w: could not resolve the acting user's permissions: %w", ErrPermissionCeiling, err) + } + return perms, nil +} + +// checkGrantCeiling validates a requested group-permission list against the +// two rules above. +// +// existing is the target group's CURRENT permission list (nil on create). A +// carved-out permission already on the group may be carried through an +// unrelated edit -- a rename must not be forced to strip it -- but only at +// constraints no broader than the ones already stored, so an edit cannot +// raise an existing MaxPurchaseAmount either. +// +// A refusal names the offending permission and returns an error: the list is +// never silently narrowed to the subset that would have been allowed, which +// is exactly the silent-corruption failure mode #1629 reports on the +// frontend side. +func (s *Service) checkGrantCeiling(ctx context.Context, actorUserID string, requested, existing []Permission) error { + if len(requested) == 0 { + return nil + } + actorPerms, err := s.grantCeilingPermissions(ctx, actorUserID) + if err != nil { + return err + } + for _, req := range requested { + if adminCarvedOuts[[2]string{req.Action, req.Resource}] { + if permissionCoveredBy(existing, req) { + continue + } + return fmt.Errorf( + "%w: %s:%s is reserved for separation of duties (issue #923) and cannot be granted through the API", + ErrPermissionNotGrantable, req.Action, req.Resource) + } + if !grantCeilingAllows(actorPerms, req) { + return fmt.Errorf( + "%w: cannot grant %s:%s because your own permissions do not include it (or not at the requested scope)", + ErrPermissionCeiling, req.Action, req.Resource) + } + } + return nil +} + +// grantCeilingAllows reports whether actorPerms holds req in full. Action and +// resource matching mirrors permissionsAllow exactly, including the admin:* +// carve-out, so the ceiling can never be looser than enforcement. It adds one +// requirement enforcement does not need: a constrained holder cannot hand out +// an unconstrained (or differently scoped) copy of their own permission. +func grantCeilingAllows(actorPerms []Permission, req Permission) bool { + for _, held := range actorPerms { + if checkAdminPermission(held) { + // admin:* carries no constraints, so it covers any requested + // constraint set. Carved-out pairs never reach here (the caller + // rejects them first), but mirror permissionsAllow anyway so the + // two stay in lockstep if the carve-out set grows (#1644). + if adminCarvedOuts[[2]string{req.Action, req.Resource}] { + continue + } + return true + } + if !checkPermissionMatch(held, req.Action, req.Resource) { + continue + } + if !constraintsCover(held.Constraints, req.Constraints) { + continue + } + return true + } + return false +} + +// permissionCoveredBy reports whether set already contains a permission with +// req's exact action and resource, at constraints no narrower than req's. It +// distinguishes "this write KEEPS what the group already had" from "this +// write grants or widens it". +func permissionCoveredBy(set []Permission, req Permission) bool { + for _, p := range set { + if p.Action != req.Action || p.Resource != req.Resource { + continue + } + if constraintsCover(p.Constraints, req.Constraints) { + return true + } + } + return false +} + +// constraintsCover reports whether a permission constrained by held is broad +// enough to cover a grant constrained by req. +// +// Mirrors the enforcement-time semantics in matchConstraints: an empty list +// or a zero MaxPurchaseAmount means "no restriction on this dimension". So an +// unconstrained holder covers anything, while a constrained one requires the +// grant to name a subset of the same values and a cap no higher than its own. +// A nil req against a constrained held is a widening and is refused. +func constraintsCover(held, req *PermissionConstraints) bool { + if held == nil { + return true + } + if req == nil { + req = &PermissionConstraints{} + } + return listCovers(held.AccountIDs, req.AccountIDs) && + listCovers(held.Providers, req.Providers) && + listCovers(held.Services, req.Services) && + listCovers(held.Regions, req.Regions) && + amountCovers(held.MaxPurchaseAmount, req.MaxPurchaseAmount) +} + +// listCovers reports whether every value in req is permitted by held. An +// empty held list is "no restriction on this dimension" and covers anything; +// a non-empty held list requires req to be a NON-EMPTY subset, so a grant can +// never drop the restriction. Values are trimmed and lower-cased, matching +// matchAllRegionsConstraint's comparison. +func listCovers(held, req []string) bool { + if len(held) == 0 { + return true + } + if len(req) == 0 { + return false + } + permitted := make(map[string]bool, len(held)) + for _, v := range held { + permitted[normalizeConstraintValue(v)] = true + } + for _, v := range req { + if !permitted[normalizeConstraintValue(v)] { + return false + } + } + return true +} + +func normalizeConstraintValue(v string) string { + return strings.ToLower(strings.TrimSpace(v)) +} + +// amountCovers reports whether a grant capped at reqMax stays within a holder +// capped at heldMax. Zero means "no cap" on both sides (mirroring +// matchPurchaseAmountConstraint), so a capped holder cannot hand out an +// uncapped grant. +func amountCovers(heldMax, reqMax float64) bool { + if heldMax <= 0 { + return true + } + return reqMax > 0 && reqMax <= heldMax +} diff --git a/internal/auth/group_ceiling_test.go b/internal/auth/group_ceiling_test.go new file mode 100644 index 000000000..064a22286 --- /dev/null +++ b/internal/auth/group_ceiling_test.go @@ -0,0 +1,517 @@ +package auth + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Grant-ceiling regression tests for issues #1550 and #1629. +// +// Every refusal case here deliberately stubs NO CreateGroup / UpdateGroup / +// DeleteGroup expectation on the mock store. testify's mock panics on an +// un-stubbed call, so if the guard under test were removed the write would +// reach the store and the test would fail loudly rather than silently pass. +// That is the mutation check: these assertions are not vacuous. + +const ( + ceilingActorID = "11111111-1111-4111-8111-111111111111" + ceilingActorGroupID = "22222222-2222-4222-8222-222222222222" + ceilingTargetID = "33333333-3333-4333-8333-333333333333" +) + +// stubActorPermissions wires the two store lookups GetUserPermissions makes +// for the acting user: the user row, then each of its groups. +func stubActorPermissions(ctx context.Context, mockStore *MockStore, perms []Permission) { + mockStore.On("GetUserByID", ctx, ceilingActorID). + Return(&User{ID: ceilingActorID, GroupIDs: []string{ceilingActorGroupID}}, nil) + mockStore.On("GetGroup", ctx, ceilingActorGroupID). + Return(&Group{ID: ceilingActorGroupID, Name: "Actor Group", Permissions: perms}, nil) +} + +func stubTargetGroup(ctx context.Context, mockStore *MockStore, group *Group) { + mockStore.On("GetGroup", ctx, ceilingTargetID).Return(group, nil) +} + +func newCeilingService(t *testing.T, mockStore *MockStore) *Service { + t.Helper() + return createTestService(mockStore, new(MockEmailSender)) +} + +func updateReqWith(perms ...APIPermission) APIUpdateGroupRequest { + return APIUpdateGroupRequest{Name: "Renamed", Permissions: perms} +} + +var adminOnly = []Permission{{Action: ActionAdmin, Resource: ResourceAll}} + +// TestGrantCeiling_CarvedOutNotGrantable is the #1550 attack itself: an admin +// writing the money verbs onto a group in a single request. +func TestGrantCeiling_CarvedOutNotGrantable(t *testing.T) { + ctx := context.Background() + + carvedOut := []APIPermission{ + {Action: ActionExecute, Resource: ResourcePurchases}, + {Action: ActionApproveAny, Resource: ResourcePurchases}, + {Action: ActionRetryAny, Resource: ResourcePurchases}, + } + + for _, perm := range carvedOut { + t.Run(perm.Action+":"+perm.Resource, func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + + stubActorPermissions(ctx, mockStore, adminOnly) + stubTargetGroup(ctx, mockStore, &Group{ID: ceilingTargetID, Name: "Administrators"}) + + result, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, updateReqWith(perm)) + + require.Error(t, err) + assert.Nil(t, result) + assert.ErrorIs(t, err, ErrPermissionNotGrantable) + // The refusal must name the exact permission (#1629: never + // silently narrow the list). + assert.Contains(t, err.Error(), perm.Action+":"+perm.Resource) + mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) + }) + } +} + +// TestGrantCeiling_CarvedOutNotGrantableByPurchaserAdmin is the load-bearing +// case. Migrations 000059/000064 backfill every Administrators member into +// the Purchaser group, so a default-deployment admin explicitly HOLDS the +// money verbs. A ceiling that only asked "do you hold it?" would let that +// admin relay the verbs onto the Administrators group and #1550 would stay +// open in the default configuration. +func TestGrantCeiling_CarvedOutNotGrantableByPurchaserAdmin(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + + stubActorPermissions(ctx, mockStore, []Permission{ + {Action: ActionAdmin, Resource: ResourceAll}, + {Action: ActionExecute, Resource: ResourcePurchases}, + {Action: ActionApproveAny, Resource: ResourcePurchases}, + {Action: ActionRetryAny, Resource: ResourcePurchases}, + }) + stubTargetGroup(ctx, mockStore, &Group{ID: ceilingTargetID, Name: "Administrators"}) + + _, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, + updateReqWith(APIPermission{Action: ActionExecute, Resource: ResourcePurchases})) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrPermissionNotGrantable) + mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) +} + +// TestGrantCeiling_CarvedOutMayBeKeptNotWidened: an unrelated edit to a group +// that already holds a money verb must not be forced to strip it, but must +// not be able to raise its cap either. +func TestGrantCeiling_CarvedOutMayBeKeptNotWidened(t *testing.T) { + ctx := context.Background() + + t.Run("kept unchanged is allowed", func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + + stubActorPermissions(ctx, mockStore, adminOnly) + stubTargetGroup(ctx, mockStore, &Group{ + ID: ceilingTargetID, + Name: "Custom Purchasers", + Permissions: []Permission{{Action: ActionExecute, Resource: ResourcePurchases}}, + }) + mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Once() + + _, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, updateReqWith( + APIPermission{Action: ActionExecute, Resource: ResourcePurchases}, + APIPermission{Action: ActionView, Resource: ResourcePlans}, + )) + require.NoError(t, err) + }) + + t.Run("widening an existing cap is refused", func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + + stubActorPermissions(ctx, mockStore, adminOnly) + stubTargetGroup(ctx, mockStore, &Group{ + ID: ceilingTargetID, + Name: "Custom Purchasers", + Permissions: []Permission{{ + Action: ActionExecute, + Resource: ResourcePurchases, + Constraints: &PermissionConstraints{MaxPurchaseAmount: 100}, + }}, + }) + + // Same action/resource, cap removed entirely. + _, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, + updateReqWith(APIPermission{Action: ActionExecute, Resource: ResourcePurchases})) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrPermissionNotGrantable) + mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) + }) +} + +// TestGrantCeiling_CannotGrantUnheldPermission covers rule 1: you cannot hand +// out what you do not hold. +func TestGrantCeiling_CannotGrantUnheldPermission(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + + // A group manager: can administer groups, but holds nothing on accounts. + stubActorPermissions(ctx, mockStore, []Permission{ + {Action: ActionUpdate, Resource: ResourceGroups}, + {Action: ActionView, Resource: ResourcePlans}, + }) + stubTargetGroup(ctx, mockStore, &Group{ID: ceilingTargetID, Name: "Team"}) + + _, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, + updateReqWith(APIPermission{Action: ActionDelete, Resource: ResourceAccounts})) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrPermissionCeiling) + assert.Contains(t, err.Error(), ActionDelete+":"+ResourceAccounts) + mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) +} + +// TestGrantCeiling_CannotWidenResourceWildcard: holding view on one resource +// does not authorize granting view on every resource. +func TestGrantCeiling_CannotWidenResourceWildcard(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + + stubActorPermissions(ctx, mockStore, []Permission{ + {Action: ActionUpdate, Resource: ResourceGroups}, + {Action: ActionView, Resource: ResourcePlans}, + }) + stubTargetGroup(ctx, mockStore, &Group{ID: ceilingTargetID, Name: "Team"}) + + _, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, + updateReqWith(APIPermission{Action: ActionView, Resource: ResourceAll})) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrPermissionCeiling) + assert.Contains(t, err.Error(), ActionView+":"+ResourceAll) + mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) +} + +// TestGrantCeiling_InCeilingUpdateSucceeds is the negative control. Without +// it, a guard that refused every write would pass every test above. +func TestGrantCeiling_InCeilingUpdateSucceeds(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + + stubActorPermissions(ctx, mockStore, []Permission{ + {Action: ActionUpdate, Resource: ResourceGroups}, + {Action: ActionView, Resource: ResourcePlans}, + {Action: ActionView, Resource: ResourceRecommendations}, + }) + stubTargetGroup(ctx, mockStore, &Group{ID: ceilingTargetID, Name: "Team"}) + + var saved *Group + mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")). + Run(func(args mock.Arguments) { + g, ok := args.Get(1).(*Group) + require.True(t, ok) + saved = g + }).Return(nil).Once() + + result, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, updateReqWith( + APIPermission{Action: ActionView, Resource: ResourcePlans}, + APIPermission{Action: ActionView, Resource: ResourceRecommendations}, + )) + + require.NoError(t, err) + require.NotNil(t, result) + require.NotNil(t, saved) + // The full requested list is persisted; nothing was quietly dropped. + assert.Equal(t, []Permission{ + {Action: ActionView, Resource: ResourcePlans}, + {Action: ActionView, Resource: ResourceRecommendations}, + }, saved.Permissions) +} + +// TestGrantCeiling_ConstraintContainment: a constrained holder may hand out a +// narrower grant but not a broader one. +func TestGrantCeiling_ConstraintContainment(t *testing.T) { + ctx := context.Background() + + held := []Permission{ + {Action: ActionUpdate, Resource: ResourceGroups}, + { + Action: ActionExecute, + Resource: ResourceRIExchange, + Constraints: &PermissionConstraints{ + Providers: []string{"aws"}, + MaxPurchaseAmount: 100, + }, + }, + } + + t.Run("narrower grant is allowed", func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + + stubActorPermissions(ctx, mockStore, held) + stubTargetGroup(ctx, mockStore, &Group{ID: ceilingTargetID, Name: "Team"}) + mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Once() + + _, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, + updateReqWith(APIPermission{ + Action: ActionExecute, + Resource: ResourceRIExchange, + Constraints: &APIPermissionConstraint{ + Providers: []string{"aws"}, + MaxAmount: 50, + }, + })) + require.NoError(t, err) + }) + + t.Run("dropping the cap is refused", func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + + stubActorPermissions(ctx, mockStore, held) + stubTargetGroup(ctx, mockStore, &Group{ID: ceilingTargetID, Name: "Team"}) + + _, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, + updateReqWith(APIPermission{Action: ActionExecute, Resource: ResourceRIExchange})) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrPermissionCeiling) + mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) + }) + + t.Run("adding an unheld provider is refused", func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + + stubActorPermissions(ctx, mockStore, held) + stubTargetGroup(ctx, mockStore, &Group{ID: ceilingTargetID, Name: "Team"}) + + _, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, + updateReqWith(APIPermission{ + Action: ActionExecute, + Resource: ResourceRIExchange, + Constraints: &APIPermissionConstraint{ + Providers: []string{"aws", "azure"}, + MaxAmount: 50, + }, + })) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrPermissionCeiling) + mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) + }) +} + +// TestGrantCeiling_FailsClosed: if the acting principal cannot be identified +// or their permissions cannot be resolved, the write is refused rather than +// falling through to "allow". +func TestGrantCeiling_FailsClosed(t *testing.T) { + ctx := context.Background() + + t.Run("unidentified actor", func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + + stubTargetGroup(ctx, mockStore, &Group{ID: ceilingTargetID, Name: "Team"}) + + _, err := svc.UpdateGroupAPI(ctx, "", ceilingTargetID, + updateReqWith(APIPermission{Action: ActionView, Resource: ResourcePlans})) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrPermissionCeiling) + mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) + }) + + t.Run("actor permission lookup fails", func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + + lookupErr := errors.New("database unavailable") + mockStore.On("GetUserByID", ctx, ceilingActorID).Return(nil, lookupErr) + stubTargetGroup(ctx, mockStore, &Group{ID: ceilingTargetID, Name: "Team"}) + + _, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, + updateReqWith(APIPermission{Action: ActionView, Resource: ResourcePlans})) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrPermissionCeiling) + mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) + }) +} + +// TestGrantCeiling_AdminAPIKeyActor: the stateless admin API key has no user +// row. It is measured as a bare {admin, *} holder, so it can seed ordinary +// groups but is subject to the same money carve-out as a human admin +// (#1550's third vector). +func TestGrantCeiling_AdminAPIKeyActor(t *testing.T) { + ctx := context.Background() + + t.Run("may create an ordinary group", func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + + mockStore.On("CreateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Once() + + _, err := svc.CreateGroupAPI(ctx, AdminAPIKeyActorID, APICreateGroupRequest{ + Name: "Viewers", + Permissions: []APIPermission{{Action: ActionView, Resource: ResourcePlans}}, + }) + require.NoError(t, err) + // No user lookup is attempted for the key. + mockStore.AssertNotCalled(t, "GetUserByID", mock.Anything, mock.Anything) + }) + + t.Run("may not grant a carved-out permission", func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + + _, err := svc.CreateGroupAPI(ctx, AdminAPIKeyActorID, APICreateGroupRequest{ + Name: "Shadow Purchasers", + Permissions: []APIPermission{{Action: ActionExecute, Resource: ResourcePurchases}}, + }) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrPermissionNotGrantable) + mockStore.AssertNotCalled(t, "CreateGroup", mock.Anything, mock.Anything) + }) +} + +// TestGrantCeiling_CreateGroupAPI covers the create path's ceiling directly. +func TestGrantCeiling_CreateGroupAPI(t *testing.T) { + ctx := context.Background() + + t.Run("refuses an unheld permission", func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + + stubActorPermissions(ctx, mockStore, []Permission{ + {Action: ActionCreate, Resource: ResourceGroups}, + {Action: ActionView, Resource: ResourcePlans}, + }) + + _, err := svc.CreateGroupAPI(ctx, ceilingActorID, APICreateGroupRequest{ + Name: "Escalated", + Permissions: []APIPermission{{Action: ActionAdmin, Resource: ResourceAll}}, + }) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrPermissionCeiling) + assert.Contains(t, err.Error(), ActionAdmin+":"+ResourceAll) + mockStore.AssertNotCalled(t, "CreateGroup", mock.Anything, mock.Anything) + }) + + t.Run("allows an in-ceiling permission", func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + + stubActorPermissions(ctx, mockStore, []Permission{ + {Action: ActionCreate, Resource: ResourceGroups}, + {Action: ActionView, Resource: ResourcePlans}, + }) + mockStore.On("CreateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Once() + + _, err := svc.CreateGroupAPI(ctx, ceilingActorID, APICreateGroupRequest{ + Name: "Plan Viewers", + Permissions: []APIPermission{{Action: ActionView, Resource: ResourcePlans}}, + }) + require.NoError(t, err) + }) +} + +// TestSystemManagedGroup_Immutable covers the second half of #1629: the +// seeded groups are owned by migrations and no API verb may reshape them. +func TestSystemManagedGroup_Immutable(t *testing.T) { + ctx := context.Background() + + seeded := func() *Group { + return &Group{ + ID: DefaultPurchaserGroupID, + Name: GroupPurchaser, + SystemManaged: true, + Permissions: []Permission{ + {Action: ActionExecute, Resource: ResourcePurchases}, + {Action: ActionApproveAny, Resource: ResourcePurchases}, + {Action: ActionRetryAny, Resource: ResourcePurchases}, + {Action: ActionView, Resource: ResourceHistory}, + }, + } + } + + t.Run("update is refused", func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + + mockStore.On("GetGroup", ctx, DefaultPurchaserGroupID).Return(seeded(), nil) + + // The exact #1629 payload: approve-any/retry-any dropped, view + // widened to the wildcard. + result, err := svc.UpdateGroupAPI(ctx, ceilingActorID, DefaultPurchaserGroupID, + APIUpdateGroupRequest{ + Description: "cosmetic edit", + Permissions: []APIPermission{ + {Action: ActionExecute, Resource: ResourcePurchases}, + {Action: ActionView, Resource: ResourceAll}, + }, + }) + + require.Error(t, err) + assert.Nil(t, result) + assert.ErrorIs(t, err, ErrSystemManagedGroup) + assert.Contains(t, err.Error(), GroupPurchaser) + mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) + }) + + t.Run("delete is refused", func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + + mockStore.On("GetGroup", ctx, DefaultPurchaserGroupID).Return(seeded(), nil) + + err := svc.DeleteGroup(ctx, DefaultPurchaserGroupID) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrSystemManagedGroup) + mockStore.AssertNotCalled(t, "DeleteGroup", mock.Anything, mock.Anything) + }) + + t.Run("an ordinary group is still deletable", func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := newCeilingService(t, mockStore) + + stubTargetGroup(ctx, mockStore, &Group{ID: ceilingTargetID, Name: "Team"}) + mockStore.On("DeleteGroup", ctx, ceilingTargetID).Return(nil).Once() + + require.NoError(t, svc.DeleteGroup(ctx, ceilingTargetID)) + }) +} diff --git a/internal/auth/service_api.go b/internal/auth/service_api.go index 4cd904994..f9b2ac6d8 100644 --- a/internal/auth/service_api.go +++ b/internal/auth/service_api.go @@ -268,15 +268,34 @@ func (s *Service) ChangePasswordAPI(ctx context.Context, userID, currentPassword return s.ChangePassword(ctx, userID, req) } -// CreateGroupAPI creates a new group via the API. -func (s *Service) CreateGroupAPI(ctx context.Context, reqInterface any) (any, error) { +// apiPermissionsToPermissions converts a request-side permission list. A nil +// result for an empty input preserves the "not sent" signal APIUpdateGroupRequest +// relies on. +func apiPermissionsToPermissions(in []APIPermission) []Permission { + if len(in) == 0 { + return nil + } + out := make([]Permission, len(in)) + for i, p := range in { + out[i] = apiPermissionToPermission(p) + } + return out +} + +// CreateGroupAPI creates a new group via the API. actorUserID is the acting +// principal (a user ID, or AdminAPIKeyActorID for the stateless admin API +// key); the requested permissions are checked against that principal's own +// effective set before anything is written (issue #1550). +func (s *Service) CreateGroupAPI(ctx context.Context, actorUserID string, reqInterface any) (any, error) { req, ok := reqInterface.(APICreateGroupRequest) if !ok { return nil, fmt.Errorf("invalid request type") } - perms := make([]Permission, len(req.Permissions)) - for i, p := range req.Permissions { - perms[i] = apiPermissionToPermission(p) + perms := apiPermissionsToPermissions(req.Permissions) + // A new group has no permissions yet, so nothing can be "carried over": + // pass a nil existing set, which makes every carved-out verb a grant. + if err := s.checkGrantCeiling(ctx, actorUserID, perms, nil); err != nil { + return nil, err } group := &Group{ Name: req.Name, @@ -284,15 +303,39 @@ func (s *Service) CreateGroupAPI(ctx context.Context, reqInterface any) (any, er Permissions: perms, AllowedAccounts: req.AllowedAccounts, } - // Use empty string for createdBy since we don't have user context here + // Use empty string for createdBy: the column is a UUID FK and + // actorUserID may be the non-UUID admin-API-key sentinel. if err := s.CreateGroup(ctx, group, ""); err != nil { return nil, err } return groupToAPIGroup(group), nil } -// UpdateGroupAPI updates a group via the API. -func (s *Service) UpdateGroupAPI(ctx context.Context, groupID string, reqInterface any) (any, error) { +// applyUpdateGroupRequest applies the set fields of req to group. perms is the +// already-converted permission list; empty means "not sent" and leaves the +// group's permissions unchanged, which is APIUpdateGroupRequest's pre-existing +// contract. +func applyUpdateGroupRequest(group *Group, req APIUpdateGroupRequest, perms []Permission) { + if req.Name != "" { + group.Name = req.Name + } + if req.Description != "" { + group.Description = req.Description + } + if len(perms) > 0 { + group.Permissions = perms + } + if req.AllowedAccounts != nil { + group.AllowedAccounts = req.AllowedAccounts + } +} + +// UpdateGroupAPI updates a group via the API. It refuses to touch a +// system-managed group at all, and checks any requested permission list +// against the acting principal's own effective permissions before writing +// (issues #1550, #1629). actorUserID is a user ID, or AdminAPIKeyActorID for +// the stateless admin API key. +func (s *Service) UpdateGroupAPI(ctx context.Context, actorUserID, groupID string, reqInterface any) (any, error) { req, ok := reqInterface.(APIUpdateGroupRequest) if !ok { return nil, fmt.Errorf("invalid request type") @@ -304,24 +347,17 @@ func (s *Service) UpdateGroupAPI(ctx context.Context, groupID string, reqInterfa if group == nil { return nil, fmt.Errorf("group not found") } - - if req.Name != "" { - group.Name = req.Name - } - if req.Description != "" { - group.Description = req.Description - } - if len(req.Permissions) > 0 { - perms := make([]Permission, len(req.Permissions)) - for i, p := range req.Permissions { - perms[i] = apiPermissionToPermission(p) - } - group.Permissions = perms + if group.SystemManaged { + return nil, fmt.Errorf("%w: %q is seeded and maintained by migrations", ErrSystemManagedGroup, group.Name) } - if req.AllowedAccounts != nil { - group.AllowedAccounts = req.AllowedAccounts + + perms := apiPermissionsToPermissions(req.Permissions) + if err := s.checkGrantCeiling(ctx, actorUserID, perms, group.Permissions); err != nil { + return nil, err } + applyUpdateGroupRequest(group, req, perms) + group.UpdatedAt = time.Now() if err := s.UpdateGroup(ctx, group); err != nil { return nil, err diff --git a/internal/auth/service_api_test.go b/internal/auth/service_api_test.go index 4964b9a62..b351547cd 100644 --- a/internal/auth/service_api_test.go +++ b/internal/auth/service_api_test.go @@ -377,7 +377,8 @@ func TestService_CreateGroupAPI(t *testing.T) { }, } - result, err := service.CreateGroupAPI(ctx, req) + // The admin API key holds {admin, *}, which covers view:recommendations. + result, err := service.CreateGroupAPI(ctx, AdminAPIKeyActorID, req) require.NoError(t, err) assert.NotNil(t, result) @@ -395,7 +396,7 @@ func TestService_CreateGroupAPI(t *testing.T) { mockEmail := new(MockEmailSender) service := createTestService(mockStore, mockEmail) - result, err := service.CreateGroupAPI(ctx, "invalid") + result, err := service.CreateGroupAPI(ctx, AdminAPIKeyActorID, "invalid") assert.Error(t, err) assert.Nil(t, result) assert.Contains(t, err.Error(), "invalid request type") @@ -431,7 +432,7 @@ func TestService_UpdateGroupAPI(t *testing.T) { }, } - result, err := service.UpdateGroupAPI(ctx, "group-123", req) + result, err := service.UpdateGroupAPI(ctx, AdminAPIKeyActorID, "group-123", req) require.NoError(t, err) assert.NotNil(t, result) @@ -449,7 +450,7 @@ func TestService_UpdateGroupAPI(t *testing.T) { mockEmail := new(MockEmailSender) service := createTestService(mockStore, mockEmail) - result, err := service.UpdateGroupAPI(ctx, "group-123", "invalid") + result, err := service.UpdateGroupAPI(ctx, AdminAPIKeyActorID, "group-123", "invalid") assert.Error(t, err) assert.Nil(t, result) assert.Contains(t, err.Error(), "invalid request type") @@ -466,7 +467,7 @@ func TestService_UpdateGroupAPI(t *testing.T) { Name: "New Name", } - result, err := service.UpdateGroupAPI(ctx, "group-123", req) + result, err := service.UpdateGroupAPI(ctx, AdminAPIKeyActorID, "group-123", req) assert.Error(t, err) assert.Nil(t, result) assert.Contains(t, err.Error(), "group not found") diff --git a/internal/auth/service_group.go b/internal/auth/service_group.go index 6dc8ee347..590cc21d4 100644 --- a/internal/auth/service_group.go +++ b/internal/auth/service_group.go @@ -32,7 +32,24 @@ func (s *Service) UpdateGroup(ctx context.Context, group *Group) error { } // DeleteGroup removes a permission group. +// +// Deleting a system-managed group is refused: it is the same defect class as +// the #1629 permission wipe, reached through a different verb. Dropping the +// seeded Purchaser group destroys the only holder of the money verbs carved +// out of admin:*, which no admin can then re-grant (see checkGrantCeiling), +// so the purchase path would be dead tenant-wide until someone re-ran the +// migration by hand. func (s *Service) DeleteGroup(ctx context.Context, groupID string) error { + group, err := s.store.GetGroup(ctx, groupID) + if err != nil && !errors.Is(err, pgx.ErrNoRows) { + return fmt.Errorf("failed to load group %s: %w", groupID, err) + } + if group == nil { + return fmt.Errorf("group not found: %s", groupID) + } + if group.SystemManaged { + return fmt.Errorf("%w: %q is seeded and maintained by migrations", ErrSystemManagedGroup, group.Name) + } return s.store.DeleteGroup(ctx, groupID) } diff --git a/internal/auth/service_group_test.go b/internal/auth/service_group_test.go index cbc3cf3d2..1fccaf6c7 100644 --- a/internal/auth/service_group_test.go +++ b/internal/auth/service_group_test.go @@ -246,6 +246,10 @@ func TestService_DeleteGroup(t *testing.T) { mockEmail := new(MockEmailSender) service := createTestService(mockStore, mockEmail) + // DeleteGroup loads the group first so it can refuse to drop a + // system-managed one (issue #1629). + mockStore.On("GetGroup", ctx, "group-123"). + Return(&Group{ID: "group-123", Name: "Test Team"}, nil).Once() mockStore.On("DeleteGroup", ctx, "group-123").Return(nil).Once() err := service.DeleteGroup(ctx, "group-123") diff --git a/internal/server/adapter_test.go b/internal/server/adapter_test.go index 2d093c0d2..6f47b9b92 100644 --- a/internal/server/adapter_test.go +++ b/internal/server/adapter_test.go @@ -210,6 +210,8 @@ func TestAuthServiceAdapter_DeleteGroup(t *testing.T) { adapter, mockStore := createMockAuthService(t) ctx := context.Background() + // DeleteGroup loads the group first to refuse system-managed ones (#1629). + mockStore.On("GetGroup", ctx, "group-1").Return(&auth.Group{ID: "group-1", Name: "team"}, nil) mockStore.On("DeleteGroup", ctx, "group-1").Return(nil) err := adapter.DeleteGroup(ctx, "group-1") @@ -347,7 +349,7 @@ func TestAuthServiceAdapter_CreateGroupAPI(t *testing.T) { mockStore.On("CreateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil) - _, err := adapter.CreateGroupAPI(ctx, map[string]interface{}{ + _, err := adapter.CreateGroupAPI(ctx, auth.AdminAPIKeyActorID, map[string]interface{}{ "name": "test-group", "permissions": []string{"read:config"}, }) @@ -364,7 +366,7 @@ func TestAuthServiceAdapter_UpdateGroupAPI(t *testing.T) { }, nil) mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil) - _, err := adapter.UpdateGroupAPI(ctx, "group-1", map[string]interface{}{ + _, err := adapter.UpdateGroupAPI(ctx, auth.AdminAPIKeyActorID, "group-1", map[string]interface{}{ "name": "new-name", }) _ = err diff --git a/internal/server/app.go b/internal/server/app.go index 0a0e4e88e..1180c2437 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -1122,12 +1122,12 @@ func (a *authServiceAdapter) MFARegenerateRecoveryCodesAPI(ctx context.Context, } // Group management methods - delegate to auth service API methods. -func (a *authServiceAdapter) CreateGroupAPI(ctx context.Context, req any) (any, error) { - return a.service.CreateGroupAPI(ctx, req) +func (a *authServiceAdapter) CreateGroupAPI(ctx context.Context, actorUserID string, req any) (any, error) { + return a.service.CreateGroupAPI(ctx, actorUserID, req) } -func (a *authServiceAdapter) UpdateGroupAPI(ctx context.Context, groupID string, req any) (any, error) { - return a.service.UpdateGroupAPI(ctx, groupID, req) +func (a *authServiceAdapter) UpdateGroupAPI(ctx context.Context, actorUserID, groupID string, req any) (any, error) { + return a.service.UpdateGroupAPI(ctx, actorUserID, groupID, req) } func (a *authServiceAdapter) DeleteGroup(ctx context.Context, groupID string) error { From dec34ff05e74994753ac4445789768f08b761589 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 02:20:06 +0200 Subject: [PATCH 2/8] sec(auth): reject blank action or resource in a group permission write A blank resource is not a request for the "*" wildcard, but that is what it became. The group-edit form picks its option with `isDefault = !currentValue && resource === '*'`, so an empty stored resource renders as the selected "All (*)" entry and saves back as view:*. Nothing validated the list on the way in, so the same widening was reachable from any API client with no form involved. The grant ceiling added in the previous commit does NOT close this. Its admin:* branch grants any (action, resource) pair that is not carved out, and ("view", "") is not carved out, so an admin wrote a blank resource straight through. Verified by execution before adding the guard: the write returned nil and reached the store. validateRequestedPermissions runs ahead of the ceiling on both write paths and refuses blank (empty or whitespace-only) actions and resources, naming the offending entry index. It fails before the actor lookup, so the refusal does not depend on who is asking. The two fields fail differently in the form, which is what shows the defect is in the defaulting rather than the parsing: a blank action is silently DROPPED (its index 0 is an empty placeholder) while a blank resource is silently WIDENED (its index 0 is the wildcard). Only the resource side escalates; both are refused, because a silently dropped permission is a different bug rather than an acceptable one. Unknown-but-non-blank values are deliberately still accepted: vocabulary validation is a separate concern and rejecting values this endpoint can already have stored would break edits of existing groups. An explicit "*" stays a legitimate value, gated by the ceiling rather than by this check. Mapped to 400, not 403: malformed input rather than an authorization failure. Refs #1730, #1550, #1629. --- internal/api/handler_groups.go | 3 + internal/auth/errors.go | 9 ++ internal/auth/group_ceiling.go | 42 ++++++++++ internal/auth/group_ceiling_test.go | 125 ++++++++++++++++++++++++++++ 4 files changed, 179 insertions(+) diff --git a/internal/api/handler_groups.go b/internal/api/handler_groups.go index ffc840b11..6a80810f9 100644 --- a/internal/api/handler_groups.go +++ b/internal/api/handler_groups.go @@ -65,6 +65,9 @@ func (h *Handler) createGroup(ctx context.Context, req *events.LambdaFunctionURL // are disjoint. Unrecognized errors pass through for handleRequestError. func mapGroupAuthError(err error) error { switch { + case errors.Is(err, auth.ErrInvalidPermission): + // Malformed input, not an authorization failure. + return NewClientError(400, err.Error()) case errors.Is(err, auth.ErrPermissionCeiling), errors.Is(err, auth.ErrPermissionNotGrantable), errors.Is(err, auth.ErrSystemManagedGroup): diff --git a/internal/auth/errors.go b/internal/auth/errors.go index 0f6326226..c9732cd3f 100644 --- a/internal/auth/errors.go +++ b/internal/auth/errors.go @@ -48,6 +48,15 @@ var ( // (issues #923, #1550). ErrPermissionNotGrantable = errors.New("permission not grantable") + // ErrInvalidPermission is returned when a group write carries a + // permission entry with a blank action or resource. A blank resource is + // malformed input, NOT a request for the "*" wildcard: the group-edit + // form renders an empty stored resource as the selected "All (*)" option + // and saves it back as view:*, and the same widening is reachable + // directly through the API because nothing validated the list. Mapped to + // 400 (issues #1730, #1550). + ErrInvalidPermission = errors.New("invalid permission") + // ErrSystemManagedGroup is returned when a write targets one of the // seeded, system-managed groups. Their contents are owned by migrations; // an API edit that reshapes them can silently disable a whole capability diff --git a/internal/auth/group_ceiling.go b/internal/auth/group_ceiling.go index bcddb3171..563f3dba3 100644 --- a/internal/auth/group_ceiling.go +++ b/internal/auth/group_ceiling.go @@ -74,6 +74,9 @@ func (s *Service) checkGrantCeiling(ctx context.Context, actorUserID string, req if len(requested) == 0 { return nil } + if err := validateRequestedPermissions(requested); err != nil { + return err + } actorPerms, err := s.grantCeilingPermissions(ctx, actorUserID) if err != nil { return err @@ -96,6 +99,45 @@ func (s *Service) checkGrantCeiling(ctx context.Context, actorUserID string, req return nil } +// validateRequestedPermissions rejects malformed entries before the ceiling +// runs (issue #1730). +// +// This is NOT redundant with the ceiling. The ceiling's admin:* branch grants +// any (action, resource) pair that is not carved out, and ("view", "") is not +// carved out, so before this check an admin could write a blank resource +// straight through. It then round-trips as the "*" WILDCARD: the group-edit +// form picks its `