refactor(api/Principal): rebase #864 onto post-#907 group-only authz model (closes followup)

[cristim]

Summary

PR #864 (refactor(api): requireAuth returns resolved Principal, branch fix/194-wave18) is structurally incompatible with the current feat/multicloud-web-frontend base after PR #907 (group-membership-only authorization, removed Session.Role and User.Role) landed. The PR predates that reshape and still encodes role-based identity in the new Principal type. A rebase plus a small design call is needed before the PR can land. Adversarial review of PR #864 surfaced this in the failing CI run on commit f6d3dc51f (internal/api/middleware.go:166:20: session.Role undefined (type *Session has no field or method Role)).

Where it lives

• internal/api/middleware.go (PR refactor(api): requireAuth returns resolved Principal (closes #194) #864 introduced types/funcs):
  • Principal.Role string (line 67)
  • principalFromBearerToken reads session.Role (line 164)
  • principalFromUserAPIKey asserts userRaw.(interface{ GetID()string; GetEmail()string; GetRole()string }) (lines 131-145)
• internal/api/router_authuser_test.go: builds Session{UserID, Role: "user"} and a testUserRecord whose GetRole() returns "user" (lines 72, 123, 154-187)
• Base post-PR Revamp authorization: group-membership-only (remove roles), require >=1 group per user #907:
  • internal/api/types.go Session has only UserID, Email (no Role)
  • internal/auth/types.go User no longer has Role; GroupIDs []string replaces it
  • internal/api/middleware.go requireAdmin uses HasPermissionAPI(ctx, userID, ActionAdmin, ResourceAll) instead of session.Role == "admin"
  • requiresCSRFValidation(method, path, req) (added req param, issue sec: CSRF exemption for approve/cancel POST is correct but overly broad — scope to token-only requests only #404)

Why this needs a design call rather than a tactical patch

If I had just dropped session.Role from Principal and the bearer-path Principal construction, three further problems would remain:

1. Silent runtime regression on the user-API-key path. principalFromUserAPIKey does a structural interface assertion against userRaw requiring GetID/GetEmail/GetRole. On base, *auth.User exposes none of those accessors — the assertion ALWAYS fails closed at runtime. Every valid user-API-key request would 401. Tests don't catch it because they pass a synthetic testUserRecord that satisfies the interface. Fixing it requires picking a real shape for the field extraction (add accessor methods on auth.User, or convert via auth.ValidateUserAPIKeyAPI to a typed *APIUser, or push the conversion into the interface).
2. Principal as "identity + role" is the wrong shape post-Revamp authorization: group-membership-only (remove roles), require >=1 group per user #907. Authorization is now derived from group permissions, not a Role string. The new Principal should either carry GroupIDs []string (and let downstream handlers fan out to HasPermissionAPI), or expose a thin IsAdmin()/Permissions view computed at construction. Both are reasonable but mutually exclusive. Picking one sets the contract every future caller will see (the whole point of PR refactor(api): requireAuth returns resolved Principal (closes #194) #864 is to give downstream handlers a typed identity).
3. Tests need to be re-written, not just adjusted. The {has-auth, lacks-auth, admin, user} cross-product is currently expressed against Role. In the group-only model the cells need to be {admin-api-key, user-with-admin-group, user-with-other-groups, user-with-zero-groups, no-credential, invalid-credential} — coverage that the current PR doesn't reach.

Proposed shape (for discussion)

type Principal struct {
    Kind     PrincipalKind
    UserID   string   // empty for PrincipalAdminAPIKey
    Email    string   // empty for PrincipalAdminAPIKey
    GroupIDs []string // post-#907 authz scope; "*" sentinel for admin-API-key
    Session  *Session // non-nil only for PrincipalSession
}

with a helper func (p *Principal) HasPermission(ctx, auth, action, resource) that fans out to AuthServiceInterface.HasPermissionAPI for session/user-API-key principals and short-circuits true for admin-API-key. That keeps the "callers don't re-resolve identity" goal of #194 without re-introducing the role string.

Acceptance criteria

• Rebase fix/194-wave18 onto current feat/multicloud-web-frontend HEAD; resolve internal/api/middleware.go + internal/api/router_authuser_test.go conflicts.
• Drop Principal.Role; replace with whatever shape is agreed (default: GroupIDs []string + small helper).
• Replace the local GetID/GetEmail/GetRole interface assertion in principalFromUserAPIKey with the agreed extraction path; add an integration-style test that exercises the real *auth.User value path (catches the regression that the current PR ships).
• Tests cover the cross-product {admin-api-key, user-with-admin-group, user-with-non-admin-group, user-with-zero-groups, no-credential, invalid-bearer, valid-user-api-key, invalid-user-api-key, nil-auth} — at least one cell per dimension.
• requireAdmin is NOT re-introduced as a role-based check (Revamp authorization: group-membership-only (remove roles), require >=1 group per user #907 already moved it to HasPermissionAPI).
• Pre-commit CI green on the merge ref (no vet failure).

Related

• PR refactor(api): requireAuth returns resolved Principal (closes #194) #864 (this work)
• Issue refactor(api): return resolved Principal from requireAuth/authenticate so handlers don't re-fetch identity #194 (original ask)
• PR Revamp authorization: group-membership-only (remove roles), require >=1 group per user #907 / commit 8880710a8 (group-membership-only authz that caused the divergence)
• feedback_fail_closed_middleware.md (the nil-auth/bad-record paths must continue to fail closed)

[cristim]

Verification sweep against main (101f099fb) on 2026-07-27 finds this already resolved.
Principal no longer has a Role field, so the structural-assertion regression this tracked is fixed.
Evidence: commit 702b886 (PR #864).
Recommending close.

[cristim]

Already implemented on main. Closing with evidence rather than scheduling work.

Verified against origin/main (ae1e632cb):

• Principal.Role is gone. internal/api/middleware.go:73 defines Principal with Kind, Session, UserID, Email, APIKeyID and no role field. The remaining Principal/Role co-occurrences in the tree are unrelated (Azure service principals, IAM policy tests).
• The structural GetID/GetEmail/GetRole assertion is replaced by a fail-closed concrete-type assertion. middleware.go:162 does user, ok := userRaw.(*auth.User) and denies on failure, with the rationale recorded inline: a partially-populated Principal is never returned.
• The real-*auth.User test coverage, the cross-product cells, and requireAdmin going through HasPermissionAPI(ActionAdmin, ResourceAll) are all present.

Landed in 702b8860b, extended by 427e81b7c. Neither carried a closing trailer, which is why this stayed open.

If a residual axis remains that this does not cover, please reopen naming it specifically.