Skip to content

Commit dadf637

Browse files
committed
sec(auth): refuse a partial actor resolution that widens the grant ceiling
grantCeilingAccounts closed only TOTAL resolution failure, on the premise that partial resolution under-reports the actor's scope and is therefore stricter. That premise is false for AllowedAccounts, which is a UNION in which the empty set means EVERYTHING: 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 [], and the actor may then widen any group to ["*"]. Verified by execution against the write path: baseline (both groups resolve) REFUSED widening to [*] PARTIAL (scoping group ErrNoRows) ACCEPTED widening to [*] The configuration needs TWO groups -- one granting update:groups with no allowed_accounts, one carrying the restriction -- which is why the earlier six-case single-group verification could not find it. The guard is now: refuse when the union is empty AND at least one group was skipped. The emptiness test is len(AllowedAccounts) == 0, deliberately NOT IsUnrestrictedAccess: a union containing "*" was already maximally wide at baseline, so no loss can widen it, and refusing it would be zero security benefit and pure availability cost. All seven seeded groups ship allowed_accounts = ARRAY['*'], so the broader predicate would have refused every seeded-group member with one stale membership. Both directions are mutation-verified and each is guarded by exactly one test: widening the predicate kills only WildcardActorToleratesSkippedGroup; removing the guard kills only PartialActorResolutionThatWidensIsRefused. Neither would catch the other's defect. The total-failure case is the degenerate partial one -- every group skipped -- so it is now caught by the skipped-group guard and carries that message. The existing test asserted the other guard's exact wording; its assertion is relaxed to the sentinel plus a substring true of both. NOTE ON OVERLAP: AuthContext.SkippedGroups and the counting in collectGroupsAndAccounts are also added by PR #1752 for the read path. The two changes are identical; whichever merges second should see no divergence. Refs #1550, #1748.
1 parent 386b316 commit dadf637

2 files changed

Lines changed: 134 additions & 1 deletion

File tree

‎internal/auth/group_account_ceiling_test.go‎

