From dbf533c384d33179810a41f4c781cf328a93c5be Mon Sep 17 00:00:00 2001 From: Ben White Date: Mon, 8 Apr 2024 13:44:23 +0200 Subject: [PATCH] feat: Allow autoswapping projects via token (#21214) --- posthog/middleware.py | 43 +++- .../test_session_recordings.ambr | 188 ++++++++++++++++++ posthog/test/test_middleware.py | 15 ++ 3 files changed, 236 insertions(+), 10 deletions(-) diff --git a/posthog/middleware.py b/posthog/middleware.py index 30b2e158f8d..0d7de559977 100644 --- a/posthog/middleware.py +++ b/posthog/middleware.py @@ -2,6 +2,7 @@ import time from ipaddress import ip_address, ip_network from typing import Any, Callable, List, Optional, cast +from django.shortcuts import redirect import structlog from corsheaders.middleware import CorsMiddleware from django.conf import settings @@ -155,8 +156,26 @@ class AutoProjectMiddleware: if request.user.is_authenticated: path_parts = request.path.strip("/").split("/") project_id_in_url = None + user = cast(User, request.user) + + if len(path_parts) >= 2 and path_parts[0] == "project" and path_parts[1].startswith("phc_"): + try: + new_team = Team.objects.get(api_token=path_parts[1]) + + if not self.can_switch_to_team(new_team, request): + raise Team.DoesNotExist + + path_parts[1] = str(new_team.pk) + return redirect("/" + "/".join(path_parts)) + + except Team.DoesNotExist: + if user.team: + path_parts[1] = str(user.team.pk) + return redirect("/" + "/".join(path_parts)) + if len(path_parts) >= 2 and path_parts[0] == "project" and path_parts[1].isdigit(): project_id_in_url = int(path_parts[1]) + elif ( len(path_parts) >= 3 and path_parts[0] == "api" @@ -165,11 +184,7 @@ class AutoProjectMiddleware: ): project_id_in_url = int(path_parts[2]) - if ( - project_id_in_url is not None - and request.user.team is not None - and request.user.team.pk != project_id_in_url - ): + if project_id_in_url and user.team and user.team.pk != project_id_in_url: try: new_team = Team.objects.get(pk=project_id_in_url) self.switch_team_if_allowed(new_team, request) @@ -220,11 +235,8 @@ class AutoProjectMiddleware: def switch_team_if_allowed(self, new_team: Team, request: HttpRequest): user = cast(User, request.user) - user_permissions = UserPermissions(user) - # :KLUDGE: This is more inefficient than needed, doing several expensive lookups - # However this should be a rare operation! - if user_permissions.team(new_team).effective_membership_level is None: - # Do something to indicate that they don't have access to the team... + + if not self.can_switch_to_team(new_team, request): return old_team_id = user.current_team_id @@ -235,6 +247,17 @@ class AutoProjectMiddleware: # Information for POSTHOG_APP_CONTEXT request.switched_team = old_team_id # type: ignore + def can_switch_to_team(self, new_team: Team, request: HttpRequest): + user = cast(User, request.user) + user_permissions = UserPermissions(user) + # :KLUDGE: This is more inefficient than needed, doing several expensive lookups + # However this should be a rare operation! + if user_permissions.team(new_team).effective_membership_level is None: + # Do something to indicate that they don't have access to the team... + return False + + return True + class CHQueries: def __init__(self, get_response): diff --git a/posthog/session_recordings/test/__snapshots__/test_session_recordings.ambr b/posthog/session_recordings/test/__snapshots__/test_session_recordings.ambr index c2982e4f852..58e44aaccd4 100644 --- a/posthog/session_recordings/test/__snapshots__/test_session_recordings.ambr +++ b/posthog/session_recordings/test/__snapshots__/test_session_recordings.ambr @@ -782,6 +782,30 @@ AND "posthog_persondistinctid"."team_id" = 2) /*controller='project_session_recordings-list',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/session_recordings/%3F%24'*/ ''' # --- +# name: TestSessionRecordings.test_get_session_recordings.36 + ''' + SELECT "posthog_persondistinctid"."id", + "posthog_persondistinctid"."team_id", + "posthog_persondistinctid"."person_id", + "posthog_persondistinctid"."distinct_id", + "posthog_persondistinctid"."version", + "posthog_person"."id", + "posthog_person"."created_at", + "posthog_person"."properties_last_updated_at", + "posthog_person"."properties_last_operation", + "posthog_person"."team_id", + "posthog_person"."properties", + "posthog_person"."is_user_id", + "posthog_person"."is_identified", + "posthog_person"."uuid", + "posthog_person"."version" + FROM "posthog_persondistinctid" + INNER JOIN "posthog_person" ON ("posthog_persondistinctid"."person_id" = "posthog_person"."id") + WHERE ("posthog_persondistinctid"."distinct_id" IN ('user2', + 'user_one_0') + AND "posthog_persondistinctid"."team_id" = 2) /*controller='project_session_recordings-list',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/session_recordings/%3F%24'*/ + ''' +# --- # name: TestSessionRecordings.test_get_session_recordings.4 ''' SELECT "posthog_team"."id", @@ -4275,6 +4299,130 @@ AND "posthog_persondistinctid"."team_id" = 2) /*controller='project_session_recordings-list',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/session_recordings/%3F%24'*/ ''' # --- +# name: TestSessionRecordings.test_listing_recordings_is_not_nplus1_for_persons.251 + ''' + SELECT "posthog_instancesetting"."id", + "posthog_instancesetting"."key", + "posthog_instancesetting"."raw_value" + FROM "posthog_instancesetting" + WHERE "posthog_instancesetting"."key" = 'constance:posthog:AGGREGATE_BY_DISTINCT_IDS_TEAMS' + ORDER BY "posthog_instancesetting"."id" ASC + LIMIT 1 /*controller='project_session_recordings-list',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/session_recordings/%3F%24'*/ + ''' +# --- +# name: TestSessionRecordings.test_listing_recordings_is_not_nplus1_for_persons.252 + ''' + SELECT "posthog_instancesetting"."id", + "posthog_instancesetting"."key", + "posthog_instancesetting"."raw_value" + FROM "posthog_instancesetting" + WHERE "posthog_instancesetting"."key" = 'constance:posthog:RECORDINGS_TTL_WEEKS' + ORDER BY "posthog_instancesetting"."id" ASC + LIMIT 1 /*controller='project_session_recordings-list',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/session_recordings/%3F%24'*/ + ''' +# --- +# name: TestSessionRecordings.test_listing_recordings_is_not_nplus1_for_persons.253 + ''' + SELECT "posthog_instancesetting"."id", + "posthog_instancesetting"."key", + "posthog_instancesetting"."raw_value" + FROM "posthog_instancesetting" + WHERE "posthog_instancesetting"."key" = 'constance:posthog:PERSON_ON_EVENTS_V2_ENABLED' + ORDER BY "posthog_instancesetting"."id" ASC + LIMIT 1 /*controller='project_session_recordings-list',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/session_recordings/%3F%24'*/ + ''' +# --- +# name: TestSessionRecordings.test_listing_recordings_is_not_nplus1_for_persons.254 + ''' + SELECT "posthog_instancesetting"."id", + "posthog_instancesetting"."key", + "posthog_instancesetting"."raw_value" + FROM "posthog_instancesetting" + WHERE "posthog_instancesetting"."key" = 'constance:posthog:PERSON_ON_EVENTS_ENABLED' + ORDER BY "posthog_instancesetting"."id" ASC + LIMIT 1 /*controller='project_session_recordings-list',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/session_recordings/%3F%24'*/ + ''' +# --- +# name: TestSessionRecordings.test_listing_recordings_is_not_nplus1_for_persons.255 + ''' + SELECT "posthog_instancesetting"."id", + "posthog_instancesetting"."key", + "posthog_instancesetting"."raw_value" + FROM "posthog_instancesetting" + WHERE "posthog_instancesetting"."key" = 'constance:posthog:AGGREGATE_BY_DISTINCT_IDS_TEAMS' + ORDER BY "posthog_instancesetting"."id" ASC + LIMIT 1 /*controller='project_session_recordings-list',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/session_recordings/%3F%24'*/ + ''' +# --- +# name: TestSessionRecordings.test_listing_recordings_is_not_nplus1_for_persons.256 + ''' + SELECT "posthog_instancesetting"."id", + "posthog_instancesetting"."key", + "posthog_instancesetting"."raw_value" + FROM "posthog_instancesetting" + WHERE "posthog_instancesetting"."key" = 'constance:posthog:PERSON_ON_EVENTS_V2_ENABLED' + ORDER BY "posthog_instancesetting"."id" ASC + LIMIT 1 /*controller='project_session_recordings-list',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/session_recordings/%3F%24'*/ + ''' +# --- +# name: TestSessionRecordings.test_listing_recordings_is_not_nplus1_for_persons.257 + ''' + SELECT "posthog_instancesetting"."id", + "posthog_instancesetting"."key", + "posthog_instancesetting"."raw_value" + FROM "posthog_instancesetting" + WHERE "posthog_instancesetting"."key" = 'constance:posthog:PERSON_ON_EVENTS_ENABLED' + ORDER BY "posthog_instancesetting"."id" ASC + LIMIT 1 /*controller='project_session_recordings-list',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/session_recordings/%3F%24'*/ + ''' +# --- +# name: TestSessionRecordings.test_listing_recordings_is_not_nplus1_for_persons.258 + ''' + SELECT "posthog_instancesetting"."id", + "posthog_instancesetting"."key", + "posthog_instancesetting"."raw_value" + FROM "posthog_instancesetting" + WHERE "posthog_instancesetting"."key" = 'constance:posthog:AGGREGATE_BY_DISTINCT_IDS_TEAMS' + ORDER BY "posthog_instancesetting"."id" ASC + LIMIT 1 /*controller='project_session_recordings-list',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/session_recordings/%3F%24'*/ + ''' +# --- +# name: TestSessionRecordings.test_listing_recordings_is_not_nplus1_for_persons.259 + ''' + SELECT "posthog_sessionrecording"."id", + "posthog_sessionrecording"."session_id", + "posthog_sessionrecording"."team_id", + "posthog_sessionrecording"."created_at", + "posthog_sessionrecording"."deleted", + "posthog_sessionrecording"."object_storage_path", + "posthog_sessionrecording"."distinct_id", + "posthog_sessionrecording"."duration", + "posthog_sessionrecording"."active_seconds", + "posthog_sessionrecording"."inactive_seconds", + "posthog_sessionrecording"."start_time", + "posthog_sessionrecording"."end_time", + "posthog_sessionrecording"."click_count", + "posthog_sessionrecording"."keypress_count", + "posthog_sessionrecording"."mouse_activity_count", + "posthog_sessionrecording"."console_log_count", + "posthog_sessionrecording"."console_warn_count", + "posthog_sessionrecording"."console_error_count", + "posthog_sessionrecording"."start_url", + "posthog_sessionrecording"."storage_version" + FROM "posthog_sessionrecording" + WHERE ("posthog_sessionrecording"."session_id" IN ('1', + '10', + '2', + '3', + '4', + '5', + '6', + '7', + '8', + '9') + AND "posthog_sessionrecording"."team_id" = 2) /*controller='project_session_recordings-list',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/session_recordings/%3F%24'*/ + ''' +# --- # name: TestSessionRecordings.test_listing_recordings_is_not_nplus1_for_persons.26 ''' SELECT "posthog_instancesetting"."id", @@ -4286,6 +4434,46 @@ LIMIT 1 /*controller='project_session_recordings-list',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/session_recordings/%3F%24'*/ ''' # --- +# name: TestSessionRecordings.test_listing_recordings_is_not_nplus1_for_persons.260 + ''' + SELECT "posthog_sessionrecordingviewed"."session_id" + FROM "posthog_sessionrecordingviewed" + WHERE ("posthog_sessionrecordingviewed"."team_id" = 2 + AND "posthog_sessionrecordingviewed"."user_id" = 2) /*controller='project_session_recordings-list',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/session_recordings/%3F%24'*/ + ''' +# --- +# name: TestSessionRecordings.test_listing_recordings_is_not_nplus1_for_persons.261 + ''' + SELECT "posthog_persondistinctid"."id", + "posthog_persondistinctid"."team_id", + "posthog_persondistinctid"."person_id", + "posthog_persondistinctid"."distinct_id", + "posthog_persondistinctid"."version", + "posthog_person"."id", + "posthog_person"."created_at", + "posthog_person"."properties_last_updated_at", + "posthog_person"."properties_last_operation", + "posthog_person"."team_id", + "posthog_person"."properties", + "posthog_person"."is_user_id", + "posthog_person"."is_identified", + "posthog_person"."uuid", + "posthog_person"."version" + FROM "posthog_persondistinctid" + INNER JOIN "posthog_person" ON ("posthog_persondistinctid"."person_id" = "posthog_person"."id") + WHERE ("posthog_persondistinctid"."distinct_id" IN ('user1', + 'user10', + 'user2', + 'user3', + 'user4', + 'user5', + 'user6', + 'user7', + 'user8', + 'user9') + AND "posthog_persondistinctid"."team_id" = 2) /*controller='project_session_recordings-list',route='api/projects/%28%3FP%3Cparent_lookup_team_id%3E%5B%5E/.%5D%2B%29/session_recordings/%3F%24'*/ + ''' +# --- # name: TestSessionRecordings.test_listing_recordings_is_not_nplus1_for_persons.27 ''' SELECT "posthog_instancesetting"."id", diff --git a/posthog/test/test_middleware.py b/posthog/test/test_middleware.py index c2cbf5c0729..e0f5283dd3c 100644 --- a/posthog/test/test_middleware.py +++ b/posthog/test/test_middleware.py @@ -310,6 +310,21 @@ class TestAutoProjectMiddleware(APIBaseTest): assert project_2_request.status_code == 200 assert response_users_api.json().get("team", {}).get("id") == self.team.id + def test_project_redirects_to_new_team_when_accessing_project_by_token(self): + res = self.client.get(f"/project/{self.second_team.api_token}/home") + assert res.status_code == 302 + assert res.headers["Location"] == f"/project/{self.second_team.pk}/home" + + def test_project_redirects_to_current_team_when_accessing_missing_project_by_token(self): + res = self.client.get(f"/project/phc_123/home") + assert res.status_code == 302 + assert res.headers["Location"] == f"/project/{self.team.pk}/home" + + def test_project_redirects_to_current_team_when_accessing_inaccessible_project_by_token(self): + res = self.client.get(f"/project/{self.no_access_team.api_token}/home") + assert res.status_code == 302 + assert res.headers["Location"] == f"/project/{self.team.pk}/home" + @override_settings(CLOUD_DEPLOYMENT="US") # As PostHog Cloud class TestPostHogTokenCookieMiddleware(APIBaseTest):