refactor(api): requireAuth returns resolved Principal (closes #194)

[cristim]

Summary

• requireAuth now returns (*Principal, error) instead of bare error by delegating to the new authenticatePrincipal helper (already added in this branch)
• authenticatePrincipal cleaned up: nil-auth guard moved before the API-key path (fail-closed per feedback_fail_closed_middleware.md), dead userIface/userFields locals removed
• Router updated to _, err := r.h.requireAuth(...) to handle the new return shape
• Existing TestRequireAuth_* tests upgraded to assert the returned Principal (kind, role, UserID, Session pointer) for each credential type

Test plan

• go build ./... clean
• go test github.com/LeanerCloud/CUDly/internal/api/... github.com/LeanerCloud/CUDly/internal/auth/... — 1852 passed
• TestRequireAuth_AdminAPIKey now asserts PrincipalAdminAPIKey / role=admin
• TestRequireAuth_UserSession now asserts PrincipalSession, UserID, Role, Session pointer
• TestRequireAuth_NoCredential_Rejects still asserts 401 ClientError

Closes #194

Summary by CodeRabbit

• Refactor
  • Upgraded authentication to resolve and return caller identity consistently across admin API keys, user API keys, and bearer sessions.
  • Centralized authentication enforcement so requests receive a uniform 401 response when credentials are missing or invalid.
• Tests
  • Expanded coverage for authentication outcomes, including success and failure cases for admin keys, user keys (including unexpected validator behavior), and bearer sessions.
  • Strengthened assertions to verify identity and 401 error details are returned as expected.

[coderabbitai]

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 40e46332-f9f5-4c8f-8d0a-1f168cd215ce

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Introduces a Principal identity model (PrincipalKind constants and a Principal struct) into the API middleware layer. Replaces the boolean authentication path with authenticatePrincipal, which returns a typed *Principal or a 401 ClientError. Updates requireAuth to return (*Principal, error) and wires the router's AuthUser branch accordingly. Extends existing tests and adds three new test cases covering all credential kinds and fail-closed edge cases.

Changes

Principal identity model and requireAuth refactor

Layer / File(s)	Summary
Principal type and kind constants internal/api/middleware.go	Adds PrincipalKind string type with PrincipalAdminAPIKey, PrincipalUserAPIKey, PrincipalSession constants, and the exported Principal struct carrying Kind, UserID, Email, and Session.
authenticatePrincipal and credential resolution helpers internal/api/middleware.go	Replaces boolean authenticate with authenticatePrincipal returning *Principal or 401 ClientError; adds principalFromUserAPIKey (fail-closed on unexpected concrete type) and principalFromBearerToken (validates session and constructs Principal with Session attached).
requireAuth signature change and router wiring internal/api/middleware.go, internal/api/router.go	requireAuth now returns (*Principal, error) by delegating to authenticatePrincipal; the AuthUser branch in Router.Route is updated to match the new signature.
Extended requireAuth test coverage internal/api/router_authuser_test.go	Strengthens existing AdminAPIKey and UserSession tests to assert Principal fields and 401 ClientError on failure; adds TestRequireAuth_UserAPIKey (valid *auth.User), TestRequireAuth_UserAPIKey_BadRecord (fail-closed 401), and TestRequireAuth_NilAuth_Rejects (nil-auth 401).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• #194 — This PR directly implements the proposal in #194: adds the Principal struct with Kind/UserID/Email/Session fields, introduces authenticatePrincipal and the requireAuth(*Principal, error) signature, and extends router_authuser_test.go with per-kind principal assertions, matching every stated constraint and test requirement.
• #1010 — Identifies structural incompatibilities: the current code defines Principal without a Role field (whereas issue #1010 expects role-based identity), and principalFromUserAPIKey validates against *auth.User (expected interface alignment for this PR's implementation).

Possibly related PRs

• LeanerCloud/CUDly#364 — Both PRs modify internal/api/router.go's Router.Route authentication enforcement path—this PR changes the AuthUser branch to use a new requireAuth that returns a resolved *Principal, while retrieved PR hardens per-route Route.Auth handling in that same routing logic.

Suggested labels

type/feat, effort/m

🐇 Hop hop, identity found,
No more re-fetching, no more round-and-round!
Principal leaps from the auth gate with grace,
Kind, UserID, Email, and Session all fall into place.
Three kinds stand guard — Admin, User, Session bright —
Fail-closed and tested, the rabbit's work done right! 🌟

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Linked Issues check	❓ Inconclusive	The PR implements the core objectives from #194 (return Principal from requireAuth, eliminate redundant identity resolution) but the PR objectives document confirms the changes are UNSTABLE due to upstream PR #907 breaking changes that removed Session.Role and User.Role, causing build/runtime failures that require follow-up issue #1010.	Resolve upstream breaking changes from PR #907 (Session.Role and User.Role removal) and implement the Principal{GroupIDs} reshape proposed in follow-up issue #1010 before merging to ensure the code is functional and aligns with the group-based authorization model.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: refactoring requireAuth to return a resolved Principal object, with the linked issue reference clearly indicating the purpose.
Out of Scope Changes check	✅ Passed	All code changes are directly scoped to the refactoring objectives: adding Principal type/constants, refactoring authenticatePrincipal/requireAuth, updating the router call site, and extending test coverage for credential types.
Docstring Coverage	✅ Passed	Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/194-wave18

---

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

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

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.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

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.

[cristim]

@coderabbitai review

[coderabbitai]

Rate Limit Exceeded

@cristim have exceeded the limit for the number of chat messages per hour. Please wait 35 minutes and 20 seconds before sending another message.

[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.

[cristim]

Adversarial review summary

UNSTABLE state is from pre-commit (CI run 27020681187) failing on the merge ref with:

internal/api/middleware.go:166:20: session.Role undefined (type *Session has no field or method Role)

Root cause is base divergence, not a fixable-in-place defect. PR #907 (feat(auth): group-membership-only authorization, remove roles, require >=1 group) has since landed on feat/multicloud-web-frontend and removed both Session.Role and User.Role. The Principal refactor in this PR predates that reshape and still encodes a role-based identity in three load-bearing spots:

File	Where	What breaks against the new base
internal/api/middleware.go:67	Principal.Role string	Field semantics gone; the right shape is groups / derived permissions.
internal/api/middleware.go:164	principalFromBearerToken reads session.Role	Build break — base Session has only UserID, Email.
internal/api/middleware.go:131-145	principalFromUserAPIKey asserts userRaw.(interface{ GetID()string; GetEmail()string; GetRole()string })	Silent runtime regression — base *auth.User has none of these accessors, so the assertion always fails closed. Every valid user-API-key request would 401 after the rebase. Tests don't catch it because they use a synthetic testUserRecord.
internal/api/router_authuser_test.go:72, 123, 154-187	Tests construct Session{UserID, Role:"user"} and testUserRecord.GetRole()	Won't compile against rebased base.

There's also a smaller signature drift in base (requiresCSRFValidation(method, path, req) from #404) that the rebase has to align — not directly broken by this PR, but the conflict will surface.

Risk-surface walk

• Auth fail-closed: nil-auth path is correct (authenticatePrincipal returns 401 before any h.auth.* call after the admin-API-key check). Matches feedback_fail_closed_middleware.md. Clean (on this branch's own HEAD).
• Identity shape: stale per the table above. Out-of-scope to fix as a tactical commit because the redesign sets the contract every future handler will see.
• Callers: requireAuth has only one prod caller (internal/api/router.go:344, already updated to _, err :=). No downstream code reads Principal.* yet, so the blast radius of the redesign is confined to this PR's reshape.
• Tests: TestRequireAuth_{AdminAPIKey,UserSession,UserAPIKey,UserAPIKey_BadRecord,NoCredential_Rejects,NilAuth_Rejects} cover the on-branch matrix, but against the rebased base the {user-with-admin-group, user-with-non-admin-group, user-with-zero-groups} cells need to be reintroduced in place of the Role:"user" / Role:"admin" cells.
• CR signal: no actionable inline findings — CR was rate-limited across all three review attempts in this window.

Action taken

• No in-scope tactical commits added. The fix is a design-bearing rebase + Principal reshape that should not be made unilaterally — it sets the typed-identity contract for downstream handlers (the whole point of this refactor).
• Follow-up issue filed: refactor(api/Principal): rebase #864 onto post-#907 group-only authz model (closes followup) #1010 with triaged priority/p1 severity/high urgency/this-sprint impact/internal effort/m type/chore. Body has a proposed Principal{GroupIDs} shape + acceptance criteria; happy to iterate on the proposal there.

Once #1010's design is settled the rebase + ship can land as a follow-up commit on this PR.

@coderabbitai review

[coderabbitai]

@cristim: Excellent adversarial breakdown — the three-spot break table and the silent-runtime-regression call-out on principalFromUserAPIKey are exactly the kind of pre-merge audit this refactor needed before landing. Triggering the incremental review now; the rate-limit window should be clear.

A few notes to complement your summary:

• Silent-fail-closed in principalFromUserAPIKey is the sharpest risk: the type assertion will always fail against the rebased *auth.User, so every user-API-key request silently 401s rather than producing a build error. Worth calling out explicitly in the #1010 acceptance criteria if not already there.
• Test cell mapping: the {user-with-admin-group, user-with-non-admin-group, user-with-zero-groups} replacement cells you noted should also cover the "zero-groups user is rejected at the authentication layer" invariant that PR #907 introduced (require >=1 group).
• requiresCSRFValidation drift (from #404): low severity today, but worth a single alignment commit at rebase time so it doesn't accumulate into a larger conflict surface.

Adding a learning about the group-membership model so future reviews don't flag Principal.Role as an obvious addition.

---

✏️ Learnings added

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/864

Timestamp: 2026-06-06T07:04:28.213Z
Learning: In LeanerCloud/CUDly, PR `#907` (`feat(auth): group-membership-only authorization, remove roles, require >=1 group`) landed on `feat/multicloud-web-frontend` and removed `Session.Role` and `User.Role` entirely. Any `Principal` struct must use `GroupIDs []string` (or similar group-based field) instead of `Role string`. The `*auth.User` type no longer exposes `GetRole()`, `GetID()`, or `GetEmail()` accessors via interface assertion — using those in a type assertion causes a silent always-false fail-closed regression at runtime. The group-membership model also requires >=1 group for a valid user session; zero-groups users must be rejected at the authentication layer. Follow-up design issue is `#1010`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

---

🧠 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.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:41:06.830Z
Learning: In LeanerCloud/CUDly PR `#804`, `canRevokeCompletedRow` in `frontend/src/history.ts` must check `canAccess` (not just `getCurrentUser()` truthiness) to align with peer predicates `canCancelPendingRow`, `canApprovePendingRow`, and `canRetryFailedRow`. `revoke-own` and `revoke-any` were added to the closed `Action` union to make future UX-vs-RBAC drift a compile error.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.770Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), the Dashboard "Upcoming purchases" widget (`dashboard.ts`) has two Cancel entry points (card-level and modal) that both call `api.deletePlannedPurchase`. The `UpcomingPurchase` response DTO must include `CreatedByUserID *string` (omitempty, mirroring `PlannedPurchase`) so the frontend can gate these buttons via a `canCancelUpcomingPurchase` helper that mirrors `canManageScheduledPurchase` on the Plans page. Without this, the backend correctly 403s non-owners but the UI has no gate and the UX is broken. Regressions: `frontend/src/__tests__/dashboard-ownership-950.test.ts` (6 tests) and `TestHandler_getUpcomingPurchases_PropagatesCreatedByUserID`. Fixed in commit 94326f6b9.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.770Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), `POST /api/plans/{id}/purchases` (`createPurchaseExecutionsTx`) must stamp `created_by_user_id` via `resolveCreatorUserID(session)` at creation time. Without this, every scheduled purchase created via the Plans-page "Create planned purchases" action ships a NULL creator, which the ownership gate treats as a legacy row and denies management to all non-`update-any` users — including the creator themselves. The admin API key path must set `CreatedByUserID` to nil (not the key identity). Regressions: `TestHandler_createPlannedPurchases_StampsCreator` and `TestHandler_createPlannedPurchases_AdminAPIKeyCreatorIsNil`. Fixed in commit 9ef1b4352.

✅ 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.

[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]

Actionable comments posted: 1

🧹 Nitpick comments (2)

internal/api/router_authuser_test.go (1)

205-219: ⚡ Quick win

Add nil-auth coverage for bearer-token credentials too.

Current nil-auth coverage only exercises X-API-Key (Lines 212-213). Add a bearer-token request case so both non-admin credential paths are pinned to fail closed.

🤖 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/router_authuser_test.go` around lines 205 - 219, The
TestRequireAuth_NilAuth_Rejects test currently only exercises the X-API-Key
credential path when auth is nil. Add an additional test case or extend the
existing test to also verify that bearer-token credentials fail with a 401
ClientError when auth is nil. This ensures both non-admin credential paths
(X-API-Key and bearer token) are pinned to fail closed in the requireAuth
method.

internal/api/router.go (1)

376-376: 🏗️ Heavy lift

Propagate the resolved Principal instead of dropping it.

Line 376 ignores the returned principal, so handlers still can’t consume caller identity directly and may continue re-resolving auth context. Consider threading *Principal via handler params or request context.

🤖 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/router.go` at line 376, The `requireAuth` call in the router is
discarding the returned `Principal` value using the blank identifier, which
forces handlers to repeatedly resolve authentication context instead of
consuming the caller identity directly. Capture the returned `Principal` from
the `requireAuth` method instead of dropping it, and thread it through the
handler parameters or store it in the request context so that downstream
handlers can access the resolved identity without needing to re-resolve the auth
context.

🤖 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.

Inline comments:
In `@internal/api/middleware.go`:
- Around line 65-70: The Principal struct contains a deprecated Role field that
encodes outdated role-based authorization semantics, which conflicts with the
new group-based auth model. Remove the Role field definition from the Principal
struct (currently at line 69 with the comment about "admin" for
PrincipalAdminAPIKey) and remove the code that sets Role to "admin" when
creating Principal instances (at line 96). This will eliminate stale
authorization signals and allow proper enforcement of group-based authorization
invariants.

---

Nitpick comments:
In `@internal/api/router_authuser_test.go`:
- Around line 205-219: The TestRequireAuth_NilAuth_Rejects test currently only
exercises the X-API-Key credential path when auth is nil. Add an additional test
case or extend the existing test to also verify that bearer-token credentials
fail with a 401 ClientError when auth is nil. This ensures both non-admin
credential paths (X-API-Key and bearer token) are pinned to fail closed in the
requireAuth method.

In `@internal/api/router.go`:
- Line 376: The `requireAuth` call in the router is discarding the returned
`Principal` value using the blank identifier, which forces handlers to
repeatedly resolve authentication context instead of consuming the caller
identity directly. Capture the returned `Principal` from the `requireAuth`
method instead of dropping it, and thread it through the handler parameters or
store it in the request context so that downstream handlers can access the
resolved identity without needing to re-resolve the auth context.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 02a9daaf-cf11-47ac-aa19-b48d324df676

📥 Commits

Reviewing files that changed from the base of the PR and between 451a70f and c4c2ce1.

📒 Files selected for processing (3)

• internal/api/middleware.go
• internal/api/router.go
• internal/api/router_authuser_test.go

[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.