feat(auth): add durable authorization read rate control#175
Open
Abiorh001 wants to merge 16 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds the ChangesAuthorization Read Rate Control
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Alembic
participant PostgreSQL
participant FastAPI
participant RateControlService
Alembic->>PostgreSQL: Add authorization_read scope
FastAPI->>RateControlService: Consume authorization_read allowance
RateControlService->>PostgreSQL: Persist counter state
PostgreSQL-->>RateControlService: Return decision
RateControlService-->>FastAPI: Allow or rate-limit request
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/alembic/versions/0032_authorization_read_rate_control.py (1)
24-31: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConfirm
pg_get_exprcanonical form is stable across the PostgreSQL versions you deploy.
_require_scope_constraintcompares the live constraint against these hardcodedANY (ARRAY[...])strings via exact equality.pg_get_exproutput formatting is PostgreSQL-version dependent, so if a deployed server's major version differs from the CI-pinnedpostgres:16,upgrade()/downgrade()could refuse a structurally-correct constraint with "unexpected API rate-control scope constraint" and block the migration. Worth confirming all target environments run the same PG major version as CI, or documenting that constraint as a deployment prerequisite.🤖 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/0032_authorization_read_rate_control.py` around lines 24 - 31, Verify and document that all deployment PostgreSQL major versions match the CI-pinned PostgreSQL 16 version before relying on the exact pg_get_expr strings used by _require_scope_constraint. If version parity cannot be guaranteed, replace the exact comparison with a version-independent structural validation while preserving the existing upgrade and downgrade constraint checks.
🤖 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/alembic/versions/0032_authorization_read_rate_control.py`:
- Around line 24-31: Verify and document that all deployment PostgreSQL major
versions match the CI-pinned PostgreSQL 16 version before relying on the exact
pg_get_expr strings used by _require_scope_constraint. If version parity cannot
be guaranteed, replace the exact comparison with a version-independent
structural validation while preserving the existing upgrade and downgrade
constraint checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 00579e92-982a-49e0-a3ac-370cad897991
📒 Files selected for processing (18)
.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DECISIONS.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-10B1-authorization-read-rate-control.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-10B1-external-review-response.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-10B1-internal-review-evidence.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-10B1-pr-trust-bundle.md.agent-loop/merge-intents/WS-AUTH-001-10B1.json.github/workflows/backend.ymlbackend/alembic/versions/0032_authorization_read_rate_control.pybackend/app/api/deps/api_controls.pybackend/app/core/config.pybackend/app/modules/api_controls/models.pybackend/app/modules/api_controls/service.pybackend/tests/test_alembic.pybackend/tests/test_api_rate_controls.pybackend/tests/test_config.pydocs/operations_authorization_service.mddocs/spec_authorization_service.mdscripts/test_agent_gates.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/test_alembic.py (1)
5328-5344: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winMake lock-wait polling deadline-based.
The helper can exhaust 100 immediate polls before the downgrade worker has connected and requested
AccessExclusiveLock, causing a flaky failure unrelated to migration correctness. Use a bounded monotonic deadline with event-loop yielding, or an explicit session handshake, instead of a fixed iteration count.Suggested adjustment
- for _ in range(100): + deadline = asyncio.get_running_loop().time() + 5 + while asyncio.get_running_loop().time() < deadline: async with engine.connect() as connection: waiting = await connection.scalar(...) if waiting: return await asyncio.sleep(0)🤖 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_alembic.py` around lines 5328 - 5344, Update _wait_for_rate_control_table_lock to poll until a bounded monotonic deadline rather than stopping after 100 iterations. Continue yielding to the event loop between checks so the downgrade worker can acquire its connection, and raise the existing assertion only when the deadline expires.
🤖 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_alembic.py`:
- Around line 2799-2808: Update the migration round-trip test around
command.upgrade and command.downgrade to seed both legacy scopes, first_access
and admin_mutation, before upgrading to 0033_authorization_read_rate. Assert
both scopes are preserved after upgrade, insert authorization_read, verify
downgrade raises the existing RuntimeError, and assert both legacy scopes remain
afterward.
---
Outside diff comments:
In `@backend/tests/test_alembic.py`:
- Around line 5328-5344: Update _wait_for_rate_control_table_lock to poll until
a bounded monotonic deadline rather than stopping after 100 iterations. Continue
yielding to the event loop between checks so the downgrade worker can acquire
its connection, and raise the existing assertion only when the deadline expires.
🪄 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: 8c2314f6-4676-41e7-b7a1-29bfcd4943d3
📒 Files selected for processing (13)
.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DECISIONS.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/INTENT.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/PLAN.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-10B1-authorization-read-rate-control.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-10B-internal-review-evidence.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-10B-pr-trust-bundle.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-10B1-external-review-response.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-10B1-internal-review-evidence.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-10B1-pr-trust-bundle.mdbackend/alembic/versions/0033_authorization_read_rate_control.pybackend/tests/test_alembic.pydocs/operations_authorization_service.mddocs/spec_authorization_service.md
🚧 Files skipped from review as they are similar to previous changes (4)
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DECISIONS.md
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-10B1-internal-review-evidence.md
- docs/operations_authorization_service.md
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-10B1-external-review-response.md
Comment on lines
+2799
to
+2808
| asyncio.run(insert("first_access", 41)) | ||
| command.upgrade(config, "0033_authorization_read_rate") | ||
| assert asyncio.run(scopes()) == ["first_access"] | ||
| asyncio.run(insert("authorization_read", 42)) | ||
| with pytest.raises( | ||
| RuntimeError, | ||
| match="cannot downgrade live authorization-read rate controls", | ||
| ): | ||
| command.downgrade(config, "0032_artifact_recovery") | ||
| assert asyncio.run(scopes()) == ["authorization_read", "first_access"] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover preservation of both legacy scopes.
Only first_access is seeded, so this test does not prove that an existing admin_mutation counter survives the 0032_artifact_recovery → 0033_authorization_read_rate round trip and remains after downgrade.
Suggested test adjustment
asyncio.run(insert("first_access", 41))
+asyncio.run(insert("admin_mutation", 42))
command.upgrade(config, "0033_authorization_read_rate")
-assert asyncio.run(scopes()) == ["first_access"]
+assert asyncio.run(scopes()) == ["admin_mutation", "first_access"]
...
-assert asyncio.run(scopes()) == ["first_access"]
+assert asyncio.run(scopes()) == ["admin_mutation", "first_access"]Also applies to: 2842-2845
🤖 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_alembic.py` around lines 2799 - 2808, Update the migration
round-trip test around command.upgrade and command.downgrade to seed both legacy
scopes, first_access and admin_mutation, before upgrading to
0033_authorization_read_rate. Assert both scopes are preserved after upgrade,
insert authorization_read, verify downgrade raises the existing RuntimeError,
and assert both legacy scopes remain afterward.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Chunk
WS-AUTH-001-10B1— Authorization Read Rate Control. Merge intent:.agent-loop/merge-intents/WS-AUTH-001-10B1.json.Goal and intent
Provide durable cross-replica abuse control before privacy-sensitive project-role reads are exposed. This chunk prepares the shared control only; AUTH-10B2 owns route attachment, disclosure, concealment, cursors, and action activation.
What changed and why
0033_authorization_read_rateafter ART0032_artifact_recoveryadds one closed PostgreSQLauthorization_readcounter scopeDesign and alternatives
The existing framed HMAC, PostgreSQL time, repository, and independently committed session remain the only implementation. A second limiter/store, raw identity persistence, application-clock windows, route activation here, and version-check bypasses were rejected.
Scope control
No production route, action activation, cursor, authorization disclosure, or mutation is added. One merge intent declares AUTH-10B2 with explicit start.
Test delta and evidence
2d6d347eagainst main92b8a7aa29876766313: preflight, API E2E, all four shards, aggregate 78% coverage, additive API-controls 90% coverage, and final gate passedReviewer results
Senior engineering, QA/test, security/auth, product/ops, architecture, CI integrity, docs, reuse/dedup, and test delta: PASS. CodeRabbit’s PostgreSQL-version comment is resolved by an executable PostgreSQL 16 preflight and stop/forward-repair guidance.
CI integrity
No skip, continue-on-error, threshold reduction, shard bypass, or weakened assertion was introduced. The hosted docstring gate passed.
Risks and recovery
Constraint drift fails closed before DDL. Downgrade locks before row checks. A non-16 PostgreSQL target must stop and use a reviewed forward migration. Stored keys contain no raw identity or authorization data.
Human review and merge ownership
Review the constraint transition, transactional downgrade, scope isolation, PostgreSQL 16 prerequisite, absence of route/action changes, and additive coverage gate. The user retains merge approval; AUTH-10B2 does not start automatically.
Summary by CodeRabbit
New Features
Documentation