diff --git a/internal/api/handler_users.go b/internal/api/handler_users.go index 746fdbac..a2bb62f2 100644 --- a/internal/api/handler_users.go +++ b/internal/api/handler_users.go @@ -85,6 +85,7 @@ func mapAuthError(err error) error { switch { case errors.Is(err, auth.ErrInvalidEmail), errors.Is(err, auth.ErrNoGroups), + errors.Is(err, auth.ErrContradictoryGroups), errors.Is(err, auth.ErrPasswordPolicy): return NewClientError(400, err.Error()) case errors.Is(err, auth.ErrSelfEscalation), diff --git a/internal/api/handler_users_test.go b/internal/api/handler_users_test.go index 231d69e4..819695c0 100644 --- a/internal/api/handler_users_test.go +++ b/internal/api/handler_users_test.go @@ -3,6 +3,7 @@ package api import ( "context" "encoding/base64" + "fmt" "testing" "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" @@ -313,3 +314,46 @@ func TestHandler_updateUser_InvalidJSON(t *testing.T) { assert.Nil(t, result) assert.Contains(t, err.Error(), "invalid request body") } + +// Issue #78: a contradictory view-only + write-capable membership refused by +// the auth service must reach a non-UI API consumer as a 400 carrying the +// explanation, on both the create and the update path, not as a generic 500. +func TestHandler_users_ContradictoryGroupsMapTo400(t *testing.T) { + ctx := context.Background() + adminSession := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} + refusal := fmt.Errorf(`%w: "Viewers" is view-only but "Plan Authors" grants write permissions; assign a view-only group or write-capable groups, not both`, + auth.ErrContradictoryGroups) + groups := `["00000000-0000-5000-8000-000000000004", "00000000-0000-5000-8000-000000000003"]` + headers := map[string]string{"Authorization": "Bearer admin-token"} + + calls := map[string]func(h *Handler, m *MockAuthService) (any, error){ + "create": func(h *Handler, m *MockAuthService) (any, error) { + m.On("CreateUserAPI", ctx, adminSession.UserID, mock.Anything).Return(nil, refusal).Once() + password := base64.StdEncoding.EncodeToString([]byte("password123")) + return h.createUser(ctx, &events.LambdaFunctionURLRequest{Headers: headers, + Body: `{"email": "new@example.com", "password": "` + password + `", "groups": ` + groups + `}`}) + }, + "update": func(h *Handler, m *MockAuthService) (any, error) { + const userID = "11111111-1111-1111-1111-111111111111" + m.On("UpdateUserAPI", ctx, adminSession.UserID, userID, mock.Anything).Return(nil, refusal).Once() + return h.updateUser(ctx, &events.LambdaFunctionURLRequest{Headers: headers, Body: `{"groups": ` + groups + `}`}, userID) + }, + } + for name, call := range calls { + t.Run(name, func(t *testing.T) { + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) + mockAuth.grantAdmin() + + result, err := call(&Handler{auth: mockAuth}, mockAuth) + + require.Error(t, err) + assert.Nil(t, result) + ce, ok := IsClientError(err) + require.True(t, ok, "a contradictory combination must map to a client error, not a 500") + assert.Equal(t, 400, ce.code) + assert.Contains(t, err.Error(), `"Viewers" is view-only`) + }) + } +} diff --git a/internal/auth/errors.go b/internal/auth/errors.go index af275339..1680ae04 100644 --- a/internal/auth/errors.go +++ b/internal/auth/errors.go @@ -34,6 +34,13 @@ var ( // permission. Mapped to 403 (issue #907). ErrSelfEscalation = errors.New("cannot escalate your own group membership") + // ErrContradictoryGroups is returned when a create/update would give a + // user a view-only group together with a write-capable group. The + // permission model is additive, so the write group silently overrides + // the read-only intent of the view-only one. Backend mirror of the + // frontend's validateGroupCombination check. Mapped to 400 (issue #78). + ErrContradictoryGroups = errors.New("contradictory group combination") + // 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 diff --git a/internal/auth/group_combination.go b/internal/auth/group_combination.go new file mode 100644 index 00000000..cff7c4c2 --- /dev/null +++ b/internal/auth/group_combination.go @@ -0,0 +1,144 @@ +package auth + +import ( + "context" + "errors" + "fmt" + "strings" + + "github.com/jackc/pgx/v5" +) + +// Group-combination guard (issue #78). +// +// A view-only group (every permission's action is "view") combined with a +// write-capable group (at least one permission with any other action) is a +// contradiction: permissions are a plain union, so the write group silently +// overrides the read-only intent of the view-only one. The frontend's +// validateGroupCombination (frontend/src/users/utils.ts) has refused this +// since #1405, but nothing stopped a non-UI API consumer. +// +// This is a port of that function, and the classification must stay exactly +// the same: +// +// - It reads each group's stored permissions, never its name. +// - view-only: at least one permission, all with action == "view". +// - write-capable: at least one permission whose action != "view" +// (so "*" and "admin" count, and a mixed view+create group counts). +// - A group with no permissions is neither and does not take part. +// - A group that does not exist (deleted, or never existed) is skipped. +// +// It is an accident guard, not a security boundary: an update:users holder +// could assign the write group alone anyway. + +// groupIsViewOnly reports whether every one of g's permissions is a view, +// with at least one permission present. +func groupIsViewOnly(g *Group) bool { + if len(g.Permissions) == 0 { + return false + } + for _, p := range g.Permissions { + if p.Action != ActionView { + return false + } + } + return true +} + +// groupIsWriteCapable reports whether any of g's permissions is not a view. +func groupIsWriteCapable(g *Group) bool { + for _, p := range g.Permissions { + if p.Action != ActionView { + return true + } + } + return false +} + +// checkGroupMembershipChange runs checkGroupCombination only when the +// membership actually changes. A user who already holds a contradictory +// membership (assigned before this check existed, or made contradictory by a +// later edit to one of the groups) must still be renamed, deactivated or +// saved by a client that echoes back the unchanged group list. Any real change +// is checked against the full resulting set, as the frontend does, so it has +// to resolve the contradiction rather than add to it. +func (s *Service) checkGroupMembershipChange(ctx context.Context, prior, next []string) error { + if sameGroupSet(prior, next) { + return nil + } + return s.checkGroupCombination(ctx, next) +} + +// checkGroupCombination returns an ErrContradictoryGroups error when groupIDs +// 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 { + ids := uniqueGroupIDs(groupIDs) + if len(ids) < 2 { + // A contradiction needs two different groups. + return 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) + } + if group == nil { + continue + } + switch { + case groupIsViewOnly(group): + viewOnly = append(viewOnly, group.Name) + case groupIsWriteCapable(group): + writeCapable = append(writeCapable, group.Name) + } + } + if len(viewOnly) == 0 || len(writeCapable) == 0 { + return nil + } + return contradictoryGroupsError(viewOnly, writeCapable) +} + +// contradictoryGroupsError builds the same message the frontend shows, so an +// API consumer and a UI user see the same explanation. +func contradictoryGroupsError(viewOnly, writeCapable []string) error { + is, grants := "is", "grants" + if len(viewOnly) > 1 { + is = "are" + } + if len(writeCapable) > 1 { + grants = "grant" + } + return fmt.Errorf("%w: %s %s view-only but %s %s write permissions; assign a view-only group or write-capable groups, not both", + ErrContradictoryGroups, quoteNames(viewOnly), is, quoteNames(writeCapable), grants) +} + +func quoteNames(names []string) string { + quoted := make([]string, len(names)) + for i, n := range names { + quoted[i] = fmt.Sprintf("%q", n) + } + return strings.Join(quoted, ", ") +} + +// uniqueGroupIDs returns ids without duplicates, keeping first-seen order so +// the error names groups in the order the caller sent them. +func uniqueGroupIDs(ids []string) []string { + out := make([]string, 0, len(ids)) + for _, id := range ids { + if !containsGroup(out, id) { + out = append(out, id) + } + } + return out +} + +// sameGroupSet reports whether a and b name the same groups, ignoring order +// and duplicates. +func sameGroupSet(a, b []string) bool { + return len(addedGroups(a, b)) == 0 && len(addedGroups(b, a)) == 0 +} diff --git a/internal/auth/group_combination_test.go b/internal/auth/group_combination_test.go new file mode 100644 index 00000000..c0f12dd7 --- /dev/null +++ b/internal/auth/group_combination_test.go @@ -0,0 +1,271 @@ +package auth + +// Backend mirror of the frontend's validateGroupCombination (issue #78). +// +// The classification cases are a one-to-one port of the jest suite in +// frontend/src/__tests__/users.test.ts ("validateGroupCombination (issue +// #1405)") so the two implementations cannot drift without a test noticing. +// +// Every refusal case stubs NO CreateUser / UpdateUserAdminFields expectation +// on the mock store, so a removed guard fails loudly (testify panics on an +// un-stubbed call) instead of passing silently. + +import ( + "context" + "errors" + "testing" + + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Fixtures mirroring the jest suite: v1 view-only, w1 write-capable, m1 mixed +// (view + create, so write-capable), e1 with no permissions (neutral). +var ( + comboViewOnly = &Group{ID: "v1", Name: "Viewers", Permissions: []Permission{{Action: ActionView, Resource: "aws"}}} + comboViewTwo = &Group{ID: "v2", Name: "Readers", Permissions: []Permission{{Action: ActionView, Resource: "gcp"}}} + comboWrite = &Group{ID: "w1", Name: "Editors", Permissions: []Permission{{Action: ActionCreate, Resource: "aws"}}} + comboWriteTwo = &Group{ID: "w2", Name: "Admins", Permissions: []Permission{{Action: ActionDelete, Resource: "aws"}}} + comboMixed = &Group{ID: "m1", Name: "Mixed", Permissions: []Permission{ + {Action: ActionView, Resource: "a"}, {Action: ActionCreate, Resource: "b"}, + }} + comboEmpty = &Group{ID: "e1", Name: "Empty", Permissions: []Permission{}} +) + +// stubComboGroups registers GetGroup for every fixture plus an unknown ID +// that resolves to pgx.ErrNoRows (a deleted / never-existing group). +func stubComboGroups(ctx context.Context, m *MockStore) { + for _, g := range []*Group{comboViewOnly, comboViewTwo, comboWrite, comboWriteTwo, comboMixed, comboEmpty} { + m.On("GetGroup", ctx, g.ID).Return(g, nil).Maybe() + } + m.On("GetGroup", ctx, "unknown-uuid").Return(nil, pgx.ErrNoRows).Maybe() +} + +// stubViewOnlyGroups registers each id as a distinct view-only group, for +// tests that send several groups but are not about their combination. +func stubViewOnlyGroups(ctx context.Context, m *MockStore, ids ...string) { + for _, id := range ids { + m.On("GetGroup", ctx, id).Return(&Group{ + ID: id, Name: id, Permissions: []Permission{{Action: ActionView, Resource: ResourcePlans}}, + }, nil).Once() + } +} + +func TestCheckGroupCombination_MirrorsFrontend(t *testing.T) { + ctx := context.Background() + + cases := []struct { + name string + ids []string + wantErr bool + contains []string + }{ + {name: "empty selection", ids: nil}, + {name: "single view-only group", ids: []string{"v1"}}, + {name: "single write-capable group", ids: []string{"w1"}}, + {name: "empty-permission group alone", ids: []string{"e1"}}, + {name: "view-only + empty-permission group (empty is neutral)", ids: []string{"v1", "e1"}}, + // Not in the jest suite, but follows from the frontend's + // permissions.length > 0 guard: an empty group is not view-only. + {name: "write-capable + empty-permission group (empty is neutral)", ids: []string{"w1", "e1"}}, + {name: "two write-capable groups", ids: []string{"w1", "w2"}}, + {name: "two view-only groups", ids: []string{"v1", "v2"}}, + {name: "unknown group ID", ids: []string{"unknown-uuid"}}, + {name: "unknown group ID next to a view-only group", ids: []string{"v1", "unknown-uuid"}}, + { + name: "view-only + write-capable", ids: []string{"v1", "w1"}, wantErr: true, + contains: []string{"contradictory", `"Viewers"`, `"Editors"`}, + }, + {name: "singular is for one view-only group", ids: []string{"v1", "w1"}, wantErr: true, contains: []string{" is view-only"}}, + {name: "plural are for several view-only groups", ids: []string{"v1", "v2", "w1"}, wantErr: true, contains: []string{" are view-only", `"Readers"`}}, + {name: "singular grants for one write-capable group", ids: []string{"v1", "w1"}, wantErr: true, contains: []string{"grants write"}}, + {name: "plural grant for several write-capable groups", ids: []string{"v1", "w1", "w2"}, wantErr: true, contains: []string{"grant write", `"Admins"`}}, + {name: "mixed group counts as write-capable", ids: []string{"v1", "m1"}, wantErr: true, contains: []string{`"Mixed"`}}, + {name: "order does not matter", ids: []string{"w1", "e1", "v1"}, wantErr: true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + m := new(MockStore) + stubComboGroups(ctx, m) + svc := createTestService(m, new(MockEmailSender)) + + err := svc.checkGroupCombination(ctx, tc.ids) + if !tc.wantErr { + require.NoError(t, err) + return + } + require.Error(t, err) + assert.ErrorIs(t, err, ErrContradictoryGroups) + for _, s := range tc.contains { + assert.Contains(t, err.Error(), s) + } + }) + } +} + +// The classification is read from the groups' stored permissions, not their +// names: a group called "Viewers" that grants a write verb is write-capable, +// and an oddly named view-only group is still view-only. +func TestCheckGroupCombination_UsesPermissionsNotNames(t *testing.T) { + ctx := context.Background() + m := new(MockStore) + svc := createTestService(m, new(MockEmailSender)) + m.On("GetGroup", ctx, "a").Return(&Group{ID: "a", Name: "Viewers", Permissions: []Permission{{Action: ActionUpdate, Resource: "plans"}}}, nil) + m.On("GetGroup", ctx, "b").Return(&Group{ID: "b", Name: "Administrators", Permissions: []Permission{{Action: ActionAdmin, Resource: ResourceAll}}}, nil) + m.On("GetGroup", ctx, "c").Return(&Group{ID: "c", Name: "Power Users", Permissions: []Permission{{Action: ActionView, Resource: ResourceAll}}}, nil) + + require.NoError(t, svc.checkGroupCombination(ctx, []string{"a", "b"}), "two write-capable groups, whatever they are called") + err := svc.checkGroupCombination(ctx, []string{"c", "b"}) + require.ErrorIs(t, err, ErrContradictoryGroups) + assert.Contains(t, err.Error(), `"Power Users" is view-only`) +} + +// A store failure is not "group missing": the check fails closed rather than +// letting an unclassified membership through. +func TestCheckGroupCombination_StoreErrorFailsClosed(t *testing.T) { + ctx := context.Background() + m := new(MockStore) + svc := createTestService(m, new(MockEmailSender)) + m.On("GetGroup", ctx, "v1").Return(comboViewOnly, nil).Maybe() + m.On("GetGroup", ctx, "w1").Return(nil, errors.New("connection reset")) + + err := svc.checkGroupCombination(ctx, []string{"v1", "w1"}) + require.Error(t, err) + assert.NotErrorIs(t, err, ErrContradictoryGroups) + assert.Contains(t, err.Error(), "connection reset") +} + +func TestCreateUser_RejectsContradictoryGroups(t *testing.T) { + ctx := context.Background() + m := new(MockStore) + t.Cleanup(func() { m.AssertExpectations(t) }) + svc := createTestService(m, new(MockEmailSender)) + stubComboGroups(ctx, m) + m.On("GetUserByEmail", ctx, "new@example.com").Return(nil, nil) + + // actorUserID "" is the trusted internal caller: the contradiction check + // is about the membership itself, so it applies to every caller. + _, err := svc.CreateUser(ctx, "", CreateUserRequest{ + Email: "new@example.com", + Password: "Sup3rSecretP@ssw0rd!", + GroupIDs: []string{"v1", "w1"}, + }) + + require.ErrorIs(t, err, ErrContradictoryGroups) + m.AssertNotCalled(t, "CreateUser", mock.Anything, mock.Anything) +} + +func TestCreateUser_AllowsConsistentGroups(t *testing.T) { + ctx := context.Background() + m := new(MockStore) + t.Cleanup(func() { m.AssertExpectations(t) }) + svc := createTestService(m, new(MockEmailSender)) + stubComboGroups(ctx, m) + m.On("GetUserByEmail", ctx, "new@example.com").Return(nil, nil) + m.On("CreateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Once() + + res, err := svc.CreateUser(ctx, "", CreateUserRequest{ + Email: "new@example.com", + Password: "Sup3rSecretP@ssw0rd!", + GroupIDs: []string{"v1", "v2", "e1"}, + }) + + require.NoError(t, err) + assert.Equal(t, []string{"v1", "v2", "e1"}, res.User.GroupIDs) +} + +// updateComboUser runs UpdateUser against a target whose stored membership is +// prior, with the given store expectations already registered by the caller. +func updateComboUser(t *testing.T, m *MockStore, prior []string, req UpdateUserRequest) (*User, error) { + t.Helper() + ctx := context.Background() + m.On("GetUserByID", ctx, "user-1").Return(&User{ + ID: "user-1", Email: "user@example.com", GroupIDs: append([]string(nil), prior...), Active: true, + }, nil).Once() + svc := createTestService(m, new(MockEmailSender)) + return svc.UpdateUser(ctx, "", "user-1", req) +} + +func TestUpdateUser_RejectsChangeToContradictoryGroups(t *testing.T) { + cases := map[string]struct{ prior, next []string }{ + "add a write group to a view-only user": {prior: []string{"v1"}, next: []string{"v1", "w1"}}, + "add a view-only group to a write user": {prior: []string{"w1"}, next: []string{"w1", "v1"}}, + "replace with a contradictory set": {prior: []string{"e1"}, next: []string{"v1", "m1"}}, + "legacy contradictory user, partial change": {prior: []string{"v1", "w1", "w2"}, next: []string{"v1", "w1"}}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + m := new(MockStore) + t.Cleanup(func() { m.AssertExpectations(t) }) + stubComboGroups(ctx, m) + + _, err := updateComboUser(t, m, tc.prior, UpdateUserRequest{GroupIDs: tc.next}) + + require.ErrorIs(t, err, ErrContradictoryGroups) + m.AssertNotCalled(t, "UpdateUserAdminFields", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) + }) + } +} + +// A user who already holds a contradictory membership (created before this +// check existed, or made contradictory later by a group edit) must not be +// locked out of edits that leave that membership alone. +func TestUpdateUser_LegacyContradictoryUserKeepsUnrelatedEdits(t *testing.T) { + legacy := []string{"v1", "w1"} + newEmail := "renamed@example.com" + inactive := false + + cases := map[string]UpdateUserRequest{ + "rename without sending groups": {Email: &newEmail}, + "rename resending the same groups": {Email: &newEmail, GroupIDs: []string{"v1", "w1"}}, + "same groups in another order": {GroupIDs: []string{"w1", "v1"}}, + "same groups with a duplicate": {GroupIDs: []string{"w1", "v1", "w1"}}, + "deactivate": {Active: &inactive}, + } + for name, req := range cases { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + m := new(MockStore) + t.Cleanup(func() { m.AssertExpectations(t) }) + if req.Email != nil { + m.On("GetUserByEmail", ctx, newEmail).Return(nil, pgx.ErrNoRows).Once() + } + if req.Active != nil { + m.On("DeleteUserSessions", ctx, "user-1").Return(nil).Once() + } + m.On("UpdateUserAdminFields", ctx, mock.AnythingOfType("*auth.User"), mock.Anything, mock.Anything, mock.Anything).Return(nil).Once() + + _, err := updateComboUser(t, m, legacy, req) + + require.NoError(t, err) + // An unchanged membership is not re-classified at all. + m.AssertNotCalled(t, "GetGroup", mock.Anything, mock.Anything) + }) + } +} + +// The way out of a contradictory membership (dropping one side of it) must be +// accepted, or a legacy user could never be repaired through the API. +func TestUpdateUser_LegacyContradictoryUserCanBeResolved(t *testing.T) { + for name, next := range map[string][]string{ + "drop the write group": {"v1"}, + "drop the view-only group": {"w1"}, + } { + t.Run(name, func(t *testing.T) { + ctx := context.Background() + m := new(MockStore) + t.Cleanup(func() { m.AssertExpectations(t) }) + stubComboGroups(ctx, m) + m.On("UpdateUserAdminFields", ctx, mock.AnythingOfType("*auth.User"), mock.Anything, mock.Anything, mock.Anything).Return(nil).Once() + + user, err := updateComboUser(t, m, []string{"v1", "w1"}, UpdateUserRequest{GroupIDs: next}) + + require.NoError(t, err) + assert.Equal(t, next, user.GroupIDs) + }) + } +} diff --git a/internal/auth/self_account_scope_test.go b/internal/auth/self_account_scope_test.go index 03609cab..7b22a544 100644 --- a/internal/auth/self_account_scope_test.go +++ b/internal/auth/self_account_scope_test.go @@ -30,10 +30,10 @@ import ( ) const ( - scopedActorID = "88888888-8888-4888-8888-888888888888" - regionalAdminGroupID = "99999999-9999-4999-8999-999999999999" - acctAViewersGroupID = "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa" - deletedGroupID = "bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb" + scopedActorID = "88888888-8888-4888-8888-888888888888" + regionalAdminGroupID = "99999999-9999-4999-8999-999999999999" + acctAPlanAuthorsGroupID = "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa" + deletedGroupID = "bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb" ) // regionalAdminGroup is the realistic shape of a scoped administrator: the @@ -48,13 +48,15 @@ func regionalAdminGroup() *Group { } } -// acctAViewersGroup is scoped to a strict SUBSET of regionalAdminGroup's -// accounts, so joining it grants nothing new on the account dimension. -func acctAViewersGroup() *Group { +// acctAPlanAuthorsGroup is scoped to a strict SUBSET of regionalAdminGroup's +// accounts, so joining it grants nothing new on the account dimension. It is +// write-capable because a view-only group next to the admin group would be +// refused as a contradictory combination (issue #78) before scope is checked. +func acctAPlanAuthorsGroup() *Group { return &Group{ - ID: acctAViewersGroupID, - Name: "Acct-A Viewers", - Permissions: []Permission{{Action: ActionView, Resource: ResourceRecommendations}}, + ID: acctAPlanAuthorsGroupID, + Name: "Acct-A Plan Authors", + Permissions: []Permission{{Action: ActionCreate, Resource: ResourcePlans}}, AllowedAccounts: []string{"acct-A"}, } } @@ -164,15 +166,15 @@ func TestSelfAccountScope_JoiningSubsetScopedGroupAllowed(t *testing.T) { svc := createTestService(mockStore, new(MockEmailSender)) stubScopedActor(ctx, mockStore, []string{regionalAdminGroupID}, - regionalAdminGroup(), acctAViewersGroup()) + regionalAdminGroup(), acctAPlanAuthorsGroup()) mockStore.On("UpdateUserAdminFields", ctx, mock.AnythingOfType("*auth.User"), mock.Anything, mock.Anything, mock.Anything).Return(nil).Once() updated, err := svc.UpdateUser(ctx, scopedActorID, scopedActorID, UpdateUserRequest{ - GroupIDs: []string{regionalAdminGroupID, acctAViewersGroupID}, + GroupIDs: []string{regionalAdminGroupID, acctAPlanAuthorsGroupID}, }) require.NoError(t, err) - assert.Equal(t, []string{regionalAdminGroupID, acctAViewersGroupID}, updated.GroupIDs) + assert.Equal(t, []string{regionalAdminGroupID, acctAPlanAuthorsGroupID}, updated.GroupIDs) } // T5 -- Fail closed when a PRIOR group cannot be resolved (issue #1748's diff --git a/internal/auth/self_escalation_carveout_test.go b/internal/auth/self_escalation_carveout_test.go index 14c53ff3..328093bd 100644 --- a/internal/auth/self_escalation_carveout_test.go +++ b/internal/auth/self_escalation_carveout_test.go @@ -115,12 +115,15 @@ func TestSelfCarvedOutGrant_OrdinaryGroupStillAllowed(t *testing.T) { t.Cleanup(func() { mockStore.AssertExpectations(t) }) svc := createTestService(mockStore, new(MockEmailSender)) - readOnly := &Group{ + // Write-capable on purpose: a view-only group next to Administrators is + // refused as a contradictory combination (issue #78), which is not what + // this control is about. + ordinary := &Group{ ID: "77777777-7777-4777-8777-777777777777", - Name: "Read-Only Users", - Permissions: []Permission{{Action: ActionView, Resource: ResourcePlans}}, + Name: "Plan Authors", + Permissions: []Permission{{Action: ActionCreate, Resource: ResourcePlans}}, } - stubSelfActor(ctx, mockStore, []string{adminGroupID}, adminGroupRow(), readOnly) + stubSelfActor(ctx, mockStore, []string{adminGroupID}, adminGroupRow(), ordinary) var saved *User mockStore.On("UpdateUserAdminFields", ctx, mock.AnythingOfType("*auth.User"), mock.Anything, mock.Anything, mock.Anything). @@ -131,12 +134,12 @@ func TestSelfCarvedOutGrant_OrdinaryGroupStillAllowed(t *testing.T) { }).Return(nil).Once() _, err := svc.UpdateUser(ctx, selfActorID, selfActorID, UpdateUserRequest{ - GroupIDs: []string{adminGroupID, readOnly.ID}, + GroupIDs: []string{adminGroupID, ordinary.ID}, }) require.NoError(t, err) require.NotNil(t, saved) - assert.Equal(t, []string{adminGroupID, readOnly.ID}, saved.GroupIDs) + assert.Equal(t, []string{adminGroupID, ordinary.ID}, saved.GroupIDs) } // Negative control 2: a user who ALREADY holds the money verbs is not blocked @@ -229,18 +232,19 @@ func TestSelfCarvedOutGrant_AdminMayAddAnotherUserToOrdinaryGroup(t *testing.T) t.Cleanup(func() { mockStore.AssertExpectations(t) }) svc := createTestService(mockStore, new(MockEmailSender)) - readOnly := &Group{ + // Write-capable for the same reason as in OrdinaryGroupStillAllowed. + ordinary := &Group{ ID: "99999999-9999-4999-8999-999999999999", - Name: "Read-Only Users", - Permissions: []Permission{{Action: ActionView, Resource: ResourcePlans}}, + Name: "Plan Authors", + Permissions: []Permission{{Action: ActionCreate, Resource: ResourcePlans}}, } mockStore.On("GetUserByID", ctx, otherUserID). Return(&User{ID: otherUserID, Active: true, GroupIDs: []string{adminGroupID}}, nil) - stubSelfActor(ctx, mockStore, []string{adminGroupID}, adminGroupRow(), readOnly) + stubSelfActor(ctx, mockStore, []string{adminGroupID}, adminGroupRow(), ordinary) mockStore.On("UpdateUserAdminFields", ctx, mock.AnythingOfType("*auth.User"), mock.Anything, mock.Anything, mock.Anything).Return(nil).Once() _, err := svc.UpdateUser(ctx, selfActorID, otherUserID, UpdateUserRequest{ - GroupIDs: []string{adminGroupID, readOnly.ID}, + GroupIDs: []string{adminGroupID, ordinary.ID}, }) require.NoError(t, err) } @@ -255,6 +259,11 @@ func TestSelfCarvedOutGrant_InternalCallerUnaffected(t *testing.T) { mockStore.On("GetUserByID", ctx, selfActorID). Return(&User{ID: selfActorID, Active: true, GroupIDs: []string{adminGroupID}}, nil) + // The issue #78 combination check reads the groups for every caller. + // Both are write-capable, so it passes; the carve-out check, were it to + // run, would still refuse execute:purchases and fail this test. + mockStore.On("GetGroup", ctx, adminGroupID).Return(adminGroupRow(), nil) + mockStore.On("GetGroup", ctx, purchaserGroup).Return(purchaserGroupRow(), nil) mockStore.On("UpdateUserAdminFields", ctx, mock.AnythingOfType("*auth.User"), mock.Anything, mock.Anything, mock.Anything).Return(nil).Once() _, err := svc.UpdateUser(ctx, "", selfActorID, UpdateUserRequest{ diff --git a/internal/auth/service_api_fields_test.go b/internal/auth/service_api_fields_test.go index 4b2e41a1..90e46478 100644 --- a/internal/auth/service_api_fields_test.go +++ b/internal/auth/service_api_fields_test.go @@ -88,6 +88,9 @@ func TestAPIAdapters_CreateUser_AllFieldsReachStore(t *testing.T) { svc := createTestService(store, new(MockEmailSender)) var got *User store.On("GetUserByEmail", ctx, req.Email).Return(nil, nil).Once() + // The issue #78 combination check reads each group; unknown groups are + // skipped, so returning none keeps this test about field plumbing. + store.On("GetGroup", ctx, mock.Anything).Return(nil, nil).Maybe() store.On("CreateUser", ctx, mock.AnythingOfType("*auth.User")). Run(func(args mock.Arguments) { got = args.Get(1).(*User) }).Return(nil).Once() _, err := svc.CreateUserAPI(ctx, "", req) diff --git a/internal/auth/service_group_only_authz_test.go b/internal/auth/service_group_only_authz_test.go index a47da917..081155d6 100644 --- a/internal/auth/service_group_only_authz_test.go +++ b/internal/auth/service_group_only_authz_test.go @@ -276,15 +276,22 @@ func TestUpdateUser_AdminEditingSelfAllowed(t *testing.T) { admin := &User{ID: "adm", GroupIDs: []string{DefaultAdminGroupID}, Active: true} // GetUserByID is hit by UpdateUser (target) and by HasPermission (actor). mockStore.On("GetUserByID", ctx, "adm").Return(admin, nil) + // A write-capable group: adding a view-only group next to Administrators + // would be refused as a contradictory combination (issue #78). + planAuthors := &Group{ + ID: "00000000-0000-5000-8000-000000000003", + Name: "Plan Authors", + Permissions: []Permission{{Action: ActionCreate, Resource: ResourcePlans}}, + } mockStore.On("GetGroup", ctx, DefaultAdminGroupID).Return(adminGroup(), nil) - mockStore.On("GetGroup", ctx, viewerGroup().ID).Return(viewerGroup(), nil) + mockStore.On("GetGroup", ctx, planAuthors.ID).Return(planAuthors, nil) mockStore.On("UpdateUserAdminFields", ctx, mock.AnythingOfType("*auth.User"), mock.Anything, mock.Anything, mock.Anything).Return(nil) - // Admin adds a viewer group to themselves while KEEPING Administrators, so + // Admin adds another group to themselves while KEEPING Administrators, so // the last-admin branch is not triggered and the self-escalation guard // passes because the actor holds {admin, *} (i.e. manage-users). updated, err := svc.UpdateUser(ctx, "adm", "adm", - UpdateUserRequest{GroupIDs: []string{DefaultAdminGroupID, viewerGroup().ID}}) + UpdateUserRequest{GroupIDs: []string{DefaultAdminGroupID, planAuthors.ID}}) require.NoError(t, err) - assert.ElementsMatch(t, []string{DefaultAdminGroupID, viewerGroup().ID}, updated.GroupIDs) + assert.ElementsMatch(t, []string{DefaultAdminGroupID, planAuthors.ID}, updated.GroupIDs) } diff --git a/internal/auth/service_user.go b/internal/auth/service_user.go index 61e865d2..d441b162 100644 --- a/internal/auth/service_user.go +++ b/internal/auth/service_user.go @@ -161,15 +161,21 @@ type CreateUserResult struct { InviteEmailError string } -// checkCreateUserCeiling applies the membership grant ceiling to a create -// request's GroupIDs, skipping it for trusted internal callers. Extracted -// from CreateUser to keep that function under gocyclo's complexity -// threshold, the same reason guardDeactivation was pulled out of UpdateUser. -func (s *Service) checkCreateUserCeiling(ctx context.Context, actorUserID string, groupIDs []string) error { - if actorUserID == "" { - return nil +// checkCreateUserGroups applies the membership grant ceiling to a create +// request's GroupIDs, skipping it for trusted internal callers, then the +// issue #78 view-only/write-capable contradiction check, which applies to +// every caller because it is about the membership rather than the actor. The +// order matches guardGroupChange: access refusals (403) win over the +// contradiction (400). Extracted from CreateUser to keep that function under +// gocyclo's complexity threshold, the same reason guardDeactivation was +// pulled out of UpdateUser. +func (s *Service) checkCreateUserGroups(ctx context.Context, actorUserID string, groupIDs []string) error { + if actorUserID != "" { + if err := s.checkMembershipGrantCeiling(ctx, actorUserID, groupIDs); err != nil { + return err + } } - return s.checkMembershipGrantCeiling(ctx, actorUserID, groupIDs) + return s.checkGroupCombination(ctx, groupIDs) } // CreateUser creates a new user (admin only). @@ -197,7 +203,7 @@ func (s *Service) CreateUser(ctx context.Context, actorUserID string, req Create if err := s.validateCreateUserRequest(ctx, req); err != nil { return nil, err } - if err := s.checkCreateUserCeiling(ctx, actorUserID, req.GroupIDs); err != nil { + if err := s.checkCreateUserGroups(ctx, actorUserID, req.GroupIDs); err != nil { return nil, err } @@ -442,10 +448,26 @@ func applyDeactivatedAtTransition(user *User, reqActive *bool) { } } -// guardGroupChange enforces the issue #907 invariants for a group-membership -// change: at least one group remains, the last Administrators-group member is -// not removed, and a non-privileged actor cannot escalate their own access. +// guardGroupChange enforces the invariants for a group-membership change: +// the issue #907 / #226 access rules in guardGroupChangeAccess, then the issue +// #78 check that the change does not leave a view-only group next to a +// write-capable one. The contradiction check runs last so a refused +// escalation is still reported as the escalation (403), and it runs for every +// caller, trusted internal ones included, because it is about the resulting +// membership rather than the actor. An unchanged membership is not +// re-checked (see checkGroupMembershipChange). func (s *Service) guardGroupChange(ctx context.Context, actorUserID, targetUserID string, prior, next []string) error { + if err := s.guardGroupChangeAccess(ctx, actorUserID, targetUserID, prior, next); err != nil { + return err + } + return s.checkGroupMembershipChange(ctx, prior, next) +} + +// guardGroupChangeAccess enforces the issue #907 invariants for a +// group-membership change: at least one group remains, the last +// Administrators-group member is not removed, and a non-privileged actor +// cannot escalate their own access. +func (s *Service) guardGroupChangeAccess(ctx context.Context, actorUserID, targetUserID string, prior, next []string) error { if len(next) == 0 { return ErrNoGroups } diff --git a/internal/auth/service_user_test.go b/internal/auth/service_user_test.go index 2207ab29..8e978658 100644 --- a/internal/auth/service_user_test.go +++ b/internal/auth/service_user_test.go @@ -633,6 +633,8 @@ func TestService_UpdateUser(t *testing.T) { } mockStore.On("GetUserByID", ctx, "user-123").Return(existingUser, nil).Once() + // Two view-only groups: a consistent combination (issue #78). + stubViewOnlyGroups(ctx, mockStore, "group-2", "group-3") mockStore.On("UpdateUserAdminFields", ctx, mock.AnythingOfType("*auth.User"), mock.Anything, mock.Anything, mock.Anything).Return(nil).Once() req := UpdateUserRequest{ @@ -1045,6 +1047,8 @@ func TestService_CreateUser_EdgeCases(t *testing.T) { service := createTestService(mockStore, mockEmail) mockStore.On("GetUserByEmail", ctx, "newuser@example.com").Return(nil, nil).Once() + // Two view-only groups: a consistent combination (issue #78). + stubViewOnlyGroups(ctx, mockStore, "group-1", "group-2") mockStore.On("CreateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Once() req := CreateUserRequest{ diff --git a/internal/server/group_combination_dispatch_test.go b/internal/server/group_combination_dispatch_test.go new file mode 100644 index 00000000..e9c17d93 --- /dev/null +++ b/internal/server/group_combination_dispatch_test.go @@ -0,0 +1,93 @@ +package server + +// End-to-end dispatch coverage for the issue #78 group-combination check: a +// real HTTP request through Handler.HandleRequest -> Router.Route -> +// requireAdmin -> createUser/updateUser -> the REAL auth.Service, with only the +// store mocked. Reuses the #1756 harness (newSelfScopeHarness), whose actor is +// an administrator over acct-A. + +import ( + "context" + "encoding/json" + "testing" + + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" +) + +// comboDispatchViewers is view-only and inside the actor's account scope, so +// the access guards pass and only the combination check can refuse it. +var comboDispatchViewers = &auth.Group{ + ID: "55555555-5555-4555-8555-555555555558", + Name: "Acct-A Viewers", + Permissions: []auth.Permission{{Action: auth.ActionView, Resource: auth.ResourceRecommendations}}, + AllowedAccounts: []string{scopeDispatchAccountID}, +} + +func usersRequest(t *testing.T, method, path string, body map[string]any) *events.LambdaFunctionURLRequest { + t.Helper() + raw, err := json.Marshal(body) + require.NoError(t, err) + req := selfMembershipRequest(t, nil) + req.Body = string(raw) + req.RequestContext.HTTP.Method = method + req.RequestContext.HTTP.Path = path + return req +} + +func TestGroupCombinationDispatch_UpdateRefusedWith400(t *testing.T) { + handler, mockStore := newSelfScopeHarness(t, []string{scopeDispatchGroupID}) + mockStore.On("GetGroup", mock.Anything, comboDispatchViewers.ID).Return(comboDispatchViewers, nil) + + resp, err := handler.HandleRequest(context.Background(), + selfMembershipRequest(t, []string{scopeDispatchGroupID, comboDispatchViewers.ID})) + + require.NoError(t, err) + assert.Equal(t, 400, resp.StatusCode, "body: %s", resp.Body) + assert.Contains(t, resp.Body, "contradictory group combination") + assert.Contains(t, resp.Body, "Acct-A Viewers") + mockStore.AssertNotCalled(t, "UpdateUserAdminFields", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything) +} + +func TestGroupCombinationDispatch_CreateRefusedWith400(t *testing.T) { + handler, mockStore := newSelfScopeHarness(t, []string{scopeDispatchGroupID}) + mockStore.On("GetGroup", mock.Anything, comboDispatchViewers.ID).Return(comboDispatchViewers, nil) + mockStore.On("GetUserByEmail", mock.Anything, "new-user@example.com").Return(nil, nil) + + resp, err := handler.HandleRequest(context.Background(), usersRequest(t, "POST", "/api/users", map[string]any{ + "email": "new-user@example.com", + "groups": []string{comboDispatchViewers.ID, scopeDispatchGroupID}, + })) + + require.NoError(t, err) + assert.Equal(t, 400, resp.StatusCode, "body: %s", resp.Body) + assert.Contains(t, resp.Body, "contradictory group combination") + mockStore.AssertNotCalled(t, "CreateUser", mock.Anything, mock.Anything) +} + +// A user who already holds a contradictory membership can still be renamed +// through the route, with the groups omitted or echoed back unchanged. +func TestGroupCombinationDispatch_LegacyUserRenameAllowed(t *testing.T) { + legacy := []string{scopeDispatchGroupID, comboDispatchViewers.ID} + for name, body := range map[string]map[string]any{ + "groups omitted": {"email": "renamed@example.com"}, + "groups echoed unchanged": {"email": "renamed@example.com", "groups": []string{comboDispatchViewers.ID, scopeDispatchGroupID}}, + } { + t.Run(name, func(t *testing.T) { + handler, mockStore := newSelfScopeHarness(t, legacy) + mockStore.On("GetGroup", mock.Anything, comboDispatchViewers.ID).Return(comboDispatchViewers, nil).Maybe() + mockStore.On("GetUserByEmail", mock.Anything, "renamed@example.com").Return(nil, nil).Once() + mockStore.On("UpdateUserAdminFields", mock.Anything, mock.AnythingOfType("*auth.User"), mock.Anything, mock.Anything, mock.Anything).Return(nil).Once() + + resp, err := handler.HandleRequest(context.Background(), + usersRequest(t, "PUT", "/api/users/"+scopeDispatchUserID, body)) + + require.NoError(t, err) + assert.Equal(t, 200, resp.StatusCode, "body: %s", resp.Body) + }) + } +} diff --git a/internal/server/self_account_scope_dispatch_test.go b/internal/server/self_account_scope_dispatch_test.go index fcc8ef21..cd979797 100644 --- a/internal/server/self_account_scope_dispatch_test.go +++ b/internal/server/self_account_scope_dispatch_test.go @@ -146,10 +146,13 @@ func TestSelfAccountScopeDispatch_LeaveScopingGroupRefused(t *testing.T) { // scope still succeeds through the same route. Without this, a guard refusing // every self-membership edit would pass both tests above. func TestSelfAccountScopeDispatch_NonWideningChangeAllowed(t *testing.T) { + // Write-capable on purpose: a view-only group next to the admin group is + // refused as a contradictory combination (issue #78), which is not what + // this control is about. subset := &auth.Group{ ID: "55555555-5555-4555-8555-555555555557", - Name: "Acct-A Viewers", - Permissions: []auth.Permission{{Action: auth.ActionView, Resource: auth.ResourceRecommendations}}, + Name: "Acct-A Plan Authors", + Permissions: []auth.Permission{{Action: auth.ActionCreate, Resource: auth.ResourcePlans}}, AllowedAccounts: []string{scopeDispatchAccountID}, } handler, mockStore := newSelfScopeHarness(t, []string{scopeDispatchGroupID})