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..1b843e3ea --- /dev/null +++ b/internal/auth/account_scope_fail_closed_test.go @@ -0,0 +1,328 @@ +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)) + }) + } +} + +// 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 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) { + 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, "a stale membership must not lock out a restricted principal") + assert.Equal(t, []string{"acct-A"}, got) + assert.False(t, IsUnrestrictedAccess(got)) +} + +// 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) }) + + 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, "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 ce23b49ff..6dc8ee347 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 } @@ -150,6 +153,74 @@ 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: 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. +// +// 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" +// 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. +// +// A restricted principal keeps working through a skipped group, because a +// non-empty union cannot have been widened by the loss. +// +// 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 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) + } + // 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) + } + 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/auth/types.go b/internal/auth/types.go index cab2240a1..c79aa50b7 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -105,6 +105,15 @@ 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), 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 } // adminCarvedOuts is the set of (action, resource) pairs that the admin:* 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.