Context
The grant endpoint added in #23 has to treat two OpenFGA outcomes as success:
granting a permission that already exists, and revoking one that is already
absent. Both are the caller's desired end state.
OpenFGA reports both as 400 write_failed_due_to_invalid_input. The endpoint
therefore catches that code and, rather than trusting it, confirms the
postcondition before returning 200 — src/martyrology_api/routers/admin.py:67-105.
That confirmation exists because the same error code has a third meaning: if the
deployed authorization model lacks a relation, or its type restriction disallows
user: on it, OpenFGA rejects the write with that code too. Mapping it straight
to 200 would have reported a failed revoke as revoked — an admin believing
access was removed when the tuple survived.
Getting the confirmation right took two review rounds:
- confirming with
check_object failed open on revoke, because check_object
fails closed to False and a revoke's desired state is False, making a
failed confirmation indistinguishable from a confirmed one;
check_object also evaluates the computed relation while write/delete
manipulate direct tuples, and the model unions editor ← admin and
reader ← editor. Revoking editor from a principal who also holds admin
left the computed check answering true, reporting 502 for a revoke that had
in fact succeeded.
It now confirms against direct tuples via read_tuples. That is correct, and
tested, but it is roughly 40 lines of branch plus an extra round trip on the
error path, guarding a distinction the server may be able to make itself.
Proposal
OpenFGA's Write API grew per-request idempotency flags (on_duplicate: "ignore"
for writes, on_missing: "ignore" for deletes) in v1.10.0. Production runs
openfga/openfga:v1.15.1, verified on the host, so the feature is available.
With those flags set, a duplicate write and a revoke of an absent tuple both
return 200 from the server. A 400 then unambiguously means what the
confirmation branch exists to detect — a genuinely invalid write — and maps
cleanly to 502.
The change:
Authz._mutate (src/martyrology_api/authz.py:76) sets on_duplicate /
on_missing on the appropriate half of the payload;
_mutate in routers/admin.py drops IDEMPOTENT_CODE and the whole
confirmation branch, keeping the audit logging;
- the idempotency tests move from "confirm the postcondition" to "the server
returned 200", and the direct-vs-computed regression test becomes moot.
Net effect: less code on the highest-risk path in the API, one fewer round trip,
and the subtlety that needed two review rounds stops being ours to get right.
Before doing this
- Confirm the exact flag names and semantics against the v1.15.1 API
reference. They are reported here second-hand from a review comment and
have not been exercised against the live store.
- Decide how to handle a version floor. If the flags are silently ignored by an
older server rather than rejected, removing the confirmation branch would
reintroduce the failed-revoke-reported-as-success bug against that server.
Either pin a minimum version somewhere visible, or verify the server rejects
unknown request fields.
- Keep the audit log lines; they are the record of who granted what.
Why it was not done in #23
It is a semantic change to the code path that most needed care, proposed after
that path had already been through two review rounds. Worth its own change with
its own review, not folded into a review-response commit.
Deferred from CodeRabbit review on #23.
Context
The grant endpoint added in #23 has to treat two OpenFGA outcomes as success:
granting a permission that already exists, and revoking one that is already
absent. Both are the caller's desired end state.
OpenFGA reports both as
400 write_failed_due_to_invalid_input. The endpointtherefore catches that code and, rather than trusting it, confirms the
postcondition before returning 200 —
src/martyrology_api/routers/admin.py:67-105.That confirmation exists because the same error code has a third meaning: if the
deployed authorization model lacks a relation, or its type restriction disallows
user:on it, OpenFGA rejects the write with that code too. Mapping it straightto 200 would have reported a failed revoke as revoked — an admin believing
access was removed when the tuple survived.
Getting the confirmation right took two review rounds:
check_objectfailed open on revoke, becausecheck_objectfails closed to
Falseand a revoke's desired state isFalse, making afailed confirmation indistinguishable from a confirmed one;
check_objectalso evaluates the computed relation whilewrite/deletemanipulate direct tuples, and the model unions
editor ← adminandreader ← editor. Revokingeditorfrom a principal who also holdsadminleft the computed check answering
true, reporting 502 for a revoke that hadin fact succeeded.
It now confirms against direct tuples via
read_tuples. That is correct, andtested, but it is roughly 40 lines of branch plus an extra round trip on the
error path, guarding a distinction the server may be able to make itself.
Proposal
OpenFGA's Write API grew per-request idempotency flags (
on_duplicate: "ignore"for writes,
on_missing: "ignore"for deletes) in v1.10.0. Production runsopenfga/openfga:v1.15.1, verified on the host, so the feature is available.With those flags set, a duplicate write and a revoke of an absent tuple both
return 200 from the server. A
400then unambiguously means what theconfirmation branch exists to detect — a genuinely invalid write — and maps
cleanly to 502.
The change:
Authz._mutate(src/martyrology_api/authz.py:76) setson_duplicate/on_missingon the appropriate half of the payload;_mutateinrouters/admin.pydropsIDEMPOTENT_CODEand the wholeconfirmation branch, keeping the audit logging;
returned 200", and the direct-vs-computed regression test becomes moot.
Net effect: less code on the highest-risk path in the API, one fewer round trip,
and the subtlety that needed two review rounds stops being ours to get right.
Before doing this
reference. They are reported here second-hand from a review comment and
have not been exercised against the live store.
older server rather than rejected, removing the confirmation branch would
reintroduce the failed-revoke-reported-as-success bug against that server.
Either pin a minimum version somewhere visible, or verify the server rejects
unknown request fields.
Why it was not done in #23
It is a semantic change to the code path that most needed care, proposed after
that path had already been through two review rounds. Worth its own change with
its own review, not folded into a review-response commit.
Deferred from CodeRabbit review on #23.