From 501f30dd006821ebf1a9ca48a8336b74acf24a22 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 4 Jun 2026 00:47:25 +0200 Subject: [PATCH 1/2] fix(api/purchases): drop removed session.Role shortcut in execute-direct gate (closes #940) PR #912 (#907) removed the Role field from api.Session, leaving authorizeSessionExecuteDirect with a dead `if session.Role == "admin"` reference that prevented the base branch from compiling. Remove the three-line shortcut and update the gate-logic doc comment to match the sibling authorizeSessionApprove/authorizeSessionCancel pattern: admins pass via the execute-any HasPermissionAPI check because the Administrators group carries the {admin,*} wildcard permission. Also fix all test Session struct literals that still set Role (now an unknown field), update the admin-role-shortcut test to properly stub the HasPermissionAPI path, and add three regression tests: - AdminGroupViaExecuteAny: confirms admin users still pass via execute-any - NoGrant: confirms non-admin without execute-any/own gets 403 - NilAuth: confirms nil auth component returns 500 (fail-closed) --- internal/api/handler_purchases.go | 10 ++-- internal/api/handler_purchases_test.go | 73 +++++++++++++++++++++++--- 2 files changed, 74 insertions(+), 9 deletions(-) diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index b366d5ccd..c8e1ae821 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -510,14 +510,18 @@ func (h *Handler) authorizeSessionApprove(ctx context.Context, session *Session, // resolveCreatorUserID before this call; "" on non-human or legacy rows). // // Gate logic (mirrors authorizeSessionApprove / authorizeSessionCancel): -// - admin role: always permitted. -// - execute-any: permitted regardless of creator. +// - stateless admin API key: always permitted (apiKeyAdminUserID sentinel). +// - execute-any: permitted regardless of creator. Administrators-group users +// pass here because {admin, *} matches ActionExecuteAny. // - execute-own: permitted only when creatorID == session.UserID and both // are non-empty (prevents an empty-string collision from granting access). // - no matching grant: 403 fail-closed; nil auth component is a 500 as // per feedback_fail_closed_middleware.md. func (h *Handler) authorizeSessionExecuteDirect(ctx context.Context, session *Session, creatorID string) error { - if session.Role == "admin" { + // Stateless admin API key: full access, no user row. Administrators-group + // users pass via the execute-any HasPermissionAPI check below, since + // {admin, *} matches any requested permission. + if session.UserID == apiKeyAdminUserID { return nil } if h.auth == nil { diff --git a/internal/api/handler_purchases_test.go b/internal/api/handler_purchases_test.go index 978f86f50..9a946bac9 100644 --- a/internal/api/handler_purchases_test.go +++ b/internal/api/handler_purchases_test.go @@ -3080,7 +3080,6 @@ func TestHandler_executePurchase_DirectExec_NoPermission(t *testing.T) { userSession := &Session{ UserID: "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb", Email: "user@example.com", - Role: "user", } mockAuth.On("ValidateSession", ctx, "user-token").Return(userSession, nil) // Base execute:purchases grant — passes the validateExecutePurchaseRequest @@ -3120,11 +3119,17 @@ func TestHandler_executePurchase_DirectExec_ExecuteAny(t *testing.T) { adminSession := &Session{ UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com", - Role: "admin", } mockAuth.On("ValidateSession", ctx, "admin-token").Return(adminSession, nil) - // Admin role short-circuits the permission matrix (no HasPermissionAPI call - // expected) but we still need ApproveAndExecute on the purchase mock. + // execute-any grant covers admin users (Administrators-group {admin,*} + // wildcard matches ActionExecuteAny). The old session.Role shortcut was + // removed in issue #940 — HasPermissionAPI is now always consulted. + // First, the outer gate: requirePermission("execute","purchases"). + mockAuth.On("HasPermissionAPI", ctx, adminSession.UserID, "execute", "purchases").Return(true, nil) + // Then the direct-execute gate: authorizeSessionExecuteDirect("execute-any"). + mockAuth.On("HasPermissionAPI", ctx, adminSession.UserID, "execute-any", "purchases").Return(true, nil) + // Scope check: no allowed_accounts restriction for this test. + mockAuth.On("GetAllowedAccountsAPI", ctx, adminSession.UserID).Return([]string{}, nil) mockPurchase.On("ApproveAndExecute", ctx, mock.AnythingOfType("string"), adminSession.Email).Return(nil) setupDirectExecMocks(ctx, mockStore) @@ -3157,7 +3162,6 @@ func TestHandler_executePurchase_DirectExec_ExecuteOwn_Owner(t *testing.T) { ownerSession := &Session{ UserID: ownerID, Email: "owner@example.com", - Role: "user", } mockAuth.On("ValidateSession", ctx, "owner-token").Return(ownerSession, nil) mockAuth.On("HasPermissionAPI", ctx, ownerID, "execute", "purchases").Return(true, nil) @@ -3198,7 +3202,7 @@ func TestHandler_authorizeSessionExecuteDirect_ExecuteOwn_NonOwner(t *testing.T) sessionUserID := "dddddddd-dddd-dddd-dddd-dddddddddddd" differentCreatorID := "eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee" - session := &Session{UserID: sessionUserID, Role: "user"} + session := &Session{UserID: sessionUserID} mockAuth.On("HasPermissionAPI", ctx, sessionUserID, "execute-any", "purchases").Return(false, nil) mockAuth.On("HasPermissionAPI", ctx, sessionUserID, "execute-own", "purchases").Return(true, nil) @@ -3211,3 +3215,60 @@ func TestHandler_authorizeSessionExecuteDirect_ExecuteOwn_NonOwner(t *testing.T) assert.Equal(t, 403, ce.code) assert.Contains(t, ce.Error(), "execute-own requires you to be the creator") } + +// TestHandler_authorizeSessionExecuteDirect_AdminGroupViaExecuteAny verifies +// that an Administrators-group user whose {admin,*} wildcard resolves to +// execute-any is PERMITTED by the HasPermissionAPI path (not a dead +// session.Role shortcut that was removed in issue #940). +func TestHandler_authorizeSessionExecuteDirect_AdminGroupViaExecuteAny(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + adminUserID := "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa" + creatorID := "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb" + session := &Session{UserID: adminUserID} + + // Administrators-group wildcard {admin,*} covers execute-any. + mockAuth.On("HasPermissionAPI", ctx, adminUserID, "execute-any", "purchases").Return(true, nil) + + handler := &Handler{auth: mockAuth} + err := handler.authorizeSessionExecuteDirect(ctx, session, creatorID) + require.NoError(t, err) +} + +// TestHandler_authorizeSessionExecuteDirect_NoGrant verifies that a session +// without execute-any or execute-own on purchases is rejected with 403. +func TestHandler_authorizeSessionExecuteDirect_NoGrant(t *testing.T) { + ctx := context.Background() + mockAuth := new(MockAuthService) + t.Cleanup(func() { mockAuth.AssertExpectations(t) }) + + userID := "cccccccc-cccc-cccc-cccc-cccccccccccc" + creatorID := "dddddddd-dddd-dddd-dddd-dddddddddddd" + session := &Session{UserID: userID} + + mockAuth.On("HasPermissionAPI", ctx, userID, "execute-any", "purchases").Return(false, nil) + mockAuth.On("HasPermissionAPI", ctx, userID, "execute-own", "purchases").Return(false, nil) + + handler := &Handler{auth: mockAuth} + err := handler.authorizeSessionExecuteDirect(ctx, session, creatorID) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError") + assert.Equal(t, 403, ce.code) +} + +// TestHandler_authorizeSessionExecuteDirect_NilAuth verifies that a nil auth +// component returns 500 (fail-closed per feedback_fail_closed_middleware.md). +func TestHandler_authorizeSessionExecuteDirect_NilAuth(t *testing.T) { + ctx := context.Background() + session := &Session{UserID: "eeeeeeee-eeee-eeee-eeee-eeeeeeeeeeee"} + + handler := &Handler{auth: nil} + err := handler.authorizeSessionExecuteDirect(ctx, session, "") + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "expected a clientError") + assert.Equal(t, 500, ce.code) +} From d7a65fa88ac140989fd3f4e0faebd6b9ff2c3987 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 4 Jun 2026 01:12:54 +0200 Subject: [PATCH 2/2] fix(db): renumber duplicate migration 000058_seed_purchaser_group to 000059 (base collision from #803+#924) The base feat/multicloud-web-frontend merged two 000058 migrations from the recent wave, which the check-migration-conflicts pre-commit hook rejects: - 000058_purchase_executions_direct_execute_audit (#803, schema change) - 000058_seed_purchaser_group (#924, data seed) Keep the schema-change audit migration at 000058 and renumber the seed migration to the next free slot, 000059 (base jumps 000058 -> 000063, so 000059-000062 are open). golang-migrate discovers migrations by filename glob, so no embed list needs updating; only two self-references were adjusted: - auth/types.go GroupPurchaser doc comment pointing at the filename - the self-referencing error message inside the up.sql Safe to renumber without applied-state reconciliation: the base has not compiled since the #803/#907 break, so neither 000058 migration has been applied to any database yet. --- internal/auth/types.go | 2 +- ...aser_group.down.sql => 000059_seed_purchaser_group.down.sql} | 0 ...urchaser_group.up.sql => 000059_seed_purchaser_group.up.sql} | 2 +- 3 files changed, 2 insertions(+), 2 deletions(-) rename internal/database/postgres/migrations/{000058_seed_purchaser_group.down.sql => 000059_seed_purchaser_group.down.sql} (100%) rename internal/database/postgres/migrations/{000058_seed_purchaser_group.up.sql => 000059_seed_purchaser_group.up.sql} (98%) diff --git a/internal/auth/types.go b/internal/auth/types.go index 3370d62f9..827dfa075 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -317,7 +317,7 @@ const DefaultPurchaserGroupID = "00000000-0000-5000-8000-000000000005" // GroupPurchaser is the canonical name of the system-managed Purchaser // group. MUST match the literal name inserted by migration -// 000058_seed_purchaser_group.up.sql so name-based lookups agree with +// 000059_seed_purchaser_group.up.sql so name-based lookups agree with // the seeded row. const GroupPurchaser = "Purchaser" diff --git a/internal/database/postgres/migrations/000058_seed_purchaser_group.down.sql b/internal/database/postgres/migrations/000059_seed_purchaser_group.down.sql similarity index 100% rename from internal/database/postgres/migrations/000058_seed_purchaser_group.down.sql rename to internal/database/postgres/migrations/000059_seed_purchaser_group.down.sql diff --git a/internal/database/postgres/migrations/000058_seed_purchaser_group.up.sql b/internal/database/postgres/migrations/000059_seed_purchaser_group.up.sql similarity index 98% rename from internal/database/postgres/migrations/000058_seed_purchaser_group.up.sql rename to internal/database/postgres/migrations/000059_seed_purchaser_group.up.sql index cd6a7b2dd..0c66eec62 100644 --- a/internal/database/postgres/migrations/000058_seed_purchaser_group.up.sql +++ b/internal/database/postgres/migrations/000059_seed_purchaser_group.up.sql @@ -35,7 +35,7 @@ BEGIN AND id <> '00000000-0000-5000-8000-000000000005' ) THEN RAISE EXCEPTION - 'migration 000058: a group named ''Purchaser'' already exists with a different id; rename it before applying this migration so the seeded id (00000000-0000-5000-8000-000000000005) can be created'; + 'migration 000059: a group named ''Purchaser'' already exists with a different id; rename it before applying this migration so the seeded id (00000000-0000-5000-8000-000000000005) can be created'; END IF; END $$;