From cf49ff71e2c57fd16c046f6cd973d08ce4a86c0a Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 09:27:21 +0000 Subject: [PATCH 1/3] fix(auth): apply the view-only group-combination rule when a group's permissions change #1205 refuses a view-only plus write-capable group combination on user create and update, but UpdateGroup could still produce one with no user write: adding a write permission to a group whose members also belong to a view-only group (or making a group view-only while its members hold a write-capable one) silently contradicted every such member. UpdateGroupAPI now builds the updated group, and when its role in the rule changes (view-only, write-capable or neutral) lists the users and checks each member's combination before and after. If any member who was consistent would become contradictory, the update is refused with ErrContradictoryGroups naming the group and how many members conflict; PUT /api/groups/{id} maps it to 400. Members who were already contradictory do not block it, matching the unchanged-membership rule on user edits. An update that keeps the role, including any update that omits or repeats the permissions, takes the fast path and reads no user. The check runs after the grant and account ceilings, so a 403 still wins over the 400. Like the #1205 check it is an accident guard, not a security boundary. checkGroupCombination now shares its classification with the new check through a group loader, which the new check caches per call. Co-Authored-By: Claude Opus 5.5 --- internal/api/group_explicit_zero_cap_test.go | 3 + internal/auth/group_ceiling_fixtures_test.go | 3 + internal/auth/group_combination.go | 167 +++++++++++++++++-- internal/auth/service_api.go | 16 +- internal/auth/service_api_test.go | 3 + 5 files changed, 177 insertions(+), 15 deletions(-) diff --git a/internal/api/group_explicit_zero_cap_test.go b/internal/api/group_explicit_zero_cap_test.go index 7b66a0a94..40b54beec 100644 --- a/internal/api/group_explicit_zero_cap_test.go +++ b/internal/api/group_explicit_zero_cap_test.go @@ -59,6 +59,9 @@ func TestGroupWritesHTTP_ExplicitZeroSpendCap(t *testing.T) { store.On("GetGroup", mock.Anything, zeroCapGroupID).Return(&auth.Group{ID: zeroCapGroupID, Name: "Team"}, nil).Maybe() store.On("CreateGroup", mock.Anything, mock.Anything).Return(nil).Maybe() store.On("UpdateGroup", mock.Anything, mock.Anything).Return(nil).Maybe() + // The PUT gives the empty group its first permission, so the + // issue #1409 member check lists users; the group has none. + store.On("ListUsers", mock.Anything).Return([]auth.User{}, nil).Maybe() mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", mock.Anything, "admin-token").Return(&Session{UserID: auth.AdminAPIKeyActorID}, nil) mockAuth.grantAdmin() diff --git a/internal/auth/group_ceiling_fixtures_test.go b/internal/auth/group_ceiling_fixtures_test.go index 75b1a15c4..2e63afbae 100644 --- a/internal/auth/group_ceiling_fixtures_test.go +++ b/internal/auth/group_ceiling_fixtures_test.go @@ -42,8 +42,11 @@ func stubActorPermissionsMaybe(ctx context.Context, mockStore *MockStore, perms Return(&Group{ID: ceilingActorGroupID, Name: "Actor Group", Permissions: perms}, nil).Maybe() } +// stubTargetGroup also lets the issue #1409 member check list users: the +// target has no members here, so a permission change never conflicts. func stubTargetGroup(ctx context.Context, mockStore *MockStore, group *Group) { mockStore.On("GetGroup", ctx, ceilingTargetID).Return(group, nil) + mockStore.On("ListUsers", ctx).Return([]User{}, nil).Maybe() } func newCeilingService(t *testing.T, mockStore *MockStore) *Service { diff --git a/internal/auth/group_combination.go b/internal/auth/group_combination.go index cff7c4c2d..3d42bbbbb 100644 --- a/internal/auth/group_combination.go +++ b/internal/auth/group_combination.go @@ -73,19 +73,46 @@ func (s *Service) checkGroupMembershipChange(ctx context.Context, prior, next [] // contains both a view-only group and a write-capable group. It fails closed // on a store error other than "not found". func (s *Service) checkGroupCombination(ctx context.Context, groupIDs []string) error { + viewOnly, writeCapable, err := splitGroupCombination(ctx, groupIDs, s.combinationGroup) + if err != nil { + return err + } + if len(viewOnly) == 0 || len(writeCapable) == 0 { + return nil + } + return contradictoryGroupsError(viewOnly, writeCapable) +} + +// groupLoader loads one group for the combination rule. It returns nil, nil +// for a group that does not exist. +type groupLoader func(ctx context.Context, id string) (*Group, error) + +// combinationGroup is the store-backed groupLoader: a missing group is nil, +// any other store error fails closed. +func (s *Service) combinationGroup(ctx context.Context, id string) (*Group, error) { + group, err := s.store.GetGroup(ctx, id) + if errors.Is(err, pgx.ErrNoRows) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("failed to load group %s: %w", id, err) + } + return group, nil +} + +// splitGroupCombination returns the names of the view-only and the +// write-capable groups among groupIDs. Fewer than two distinct groups cannot +// contradict, so it loads nothing for them. +func splitGroupCombination(ctx context.Context, groupIDs []string, load groupLoader) (viewOnly, writeCapable []string, err error) { ids := uniqueGroupIDs(groupIDs) if len(ids) < 2 { // A contradiction needs two different groups. - return nil + return nil, nil, nil } - var viewOnly, writeCapable []string for _, id := range ids { - group, err := s.store.GetGroup(ctx, id) - if errors.Is(err, pgx.ErrNoRows) { - continue - } - if err != nil { - return fmt.Errorf("failed to load group %s: %w", id, err) + group, loadErr := load(ctx, id) + if loadErr != nil { + return nil, nil, loadErr } if group == nil { continue @@ -97,10 +124,130 @@ func (s *Service) checkGroupCombination(ctx context.Context, groupIDs []string) writeCapable = append(writeCapable, group.Name) } } - if len(viewOnly) == 0 || len(writeCapable) == 0 { + return viewOnly, writeCapable, nil +} + +// groupsContradict reports whether groupIDs hold both a view-only and a +// write-capable group. +func groupsContradict(ctx context.Context, groupIDs []string, load groupLoader) (bool, error) { + viewOnly, writeCapable, err := splitGroupCombination(ctx, groupIDs, load) + return len(viewOnly) > 0 && len(writeCapable) > 0, err +} + +// combinationRole is the part a group plays in the combination rule. +type combinationRole int + +const ( + roleNeutral combinationRole = iota + roleViewOnly + roleWriteCapable +) + +func groupCombinationRole(g *Group) combinationRole { + switch { + case groupIsViewOnly(g): + return roleViewOnly + case groupIsWriteCapable(g): + return roleWriteCapable + } + return roleNeutral +} + +// checkGroupPermissionChange applies the combination rule to a group update +// (issue #1409). Changing a group's permissions changes the combination of +// every member, with no user write: adding a write permission to a group +// whose members also hold a view-only group contradicts each of them, and so +// does making a group view-only while its members also hold a write-capable +// one. +// +// Only the group's role in the rule matters, so an update that keeps it +// view-only, write-capable or neutral returns at once without reading any +// user. Otherwise the update is refused when it would make at least one +// member contradictory who was not before. A member who was already +// contradictory (assigned before #1205, or by an earlier group edit) does not +// block it, as an unchanged membership does not block a user edit. +// +// Like checkGroupCombination it is an accident guard, not a security +// boundary, and it fails closed on a store error. +func (s *Service) checkGroupPermissionChange(ctx context.Context, prior, next *Group) error { + if groupCombinationRole(prior) == groupCombinationRole(next) { return nil } - return contradictoryGroupsError(viewOnly, writeCapable) + users, err := s.store.ListUsers(ctx) + if err != nil { + return fmt.Errorf("failed to list users for the group combination check: %w", err) + } + stored := cachedGroupLoader(s.combinationGroup) + updated := func(ctx context.Context, id string) (*Group, error) { + if id == next.ID { + return next, nil + } + return stored(ctx, id) + } + affected, err := countNewlyContradictory(ctx, users, next.ID, stored, updated) + if err != nil { + return err + } + if affected == 0 { + return nil + } + return groupChangeContradictionError(next, affected) +} + +// countNewlyContradictory counts the members of groupID whose groups do not +// contradict when loaded with before but do when loaded with after. +func countNewlyContradictory(ctx context.Context, users []User, groupID string, before, after groupLoader) (int, error) { + affected := 0 + for i := range users { + ids := users[i].GroupIDs + if !containsGroup(ids, groupID) { + continue + } + was, err := groupsContradict(ctx, ids, before) + if err != nil { + return 0, err + } + now, err := groupsContradict(ctx, ids, after) + if err != nil { + return 0, err + } + if now && !was { + affected++ + } + } + return affected, nil +} + +// cachedGroupLoader loads each group at most once per check: every member of +// the updated group shares it, and usually most of their other groups. +func cachedGroupLoader(load groupLoader) groupLoader { + cache := map[string]*Group{} + return func(ctx context.Context, id string) (*Group, error) { + if g, ok := cache[id]; ok { + return g, nil + } + g, err := load(ctx, id) + if err != nil { + return nil, err + } + cache[id] = g + return g, nil + } +} + +// groupChangeContradictionError explains a refused group update in the terms +// of the change: what the group would become and how many members conflict. +func groupChangeContradictionError(next *Group, affected int) error { + members, belong := "1 of its members", "belongs" + if affected > 1 { + members, belong = fmt.Sprintf("%d of its members", affected), "belong" + } + becomes, other := "would become view-only", "write-capable" + if groupCombinationRole(next) == roleWriteCapable { + becomes, other = "would grant write permissions", "view-only" + } + return fmt.Errorf("%w: group %q %s, but %s also %s to a %s group; remove them from one of the two groups first", + ErrContradictoryGroups, next.Name, becomes, members, belong, other) } // contradictoryGroupsError builds the same message the frontend shows, so an diff --git a/internal/auth/service_api.go b/internal/auth/service_api.go index 4bc7c1eb1..967f9483d 100644 --- a/internal/auth/service_api.go +++ b/internal/auth/service_api.go @@ -396,7 +396,9 @@ func updateRequestPermissions(req APIUpdateGroupRequest) ([]Permission, error) { // 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 +// (issues #1550, #1629). A permission change that would give a member both a +// view-only and a write-capable group is refused after those checks (issue +// #1409), so an access refusal (403) wins over the contradiction (400). actorUserID is a user ID, or AdminAPIKeyActorID for // the stateless admin API key. func (s *Service) UpdateGroupAPI(ctx context.Context, actorUserID, groupID string, req APIUpdateGroupRequest) (*APIGroup, error) { group, err := s.GetGroup(ctx, groupID) @@ -421,13 +423,17 @@ func (s *Service) UpdateGroupAPI(ctx context.Context, actorUserID, groupID strin return nil, err } - applyUpdateGroupRequest(group, req, perms) + updated := *group + applyUpdateGroupRequest(&updated, req, perms) + if err := s.checkGroupPermissionChange(ctx, group, &updated); err != nil { + return nil, err + } - group.UpdatedAt = time.Now() - if err := s.UpdateGroup(ctx, group); err != nil { + updated.UpdatedAt = time.Now() + if err := s.UpdateGroup(ctx, &updated); err != nil { return nil, err } - return groupToAPIGroup(group), nil + return groupToAPIGroup(&updated), nil } // GetGroupAPI returns a group by ID via the API. diff --git a/internal/auth/service_api_test.go b/internal/auth/service_api_test.go index 6a118f307..cebf35942 100644 --- a/internal/auth/service_api_test.go +++ b/internal/auth/service_api_test.go @@ -516,6 +516,9 @@ func TestService_UpdateGroupAPI(t *testing.T) { } mockStore.On("GetGroup", ctx, "group-123").Return(existingGroup, nil).Once() + // The group gains its first permission, so the issue #1409 member + // check runs; the group has no members. + mockStore.On("ListUsers", ctx).Return([]User{}, nil).Once() mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Once() req := APIUpdateGroupRequest{ From 0cc3f4271e20c0da8a4e993cc51b02456d2785f5 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 09:27:21 +0000 Subject: [PATCH 2/3] test(auth): cover the group-combination check on group updates Pins the #1409 rule through UpdateGroupAPI: adding a write permission to a group whose members also hold a view-only group is refused with the member count, as is making a group view-only while members hold a write-capable one, and a first permission on an empty group is checked too. A role change with no conflicting member saves, a member who was already contradictory does not block, and an update that keeps the group's role reads no user. Store errors fail closed and are not reported as a contradiction. Co-Authored-By: Claude Opus 5.5 --- internal/auth/group_permission_change_test.go | 188 ++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 internal/auth/group_permission_change_test.go diff --git a/internal/auth/group_permission_change_test.go b/internal/auth/group_permission_change_test.go new file mode 100644 index 000000000..0078fbf5b --- /dev/null +++ b/internal/auth/group_permission_change_test.go @@ -0,0 +1,188 @@ +package auth + +// Issue #1409: the view-only/write-capable combination rule (issue #78) +// applied to a group update. Changing a group's permissions changes the +// combination of every member without any user write. +// +// Every refusal case stubs NO UpdateGroup expectation on the mock store, so a +// removed guard fails loudly (testify panics on an un-stubbed call). + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +const changeTargetID = "t1" + +var ( + viewPerm = APIPermission{Action: ActionView, Resource: ResourcePlans} + writePerm = APIPermission{Action: ActionCreate, Resource: ResourcePlans} +) + +func changeTarget(perms ...Permission) *Group { + return &Group{ID: changeTargetID, Name: "Team", Permissions: perms} +} + +// newChangeService stubs the target group and the combo fixtures on m. users +// nil means ListUsers must not be called at all. The caller creates m so the +// mocks package's assertion guard can resolve its type. +func newChangeService(t *testing.T, m *MockStore, target *Group, users []User) *Service { + t.Helper() + ctx := context.Background() + t.Cleanup(func() { m.AssertExpectations(t) }) + m.On("GetGroup", ctx, changeTargetID).Return(target, nil) + stubComboGroups(ctx, m) + if users != nil { + m.On("ListUsers", ctx).Return(users, nil).Once() + } + return createTestService(m, new(MockEmailSender)) +} + +func update(svc *Service, perms ...APIPermission) (*APIGroup, error) { + return svc.UpdateGroupAPI(context.Background(), AdminAPIKeyActorID, changeTargetID, + APIUpdateGroupRequest{Name: "Team", Permissions: perms}) +} + +func expectSave(m *MockStore) { + m.On("UpdateGroup", mock.Anything, mock.AnythingOfType("*auth.Group")).Return(nil).Once() +} + +func TestUpdateGroupAPI_RefusesWritePermissionOnGroupWithViewOnlyMembers(t *testing.T) { + m := new(MockStore) + svc := newChangeService(t, m, changeTarget(Permission{Action: ActionView, Resource: ResourcePlans}), []User{ + {ID: "u1", GroupIDs: []string{changeTargetID, "v1"}}, + {ID: "u2", GroupIDs: []string{"v2", changeTargetID}}, + {ID: "u3", GroupIDs: []string{changeTargetID}}, // alone: unaffected + {ID: "u4", GroupIDs: []string{changeTargetID, "e1"}}, // empty group is neutral + {ID: "u5", GroupIDs: []string{"v1", "w1"}}, // not a member + }) + + _, err := update(svc, viewPerm, writePerm) + + require.ErrorIs(t, err, ErrContradictoryGroups) + assert.Contains(t, err.Error(), `group "Team" would grant write permissions, but 2 of its members also belong to a view-only group`) + m.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) +} + +func TestUpdateGroupAPI_RefusesViewOnlyGroupWithWriteCapableMembers(t *testing.T) { + m := new(MockStore) + svc := newChangeService(t, m, changeTarget(Permission{Action: ActionCreate, Resource: ResourcePlans}), []User{ + {ID: "u1", GroupIDs: []string{changeTargetID, "m1"}}, // mixed counts as write-capable + {ID: "u2", GroupIDs: []string{changeTargetID, "v1"}}, + }) + + _, err := update(svc, viewPerm) + + require.ErrorIs(t, err, ErrContradictoryGroups) + assert.Contains(t, err.Error(), `group "Team" would become view-only, but 1 of its members also belongs to a write-capable group`) + m.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) +} + +func TestUpdateGroupAPI_FirstPermissionsOnEmptyGroupAreChecked(t *testing.T) { + // A group with no permissions takes no part in the rule; its first + // permissions can still contradict a member. + m := new(MockStore) + svc := newChangeService(t, m, changeTarget(), []User{{ID: "u1", GroupIDs: []string{changeTargetID, "w1"}}}) + + _, err := update(svc, viewPerm) + + require.ErrorIs(t, err, ErrContradictoryGroups) + m.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) +} + +func TestUpdateGroupAPI_RoleChangeWithoutConflictingMembersSucceeds(t *testing.T) { + m := new(MockStore) + svc := newChangeService(t, m, changeTarget(Permission{Action: ActionView, Resource: ResourcePlans}), []User{ + {ID: "u1", GroupIDs: []string{changeTargetID, "w1"}}, + {ID: "u2", GroupIDs: []string{changeTargetID}}, + {ID: "u3", GroupIDs: []string{"v1"}}, + }) + expectSave(m) + + got, err := update(svc, viewPerm, writePerm) + + require.NoError(t, err) + assert.Len(t, got.Permissions, 2) +} + +// A member who was already contradictory (assigned before #1205) does not +// block the update, as an unchanged membership does not block a user edit. +func TestUpdateGroupAPI_AlreadyContradictoryMemberDoesNotBlock(t *testing.T) { + m := new(MockStore) + svc := newChangeService(t, m, changeTarget(Permission{Action: ActionView, Resource: ResourcePlans}), []User{ + {ID: "u1", GroupIDs: []string{changeTargetID, "w1", "v1"}}, + }) + expectSave(m) + + _, err := update(svc, writePerm) + + require.NoError(t, err) +} + +// The fast path: when the group keeps its role in the rule, no member's +// combination can change, so no user is read. newChangeService registers no +// ListUsers, so reading users would panic. +func TestUpdateGroupAPI_SameRoleSkipsMemberCheck(t *testing.T) { + cases := []struct { + name string + target *Group + perms []APIPermission + }{ + {"permissions omitted", changeTarget(Permission{Action: ActionView, Resource: ResourcePlans}), nil}, + {"permissions unchanged", changeTarget(Permission{Action: ActionView, Resource: ResourcePlans}), []APIPermission{viewPerm}}, + {"view-only stays view-only", changeTarget(Permission{Action: ActionView, Resource: ResourcePlans}), + []APIPermission{viewPerm, {Action: ActionView, Resource: ResourceRecommendations}}}, + {"write-capable stays write-capable", changeTarget(Permission{Action: ActionCreate, Resource: ResourcePlans}), + []APIPermission{viewPerm, {Action: ActionUpdate, Resource: ResourcePlans}}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + m := new(MockStore) + svc := newChangeService(t, m, tc.target, nil) + expectSave(m) + + _, err := update(svc, tc.perms...) + + require.NoError(t, err) + m.AssertNotCalled(t, "ListUsers", mock.Anything) + }) + } +} + +// A store failure fails closed: the update is refused, and not as a +// contradiction. +func TestUpdateGroupAPI_MemberCheckStoreErrorFailsClosed(t *testing.T) { + t.Run("listing users fails", func(t *testing.T) { + ctx := context.Background() + m := new(MockStore) + m.On("GetGroup", ctx, changeTargetID).Return(changeTarget(), nil) + m.On("ListUsers", ctx).Return(nil, errors.New("connection reset")) + svc := createTestService(m, new(MockEmailSender)) + + _, err := update(svc, writePerm) + + require.Error(t, err) + assert.NotErrorIs(t, err, ErrContradictoryGroups) + assert.Contains(t, err.Error(), "connection reset") + m.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) + }) + t.Run("loading a member's other group fails", func(t *testing.T) { + ctx := context.Background() + m := new(MockStore) + m.On("GetGroup", ctx, changeTargetID).Return(changeTarget(), nil) + m.On("GetGroup", ctx, "x1").Return(nil, errors.New("connection reset")) + m.On("ListUsers", ctx).Return([]User{{ID: "u1", GroupIDs: []string{changeTargetID, "x1"}}}, nil) + svc := createTestService(m, new(MockEmailSender)) + + _, err := update(svc, writePerm) + + require.Error(t, err) + assert.NotErrorIs(t, err, ErrContradictoryGroups) + m.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything) + }) +} From b0f812629fb28dcdbca9dca554593f92a7fcff91 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 09:27:22 +0000 Subject: [PATCH 3/3] fix(api): answer a contradictory group update with 400 UpdateGroupAPI now refuses a permission change that would give a member both a view-only and a write-capable group (ErrContradictoryGroups). mapGroupAuthError did not know that sentinel, so the refusal surfaced as a 500. Map it to 400 like the other malformed-input group errors, and document the case on PUT /api/groups/{id}. Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_groups.go | 6 ++-- internal/api/handler_groups_ceiling_test.go | 33 +++++++++++++++++++++ internal/api/openapi.yaml | 12 +++++++- 3 files changed, 48 insertions(+), 3 deletions(-) diff --git a/internal/api/handler_groups.go b/internal/api/handler_groups.go index 8be7d4ad5..3626a04bf 100644 --- a/internal/api/handler_groups.go +++ b/internal/api/handler_groups.go @@ -65,8 +65,10 @@ 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), errors.Is(err, auth.ErrEmptyPermissions): - // Malformed input, not an authorization failure. + case errors.Is(err, auth.ErrInvalidPermission), errors.Is(err, auth.ErrEmptyPermissions), + errors.Is(err, auth.ErrContradictoryGroups): + // Malformed input, or an update that would contradict a member's + // group combination (issue #1409); not an authorization failure. return NewClientError(400, err.Error()) case errors.Is(err, auth.ErrPermissionCeiling), errors.Is(err, auth.ErrPermissionNotGrantable), diff --git a/internal/api/handler_groups_ceiling_test.go b/internal/api/handler_groups_ceiling_test.go index 5a88e86cd..30fe85fb2 100644 --- a/internal/api/handler_groups_ceiling_test.go +++ b/internal/api/handler_groups_ceiling_test.go @@ -138,3 +138,36 @@ func TestUpdateGroup_AdminAPIKeyActorIsSentinel(t *testing.T) { require.NoError(t, err) assert.Equal(t, updated, result) } + +// TestUpdateGroup_ContradictoryMembersMaps400 pins issue #1409's wiring: a +// group update the auth service refuses because it would contradict members' +// group combinations is a 400 with the explanation, not a 500. +func TestUpdateGroup_ContradictoryMembersMaps400(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: group %q would grant write permissions, but 2 of its members also belong to a view-only group; remove them from one of the two groups first", + auth.ErrContradictoryGroups, "Team") + 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":"create","resource":"plans"}]}`, + } + + result, err := h.updateGroup(ctx, req, ceilingGroupID) + + require.Error(t, err) + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok, "a contradictory group update must map to a client error, not a 500") + assert.Equal(t, 400, ce.code) + assert.Contains(t, err.Error(), "2 of its members") +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index 51a497744..a434d7ae8 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -3798,7 +3798,17 @@ paths: schema: $ref: '#/components/schemas/Group' '400': - $ref: '#/components/responses/BadRequest' + description: > + Invalid request, or a permission change that would give a member + both a view-only and a write-capable group (for example adding a + write permission to a group whose members also belong to a + view-only group). The message names the group and how many + members conflict. Members who already held such a combination do + not block the update. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' '401': $ref: '#/components/responses/Unauthorized' '403':