Add bounded actor and identity-link administration reads#146
Conversation
…-actor-identity-admin-reads
|
Warning Review limit reached
Next review available in: 49 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 (12)
📝 WalkthroughWalkthroughAUTH-09C activates two administrative actor read routes, adds strict response and authorization contracts, implements serialized grant checks and bounded disclosure, expands privacy/concurrency/failure testing, and updates rollout, documentation, review evidence, and agent-gate state. ChangesAUTH-09C administrative reads
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant AuthorizationRouter
participant AuthorizationService
participant ActorService
participant PostgreSQL
Caller->>AuthorizationRouter: Request actor profile or identity-link
AuthorizationRouter->>AuthorizationService: Authorize exact admin read
AuthorizationService->>PostgreSQL: Lock and revalidate caller grant
AuthorizationRouter->>ActorService: Read bounded target representation
ActorService->>PostgreSQL: Fetch exact target
AuthorizationRouter->>PostgreSQL: Touch caller and commit
AuthorizationRouter-->>Caller: Return response or structured error
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 1
🧹 Nitpick comments (4)
backend/tests/test_authorization.py (3)
535-541: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the exact self-target freshness update.
Checking only
len(response.updates)passes if either route patches the wrong fields or values. Assertupdated_at/last_seen_atfor profile reads andlast_verified_atfor identity-link reads.Also applies to: 579-585
🤖 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` around lines 535 - 541, Update the response assertions in the authorization tests using the Response test double’s updates collection: for profile-read routes, assert the exact self-target freshness update contains updated_at and last_seen_at; for identity-link-read routes, assert it contains last_verified_at. Replace count-only checks while preserving the existing route coverage and expected update values.
801-801: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the caller and permission passed to
find_effective_grant.The helper discards positional arguments, so this security test still passes if the kernel queries a grant for the wrong actor or permission. Record
_argsand assert the caller profile ID and action permission alongside the locking options.Also applies to: 815-817, 893-897
🤖 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 801, Update the authorization test mocks around find_effective_grant to retain positional arguments in find_calls instead of discarding _args. Extend the relevant assertions to verify the caller profile ID and action permission, while preserving the existing locking-option checks across all affected cases.
588-627: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the identity-link route’s missing-resource rollback.
This test exercises only
read_actor_profile, leaving the second new endpoint’s stable 404 and no-touch/no-commit contract unprotected. Parameterize both routes as done for the success path.🤖 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` around lines 588 - 627, The test coverage currently validates missing-resource rollback only for read_actor_profile. Parameterize test_actor_admin_missing_resource_rolls_back_without_touch_or_commit across both identity-link routes, invoking each route with the appropriate arguments while preserving the expected 404 actor_resource_not_found response and authorize/lookup/rollback event sequence; ensure neither route records touch or commit.backend/tests/test_auth.py (1)
2361-2393: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExercise project-scoped denial for both admin read actions.
Only the profile route is checked. Add the identity-link request because it has a distinct action/permission and must independently reject project-scoped Audit Authority grants.
Proposed test extension
project_audit_actor_read = await client.get( f"/api/v1/actors/{target_id}", headers=audit_headers, ) +project_audit_link_read = await client.get( + f"/api/v1/actors/{target_id}/identity-links", + headers=audit_headers, +) @@ -assert project_audit_actor_read.status_code == 403 -assert project_audit_actor_read.json()["error"]["code"] == "permission_not_granted" +for response in (project_audit_actor_read, project_audit_link_read): + assert response.status_code == 403 + assert response.json()["error"]["code"] == "permission_not_granted"🤖 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_auth.py` around lines 2361 - 2393, Extend the audit-authority denial test around project_audit_actor_read to also request the target actor’s identity links using audit_headers. Assert that this identity-link request independently returns 403 with the permission_not_granted error, alongside the existing profile-route denial.
🤖 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 `@backend/tests/test_auth.py`:
- Around line 3373-3388: The wait_for_transition_lock helper currently accepts
any PostgreSQL lock waiter, so it must correlate the observed wait with
transition_task. Track the transition backend PID and query pg_blocking_pids for
that PID, or identify the exact transition query and locked row, returning only
when that specific transition is blocked while preserving the existing timeout
and assertion behavior.
---
Nitpick comments:
In `@backend/tests/test_auth.py`:
- Around line 2361-2393: Extend the audit-authority denial test around
project_audit_actor_read to also request the target actor’s identity links using
audit_headers. Assert that this identity-link request independently returns 403
with the permission_not_granted error, alongside the existing profile-route
denial.
In `@backend/tests/test_authorization.py`:
- Around line 535-541: Update the response assertions in the authorization tests
using the Response test double’s updates collection: for profile-read routes,
assert the exact self-target freshness update contains updated_at and
last_seen_at; for identity-link-read routes, assert it contains
last_verified_at. Replace count-only checks while preserving the existing route
coverage and expected update values.
- Line 801: Update the authorization test mocks around find_effective_grant to
retain positional arguments in find_calls instead of discarding _args. Extend
the relevant assertions to verify the caller profile ID and action permission,
while preserving the existing locking-option checks across all affected cases.
- Around line 588-627: The test coverage currently validates missing-resource
rollback only for read_actor_profile. Parameterize
test_actor_admin_missing_resource_rolls_back_without_touch_or_commit across both
identity-link routes, invoking each route with the appropriate arguments while
preserving the expected 404 actor_resource_not_found response and
authorize/lookup/rollback event sequence; ensure neither route records touch or
commit.
🪄 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: 313d93f6-0386-4cce-97a6-d9442bdeebb3
📒 Files selected for processing (25)
.agent-loop/LOOP_STATE.md.agent-loop/REVIEW_LOG.md.agent-loop/WORK_QUEUE.md.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-09C-actor-identity-admin-reads.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-09C-internal-review-evidence.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-09C-pr-trust-bundle.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-09C-preimplementation-plan-review.md.agent-loop/merge-intents/WS-AUTH-001-09C.jsonbackend/app/modules/actors/schemas.pybackend/app/modules/actors/service.pybackend/app/modules/authorization/catalogue.pybackend/app/modules/authorization/kernel.pybackend/app/modules/authorization/router.pybackend/app/modules/authorization/runtime.pybackend/scripts/api_contract_e2e.pybackend/tests/test_actors.pybackend/tests/test_api_controls.pybackend/tests/test_audit.pybackend/tests/test_auth.pybackend/tests/test_authorization.pydocs/operations_authorization_service.mddocs/spec_authorization_service.mdscripts/test_agent_gates.py
…-actor-identity-admin-reads # Conflicts: # .agent-loop/LOOP_STATE.md # .agent-loop/WORK_QUEUE.md
2 participants
WS-AUTH-001-09C PR Trust Bundle
Chunk
WS-AUTH-001-09C- Actor And Identity-Link Administration ReadsGoal
Give authorized human system administrators one exact, privacy-bounded view of
an ActorProfile or its single v0.1 ActorIdentityLink before lifecycle mutations
are introduced.
Human-Approved Intent
The user explicitly started AUTH-09C after PR #143 merged and signed memory
stopped. No compatibility route, service admission, lifecycle mutation, or
feature-owned authorization is authorized.
What Changed
GET /api/v1/actors/{actor_profile_id}andGET /api/v1/actors/{actor_profile_id}/identity-linksresponses.actor.profile.readandactor.identity_link.read, moving thecatalogue to 12 active / 53 planned actions.
revalidation through target disclosure and route commit.
HTTP proof, and behavior-first privacy, evidence, rollback, and race tests.
Why It Changed
AUTH-09D needs administrators to select an exact actor or identity-link target.
The selection surface must exist first without adding list/search or revealing
external identity and authority data.
Design Chosen
The server composes action-specific contexts from the path, authorizes before
lookup, reuses ActorService and ActorRepository exact reads, touches only the
verified caller, then commits the allow evidence and touch once. Missing targets
roll back and share one 404 contract. Self-target responses use the refreshed
caller timestamps from that same touch.
Alternatives Rejected
Collection/search APIs, client-supplied actions or roles, project-scoped actor
registry authority, target-row locks,
AuthorityControlon reads, a secondsession, compatibility aliases, and raw issuer/subject output were rejected as
outside this chunk or unsafe.
Scope Control
Current main at
e118e33is integrated and PR #144's WS-CON artifacts areunchanged. No migration, dependency, workflow, grant/assignment behavior,
service admission, ART/REV/CON activation, or lifecycle mutation changed.
Product Behavior
Effective system Access Administrator and Audit Authority grants may inspect
active, suspended, or deactivated human/service profiles and active/revoked
links. Project-scoped Audit Authority and every unsupported caller are denied
before target lookup.
Acceptance Criteria Proof
Strict response fields, exact system authority, authorization-before-lookup,
stable 404 rollback, self-target freshness, exact persisted evidence, null
unverified service timestamps, response/log privacy, SQL failure rollback, and
real lock serialization are all covered by behavior tests.
Tests And Checks Run
intent, diff integrity, and all 80 Agent Gates tests: passed.
Test Delta
Tests add exact role/scope, cross-context, response, OpenAPI, audit, privacy,
self-target, missing-target, injected-failure, and two-session disabling-race
assertions. No test was skipped, weakened, or changed to conceal behavior.
CI Integrity
The repository-wide 78 percent floor and focused 90 percent actor and
authorization floors are unchanged. No workflow, dependency, exclusion, or
threshold changed. GitHub Backend remains authoritative for the full suite.
Reviewer Results
Senior engineering, QA/test, security/auth, product/ops, architecture, CI
integrity, docs, reuse/dedup, and test delta all pass exact reviewed code SHA
6791381ceb9cb0c7f6ba163d4525c6c770c02ca6after valid findings were repaired.External Review
GitHub Backend, Agent Gates, CodeRabbit, and human review are pending on the
published head. External findings will be triaged without weakening the chunk.
Remaining Risks
The endpoints reveal bounded lifecycle timestamps and the closed local service
identity to system authorities by design. Exact external identity, provenance,
grant contents, and service authority remain concealed.
Follow-Up Work
AUTH-09D owns lifecycle mutations only after this chunk merges, signed memory
passes, and the user gives a separate explicit start. AUTH-09E remains the
later fixed-service runtime-admission boundary.
Human Review Focus
Review system-only role separation, caller/link/grant lock lifetime, response
privacy, self-target timestamp freshness, stable rollback behavior, exact audit
binding, and the absence of mutation or service admission.
Human Merge Ownership
The agent may publish and repair this branch but may not merge it. Only the
human may approve and merge the PR. Trusted-main automation owns post-merge
schema-v2 memory; AUTH-09D does not start automatically.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests