From 30aa9b731efbb66b4004f5649b3b19b096557ddf Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 12 Aug 2026 02:50:05 +0200 Subject: [PATCH 1/2] sec(auth): bound account scope on self-membership changes, both directions An actor could raise their own account scope in a single self-edit through PUT /api/users/{id}, by two routes running in opposite directions. Joining a wider group: guardSelfEscalation gated self-added groups on update:users, which the actor already holds, then checked only the carved-out money verbs of the group being joined. The account dimension was never checked, and a default deployment ships exactly one broader group -- the seeded Administrators group, allowed_accounts = ARRAY['*'] (migrations 000024/000057). Leaving the scoping group: guardGroupChange returned early unless the change added a group, so a self-edit that only removed groups was not guarded at all. Dropping the group carrying the restriction collapses the allowed_accounts union to empty, and IsUnrestrictedAccess reads empty as every account. Removing the restriction granted the restriction. guardSelfAccountScope now compares the RESULTING scope against the PRIOR scope, which is what closes both routes: inspecting only what is being added can never see the removal. It reuses accountScopeGap from the group-write ceiling, which treats empty and "*" as unrestricted on EITHER side, so the comparison is not a subset test (the empty set is a subset of everything while meaning the opposite of narrow). The addsNewGroup screen moves from guardGroupChange into guardSelfEscalation, where it now covers only the permission checks. That reach is correct for permissions, which are a plain union that a removal can only shrink, and wrong for accounts, whose union inverts on empty. accountsForGroups resolves the union from a membership list and FAILS CLOSED on any group it cannot load. Its permission-side twin permissionsForGroups skips a missing group safely; here a skipped group widens the result, so swallowing it would compute an unrestricted prior scope and wave every change through -- issue #1748's mechanism in a new place. Verification covers both refusals and, as importantly, both controls: a removal that does not widen scope and a join of a subset-scoped group are still allowed, so a guard that refused every self-membership edit fails the suite. Coverage runs at the service layer and end to end through Handler.HandleRequest -> Router.Route -> the real auth.Service, since a handler-level test has missed router-level bypasses here before. Closes #1756 --- internal/auth/self_account_scope_test.go | 206 ++++++++++++++++++ internal/auth/service_group.go | 31 +++ internal/auth/service_user.go | 95 ++++++-- .../self_account_scope_dispatch_test.go | 164 ++++++++++++++ 4 files changed, 481 insertions(+), 15 deletions(-) create mode 100644 internal/auth/self_account_scope_test.go create mode 100644 internal/server/self_account_scope_dispatch_test.go diff --git a/internal/auth/self_account_scope_test.go b/internal/auth/self_account_scope_test.go new file mode 100644 index 000000000..686f4a385 --- /dev/null +++ b/internal/auth/self_account_scope_test.go @@ -0,0 +1,206 @@ +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) +} diff --git a/internal/auth/service_group.go b/internal/auth/service_group.go index d7f31c2bc..2ba76a34b 100644 --- a/internal/auth/service_group.go +++ b/internal/auth/service_group.go @@ -120,6 +120,37 @@ 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 on any group it cannot resolve, which is the one place it +// deliberately differs from its permission-side twin permissionsForGroups. +// 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 silently skipped group WIDENS the result. The +// union of [] and ["acct-A"] is restricted; lose the group carrying +// ["acct-A"] and it reads as unrestricted. A guard that swallowed the skip +// would compute an unrestricted prior scope and then wave through every +// change -- issue #1748's failure mode reappearing in a new place. +// +// Refusing is safe here in a way it is not for the broadly-consumed +// ResolveAllowedAccounts, which has to keep a user with one stale membership +// working everywhere else: this path is a single self-edit, so the cost of a +// refusal is one rejected request naming the group to clean up. +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 authCtx.SkippedGroups > 0 { + return nil, fmt.Errorf( + "failed to resolve account scope: %d group(s) could not be loaded", authCtx.SkippedGroups) + } + 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) +} From cd20d5250b823e7217f61422272162a18a39e6ae Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 12 Aug 2026 03:23:02 +0200 Subject: [PATCH 2/2] sec(auth): match the self-edit scope ceiling's skip rule to its siblings accountsForGroups refused on ANY group it could not resolve. That is stricter than the widening it guards against, and the extra strictness has no security value while costing real availability. A skip can only widen the union when what SURVIVES is empty, because empty is the value that reads as every account. When the surviving union is non-empty the computed scope is exactly the scope enforcement will apply: BuildAuthContext skips an unresolvable group too, so both sides of the comparison already describe effective access rather than an approximation of it. The refused-for-nothing case is not hypothetical. DeleteGroup drops the groups row and never purges users.group_ids, an array column with no foreign key, so a dangling membership id is the ordinary state after any custom group is deleted. Under refuse-on-any-skip an administrator who is already unrestricted could not remove that dangling id from their own membership: a change that widens nothing, from a principal at maximum scope. In a single-admin deployment nobody could clean it up. The two conditions now match grantCeilingAccounts and ResolveAllowedAccounts exactly: refuse when the surviving union is empty and something was skipped, and refuse when no group resolved at all. The second is unreachable while migration 000057's users_min_one_group CHECK holds, and is carried for the same reason its siblings carry it, an unresolved scope being unknown rather than unrestricted. The new control test pins the availability half so a future tightening cannot quietly reintroduce it. Mutation-verified per test: tightening back to refuse-on-any-skip fails that control alone, and dropping the emptiness condition fails the fail-closed test alone. --- internal/auth/self_account_scope_test.go | 34 ++++++++++++++++++ internal/auth/service_group.go | 45 ++++++++++++++++-------- 2 files changed, 64 insertions(+), 15 deletions(-) diff --git a/internal/auth/self_account_scope_test.go b/internal/auth/self_account_scope_test.go index 686f4a385..401f18c5e 100644 --- a/internal/auth/self_account_scope_test.go +++ b/internal/auth/self_account_scope_test.go @@ -204,3 +204,37 @@ func TestSelfAccountScope_FailsClosedOnUnresolvablePriorGroup(t *testing.T) { 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 2ba76a34b..d4b85b7bc 100644 --- a/internal/auth/service_group.go +++ b/internal/auth/service_group.go @@ -125,28 +125,43 @@ func (s *Service) permissionsForGroups(ctx context.Context, groupIDs []string) ( // membership CHANGE can evaluate the PRIOR and the RESULTING scope with the // same function (see guardSelfAccountScope). // -// It FAILS CLOSED on any group it cannot resolve, which is the one place it -// deliberately differs from its permission-side twin permissionsForGroups. -// 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 silently skipped group WIDENS the result. The -// union of [] and ["acct-A"] is restricted; lose the group carrying -// ["acct-A"] and it reads as unrestricted. A guard that swallowed the skip -// would compute an unrestricted prior scope and then wave through every -// change -- issue #1748's failure mode reappearing in a new place. +// 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. // -// Refusing is safe here in a way it is not for the broadly-consumed -// ResolveAllowedAccounts, which has to keep a user with one stale membership -// working everywhere else: this path is a single self-edit, so the cost of a -// refusal is one rejected request naming the group to clean up. +// 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 authCtx.SkippedGroups > 0 { + if len(authCtx.AllowedAccounts) == 0 && authCtx.SkippedGroups > 0 { return nil, fmt.Errorf( - "failed to resolve account scope: %d group(s) could not be loaded", authCtx.SkippedGroups) + "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 }