Allow export retrieve/content across teammates in project#57541
Allow export retrieve/content across teammates in project#57541
Conversation
- Detail actions scoped to team; list stays created_by-only - Session listing filter narrowed to user's exports only - Tests for teammate png/video access and RBAC - Drop verbose session-problem pre-emission workflow log
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def test_teammate_can_access_system_session_video_export_content(self) -> None: | ||
| from posthog.session_recordings.models.session_recording import SessionRecording | ||
|
|
||
| owner = self.user | ||
| teammate = User.objects.create_and_join(self.organization, "signal-session-viewer@posthog.com", "password") | ||
| SessionRecording.objects.create(team=self.team, session_id="signal-sess-shared") | ||
|
|
||
| export = ExportedAsset.objects.create( | ||
| team=self.team, | ||
| export_format="video/mp4", | ||
| export_context={"session_recording_id": "signal-sess-shared"}, | ||
| created_by=owner, | ||
| is_system=True, | ||
| content=b"videobytes", | ||
| ) | ||
|
|
||
| self.client.force_login(teammate) | ||
| response = self.client.get(f"/api/projects/{self.team.id}/exports/{export.id}/content/") | ||
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
| self.assertEqual(response.content, b"videobytes") |
There was a problem hiding this comment.
/content/, missing parameterised retrieve case
Both test_teammate_can_access_system_session_video_export_content and test_teammate_blocked_from_system_video_export_when_no_session_access test only the /content/ endpoint. The existing pattern (used in test_teammate_can_access_other_users_export_by_id, test_cannot_access_export_after_losing_resource_access, and test_cannot_access_session_recording_export_after_losing_access) parameterises over both retrieve and content. Because safely_get_object is shared by both actions, the same RBAC path is exercised either way — but an omitted retrieve case means the test suite doesn't catch a regression where, say, the retrieve serializer bypasses the queryset lookup differently.
Consider parameterising these two new tests the same way the neighbouring tests are written.
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/api/test/test_exports.py
Line: 710-729
Comment:
**Tests only cover `/content/`, missing parameterised `retrieve` case**
Both `test_teammate_can_access_system_session_video_export_content` and `test_teammate_blocked_from_system_video_export_when_no_session_access` test only the `/content/` endpoint. The existing pattern (used in `test_teammate_can_access_other_users_export_by_id`, `test_cannot_access_export_after_losing_resource_access`, and `test_cannot_access_session_recording_export_after_losing_access`) parameterises over both `retrieve` and `content`. Because `safely_get_object` is shared by both actions, the same RBAC path is exercised either way — but an omitted `retrieve` case means the test suite doesn't catch a regression where, say, the retrieve serializer bypasses the queryset lookup differently.
Consider parameterising these two new tests the same way the neighbouring tests are written.
How can I resolve this? If you propose a fix, please make it concise.| @@ -400,6 +412,8 @@ def safely_get_queryset(self, queryset): | |||
| if export_format_filter and export_format_filter in ExportedAsset.get_supported_format_values(): | |||
| queryset = queryset.filter(export_format=export_format_filter) | |||
|
|
|||
| return queryset | |||
|
|
|||
| return queryset | |||
There was a problem hiding this comment.
return queryset
The early return queryset inside the if self.action == "list": block already returns for list actions. The trailing bare return queryset is only reached for non-list actions where the queryset is never mutated. Per the project's simplicity rules this is a superfluous part — collapsing to a single trailing return removes the duplication:
| def safely_get_queryset(self, queryset): | |
| """ | |
| List shows only exports you created (quota + history are per user). | |
| Retrieve / content fetch any export in this project by id; safely_get_object still | |
| enforces dashboard, insight, or session recording viewer access when applicable. | |
| """ | |
| if self.action == "list": | |
| queryset = queryset.filter(created_by=self.request.user) | |
| session_recording_filter = self.request.query_params.get("session_recording_id") | |
| if session_recording_filter: | |
| queryset = queryset.filter( | |
| export_context__session_recording_id=session_recording_filter, | |
| ) | |
| context_path_filter = self.request.query_params.get("context_path") | |
| if context_path_filter: | |
| queryset = queryset.filter(export_context__path__icontains=context_path_filter) | |
| # Add export format filter | |
| export_format_filter = self.request.query_params.get("export_format") | |
| if export_format_filter and export_format_filter in ExportedAsset.get_supported_format_values(): | |
| queryset = queryset.filter(export_format=export_format_filter) | |
| return queryset |
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog/api/exports.py
Line: 390-417
Comment:
**Superfluous final `return queryset`**
The early `return queryset` inside the `if self.action == "list":` block already returns for list actions. The trailing bare `return queryset` is only reached for non-list actions where the queryset is never mutated. Per the project's simplicity rules this is a superfluous part — collapsing to a single trailing return removes the duplication:
```suggestion
def safely_get_queryset(self, queryset):
"""
List shows only exports you created (quota + history are per user).
Retrieve / content fetch any export in this project by id; safely_get_object still
enforces dashboard, insight, or session recording viewer access when applicable.
"""
if self.action == "list":
queryset = queryset.filter(created_by=self.request.user)
session_recording_filter = self.request.query_params.get("session_recording_id")
if session_recording_filter:
queryset = queryset.filter(
export_context__session_recording_id=session_recording_filter,
)
context_path_filter = self.request.query_params.get("context_path")
if context_path_filter:
queryset = queryset.filter(export_context__path__icontains=context_path_filter)
# Add export format filter
export_format_filter = self.request.query_params.get("export_format")
if export_format_filter and export_format_filter in ExportedAsset.get_supported_format_values():
queryset = queryset.filter(export_format=export_format_filter)
return queryset
```
How can I resolve this? If you propose a fix, please make it concise.![hex-security-app[bot]](https://avatars.githubusercontent.com/in/2665233?s=60&v=4){kind=small}
|
|
||
| return queryset | ||
|
|
||
| return queryset |
There was a problem hiding this comment.
SessionRecording DB row is absent
The new queryset scoping lets any authenticated project member reach the safely_get_object RBAC gate for every export in the team. That gate has a fail-open path: when export_context.session_recording_id is set but no SessionRecording row exists in Postgres, resource is None and check_access_level_for_object is never called.
ExportedAsset.id is an auto-increment integer (confirmed in migration 0001), so a project member can enumerate all team exports with sequential GETs against /api/projects/{team}/exports/{id}/content/.
For an EE org with a team-wide session_recording resource-level default of "none" (blocking all recordings by default), any project member can retrieve the raw video bytes of a session-recording export whenever the SessionRecording DB row does not yet exist (e.g. the signals pipeline created the export before the recording was materialised in Postgres, or the recording row was deleted after the export was created—there is no FK cascade since the link is a plain JSON string in export_context).
safely_get_object is the only RBAC gate for non-list actions; _filter_queryset_by_access_level explicitly skips non-list actions (routing.py line 185-188) and check_object_permissions passes through because model_to_resource(ExportedAsset) returns None.
Prompt To Fix With AI
In `ExportedAssetViewSet.safely_get_object`, when `session_recording_id` is set in `export_context` but no `SessionRecording` row is found, fall back to the resource-level RBAC default instead of skipping the check:
```python
if session_recording_id:
resource = SessionRecording.objects.filter(
team_id=instance.team_id, session_id=session_recording_id
).first()
if resource is None:
# No DB row — cannot do an object-level check, but still enforce the
# team-wide session_recording resource default (which may be "none").
if not self.user_access_control.check_access_level_for_resource(
"session_recording", required_level="viewer"
):
raise NotFound()
```
Alternatively, fail closed by raising NotFound() when the row is absent, forcing the calling code to ensure the SessionRecording row is created before the export becomes accessible.
Also consider migrating ExportedAsset.id to a UUID primary key to eliminate sequential enumeration of export IDs across project members.
Severity: medium | Confidence: 70%
1 participant
Problem
Session-problem rasterized replay and sharing expected exported assets referenced by UUID to load for anyone with project (team) access, but
/api/projects/{}/exports/detail/queryset filtering prevented teammates from retrieving another user’s export even when RBAC permitted the underlying dashboard/insight/session.The [PostHog Code companion PR](PostHog/code#2011) now loads inbox replay video only via
exported_asset_idinstead of discovering exports via list +session_recording_id. This backend change is expected to merge first.Changes
created_by=request.userexports only (plus existing list filters moved under the list branch):session_recording_id,context_path,export_format./content/: queryset is scoped to team/project, not creator;safely_get_objectstill applies dashboard, insight, and session-recording viewer RBAC.logger.infoblock before emitting session-problem signals (debug-style payload dump).How did you test this code?
Ran local pre-commit hooks on commit (
hogliPython lint/format +uv run ty check). No ClickHouse Postgres test suite run in this agent session (pytest …/test_exports)—reviewer should rely onposthog/api/test/test_exports.pyin CI once available.👉 Stay up-to-date with PostHog coding conventions for smoother review.
Publish to changelog?
no
Docs update
N/A unless we document export sharing semantics externally.
🤖 Agent context
Co-authored tooling: branch cut from
origin/master; companion linked toPostHog/code#2011.