Zitadel role gate, platform superuser, and grant endpoint - #23
Merged
Conversation
Records the design for making the governance-body OpenFGA model operable: a coarse Zitadel role gate on curation writes, a platform superuser expressed as an OpenFGA type rather than a role bypass, and a grant endpoint scoped structurally to governance bodies. Also records a blocking defect found while designing: Authz.check() sends no Authorization header while the deployed OpenFGA runs with OPENFGA_AUTHN_METHOD=preshared, so every check 401s and returns False. Curation is denied for everyone and restricted texts are redacted for everyone, including licensed readers. Verified against production. Departs from LiturgicalCalendarAPI in two places, both deliberate and argued in the document: the admin role is not an OpenFGA bypass, and the grant endpoint is gated on body admin alone rather than on a platform-wide role. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six tasks, 45 steps, TDD throughout. Tasks 1-5 are martyrology-api: authenticate to OpenFGA and generalise the check to any object type; read project-scoped Zitadel roles into Identity; gate curation writes on those roles; give Authz write/delete/read; add the /api/v1/admin/permissions router. Task 6 is cdcf-infra: the platform type, the on_platform tuples, the three project roles, and the handoff doc. Task 1 stands alone as the repair for the dead authorization check, and is worth landing regardless of the rest. Task 6 is grounded in the actual scripts rather than described: create_roles at setup-zitadel.sh:366 is the existing helper LitCal calls at :708, and --create-martyrology-store is the correct action because do_create_store uploads a changed model before seeding, where --seed-tuples would write the new on_platform tuples against a model that lacks the relation. It also lists the three comment blocks in setup-zitadel.sh that currently assert the opposite of what will be true. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Address review findings on Task 4: guard page.get(...) calls with an isinstance(page, dict) check so a malformed 200 body (e.g. a JSON array) can no longer raise AttributeError out of read_tuples, which must fail closed and never raise. Also log a warning when the loop exhausts MAX_READ_PAGES while a continuation_token is still outstanding, so silent truncation is at least observable.
…nd full route coverage - CacheHeadersMiddleware defaults every 200 GET under /api/v1 to a public, 24h cache unless the handler opts out; list_permissions and check_permission never did, so a body's admin roster and live allowed/denied checks were being served from shared caches. Both now set request.state.cache_private. - The idempotency mapping for write_failed_due_to_invalid_input assumed the caller's desired end state held instead of confirming it. _mutate now re-checks the tuple via check_object after catching that code and only reports success when the postcondition actually matches (grant -> True, revoke -> False); otherwise it raises the same 502. - Authz.read_tuples silently returned a partial/empty list on transport errors, non-200 responses, and non-dict JSON bodies. It now raises AuthzError like write/delete do (the unconfigured case still returns []), and list_permissions maps that error to a 502. - Added 401/403 coverage for the DELETE and /check routes (the DELETE admin gate was previously untested) and a test pinning a double-prefixed user id to 422.
…check_object check_object evaluates the computed relation, but write/delete manipulate direct tuples and the model unions them (editor: [user] or admin), so an admin's computed editor check is always True even absent a direct editor tuple. Confirming a direct-tuple mutation with a computed-relation query was wrong in both directions: a genuinely successful revoke of editor from an admin-holder could be reported 502, and check_object's fail-closed False on infrastructure failure meant a failed revoke confirmation could misreport success. _mutate now confirms via read_tuples(obj, rel) and checks for the exact user, treating a raised AuthzError from the confirming read itself as unconfirmed (502), never as success.
…sed authz Three Important findings from the final whole-branch review of authz-roles-and-grants: - .env.example and setup-vps-deploy-user.sh's $SECRET_ENV skeleton never learned MARTYROLOGY_OPENFGA_API_TOKEN or MARTYROLOGY_ZITADEL_PROJECT_ID, so a freshly provisioned host reproduced the silent lockout this branch exists to end, under a new name. Added both keys, grouped with their siblings, with comments on the failure mode when empty; added a companion note to the NEXT STEPS message for the partial-configuration case. - create_app warned on OpenFGA token misconfiguration but not on the equivalent Zitadel project-id gap, which permanently 403s every curation write. Added the mirrored warning and tests asserting each fires only in its own partial-configuration state. - Authz.check_object raised AttributeError (surfaced as a 500) on a non-dict JSON body instead of returning its documented fail-closed False, unlike read_tuples which already guards for this. Authz.read_tuples raised a bare TypeError instead of AuthzError when "tuples" was a non-iterable scalar, escaping the AuthzError handling in admin.py. Fixed both, with regression tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe API now supports bearer-authenticated OpenFGA checks and tuple operations, project-scoped Zitadel roles, governance-body permission management, curation role gates, incomplete-configuration warnings, and opt-in public caching. ChangesAuthorization roles and grants
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AdminRouter
participant Authenticator
participant Authz
participant OpenFGA
Client->>AdminRouter: Submit governance-body permission grant
AdminRouter->>Authenticator: Validate bearer token
Authenticator-->>AdminRouter: Return authenticated Identity
AdminRouter->>Authz: Check governance-body admin relation
Authz->>OpenFGA: Send authenticated check
OpenFGA-->>Authz: Return authorization result
AdminRouter->>Authz: Write permission tuple
Authz->>OpenFGA: Send authenticated tuple mutation
OpenFGA-->>Authz: Return mutation result
AdminRouter-->>Client: Return PermissionOut
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 3
🧹 Nitpick comments (4)
docs/superpowers/plans/2026-08-02-authz-roles-and-grants.md (2)
827-861: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the
read_tuplespseudocode to match the shipped fail-open-then-raise behavior.This pseudocode returns
outsilently on transport errors, non-200 responses, and malformed JSON. The shippedread_tuplesinsrc/martyrology_api/authz.pyraisesAuthzErrorin each of those cases instead, and this behavior is covered bytest_read_tuples_raises_on_transport_error,test_read_tuples_raises_on_non_200_response,test_read_tuples_non_dict_json_body_raises, andtest_read_tuples_non_iterable_tuples_field_raises_authz_error. A reader implementing directly from this plan would build a silently-degrading reader instead of the fail-loud one that shipped.Update this code block to match the final implementation, or add a note that it was superseded during implementation.
🤖 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 `@docs/superpowers/plans/2026-08-02-authz-roles-and-grants.md` around lines 827 - 861, Update the read_tuples pseudocode to reflect the shipped fail-open-then-raise behavior: transport errors, non-200 responses, malformed or non-dict JSON, and invalid tuples fields must raise AuthzError rather than return the partial out result. Alternatively, clearly mark this pseudocode as superseded during implementation.
1247-1269: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the
_mutatepseudocode to reflect the postcondition-confirmation logic that shipped.This pseudocode treats any
IDEMPOTENT_CODEfailure as an unconditional "noop". The shipped_mutateinsrc/martyrology_api/routers/admin.pyinstead confirms the postcondition with a follow-upread_tuplescall before deciding "noop" vs "error:unconfirmed", a materially different (and safer) behavior exercised bytest_duplicate_grant_with_unconfirmed_postcondition_is_502andtest_revoke_with_unconfirmed_postcondition_is_502.Update this code block to match the final implementation, or add a note that it was superseded during implementation.
🤖 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 `@docs/superpowers/plans/2026-08-02-authz-roles-and-grants.md` around lines 1247 - 1269, The `_mutate` pseudocode must reflect postcondition confirmation for `IDEMPOTENT_CODE` failures instead of unconditionally returning “noop”. Update the `AuthzError` handling to call `read_tuples` and return “noop” only when the requested state is confirmed; otherwise set `outcome` to `error:unconfirmed` and raise the same authorization-store `ApiProblem`. Alternatively, explicitly note that this pseudocode was superseded by the shipped implementation.src/martyrology_api/authz.py (2)
68-104: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftAdopt OpenFGA's native
on_duplicate/on_missingidempotency instead of a manual read-confirm round trip.Authz.write/Authz.deletedon't request server-side idempotent writes, soadmin.py's_mutatehas to detect anIDEMPOTENT_CODEfailure and issue a secondread_tuplescall to confirm the postcondition before reporting success. OpenFGA (v1.10.0+) supportson_duplicate: "ignore"in thewritesblock andon_missing: "ignore"in thedeletesblock, which makes both cases a server-side no-op with no client-side confirmation needed.
src/martyrology_api/authz.py#L68-L104: addon_duplicate: "ignore"to the writes payload andon_missing: "ignore"to the deletes payload in_mutate(server version permitting), so a duplicate write or missing delete no longer raiseswrite_failed_due_to_invalid_input.src/martyrology_api/routers/admin.py#L67-L119: once the client-side change lands, remove theIDEMPOTENT_CODE/read_tuples-confirmation branch from_mutate, since OpenFGA itself now reports success for both cases.Confirm the deployed OpenFGA server version supports this (requires v1.10.0+) before adopting.
🤖 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 `@src/martyrology_api/authz.py` around lines 68 - 104, Confirm the deployed OpenFGA version is at least v1.10.0, then update Authz._mutate in src/martyrology_api/authz.py (lines 68-104) to request on_duplicate: "ignore" for writes and on_missing: "ignore" for deletes, preserving the existing payload structure. In src/martyrology_api/routers/admin.py (lines 67-119), remove the IDEMPOTENT_CODE/read_tuples confirmation branch because the server now handles these cases idempotently.
68-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated HTTP-call boilerplate.
check_object,_mutate, andread_tupleseach open anhttpx.AsyncClient(transport=self._transport), POST a JSON body, catchhttpx.HTTPError, and parse the JSON response with near-identical code. Extract a small internal helper (for exampleasync def _post(self, path, payload) -> tuple[int, object | None]) that performs the request and returns the status code and parsed body (orNoneon a parse failure), and let each public method apply its own policy (check_objectfails closed toFalse;_mutate/read_tuplesraiseAuthzError) on top of that shared result.This reduces duplicated logic in a security-critical client and makes future changes (for example, adding a timeout or a retry policy) a one-place edit instead of three.
🤖 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 `@src/martyrology_api/authz.py` around lines 68 - 171, Extract the duplicated AsyncClient POST, HTTPError handling, and JSON parsing from check_object, _mutate, and read_tuples into a private helper such as _post(path, payload) returning the status code and parsed body, or None when parsing fails. Update each caller to use this helper while preserving its existing policy: check_object fails closed to False, whereas _mutate and read_tuples raise AuthzError for transport, status, or invalid-response errors.
🤖 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 `@docs/superpowers/specs/2026-08-02-authz-roles-and-grants-design.md`:
- Around line 206-213: Update the D5 Authz API documentation to use the
implemented read_tuples(obj, relation="") method instead of read(object,
relation=None), and align related examples or tests so callers target
read_tuples consistently.
- Around line 151-155: The admin permission endpoints in the design are missing
the mounted /api/v1 prefix. Update the endpoint examples around the permissions
API and rollout step 6 to use /api/v1/admin/permissions... consistently, or
explicitly label the existing paths as router-relative; ensure all
operator-facing URLs match create_app()’s admin.router mount.
In `@src/martyrology_api/app.py`:
- Around line 43-48: Update the OpenFGA startup warning around
Settings.authz_enabled to detect any partial configuration across
openfga_api_url, openfga_store_id, and openfga_api_token, not just a missing
token. Report every unset variable in the warning while preserving the
no-warning behavior for fully configured or entirely absent settings, and add a
regression test covering URL and token set with an empty store ID.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-08-02-authz-roles-and-grants.md`:
- Around line 827-861: Update the read_tuples pseudocode to reflect the shipped
fail-open-then-raise behavior: transport errors, non-200 responses, malformed or
non-dict JSON, and invalid tuples fields must raise AuthzError rather than
return the partial out result. Alternatively, clearly mark this pseudocode as
superseded during implementation.
- Around line 1247-1269: The `_mutate` pseudocode must reflect postcondition
confirmation for `IDEMPOTENT_CODE` failures instead of unconditionally returning
“noop”. Update the `AuthzError` handling to call `read_tuples` and return “noop”
only when the requested state is confirmed; otherwise set `outcome` to
`error:unconfirmed` and raise the same authorization-store `ApiProblem`.
Alternatively, explicitly note that this pseudocode was superseded by the
shipped implementation.
In `@src/martyrology_api/authz.py`:
- Around line 68-104: Confirm the deployed OpenFGA version is at least v1.10.0,
then update Authz._mutate in src/martyrology_api/authz.py (lines 68-104) to
request on_duplicate: "ignore" for writes and on_missing: "ignore" for deletes,
preserving the existing payload structure. In
src/martyrology_api/routers/admin.py (lines 67-119), remove the
IDEMPOTENT_CODE/read_tuples confirmation branch because the server now handles
these cases idempotently.
- Around line 68-171: Extract the duplicated AsyncClient POST, HTTPError
handling, and JSON parsing from check_object, _mutate, and read_tuples into a
private helper such as _post(path, payload) returning the status code and parsed
body, or None when parsing fails. Update each caller to use this helper while
preserving its existing policy: check_object fails closed to False, whereas
_mutate and read_tuples raise AuthzError for transport, status, or
invalid-response errors.
🪄 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: a48a7175-b815-4cf2-9792-404bdc737b16
📒 Files selected for processing (21)
.env.exampledocs/superpowers/plans/2026-08-02-authz-roles-and-grants.mddocs/superpowers/specs/2026-08-02-authz-roles-and-grants-design.mdscripts/deploy/setup-vps-deploy-user.shsrc/martyrology_api/app.pysrc/martyrology_api/auth.pysrc/martyrology_api/authz.pysrc/martyrology_api/caching.pysrc/martyrology_api/config.pysrc/martyrology_api/models.pysrc/martyrology_api/routers/admin.pysrc/martyrology_api/routers/curation.pysrc/martyrology_api/routers/discovery.pysrc/martyrology_api/routers/read.pytests/test_admin_api.pytests/test_app.pytests/test_auth.pytests/test_authz.pytests/test_caching.pytests/test_config.pytests/test_curation_api.py
CI was failing on pyright, not on tests: tests/test_config.py called Settings(_env_file=None, ...), which pydantic-settings accepts at runtime but which is absent from the generated __init__ signature. conftest.py already carries the `pyright: ignore[reportCallIssue]` for the identical call; the plan's test simply omitted it. The startup warning covered only openfga_api_url set with an empty token, but an empty store_id short-circuits check_object to False just as silently. It now reports every unset variable across url/store/token whenever the configuration is partial, and stays quiet when all three are set or all three are empty. Two existing tests changed with it: "URL unset" is no longer the boundary, so they now pin the entirely-unconfigured case and the token-only case explicitly. Doc corrections: the spec described `read(object, relation=None)`, which shipped as `read_tuples(obj, relation="")`, and wrote the grant endpoint's paths router-relative where an operator needs the mounted /api/v1 form. The spec now also records why check_object and read_tuples have opposite error contracts, since callers depend on the difference. The plan gains a short "superseded during implementation" section naming the three places review findings changed its pseudocode, rather than back-dating the plan to match. Skipped, with reasons: - OpenFGA on_duplicate/on_missing "ignore" (v1.10+, and production runs v1.15.1) would let the server handle idempotency and remove the postcondition-confirmation branch entirely. It is a real improvement, but a semantic change to the highest-risk path after it has been reviewed twice; worth its own change, not this one. - Extracting a shared _post helper from check_object/_mutate/read_tuples is a pure refactor of working code, and the three deliberately differ in error policy, which is most of what the helper would have to parameterise. - codecov's "no JUnit XML found" is CI reporting configuration, unrelated to this branch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes the governance-body OpenFGA model operable, and repairs a defect that had
silently disabled every authorization decision in production.
The defect
Authz.check()sent noAuthorizationheader while the deployed OpenFGA runsOPENFGA_AUTHN_METHOD=preshared. Verified against production:Every check took the
status_code != 200path and returnedFalse, so:after grants existed;
including correctly licensed ones —
licensing.pyruns the same dead check.What this adds
check_objectfor arbitraryobject refs (
MARTYROLOGY_OPENFGA_API_TOKEN).Identity, read fromurn:zitadel:iam:org:project:<PROJECT_ID>:roles. The generic claim isdeliberately ignored — it can carry roles held in other projects of the same
umbrella instance (
MARTYROLOGY_ZITADEL_PROJECT_ID).martyrology_editororadmin,checked before OpenFGA is consulted.
admingrants no bypass — unlikeLiturgicalCalendarAPI, platform authority is a tuple you can list and revoke,
not a role that silently defeats the governance model.
write/delete/read_tupleson the OpenFGA client./api/v1/admin/permissions— list, grant, revoke, check. The object typeis never a request parameter; it is always
governance_body, fixed by theroute, which makes
edition:andplatform:tuples structurally unreachable.Authorization is the OpenFGA
adminrelation on the target body alone, so abody admin who is not platform staff can still delegate within their body.
under
/api/v1topublic, max-age=86400unless a handler opted out; the newadmin routes inherited that and published the permission roster to shared
caches for 24h. A router that declares nothing now gets
private, max-age=0.Review history
Every task passed a scoped review; two needed fix rounds. The reviews caught,
among others:
read_tuplesraising on a non-dict JSON body despite anever-raises contract; the cache leak above; and a postcondition check that used
the computed relation to confirm a direct-tuple mutation, which both failed
open on a revoke whose confirmation had itself failed and reported 502 for a
successful revoke of
editorfrom anadminholder.The whole-branch review additionally found that
setup-vps-deploy-user.sh—which authors
/etc/martyrology/api.env— had never learned the two newsettings, so provisioning a fresh host would have reproduced the same silent
lockout under a new name. Fixed in
dd4750a.307 tests pass, up from 249.
Not live on merge
Depends on CatholicOS/cdcf-infra#18 (the
platformtype andon_platformtuples), the out-of-band bootstrap superuser tuple, theMARTYROLOGY_OPENFGA_MODEL_IDre-pin, and the two new env vars. Until the modelis uploaded, no principal holds a direct
governance_body#admintuple, soPOST /admin/permissionsreturns 403 for everyone.Design:
docs/superpowers/specs/2026-08-02-authz-roles-and-grants-design.md🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Configuration
Documentation