fix(api/purchases): drop removed session.Role shortcut in execute-direct gate (closes #940)

[cristim]

Summary

This PR restores feat/multicloud-web-frontend to a buildable + pre-commit-clean state by fixing two independent base regressions from the recent merge wave.

Fix 1 — base does not compile (session.Role undefined)

• PR feat(auth): group-membership-only authorization, remove roles, require >=1 group (closes #907) #912 (Revamp authorization: group-membership-only (remove roles), require >=1 group per user #907) removed Role from api.Session (type Session struct { UserID string; Email string }), leaving authorizeSessionExecuteDirect with a dead if session.Role == "admin" { return nil } reference.
• Build error: internal/api/handler_purchases.go:520:13: session.Role undefined -- the base branch did not compile.
• Fix: remove the three-line admin-role shortcut and replace the gate-logic doc comment with the pattern used by authorizeSessionApprove / authorizeSessionCancel: admin users pass via the execute-any HasPermissionAPI check because the Administrators group carries the {admin, *} wildcard permission (exactly like the sibling gates).
• Also remove stale Role: "admin"/"user" fields from four existing Session struct literals in tests (now unknown fields caught by go vet), and update the test that assumed the admin-role short-circuit to properly stub the HasPermissionAPI call path.

Fix 2 — duplicate migration number 000058 (check-migration-conflicts hook failing)

The base merged two 000058 migrations from the recent wave, which the check-migration-conflicts pre-commit hook rejects:

• 000058_purchase_executions_direct_execute_audit.{up,down}.sql (feat(api,recs): execute-any/own permission gate on direct execute (closes #289) #803, schema change)
• 000058_seed_purchaser_group.{up,down}.sql (feat(auth): add Purchaser group + carve execute/approve-any/retry-any out of admin wildcard (closes #923) #924, data seed)

Fix: 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: the GroupPurchaser doc comment in internal/auth/types.go and the self-referencing error message string inside the up.sql.

Renumber safety: deploys have been failing because the base did not compile, so neither 000058 migration has been applied to any database yet. Renumbering before the next successful deploy is clean -- no applied-state reconciliation needed.

Regression tests added

Test	Assertion
TestHandler_authorizeSessionExecuteDirect_AdminGroupViaExecuteAny	Administrators-group user with {admin,*} wildcard is permitted via HasPermissionAPI(ExecuteAny) -- no dead Role shortcut
TestHandler_authorizeSessionExecuteDirect_NoGrant	Session without execute-any/own gets 403
TestHandler_authorizeSessionExecuteDirect_NilAuth	nil auth component returns 500 (fail-closed per feedback_fail_closed_middleware.md)

Test plan

• go build github.com/LeanerCloud/CUDly/internal/... succeeds
• go vet ./internal/api/... ./internal/auth/... passes
• go test ./internal/api/... -count=1 -- 1413 passed, 0 failed
• pre-commit run check-migration-conflicts --all-files -- Passed (no dup)
• All three new regression tests pass

Summary by CodeRabbit

• Bug Fixes

  • Strengthened authorization validation for admin-level purchase operations, ensuring more consistent and reliable permission enforcement through improved authentication mechanisms.

• Tests

  • Expanded authorization test coverage to include administrator permission scenarios, rejection cases for missing permissions, and error-handling edge cases to improve reliability.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR refactors purchase direct-execution authorization by replacing session-role-based admin shortcuts with an API key sentinel check (apiKeyAdminUserID), and updates the test suite to validate permission-matrix enforcement without role dependencies. Authorization logic now consistently applies RBAC gates for all non-admin cases.

Changes

Admin Authorization Bypass Refactor

Layer / File(s)	Summary
Admin API key authorization shortcut internal/api/handler_purchases.go	The initial always-permitted branch in authorizeSessionExecuteDirect changes from session.Role == "admin" to session.UserID == apiKeyAdminUserID, with updated gate documentation describing execute-any vs execute-own behavior and fail-closed handling when no matching permission grant exists.
Direct-execute authorization test suite internal/api/handler_purchases_test.go	Existing direct-execution tests remove role short-circuit expectations and explicitly mock permission-matrix flows (execute gate, execute-any/execute-own gates, allowed-accounts scope). Three new tests are added: admin-group approval via execute-any, 403 rejection when both execute-any and execute-own permissions are absent, and fail-closed 500 behavior when Handler.auth is nil.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

• LeanerCloud/CUDly#940: Both changes modify the same authorizeSessionExecuteDirect admin-shortcut logic, removing reliance on session.Role and changing how admin access is granted via API key sentinel.

Possibly related PRs

• LeanerCloud/CUDly#726: Both PRs modify the authorization gate for purchase "execute/direct" flows, updating admin-bypass behavior and associated permission-check mock expectations in authorizeSessionExecuteDirect.

Suggested labels

effort/s

Poem

A rabbit hops through auth's great hall,
Where roles once ruled but role no more,
The API key now stands tall,
Permission checks enforce the law,
No shortcuts—just what's due. 🐰✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: removing the session.Role shortcut from the execute-direct authorization gate, which was necessary after session.Role was removed in a prior PR.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/940-base-build-execute-direct

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

🧹 Nitpick comments (1)

internal/api/handler_purchases_test.go (1)

3219-3274: ⚡ Quick win

Add a regression test for the apiKeyAdminUserID bypass.

These additions cover the execute-any path, but not the actual stateless admin API-key short-circuit introduced in authorizeSessionExecuteDirect. A future regression could break that sentinel branch while this suite still passes through HasPermissionAPI("execute-any").

Proposed test

+func TestHandler_authorizeSessionExecuteDirect_AdminAPIKeySentinel(t *testing.T) {
+	ctx := context.Background()
+	session := &Session{UserID: apiKeyAdminUserID}
+
+	handler := &Handler{auth: new(MockAuthService)}
+	err := handler.authorizeSessionExecuteDirect(ctx, session, "")
+
+	require.NoError(t, err)
+}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/api/handler_purchases_test.go` around lines 3219 - 3274, Add a
regression test that verifies the stateless admin API-key short-circuit: create
a session whose UserID is the apiKeyAdminUserID constant, construct a Handler
with auth == nil, call authorizeSessionExecuteDirect(ctx, session, anyCreatorID)
and assert no error (i.e. admin API key bypasses HasPermissionAPI); reference
the apiKeyAdminUserID, Handler, Session, and authorizeSessionExecuteDirect
symbols when adding this test (mirror style of existing
TestHandler_authorizeSessionExecuteDirect_* tests).

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/api/handler_purchases_test.go`:
- Around line 3219-3274: Add a regression test that verifies the stateless admin
API-key short-circuit: create a session whose UserID is the apiKeyAdminUserID
constant, construct a Handler with auth == nil, call
authorizeSessionExecuteDirect(ctx, session, anyCreatorID) and assert no error
(i.e. admin API key bypasses HasPermissionAPI); reference the apiKeyAdminUserID,
Handler, Session, and authorizeSessionExecuteDirect symbols when adding this
test (mirror style of existing TestHandler_authorizeSessionExecuteDirect_*
tests).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 99841b6f-71ec-4b92-91a5-ad16b1c18e30

📥 Commits

Reviewing files that changed from the base of the PR and between 6777346 and 501f30d.

📒 Files selected for processing (2)

• internal/api/handler_purchases.go
• internal/api/handler_purchases_test.go

[cristim]

Pushed a second base-regression fix to this PR (commit d7a65fa).

The check-migration-conflicts pre-commit hook was failing on a duplicate migration number 000058 in the base: the recent wave merged both 000058_purchase_executions_direct_execute_audit (#803, schema change) and 000058_seed_purchaser_group (#924, data seed).

Renumbered the seed migration to 000059 (kept the schema-change audit at 000058; base jumps 000058 -> 000063 so 000059-000062 are free). golang-migrate discovers by filename glob, so no embed list changed; updated only the two self-references (the GroupPurchaser doc comment and the up.sql error-message string).

Safe to renumber without applied-state reconciliation: the base hasn't compiled since the #803/#907 break, so neither 000058 migration has been applied to any DB yet.

@coderabbitai review

[coderabbitai]

@cristim: Thanks for the detailed explanation on the migration renumbering fix. The reasoning is sound — since neither 000058 migration has been applied to any DB yet (the base hasn't compiled since the #803/#907 break), renumbering the seed to 000059 is safe and keeps the schema-change audit migration at its original number. Triggering a review of the new commit now.

---

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.