feat: add authenticated streaming group export with self-service read tokens#199
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds shared bearer-token crypto, a new ChangesPersonal access tokens and streaming group export
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Ready to review this PR? Stage has broken it down into 7 individual chapters for you: 
Chapters generated by Stage for commit 209bf45 on Jul 8, 2026 1:56pm UTC. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Dockerfile (1)
33-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGunicorn flags duplicated across three files risk drifting.
The same
--workers 3 --threads 4 --timeout 300 --max-requests 500 --max-requests-jitter 50string is hardcoded here, indocker-compose.yml, and described indocs/deployment.md. A future tuning change is likely to update one and miss the others.Consider centralizing these in a single source (e.g., a
gunicorn.conf.py, an entrypoint script, or env-var-drivenGUNICORN_CMD_ARGS) referenced by both the Dockerfile and compose file.🤖 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 `@Dockerfile` around lines 33 - 35, The Gunicorn startup options are duplicated in the Dockerfile, compose setup, and deployment docs, so update the service to read them from one shared source instead of hardcoding the same flags in multiple places. Centralize the configuration in a single location such as a gunicorn.conf.py, entrypoint script, or GUNICORN_CMD_ARGS, and have both the Dockerfile CMD and docker-compose.yml reference that shared source while keeping docs aligned with the chosen approach.
🤖 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 `@Dockerfile`:
- Around line 33-35: The Gunicorn startup options are duplicated in the
Dockerfile, compose setup, and deployment docs, so update the service to read
them from one shared source instead of hardcoding the same flags in multiple
places. Centralize the configuration in a single location such as a
gunicorn.conf.py, entrypoint script, or GUNICORN_CMD_ARGS, and have both the
Dockerfile CMD and docker-compose.yml reference that shared source while keeping
docs aligned with the chosen approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df956539-5b76-45bc-8e6d-7716d5a26a39
📒 Files selected for processing (17)
AGENTS.mdDockerfileconfig/settings.pydocker-compose.ymldocs/api-v1.mddocs/deployment.mdforensics/admin.pyforensics/analysis.pyforensics/management/commands/create_access_token.pyforensics/migrations/0013_personalaccesstoken.pyforensics/models.pyforensics/streaming.pyforensics/templates/forensics/profile.htmlforensics/tests.pyforensics/token_crypto.pyforensics/urls.pyforensics/views.py
erskingardner
left a comment
erskingardner
left a comment
There was a problem hiding this comment.
Two findings from an adversarial pass. The first one is the merge-blocker in my view; the second is a smaller fail-closed cleanup for token expiry handling.
d4a80fd to
b1f2b94
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
forensics/models.py (1)
29-141: 📐 Maintainability & Code Quality | 🔵 TrivialConsider extracting shared token fields/behavior into an abstract base model.
UploadTokenandPersonalAccessTokenduplicate the entire field set (token_prefix,token_hash,is_active,created_at,expires_at,last_used_at) plus identicalmark_used()bodies and nearly-identicalis_expired()/authenticate()wrappers. An abstract base model (token_crypto-backed mixin) would keep the two credential types in sync as the crypto/lifecycle logic evolves, and would also resolve the asymmetry where onlyUploadTokenexposes a publichash_secret()wrapper.Not blocking, since the delegation to
token_cryptoalready centralizes the actual crypto logic — this is purely about reducing duplicated field/method boilerplate between the two models.🤖 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/models.py` around lines 29 - 141, Extract the duplicated token fields and lifecycle helpers from UploadToken and PersonalAccessToken into a shared abstract base model or token_crypto-backed mixin. Move the common fields (token_prefix, token_hash, is_active, created_at, expires_at, last_used_at) and shared methods like mark_used(), is_expired(), and the common authenticate/hash wrapper behavior into the base, then keep only model-specific pieces such as PersonalAccessToken.user and its owner-active check in authenticate(). This keeps both models aligned and removes the current asymmetry where only UploadToken exposes hash_secret().
🤖 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 2871-2877: Guard the expiry parsing in the shared token-expiration
logic so extreme `expires_in_days` values cannot reach `timezone.now() +
timedelta(days=days)` and raise `OverflowError`; in the function that currently
converts `value` with `int()` and returns the timedelta, add an upper-bound
validation and reject too-large inputs with a `ValueError` before the addition.
Apply the same protection in `create_access_token` so both the POST path and the
CLI command use the same safe validation behavior.
---
Nitpick comments:
In `@forensics/models.py`:
- Around line 29-141: Extract the duplicated token fields and lifecycle helpers
from UploadToken and PersonalAccessToken into a shared abstract base model or
token_crypto-backed mixin. Move the common fields (token_prefix, token_hash,
is_active, created_at, expires_at, last_used_at) and shared methods like
mark_used(), is_expired(), and the common authenticate/hash wrapper behavior
into the base, then keep only model-specific pieces such as
PersonalAccessToken.user and its owner-active check in authenticate(). This
keeps both models aligned and removes the current asymmetry where only
UploadToken exposes hash_secret().
🪄 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
Run ID: 614663a0-689e-46a1-ae37-17280dd69049
📒 Files selected for processing (17)
AGENTS.mdDockerfileconfig/settings.pydocker-compose.ymldocs/api-v1.mddocs/deployment.mdforensics/admin.pyforensics/analysis.pyforensics/management/commands/create_access_token.pyforensics/migrations/0013_personalaccesstoken.pyforensics/models.pyforensics/streaming.pyforensics/templates/forensics/profile.htmlforensics/tests.pyforensics/token_crypto.pyforensics/urls.pyforensics/views.py
✅ Files skipped from review due to trivial changes (3)
- AGENTS.md
- docs/deployment.md
- docker-compose.yml
🚧 Files skipped from review as they are similar to previous changes (8)
- forensics/management/commands/create_access_token.py
- Dockerfile
- config/settings.py
- forensics/analysis.py
- forensics/urls.py
- forensics/admin.py
- forensics/token_crypto.py
- forensics/templates/forensics/profile.html
b1f2b94 to
d3ffcab
Compare
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/token_crypto.py`:
- Line 42: The lookup prefix generated in token_crypto is too short and can
collide during row creation. Update the lookup prefix generation used for
token_prefix to use a wider value in the same spot where secrets.token_hex(4) is
called, and keep it within the existing max_length constraints by switching to a
longer hex token. This change should be applied in the token generation flow
that feeds both token models so creation does not fail on avoidable collisions.
🪄 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
Run ID: 33de30af-9353-4889-b841-d1a8b39288a4
📒 Files selected for processing (18)
AGENTS.mdDockerfileconfig/settings.pydocker-compose.ymldocs/api-v1.mddocs/deployment.mdforensics/admin.pyforensics/analysis.pyforensics/management/commands/create_access_token.pyforensics/management/commands/create_upload_token.pyforensics/migrations/0013_personalaccesstoken.pyforensics/models.pyforensics/streaming.pyforensics/templates/forensics/profile.htmlforensics/tests.pyforensics/token_crypto.pyforensics/urls.pyforensics/views.py
✅ Files skipped from review due to trivial changes (2)
- AGENTS.md
- docs/deployment.md
🚧 Files skipped from review as they are similar to previous changes (9)
- docker-compose.yml
- forensics/urls.py
- forensics/admin.py
- forensics/templates/forensics/profile.html
- forensics/analysis.py
- config/settings.py
- forensics/management/commands/create_access_token.py
- Dockerfile
- forensics/tests.py
d3ffcab to
c5e0e95
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 13-19: The personal access token description is too broad and
should be scoped to the streaming group export only. Update the wording in the
export docs so the PAT text near the Authorization header and the `gpat_…`
mention states it can read the streaming group export, not general forensic data
or other forensic APIs. Keep the distinction from `goggles_…` upload tokens, and
adjust the `docs/api-v1.md` copy accordingly.
- Around line 125-136: The fenced NDJSON example in the API v1 docs is missing
an explicit language tag, which triggers the markdown lint warning. Update the
code block that contains the manifest/source/event/.../eof examples to use a
text fence instead of an unlabeled block, keeping the mixed NDJSON-and-comment
content readable and lint-compliant.
In `@forensics/views.py`:
- Around line 2918-2925: The one-time PAT creation response in the view that
issues PersonalAccessToken should be marked non-cacheable because it renders the
raw token once and must not be retained by caches. Update the return path that
renders "forensics/profile.html" with profile_context(..., new_token=raw_token)
to attach a Cache-Control: no-store header, following the same pattern used by
the export response, so the sensitive token is never cached.
🪄 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
Run ID: d019dd89-4af2-4bda-9dd8-fff3d4ce3c43
📒 Files selected for processing (18)
AGENTS.mdDockerfileconfig/settings.pydocker-compose.ymldocs/api-v1.mddocs/deployment.mdforensics/admin.pyforensics/analysis.pyforensics/management/commands/create_access_token.pyforensics/management/commands/create_upload_token.pyforensics/migrations/0013_personalaccesstoken.pyforensics/models.pyforensics/streaming.pyforensics/templates/forensics/profile.htmlforensics/tests.pyforensics/token_crypto.pyforensics/urls.pyforensics/views.py
✅ Files skipped from review due to trivial changes (3)
- docs/deployment.md
- AGENTS.md
- config/settings.py
🚧 Files skipped from review as they are similar to previous changes (10)
- forensics/management/commands/create_upload_token.py
- forensics/analysis.py
- forensics/urls.py
- docker-compose.yml
- Dockerfile
- forensics/admin.py
- forensics/templates/forensics/profile.html
- forensics/management/commands/create_access_token.py
- forensics/token_crypto.py
- forensics/tests.py
c5e0e95 to
441a15e
Compare
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
forensics/views.py (1)
2185-2186: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winClear ordering before
DISTINCTindistinct_values().
AuditEventhas default ordering, so this query can carry ordering columns intoDISTINCTand return extra rows for the same identity. Clearing ordering keeps the dedupe at the database level.♻️ Proposed change
def distinct_values(queryset, field: str) -> set[str]: - return {value for value in queryset.values_list(field, flat=True).distinct() if value} + return { + value + for value in queryset.order_by().values_list(field, flat=True).distinct() + if value + }🤖 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/views.py` around lines 2185 - 2186, The distinct_values() helper is relying on the queryset’s default ordering, which can leak ordering columns into the DISTINCT and produce duplicate identities. Update distinct_values() to clear ordering on the queryset before calling values_list(...).distinct(), preserving the existing field filtering while ensuring deduplication happens correctly at the database level.
🤖 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 `@forensics/views.py`:
- Around line 2185-2186: The distinct_values() helper is relying on the
queryset’s default ordering, which can leak ordering columns into the DISTINCT
and produce duplicate identities. Update distinct_values() to clear ordering on
the queryset before calling values_list(...).distinct(), preserving the existing
field filtering while ensuring deduplication happens correctly at the database
level.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 027a2fdc-9bc7-408b-abf1-b42a956fbac7
📒 Files selected for processing (18)
AGENTS.mdDockerfileconfig/settings.pydocker-compose.ymldocs/api-v1.mddocs/deployment.mdforensics/admin.pyforensics/analysis.pyforensics/management/commands/create_access_token.pyforensics/management/commands/create_upload_token.pyforensics/migrations/0013_personalaccesstoken.pyforensics/models.pyforensics/streaming.pyforensics/templates/forensics/profile.htmlforensics/tests.pyforensics/token_crypto.pyforensics/urls.pyforensics/views.py
✅ Files skipped from review due to trivial changes (3)
- forensics/analysis.py
- docs/deployment.md
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (11)
- config/settings.py
- forensics/urls.py
- Dockerfile
- forensics/admin.py
- forensics/templates/forensics/profile.html
- forensics/management/commands/create_upload_token.py
- forensics/management/commands/create_access_token.py
- docker-compose.yml
- forensics/token_crypto.py
- docs/api-v1.md
- forensics/tests.py
|
I don't know if this is that helpful at this point but I ran another agent (composer 2.5) against this PR and got all this back: Adversarial review of PR #199. CI is green and several earlier review threads (filter contract, expiry validation) look addressed. Below are the issues I’d still raise, ordered by severity. Critical / high1.
|
you're right, and I went the other way instead of completing the filtering: the export now takes no query filters at all, filters is gone from the manifest too. Applying them to only some sections misrepresents the payload (exactly what you reproduced), and filtering raw events would break the completeness contract the export exists to provide (it feeds the fail-closed replay pipeline, which needs the whole group). Narrowed views stay on the paginated projection endpoints; rationale documented in api-v1.md. Shout if you'd rather have real filtering across all sections, but I think complete-or-nothing is the more honest contract for this endpoint.
fixed, parse_access_token_expiry now validates server-side and rejects non-positive/unparsable values with a rendered error (no token minted), matching the management command's behavior. Tests cover 0, negatives, and garbage input. |
2 participants
Summary by CodeRabbit
GOGGLES_EXPORTS_ENABLEDkill-switch.