Skip to content

fix(authz): complete the #955 prune, dropping general_roman_calendar (#539) - #540

Merged
JohnRDOrazio merged 2 commits into
developmentfrom
fix/539-prune-legacy-grc
Sep 3, 2026
Merged

fix(authz): complete the #955 prune, dropping general_roman_calendar (#539)#540
JohnRDOrazio merged 2 commits into
developmentfrom
fix/539-prune-legacy-grc

Conversation

@JohnRDOrazio

Copy link
Copy Markdown
Member

Closes #539. Mirrors LiturgicalCalendarAPI#970
and CatholicOS/cdcf-infra#44.

This unblocks development and every open PR. The rbac e2e seed has been
failing since 2026-09-02 07:37:

FGA write 400: Invalid tuple
'general_roman_calendar:temporale#admin@user:389059157879160837'.
Reason: type 'general_roman_calendar' not found

#530 is not at fault

It was written to issue #527's explicit instruction — "the API stays additive for
a whole migration window"
— and deliberately kept the e2e seeds on the legacy
type, because the API's fallback made them resolve. The window closed 18 hours
after it merged:

when (UTC) what
2026-09-01 22:32 cdcf-infra#41 — adds rite_calendar. Additive.
2026-09-01 23:09 #530 merged — mirrors it, keeps legacy working
2026-09-02 17:11 API#970 — drops every legacy code path
2026-09-02 17:22 cdcf-infra#44 — drops the types from the model

authz-seed clones cdcf-infra at main at run time, so CI picked it up
immediately. A stack seeded before the prune keeps passing, which is why this can
look machine-specific — it is not.

Why no fallback could have rescued it. The API's widening applied to checks.
This is a write, and OpenFGA validates a tuple's type against the model before
any API code runs.

Three groups, treated differently

1. Seeds and specs. users.ts seeds grc-admin/grc-editor on
rite_calendar:roman/temporale; spec 14's decrees object becomes
rite_calendar:roman/decrees; requestAccess.ts's union follows
AccessRequestRepository::VALID_OBJECT_TYPES.

2. The authorization fallback — now dead weight. legacyRiteCalendarObject()
and LEGACY_RITE_CALENDAR_TYPE are gone, as is the second ask in
capabilities.js, admin-decrees.js's legacy DECREES_OBJECTS entry, and
AuthHelper::canViewRiteCalendarResource()'s legacy branch. Nothing resolves
against the type, so each was a round-trip to a guaranteed negative. PHPStan then
caught RITE_CALENDAR_FIXED_SUBRESOURCES left unused by that removal — worth
noting as evidence the analysis gate earns its place.

3. Grant dropdowns — actively harmful. The grant modal and
permission-requests.js's roleObjectTypes still offered the retired types;
selecting one handed the user the same FGA error the seed hit. Both now offer
rite_calendar and rite_calendar_test.

That closes a gap #530 left open on purpose: test_editor was still on
general_roman_calendar_test and, once that type went, had no rite-level option
at all
. It has one now — and rite_calendar_test ids are the bare rite
(roman, ambrosian), because there the id is the rite, unlike the
rite-qualified data types.

The filter select keeps both legacy options. Filtering finds, it does not
create, and stored access requests still carry the old names; an admin who cannot
filter for them cannot find them.

Deliberately kept

The objectTypeNames display maps and includes/change-request-i18n.php.
audit_log rows are never rewritten, so historical records keep the old names
permanently — a map without them renders those rows as a raw type id. #530's
regression test, "STILL labels a legacy general_roman_calendar row after
#955"
, stays green.

admin-tests.php's general_roman_calendar is untouched: it is a UI-local
#testScopeType token that deriveScope() turns into rite_calendar_test:<rite>.

The tests assert the absence

The four cases that pinned the fallback are rewritten, not deleted. A fallback
creeping back would restore an authorization path the model no longer has, and
would show controls whose writes the API refuses.

Verification — against the real model, not just statically

The whole point is a model change, so I reproduced it rather than reasoning about
it. Re-seeded the local OpenFGA store from cdcf-infra main (confirmed
general_roman_calendar gone, rite_calendar present), repinned
OPENFGA_MODEL_ID, recreated the API, then:

yarn playwright test --project=rbac
34 passed (2.0m)

including 07 grc-admin — card visibility and the whole 14 admin-decrees capability matrix — precisely the specs that were previously green because of
the fallback this removes.

A second commit fixes one straggler the first missed: admin-dashboard.php's
test_editor gate still asked about general_roman_calendar_test. It reads
like a display list but is an authorization ask. Re-verified both halves of the
Tests-card matrix afterwards (9 passed).

Also green: yarn test:unit (518), yarn lint, yarn typecheck,
composer parallel-lint, composer lint, composer analyse, composer test
(52 tests, 162 assertions).

🤖 Generated with Claude Code

JohnRDOrazio and others added 2 commits September 3, 2026 02:26
…539)

Closes #539. Mirrors LiturgicalCalendarAPI#970 and CatholicOS/cdcf-infra#44.

The rbac e2e seed has been failing on `development` and on every open PR since
2026-09-02 07:37:

    FGA write 400: Invalid tuple
    'general_roman_calendar:temporale#admin@user:389059157879160837'.
    Reason: type 'general_roman_calendar' not found

#530 is not at fault. It was written to issue #527's instruction — "the API stays
additive for a whole migration window" — and deliberately kept the e2e seeds on
the legacy type, because the API's fallback made them resolve. That window closed
18 hours after #530 merged: API#970 dropped every legacy code path at 17:11, and
cdcf-infra#44 removed both retired types from the model at 17:22. `authz-seed`
clones cdcf-infra at `main` AT RUN TIME, so CI picked it up immediately.

