AUTH-12A: register project mutation authorization contracts - #226
Conversation
📝 WalkthroughWalkthroughAUTH-12A adds 18 planned project-mutation authorization actions, typed resource contexts, scope derivation, stricter resource matching, migration ChangesProject mutation authorization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PreparedAuthorizationService
participant AuthorizationResourceContext
participant AuthorizationService
Client->>PreparedAuthorizationService: prepare planned project-mutation action
PreparedAuthorizationService->>AuthorizationResourceContext: validate typed resource context
AuthorizationResourceContext-->>PreparedAuthorizationService: validated system/project scope
PreparedAuthorizationService->>AuthorizationService: check action availability
AuthorizationService-->>Client: action_unavailable without handle or evidence
Possibly related PRs
Suggested reviewers: 🚥 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 review |
✅ Action performedReview finished.
|
![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 (5)
backend/alembic/versions/0041_project_mutation_action_evidence.py (1)
69-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the dependency on PostgreSQL's exact
pg_get_constraintdefrendering.
_pair_tokenreproduces the server's normalized form ((((action_id)::text = 'x'::text) AND ...)) verbatim, and_rewriterelies on it appearing exactly twice. This is fail-closed — a rendering change makes the marker count check raise rather than silently mangling the constraint — but the coupling is invisible to a future reader. A short comment on_pair_tokenstating that the string must matchpg_get_constraintdefoutput, and that the marker-count guards exist to detect drift, would save the next maintainer the reverse-engineering.🤖 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 `@backend/alembic/versions/0041_project_mutation_action_evidence.py` around lines 69 - 90, Document in the _pair_token function that its generated string must exactly match PostgreSQL’s pg_get_constraintdef rendering, and note that _rewrite’s marker-count checks detect formatting drift before modifying the constraint. Keep the existing fail-closed behavior and logic unchanged.backend/tests/test_authorization.py (1)
2292-2292: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePassing
Noneasselfcouples the test to the method's current implementation.
PreparedAuthorizationService._scope_from_resource(None, action_id, resource)only works because the method never readsself. If scope derivation later consults instance state (context, session), this fails with an opaqueAttributeErroronNonerather than a clear signal. A tiny module-level helper, or reusing the_PreparedTestSession-backed service already constructed elsewhere in this file, would be more durable.🤖 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 `@backend/tests/test_authorization.py` at line 2292, Update the test call to _scope_from_resource so it uses an actual PreparedAuthorizationService instance, preferably the existing _PreparedTestSession-backed service in the test setup, instead of passing None as self. Preserve the current action_id and resource inputs and assertions.backend/app/modules/authorization/runtime.py (2)
787-850: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winFreeze these catalogue mappings.
PROJECT_MUTATION_RESOURCE_BY_ACTIONand the three target-kind maps are exported and consumed inside the fail-closed matching path inkernel.py(_admin_resource_matches) andprepared.py(_scope_from_resource), yet they are plain mutable dicts.ACTION_BY_IDincatalogue.pyalready usesMappingProxyType; matching that here removes any chance of runtime mutation altering authorization decisions.🛡️ Proposed change
-PROJECT_MUTATION_RESOURCE_BY_ACTION = { +PROJECT_MUTATION_RESOURCE_BY_ACTION: Mapping[ActionId, type[BaseModel]] = MappingProxyType({ ActionId.PROJECT_CREATE: ProjectCreateResourceContext, @@ ActionId.PROJECT_GUIDE_ACTIVATE: ProjectGuideActivationResourceContext, -} +})(same treatment for the three
*_TARGET_KIND_BY_ACTIONmaps)🤖 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 `@backend/app/modules/authorization/runtime.py` around lines 787 - 850, Wrap PROJECT_MUTATION_RESOURCE_BY_ACTION and each of the three target-kind mappings—PROJECT_SUFFICIENCY_TARGET_KIND_BY_ACTION, PROJECT_GUIDE_TARGET_KIND_BY_ACTION, PROJECT_SUBMISSION_POLICY_TARGET_KIND_BY_ACTION, and PROJECT_POST_SUBMIT_POLICY_TARGET_KIND_BY_ACTION—in MappingProxyType, matching ACTION_BY_ID in catalogue.py. Add or reuse the required MappingProxyType import while preserving all existing entries and lookup behavior.
605-632: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared setup-service custody validation.
The three validators repeat the same custody sequence (custody presence ↔
execution_kind, expected step, generation, stale digest, lineage triple) with only the step name and digest source varying. A small shared helper keeps future custody rules from drifting between the three contexts.♻️ Sketch
def _require_setup_custody( custody: ProjectSetupServiceCustodyContext, *, label: str, expected_step: str, setup_generation: int, stale_output_digest: str | None, scope_project_id: UUID, guide_id: UUID, source_snapshot_id: UUID, ) -> None: if custody.expected_step != expected_step: raise ValueError(f"{label} setup-service step is inconsistent") if custody.setup_generation != setup_generation: raise ValueError(f"{label} setup generation is inconsistent") if custody.stale_output_digest != stale_output_digest: raise ValueError(f"{label} stale output is inconsistent") if ( custody.scope_project_id != scope_project_id or custody.guide_id != guide_id or custody.source_snapshot_id != source_snapshot_id ): raise ValueError(f"{label} setup lineage is inconsistent")Note the current error-message prefixes are asserted in
backend/tests/test_authorization.py(lines 2362-2393), so keep the same wording if you refactor.Also applies to: 661-686, 709-732
🤖 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 `@backend/app/modules/authorization/runtime.py` around lines 605 - 632, Extract the repeated setup-service custody field checks from require_sufficiency_identity and the corresponding validators near the other custody validation blocks into one shared helper, such as _require_setup_custody. Pass each context’s label, expected step, setup generation, digest source, and lineage identifiers; preserve the existing validation order and exact error-message prefixes asserted by the tests, while retaining each validator’s context-specific presence and execution-kind checks.backend/app/modules/authorization/kernel.py (1)
1078-1114: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winHoist the expected-resource mapping out of the call path.
This dict literal (now ~50 entries after merging
PROJECT_MUTATION_RESOURCE_BY_ACTION) is rebuilt on every_admin_resource_matchescall, which runs on each admin authorization evaluation and again during prepared-scope derivation. It is fully static; moving it to a module-level frozen mapping removes the per-call allocation.♻️ Proposed change
+_ADMIN_EXPECTED_RESOURCES = MappingProxyType({ + ActionId.AUTHORIZATION_PERMISSION_CATALOGUE_READ: PermissionCatalogueResourceContext, + ... + **PROJECT_MUTATION_RESOURCE_BY_ACTION, +}) + `@staticmethod` def _admin_resource_matches( action_id: ActionId, resource: AuthorizationResourceContext, ) -> bool: - expected = { - ... - **PROJECT_MUTATION_RESOURCE_BY_ACTION, - }.get(action_id) + expected = _ADMIN_EXPECTED_RESOURCES.get(action_id)🤖 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 `@backend/app/modules/authorization/kernel.py` around lines 1078 - 1114, Hoist the static expected-resource mapping currently built inside _admin_resource_matches to a module-level immutable mapping, including PROJECT_MUTATION_RESOURCE_BY_ACTION entries. Update _admin_resource_matches to reuse that mapping for action_id lookups while preserving the existing resource-context associations and default behavior.
🤖 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
@.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-12A-project-mutation-catalogue.md:
- Around line 79-100: Update the Ruff verification command in the “Verification
commands” section to include backend/tests/conftest.py alongside the existing
test files, matching the allowed-file list and ensuring every changed Python
file is checked.
- Around line 5-7: Update the allowed-files entry in the migration catalogue to
exactly use backend/alembic/versions/0041_project_mutation_evidence.py, matching
the frozen migration revision and filename stated above. Remove the placeholder
and project_mutation_action_evidence suffix.
In `@backend/tests/test_alembic.py`:
- Line 1985: Update the permission validation assertion in the relevant test to
compare definition.permission_id values against the set of permissions defined
before migration 0041, rather than set(PermissionId). Preserve the subset
assertion while using the pre-0041 permission collection so it verifies no new
permissions were introduced.
---
Nitpick comments:
In `@backend/alembic/versions/0041_project_mutation_action_evidence.py`:
- Around line 69-90: Document in the _pair_token function that its generated
string must exactly match PostgreSQL’s pg_get_constraintdef rendering, and note
that _rewrite’s marker-count checks detect formatting drift before modifying the
constraint. Keep the existing fail-closed behavior and logic unchanged.
In `@backend/app/modules/authorization/kernel.py`:
- Around line 1078-1114: Hoist the static expected-resource mapping currently
built inside _admin_resource_matches to a module-level immutable mapping,
including PROJECT_MUTATION_RESOURCE_BY_ACTION entries. Update
_admin_resource_matches to reuse that mapping for action_id lookups while
preserving the existing resource-context associations and default behavior.
In `@backend/app/modules/authorization/runtime.py`:
- Around line 787-850: Wrap PROJECT_MUTATION_RESOURCE_BY_ACTION and each of the
three target-kind mappings—PROJECT_SUFFICIENCY_TARGET_KIND_BY_ACTION,
PROJECT_GUIDE_TARGET_KIND_BY_ACTION,
PROJECT_SUBMISSION_POLICY_TARGET_KIND_BY_ACTION, and
PROJECT_POST_SUBMIT_POLICY_TARGET_KIND_BY_ACTION—in MappingProxyType, matching
ACTION_BY_ID in catalogue.py. Add or reuse the required MappingProxyType import
while preserving all existing entries and lookup behavior.
- Around line 605-632: Extract the repeated setup-service custody field checks
from require_sufficiency_identity and the corresponding validators near the
other custody validation blocks into one shared helper, such as
_require_setup_custody. Pass each context’s label, expected step, setup
generation, digest source, and lineage identifiers; preserve the existing
validation order and exact error-message prefixes asserted by the tests, while
retaining each validator’s context-specific presence and execution-kind checks.
In `@backend/tests/test_authorization.py`:
- Line 2292: Update the test call to _scope_from_resource so it uses an actual
PreparedAuthorizationService instance, preferably the existing
_PreparedTestSession-backed service in the test setup, instead of passing None
as self. Preserve the current action_id and resource inputs and assertions.
🪄 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: ffa26361-5153-487d-a514-8496d99674df
📒 Files selected for processing (13)
.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/CHUNK_MAP.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-12A-project-mutation-catalogue.mdbackend/alembic/versions/0041_project_mutation_action_evidence.pybackend/app/modules/authorization/catalogue.pybackend/app/modules/authorization/kernel.pybackend/app/modules/authorization/prepared.pybackend/app/modules/authorization/runtime.pybackend/tests/conftest.pybackend/tests/test_alembic.pybackend/tests/test_authorization.pydocs/operations_authorization_service.mddocs/spec_authorization_service.md
2 participants
Chunk
WS-AUTH-001-12A — Project Mutation Catalogue And PREP Foundation
Goal
Register the exact eighteen project-mutation actions, typed resource/PREP contracts, and PostgreSQL evidence vocabulary without activating product behavior.
Human-approved intent
The user explicitly started AUTH-12A after the reviewed AUTH-12 split.
What changed
Why it changed
Future mutation cutovers must reuse a closed authorization vocabulary and cannot invent generic policy contexts, parallel authorization paths, or underspecified service custody.
Design chosen
All actions remain planned. project.create alone derives system scope; the other 17 derive exact project scope. Shared families carry action-specific operation kinds. Setup-service derivations require typed custody and cannot be represented as human authority.
Alternatives rejected
Activation in the foundation chunk; generic project/policy contexts; conflating policy and setup generation; serialized/Celery PREP handles.
Scope control
No route, product service, Celery task, service provisioning, permission addition, CI configuration, dependency, or coverage threshold changed.
Product behavior
No product mutation is activated. Catalogue totals become 96 actions: 37 active and 59 planned.
Acceptance criteria proof
Exact 18-row parity; exact action/resource matching; planned denial before handle issuance; one Alembic head at 0041; all-18 migration pair parity plus representative direct and linked-idempotency downgrade custody.
Tests/checks run
Ruff and Python compilation pass. Alembic reports one head. Focused authorization proof passes 21 tests. Stale authorization docs, stale Workstream wording, Markdown links, and git diff integrity pass. The safe local DB runner rejected an unsafe admin database, so hosted Backend owns isolated PostgreSQL and full coverage proof.
Test delta
Added catalogue/resource/PREP and 0041 migration tests. No test was deleted, skipped, or weakened.
CI integrity
No CI or threshold change. GitHub Backend and Agent Gates pass on exact final head 76ef371; no CI or threshold change was made.
Reviewer results
Architecture, security, QA, product/ops, senior engineering, CI integrity, docs, reuse/dedup, and test-delta reviews pass after blocking setup-custody and operation-kind repairs.
External review
GitHub Backend and Agent Gates pass on exact final head 76ef371. CodeRabbit completed substantive review; all 3 actionable comments and 5 collapsed nitpicks were addressed, acknowledged, and resolved.
Remaining risks
Later 12E/12F/12G activation must bind execution_kind to the real actor/service matrix path and bind exact final resource facts through the canonical request digest.
Follow-up work
12B establishes the fixed setup-service identity and planned matrix after this chunk merges. No successor starts automatically.
Human review focus
Exact inventory, zero activation, service-custody closure, scope split, and migration custody.
Human merge ownership
Human approval is required. This agent will not merge the PR.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests