diff --git a/internal/auth/self_account_scope_test.go b/internal/auth/self_account_scope_test.go new file mode 100644 index 000000000..401f18c5e --- /dev/null +++ b/internal/auth/self_account_scope_test.go @@ -0,0 +1,240 @@ +package auth + +// Account-scope ceiling on self-membership changes (issue #1756). +// +// The bug had two routes running in OPPOSITE directions, and a guard that only +// inspects what is being ADDED sees exactly one of them: +// +// join a wider group -> Administrators ships allowed_accounts = ARRAY['*'] +// leave the scoping group -> the union collapses to [], which +// IsUnrestrictedAccess reads as every account +// +// Both refusals below are therefore paired with a control that must still be +// ALLOWED, because a guard that refused every self-membership edit would pass +// the refusals on its own. The controls are the load-bearing half of this file: +// +// drop the group that carries NO restriction -> allowed (T3) +// join a group scoped to a SUBSET of your own accounts -> allowed (T4) +// +// T3 is deliberately the same shape as the T2 refusal (a removal by the same +// actor from the same two groups), so it pins that the guard distinguishes +// WHICH group carried the restriction rather than refusing removals wholesale. + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +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" +) + +// regionalAdminGroup is the realistic shape of a scoped administrator: the +// full {admin, *} capability (which is what PUT /api/users/{id} gates on), but +// limited to two cloud accounts. Everything this file tests starts here. +func regionalAdminGroup() *Group { + return &Group{ + ID: regionalAdminGroupID, + Name: "Regional Administrators", + Permissions: []Permission{{Action: ActionAdmin, Resource: ResourceAll}}, + AllowedAccounts: []string{"acct-A", "acct-B"}, + } +} + +// acctAViewersGroup is scoped to a strict SUBSET of regionalAdminGroup's +// accounts, so joining it grants nothing new on the account dimension. +func acctAViewersGroup() *Group { + return &Group{ + ID: acctAViewersGroupID, + Name: "Acct-A Viewers", + Permissions: []Permission{{Action: ActionView, Resource: ResourceRecommendations}}, + AllowedAccounts: []string{"acct-A"}, + } +} + +// stubScopedActor wires the actor's user row and the groups the change touches. +// +// Refusal cases add a permissive UpdateUser stub on purpose: without one, a +// guard removal kills the test by PANICKING on an unstubbed write, and that +// kill evaporates the moment someone adds a stub while tidying fixtures. With +// it, the removal is caught by an assertion instead. +// +// The GetGroup stubs are .Maybe() for the mirror-image reason. They are the +// guard's own reads, so a guard removal leaves them unmet and every test in +// this file fails on the unmet expectation -- including the CONTROLS, which +// exist precisely to pass when the guard is disabled and fail when it refuses +// everything. Asserting them would collapse that distinction into "all eight +// tests turn red", which measures nothing. Each test asserts its outcome +// directly instead. +func stubScopedActor(ctx context.Context, mockStore *MockStore, priorGroups []string, groups ...*Group) { + mockStore.On("GetUserByID", ctx, scopedActorID). + Return(&User{ID: scopedActorID, Active: true, GroupIDs: priorGroups}, nil) + for _, g := range groups { + mockStore.On("GetGroup", ctx, g.ID).Return(g, nil).Maybe() + } +} + +// T1 -- Route 1: joining a wider group. The seeded Administrators group ships +// allowed_accounts = ARRAY['*'] (migrations 000024/000057), so a default +// deployment always has one group to launder scope through. The pre-existing +// guards pass this: update:users is what they ask for and admin:* grants it, +// and {admin, *} is not a carved-out money verb. +func TestSelfAccountScope_JoiningWiderGroupRefused(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := createTestService(mockStore, new(MockEmailSender)) + + stubScopedActor(ctx, mockStore, []string{regionalAdminGroupID}, + regionalAdminGroup(), adminGroup()) + mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Maybe() + + _, err := svc.UpdateUser(ctx, scopedActorID, scopedActorID, UpdateUserRequest{ + GroupIDs: []string{regionalAdminGroupID, DefaultAdminGroupID}, + }) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrSelfEscalation) + assert.Contains(t, err.Error(), "all cloud accounts") + mockStore.AssertNotCalled(t, "UpdateUser", mock.Anything, mock.Anything) +} + +// T2 -- Route 2: leaving the group that carries the restriction. Nothing is +// added, so every add-oriented check is a no-op; the union collapses from +// [acct-A acct-B] to [] and empty means EVERY account. Removing the +// restriction grants the restriction. +func TestSelfAccountScope_LeavingScopingGroupRefused(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := createTestService(mockStore, new(MockEmailSender)) + + // viewerGroup carries no allowed_accounts, so it contributes nothing to + // the union: dropping regionalAdminGroup leaves the actor unrestricted. + stubScopedActor(ctx, mockStore, []string{regionalAdminGroupID, viewerGroup().ID}, + regionalAdminGroup(), viewerGroup()) + mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Maybe() + + _, err := svc.UpdateUser(ctx, scopedActorID, scopedActorID, UpdateUserRequest{ + GroupIDs: []string{viewerGroup().ID}, + }) + + require.Error(t, err) + assert.ErrorIs(t, err, ErrSelfEscalation) + assert.Contains(t, err.Error(), "all cloud accounts") + mockStore.AssertNotCalled(t, "UpdateUser", mock.Anything, mock.Anything) +} + +// T3 -- Control for T2: the SAME actor removing the OTHER group. viewerGroup +// contributes no accounts, so the surviving union is unchanged and the change +// must go through. A guard that refused removals wholesale, or one that only +// checked "is the resulting scope non-empty", fails here. +func TestSelfAccountScope_NonWideningRemovalAllowed(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := createTestService(mockStore, new(MockEmailSender)) + + stubScopedActor(ctx, mockStore, []string{regionalAdminGroupID, viewerGroup().ID}, + regionalAdminGroup(), viewerGroup()) + mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Once() + + updated, err := svc.UpdateUser(ctx, scopedActorID, scopedActorID, UpdateUserRequest{ + GroupIDs: []string{regionalAdminGroupID}, + }) + + require.NoError(t, err) + assert.Equal(t, []string{regionalAdminGroupID}, updated.GroupIDs) +} + +// T4 -- Control for T1: joining a group whose scope is a strict SUBSET of the +// actor's own grants nothing, so it must still be allowed. Ordinary membership +// administration inside one's own scope keeps working. +func TestSelfAccountScope_JoiningSubsetScopedGroupAllowed(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := createTestService(mockStore, new(MockEmailSender)) + + stubScopedActor(ctx, mockStore, []string{regionalAdminGroupID}, + regionalAdminGroup(), acctAViewersGroup()) + mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Once() + + updated, err := svc.UpdateUser(ctx, scopedActorID, scopedActorID, UpdateUserRequest{ + GroupIDs: []string{regionalAdminGroupID, acctAViewersGroupID}, + }) + + require.NoError(t, err) + assert.Equal(t, []string{regionalAdminGroupID, acctAViewersGroupID}, updated.GroupIDs) +} + +// T5 -- Fail closed when a PRIOR group cannot be resolved (issue #1748's +// mechanism, on this path). A silently skipped group leaves an empty prior +// union, which reads as unrestricted and turns the ceiling into a no-op on +// exactly the change it guards: this actor's real scope is [acct-A acct-B] and +// swallowing the skip would compute "unrestricted" and wave the removal +// through. The removal route is the one that matters here, because nothing is +// added and no other guard runs at all. +func TestSelfAccountScope_FailsClosedOnUnresolvablePriorGroup(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := createTestService(mockStore, new(MockEmailSender)) + + mockStore.On("GetUserByID", ctx, scopedActorID). + Return(&User{ID: scopedActorID, Active: true, + GroupIDs: []string{deletedGroupID, viewerGroup().ID}}, nil) + // The store returns (nil, nil) for a deleted group. + mockStore.On("GetGroup", ctx, deletedGroupID).Return(nil, nil) + mockStore.On("GetGroup", ctx, viewerGroup().ID).Return(viewerGroup(), nil) + mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Maybe() + + _, err := svc.UpdateUser(ctx, scopedActorID, scopedActorID, UpdateUserRequest{ + GroupIDs: []string{viewerGroup().ID}, + }) + + require.Error(t, err) + assert.Contains(t, err.Error(), "could not be loaded") + mockStore.AssertNotCalled(t, "UpdateUser", mock.Anything, mock.Anything) +} + +// T6 -- Control for T5, and the reason the fail-closed rule is "the surviving +// union is EMPTY", not "anything was skipped". +// +// DeleteGroup drops the groups row and never purges users.group_ids (an array +// column with no FK), so a dangling membership id is the ORDINARY state after +// any custom group is deleted. Refusing on any skip would leave an admin who +// is already unrestricted unable to remove that dangling id from their own +// membership -- a change that widens nothing, from a principal at maximum +// scope -- so a single-admin deployment could never clean up after itself. +// +// The surviving union here is ["*"], so the skip cannot have widened anything +// and the cleanup must go through. Only an EMPTY surviving union (T5) is +// ambiguous enough to refuse. +func TestSelfAccountScope_UnrestrictedActorCanDropDanglingGroup(t *testing.T) { + ctx := context.Background() + mockStore := new(MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + svc := createTestService(mockStore, new(MockEmailSender)) + + mockStore.On("GetUserByID", ctx, scopedActorID). + Return(&User{ID: scopedActorID, Active: true, + GroupIDs: []string{DefaultAdminGroupID, deletedGroupID}}, nil) + mockStore.On("GetGroup", ctx, DefaultAdminGroupID).Return(adminGroup(), nil).Maybe() + mockStore.On("GetGroup", ctx, deletedGroupID).Return(nil, nil).Maybe() + mockStore.On("UpdateUser", ctx, mock.AnythingOfType("*auth.User")).Return(nil).Once() + + updated, err := svc.UpdateUser(ctx, scopedActorID, scopedActorID, UpdateUserRequest{ + GroupIDs: []string{DefaultAdminGroupID}, + }) + + require.NoError(t, err) + assert.Equal(t, []string{DefaultAdminGroupID}, updated.GroupIDs) +} diff --git a/internal/auth/service_group.go b/internal/auth/service_group.go index d7f31c2bc..d4b85b7bc 100644 --- a/internal/auth/service_group.go +++ b/internal/auth/service_group.go @@ -120,6 +120,52 @@ func (s *Service) permissionsForGroups(ctx context.Context, groupIDs []string) ( return permissions, nil } +// accountsForGroups returns the union of allowed_accounts across the given +// groups, taking the membership list directly so a caller reasoning about a +// membership CHANGE can evaluate the PRIOR and the RESULTING scope with the +// same function (see guardSelfAccountScope). +// +// It FAILS CLOSED where its permission-side twin permissionsForGroups can +// safely fail open. That twin skips a missing group because a lost group can +// only shrink a permission union. Here the union is inverted: EMPTY means +// every account (IsUnrestrictedAccess), so a skipped group can WIDEN the +// result. The union of [] and ["acct-A"] is restricted; lose the group +// carrying ["acct-A"] and it reads as unrestricted -- issue #1748's failure +// mode, which a guard swallowing the skip would reproduce here by computing +// an unrestricted prior scope and then waving through every change. +// +// The two refusal conditions are deliberately IDENTICAL to grantCeilingAccounts +// and ResolveAllowedAccounts, and the emptiness test is the load-bearing part: +// a skip only widens the union when what survives is EMPTY. Otherwise the +// computed union is a SUBSET of the true one, which makes this ceiling +// stricter than reality rather than looser, and stricter fails closed. +// +// Refusing on ANY skip instead is the tempting stronger rule and is wrong. +// DeleteGroup drops the groups row without purging users.group_ids (there is +// no FK on that array column), so a dangling membership id is the ordinary +// state after any custom group is deleted. Under a refuse-on-any-skip rule an +// unrestricted admin carrying one dangling id cannot even remove that id from +// their own membership -- a change that widens nothing, from a principal +// already at maximum scope -- so a single-admin deployment has no way to clean +// up. That is pure availability cost for zero security benefit, which is +// precisely the trade-off grantCeilingAccounts documents rejecting. +func (s *Service) accountsForGroups(ctx context.Context, groupIDs []string) ([]string, error) { + authCtx := &AuthContext{} + if err := s.collectGroupsAndAccounts(ctx, authCtx, groupIDs); err != nil { + return nil, fmt.Errorf("failed to resolve account scope: %w", err) + } + if len(authCtx.AllowedAccounts) == 0 && authCtx.SkippedGroups > 0 { + return nil, fmt.Errorf( + "failed to resolve account scope: %d group(s) could not be loaded and the remaining scope is unrestricted", + authCtx.SkippedGroups) + } + // An unresolved scope is UNKNOWN, not unrestricted. + if len(authCtx.Groups) == 0 { + return nil, fmt.Errorf("failed to resolve account scope: no group could be loaded") + } + return authCtx.AllowedAccounts, nil +} + // BuildAuthContext builds a complete authorization context for a user. // Permissions and allowed accounts are derived purely from the union of the // user's group memberships; a user with no groups gets an empty context and diff --git a/internal/auth/service_user.go b/internal/auth/service_user.go index 635a45171..4951cd797 100644 --- a/internal/auth/service_user.go +++ b/internal/auth/service_user.go @@ -381,20 +381,39 @@ func (s *Service) guardGroupChange(ctx context.Context, actorUserID, targetUserI } // Self-escalation guard: when the actor is editing their own membership, - // any group being ADDED that they did not already have requires the actor - // to hold the manage-users permission. A non-privileged user therefore - // cannot grant themselves a more powerful group. Internal callers - // (actorUserID == "") are already trusted and skip this check. - if actorUserID == "" || actorUserID != targetUserID || !addsNewGroup(prior, next) { + // the change must not leave them holding more than they went in with. + // Internal callers (actorUserID == "") are already trusted and skip it. + // + // This deliberately does NOT filter on addsNewGroup, and that condition + // used to live here (issue #1756). Screening the whole guard on "does this + // add a group?" is right for the permission dimension and wrong for the + // account dimension, because a REMOVAL widens account scope: the union of + // allowed_accounts is a set in which EMPTY means every account, so dropping + // the group carrying the restriction removes the restriction. The + // add-only screen therefore left the pure-removal self-edit completely + // unguarded. addsNewGroup now sits inside guardSelfEscalation, scoped to + // the dimension it is actually true for. + if actorUserID == "" || actorUserID != targetUserID { return nil } return s.guardSelfEscalation(ctx, prior, next) } -// guardSelfEscalation runs the two self-edit checks in order: the #907 -// manage-users gate, then the #1550 carved-out-verb gate that manage-users is -// not sufficient to pass. Split out of guardGroupChange to keep that function -// under gocyclo's threshold. +// guardSelfEscalation runs the self-edit checks: the #907 manage-users gate, +// the #1550 carved-out-verb gate that manage-users is not sufficient to pass, +// and the #1756 account ceiling. Split out of guardGroupChange to keep that +// function under gocyclo's threshold. +// +// The two dimensions have deliberately different reach, and the difference is +// a property of the data rather than a convenience: +// +// - Permissions are a plain union with no inverted empty case, so dropping a +// group can only SHRINK the effective set. Only an added group can grant a +// permission, which is why the permission checks stay behind addsNewGroup. +// - allowed_accounts is a union in which the EMPTY set means every account +// (IsUnrestrictedAccess). Dropping the group that carries the restriction +// collapses the union to empty and thereby WIDENS scope, so the account +// ceiling has to run on every self-edit, additions or not. // // DO NOT "simplify" this back to re-reading the actor's row // (GetUserPermissions, or any fresh GetUserByID). That is the shorter @@ -419,15 +438,61 @@ func (s *Service) guardGroupChange(ctx context.Context, actorUserID, targetUserI // Enforced by mutation, not just by this comment: swapping `prior` for `next` // below fails the suite (see the M13 row in the PR #1737 mutation matrix). func (s *Service) guardSelfEscalation(ctx context.Context, prior, next []string) error { - heldBefore, err := s.permissionsForGroups(ctx, prior) + if addsNewGroup(prior, next) { + heldBefore, err := s.permissionsForGroups(ctx, prior) + if err != nil { + return fmt.Errorf("failed to verify manage-users permission: %w", err) + } + if !s.permissionsAllow(heldBefore, ActionUpdate, ResourceUsers, nil) { + return ErrSelfEscalation + } + // Holding update:users is NOT enough to hand yourself the money verbs. + if err := s.guardSelfCarvedOutGrant(ctx, heldBefore, next, prior); err != nil { + return err + } + } + // Runs whether or not anything was added: see the account bullet above. + return s.guardSelfAccountScope(ctx, prior, next) +} + +// guardSelfAccountScope bounds the account dimension of a self-membership +// change: the scope the actor comes out with may not reach beyond the scope +// they went in with (issue #1756). +// +// It compares RESULTING scope against PRIOR scope rather than inspecting what +// is being added, and that is the whole point. Two routes widen scope and they +// run in opposite directions, so an "is anything being granted?" check only +// ever sees one of them: +// +// - JOIN a wider group. The seeded Administrators group ships +// allowed_accounts = ARRAY['*'] (migrations 000024/000057), so adding it +// hands the actor every account. The permission gate passes this happily: +// update:users is what it asks for and the actor already holds it. +// - LEAVE the scoping group. The union collapses to empty, which +// IsUnrestrictedAccess reads as every account. Removing the restriction +// grants the restriction. +// +// accountScopeGap gets both cases right because it treats empty and "*" as +// UNRESTRICTED on EITHER side, so it is not a subset test: the empty set is a +// subset of everything while meaning the opposite of narrow. Prior +// unrestricted accepts anything (nothing left to widen); next unrestricted +// against a restricted prior is refused whatever produced it; otherwise next +// must name only accounts prior already named. +func (s *Service) guardSelfAccountScope(ctx context.Context, prior, next []string) error { + priorAccounts, err := s.accountsForGroups(ctx, prior) if err != nil { - return fmt.Errorf("failed to verify manage-users permission: %w", err) + return err } - if !s.permissionsAllow(heldBefore, ActionUpdate, ResourceUsers, nil) { - return ErrSelfEscalation + nextAccounts, err := s.accountsForGroups(ctx, next) + if err != nil { + return err } - // Holding update:users is NOT enough to hand yourself the money verbs. - return s.guardSelfCarvedOutGrant(ctx, heldBefore, next, prior) + if gap := accountScopeGap(priorAccounts, nextAccounts); gap != "" { + return fmt.Errorf( + "%w: this change would give you %s, beyond your current account scope; ask another administrator to make it", + ErrSelfEscalation, gap) + } + return nil } // guardSelfCarvedOutGrant blocks the membership route to the same escalation diff --git a/internal/server/self_account_scope_dispatch_test.go b/internal/server/self_account_scope_dispatch_test.go new file mode 100644 index 000000000..7b3139f08 --- /dev/null +++ b/internal/server/self_account_scope_dispatch_test.go @@ -0,0 +1,164 @@ +package server + +// End-to-end dispatch coverage for the #1756 account-scope ceiling on +// self-membership changes. +// +// The service-level tests live in internal/auth. This one exists because a +// handler-level test has repeatedly missed router-level bypasses in this +// codebase (#1757, #1773): it drives a real HTTP request through +// Handler.HandleRequest -> Router.Route -> requireAdmin -> updateUser -> the +// REAL auth.Service (only the store is mocked), so it proves the guard is +// actually reachable on the route rather than only callable in isolation. +// +// The actor holds {admin, *} deliberately. PUT /api/users/{id} is an AuthAdmin +// route, so a scoped ADMINISTRATOR is the only principal that can reach the +// endpoint at all, and is therefore the realistic attacker for this issue. + +import ( + "context" + "encoding/json" + "testing" + "time" + + "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/CUDly/internal/api" + "github.com/LeanerCloud/CUDly/internal/auth" +) + +const ( + scopeDispatchUserID = "77777777-7777-4777-8777-777777777777" + scopeDispatchGroupID = "66666666-6666-4666-8666-666666666666" + scopeDispatchRawToken = "self-scope-session-token" + scopeDispatchAccountID = "acct-A" +) + +// newSelfScopeHarness wires a real auth.Service (mock store only) behind the +// real API handler and returns both so a test can drive requests and adjust +// expectations. +func newSelfScopeHarness(t *testing.T, priorGroups []string) (*api.Handler, *auth.MockStore) { + t.Helper() + mockStore := new(auth.MockStore) + t.Cleanup(func() { mockStore.AssertExpectations(t) }) + + mockStore.On("GetSession", mock.Anything, mock.AnythingOfType("string")).Return(&auth.Session{ + Token: hashSessionTokenForTest(scopeDispatchRawToken), + UserID: scopeDispatchUserID, + Email: "regional-admin@example.com", + ExpiresAt: time.Now().Add(time.Hour), + }, nil) + mockStore.On("GetUserByID", mock.Anything, scopeDispatchUserID).Return(&auth.User{ + ID: scopeDispatchUserID, + Email: "regional-admin@example.com", + Active: true, + GroupIDs: priorGroups, + }, nil) + // A scoped administrator: full {admin, *}, but only over acct-A. + mockStore.On("GetGroup", mock.Anything, scopeDispatchGroupID).Return(&auth.Group{ + ID: scopeDispatchGroupID, + Name: "Regional Administrators", + Permissions: []auth.Permission{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}}, + AllowedAccounts: []string{scopeDispatchAccountID}, + }, nil) + + service := auth.NewService(auth.ServiceConfig{ + Store: mockStore, + SessionDuration: time.Hour, + CSRFKey: auth.TestCSRFKey(), + }) + handler := api.NewHandler(api.HandlerConfig{AuthService: newAuthServiceAdapter(service)}) + return handler, mockStore +} + +// selfMembershipRequest builds the session-authenticated PUT that rewrites the +// actor's own group membership. +func selfMembershipRequest(t *testing.T, groupIDs []string) *events.LambdaFunctionURLRequest { + t.Helper() + body, err := json.Marshal(map[string]any{"groups": groupIDs}) + require.NoError(t, err) + return &events.LambdaFunctionURLRequest{ + Headers: map[string]string{ + "Authorization": "Bearer " + scopeDispatchRawToken, + "X-CSRF-Token": auth.DeriveTestCSRFToken(scopeDispatchRawToken), + "Content-Type": "application/json", + }, + Body: string(body), + RequestContext: events.LambdaFunctionURLRequestContext{ + HTTP: events.LambdaFunctionURLRequestContextHTTPDescription{ + Method: "PUT", + Path: "/api/users/" + scopeDispatchUserID, + }, + }, + } +} + +// A scoped administrator adding themselves to the seeded Administrators group +// (allowed_accounts = ARRAY['*']) must be refused with a 403 by the time the +// request comes back out of the router, and no user row may be written. +func TestSelfAccountScopeDispatch_JoinWiderGroupRefused(t *testing.T) { + handler, mockStore := newSelfScopeHarness(t, []string{scopeDispatchGroupID}) + mockStore.On("GetGroup", mock.Anything, auth.DefaultAdminGroupID).Return(&auth.Group{ + ID: auth.DefaultAdminGroupID, + Name: "Administrators", + Permissions: []auth.Permission{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}}, + AllowedAccounts: []string{"*"}, + }, nil) + // Permissive write stub so a guard removal fails by assertion below rather + // than by panicking on an unstubbed call. + mockStore.On("UpdateUser", mock.Anything, mock.AnythingOfType("*auth.User")).Return(nil).Maybe() + + resp, err := handler.HandleRequest(context.Background(), + selfMembershipRequest(t, []string{scopeDispatchGroupID, auth.DefaultAdminGroupID})) + + require.NoError(t, err) + assert.Equal(t, 403, resp.StatusCode, "body: %s", resp.Body) + assert.Contains(t, resp.Body, "all cloud accounts") + mockStore.AssertNotCalled(t, "UpdateUser", mock.Anything, mock.Anything) +} + +// The same actor dropping the group that carries their restriction: the +// resulting union is empty, which reads as every account. Also a 403, with no +// write. Nothing is being added here, so this request passes every add-oriented +// check untouched. +func TestSelfAccountScopeDispatch_LeaveScopingGroupRefused(t *testing.T) { + viewers := &auth.Group{ + ID: "55555555-5555-4555-8555-555555555556", + Name: "Viewers", + Permissions: []auth.Permission{{Action: auth.ActionView, Resource: auth.ResourceRecommendations}}, + } + handler, mockStore := newSelfScopeHarness(t, []string{scopeDispatchGroupID, viewers.ID}) + mockStore.On("GetGroup", mock.Anything, viewers.ID).Return(viewers, nil) + mockStore.On("UpdateUser", mock.Anything, mock.AnythingOfType("*auth.User")).Return(nil).Maybe() + + resp, err := handler.HandleRequest(context.Background(), + selfMembershipRequest(t, []string{viewers.ID})) + + require.NoError(t, err) + assert.Equal(t, 403, resp.StatusCode, "body: %s", resp.Body) + assert.Contains(t, resp.Body, "all cloud accounts") + mockStore.AssertNotCalled(t, "UpdateUser", mock.Anything, mock.Anything) +} + +// Control: the same actor making a membership change that does NOT widen their +// 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) { + subset := &auth.Group{ + ID: "55555555-5555-4555-8555-555555555557", + Name: "Acct-A Viewers", + Permissions: []auth.Permission{{Action: auth.ActionView, Resource: auth.ResourceRecommendations}}, + AllowedAccounts: []string{scopeDispatchAccountID}, + } + handler, mockStore := newSelfScopeHarness(t, []string{scopeDispatchGroupID}) + mockStore.On("GetGroup", mock.Anything, subset.ID).Return(subset, nil) + mockStore.On("UpdateUser", mock.Anything, mock.AnythingOfType("*auth.User")).Return(nil).Once() + + resp, err := handler.HandleRequest(context.Background(), + selfMembershipRequest(t, []string{scopeDispatchGroupID, subset.ID})) + + require.NoError(t, err) + assert.Equal(t, 200, resp.StatusCode, "body: %s", resp.Body) +}