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..4059fdc92 --- /dev/null +++ b/internal/api/grantadmin_carveout_test.go @@ -0,0 +1,193 @@ +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) +} + +// 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/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..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" @@ -276,12 +277,36 @@ 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) + // 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) } func (m *MockAuthService) GetUserPermissionsAPI(ctx context.Context, userID string) (any, error) { @@ -301,26 +326,94 @@ 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. +// +// 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}}) +} + +// 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) {