Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: add test for loading session recording snapshots using personal key #21674

Closed

Conversation

marandaneto
Copy link
Member

@marandaneto marandaneto commented Apr 19, 2024

Problem

Relates to https://posthog.com/questions/getting-snapshots-through-api

I'm not sure this API should be public.
The docs don't mention the required scope but all the other endpoints do specify the scope, assuming session_recording:read

Example:

curl \
-H "Authorization: Bearer $POSTHOG_PERSONAL_API_KEY" \
https://app.posthog.com/api/projects/$projectId/session_recordings/$sessionId/snapshots

Throws error:

{"type":"authentication_error","code":"permission_denied","detail":"This action does not support Personal API Key access","attr":null}

The POSTHOG_PERSONAL_API_KEY used is a org/project owner with full scope.

API code path.

Similar to this problem #20983

Changes

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

Does this work well for both Cloud and self-hosted?

How did you test this code?

@marandaneto
Copy link
Member Author

marandaneto commented Apr 19, 2024

@xvello Since Paul/Ben are OOO, do you have any ideas if this API should have been public (accessed via personal key) and its a bug in our permission system, or is it intended to be private/internal anyway?

@pauldambra
Copy link
Member

pauldambra commented Apr 19, 2024

🤔 well, I think this probably should work (as in we should allow it) - but...

I don't think we've done the thinking about what makes it safe (caching/ratelimiting/etags etc).

So, no reason not to allow this - although interested why the customer wants it. But we should follow up with API docs / caching all the good stuff

@marandaneto
Copy link
Member Author

🤔 well, I think this probably should work (as in we should allow it) - but...

I don't think we've done the thinking about what makes it safe (caching/ratelimiting/etags etc).

So, no reason not to allow this - although interested why the customer wants it. But we should follow up with API docs / caching all the good stuff

Gotcha, The API is already documented, the issue is that it does not mention the required scope, this is the first issue.
The second issue is that even with full scope, the API returns 403.
The caching/ratelimiting/etags bits are probably something to follow up on after figuring out the access 😅

@@ -440,6 +442,23 @@ def test_works_with_routes_missing_action(self):
)
assert response.status_code == status.HTTP_200_OK

def test_works_with_session_recording_snapshots(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this test should be here. This is about generally testing the API key functionality. If you just want to add a scope and test that it works then it should be part of the api tests for that endpoint (or just not bother as we know generally that adding the required_scopes property works.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benjackwhite happy to change where the test is placed, but the goal was just to demonstrate the issue/bug.

self.key.scopes = ["session_recording:read"]
self.key.save()
response = self.client.patch(
f"/api/projects/{self.team.id}/session_recordings/{session_id}/snapshots?version=2"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally as to whether this makes sense - sure - "I guess so". The reason we were cagey about it before is this API has changed a lot so we don't want to make it public unless we are really happy to support the format basically forever.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so this API should not be public hence even full token permission does not work?
What I am looking after is: hey this should work but we have a permission bug or hey this is not a public API (I can fix the docs if that's the case) and follow up asking their use case to call the API directly.

@marandaneto
Copy link
Member Author

This API wasn't meant to be public for now hence adding the test won't make sense.

@marandaneto
Copy link
Member Author

issue tracking this request #22520

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants