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/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': 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/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) + }) +} 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{