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..6a80810f9 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,32 @@ 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.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): + 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 +102,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 +132,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..c9732cd3f 100644 --- a/internal/auth/errors.go +++ b/internal/auth/errors.go @@ -34,6 +34,35 @@ 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") + + // 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 + // 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_account_ceiling_test.go b/internal/auth/group_account_ceiling_test.go new file mode 100644 index 000000000..d80380017 --- /dev/null +++ b/internal/auth/group_account_ceiling_test.go @@ -0,0 +1,443 @@ +package auth + +import ( + "context" + "testing" + + "github.com/jackc/pgx/v5" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Account-scope ceiling on group writes (issue #1738, folded into #1737). +// +// EVERY refusal case here sends allowed_accounts with NO "permissions" key. +// That shape is the whole finding: checkGrantCeiling returns early when no +// permissions are sent, so a test that includes permissions alongside would +// PASS WITH THE BUG PRESENT, because the permission ceiling then runs and +// refuses for an unrelated reason. + +const scopedAccountA = "acct-A" + +// scopedActorService wires an actor holding only update:groups, scoped to +// exactly one cloud account, editing a group scoped to that same account. +func scopedActorService(t *testing.T, ctx context.Context, mockStore *MockStore) *Service { + t.Helper() + mockStore.On("GetUserByID", ctx, ceilingActorID). + Return(&User{ID: ceilingActorID, GroupIDs: []string{ceilingActorGroupID}}, nil) + mockStore.On("GetGroup", ctx, ceilingActorGroupID).Return(&Group{ + ID: ceilingActorGroupID, + Name: "Scoped Group Managers", + Permissions: []Permission{{Action: ActionUpdate, Resource: ResourceGroups}}, + AllowedAccounts: []string{scopedAccountA}, + }, nil) + mockStore.On("GetGroup", ctx, ceilingTargetID).Return(&Group{ + ID: ceilingTargetID, + Name: "Team", + AllowedAccounts: []string{scopedAccountA}, + }, nil) + return createTestService(mockStore, new(MockEmailSender)) +} + +func TestAccountCeiling_RefusesWideningWithNoPermissionsSent(t *testing.T) { + ctx := context.Background() + + widenings := []struct { + name string + accounts []string + wantIn string + }{ + {"to additional accounts", []string{scopedAccountA, "acct-B"}, `"acct-B"`}, + // The two unrestricted spellings. An empty list is NOT a narrowing: + // IsUnrestrictedAccess reads it as "all accounts". + {"to the empty list (unrestricted)", []string{}, "unrestricted"}, + {"to the wildcard", []string{"*"}, "unrestricted"}, + } + + for _, w := range widenings { + t.Run(w.name, func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := scopedActorService(t, ctx, mockStore) + + // NOTE: no Permissions field. This is the request shape the bug + // lives in; adding permissions here would mask it entirely. + result, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, + APIUpdateGroupRequest{AllowedAccounts: w.accounts}) + + require.Error(t, err) + assert.Nil(t, result) + assert.ErrorIs(t, err, ErrPermissionCeiling) + assert.Contains(t, err.Error(), w.wantIn) + mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) + }) + } +} + +// Negative control 1: a write that WIDENS the group but stays inside the +// actor's own scope succeeds. +// +// This deliberately widens rather than repeating the group's current scope. A +// same-value write returns on the not-a-widening branch without ever +// resolving the actor, so it would prove only that the early return works -- +// not that the actor-scope branch actually grants anything. +func TestAccountCeiling_AllowsInScopeWidening(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := createTestService(mockStore, new(MockEmailSender)) + + // Actor may reach A and B; the group currently reaches only A. + mockStore.On("GetUserByID", ctx, ceilingActorID). + Return(&User{ID: ceilingActorID, GroupIDs: []string{ceilingActorGroupID}}, nil) + mockStore.On("GetGroup", ctx, ceilingActorGroupID).Return(&Group{ + ID: ceilingActorGroupID, + Permissions: []Permission{{Action: ActionUpdate, Resource: ResourceGroups}}, + AllowedAccounts: []string{scopedAccountA, "acct-B"}, + }, nil) + mockStore.On("GetGroup", ctx, ceilingTargetID).Return(&Group{ + ID: ceilingTargetID, Name: "Team", AllowedAccounts: []string{scopedAccountA}, + }, nil) + + var saved *Group + mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")). + Run(func(a mock.Arguments) { + g, ok := a.Get(1).(*Group) + require.True(t, ok) + saved = g + }).Return(nil).Once() + + _, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, + APIUpdateGroupRequest{AllowedAccounts: []string{scopedAccountA, "acct-B"}}) + + require.NoError(t, err) + require.NotNil(t, saved) + assert.Equal(t, []string{scopedAccountA, "acct-B"}, saved.AllowedAccounts) +} + +// Negative control 1b: repeating the group's current scope is not a widening +// and must not require an authorization round-trip at all. +func TestAccountCeiling_UnchangedScopeSkipsActorLookup(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := createTestService(mockStore, new(MockEmailSender)) + + mockStore.On("GetGroup", ctx, ceilingTargetID).Return(&Group{ + ID: ceilingTargetID, Name: "Team", AllowedAccounts: []string{scopedAccountA}, + }, nil) + mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Once() + + _, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, + APIUpdateGroupRequest{AllowedAccounts: []string{scopedAccountA}}) + + require.NoError(t, err) + mockStore.AssertNotCalled(t, "GetUserByID", mock.Anything, mock.Anything) +} + +// Negative control 2: an UNRESTRICTED actor may still widen a group, so the +// refusals above come from the actor's scope and not from the check refusing +// all widenings outright. +func TestAccountCeiling_UnrestrictedActorMayWiden(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := createTestService(mockStore, new(MockEmailSender)) + + stubActorPermissions(ctx, mockStore, adminOnly) // no AllowedAccounts -> unrestricted + stubTargetGroup(ctx, mockStore, &Group{ + ID: ceilingTargetID, Name: "Team", AllowedAccounts: []string{scopedAccountA}, + }) + mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Once() + + _, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, + APIUpdateGroupRequest{AllowedAccounts: []string{"*"}}) + require.NoError(t, err) +} + +// Negative control 3: omitting allowed_accounts entirely (nil, "not sent") +// must not be treated as a write, or every name-only edit would be refused. +func TestAccountCeiling_NotSentIsNotAWrite(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := createTestService(mockStore, new(MockEmailSender)) + + // Only the target group is stubbed. Neither ceiling resolves the actor + // for a request that sends nothing they gate, and asserting that here + // (via AssertExpectations on an actor-free mock) is the point: a + // name-only edit must not become an authorization round-trip. + mockStore.On("GetGroup", ctx, ceilingTargetID).Return(&Group{ + ID: ceilingTargetID, Name: "Team", AllowedAccounts: []string{scopedAccountA}, + }, nil) + + var saved *Group + mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")). + Run(func(a mock.Arguments) { + g, ok := a.Get(1).(*Group) + require.True(t, ok) + saved = g + }).Return(nil).Once() + + _, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, + APIUpdateGroupRequest{Name: "Renamed"}) + + require.NoError(t, err) + require.NotNil(t, saved) + assert.Equal(t, "Renamed", saved.Name) + assert.Equal(t, []string{scopedAccountA}, saved.AllowedAccounts, "scope must be untouched") +} + +// Narrowing is always allowed, whoever the actor is: it cannot widen anything. +func TestAccountCeiling_NarrowingIsAllowed(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := createTestService(mockStore, new(MockEmailSender)) + + mockStore.On("GetUserByID", ctx, ceilingActorID). + Return(&User{ID: ceilingActorID, GroupIDs: []string{ceilingActorGroupID}}, nil).Maybe() + mockStore.On("GetGroup", ctx, ceilingActorGroupID).Return(&Group{ + ID: ceilingActorGroupID, + Permissions: []Permission{{Action: ActionUpdate, Resource: ResourceGroups}}, + AllowedAccounts: []string{"acct-Z"}, + }, nil).Maybe() + mockStore.On("GetGroup", ctx, ceilingTargetID).Return(&Group{ + ID: ceilingTargetID, Name: "Team", AllowedAccounts: []string{scopedAccountA, "acct-B"}, + }, nil) + mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Once() + + _, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, + APIUpdateGroupRequest{AllowedAccounts: []string{scopedAccountA}}) + require.NoError(t, err, "narrowing the group's own scope is never a widening") +} + +// CreateGroupAPI has no existing scope to compare against, so any +// allowed_accounts value must sit inside the creator's own. +func TestAccountCeiling_CreateIsBounded(t *testing.T) { + ctx := context.Background() + + t.Run("out of scope is refused", func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := createTestService(mockStore, new(MockEmailSender)) + + mockStore.On("GetUserByID", ctx, ceilingActorID). + Return(&User{ID: ceilingActorID, GroupIDs: []string{ceilingActorGroupID}}, nil) + mockStore.On("GetGroup", ctx, ceilingActorGroupID).Return(&Group{ + ID: ceilingActorGroupID, + Permissions: []Permission{{Action: ActionCreate, Resource: ResourceGroups}}, + AllowedAccounts: []string{scopedAccountA}, + }, nil) + + _, err := svc.CreateGroupAPI(ctx, ceilingActorID, APICreateGroupRequest{ + Name: "Wider", + AllowedAccounts: []string{"acct-B"}, + }) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrPermissionCeiling) + mockStore.AssertNotCalled(t, "CreateGroup", mock.Anything, mock.Anything) + }) + + t.Run("in scope is allowed", func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := createTestService(mockStore, new(MockEmailSender)) + + mockStore.On("GetUserByID", ctx, ceilingActorID). + Return(&User{ID: ceilingActorID, GroupIDs: []string{ceilingActorGroupID}}, nil) + mockStore.On("GetGroup", ctx, ceilingActorGroupID).Return(&Group{ + ID: ceilingActorGroupID, + Permissions: []Permission{{Action: ActionCreate, Resource: ResourceGroups}}, + AllowedAccounts: []string{scopedAccountA}, + }, nil) + mockStore.On("CreateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Once() + + _, err := svc.CreateGroupAPI(ctx, ceilingActorID, APICreateGroupRequest{ + Name: "Narrower", + AllowedAccounts: []string{scopedAccountA}, + }) + require.NoError(t, err) + }) +} + +// Fail closed: an actor whose scope cannot be resolved cannot widen. +func TestAccountCeiling_FailsClosedOnUnidentifiedActor(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := createTestService(mockStore, new(MockEmailSender)) + + stubTargetGroup(ctx, mockStore, &Group{ + ID: ceilingTargetID, Name: "Team", AllowedAccounts: []string{scopedAccountA}, + }) + + _, err := svc.UpdateGroupAPI(ctx, "", ceilingTargetID, + APIUpdateGroupRequest{AllowedAccounts: []string{"*"}}) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrPermissionCeiling) + mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) +} + +// An actor whose groups do not resolve has an UNKNOWN scope, not an +// unrestricted one. collectGroupsAndAccounts skips a missing or deleted group +// silently, so before this guard a total resolution failure produced an empty +// list -- read as "all accounts" -- and the ceiling became a no-op on exactly +// the path it guards. +// +// The permission ceiling already failed closed on the same input (an empty +// permission set grants nothing); this closes the asymmetry. +func TestAccountCeiling_FailsClosedWhenActorScopeUnresolvable(t *testing.T) { + ctx := context.Background() + + for _, tc := range []struct { + name string + groupResp func(*MockStore) + }{ + {"actor's only group is missing", func(ms *MockStore) { + ms.On("GetGroup", ctx, ceilingActorGroupID).Return(nil, pgx.ErrNoRows) + }}, + {"actor's only group resolves to nil", func(ms *MockStore) { + ms.On("GetGroup", ctx, ceilingActorGroupID).Return(nil, nil) + }}, + } { + t.Run(tc.name, func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := createTestService(mockStore, new(MockEmailSender)) + + mockStore.On("GetUserByID", ctx, ceilingActorID). + Return(&User{ID: ceilingActorID, GroupIDs: []string{ceilingActorGroupID}}, nil) + tc.groupResp(mockStore) + mockStore.On("GetGroup", ctx, ceilingTargetID).Return(&Group{ + ID: ceilingTargetID, Name: "Team", AllowedAccounts: []string{scopedAccountA}, + }, nil) + + _, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, + APIUpdateGroupRequest{AllowedAccounts: []string{"*"}}) + + require.Error(t, err, "an unresolvable scope must refuse the widening, not permit it") + assert.ErrorIs(t, err, ErrPermissionCeiling) + // Total failure is the degenerate case of a partial one -- every + // group skipped -- so it is caught by the skipped-group guard and + // carries that message. Assert the sentinel plus the shared + // substring rather than one guard's exact wording. + assert.Contains(t, err.Error(), "could not be resolved") + mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) + }) + } +} + +// A PARTIAL group resolution can WIDEN the actor's own scope, letting them +// grant what their real configuration forbids (#1737 A1, same defect as the +// read path in #1752). +// +// Needs TWO groups: one granting update:groups with no allowed_accounts +// (unrestricted alone), one carrying the restriction. The union is +// restricted; lose the restricting group and it collapses to empty = every +// account, and the ceiling then permits widening a group to ["*"]. +func TestAccountCeiling_PartialActorResolutionThatWidensIsRefused(t *testing.T) { + ctx := context.Background() + const permGroup, scopeGroup = "g-perm", "g-scope" + + for _, tc := range []struct { + name string + scopeResp func(*MockStore) + }{ + {"restricting group missing (ErrNoRows)", func(ms *MockStore) { + ms.On("GetGroup", ctx, scopeGroup).Return(nil, pgx.ErrNoRows) + }}, + {"restricting group resolves to (nil, nil)", func(ms *MockStore) { + ms.On("GetGroup", ctx, scopeGroup).Return(nil, nil) + }}, + } { + t.Run(tc.name, func(t *testing.T) { + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := createTestService(mockStore, new(MockEmailSender)) + + mockStore.On("GetUserByID", ctx, ceilingActorID). + Return(&User{ID: ceilingActorID, GroupIDs: []string{permGroup, scopeGroup}}, nil) + mockStore.On("GetGroup", ctx, permGroup).Return(&Group{ + ID: permGroup, + Permissions: []Permission{{Action: ActionUpdate, Resource: ResourceGroups}}, + }, nil) + tc.scopeResp(mockStore) + mockStore.On("GetGroup", ctx, ceilingTargetID).Return(&Group{ + ID: ceilingTargetID, Name: "Team", AllowedAccounts: []string{scopedAccountA}, + }, nil) + + _, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, + APIUpdateGroupRequest{AllowedAccounts: []string{"*"}}) + + require.Error(t, err, "a widened actor scope must not authorize widening a group") + assert.ErrorIs(t, err, ErrPermissionCeiling) + mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) + }) + } +} + +// Baseline control: both groups resolving, the same actor is correctly +// refused for a DIFFERENT reason (their real scope does not cover "*"), which +// is what makes the partial case a genuine bypass rather than a no-op. +func TestAccountCeiling_PartialActorBaselineIsRefusedOnRealScope(t *testing.T) { + ctx := context.Background() + const permGroup, scopeGroup = "g-perm", "g-scope" + + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := createTestService(mockStore, new(MockEmailSender)) + + mockStore.On("GetUserByID", ctx, ceilingActorID). + Return(&User{ID: ceilingActorID, GroupIDs: []string{permGroup, scopeGroup}}, nil) + mockStore.On("GetGroup", ctx, permGroup).Return(&Group{ + ID: permGroup, Permissions: []Permission{{Action: ActionUpdate, Resource: ResourceGroups}}, + }, nil) + mockStore.On("GetGroup", ctx, scopeGroup).Return(&Group{ + ID: scopeGroup, AllowedAccounts: []string{scopedAccountA}, + }, nil) + mockStore.On("GetGroup", ctx, ceilingTargetID).Return(&Group{ + ID: ceilingTargetID, Name: "Team", AllowedAccounts: []string{scopedAccountA}, + }, nil) + + _, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, + APIUpdateGroupRequest{AllowedAccounts: []string{"*"}}) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrPermissionCeiling) +} + +// An actor whose surviving union carries "*" was already maximally wide, so a +// lost group cannot widen them. Refusing them would be pure availability cost +// -- and all seven seeded groups ship allowed_accounts = ARRAY['*']. +func TestAccountCeiling_WildcardActorToleratesSkippedGroup(t *testing.T) { + ctx := context.Background() + const seededGroup, scopeGroup = "g-seeded", "g-scope" + + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := createTestService(mockStore, new(MockEmailSender)) + + mockStore.On("GetUserByID", ctx, ceilingActorID). + Return(&User{ID: ceilingActorID, GroupIDs: []string{seededGroup, scopeGroup}}, nil) + mockStore.On("GetGroup", ctx, seededGroup).Return(&Group{ + ID: seededGroup, + Permissions: []Permission{{Action: ActionUpdate, Resource: ResourceGroups}}, + AllowedAccounts: []string{"*"}, + }, nil) + mockStore.On("GetGroup", ctx, scopeGroup).Return(nil, pgx.ErrNoRows) + mockStore.On("GetGroup", ctx, ceilingTargetID).Return(&Group{ + ID: ceilingTargetID, Name: "Team", AllowedAccounts: []string{scopedAccountA}, + }, nil) + mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Once() + + _, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID, + APIUpdateGroupRequest{AllowedAccounts: []string{"*"}}) + + require.NoError(t, err, + "an actor already unrestricted at baseline must not be refused for a lost group") +} diff --git a/internal/auth/group_ceiling.go b/internal/auth/group_ceiling.go new file mode 100644 index 000000000..c89729514 --- /dev/null +++ b/internal/auth/group_ceiling.go @@ -0,0 +1,390 @@ +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 + } + if err := validateRequestedPermissions(requested); err != nil { + return err + } + 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 +} + +// 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 `