sec(auth): bound AllowedAccounts on group writes, the fifth write path

An allowed_accounts-only PUT never reached the ceiling at all.
checkGrantCeiling opens with `if len(requested) == 0 { return nil }`, and
APIUpdateGroupRequest's "empty means not sent" contract makes an
accounts-only request the natural shape to send. Verified by execution with
an actor holding only update:groups in one group scoped to one account:
widening to more accounts, to [] and to ["*"] were all ACCEPTED, while the
control -- widening Permissions[].Constraints.AccountIDs on the same call --
was correctly refused. One account dimension was bounded carefully and its
sibling left open on the same request.

checkAccountCeiling is deliberately a separate call rather than a branch
inside checkGrantCeiling, so it cannot inherit that early return. A write is
in ceiling if it does not widen the group's existing scope, or if it stays
inside the acting principal's own.

Two traps are handled explicitly. Empty and "*" both mean UNRESTRICTED, so
this cannot be a subset test: the empty set is a subset of everything and
means the opposite of narrow. And on create there is no prior scope, where a
nil existing would read as unrestricted and swallow every check -- so
CreateGroupAPI calls checkAccountGrant directly, and an omitted
allowed_accounts is checked too, because it produces an unrestricted group.

Every refusal test sends allowed_accounts with no permissions key. A test
that included permissions would pass with the bug present, since the
permission ceiling would then run and refuse for an unrelated reason.

Also strengthens three mutation kills that rested on a missing stub rather
than on an assertion. M5, M7 and TestUpdateUser_SelfEscalationDenied panicked
on an unstubbed READ downstream of the guard, a kill that disappears the
moment someone adds a permissive stub while tidying fixtures. Those now
register the downstream reads with .Maybe() so removing the guard cannot
panic, leaving the test's own assertions as the only thing that can fail it;
M7 now dies on require.Error. FailsClosedOnGroupLoadError asserts
errors.Is(err, loadErr) rather than message text.

Splits group_ceiling_test.go, which exceeded the repo's 500-line guideline,
by concern rather than by line count: fixtures, the permission ceiling,
blank-field validation, and system_managed. Verified no test was lost -- 707
passing test names before and after, sorted and identical.

Closes #1738.
Refs #1550, #1629, #1730.

Author: cristim

internal/auth/group_account_ceiling_test.go

@@ -0,0 +1,282 @@
+package auth
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+)
+
+// Account-scope ceiling on group writes (issue #1738, folded into #1737).
+//
+// EVERY refusal case here sends allowed_accounts with NO "permissions" key.
+// That shape is the whole finding: checkGrantCeiling returns early when no
+// permissions are sent, so a test that includes permissions alongside would
+// PASS WITH THE BUG PRESENT, because the permission ceiling then runs and
+// refuses for an unrelated reason.
+
+const scopedAccountA = "acct-A"
+
+// scopedActorService wires an actor holding only update:groups, scoped to
+// exactly one cloud account, editing a group scoped to that same account.
+func scopedActorService(t *testing.T, ctx context.Context, mockStore *MockStore) *Service {
+	t.Helper()
+	mockStore.On("GetUserByID", ctx, ceilingActorID).
+		Return(&User{ID: ceilingActorID, GroupIDs: []string{ceilingActorGroupID}}, nil)
+	mockStore.On("GetGroup", ctx, ceilingActorGroupID).Return(&Group{
+		ID:              ceilingActorGroupID,
+		Name:            "Scoped Group Managers",
+		Permissions:     []Permission{{Action: ActionUpdate, Resource: ResourceGroups}},
+		AllowedAccounts: []string{scopedAccountA},
+	}, nil)
+	mockStore.On("GetGroup", ctx, ceilingTargetID).Return(&Group{
+		ID:              ceilingTargetID,
+		Name:            "Team",
+		AllowedAccounts: []string{scopedAccountA},
+	}, nil)
+	return createTestService(mockStore, new(MockEmailSender))
+}
+
+func TestAccountCeiling_RefusesWideningWithNoPermissionsSent(t *testing.T) {
+	ctx := context.Background()
+
+	widenings := []struct {
+		name     string
+		accounts []string
+		wantIn   string
+	}{
+		{"to additional accounts", []string{scopedAccountA, "acct-B"}, `"acct-B"`},
+		// The two unrestricted spellings. An empty list is NOT a narrowing:
+		// IsUnrestrictedAccess reads it as "all accounts".
+		{"to the empty list (unrestricted)", []string{}, "unrestricted"},
+		{"to the wildcard", []string{"*"}, "unrestricted"},
+	}
+
+	for _, w := range widenings {
+		t.Run(w.name, func(t *testing.T) {
+			mockStore := new(MockStore)
+			t.Cleanup(func() { mockStore.AssertExpectations(t) })
+			svc := scopedActorService(t, ctx, mockStore)
+
+			// NOTE: no Permissions field. This is the request shape the bug
+			// lives in; adding permissions here would mask it entirely.
+			result, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID,
+				APIUpdateGroupRequest{AllowedAccounts: w.accounts})
+
+			require.Error(t, err)
+			assert.Nil(t, result)
+			assert.ErrorIs(t, err, ErrPermissionCeiling)
+			assert.Contains(t, err.Error(), w.wantIn)
+			mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything)
+		})
+	}
+}
+
+// Negative control 1: a write that WIDENS the group but stays inside the
+// actor's own scope succeeds.
+//
+// This deliberately widens rather than repeating the group's current scope. A
+// same-value write returns on the not-a-widening branch without ever
+// resolving the actor, so it would prove only that the early return works --
+// not that the actor-scope branch actually grants anything.
+func TestAccountCeiling_AllowsInScopeWidening(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockStore)
+	t.Cleanup(func() { mockStore.AssertExpectations(t) })
+	svc := createTestService(mockStore, new(MockEmailSender))
+
+	// Actor may reach A and B; the group currently reaches only A.
+	mockStore.On("GetUserByID", ctx, ceilingActorID).
+		Return(&User{ID: ceilingActorID, GroupIDs: []string{ceilingActorGroupID}}, nil)
+	mockStore.On("GetGroup", ctx, ceilingActorGroupID).Return(&Group{
+		ID:              ceilingActorGroupID,
+		Permissions:     []Permission{{Action: ActionUpdate, Resource: ResourceGroups}},
+		AllowedAccounts: []string{scopedAccountA, "acct-B"},
+	}, nil)
+	mockStore.On("GetGroup", ctx, ceilingTargetID).Return(&Group{
+		ID: ceilingTargetID, Name: "Team", AllowedAccounts: []string{scopedAccountA},
+	}, nil)
+
+	var saved *Group
+	mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).
+		Run(func(a mock.Arguments) {
+			g, ok := a.Get(1).(*Group)
+			require.True(t, ok)
+			saved = g
+		}).Return(nil).Once()
+
+	_, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID,
+		APIUpdateGroupRequest{AllowedAccounts: []string{scopedAccountA, "acct-B"}})
+
+	require.NoError(t, err)
+	require.NotNil(t, saved)
+	assert.Equal(t, []string{scopedAccountA, "acct-B"}, saved.AllowedAccounts)
+}
+
+// Negative control 1b: repeating the group's current scope is not a widening
+// and must not require an authorization round-trip at all.
+func TestAccountCeiling_UnchangedScopeSkipsActorLookup(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockStore)
+	t.Cleanup(func() { mockStore.AssertExpectations(t) })
+	svc := createTestService(mockStore, new(MockEmailSender))
+
+	mockStore.On("GetGroup", ctx, ceilingTargetID).Return(&Group{
+		ID: ceilingTargetID, Name: "Team", AllowedAccounts: []string{scopedAccountA},
+	}, nil)
+	mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Once()
+
+	_, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID,
+		APIUpdateGroupRequest{AllowedAccounts: []string{scopedAccountA}})
+
+	require.NoError(t, err)
+	mockStore.AssertNotCalled(t, "GetUserByID", mock.Anything, mock.Anything)
+}
+
+// Negative control 2: an UNRESTRICTED actor may still widen a group, so the
+// refusals above come from the actor's scope and not from the check refusing
+// all widenings outright.
+func TestAccountCeiling_UnrestrictedActorMayWiden(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockStore)
+	t.Cleanup(func() { mockStore.AssertExpectations(t) })
+	svc := createTestService(mockStore, new(MockEmailSender))
+
+	stubActorPermissions(ctx, mockStore, adminOnly) // no AllowedAccounts -> unrestricted
+	stubTargetGroup(ctx, mockStore, &Group{
+		ID: ceilingTargetID, Name: "Team", AllowedAccounts: []string{scopedAccountA},
+	})
+	mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Once()
+
+	_, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID,
+		APIUpdateGroupRequest{AllowedAccounts: []string{"*"}})
+	require.NoError(t, err)
+}
+
+// Negative control 3: omitting allowed_accounts entirely (nil, "not sent")
+// must not be treated as a write, or every name-only edit would be refused.
+func TestAccountCeiling_NotSentIsNotAWrite(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockStore)
+	t.Cleanup(func() { mockStore.AssertExpectations(t) })
+	svc := createTestService(mockStore, new(MockEmailSender))
+
+	// Only the target group is stubbed. Neither ceiling resolves the actor
+	// for a request that sends nothing they gate, and asserting that here
+	// (via AssertExpectations on an actor-free mock) is the point: a
+	// name-only edit must not become an authorization round-trip.
+	mockStore.On("GetGroup", ctx, ceilingTargetID).Return(&Group{
+		ID: ceilingTargetID, Name: "Team", AllowedAccounts: []string{scopedAccountA},
+	}, nil)
+
+	var saved *Group
+	mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).
+		Run(func(a mock.Arguments) {
+			g, ok := a.Get(1).(*Group)
+			require.True(t, ok)
+			saved = g
+		}).Return(nil).Once()
+
+	_, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID,
+		APIUpdateGroupRequest{Name: "Renamed"})
+
+	require.NoError(t, err)
+	require.NotNil(t, saved)
+	assert.Equal(t, "Renamed", saved.Name)
+	assert.Equal(t, []string{scopedAccountA}, saved.AllowedAccounts, "scope must be untouched")
+}
+
+// Narrowing is always allowed, whoever the actor is: it cannot widen anything.
+func TestAccountCeiling_NarrowingIsAllowed(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockStore)
+	t.Cleanup(func() { mockStore.AssertExpectations(t) })
+	svc := createTestService(mockStore, new(MockEmailSender))
+
+	mockStore.On("GetUserByID", ctx, ceilingActorID).
+		Return(&User{ID: ceilingActorID, GroupIDs: []string{ceilingActorGroupID}}, nil).Maybe()
+	mockStore.On("GetGroup", ctx, ceilingActorGroupID).Return(&Group{
+		ID:              ceilingActorGroupID,
+		Permissions:     []Permission{{Action: ActionUpdate, Resource: ResourceGroups}},
+		AllowedAccounts: []string{"acct-Z"},
+	}, nil).Maybe()
+	mockStore.On("GetGroup", ctx, ceilingTargetID).Return(&Group{
+		ID: ceilingTargetID, Name: "Team", AllowedAccounts: []string{scopedAccountA, "acct-B"},
+	}, nil)
+	mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Once()
+
+	_, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID,
+		APIUpdateGroupRequest{AllowedAccounts: []string{scopedAccountA}})
+	require.NoError(t, err, "narrowing the group's own scope is never a widening")
+}
+
+// CreateGroupAPI has no existing scope to compare against, so any
+// allowed_accounts value must sit inside the creator's own.
+func TestAccountCeiling_CreateIsBounded(t *testing.T) {
+	ctx := context.Background()
+
+	t.Run("out of scope is refused", func(t *testing.T) {
+		mockStore := new(MockStore)
+		t.Cleanup(func() { mockStore.AssertExpectations(t) })
+		svc := createTestService(mockStore, new(MockEmailSender))
+
+		mockStore.On("GetUserByID", ctx, ceilingActorID).
+			Return(&User{ID: ceilingActorID, GroupIDs: []string{ceilingActorGroupID}}, nil)
+		mockStore.On("GetGroup", ctx, ceilingActorGroupID).Return(&Group{
+			ID:              ceilingActorGroupID,
+			Permissions:     []Permission{{Action: ActionCreate, Resource: ResourceGroups}},
+			AllowedAccounts: []string{scopedAccountA},
+		}, nil)
+
+		_, err := svc.CreateGroupAPI(ctx, ceilingActorID, APICreateGroupRequest{
+			Name:            "Wider",
+			AllowedAccounts: []string{"acct-B"},
+		})
+
+		require.Error(t, err)
+		assert.ErrorIs(t, err, ErrPermissionCeiling)
+		mockStore.AssertNotCalled(t, "CreateGroup", mock.Anything, mock.Anything)
+	})
+
+	t.Run("in scope is allowed", func(t *testing.T) {
+		mockStore := new(MockStore)
+		t.Cleanup(func() { mockStore.AssertExpectations(t) })
+		svc := createTestService(mockStore, new(MockEmailSender))
+
+		mockStore.On("GetUserByID", ctx, ceilingActorID).
+			Return(&User{ID: ceilingActorID, GroupIDs: []string{ceilingActorGroupID}}, nil)
+		mockStore.On("GetGroup", ctx, ceilingActorGroupID).Return(&Group{
+			ID:              ceilingActorGroupID,
+			Permissions:     []Permission{{Action: ActionCreate, Resource: ResourceGroups}},
+			AllowedAccounts: []string{scopedAccountA},
+		}, nil)
+		mockStore.On("CreateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Once()
+
+		_, err := svc.CreateGroupAPI(ctx, ceilingActorID, APICreateGroupRequest{
+			Name:            "Narrower",
+			AllowedAccounts: []string{scopedAccountA},
+		})
+		require.NoError(t, err)
+	})
+}
+
+// Fail closed: an actor whose scope cannot be resolved cannot widen.
+func TestAccountCeiling_FailsClosedOnUnidentifiedActor(t *testing.T) {
+	ctx := context.Background()
+	mockStore := new(MockStore)
+	t.Cleanup(func() { mockStore.AssertExpectations(t) })
+	svc := createTestService(mockStore, new(MockEmailSender))
+
+	stubTargetGroup(ctx, mockStore, &Group{
+		ID: ceilingTargetID, Name: "Team", AllowedAccounts: []string{scopedAccountA},
+	})
+
+	_, err := svc.UpdateGroupAPI(ctx, "", ceilingTargetID,
+		APIUpdateGroupRequest{AllowedAccounts: []string{"*"}})
+
+	require.Error(t, err)
+	assert.ErrorIs(t, err, ErrPermissionCeiling)
+	mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything)
+}

