Correct three disproven claims in the Martyrology handoff - #21
Conversation
A live authorization-code flow against production on 2026-08-03 contradicted the "What's NOT provisioned" list: - Project roles DO exist on the live MartyrologyAPI project (owner-confirmed, corroborated by a token carrying admin). - priest@johnromanodorazio.com DOES hold the Zitadel admin role. - can_read_texts resolves true, so some OpenFGA grant path exists — the superuser entry claimed none did. The superuser entry is corrected to "needs re-checking" rather than "done": the run proves a grant path resolves, not which one. Asserting the mechanism without reading the store is what made the entry wrong in the first place. Adds a "Verified end to end" section recording what was established and the identity trap that produced a convincing false negative — a second account with an active SSO session yields no roles claim and redacted text, which is indistinguishable from the design failing unless you decode the id_token. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe handoff documents production verification for sibling-app tokens, role claims, access behavior, and licensing. It also records a verified project administrator, provisioned Zitadel roles, and an unresolved superuser or direct-admin grant path. ChangesProduction authentication verification
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
auth/handoffs/martyrology.md (1)
379-379: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSeparate Zitadel project roles from OpenFGA grants.
This line records a Zitadel project role and then refers to a later “grant” without naming the authorization system. The provisioning notes state that Zitadel roles are coarse population gates, while OpenFGA controls per-resource access. State explicitly that the verified
adminproject role does not prove an OpenFGAadmingrant.Proposed clarification
- `priest@johnromanodorazio.com` (`sub` `384646678734438403`) holds the Zitadel project role `admin`, verified in a live token — see "Verified end to end" below. The mechanism is unchanged and still applies to everyone else: + `priest@johnromanodorazio.com` (`sub` `384646678734438403`) holds the Zitadel project role `admin`, verified in a live token. This does not prove an OpenFGA `admin` grant on any governance body. The curator flow still applies to everyone else:🤖 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 `@auth/handoffs/martyrology.md` at line 379, Clarify the Martyrology entry around the verified Zitadel project role by explicitly distinguishing it from OpenFGA grants: state that the confirmed Zitadel `admin` role is only a coarse population gate and does not establish an OpenFGA `admin` grant. Keep the existing provisioning flow and references unchanged.
🤖 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 `@auth/handoffs/martyrology.md`:
- Around line 367-373: Rewrite the operator guidance in the “A trap worth
knowing about.” section to clearly distinguish the two token identities: compare
the access token’s sub value first, then decode the id_token to map that subject
to an email or account. Preserve the example subject and warning about
misleading role-free results.
- Around line 380-381: Update the “Superuser bootstrap — needs re-checking”
entry to reconcile the observed can_read_texts grant with the deployed
Martyrology model: verify whether production uses the pre-Task-6 model or the
Task-6 model, then document only the corresponding active grant path—an actual
reader/editor/admin tuple on the governing body for martyrologium_romanum_2004,
or the platform:martyrology superuser path. Remove unsupported alternative
candidates and preserve the instruction to inspect the live store.
---
Nitpick comments:
In `@auth/handoffs/martyrology.md`:
- Line 379: Clarify the Martyrology entry around the verified Zitadel project
role by explicitly distinguishing it from OpenFGA grants: state that the
confirmed Zitadel `admin` role is only a coarse population gate and does not
establish an OpenFGA `admin` grant. Keep the existing provisioning flow and
references unchanged.
🪄 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 Plus
Run ID: 6a630e6b-9f06-48e9-9563-389680059508
📒 Files selected for processing (1)
auth/handoffs/martyrology.md
All three findings verified against the model and the script before fixing. - Identity check is now two ordered steps. The access token does carry `sub`, so comparing it is the cheap first check; the id_token is only needed to map an unrecognised `sub` to a person. The previous text sent readers to the id_token immediately. - The superuser entry named "a direct admin tuple" as one candidate. Reading auth/models/Martyrology.json shows that was both over-broad and under-specified: can_read_texts resolves through governed_by to governance_body:reader, and reader = direct ∪ editor, editor = direct ∪ admin, admin = direct ∪ (on_platform → platform:superuser). So a direct reader OR editor OR admin tuple grants it, as does the platform path. Records that the platform path is impossible if production really runs the pre-Task-6 model — while noting that claim is itself untested, so the entry still says to read MARTYROLOGY_OPENFGA_MODEL_ID and the store rather than reason it out. - States explicitly that the verified Zitadel `admin` role is a coarse population gate and implies no OpenFGA admin grant. Two identically named things granted by separate operations is a trap worth closing in a document whose whole subject is authorization. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
A live authorization-code flow against production on 2026-08-03, run as part of verifying the OIDC login client from #19, contradicted three entries in
auth/handoffs/martyrology.md.Corrected
adminpriest@johnromanodorazio.comholdsadmincan_read_textsresolves true, so some grant path existsThe superuser entry is marked needs re-checking, not done. The run proves a grant path resolves; it does not show whether that is the
platform:martyrologysuperuser tuple or a directadmintuple on a body. Asserting a mechanism without reading the store is what made the entry wrong originally, so it now says to read the store first.The "Platform model not yet uploaded" entry is untouched — nothing in this run bears on it.
Added
A Verified end to end section recording what the flow established: that the API accepts a token minted by a sibling app in the same project, that the roles claim arrives in the introspection response without an
:audscope, and that licensing works in its allow direction.It also documents an identity trap that cost real time. An earlier attempt authenticated as
sub373244245643558915— a different account with an active SSO session in the same browser — and produced no roles claim and redacted text, which is indistinguishable from the design being broken. Zitadel puts noemailin the access token, so the identity is only visible in the id_token.🤖 Generated with Claude Code
Summary by CodeRabbit