feat(auth): cut over current guide policy reads - #221
Conversation
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (13)
📝 WalkthroughWalkthroughAUTH-11C2 activates project policy and active-guide read actions, adds strict authorization contexts and locked policy-chain reads, routes endpoints through authorization-driven composition, tightens response schemas, and updates tests, E2E checks, workflow coverage, documentation, and status tracking. ChangesProject policy and active-guide authorization
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ProjectsRouter
participant ProjectAuthorizationReads
participant ProjectRepository
participant AuthorizationKernel
Client->>ProjectsRouter: GET active-guide or policy endpoint
ProjectsRouter->>ProjectAuthorizationReads: authorize requested project read
ProjectAuthorizationReads->>ProjectRepository: lock project, guide, and policy-chain rows
ProjectAuthorizationReads->>AuthorizationKernel: require strict resource context
AuthorizationKernel-->>ProjectsRouter: allow or concealed denial
ProjectsRouter-->>Client: validated response or 404
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
…2-effective-policy-active-guide-reads
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
backend/app/modules/projects/authorization_reads.py (1)
107-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the policy-chain consistency predicate.
These row-vs-row equality checks largely repeat what
authorize_project_active_guide_read(Lines 297-347) andProjectService.validate_activation_readyalready assert. A shared helper (e.g.effective_chain_is_consistent(guide, snapshot, effective, submission, checker)) would keep the three sites from drifting as the chain grows.🤖 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/projects/authorization_reads.py` around lines 107 - 163, The policy-chain consistency checks in the shown target validation repeat logic from authorize_project_active_guide_read and ProjectService.validate_activation_ready. Extract a shared helper such as effective_chain_is_consistent accepting guide, snapshot, effective, submission, and checker, move the row-to-row and integrity predicates into it, and update all three call sites to reuse the helper while preserving the current failure behavior.backend/app/modules/authorization/kernel.py (1)
83-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winActive-guide admin role allowlist is defined twice. Enforcement (kernel) and projection (read service) each own a private copy of the same frozenset; if they drift,
effective_action_idswill advertiseproject.active_guide.readto an actor the kernel denies (or hide it from one it allows).
backend/app/modules/authorization/kernel.py#L83-L85: import the shared allowlist instead of declaring it locally.backend/app/modules/authorization/read_service.py#L65-L67: remove this duplicate and import the same shared constant (e.g. exported fromcatalogue.py).🤖 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 83 - 85, Centralize the active-guide admin role allowlist in a shared authorization module such as catalogue.py. In backend/app/modules/authorization/kernel.py lines 83-85, remove the local _ACTIVE_GUIDE_ADMIN_ROLES declaration and import the shared constant; do the same in backend/app/modules/authorization/read_service.py lines 65-67, preserving both consumers’ existing behavior.backend/app/modules/authorization/runtime.py (1)
320-331: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin
guide_status == "active"in the policy-readboundpredicate.
ProjectActiveGuideReadResourceContextrequires the active guide status, but this model derivesboundwithout it, so a caller could asserttarget_exists=Truefor a draft/superseded guide. The current composer nulls non-active guides, so this is defensive rather than exploitable, but the strict context should carry the invariant itself. The strayandon its own line (Line 322) can also be folded.🛡️ Proposed tightening
bound = ( self.project_status == "active" - and - self.source_snapshot_id is not None + and self.guide_status == "active" + and self.source_snapshot_id is not None and self.source_snapshot_hash is not None🤖 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 320 - 331, Update the policy-read bound predicate around the bound computation to require guide_status == "active" alongside the existing project and policy checks, ensuring target_exists=True is rejected for draft or superseded guides. Fold the standalone and into the adjacent condition without changing the existing consistency validation.backend/app/modules/projects/repository.py (1)
219-227: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
lock_guide_source_snapshotstub.This by-ID row-lock method is only defined in
backend/app/modules/projects/repository.py; the active-guide path useslock_latest_guide_source_snapshotplus the already-calledlock_guide_source_snapshot_items(snapshot.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/projects/repository.py` around lines 219 - 227, Remove the unused lock_guide_source_snapshot method from the repository. Leave lock_latest_guide_source_snapshot and lock_guide_source_snapshot_items unchanged, as they provide the active-guide locking flow.backend/app/modules/projects/service.py (1)
1880-1918: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared bundle-shaping logic to avoid drift with
_active_response.This new method duplicates 8 of the 9
model_validatecalls in_active_response(guide, sufficiency, submission, effective, pre-submit checker, post-submit checker, review, revision) verbatim — only the snapshot construction andpayment_policydiffer. A future field addition to the shared bundle risks being applied to only one of the two projections.♻️ Suggested extraction of shared shaping logic
+ def _shared_active_bundle_fields( + self, + guide: ProjectGuide, + sufficiency_report: GuideSufficiencyReport, + submission_artifact_policy: SubmissionArtifactPolicy, + effective_policy: EffectiveProjectSubmissionArtifactPolicy, + pre_submit_checker_policy: PreSubmitCheckerPolicy, + post_submit_checker_policy: PostSubmitCheckerPolicy, + review_policy: ReviewPolicy, + revision_policy: RevisionPolicy, + ) -> dict[str, Any]: + """Shape the fields shared by active-guide activation and read projections.""" + return { + "guide": ProjectGuideResponse.model_validate(guide), + "guide_sufficiency_report": GuideSufficiencyReportResponse.model_validate( + sufficiency_report + ), + "submission_artifact_policy": SubmissionArtifactPolicyResponse.model_validate( + submission_artifact_policy + ), + "effective_submission_artifact_policy": ( + EffectiveProjectSubmissionArtifactPolicyResponse.model_validate(effective_policy) + ), + "pre_submit_checker_policy": ActiveGuidePreSubmitCheckerPolicyResponse.model_validate( + pre_submit_checker_policy + ), + "post_submit_checker_policy": PostSubmitCheckerPolicyResponse.model_validate( + post_submit_checker_policy + ), + "review_policy": ReviewPolicyResponse.model_validate(review_policy), + "revision_policy": RevisionPolicyResponse.model_validate(revision_policy), + }Then both
active_guide_read_responseand_active_responsecall this helper and only add their ownguide_source_snapshot(+payment_policyfor_active_response).🤖 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/projects/service.py` around lines 1880 - 1918, Extract the shared policy and guide response shaping from active_guide_read_response and _active_response into a common helper, preserving the existing model_validate mappings. Update both callers to use the helper, with active_guide_read_response adding only its source snapshot and _active_response adding its source snapshot and payment policy.
🤖 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.
Nitpick comments:
In `@backend/app/modules/authorization/kernel.py`:
- Around line 83-85: Centralize the active-guide admin role allowlist in a
shared authorization module such as catalogue.py. In
backend/app/modules/authorization/kernel.py lines 83-85, remove the local
_ACTIVE_GUIDE_ADMIN_ROLES declaration and import the shared constant; do the
same in backend/app/modules/authorization/read_service.py lines 65-67,
preserving both consumers’ existing behavior.
In `@backend/app/modules/authorization/runtime.py`:
- Around line 320-331: Update the policy-read bound predicate around the bound
computation to require guide_status == "active" alongside the existing project
and policy checks, ensuring target_exists=True is rejected for draft or
superseded guides. Fold the standalone and into the adjacent condition without
changing the existing consistency validation.
In `@backend/app/modules/projects/authorization_reads.py`:
- Around line 107-163: The policy-chain consistency checks in the shown target
validation repeat logic from authorize_project_active_guide_read and
ProjectService.validate_activation_ready. Extract a shared helper such as
effective_chain_is_consistent accepting guide, snapshot, effective, submission,
and checker, move the row-to-row and integrity predicates into it, and update
all three call sites to reuse the helper while preserving the current failure
behavior.
In `@backend/app/modules/projects/repository.py`:
- Around line 219-227: Remove the unused lock_guide_source_snapshot method from
the repository. Leave lock_latest_guide_source_snapshot and
lock_guide_source_snapshot_items unchanged, as they provide the active-guide
locking flow.
In `@backend/app/modules/projects/service.py`:
- Around line 1880-1918: Extract the shared policy and guide response shaping
from active_guide_read_response and _active_response into a common helper,
preserving the existing model_validate mappings. Update both callers to use the
helper, with active_guide_read_response adding only its source snapshot and
_active_response adding its source snapshot and payment policy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e35564f-6649-4881-9192-ef04c11dc3ce
📒 Files selected for processing (24)
.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-11C2-effective-policy-active-guide-reads.md.github/workflows/backend.ymlbackend/app/api/deps/authorization.pybackend/app/modules/authorization/catalogue.pybackend/app/modules/authorization/kernel.pybackend/app/modules/authorization/read_service.pybackend/app/modules/authorization/repository.pybackend/app/modules/authorization/runtime.pybackend/app/modules/projects/authorization_reads.pybackend/app/modules/projects/repository.pybackend/app/modules/projects/router.pybackend/app/modules/projects/schemas.pybackend/app/modules/projects/service.pybackend/scripts/api_contract_e2e.pybackend/tests/test_api_controls.pybackend/tests/test_audit.pybackend/tests/test_authorization.pybackend/tests/test_projects.pydocs/operations_authorization_service.mddocs/operations_project_operating_manual.mddocs/operations_roles_permissions.mddocs/spec_authorization_service.mddocs/spec_chunk_3_project_guide_foundation.md
…2-effective-policy-active-guide-reads
2 participants
Chunk
WS-AUTH-001-11C2— effective policy and active-guide read cutoverOutcome
Hard-cuts exactly three administrative GET actions:
project.effective_submission_artifact_policy.readproject.pre_submit_checker_policy.readproject.active_guide.readOnly a covered Project Manager, covered Audit Authority, or system Operator can read them. Finance, Access Administrator, contributors, services, wrong-scope callers, and token-role-only callers receive centralized concealed denial.
Security and design
AuthorizationService.require.GET /active-guideomitspayment_policy.External review repairs
All five CodeRabbit comments were addressed:
Detailed disposition:
.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-11C2-external-review-response.md.Verification
Exact head:
120bc553aeabfb6cc42d7fa0360552068204d66f30444471663.30444471409.Internal review
Architecture, security, product/ops, QA, senior engineering, CI integrity, docs, reuse/dedup, and test-delta passed after repair.
Human review focus
Human merge approval remains required.