From 6318fc4ce7df96a5ab0a1c17c6dac4754907233e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 05:10:18 +0200 Subject: [PATCH 1/4] sec(auth): fail closed when a user's account scope cannot be established A user scoped to specific cloud accounts got unrestricted access to ALL of them whenever their group memberships failed to resolve. An empty allowed-accounts list means UNRESTRICTED (IsUnrestrictedAccess), a deliberate backward-compat default so a group with no allowed_accounts configured grants full access. But collectGroupsAndAccounts also skips a group it cannot load -- pgx.ErrNoRows, or a store returning (nil, nil) -- so a resolution failure produced the SAME empty value. Absence and unrestricted shared a representation, and every failure silently granted everything. No error, no log. Verified by execution before fixing: scoped user, group loads fine (control) -> unrestricted=false scoped user, group missing (ErrNoRows) -> unrestricted=TRUE scoped user, group returns (nil, nil) -> unrestricted=TRUE Three producers of the empty value are closed: 1. h.auth == nil in getAllowedAccounts returned (nil, nil). Auth components must fail closed when nil, never fall through. 2. a group that fails to load with pgx.ErrNoRows 3. a store path returning (nil, nil) 2 and 3 are closed at the single point where the scope is produced rather than per-caller: ResolveAllowedAccounts requires at least one group to have actually resolved. Partial resolution still under-reports scope, which makes access STRICTER, so only total failure had to be refused. The design constraint that shaped this: there are THREE legitimate unrestricted principals, not two, and the third shares its representation with the failures. The stateless admin API key resolves to empty, an Administrators member carries "*", and a group with NO allowed_accounts configured also resolves to empty. Making absence fail closed blindly would have broken every legacy group. Requiring a resolved GROUP rather than a resolved account list separates them cleanly -- verified across all six cases. Every failure test is paired with a control proving a legitimate unrestricted principal still passes, because a fix that refused everyone would satisfy a refusal-only suite. Mutation-verified per guard: removing the no-group- resolved guard kills only its own test, reverting the nil-auth guard kills only its own, and an inverse mutation that refuses everyone kills the legitimate-principal controls. Closes #1748. --- .../api/account_scope_fail_closed_test.go | 158 +++++++++++++++++ internal/api/handler.go | 6 +- .../auth/account_scope_fail_closed_test.go | 163 ++++++++++++++++++ internal/auth/service_group.go | 31 ++++ internal/server/app.go | 9 +- 5 files changed, 361 insertions(+), 6 deletions(-) create mode 100644 internal/api/account_scope_fail_closed_test.go create mode 100644 internal/auth/account_scope_fail_closed_test.go diff --git a/internal/api/account_scope_fail_closed_test.go b/internal/api/account_scope_fail_closed_test.go new file mode 100644 index 000000000..b05c4b343 --- /dev/null +++ b/internal/api/account_scope_fail_closed_test.go @@ -0,0 +1,158 @@ +package api + +import ( + "context" + "errors" + "testing" + + "github.com/LeanerCloud/CUDly/internal/auth" + "github.com/LeanerCloud/CUDly/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// The API-side half of issue #1748. +// +// getAllowedAccounts returned (nil, nil) when h.auth was nil, and an empty +// list means UNRESTRICTED (IsUnrestrictedAccess) -- so a handler running +// without an auth service granted every caller access to every cloud account. +// Auth components must fail closed when nil, never fall through. + +const ( + scopeSessionUser = "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + scopeAcctA = "11111111-1111-4111-8111-111111111111" + scopeAcctB = "22222222-2222-4222-8222-222222222222" +) + +// ---------- the failure modes: each must REFUSE ---------- + +func TestGetAllowedAccounts_FailsClosedWhenAuthMissing(t *testing.T) { + ctx := context.Background() + h := &Handler{auth: nil} + + got, err := h.getAllowedAccounts(ctx, &Session{UserID: scopeSessionUser}) + + require.Error(t, err, "a nil auth service must refuse, not grant unrestricted access") + assert.Nil(t, got) + assert.Contains(t, err.Error(), "cannot establish account scope") +} + +// The error from the resolver must reach the caller rather than being +// flattened into an empty (= unrestricted) list. +func TestGetAllowedAccounts_PropagatesResolverFailure(t *testing.T) { + ctx := context.Background() + m := new(MockAuthService) + t.Cleanup(func() { m.AssertExpectations(t) }) + + boom := errors.New("account scope could not be established for user: no group resolved") + m.On("GetAllowedAccountsAPI", ctx, scopeSessionUser).Return([]string(nil), boom) + + h := &Handler{auth: m} + got, err := h.getAllowedAccounts(ctx, &Session{UserID: scopeSessionUser}) + + require.Error(t, err) + assert.Nil(t, got) +} + +// End-to-end through the shared scoping seam every scoped handler uses: an +// unestablishable scope must not turn into access. +func TestRequireAccountAccess_RefusesWhenScopeUnestablishable(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + m := new(MockAuthService) + t.Cleanup(func() { m.AssertExpectations(t) }) + + mockStore.On("GetCloudAccount", ctx, scopeAcctB). + Return(&config.CloudAccount{ID: scopeAcctB, Name: "other"}, nil) + m.On("GetAllowedAccountsAPI", ctx, scopeSessionUser). + Return([]string(nil), errors.New("no group resolved")) + + h := &Handler{config: mockStore, auth: m} + got, err := h.requireAccountAccess(ctx, &Session{UserID: scopeSessionUser}, scopeAcctB) + + require.Error(t, err, "an unestablishable scope must not grant account access") + assert.Nil(t, got) +} + +// ---------- the controls: legitimate principals must still PASS ---------- + +// The stateless admin API key has no user row and is unrestricted by design. +// Its unrestricted-ness is expressed POSITIVELY (keyed on the sentinel), not +// by absence, so making absence fail closed must not break it. +func TestGetAllowedAccounts_AdminAPIKeyStillUnrestricted(t *testing.T) { + ctx := context.Background() + h := &Handler{auth: new(MockAuthService)} + + got, err := h.getAllowedAccounts(ctx, &Session{UserID: apiKeyAdminUserID}) + + require.NoError(t, err, "the admin API key must remain unrestricted") + assert.True(t, auth.IsUnrestrictedAccess(got)) +} + +// The other two legitimate unrestricted principals, resolved through the auth +// service: an Administrators member carrying "*", and a group with no +// allowed_accounts configured (the backward-compat default). The third is the +// one that shares its representation with the failure modes, which is why it +// is pinned here. +func TestGetAllowedAccounts_LegitimateUnrestrictedPrincipalsPass(t *testing.T) { + ctx := context.Background() + + for _, tc := range []struct { + name string + resolved []string + }{ + {"Administrators member carrying the * wildcard", []string{"*"}}, + {"group with no allowed_accounts configured", []string{}}, + } { + t.Run(tc.name, func(t *testing.T) { + m := new(MockAuthService) + t.Cleanup(func() { m.AssertExpectations(t) }) + m.On("GetAllowedAccountsAPI", ctx, scopeSessionUser).Return(tc.resolved, nil) + + h := &Handler{auth: m} + got, err := h.getAllowedAccounts(ctx, &Session{UserID: scopeSessionUser}) + + require.NoError(t, err, "a successfully resolved scope must not be refused") + assert.True(t, auth.IsUnrestrictedAccess(got), + "a successful resolution to an empty/wildcard scope still means all accounts") + }) + } +} + +// And the scoped principal still gets exactly its own accounts -- proving the +// fix did not collapse everything into either extreme. +func TestRequireAccountAccess_ScopedPrincipalUnchanged(t *testing.T) { + ctx := context.Background() + + t.Run("in-scope account is reachable", func(t *testing.T) { + mockStore := new(MockConfigStore) + m := new(MockAuthService) + t.Cleanup(func() { m.AssertExpectations(t) }) + mockStore.On("GetCloudAccount", ctx, scopeAcctA). + Return(&config.CloudAccount{ID: scopeAcctA, Name: "mine"}, nil) + m.On("GetAllowedAccountsAPI", ctx, scopeSessionUser).Return([]string{scopeAcctA}, nil) + + h := &Handler{config: mockStore, auth: m} + got, err := h.requireAccountAccess(ctx, &Session{UserID: scopeSessionUser}, scopeAcctA) + require.NoError(t, err) + require.NotNil(t, got) + }) + + t.Run("out-of-scope account is refused", func(t *testing.T) { + mockStore := new(MockConfigStore) + m := new(MockAuthService) + t.Cleanup(func() { m.AssertExpectations(t) }) + mockStore.On("GetCloudAccount", ctx, scopeAcctB). + Return(&config.CloudAccount{ID: scopeAcctB, Name: "other"}, nil) + m.On("GetAllowedAccountsAPI", ctx, scopeSessionUser).Return([]string{scopeAcctA}, nil) + + h := &Handler{config: mockStore, auth: m} + got, err := h.requireAccountAccess(ctx, &Session{UserID: scopeSessionUser}, scopeAcctB) + require.Error(t, err) + assert.Nil(t, got) + assert.ErrorIs(t, err, errNotFound) + }) + + _ = mock.Anything +} diff --git a/internal/api/handler.go b/internal/api/handler.go index 5da91e565..d61f74986 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -529,7 +529,11 @@ func (h *Handler) getAllowedAccounts(ctx context.Context, session *Session) ([]s return nil, nil // stateless admin API key = all access } if h.auth == nil { - return nil, nil + // Fail closed. Returning an empty list here meant "all accounts", so a + // handler running without an auth service granted every caller access + // to every cloud account (issue #1748). Auth components must fail + // closed when nil, never fall through. + return nil, fmt.Errorf("authentication service not configured: cannot establish account scope") } return h.auth.GetAllowedAccountsAPI(ctx, session.UserID) } diff --git a/internal/auth/account_scope_fail_closed_test.go b/internal/auth/account_scope_fail_closed_test.go new file mode 100644 index 000000000..0c4302741 --- /dev/null +++ b/internal/auth/account_scope_fail_closed_test.go @@ -0,0 +1,163 @@ +package auth + +import ( + "context" + "errors" + "testing" + + "github.com/jackc/pgx/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Account scope must fail CLOSED when it cannot be established (issue #1748). +// +// The trap this guards: an empty result means UNRESTRICTED +// (IsUnrestrictedAccess), a deliberate backward-compat default. But +// collectGroupsAndAccounts silently skips a group it cannot load, so a +// resolution failure produced the SAME empty value and granted access to every +// cloud account. Absence and unrestricted shared a representation. +// +// Both halves are asserted here. A refusal-only suite would be passed by an +// implementation that refuses everyone, so every failure case is paired with a +// control proving a legitimate unrestricted principal still gets through. + +const scopeUserID = "99999999-9999-4999-8999-999999999999" + +func scopeStore(t *testing.T) (*MockStore, *Service) { + t.Helper() + ms := new(MockStore) + return ms, createTestService(ms, new(MockEmailSender)) +} + +// ---------- the failure modes: each must REFUSE ---------- + +func TestResolveAllowedAccounts_FailsClosed(t *testing.T) { + ctx := context.Background() + + for _, tc := range []struct { + name string + setup func(*MockStore) + }{ + {"group missing (pgx.ErrNoRows)", func(ms *MockStore) { + ms.On("GetUserByID", ctx, scopeUserID). + Return(&User{ID: scopeUserID, GroupIDs: []string{"g1"}}, nil) + ms.On("GetGroup", ctx, "g1").Return(nil, pgx.ErrNoRows) + }}, + {"group resolves to (nil, nil)", func(ms *MockStore) { + ms.On("GetUserByID", ctx, scopeUserID). + Return(&User{ID: scopeUserID, GroupIDs: []string{"g1"}}, nil) + ms.On("GetGroup", ctx, "g1").Return(nil, nil) + }}, + {"several groups, none resolve", func(ms *MockStore) { + ms.On("GetUserByID", ctx, scopeUserID). + Return(&User{ID: scopeUserID, GroupIDs: []string{"g1", "g2"}}, nil) + ms.On("GetGroup", ctx, "g1").Return(nil, pgx.ErrNoRows) + ms.On("GetGroup", ctx, "g2").Return(nil, nil) + }}, + {"user has no groups at all", func(ms *MockStore) { + ms.On("GetUserByID", ctx, scopeUserID).Return(&User{ID: scopeUserID}, nil) + }}, + } { + t.Run(tc.name, func(t *testing.T) { + ms, svc := scopeStore(t) + t.Cleanup(func() { ms.AssertExpectations(t) }) + tc.setup(ms) + + got, err := svc.ResolveAllowedAccounts(ctx, scopeUserID) + + require.Error(t, err, "an unestablishable scope must be refused, not treated as unrestricted") + assert.Nil(t, got) + assert.Contains(t, err.Error(), "could not be established") + // The property in its own terms: whatever comes back must not be + // readable as "all accounts". + assert.False(t, IsUnrestrictedAccess(got) && err == nil, + "a failed resolution must never yield an unrestricted scope") + }) + } +} + +// A store error still propagates rather than being converted into a scope. +func TestResolveAllowedAccounts_PropagatesStoreError(t *testing.T) { + ctx := context.Background() + ms, svc := scopeStore(t) + t.Cleanup(func() { ms.AssertExpectations(t) }) + + boom := errors.New("db unavailable") + ms.On("GetUserByID", ctx, scopeUserID).Return(nil, boom) + + got, err := svc.ResolveAllowedAccounts(ctx, scopeUserID) + require.Error(t, err) + assert.Nil(t, got) + assert.ErrorIs(t, err, boom) +} + +// ---------- the controls: legitimate principals must still PASS ---------- + +// Three distinct principals are legitimately unrestricted, and the third is +// the one that collides with the failure modes: a group with no +// allowed_accounts configured resolves to the SAME empty list. Making absence +// fail closed without this control would have broken every legacy group. +func TestResolveAllowedAccounts_LegitimatePrincipalsStillResolve(t *testing.T) { + ctx := context.Background() + + for _, tc := range []struct { + name string + group *Group + wantAccounts []string + wantUnrestricted bool + }{ + { + name: "group carrying the * wildcard (Administrators)", + group: &Group{ID: "g1", AllowedAccounts: []string{"*"}}, + wantAccounts: []string{"*"}, + wantUnrestricted: true, + }, + { + name: "group with NO allowed_accounts configured (legacy default)", + group: &Group{ID: "g1"}, + wantAccounts: []string{}, + wantUnrestricted: true, + }, + { + name: "group scoped to one account stays scoped", + group: &Group{ID: "g1", AllowedAccounts: []string{"acct-A"}}, + wantAccounts: []string{"acct-A"}, + wantUnrestricted: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + ms, svc := scopeStore(t) + t.Cleanup(func() { ms.AssertExpectations(t) }) + ms.On("GetUserByID", ctx, scopeUserID). + Return(&User{ID: scopeUserID, GroupIDs: []string{"g1"}}, nil) + ms.On("GetGroup", ctx, "g1").Return(tc.group, nil) + + got, err := svc.ResolveAllowedAccounts(ctx, scopeUserID) + + require.NoError(t, err, "a resolvable scope must not be refused") + assert.ElementsMatch(t, tc.wantAccounts, got) + assert.Equal(t, tc.wantUnrestricted, IsUnrestrictedAccess(got)) + }) + } +} + +// A partially-resolvable membership keeps working and reports only what +// resolved. Under-reporting scope makes access STRICTER, so it is safe; only +// total failure had to be refused. +func TestResolveAllowedAccounts_PartialResolutionIsAllowedAndNarrower(t *testing.T) { + ctx := context.Background() + ms, svc := scopeStore(t) + t.Cleanup(func() { ms.AssertExpectations(t) }) + + ms.On("GetUserByID", ctx, scopeUserID). + Return(&User{ID: scopeUserID, GroupIDs: []string{"g1", "g2"}}, nil) + ms.On("GetGroup", ctx, "g1").Return(&Group{ID: "g1", AllowedAccounts: []string{"acct-A"}}, nil) + ms.On("GetGroup", ctx, "g2").Return(nil, pgx.ErrNoRows) + + got, err := svc.ResolveAllowedAccounts(ctx, scopeUserID) + + require.NoError(t, err) + assert.Equal(t, []string{"acct-A"}, got) + assert.False(t, IsUnrestrictedAccess(got), "a partial resolution must not widen to all accounts") +} diff --git a/internal/auth/service_group.go b/internal/auth/service_group.go index ce23b49ff..2d117fb7a 100644 --- a/internal/auth/service_group.go +++ b/internal/auth/service_group.go @@ -150,6 +150,37 @@ func (s *Service) collectGroupsAndAccounts(ctx context.Context, authCtx *AuthCon return nil } +// ResolveAllowedAccounts returns the cloud accounts a user may access, and +// FAILS CLOSED when that scope cannot be established (issue #1748). +// +// The subtlety this exists for: an empty result means UNRESTRICTED +// (IsUnrestrictedAccess), a deliberate backward-compat default so a group with +// no allowed_accounts configured grants full access. But +// collectGroupsAndAccounts also skips a group it cannot load -- pgx.ErrNoRows, +// or a store returning (nil, nil) -- so a user whose groups all fail to +// resolve produced the SAME empty value. Absence and unrestricted shared a +// representation, and every failure silently granted access to every account. +// +// Requiring at least one group to have actually resolved separates the two +// cleanly. Verified across all six cases: a group with allowed_accounts=["*"], +// a group with none configured (the legacy default), and a scoped group all +// resolve at least one group and keep working; a missing group, a (nil, nil) +// group, and a user with no groups at all resolve none and are refused. +// +// A user with no groups holds no permissions either, so refusing them here +// costs nothing and closes the same hole from the other side. +func (s *Service) ResolveAllowedAccounts(ctx context.Context, userID string) ([]string, error) { + authCtx, err := s.BuildAuthContext(ctx, userID) + if err != nil { + return nil, err + } + if len(authCtx.Groups) == 0 { + return nil, fmt.Errorf( + "account scope could not be established for user %s: no group resolved", userID) + } + return authCtx.AllowedAccounts, nil +} + // GetAuthContext is an alias for BuildAuthContext for backward compatibility. func (s *Service) GetAuthContext(ctx context.Context, userID string) (*AuthContext, error) { return s.BuildAuthContext(ctx, userID) diff --git a/internal/server/app.go b/internal/server/app.go index fcdbfe845..0a0e4e88e 100644 --- a/internal/server/app.go +++ b/internal/server/app.go @@ -1157,11 +1157,10 @@ func (a *authServiceAdapter) GetUserPermissionsAPI(ctx context.Context, userID s // Account access. func (a *authServiceAdapter) GetAllowedAccountsAPI(ctx context.Context, userID string) ([]string, error) { - authCtx, err := a.service.BuildAuthContext(ctx, userID) - if err != nil { - return nil, err - } - return authCtx.AllowedAccounts, nil + // ResolveAllowedAccounts rather than BuildAuthContext: it fails closed when + // the scope cannot be established, instead of returning the empty list that + // IsUnrestrictedAccess reads as "all accounts" (issue #1748). + return a.service.ResolveAllowedAccounts(ctx, userID) } // CSRF validation. From 86d1164b9788922a8e1f3d866aabaaf51d8b79db Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 06:28:07 +0200 Subject: [PATCH 2/4] sec(auth): refuse a partial group resolution that widens account scope The previous guard closed only TOTAL resolution failure. It rested on a premise that is false for AllowedAccounts: "partial resolution under-reports scope, which makes access stricter". AllowedAccounts is a UNION in which the empty set means EVERYTHING, so dropping a contributing group does not narrow it. The union of [] and ["acct-A"] is restricted; lose the group carrying ["acct-A"] and it collapses to [], which reads as every account. Dropping a group WIDENS. Reproduced by execution with a two-group actor, one granting update:groups with no allowed_accounts, one carrying the restriction: baseline: both groups resolve scope=[acct-A] unrestricted=false PARTIAL: restricting group ErrNoRows scope=[] unrestricted=TRUE PARTIAL: restricting group (nil,nil) scope=[] unrestricted=TRUE TOTAL failure (the old guard's case) refused The open half was also the more reachable one: under total failure the actor loses every permission too, so requirePermission denies at the gate before scope is consulted. A single-group configuration cannot exhibit this, which is why the earlier "verified across all six cases" found nothing -- all six were single-group. The guard is now: refuse when the union is empty AND at least one group was skipped. That targets exactly the widening. It deliberately does NOT refuse on any unresolved group, which would reverse the intentional "group was deleted; skip it rather than failing the entire request" behaviour and lock out a user with one stale membership everywhere until an admin cleaned up -- an unconditional availability regression traded for a conditional security hole, on a path with ~37 consumers. The skip count is recorded in collectGroupsAndAccounts at the point of skipping, NOT derived by comparing len(Groups) to len(User.GroupIDs): GroupIDs may contain duplicates, so that comparison reports phantom skips and refuses legitimate principals. Covered by a regression test. TestResolveAllowedAccounts_PartialResolutionIsAllowedAndNarrower asserted the false invariant and passed with the bug present. It is INVERTED rather than supplemented: a test encoding a false invariant is worse than no test, because it tells the next reader the case is covered. Its replacement uses the two-group shape and fails against the old guard. Preconditions, stated precisely: all seven seeded groups ship allowed_accounts = ARRAY['*'], which is already unrestricted, so nothing widens by default and this is not exploitable out of the box. It is reachable on operator-configured groups, because CreateGroupAPI accepts an omitted allowed_accounts (yielding []) and UpdateGroupAPI accepts an explicit [] -- the documented backward-compat default. A deployment with one such group alongside per-account scoped groups is exposed, on the same triggers as the rest of #1748. Refs #1748. --- .../auth/account_scope_fail_closed_test.go | 110 +++++++++++++++++- internal/auth/service_group.go | 60 +++++++--- internal/auth/types.go | 7 ++ 3 files changed, 155 insertions(+), 22 deletions(-) diff --git a/internal/auth/account_scope_fail_closed_test.go b/internal/auth/account_scope_fail_closed_test.go index 0c4302741..9f80e0f92 100644 --- a/internal/auth/account_scope_fail_closed_test.go +++ b/internal/auth/account_scope_fail_closed_test.go @@ -142,10 +142,90 @@ func TestResolveAllowedAccounts_LegitimatePrincipalsStillResolve(t *testing.T) { } } -// A partially-resolvable membership keeps working and reports only what -// resolved. Under-reporting scope makes access STRICTER, so it is safe; only -// total failure had to be refused. -func TestResolveAllowedAccounts_PartialResolutionIsAllowedAndNarrower(t *testing.T) { +// THE MULTI-GROUP WIDENING (issue #1748, the case an earlier version of this +// guard missed). +// +// This test replaces one that asserted the opposite -- that partial resolution +// is "allowed and narrower". That invariant is FALSE and the old test passed +// with the bug present, because its surviving group carried the restriction. +// A test encoding a false invariant is worse than no test: it tells the next +// reader the case is covered. +// +// The configuration needs TWO groups, which is why a six-case single-group +// verification could not find it: one granting a permission with NO +// allowed_accounts (unrestricted on its own), one carrying the restriction. +// The union is restricted; lose the restricting group and it collapses to +// empty, which reads as EVERY account. Dropping a group WIDENS. +func TestResolveAllowedAccounts_PartialResolutionThatWidensIsRefused(t *testing.T) { + ctx := context.Background() + const permGroup, scopeGroup = "g-perm", "g-scope" + + for _, tc := range []struct { + name string + scopeResp func(*MockStore) + }{ + {"restricting group missing (ErrNoRows)", func(ms *MockStore) { + ms.On("GetGroup", ctx, scopeGroup).Return(nil, pgx.ErrNoRows) + }}, + {"restricting group resolves to (nil, nil)", func(ms *MockStore) { + ms.On("GetGroup", ctx, scopeGroup).Return(nil, nil) + }}, + } { + t.Run(tc.name, func(t *testing.T) { + ms, svc := scopeStore(t) + t.Cleanup(func() { ms.AssertExpectations(t) }) + ms.On("GetUserByID", ctx, scopeUserID). + Return(&User{ID: scopeUserID, GroupIDs: []string{permGroup, scopeGroup}}, nil) + // Survives, and contributes NO accounts -- unrestricted alone. + ms.On("GetGroup", ctx, permGroup).Return(&Group{ + ID: permGroup, + Permissions: []Permission{{Action: ActionUpdate, Resource: ResourceGroups}}, + }, nil) + tc.scopeResp(ms) + + got, err := svc.ResolveAllowedAccounts(ctx, scopeUserID) + + require.Error(t, err, + "losing the restricting group collapses the union to empty = ALL accounts; that must be refused") + assert.Nil(t, got) + assert.Contains(t, err.Error(), "could not be established") + assert.False(t, IsUnrestrictedAccess(got) && err == nil) + }) + } +} + +// The baseline control for the case above: with BOTH groups resolving, the +// same actor is correctly restricted. Without this, a guard that refused the +// two-group shape outright would pass the test above. +func TestResolveAllowedAccounts_MultiGroupBaselineStaysRestricted(t *testing.T) { + ctx := context.Background() + const permGroup, scopeGroup = "g-perm", "g-scope" + + ms, svc := scopeStore(t) + t.Cleanup(func() { ms.AssertExpectations(t) }) + ms.On("GetUserByID", ctx, scopeUserID). + Return(&User{ID: scopeUserID, GroupIDs: []string{permGroup, scopeGroup}}, nil) + ms.On("GetGroup", ctx, permGroup).Return(&Group{ + ID: permGroup, + Permissions: []Permission{{Action: ActionUpdate, Resource: ResourceGroups}}, + }, nil) + ms.On("GetGroup", ctx, scopeGroup).Return(&Group{ + ID: scopeGroup, AllowedAccounts: []string{"acct-A"}, + }, nil) + + got, err := svc.ResolveAllowedAccounts(ctx, scopeUserID) + + require.NoError(t, err) + assert.Equal(t, []string{"acct-A"}, got) + assert.False(t, IsUnrestrictedAccess(got)) +} + +// Deleted-group tolerance is PRESERVED for a restricted principal: a +// non-empty union cannot have been widened by the loss, so the skip is +// absorbed rather than refused. This is the behaviour option 1 ("refuse on any +// unresolved group") would have destroyed, locking out every user with one +// stale membership. +func TestResolveAllowedAccounts_SkippedGroupToleratedWhenUnionStaysRestricted(t *testing.T) { ctx := context.Background() ms, svc := scopeStore(t) t.Cleanup(func() { ms.AssertExpectations(t) }) @@ -157,7 +237,25 @@ func TestResolveAllowedAccounts_PartialResolutionIsAllowedAndNarrower(t *testing got, err := svc.ResolveAllowedAccounts(ctx, scopeUserID) - require.NoError(t, err) + require.NoError(t, err, "a stale membership must not lock out a restricted principal") assert.Equal(t, []string{"acct-A"}, got) - assert.False(t, IsUnrestrictedAccess(got), "a partial resolution must not widen to all accounts") + assert.False(t, IsUnrestrictedAccess(got)) +} + +// Duplicate GroupIDs must not be mistaken for skips. Detecting skips by +// comparing len(Groups) to len(GroupIDs) would see 2 ids resolving to 1 group +// and refuse this legitimate principal. +func TestResolveAllowedAccounts_DuplicateGroupIDsAreNotSkips(t *testing.T) { + ctx := context.Background() + ms, svc := scopeStore(t) + t.Cleanup(func() { ms.AssertExpectations(t) }) + + ms.On("GetUserByID", ctx, scopeUserID). + Return(&User{ID: scopeUserID, GroupIDs: []string{"g1", "g1"}}, nil) + ms.On("GetGroup", ctx, "g1").Return(&Group{ID: "g1"}, nil) // no allowed_accounts + + got, err := svc.ResolveAllowedAccounts(ctx, scopeUserID) + + require.NoError(t, err, "duplicate memberships are not unresolved groups") + assert.True(t, IsUnrestrictedAccess(got)) } diff --git a/internal/auth/service_group.go b/internal/auth/service_group.go index 2d117fb7a..11e617fcd 100644 --- a/internal/auth/service_group.go +++ b/internal/auth/service_group.go @@ -126,13 +126,16 @@ func (s *Service) collectGroupsAndAccounts(ctx context.Context, authCtx *AuthCon group, err := s.store.GetGroup(ctx, groupID) if err != nil { if errors.Is(err, pgx.ErrNoRows) { - // Group was deleted; skip it rather than failing the entire request. + // Group was deleted; skip it rather than failing the entire + // request. RECORD the skip: a caller reasoning about account + // scope must know the union is incomplete (issue #1748). + authCtx.SkippedGroups++ continue } return fmt.Errorf("fetching group %s: %w", groupID, err) } if group == nil { - // Group was deleted; skip it rather than failing the entire request. + authCtx.SkippedGroups++ continue } @@ -153,27 +156,52 @@ func (s *Service) collectGroupsAndAccounts(ctx context.Context, authCtx *AuthCon // ResolveAllowedAccounts returns the cloud accounts a user may access, and // FAILS CLOSED when that scope cannot be established (issue #1748). // -// The subtlety this exists for: an empty result means UNRESTRICTED -// (IsUnrestrictedAccess), a deliberate backward-compat default so a group with -// no allowed_accounts configured grants full access. But -// collectGroupsAndAccounts also skips a group it cannot load -- pgx.ErrNoRows, -// or a store returning (nil, nil) -- so a user whose groups all fail to -// resolve produced the SAME empty value. Absence and unrestricted shared a -// representation, and every failure silently granted access to every account. +// The subtlety: an empty result means UNRESTRICTED (IsUnrestrictedAccess), a +// deliberate backward-compat default so a group with no allowed_accounts +// configured grants full access. But collectGroupsAndAccounts skips a group it +// cannot load, so a skipped group can leave an empty union that reads as +// "every account". +// +// DROPPING A GROUP CAN WIDEN ACCESS. This is the part that is easy to get +// wrong, and an earlier version of this guard did: the union of [] and +// ["acct-A"] is RESTRICTED, so losing the group carrying ["acct-A"] collapses +// it to [] -- unrestricted. Partial resolution is therefore NOT safely +// "narrower"; it is only narrower when the surviving groups still contribute +// entries. Reproduced by execution with a two-group actor (one granting +// update:groups with no allowed_accounts, one carrying the restriction): +// losing the restricting group turned a scope of [acct-A] into all accounts. +// +// A single-group configuration cannot exhibit this, which is why a six-case +// single-group verification found nothing. // -// Requiring at least one group to have actually resolved separates the two -// cleanly. Verified across all six cases: a group with allowed_accounts=["*"], -// a group with none configured (the legacy default), and a scoped group all -// resolve at least one group and keep working; a missing group, a (nil, nil) -// group, and a user with no groups at all resolve none and are refused. +// The guard is therefore: refuse when the union is empty AND at least one +// group was skipped. That targets exactly the widening. It deliberately does +// NOT refuse on any unresolved group: that would reverse the intentional +// "group was deleted; skip it rather than failing the entire request" +// behaviour and lock out a user with one stale membership everywhere until an +// admin cleaned up -- trading a conditional security hole for an +// unconditional availability regression on a path this widely consumed. // -// A user with no groups holds no permissions either, so refusing them here -// costs nothing and closes the same hole from the other side. +// A restricted principal keeps working through a skipped group, because a +// non-empty union cannot have been widened by the loss. +// +// The skip count comes from collectGroupsAndAccounts, NOT from comparing +// len(Groups) to len(User.GroupIDs) -- GroupIDs may contain duplicates, so +// that comparison would report phantom skips and refuse real users. func (s *Service) ResolveAllowedAccounts(ctx context.Context, userID string) ([]string, error) { authCtx, err := s.BuildAuthContext(ctx, userID) if err != nil { return nil, err } + if IsUnrestrictedAccess(authCtx.AllowedAccounts) && authCtx.SkippedGroups > 0 { + return nil, fmt.Errorf( + "account scope could not be established for user %s: %d group(s) could not be resolved "+ + "and the remaining scope is unrestricted", userID, authCtx.SkippedGroups) + } + // A principal belonging to no group holds no permissions; treating that as + // unrestricted account access is indefensible for a scope check even + // though migration 000057's users_min_one_group CHECK makes it + // structurally unreachable today. if len(authCtx.Groups) == 0 { return nil, fmt.Errorf( "account scope could not be established for user %s: no group resolved", userID) diff --git a/internal/auth/types.go b/internal/auth/types.go index cab2240a1..5acc93bee 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -105,6 +105,13 @@ type AuthContext struct { //nolint:revive // exported: doc comment style intenti Groups []*Group AllowedAccounts []string // Computed from all groups (union) Permissions []Permission // Computed from group memberships + + // SkippedGroups counts memberships that could not be resolved (deleted or + // unreadable). It is NOT derivable by comparing len(Groups) to + // len(User.GroupIDs): GroupIDs may contain duplicates, so that comparison + // reports phantom skips and refuses legitimate principals. Counted at the + // point of skipping instead (issue #1748). + SkippedGroups int } // adminCarvedOuts is the set of (action, resource) pairs that the admin:* From fb7a148b33ea0069741c9fe2661bf599193bb9dc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 06:53:29 +0200 Subject: [PATCH 3/4] style(auth): use US spelling in the partial-resolution comments misspell enforces US English; two comments carried 'behaviour'. The wording came from a briefing message written in British English and was transcribed into code without being re-checked against the repo's own gate. Refs #1748. --- internal/auth/account_scope_fail_closed_test.go | 2 +- internal/auth/service_group.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/auth/account_scope_fail_closed_test.go b/internal/auth/account_scope_fail_closed_test.go index 9f80e0f92..181d3ca14 100644 --- a/internal/auth/account_scope_fail_closed_test.go +++ b/internal/auth/account_scope_fail_closed_test.go @@ -222,7 +222,7 @@ func TestResolveAllowedAccounts_MultiGroupBaselineStaysRestricted(t *testing.T) // Deleted-group tolerance is PRESERVED for a restricted principal: a // non-empty union cannot have been widened by the loss, so the skip is -// absorbed rather than refused. This is the behaviour option 1 ("refuse on any +// absorbed rather than refused. This is the behavior option 1 ("refuse on any // unresolved group") would have destroyed, locking out every user with one // stale membership. func TestResolveAllowedAccounts_SkippedGroupToleratedWhenUnionStaysRestricted(t *testing.T) { diff --git a/internal/auth/service_group.go b/internal/auth/service_group.go index 11e617fcd..b0c814cf7 100644 --- a/internal/auth/service_group.go +++ b/internal/auth/service_group.go @@ -178,7 +178,7 @@ func (s *Service) collectGroupsAndAccounts(ctx context.Context, authCtx *AuthCon // group was skipped. That targets exactly the widening. It deliberately does // NOT refuse on any unresolved group: that would reverse the intentional // "group was deleted; skip it rather than failing the entire request" -// behaviour and lock out a user with one stale membership everywhere until an +// behavior and lock out a user with one stale membership everywhere until an // admin cleaned up -- trading a conditional security hole for an // unconditional availability regression on a path this widely consumed. // From 07e88eebdf2c13c6eef4d93af1696d1ff739a7e3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 07:20:41 +0200 Subject: [PATCH 4/4] fix(auth): do not refuse a principal that was already unrestricted The partial-resolution guard tested IsUnrestrictedAccess(AllowedAccounts), which is true for an empty union OR for one containing "*". A principal whose surviving union carries "*" was ALREADY maximally wide at baseline, so no lost group can widen them. Refusing them is zero security benefit and pure availability cost. All seven seeded groups ship allowed_accounts = ARRAY['*'], so this hit the default shape: delete any group, and its members who also hold a seeded group got errors on every account-scoped endpoint until an admin edited each of them individually. That is the unconditional availability regression that option 1 was rejected to avoid, arriving through the predicate instead. Measured, survivor carrying ["*"]: baseline (both resolve) scope=[* acct-A] unrestricted=true stale membership ErrNoRows REFUSED <-- regression stale membership (nil,nil) REFUSED <-- regression The test is now len(AllowedAccounts) == 0. Only an EMPTY union can have been widened by a loss. Both directions are mutation-verified and each is guarded by exactly one test: reverting the predicate kills only WildcardSurvivorToleratesSkippedGroup; removing the guard entirely kills only PartialResolutionThatWidensIsRefused. Neither test would catch the other's defect, so both are load-bearing. Also retracts an incorrect claim recorded in two comments and a test name. The skip count was documented as NOT derivable from len(GroupIDs) - len(Groups) because GroupIDs may contain duplicates. That is wrong: Groups is appended once per ID with no dedup, so duplicate IDs produce duplicate entries and the counts stay aligned on every shape. The counted-skip implementation is kept because it states the intent directly rather than inferring it from two lengths, but the comments no longer assert a trap that does not exist. TestResolveAllowedAccounts_DuplicateGroupIDsAreNotSkips could not fail under the implementation it claimed to exclude -- swapping the counted skip for the length subtraction left it passing. It does catch over-blocking, so it is renamed to DuplicateMembershipIsAllowed rather than deleted: a test whose name promises something it cannot check is worse than one scoped to what it proves. Refs #1748. --- .../auth/account_scope_fail_closed_test.go | 77 +++++++++++++++++-- internal/auth/service_group.go | 20 ++++- internal/auth/types.go | 10 ++- 3 files changed, 94 insertions(+), 13 deletions(-) diff --git a/internal/auth/account_scope_fail_closed_test.go b/internal/auth/account_scope_fail_closed_test.go index 181d3ca14..1b843e3ea 100644 --- a/internal/auth/account_scope_fail_closed_test.go +++ b/internal/auth/account_scope_fail_closed_test.go @@ -242,10 +242,15 @@ func TestResolveAllowedAccounts_SkippedGroupToleratedWhenUnionStaysRestricted(t assert.False(t, IsUnrestrictedAccess(got)) } -// Duplicate GroupIDs must not be mistaken for skips. Detecting skips by -// comparing len(Groups) to len(GroupIDs) would see 2 ids resolving to 1 group -// and refuse this legitimate principal. -func TestResolveAllowedAccounts_DuplicateGroupIDsAreNotSkips(t *testing.T) { +// A duplicated membership resolving to an unrestricted group is allowed. +// +// This was named DuplicateGroupIDsAreNotSkips and claimed to exclude a +// len(Groups) vs len(GroupIDs) skip count. It cannot: Groups is appended once +// per ID with no dedup, so duplicates produce duplicate entries and the two +// implementations agree on every shape -- swapping one for the other leaves +// this test passing. Renamed to what it does guard, which is over-blocking: +// a guard that refused any multi-entry membership would fail here. +func TestResolveAllowedAccounts_DuplicateMembershipIsAllowed(t *testing.T) { ctx := context.Background() ms, svc := scopeStore(t) t.Cleanup(func() { ms.AssertExpectations(t) }) @@ -256,6 +261,68 @@ func TestResolveAllowedAccounts_DuplicateGroupIDsAreNotSkips(t *testing.T) { got, err := svc.ResolveAllowedAccounts(ctx, scopeUserID) - require.NoError(t, err, "duplicate memberships are not unresolved groups") + require.NoError(t, err, "a duplicated membership must not be refused") assert.True(t, IsUnrestrictedAccess(got)) } + +// A survivor carrying "*" was ALREADY maximally wide at baseline, so no lost +// group can widen it. Refusing it is zero security benefit and pure +// availability cost -- and it is the shape a default deployment produces, +// because all seven seeded groups ship allowed_accounts = ARRAY['*']. +// +// This is why the guard tests len(AllowedAccounts) == 0 rather than +// IsUnrestrictedAccess: the latter is true for "*" too, and using it 500'd +// every account-scoped endpoint for any member of a seeded group who also had +// one stale membership. +func TestResolveAllowedAccounts_WildcardSurvivorToleratesSkippedGroup(t *testing.T) { + ctx := context.Background() + const seeded, scoped = "g-seeded", "g-scoped" + + for _, tc := range []struct { + name string + scopeResp func(*MockStore) + }{ + {"stale membership missing (ErrNoRows)", func(ms *MockStore) { + ms.On("GetGroup", ctx, scoped).Return(nil, pgx.ErrNoRows) + }}, + {"stale membership resolves to (nil, nil)", func(ms *MockStore) { + ms.On("GetGroup", ctx, scoped).Return(nil, nil) + }}, + } { + t.Run(tc.name, func(t *testing.T) { + ms, svc := scopeStore(t) + t.Cleanup(func() { ms.AssertExpectations(t) }) + ms.On("GetUserByID", ctx, scopeUserID). + Return(&User{ID: scopeUserID, GroupIDs: []string{seeded, scoped}}, nil) + ms.On("GetGroup", ctx, seeded).Return(&Group{ + ID: seeded, AllowedAccounts: []string{"*"}, + }, nil) + tc.scopeResp(ms) + + got, err := svc.ResolveAllowedAccounts(ctx, scopeUserID) + + require.NoError(t, err, + "a principal already unrestricted at baseline must not be refused for a lost group") + assert.True(t, IsUnrestrictedAccess(got)) + }) + } +} + +// The baseline control: the same principal, both groups resolving, is +// unrestricted anyway -- which is what makes the refusal above pointless. +func TestResolveAllowedAccounts_WildcardSurvivorBaselineIsAlreadyUnrestricted(t *testing.T) { + ctx := context.Background() + ms, svc := scopeStore(t) + t.Cleanup(func() { ms.AssertExpectations(t) }) + + ms.On("GetUserByID", ctx, scopeUserID). + Return(&User{ID: scopeUserID, GroupIDs: []string{"g-seeded", "g-scoped"}}, nil) + ms.On("GetGroup", ctx, "g-seeded").Return(&Group{ID: "g-seeded", AllowedAccounts: []string{"*"}}, nil) + ms.On("GetGroup", ctx, "g-scoped").Return(&Group{ID: "g-scoped", AllowedAccounts: []string{"acct-A"}}, nil) + + got, err := svc.ResolveAllowedAccounts(ctx, scopeUserID) + + require.NoError(t, err) + assert.True(t, IsUnrestrictedAccess(got), + "the wildcard makes this principal unrestricted before any group is lost") +} diff --git a/internal/auth/service_group.go b/internal/auth/service_group.go index b0c814cf7..6dc8ee347 100644 --- a/internal/auth/service_group.go +++ b/internal/auth/service_group.go @@ -185,15 +185,27 @@ func (s *Service) collectGroupsAndAccounts(ctx context.Context, authCtx *AuthCon // A restricted principal keeps working through a skipped group, because a // non-empty union cannot have been widened by the loss. // -// The skip count comes from collectGroupsAndAccounts, NOT from comparing -// len(Groups) to len(User.GroupIDs) -- GroupIDs may contain duplicates, so -// that comparison would report phantom skips and refuse real users. +// The emptiness test is len(AllowedAccounts) == 0, deliberately NOT +// IsUnrestrictedAccess. The latter is also true for a union containing "*", +// and such a principal was ALREADY maximally wide at baseline -- no lost group +// can widen them further, so refusing them buys nothing and costs +// availability. All seven seeded groups ship allowed_accounts = ARRAY['*'], so +// using IsUnrestrictedAccess here 500'd every account-scoped endpoint for any +// member of a seeded group who also had one stale membership. Only an EMPTY +// union can have been widened by a loss. +// +// The skip count comes from collectGroupsAndAccounts, counted where the skip +// happens. len(User.GroupIDs) - len(Groups) would in fact give the same answer +// -- Groups is appended once per ID with no dedup, so duplicate IDs produce +// duplicate entries and the counts stay aligned -- but counting at the point +// of skipping states the intent directly instead of inferring it from two +// lengths that happen to line up. func (s *Service) ResolveAllowedAccounts(ctx context.Context, userID string) ([]string, error) { authCtx, err := s.BuildAuthContext(ctx, userID) if err != nil { return nil, err } - if IsUnrestrictedAccess(authCtx.AllowedAccounts) && authCtx.SkippedGroups > 0 { + if len(authCtx.AllowedAccounts) == 0 && authCtx.SkippedGroups > 0 { return nil, fmt.Errorf( "account scope could not be established for user %s: %d group(s) could not be resolved "+ "and the remaining scope is unrestricted", userID, authCtx.SkippedGroups) diff --git a/internal/auth/types.go b/internal/auth/types.go index 5acc93bee..c79aa50b7 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -107,10 +107,12 @@ type AuthContext struct { //nolint:revive // exported: doc comment style intenti Permissions []Permission // Computed from group memberships // SkippedGroups counts memberships that could not be resolved (deleted or - // unreadable). It is NOT derivable by comparing len(Groups) to - // len(User.GroupIDs): GroupIDs may contain duplicates, so that comparison - // reports phantom skips and refuses legitimate principals. Counted at the - // point of skipping instead (issue #1748). + // unreadable), counted where the skip happens. + // + // len(User.GroupIDs) - len(Groups) would give the same answer, since Groups + // is appended once per ID with no dedup and duplicate IDs therefore produce + // duplicate entries. Counting explicitly states the intent rather than + // relying on two lengths staying aligned (issue #1748). SkippedGroups int }