Lines changed: 115 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -322,8 +322,122 @@ func TestAccountCeiling_FailsClosedWhenActorScopeUnresolvable(t *testing.T) {
322322

323323
require.Error(t, err, "an unresolvable scope must refuse the widening, not permit it")
324324
assert.ErrorIs(t, err, ErrPermissionCeiling)
325-
assert.Contains(t, err.Error(), "could not be established")
325+
// Total failure is the degenerate case of a partial one -- every
326+
// group skipped -- so it is caught by the skipped-group guard and
327+
// carries that message. Assert the sentinel plus the shared
328+
// substring rather than one guard's exact wording.
329+
assert.Contains(t, err.Error(), "could not be resolved")
326330
mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything)
327331
})
328332
}
329333
}
334+
335+
// A PARTIAL group resolution can WIDEN the actor's own scope, letting them
336+
// grant what their real configuration forbids (#1737 A1, same defect as the
337+
// read path in #1752).
338+
//
339+
// Needs TWO groups: one granting update:groups with no allowed_accounts
340+
// (unrestricted alone), one carrying the restriction. The union is
341+
// restricted; lose the restricting group and it collapses to empty = every
342+
// account, and the ceiling then permits widening a group to ["*"].
343+
func TestAccountCeiling_PartialActorResolutionThatWidensIsRefused(t *testing.T) {
344+
ctx := context.Background()
345+
const permGroup, scopeGroup = "g-perm", "g-scope"
346+
347+
for _, tc := range []struct {
348+
name string
349+
scopeResp func(*MockStore)
350+
}{
351+
{"restricting group missing (ErrNoRows)", func(ms *MockStore) {
352+
ms.On("GetGroup", ctx, scopeGroup).Return(nil, pgx.ErrNoRows)
353+
}},
354+
{"restricting group resolves to (nil, nil)", func(ms *MockStore) {
355+
ms.On("GetGroup", ctx, scopeGroup).Return(nil, nil)
356+
}},
357+
} {
358+
t.Run(tc.name, func(t *testing.T) {
359+
mockStore := new(MockStore)
360+
t.Cleanup(func() { mockStore.AssertExpectations(t) })
361+
svc := createTestService(mockStore, new(MockEmailSender))
362+
363+
mockStore.On("GetUserByID", ctx, ceilingActorID).
364+
Return(&User{ID: ceilingActorID, GroupIDs: []string{permGroup, scopeGroup}}, nil)
365+
mockStore.On("GetGroup", ctx, permGroup).Return(&Group{
366+
ID: permGroup,
367+
Permissions: []Permission{{Action: ActionUpdate, Resource: ResourceGroups}},
368+
}, nil)
369+
tc.scopeResp(mockStore)
370+
mockStore.On("GetGroup", ctx, ceilingTargetID).Return(&Group{
371+
ID: ceilingTargetID, Name: "Team", AllowedAccounts: []string{scopedAccountA},
372+
}, nil)
373+
374+
_, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID,
375+
APIUpdateGroupRequest{AllowedAccounts: []string{"*"}})
376+
377+
require.Error(t, err, "a widened actor scope must not authorize widening a group")
378+
assert.ErrorIs(t, err, ErrPermissionCeiling)
379+
mockStore.AssertNotCalled(t, "UpdateGroup", mock.Anything, mock.Anything)
380+
})
381+
}
382+
}
383+
384+
// Baseline control: both groups resolving, the same actor is correctly
385+
// refused for a DIFFERENT reason (their real scope does not cover "*"), which
386+
// is what makes the partial case a genuine bypass rather than a no-op.
387+
func TestAccountCeiling_PartialActorBaselineIsRefusedOnRealScope(t *testing.T) {
388+
ctx := context.Background()
389+
const permGroup, scopeGroup = "g-perm", "g-scope"
390+
391+
mockStore := new(MockStore)
392+
t.Cleanup(func() { mockStore.AssertExpectations(t) })
393+
svc := createTestService(mockStore, new(MockEmailSender))
394+
395+
mockStore.On("GetUserByID", ctx, ceilingActorID).
396+
Return(&User{ID: ceilingActorID, GroupIDs: []string{permGroup, scopeGroup}}, nil)
397+
mockStore.On("GetGroup", ctx, permGroup).Return(&Group{
398+
ID: permGroup, Permissions: []Permission{{Action: ActionUpdate, Resource: ResourceGroups}},
399+
}, nil)
400+
mockStore.On("GetGroup", ctx, scopeGroup).Return(&Group{
401+
ID: scopeGroup, AllowedAccounts: []string{scopedAccountA},
402+
}, nil)
403+
mockStore.On("GetGroup", ctx, ceilingTargetID).Return(&Group{
404+
ID: ceilingTargetID, Name: "Team", AllowedAccounts: []string{scopedAccountA},
405+
}, nil)
406+
407+
_, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID,
408+
APIUpdateGroupRequest{AllowedAccounts: []string{"*"}})
409+
410+
require.Error(t, err)
411+
assert.ErrorIs(t, err, ErrPermissionCeiling)
412+
}
413+
414+
// An actor whose surviving union carries "*" was already maximally wide, so a
415+
// lost group cannot widen them. Refusing them would be pure availability cost
416+
// -- and all seven seeded groups ship allowed_accounts = ARRAY['*'].
417+
func TestAccountCeiling_WildcardActorToleratesSkippedGroup(t *testing.T) {
418+
ctx := context.Background()
419+
const seededGroup, scopeGroup = "g-seeded", "g-scope"
420+
421+
mockStore := new(MockStore)
422+
t.Cleanup(func() { mockStore.AssertExpectations(t) })
423+
svc := createTestService(mockStore, new(MockEmailSender))
424+
425+
mockStore.On("GetUserByID", ctx, ceilingActorID).
426+
Return(&User{ID: ceilingActorID, GroupIDs: []string{seededGroup, scopeGroup}}, nil)
427+
mockStore.On("GetGroup", ctx, seededGroup).Return(&Group{
428+
ID: seededGroup,
429+
Permissions: []Permission{{Action: ActionUpdate, Resource: ResourceGroups}},
430+
AllowedAccounts: []string{"*"},
431+
}, nil)
432+
mockStore.On("GetGroup", ctx, scopeGroup).Return(nil, pgx.ErrNoRows)
433+
mockStore.On("GetGroup", ctx, ceilingTargetID).Return(&Group{
434+
ID: ceilingTargetID, Name: "Team", AllowedAccounts: []string{scopedAccountA},
435+
}, nil)
436+
mockStore.On("UpdateGroup", ctx, mock.AnythingOfType("*auth.Group")).Return(nil).Once()
437+
438+
_, err := svc.UpdateGroupAPI(ctx, ceilingActorID, ceilingTargetID,
439+
APIUpdateGroupRequest{AllowedAccounts: []string{"*"}})
440+
441+
require.NoError(t, err,
442+
"an actor already unrestricted at baseline must not be refused for a lost group")
443+
}

‎internal/auth/group_ceiling.go‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,25 @@ func (s *Service) grantCeilingAccounts(ctx context.Context, actorUserID string)
155155
if err != nil {
156156
return nil, fmt.Errorf("%w: could not resolve the acting user's account scope: %w", ErrPermissionCeiling, err)
157157
}
158+
// A PARTIAL resolution can WIDEN the actor's scope, which is the case that
159+
// matters and the one an earlier version of this guard missed.
160+
//
161+
// AllowedAccounts is a union in which the empty set means EVERYTHING, so
162+
// dropping a contributing group does not narrow it. The union of [] and
163+
// ["acct-A"] is restricted; lose the group carrying ["acct-A"] and it
164+
// collapses to [] = unrestricted, and the actor can then widen any group
165+
// to ["*"]. Verified by execution against the write path: baseline
166+
// REFUSED, one skipped group ACCEPTED.
167+
//
168+
// The test is len(AllowedAccounts) == 0, NOT IsUnrestrictedAccess. A union
169+
// containing "*" was already maximally wide at baseline, so no loss can
170+
// widen it; refusing it would be zero security benefit and pure
171+
// availability cost, and all seven seeded groups ship ARRAY['*'].
172+
if len(authCtx.AllowedAccounts) == 0 && authCtx.SkippedGroups > 0 {
173+
return nil, fmt.Errorf(
174+
"%w: %d group(s) could not be resolved and the acting user's remaining scope is unrestricted",
175+
ErrPermissionCeiling, authCtx.SkippedGroups)
176+
}
158177
// An unresolved scope is UNKNOWN, not unrestricted.
159178
//
160179
// collectGroupsAndAccounts skips a missing or deleted group silently

0 commit comments

Comments
 (0)