feat: allow access tokens to enumerate groups - #332
Conversation
WalkthroughThe change enables session and personal-access-token access to a paginated group index, adds readable-group scoping to exports, supports ChangesReadable group index API
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@forensics/views.py`:
- Around line 874-881: Replace offset-based pagination in forensics/views.py
lines 874-881 with an opaque keyset cursor tied to a server-provided polling
snapshot or equivalent fixed watermark, while preserving efficient polling and
change detection. Add a regression test in forensics/tests.py lines 5900-5955
that updates a later-page group between requests and verifies it remains
returned. Update docs/api-v1.md lines 130-135 to document the snapshot/cursor
contract and remove any claim that live offset pagination is stable.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 32d24c3f-989b-491b-8d60-53b3ea1014d0
📒 Files selected for processing (7)
AGENTS.mddocs/api-v1.mdforensics/analysis.pyforensics/migrations/0014_auditgroup_updated_at_slug_index.pyforensics/models.pyforensics/tests.pyforensics/views.py
Pip adversarial reviewVerdict: BLOCKING (1 blocking finding, 0 suggestions, 0 nitpicks) Blocking
Verification
Sensitive paths reviewed: forensics/views.py, forensics/models.py, and forensics/migrations/0014_auditgroup_updated_at_slug_index.py. |
Replace mutable offsets with a signed keyset cursor bound to a fixed polling watermark and the original change filter. Fixes #331
|
Addressed the blocking pagination finding in b6eab0d. Mutable offsets are replaced with a signed keyset cursor, fixed polling watermark, and cursor-bound |
Pip adversarial re-reviewVerdict: BLOCKING (the prior page-shift blocker is resolved; 1 remaining blocking finding) Prior blockerThe deletion/scope-removal case from the prior review is fixed at Blocking
Verification
Sensitive paths reviewed: |
Require periodic full index scans with slug deduplication instead of treating a wall-clock watermark as a strict checkpoint. Cover the real delayed-commit race with a second PostgreSQL transaction. Fixes #331
|
Addressed the remaining commit-order polling blocker in signed commit
Verification:
Sensitive scope: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
forensics/group_list_cursor.py (2)
51-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecific validation messages are swallowed by the broad
except.
InvalidGroupListCursorsubclassesValueError, so the targeted raises inside thetry(bad timestamp, bad slug) are caught at Line 68 and re-raised as the generic"cursor is invalid". Harmless for the API contract (all map to400), but the finer-grained messages are unreachable — consider re-raisingInvalidGroupListCursoruntouched if those messages are meant for logs.♻️ Preserve specific messages
+ except InvalidGroupListCursor: + raise except (BadSignature, TypeError, json.JSONDecodeError, ValueError) as exc: raise InvalidGroupListCursor("cursor is invalid") from exc🤖 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 `@forensics/group_list_cursor.py` around lines 51 - 69, Update decode_group_list_cursor so InvalidGroupListCursor exceptions raised for invalid timestamps or slug validation pass through unchanged instead of being converted to the generic “cursor is invalid” message. Add the specific exception type to the appropriate handling order while preserving generic wrapping for BadSignature, malformed JSON, TypeError, and other ValueError failures.
14-14: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCursor payload is signed but human-readable; the group slug travels in the query string as cleartext.
Signer.signonly appends a signature — the JSON (includingslug) stays readable in the cursor, which is passed as a URL query parameter and therefore lands in web-server/proxy access logs. Group references are sensitive data, anddocs/api-v1.mddescribes the cursor as "opaque".django.core.signing.dumps/loadsgives a base64 payload plus a timestamp, so you also get optionalmax_ageexpiry for stale cursors.♻️ Suggested change to opaque, expiring cursors
-from django.core.signing import BadSignature, Signer +from django.core import signing +from django.core.signing import BadSignature @@ -CURSOR_SIGNER = Signer(salt="goggles-groups-v1-cursor") +CURSOR_SALT = "goggles-groups-v1-cursor" +CURSOR_MAX_AGE_SECONDS = 24 * 60 * 60 @@ - return CURSOR_SIGNER.sign(json.dumps(payload, separators=(",", ":"))) + return signing.dumps(payload, salt=CURSOR_SALT, compress=True)And in
decode_group_list_cursor:payload = signing.loads(raw_cursor, salt=CURSOR_SALT, max_age=CURSOR_MAX_AGE_SECONDS)
signing.SignatureExpiredsubclassesBadSignature, so the existingexceptclause still maps it to400 invalid cursor.Note: the ast-grep
use-jsonifyhint on Line 47 is a Flask-specific rule and does not apply here.Also applies to: 42-48
🤖 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 `@forensics/group_list_cursor.py` at line 14, Replace the human-readable CURSOR_SIGNER encoding with django.core.signing.dumps/loads for group list cursors. Update decode_group_list_cursor to load using the cursor salt and an expiry max age, preserving the existing BadSignature handling so expired or invalid cursors return 400 invalid cursor. Ensure the serialized payload, including slug, is opaque in the query string.Sources: Coding guidelines, Linters/SAST tools
forensics/tests.py (1)
6169-6206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSecond connection ignores
OPTIONSand has no connect timeout; the raw INSERT column list is brittle.Two small robustness gaps in the PostgreSQL-only race test:
_postgres_connectiondropssettings_dict["OPTIONS"](sslmode, service, unix socket, etc.), so it can fail or connect differently from Django's own connection in CI. Noconnect_timeoutmeans a stuck server hangs the suite instead of failing.- The hardcoded column list will break the moment
AuditGroupgains a NOT NULL field without a DB default, and the failure will look like an unrelated test error.♻️ Suggested hardening
def _postgres_connection(self): import psycopg settings = connection.settings_dict + options = { + key: value + for key, value in (settings.get("OPTIONS") or {}).items() + if key != "pool" + } return psycopg.connect( host=settings["HOST"], port=settings["PORT"], dbname=settings["NAME"], user=settings["USER"], password=settings["PASSWORD"], + connect_timeout=10, + **options, )For the INSERT, deriving the column list from
AuditGroup._meta(or asserting the concrete field set once) keeps the failure mode obvious if the model changes.🤖 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 `@forensics/tests.py` around lines 6169 - 6206, Harden _postgres_connection by passing the relevant connection settings from settings["OPTIONS"] through to psycopg.connect and enforcing a finite connect timeout, while preserving Django’s host, port, database, and credential behavior. In test_delayed_upload_commit_is_discovered_by_full_rescan_not_incremental_watermark, replace the brittle hardcoded INSERT column list with columns derived from AuditGroup metadata (or an equivalent explicit model-field assertion), ensuring every required non-default field is supplied and future model changes fail clearly.
🤖 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 `@docs/api-v1.md`:
- Around line 127-131: Promote the “Polling contract” body text near the polling
guidance into a Markdown heading so it generates the `#polling-contract` anchor
referenced by updated_since. Preserve the existing wording and section placement
while ensuring the heading level matches the surrounding document structure.
---
Nitpick comments:
In `@forensics/group_list_cursor.py`:
- Around line 51-69: Update decode_group_list_cursor so InvalidGroupListCursor
exceptions raised for invalid timestamps or slug validation pass through
unchanged instead of being converted to the generic “cursor is invalid” message.
Add the specific exception type to the appropriate handling order while
preserving generic wrapping for BadSignature, malformed JSON, TypeError, and
other ValueError failures.
- Line 14: Replace the human-readable CURSOR_SIGNER encoding with
django.core.signing.dumps/loads for group list cursors. Update
decode_group_list_cursor to load using the cursor salt and an expiry max age,
preserving the existing BadSignature handling so expired or invalid cursors
return 400 invalid cursor. Ensure the serialized payload, including slug, is
opaque in the query string.
In `@forensics/tests.py`:
- Around line 6169-6206: Harden _postgres_connection by passing the relevant
connection settings from settings["OPTIONS"] through to psycopg.connect and
enforcing a finite connect timeout, while preserving Django’s host, port,
database, and credential behavior. In
test_delayed_upload_commit_is_discovered_by_full_rescan_not_incremental_watermark,
replace the brittle hardcoded INSERT column list with columns derived from
AuditGroup metadata (or an equivalent explicit model-field assertion), ensuring
every required non-default field is supplied and future model changes fail
clearly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 103e62a1-ff46-433c-9ad8-57ed259e99a7
📒 Files selected for processing (4)
docs/api-v1.mdforensics/group_list_cursor.pyforensics/tests.pyforensics/views.py
🚧 Files skipped from review as they are similar to previous changes (1)
- forensics/views.py
Serialize signed cursor payloads so sensitive group slugs are not written verbatim in query strings. Repair the polling docs anchor and harden the PostgreSQL commit-race test connection. Fixes #331
|
Addressed the remaining exact-head CodeRabbit feedback in signed commit
Added a cursor-opacity regression (observed RED against |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Pip exact-head adversarial re-reviewVerdict: BLOCKING (the prior commit-order omission is resolved; 1 blocking security finding remains) Prior blockerThe revised polling contract at exact head
The shared Blocking
Verification
Sensitive paths reviewed: |
|
Addressed the exact-head cursor disclosure blocker in signed commit
Verification:
|
Pip exact-head adversarial re-reviewVerdict: LGTM — prior BLOCKING finding cleared at exact signed head Resolution evidence
Verification
No blocking findings, suggestions, or nitpicks remain. Sensitive paths reviewed: |
1 participant
Fixes #331
Summary
updated_since, a periodic full-rescan contract for delayed commits, and a matching composite indexVerification
uv sync --frozenDJANGO_DEBUG=1 just check(276 passed; 1 PostgreSQL-only test skipped)DJANGO_DEBUG=1 just test-postgres(276 passed)DJANGO_DEBUG=1 uv run python manage.py checkuv run ruff check .uv run ruff format --check .DJANGO_DEBUG=1 uv run python manage.py makemigrations --check --dry-runSensitive paths
forensics/views.py: reader authentication principal and shared object-level group scopeforensics/group_list_cursor.py: signed cursor uses a non-sensitive internal-ID tie-breaker instead of serializing group slugsforensics/models.py: polling index on group metadataforensics/migrations/0014_auditgroup_updated_at_id_index.py: database index migrationRelease hygiene
Summary by CodeRabbit
GET /api/v1/groups/with cursor-based pagination, stable ordering, andupdated_sincepolling via a signed cursor.401(no HTML redirects).404for export.