sec(auth): bound account scope on self-membership changes, both directions

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)
+}

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