From b9109a56a24803ffe3fd4eb95026441cf350d4c5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 03:52:00 +0200 Subject: [PATCH 1/2] fix(test): make grantAdmin model the principal instead of stubbing the answer grantAdmin registered HasPermissionAPI returning a constant true for every (userID, action, resource) triple. HasPermissionAPI is the authorization decision, so 235 tests across 26 files asserted downstream behaviour with the gate already answered. The consequence is sharper than "too permissive". Two of the pairs handlers actually ask for under grantAdmin are money verbs carved out of the admin:* wildcard by issue #923: 12 calls execute:purchases grantAdmin answered TRUE 23 calls approve-any:purchases grantAdmin answered TRUE A real Administrators member gets false for all three carved-out verbs. So grantAdmin modeled a principal production cannot have: an admin who may spend money. Verified by mutation: with adminCarvedOuts emptied, internal/api passed green. The separation-of-duties control that #923, #1550 and #1737 exist to defend had no handler-level regression barrier at all. grantAdmin now models the principal's STATE -- it holds exactly {admin, *} -- and every question is answered by the real matcher, auth.AuthContext .HasPermission, which applies the carve-out. The decision function is read after m.Called so the invocation is still recorded; short-circuiting ahead of m.Called is what made 20 assertions vacuous in #1595 and is not reintroduced. 21 purchase-path tests failed once the answer became real, all with "permission denied: requires execute on purchases". Each is a fixture defect, not a handler defect: the test's principal was under-specified. They now use grantAdminPurchaser, modeling an admin who is also in the Purchaser group, which migrations 000059/000064 backfill onto every admin, so it is the default real operator on the money paths. No handler behaviour changed. Adds grantScoped(accounts...) for the restricted allow-list. grantAdmin pins GetAllowedAccountsAPI to nil, read as unrestricted, so no test could exercise account scoping, the structural reason the #950/#956 filter regressions survived four "fixed, tests are green" rounds. New tests pin both properties and fail when the control is removed: TestGrantAdmin_CarveOutIsEnforcedAtHandler, TestExecutePurchase_ PlainAdminIsRefused, and the grantScoped seam tests, each with a negative control so a guard that refused everything would not pass. Refs #1596. --- .../api/executed_notification_flow_test.go | 2 +- internal/api/grantadmin_carveout_test.go | 161 ++++++++++++++++++ internal/api/grantscoped_test.go | 113 ++++++++++++ internal/api/handler_purchases_guards_test.go | 4 +- internal/api/handler_purchases_test.go | 30 ++-- internal/api/handler_ri_exchange_test.go | 4 +- internal/api/middleware_test.go | 2 +- internal/api/mocks_test.go | 110 ++++++++++-- 8 files changed, 387 insertions(+), 39 deletions(-) create mode 100644 internal/api/grantadmin_carveout_test.go create mode 100644 internal/api/grantscoped_test.go diff --git a/internal/api/executed_notification_flow_test.go b/internal/api/executed_notification_flow_test.go index 2d59b2e44..c50ec6832 100644 --- a/internal/api/executed_notification_flow_test.go +++ b/internal/api/executed_notification_flow_test.go @@ -219,7 +219,7 @@ func TestExecutedNotification_SessionApprovePath(t *testing.T) { mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: adminEmail}, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) mockPurchase := new(MockPurchaseManager) diff --git a/internal/api/grantadmin_carveout_test.go b/internal/api/grantadmin_carveout_test.go new file mode 100644 index 000000000..4a75c3580 --- /dev/null +++ b/internal/api/grantadmin_carveout_test.go @@ -0,0 +1,161 @@ +package api + +import ( + "context" + "testing" + + "github.com/LeanerCloud/CUDly/internal/auth" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Handler-level coverage for the #923 money separation-of-duties carve-out. +// +// Before issue #1596 this package had NONE. grantAdmin stubbed +// HasPermissionAPI to a constant true, so every admin-gated purchase test +// modeled a principal production cannot have: an admin who may spend money. +// The practical consequence was that `adminCarvedOuts` could have been +// deleted outright and not one test in internal/api would have failed -- the +// control that #923, #1550 and #1737 exist to defend had no handler-level +// regression barrier at all. +// +// These tests are that barrier. They must FAIL if adminCarvedOuts is emptied. + +// carvedOutVerbs is the set the admin:* wildcard must NOT cover. +var carvedOutVerbs = [][2]string{ + {auth.ActionExecute, auth.ResourcePurchases}, + {auth.ActionApproveAny, auth.ResourcePurchases}, + {auth.ActionRetryAny, auth.ResourcePurchases}, +} + +// TestGrantAdmin_CarveOutIsEnforcedAtHandler pins the authorization boundary +// itself: requirePermission must refuse a plain admin the money verbs. +func TestGrantAdmin_CarveOutIsEnforcedAtHandler(t *testing.T) { + ctx := context.Background() + + for _, verb := range carvedOutVerbs { + action, resource := verb[0], verb[1] + t.Run(action+":"+resource, func(t *testing.T) { + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(session, nil) + mockAuth.grantAdmin() + + h := &Handler{auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + } + + got, err := h.requirePermission(ctx, req, action, resource) + + require.Error(t, err, "admin:* must NOT be granted %s:%s (issue #923)", action, resource) + assert.Nil(t, got) + ce, ok := IsClientError(err) + require.True(t, ok, "a carve-out denial must be a client error, not a 500") + assert.Equal(t, 403, ce.code) + assert.Contains(t, err.Error(), action) + assert.Contains(t, err.Error(), resource) + }) + } +} + +// TestGrantAdmin_NonCarvedVerbsStillGranted is the negative control. Without +// it, a mock that denied everything would satisfy the test above. +func TestGrantAdmin_NonCarvedVerbsStillGranted(t *testing.T) { + ctx := context.Background() + + // Verbs an Administrators member genuinely holds via the wildcard, + // including two on the same resource as the carved-out ones so the + // assertion is about the specific pair and not about "purchases". + granted := [][2]string{ + {auth.ActionView, auth.ResourcePurchases}, + {auth.ActionUpdateAny, auth.ResourcePurchases}, + {auth.ActionUpdate, auth.ResourceConfig}, + {auth.ActionCreate, auth.ResourceUsers}, + } + + for _, verb := range granted { + action, resource := verb[0], verb[1] + t.Run(action+":"+resource, func(t *testing.T) { + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(session, nil) + mockAuth.grantAdmin() + + h := &Handler{auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + } + + got, err := h.requirePermission(ctx, req, action, resource) + require.NoError(t, err, "admin:* must still grant %s:%s", action, resource) + assert.NotNil(t, got) + }) + } +} + +// TestGrantAdminPurchaser_GrantsCarvedOutVerbs pins the other half: explicit +// Purchaser membership is what unlocks the money verbs, which is why the 21 +// purchase-path tests repaired in #1596 use grantAdminPurchaser. +func TestGrantAdminPurchaser_GrantsCarvedOutVerbs(t *testing.T) { + ctx := context.Background() + + for _, verb := range carvedOutVerbs { + action, resource := verb[0], verb[1] + t.Run(action+":"+resource, func(t *testing.T) { + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(session, nil) + mockAuth.grantAdminPurchaser() + + h := &Handler{auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + } + + got, err := h.requirePermission(ctx, req, action, resource) + require.NoError(t, err, "admin + Purchaser must grant %s:%s", action, resource) + assert.NotNil(t, got) + }) + } +} + +// TestExecutePurchase_PlainAdminIsRefused is the end-to-end form: the real +// executePurchase handler, the real request body, a plain admin. Before #1596 +// this returned 200 and wrote a purchase execution. +func TestExecutePurchase_PlainAdminIsRefused(t *testing.T) { + ctx := context.Background() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + session := &Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"} + mockAuth.On("ValidateSession", ctx, "admin-token").Return(session, nil) + mockAuth.grantAdmin() + + h := &Handler{config: mockStore, auth: mockAuth} + req := &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"Authorization": "Bearer admin-token"}, + Body: `{"recommendations": [{"id": "rec-1", "provider": "aws", "service": "ec2", "count": 1, "term": 1, "payment": "all-upfront", "upfront_cost": 100.0, "savings": 50.0}]}`, + } + + result, err := h.executePurchase(ctx, req) + + require.Error(t, err, "a plain admin must not be able to execute a purchase (#923)") + assert.Nil(t, result) + assert.Contains(t, err.Error(), "execute") + assert.Contains(t, err.Error(), "purchases") + // No purchase execution may be written. Stubbed with no expectation, so + // testify would panic if the handler got this far; the explicit + // per-parameter matchers make the assertion non-vacuous either way + // (issue #1595: a name-only AssertNotCalled can never fail). + mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) +} diff --git a/internal/api/grantscoped_test.go b/internal/api/grantscoped_test.go new file mode 100644 index 000000000..659f4304e --- /dev/null +++ b/internal/api/grantscoped_test.go @@ -0,0 +1,113 @@ +package api + +import ( + "context" + "testing" + + "github.com/LeanerCloud/CUDly/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Restricted-account coverage (issue #1596). +// +// grantAdmin pins GetAllowedAccountsAPI to nil, which the API layer reads as +// unrestricted, so before grantScoped existed NO test in this package could +// exercise a restricted allow-list. That is the structural reason the +// #950/#956 account-filter regressions survived four rounds of "fixed, tests +// are green": the suite had no restriction to enforce. +// +// These pin the shared seam every scoped handler funnels through +// (getAllowedAccounts -> requireAccountAccess / requirePlanAccess), so a +// regression in the seam fails here rather than silently in production. + +const ( + scopedInAccount = "11111111-1111-4111-8111-111111111111" + scopedOutAccount = "22222222-2222-4222-8222-222222222222" + scopedToken = "scoped-token" + scopedUserID = "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" +) + +func scopedHandler(t *testing.T, accounts ...string) (*Handler, *MockConfigStore) { + t.Helper() + mockStore := new(MockConfigStore) + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + mockAuth.On("ValidateSession", mock.Anything, scopedToken). + Return(&Session{UserID: scopedUserID}, nil).Maybe() + if len(accounts) == 0 { + mockAuth.grantAdmin() + } else { + mockAuth.grantScoped(accounts...) + } + return &Handler{config: mockStore, auth: mockAuth}, mockStore +} + +// TestGrantScoped_RestrictsAccountAccess is the core assertion: a principal +// scoped to one account cannot reach another, and the refusal is the +// enumeration-safe errNotFound rather than a 403 that would confirm the +// account exists. +func TestGrantScoped_RestrictsAccountAccess(t *testing.T) { + ctx := context.Background() + h, mockStore := scopedHandler(t, scopedInAccount) + + other := &config.CloudAccount{ID: scopedOutAccount, Name: "other-account"} + mockStore.On("GetCloudAccount", ctx, scopedOutAccount).Return(other, nil) + + got, err := h.requireAccountAccess(ctx, &Session{UserID: scopedUserID}, scopedOutAccount) + + require.Error(t, err, "an account outside the allow-list must not be reachable") + assert.Nil(t, got) + assert.ErrorIs(t, err, errNotFound) +} + +// Negative control: the same handler, the same code path, an account that IS +// in the allow-list. Without this, a seam that refused everything would pass +// the test above. +func TestGrantScoped_AllowsInScopeAccount(t *testing.T) { + ctx := context.Background() + h, mockStore := scopedHandler(t, scopedInAccount) + + mine := &config.CloudAccount{ID: scopedInAccount, Name: "my-account"} + mockStore.On("GetCloudAccount", ctx, scopedInAccount).Return(mine, nil) + + got, err := h.requireAccountAccess(ctx, &Session{UserID: scopedUserID}, scopedInAccount) + + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, scopedInAccount, got.ID) +} + +// Second negative control: an UNRESTRICTED admin still reaches the same +// account grantScoped refuses. This is what proves the refusal above comes +// from the allow-list and not from some unrelated failure in the fixture. +func TestGrantAdmin_UnrestrictedReachesAnyAccount(t *testing.T) { + ctx := context.Background() + h, mockStore := scopedHandler(t) // no accounts -> grantAdmin, unrestricted + + other := &config.CloudAccount{ID: scopedOutAccount, Name: "other-account"} + mockStore.On("GetCloudAccount", ctx, scopedOutAccount).Return(other, nil) + + got, err := h.requireAccountAccess(ctx, &Session{UserID: scopedUserID}, scopedOutAccount) + + require.NoError(t, err, "an unrestricted admin must still reach any account") + require.NotNil(t, got) +} + +// The allow-list matches on display name as well as ID (auth.MatchesAccount), +// so a scoped principal named by account NAME resolves too. Pinned because a +// regression here silently widens or narrows every scoped handler at once. +func TestGrantScoped_MatchesByAccountName(t *testing.T) { + ctx := context.Background() + h, mockStore := scopedHandler(t, "prod-account") + + mine := &config.CloudAccount{ID: scopedInAccount, Name: "prod-account"} + mockStore.On("GetCloudAccount", ctx, scopedInAccount).Return(mine, nil) + + got, err := h.requireAccountAccess(ctx, &Session{UserID: scopedUserID}, scopedInAccount) + + require.NoError(t, err) + require.NotNil(t, got) +} diff --git a/internal/api/handler_purchases_guards_test.go b/internal/api/handler_purchases_guards_test.go index c15185d7a..8ff602897 100644 --- a/internal/api/handler_purchases_guards_test.go +++ b/internal/api/handler_purchases_guards_test.go @@ -359,7 +359,7 @@ func TestHandler_executePurchase_SurfacesPaymentAdjustments(t *testing.T) { Email: "admin@example.com", } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil) @@ -409,7 +409,7 @@ func TestHandler_executePurchase_NoAdjustmentsWhenCanonical(t *testing.T) { Email: "admin@example.com", } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil) diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 5878c99c2..1e38ce9a2 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -307,7 +307,7 @@ func TestHandler_approvePurchase_SessionApproveAnyChainsToExecute(t *testing.T) mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: adminEmail}, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() // approvePurchaseViaSession enforces CSRF on the session-authed path (issue #404). mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) @@ -356,7 +356,7 @@ func TestHandler_approvePurchase_SessionExecuteFailureSurfacesAs409(t *testing.T mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: adminEmail}, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() // approvePurchaseViaSession enforces CSRF on the session-authed path (issue #404). mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) @@ -449,7 +449,7 @@ func TestHandler_approvePurchaseViaSession_GlobalConfigError_FailsClosed(t *test mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: adminEmail}, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() // approvePurchaseViaSession enforces CSRF. mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) @@ -572,7 +572,7 @@ func TestHandler_approvePurchase_AWSOrphanFallsThrough(t *testing.T) { mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: adminEmail}, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() // approvePurchaseViaSession enforces CSRF on the session-authed path (issue #404). mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) @@ -613,7 +613,7 @@ func TestHandler_approvePurchase_NonOrphanUnchanged(t *testing.T) { mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{Email: adminEmail}, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() // approvePurchaseViaSession enforces CSRF on the session-authed path (issue #404). mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) @@ -1258,7 +1258,7 @@ func TestHandler_runPlannedPurchase(t *testing.T) { } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() mockStore.On("TransitionExecutionStatus", ctx, "11111111-1111-1111-1111-111111111111", []string{"pending", "paused"}, "running", mock.Anything).Return(transitioned, nil) handler := &Handler{config: mockStore, auth: mockAuth} @@ -2192,7 +2192,7 @@ func TestHandler_executePurchase_Success(t *testing.T) { } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(nil) // executePurchase reads GlobalConfig to look up the per-provider // grace period. Return an empty-but-valid config so the grace @@ -2239,7 +2239,7 @@ func TestHandler_executePurchase_InvalidBody(t *testing.T) { } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() handler := &Handler{auth: mockAuth} @@ -2265,7 +2265,7 @@ func TestHandler_executePurchase_EmptyRecommendations(t *testing.T) { } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() handler := &Handler{auth: mockAuth} @@ -2291,7 +2291,7 @@ func TestHandler_executePurchase_NegativeUpfrontCost(t *testing.T) { } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() handler := &Handler{auth: mockAuth} @@ -2317,7 +2317,7 @@ func TestHandler_executePurchase_NegativeSavings(t *testing.T) { } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() handler := &Handler{auth: mockAuth} @@ -2343,7 +2343,7 @@ func TestHandler_executePurchase_TooManyRecommendations(t *testing.T) { } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() handler := &Handler{auth: mockAuth} @@ -2380,7 +2380,7 @@ func TestHandler_executePurchase_ExceedsMaxAmount(t *testing.T) { } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() handler := &Handler{auth: mockAuth} @@ -2407,7 +2407,7 @@ func TestHandler_executePurchase_SaveError(t *testing.T) { } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() mockStore.On("SavePurchaseExecution", ctx, mock.AnythingOfType("*config.PurchaseExecution")).Return(errors.New("database error")) mockStore.On("GetGlobalConfig", ctx).Return(&config.GlobalConfig{}, nil) mockStore.On("GetPendingExecutions", ctx).Return([]config.PurchaseExecution{}, nil) @@ -5664,7 +5664,7 @@ func TestHandler_approvePurchaseViaSession_FourEyesOn_DifferentApproverSucceeds( mockAuth := new(MockAuthService) mockAuth.On("ValidateSession", ctx, "sess-tok").Return(&Session{UserID: approverID, Email: approverEmail}, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "").Return(nil) realManager := purchase.NewManager(purchase.ManagerConfig{ConfigStore: mockConfig, EmailSender: &stubEmailNotifier{}}) diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 11fb15e81..6359041df 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -369,7 +369,7 @@ func TestApproveRIExchange_SessionAdmin(t *testing.T) { adminSession := &Session{UserID: "admin-uuid", Email: "admin@example.com"} mockAuth.On("ValidateSession", ctx, "admin-bearer").Return(adminSession, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() // authorizeSessionApproveRIExchange: admin role short-circuits (no HasPermissionAPI call) @@ -1755,7 +1755,7 @@ func TestApproveRIExchange_SessionActorStamped(t *testing.T) { adminSession := &Session{UserID: actorID, Email: "admin@example.com"} mockAuth.On("ValidateSession", ctx, "admin-bearer").Return(adminSession, nil) - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() mockStore.On("GetRIExchangeRecord", ctx, id).Return(&config.RIExchangeRecord{ ID: id, Status: "pending", ApprovalToken: "tok", SourceRIIDs: []string{"ri-1"}, PaymentDue: "10.00", diff --git a/internal/api/middleware_test.go b/internal/api/middleware_test.go index a54a84019..018ac17df 100644 --- a/internal/api/middleware_test.go +++ b/internal/api/middleware_test.go @@ -361,7 +361,7 @@ func TestApproveViaSession_PassesCSRF(t *testing.T) { mockAuth.On("ValidateSession", ctx, "sess-tok").Return(adminSession, nil) // Authorization is group-membership-only after issue #907: the session must // pass the approve-* HasPermissionAPI check before the CSRF guard runs. - mockAuth.grantAdmin() + mockAuth.grantAdminPurchaser() // Valid CSRF token supplied → ValidateCSRFToken succeeds. mockAuth.On("ValidateCSRFToken", ctx, "sess-tok", "csrf-abc").Return(nil) t.Cleanup(func() { mockAuth.AssertExpectations(t) }) diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index c25658d06..b9c2cd10e 100644 --- a/internal/api/mocks_test.go +++ b/internal/api/mocks_test.go @@ -276,12 +276,24 @@ func (m *MockAuthService) ListGroupsAPI(ctx context.Context) (interface{}, error func (m *MockAuthService) HasPermissionAPI(ctx context.Context, userID, action, resource string) (bool, error) { args := m.Called(ctx, userID, action, resource) - return args.Bool(0), args.Error(1) + return permissionDecision(args, action, resource), args.Error(1) +} + +// permissionDecision reads a mock return that is either a constant bool or a +// decision function (registered by grantPermissions, which answers through the +// real matcher). It is called AFTER m.Called so the invocation is always +// recorded first: short-circuiting ahead of m.Called is what made 20 +// assertions vacuous in issue #1595, and this must not reintroduce it. +func permissionDecision(args mock.Arguments, action, resource string) bool { + if decide, ok := args.Get(0).(func(action, resource string) bool); ok { + return decide(action, resource) + } + return args.Bool(0) } func (m *MockAuthService) HasPermissionForConstraintsAPI(ctx context.Context, userID, action, resource string, constraintSets []auth.PermissionConstraints) (bool, error) { args := m.Called(ctx, userID, action, resource, constraintSets) - return args.Bool(0), args.Error(1) + return permissionDecision(args, action, resource), args.Error(1) } func (m *MockAuthService) GetUserPermissionsAPI(ctx context.Context, userID string) (any, error) { @@ -301,26 +313,88 @@ func (m *MockAuthService) allowConstraintChecks() { Return(true, nil).Maybe() } -// grantAdmin makes every HasPermissionAPI check succeed, modeling an -// Administrators-group member. Authorization is group-membership-only after -// issue #907, so admin-gated handlers resolve "is admin" / specific permissions -// through HasPermissionAPI rather than a Session.Role short-circuit; tests that -// previously set Role:"admin" register this instead. Uses .Maybe() so handlers -// that don't reach a permission check don't fail the expectation, and matches -// any userID so a single call covers the test's admin session regardless of its -// UUID. +// grantAdmin models an Administrators-group member: a principal holding +// exactly {admin, *}, with unrestricted account access (the seeded group +// carries allowed_accounts ARRAY['*'], surfaced as nil). +// +// It does NOT stub the authorization decision. Every question is answered by +// the REAL matcher, auth.AuthContext.HasPermission, so the mock models the +// principal's STATE and lets production logic work out the answer -- including +// the adminCarvedOuts money verbs, which admin:* does NOT grant. +// +// Before issue #1596 this returned a constant true for every (action, +// resource) triple. That modeled a principal production cannot have: an admin +// who may spend money. The practical effect was that adminCarvedOuts could +// have been deleted outright without a single test in this package failing, +// so the #923 separation-of-duties control had no handler-level coverage at +// all. See TestGrantAdmin_CarveOutIsEnforcedAtHandler. func (m *MockAuthService) grantAdmin() { + m.grantPermissions([]auth.Permission{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}}) +} + +// grantAdminPurchaser models the principal a purchase actually requires: an +// Administrators member who is ALSO in the Purchaser group. +// +// admin:* alone cannot execute, approve-any or retry-any a purchase -- those +// three verbs are carved out of the wildcard for separation of duties (#923), +// so they require explicit membership in a group that grants them. Migrations +// 000059/000064 backfill exactly this pairing onto every existing admin, so +// this is the DEFAULT real-world operator on the money paths, not an exotic +// one. +// +// Tests on execute/approve/retry handlers must use this rather than +// grantAdmin. Before #1596 they used grantAdmin and passed anyway, because the +// stub answered true for verbs production denies. +func (m *MockAuthService) grantAdminPurchaser() { + m.grantPermissions([]auth.Permission{ + {Action: auth.ActionAdmin, Resource: auth.ResourceAll}, + {Action: auth.ActionExecute, Resource: auth.ResourcePurchases}, + {Action: auth.ActionApproveAny, Resource: auth.ResourcePurchases}, + {Action: auth.ActionRetryAny, Resource: auth.ResourcePurchases}, + }) +} + +// grantScoped models an admin whose account access is RESTRICTED to the given +// accounts, so handlers that claim to honor allowed_accounts can be exercised +// on the restricted path. grantAdmin's nil allow-list means unrestricted, so +// without this no test could ever exercise account scoping (issue #1596; the +// #950/#956 regression class this made untestable). +func (m *MockAuthService) grantScoped(accounts ...string) { + m.grantPermissionsScoped( + []auth.Permission{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}}, + accounts, + ) +} + +// grantPermissions models a principal holding exactly perms, with +// unrestricted account access. +func (m *MockAuthService) grantPermissions(perms []auth.Permission) { + m.grantPermissionsScoped(perms, nil) +} + +// grantPermissionsScoped is the shared implementation. The permission checks +// are answered by auth.AuthContext.HasPermission rather than by a constant, so +// a handler asking for a verb this principal does not hold is DENIED exactly +// as it would be in production. +// +// .Maybe() is retained: many handlers legitimately return before reaching a +// permission check (bad body, bad UUID). Asserting the check happened is a +// per-handler contract that grantAdmin cannot state for all 235 call sites; +// tests that need it should assert the call explicitly. +func (m *MockAuthService) grantPermissionsScoped(perms []auth.Permission, accounts []string) { + authCtx := &auth.AuthContext{Permissions: perms} + decide := func(action, resource string) bool { + return authCtx.HasPermission(action, resource) + } m.On("HasPermissionAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything). - Return(true, nil).Maybe() - // Admins hold {admin, *}, which grants every constraint set, so the - // SEC-01 execution-time constraint check succeeds too. + Return(decide, nil).Maybe() + // The SEC-01 execution-time constraint check. A principal holding only + // admin:* satisfies any constraint set for the verbs it holds, and holds + // none of the carved-out verbs, so the same decision function applies. m.On("HasPermissionForConstraintsAPI", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything). - Return(true, nil).Maybe() - // Administrators-group members carry the "*" wildcard, surfaced as - // unrestricted access (nil/empty). Handlers that scope by account call - // GetAllowedAccountsAPI after the permission check, so stub it too. + Return(decide, nil).Maybe() m.On("GetAllowedAccountsAPI", mock.Anything, mock.Anything). - Return([]string(nil), nil).Maybe() + Return(accounts, nil).Maybe() } func (m *MockAuthService) GetAllowedAccountsAPI(ctx context.Context, userID string) ([]string, error) { From 40325fc81b599a1084f6aa2ac272bd3050fedb56 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 05:08:57 +0200 Subject: [PATCH 2/2] fix(test): fail closed on empty constraintSets in the auth mock HasPermissionForConstraintsAPI's decision-function path (registered by grantPermissionsScoped, used by grantAdmin/grantAdminPurchaser/ grantScoped) answered purely from action/resource and never inspected constraintSets, so it allowed an empty slice for any verb the mocked principal holds. Production (auth.Service.HasPermissionForConstraintsAPI) fails closed on an empty constraintSets slice: it's a caller bug, not a grant. Harmless today since every current grant helper passes only Constraints == nil permissions and every constrained-permission test registers its own explicit mock.On(...) expectation, but a trap for a future test that reaches the constrained check through the auto-answering path instead. Guards the decision-function branch on len(constraintSets) == 0 and fails closed with an error, matching production exactly. Explicit mock.On(...).Return(bool, err) expectations are untouched. Adds TestGrantPermissionsScoped_ConstrainedCheckFailsClosedOnEmptyConstraintSets, covering both the fail-closed empty-set case and a positive control. Verified fail-then-pass: reverting the guard, the empty-set assertion fails with "An error is expected but got nil" (mock returned (true, nil)); with the guard, it returns (false, err) as production would. Also documents the ordering trap CodeRabbit's earlier N1 finding flagged on grantAdmin: a test-specific mock.On(...) expectation registered AFTER grantAdmin is silently shadowed by the generic catch-all grantAdmin registers first (testify serves the first matching expectation), so it must be registered before grantAdmin or expressed via grantPermissions instead. One sentence added to grantAdmin's doc comment ahead of the ~235 remaining grantAdmin conversions #1596 has left. Declined the third finding (middleware_test.go:321, grantAdmin -> grantAdminPurchaser on TestApproveViaSession_RequiresCSRF): verified by execution that the test passes identically with either principal, since approvePurchase's dispatch falls through unconditionally to approvePurchaseViaSession when token=="", and that function checks CSRF before re-running the RBAC check. Declined in a reply on the review thread with the execution evidence; no source change. --- internal/api/grantadmin_carveout_test.go | 32 ++++++++++++++++++++++++ internal/api/mocks_test.go | 19 ++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/internal/api/grantadmin_carveout_test.go b/internal/api/grantadmin_carveout_test.go index 4a75c3580..4059fdc92 100644 --- a/internal/api/grantadmin_carveout_test.go +++ b/internal/api/grantadmin_carveout_test.go @@ -159,3 +159,35 @@ func TestExecutePurchase_PlainAdminIsRefused(t *testing.T) { // (issue #1595: a name-only AssertNotCalled can never fail). mockStore.AssertNotCalled(t, "SavePurchaseExecution", mock.Anything, mock.Anything) } + +// TestGrantPermissionsScoped_ConstrainedCheckFailsClosedOnEmptyConstraintSets +// pins the mock's HasPermissionForConstraintsAPI to the same fail-closed +// contract as auth.Service.HasPermissionForConstraintsAPI (SEC-01, issue +// #1141): an empty constraintSets is a caller bug, not a grant. +// +// Before this fix, grantAdmin/grantAdminPurchaser/grantScoped's shared +// decision function answered purely from action/resource and never looked at +// constraintSets, so it allowed an empty slice for any held verb. Harmless +// today -- every current grant helper passes only Constraints == nil +// permissions, so no test exercises the divergence -- but a trap for the +// first constrained-permission test that reaches HasPermissionForConstraintsAPI +// through the auto-answering path instead of an explicit mock.On(...) +// expectation (found in review of #1596). +func TestGrantPermissionsScoped_ConstrainedCheckFailsClosedOnEmptyConstraintSets(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + mockAuth.grantAdminPurchaser() + + has, err := mockAuth.HasPermissionForConstraintsAPI(ctx, "u1", auth.ActionExecute, auth.ResourcePurchases, nil) + require.Error(t, err, "an empty constraintSets must be refused, matching auth.Service's fail-closed contract") + assert.False(t, has) + + // Positive control: the same verb with a non-empty constraint set still + // resolves through the decision function instead of being rejected + // outright. + has, err = mockAuth.HasPermissionForConstraintsAPI(ctx, "u1", auth.ActionExecute, auth.ResourcePurchases, + []auth.PermissionConstraints{{}}) + require.NoError(t, err) + assert.True(t, has) +} diff --git a/internal/api/mocks_test.go b/internal/api/mocks_test.go index b9c2cd10e..081a5f92c 100644 --- a/internal/api/mocks_test.go +++ b/internal/api/mocks_test.go @@ -2,6 +2,7 @@ package api import ( "context" + "fmt" "sync" "github.com/LeanerCloud/CUDly/internal/auth" @@ -293,6 +294,18 @@ func permissionDecision(args mock.Arguments, action, resource string) bool { func (m *MockAuthService) HasPermissionForConstraintsAPI(ctx context.Context, userID, action, resource string, constraintSets []auth.PermissionConstraints) (bool, error) { args := m.Called(ctx, userID, action, resource, constraintSets) + // The decision-function path (registered by grantPermissionsScoped) must + // mirror auth.Service.HasPermissionForConstraintsAPI's fail-closed + // contract: an empty constraintSets is a caller bug, not a grant. Explicit + // mock.On(...).Return(bool, err) expectations for constrained-permission + // tests are untouched -- those already encode the intended outcome for + // whatever constraintSets the test passes, via permissionDecision below. + if decide, ok := args.Get(0).(func(action, resource string) bool); ok { + if len(constraintSets) == 0 { + return false, fmt.Errorf("no permission constraint sets provided for %s on %s", action, resource) + } + return decide(action, resource), args.Error(1) + } return permissionDecision(args, action, resource), args.Error(1) } @@ -328,6 +341,12 @@ func (m *MockAuthService) allowConstraintChecks() { // have been deleted outright without a single test in this package failing, // so the #923 separation-of-duties control had no handler-level coverage at // all. See TestGrantAdmin_CarveOutIsEnforcedAtHandler. +// +// Register any test-specific mock.On(...) expectation for a method this +// grants (e.g. a HasPermissionAPI denial) BEFORE calling grantAdmin, or +// express it via grantPermissions([...]) instead: testify serves the first +// registered matching expectation, so one added after this call is silently +// shadowed by the generic one grantAdmin registers and never actually runs. func (m *MockAuthService) grantAdmin() { m.grantPermissions([]auth.Permission{{Action: auth.ActionAdmin, Resource: auth.ResourceAll}}) }