The failure is a WRITE, which no fallback could ever have rescued: the API's
widening applied to checks, while OpenFGA validates a tuple's type against the
model before any API code runs.

Three groups, treated differently.

**Seeds and specs.** `users.ts` seeds grc-admin/grc-editor on
`rite_calendar:roman/temporale`; spec 14's decrees object becomes
`rite_calendar:roman/decrees`; `requestAccess.ts`'s union follows
`AccessRequestRepository::VALID_OBJECT_TYPES`.

**The authorization fallback, now dead weight.** `legacyRiteCalendarObject()` and
`LEGACY_RITE_CALENDAR_TYPE` are gone, as is the second ask in `capabilities.js`,
`admin-decrees.js`'s legacy `DECREES_OBJECTS` entry, and
`AuthHelper::canViewRiteCalendarResource()`'s legacy branch. Nothing resolves
against the type, so each was a round-trip to a guaranteed negative. PHPStan then
caught `RITE_CALENDAR_FIXED_SUBRESOURCES` left unused by that branch's removal.

**Grant dropdowns, which were actively harmful.** `admin-permissions.php`'s grant
modal and `permission-requests.js`'s `roleObjectTypes` still OFFERED the retired
types — selecting one handed the user the same FGA error the seed just hit. Both
now offer `rite_calendar` and `rite_calendar_test`. That closes a gap #530 left
open on purpose: `test_editor` was still on `general_roman_calendar_test` and had
no rite-level option at all, because no id branch existed for its successor. It
has one now, and `rite_calendar_test` ids are the BARE rite (`roman`,
`ambrosian`) — its id IS the rite — not rite-qualified like the data types.

The FILTER select keeps both legacy options. Filtering finds, it does not create,
and stored access requests still carry the old names; an admin who cannot filter
for them cannot find them.

**Deliberately kept.** The `objectTypeNames` display maps in
`admin-permissions.js` and `permission-requests.js`, and
`includes/change-request-i18n.php`. `audit_log` rows are never rewritten, so
historical records keep the old names permanently; a map without them renders
those rows as a raw type id. The #530 regression test — "STILL labels a legacy
general_roman_calendar row after #955" — stays green. `admin-tests.php`'s
`general_roman_calendar` is untouched: it is a UI-local `#testScopeType` token
that `deriveScope()` turns into `rite_calendar_test:<rite>`.

The four tests that pinned the fallback now assert its ABSENCE rather than being
deleted. A fallback creeping back would restore an authorization path the model
no longer has, and would show controls whose writes the API refuses.

Verified against the real post-prune model, not just statically: re-seeded the
local OpenFGA store from cdcf-infra `main` (`general_roman_calendar` gone,
`rite_calendar` present), repinned OPENFGA_MODEL_ID, recreated the API, and ran

    yarn playwright test --project=rbac
    34 passed (2.0m)

including `07 grc-admin — card visibility` and the whole `14 admin-decrees
capability matrix` — the specs that were previously green only because of the
fallback this removes.

Also: yarn test:unit (518), yarn lint, yarn typecheck, composer parallel-lint,
composer lint, composer analyse, composer test (52 tests, 162 assertions).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011r2AEViGig1EQXoP6ZNuUs
…test type

The Administration section's test_editor gate asked
`canViewAnyResourceOfType(..., 'general_roman_calendar_test')` alongside the three
current types. That name was removed from the FGA model at the #955 prune
milestone (CatholicOS/cdcf-infra#44), so `/auth/dashboard-scopes` cannot report a
scope under it — the extra argument could only ever match nothing.

Missed in the previous commit because it reads as a display list rather than an
authorization ask; it is the latter.

Verified the gate still behaves, rather than assuming a removal is inert:

    yarn playwright test --project=rbac \
      e2e/rbac/15-dashboard-tests-card-matrix.spec.ts \
      e2e/rbac/07-dashboard-card-scoping.spec.ts
    9 passed (30.3s)

covering both halves of the Tests-card matrix — a test_editor WITH a scope still
sees the card, one WITHOUT still does not.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011r2AEViGig1EQXoP6ZNuUs
@coderabbitai

coderabbitai Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

Next included review available in 20 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: a92cf7ee-df12-4e9c-b9e6-763807214d21

📥 Commits

Reviewing files that changed from the base of the PR and between 31b8374 and 16319e6.

📒 Files selected for processing (14)
  • admin-dashboard.php
  • admin-permissions.php
  • assets/js/__tests__/capabilities.test.js
  • assets/js/__tests__/riteScopedObjectId.test.js
  • assets/js/admin-decrees.js
  • assets/js/admin-permissions.js
  • assets/js/capabilities.js
  • assets/js/permission-requests.js
  • assets/js/riteScopedObjectId.js
  • e2e/rbac/14-admin-decrees-capability-matrix.spec.ts
  • e2e/rbac/support/requestAccess.ts
  • e2e/rbac/support/users.ts
  • src/AuthHelper.php
  • tests/AuthHelperDashboardScopesTest.php

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@JohnRDOrazio
JohnRDOrazio merged commit edcb412 into development Sep 3, 2026
12 checks passed
@JohnRDOrazio
JohnRDOrazio deleted the fix/539-prune-legacy-grc branch September 3, 2026 00:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Complete the #955 prune milestone: drop general_roman_calendar, which no longer exists in the FGA model

1 participant