From 640ce68048a2753ec8611e5d6e605096a631afdc Mon Sep 17 00:00:00 2001 From: MichaelSun48 Date: Fri, 4 Apr 2025 12:50:42 -0700 Subject: [PATCH] Refactor delete endpoint permissions and starred/lastvisited deltion logic --- .../organization_group_search_view_details.py | 39 ++--- src/sentry/models/groupsearchviewstarred.py | 4 + ..._organization_group_search_view_details.py | 134 +++++++++++++++++- 3 files changed, 157 insertions(+), 20 deletions(-) diff --git a/src/sentry/issues/endpoints/organization_group_search_view_details.py b/src/sentry/issues/endpoints/organization_group_search_view_details.py index bd697c4269290a..e6ecd9b3e033ae 100644 --- a/src/sentry/issues/endpoints/organization_group_search_view_details.py +++ b/src/sentry/issues/endpoints/organization_group_search_view_details.py @@ -1,6 +1,5 @@ from typing import Any, NotRequired, TypedDict -from django.db.models import F from rest_framework import status from rest_framework.request import Request from rest_framework.response import Response @@ -15,6 +14,7 @@ from sentry.api.serializers.rest_framework.groupsearchview import GroupSearchViewDetailsPutValidator from sentry.issues.endpoints.organization_group_search_views import pick_default_project from sentry.models.groupsearchview import GroupSearchView +from sentry.models.groupsearchviewlastvisited import GroupSearchViewLastVisited from sentry.models.groupsearchviewstarred import GroupSearchViewStarred from sentry.models.organization import Organization from sentry.models.savedsearch import SORT_LITERALS @@ -111,30 +111,33 @@ def delete(self, request: Request, organization: Organization, view_id: str) -> return Response(status=status.HTTP_404_NOT_FOUND) try: - view = GroupSearchView.objects.get( - id=view_id, organization=organization, user_id=request.user.id - ) + view = GroupSearchView.objects.get(id=view_id) except GroupSearchView.DoesNotExist: return Response(status=status.HTTP_404_NOT_FOUND) - # Check if the view is starred by the current user + # Only view creators and org admins can delete views + has_delete_access = ( + view.user_id == request.user.id + or request.access.has_scope("org:admin") + or request.access.has_scope("team:admin") + ) + if not has_delete_access: + return Response(status=status.HTTP_403_FORBIDDEN) + try: - starredView = GroupSearchViewStarred.objects.get( - organization=organization, user_id=request.user.id, group_search_view=view + GroupSearchViewStarred.objects.clear_starred_view_for_all_members( + organization=organization, view=view ) - deleted_position = starredView.position - # Delete the starred view - starredView.delete() - - # Decrement the position of all other starred views with higher position - GroupSearchViewStarred.objects.filter( - organization=organization, - user_id=request.user.id, - position__gt=deleted_position, - ).update(position=F("position") - 1) - except GroupSearchViewStarred.DoesNotExist: pass + try: + GroupSearchViewLastVisited.objects.filter( + organization=organization, group_search_view=view + ).delete() + except GroupSearchViewLastVisited.DoesNotExist: + pass + view.delete() + return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/src/sentry/models/groupsearchviewstarred.py b/src/sentry/models/groupsearchviewstarred.py index 5f57956460de8b..cd3ed5494e5935 100644 --- a/src/sentry/models/groupsearchviewstarred.py +++ b/src/sentry/models/groupsearchviewstarred.py @@ -142,6 +142,10 @@ def delete_starred_view( ).update(position=models.F("position") - 1) return True + def clear_starred_view_for_all_members(self, organization: Organization, view: GroupSearchView): + for starred_view in self.filter(organization=organization, group_search_view=view): + self.delete_starred_view(organization, starred_view.user_id, view) + @region_silo_model class GroupSearchViewStarred(DefaultFieldsModel): diff --git a/tests/sentry/issues/endpoints/test_organization_group_search_view_details.py b/tests/sentry/issues/endpoints/test_organization_group_search_view_details.py index 46d93c2b294fff..8a8897b506b769 100644 --- a/tests/sentry/issues/endpoints/test_organization_group_search_view_details.py +++ b/tests/sentry/issues/endpoints/test_organization_group_search_view_details.py @@ -1,8 +1,11 @@ from django.urls import reverse +from django.utils import timezone from sentry.models.groupsearchview import GroupSearchView +from sentry.models.groupsearchviewlastvisited import GroupSearchViewLastVisited from sentry.models.groupsearchviewstarred import GroupSearchViewStarred from sentry.silo.base import SiloMode +from sentry.testutils.cases import APITestCase from sentry.testutils.helpers import with_feature from sentry.testutils.silo import assume_test_silo_mode from tests.sentry.issues.endpoints.test_organization_group_search_views import BaseGSVTestCase @@ -54,18 +57,39 @@ def test_delete_nonexistent_view(self) -> None: @with_feature({"organizations:issue-stream-custom-views": True}) def test_delete_view_from_another_user(self) -> None: # Get a view ID from user_two - view_id = str(self.base_data["user_two_views"][0].id) + self.login_as(user=self.user_2) + view_id = str(self.base_data["user_one_views"][0].id) url = reverse( "sentry-api-0-organization-group-search-view-details", kwargs={"organization_id_or_slug": self.organization.slug, "view_id": view_id}, ) response = self.client.delete(url) - assert response.status_code == 404 + assert response.status_code == 403 # Verify the view still exists (this will error out if not) GroupSearchView.objects.get(id=view_id) + @with_feature({"organizations:issue-stream-custom-views": True}) + def test_admin_can_delete_view_from_another_user(self) -> None: + self.admin_user = self.create_user() + self.create_member( + user=self.admin_user, + organization=self.organization, + role="admin", + ) + self.login_as(user=self.admin_user) + view_id = str(self.base_data["user_one_views"][0].id) + url = reverse( + "sentry-api-0-organization-group-search-view-details", + kwargs={"organization_id_or_slug": self.organization.slug, "view_id": view_id}, + ) + + response = self.client.delete(url) + assert response.status_code == 204 + + assert not GroupSearchView.objects.filter(id=view_id).exists() + @with_feature({"organizations:issue-stream-custom-views": True}) def test_delete_first_starred_view_decrements_succeeding_positions(self) -> None: # Delete the first view @@ -123,6 +147,112 @@ def test_delete_without_feature_flag(self) -> None: GroupSearchView.objects.get(id=self.view_id) +class OrganizationGroupSearchViewsDeleteStarredAndLastVisitedTest(APITestCase): + endpoint = "sentry-api-0-organization-group-search-view-details" + + def setUp(self) -> None: + super().setUp() + + self.user_1 = self.user + self.user_2 = self.create_user() + self.create_member(organization=self.organization, user=self.user_2) + + self.user_1_view = GroupSearchView.objects.create( + organization=self.organization, + user_id=self.user_1.id, + name="User 1's View", + query="is:unresolved", + ) + GroupSearchViewStarred.objects.create( + organization=self.organization, + user_id=self.user_1.id, + group_search_view=self.user_1_view, + position=0, + ) + GroupSearchViewLastVisited.objects.create( + organization=self.organization, + user_id=self.user_1.id, + group_search_view=self.user_1_view, + last_visited=timezone.now(), + ) + + self.user_2_view = GroupSearchView.objects.create( + organization=self.organization, + user_id=self.user_2.id, + name="User 2's View", + query="is:unresolved", + ) + + GroupSearchViewStarred.objects.create( + organization=self.organization, + user_id=self.user_2.id, + group_search_view=self.user_1_view, + position=0, + ) + GroupSearchViewStarred.objects.create( + organization=self.organization, + user_id=self.user_2.id, + group_search_view=self.user_2_view, + position=1, + ) + + GroupSearchViewLastVisited.objects.create( + organization=self.organization, + user_id=self.user_2.id, + group_search_view=self.user_1_view, + last_visited=timezone.now(), + ) + + @with_feature({"organizations:issue-stream-custom-views": True}) + def test_cannot_delete_other_users_view(self) -> None: + self.login_as(user=self.user_2) + + response = self.client.delete( + reverse( + self.endpoint, + kwargs={ + "organization_id_or_slug": self.organization.slug, + "view_id": self.user_1_view.id, + }, + ) + ) + + assert response.status_code == 403 + + @with_feature({"organizations:issue-stream-custom-views": True}) + def test_deleting_my_view_deletes_from_others_starred_views(self) -> None: + self.login_as(user=self.user_1) + + response = self.client.delete( + reverse( + self.endpoint, + kwargs={ + "organization_id_or_slug": self.organization.slug, + "view_id": self.user_1_view.id, + }, + ) + ) + + assert response.status_code == 204 + # User 2 starred User 1's view. After User 1 deletes the view, it should not be starred for anyone anymore + assert not GroupSearchViewStarred.objects.filter( + organization_id=self.organization.id, group_search_view=self.user_1_view + ).exists() + # User 2's other starred view should be moved up to position 0 + assert ( + GroupSearchViewStarred.objects.get( + organization_id=self.organization.id, + user_id=self.user_2.id, + group_search_view=self.user_2_view, + ).position + == 0 + ) + # All last visited entries should be deleted as well + assert not GroupSearchViewLastVisited.objects.filter( + organization_id=self.organization.id, group_search_view=self.user_1_view + ).exists() + + class OrganizationGroupSearchViewsPutTest(BaseGSVTestCase): endpoint = "sentry-api-0-organization-group-search-view-details" method = "put"