internal/auth/group_ceiling.go

@@ -138,6 +138,104 @@ func validateRequestedPermissions(requested []Permission) error {
 	return nil
 }
 
+// grantCeilingAccounts returns the account scope a group write is measured
+// against: the union of the acting principal's groups' AllowedAccounts.
+// Fails closed on an unidentified actor or a resolution error.
+//
+// The stateless admin API key has no user row and is unrestricted everywhere
+// else (see getAllowedAccounts in internal/api), so it is unrestricted here.
+func (s *Service) grantCeilingAccounts(ctx context.Context, actorUserID string) ([]string, error) {
+	if actorUserID == "" {
+		return nil, fmt.Errorf("%w: the acting user could not be identified", ErrPermissionCeiling)
+	}
+	if actorUserID == AdminAPIKeyActorID {
+		return nil, nil
+	}
+	authCtx, err := s.BuildAuthContext(ctx, actorUserID)
+	if err != nil {
+		return nil, fmt.Errorf("%w: could not resolve the acting user's account scope: %w", ErrPermissionCeiling, err)
+	}
+	return authCtx.AllowedAccounts, nil
+}
+
+// checkAccountCeiling bounds the OTHER account dimension of a group write.
+//
+// This is deliberately a separate call from checkGrantCeiling rather than a
+// branch inside it. checkGrantCeiling returns early when no permissions are
+// sent, and APIUpdateGroupRequest's "empty means not sent" contract makes an
+// allowed_accounts-only PUT the natural shape -- so folding this in would
+// leave exactly the request that widens account scope unchecked. That was the
+// live gap: widening AllowedAccounts to more accounts, to [] or to ["*"] was
+// accepted, while widening Permissions[].Constraints.AccountIDs on the SAME
+// call was correctly refused.
+//
+// requested == nil means "not sent" and is left alone. A write is in ceiling
+// if it does not widen the group's existing scope, or if it stays within the
+// acting principal's own scope.
+func (s *Service) checkAccountCeiling(ctx context.Context, actorUserID string, requested, existing []string) error {
+	if requested == nil {
+		return nil
+	}
+	// Not a widening of what the group already had -- safe whoever the actor
+	// is. Only reachable on update: a new group has no prior scope, and nil
+	// existing would read as UNRESTRICTED here and swallow every check, which
+	// is why CreateGroupAPI calls checkAccountGrant directly instead.
+	if accountScopeGap(existing, requested) == "" {
+		return nil
+	}
+	return s.checkAccountGrant(ctx, actorUserID, requested)
+}
+
+// checkAccountGrant requires requested to sit inside the acting principal's
+// own account scope. This is the create-path entry point, where there is no
+// prior scope to compare against, so every value is a grant.
+//
+// A nil requested is checked too, and deliberately: on create, omitting
+// allowed_accounts produces an UNRESTRICTED group (IsUnrestrictedAccess reads
+// empty as "all accounts"), so for a scoped actor that is the widest possible
+// grant rather than a no-op.
+func (s *Service) checkAccountGrant(ctx context.Context, actorUserID string, requested []string) error {
+	actorAccounts, err := s.grantCeilingAccounts(ctx, actorUserID)
+	if err != nil {
+		return err
+	}
+	if gap := accountScopeGap(actorAccounts, requested); gap != "" {
+		return fmt.Errorf(
+			"%w: cannot grant %s because your own account scope does not include it",
+			ErrPermissionCeiling, gap)
+	}
+	return nil
+}
+
+// accountScopeGap returns a description of the first way requested reaches
+// beyond outer, or "" when outer covers it entirely.
+//
+// Empty and "*" both mean UNRESTRICTED on either side (IsUnrestrictedAccess),
+// which is why this cannot be a plain subset test: the empty set is a subset
+// of everything but means the opposite of narrow. An unrestricted request
+// against a restricted holder is the widening this exists to catch.
+//
+// Comparison is exact, matching MatchesAccount. Case folding would only make
+// the check more permissive, which is the wrong direction for a ceiling.
+func accountScopeGap(outer, requested []string) string {
+	if IsUnrestrictedAccess(outer) {
+		return ""
+	}
+	if IsUnrestrictedAccess(requested) {
+		return "unrestricted access to all cloud accounts"
+	}
+	permitted := make(map[string]bool, len(outer))
+	for _, a := range outer {
+		permitted[strings.TrimSpace(a)] = true
+	}
+	for _, a := range requested {
+		if !permitted[strings.TrimSpace(a)] {
+			return fmt.Sprintf("access to cloud account %q", a)
+		}
+	}
+	return ""
+}
+
 // grantCeilingAllows reports whether actorPerms holds req in full. Action and
 // resource matching mirrors permissionsAllow exactly, including the admin:*
 // carve-out, so the ceiling can never be looser than enforcement. It adds one