chore(staff): Let staff access user permission endpoints (#64440)

Let staff access endpoints to manage user permissions

Requires https://github.com/getsentry/sentry/pull/64429

src/sentry/api/endpoints/user_permission_details.py

@@ -10,7 +10,7 @@ from sentry.api.api_publish_status import ApiPublishStatus
 from sentry.api.base import control_silo_endpoint
 from sentry.api.bases.user import UserEndpoint
 from sentry.api.decorators import sudo_required
-from sentry.api.permissions import SuperuserPermission
+from sentry.api.permissions import SuperuserOrStaffFeatureFlaggedPermission
 from sentry.models.userpermission import UserPermission
 
 audit_logger = logging.getLogger("sentry.audit.user")
@@ -24,7 +24,7 @@ class UserPermissionDetailsEndpoint(UserEndpoint):
         "POST": ApiPublishStatus.PRIVATE,
     }
     owner = ApiOwner.ENTERPRISE
-    permission_classes = (SuperuserPermission,)
+    permission_classes = (SuperuserOrStaffFeatureFlaggedPermission,)
 
     def get(self, request: Request, user, permission_name) -> Response:
         # XXX(dcramer): we may decide to relax "view" permission over time, but being more restrictive by default

tests/sentry/api/endpoints/test_user_permission_details.py

@@ -1,5 +1,9 @@
+from unittest.mock import patch
+
+from sentry.api.permissions import StaffPermission
 from sentry.models.userpermission import UserPermission
 from sentry.testutils.cases import APITestCase
+from sentry.testutils.helpers import with_feature
 from sentry.testutils.silo import control_silo_test
 
 
@@ -9,68 +13,149 @@ class UserDetailsTest(APITestCase):
 
     def setUp(self):
         super().setUp()
-        self.user = self.create_user(is_superuser=True)
-        self.login_as(user=self.user, superuser=True)
-        self.add_user_permission(self.user, "users.admin")
+        self.superuser = self.create_user(is_superuser=True)
+        self.add_user_permission(self.superuser, "users.admin")
 
-    def test_fails_without_superuser(self):
-        self.user = self.create_user(is_superuser=False)
-        self.login_as(self.user)
+        self.staff_user = self.create_user(is_staff=True)
+        self.add_user_permission(self.staff_user, "users.admin")
 
-        resp = self.get_response("me", "broadcasts.admin")
-        assert resp.status_code == 403
+        self.normal_user = self.create_user(is_superuser=False, is_staff=False)
 
-        self.user.update(is_superuser=True)
-        resp = self.get_response("me", "broadcasts.admin")
-        assert resp.status_code == 403
+    # For each request method testcase, ensure regular users fail
+    def test_fails_without_superuser_or_staff(self):
+        self.login_as(self.normal_user)
+        response = self.get_response("me", "broadcasts.admin")
+        assert response.status_code == 403
 
+    # For each request method testcase, ensure superuser+staff without users.admin fail
     def test_fails_without_users_admin_permission(self):
-        self.user = self.create_user(is_superuser=True)
-        self.login_as(self.user, superuser=True)
-        resp = self.get_response("me", "broadcasts.admin")
-        assert resp.status_code == 403
+        self.superuser_and_staff = self.create_user(is_superuser=True, is_staff=True)
+        self.login_as(self.superuser_and_staff, superuser=True, staff=True)
+
+        # We are active superuser and staff but lack the users.admin permission
+        response = self.get_response("me", "broadcasts.admin", status_code=403)
+        assert response.status_code == 403
 
 
 class UserPermissionDetailsGetTest(UserDetailsTest):
-    def test_with_permission(self):
-        UserPermission.objects.create(user=self.user, permission="broadcasts.admin")
-        resp = self.get_response("me", "broadcasts.admin")
-        assert resp.status_code == 204
+    method = "GET"
+
+    def test_superuser_with_permission(self):
+        self.login_as(self.superuser, superuser=True)
+        self.add_user_permission(self.superuser, "broadcasts.admin")
+        self.get_success_response("me", "broadcasts.admin", status_code=204)
+
+    def test_superuser_without_permission(self):
+        self.login_as(self.superuser, superuser=True)
+        self.get_error_response("me", "broadcasts.admin", status_code=404)
+
+    @with_feature("auth:enterprise-staff-cookie")
+    @patch.object(StaffPermission, "has_permission", wraps=StaffPermission().has_permission)
+    def test_staff_with_permission(self, mock_has_permission):
+        self.login_as(self.staff_user, staff=True)
+        self.add_user_permission(self.staff_user, "broadcasts.admin")
+
+        self.get_success_response("me", "broadcasts.admin", status_code=204)
+        # ensure we fail the scope check and call is_active_staff
+        assert mock_has_permission.call_count == 1
+
+    @with_feature("auth:enterprise-staff-cookie")
+    @patch.object(StaffPermission, "has_permission", wraps=StaffPermission().has_permission)
+    def test_staff_without_permission(self, mock_has_permission):
+        self.login_as(self.staff_user, staff=True)
 
-    def test_without_permission(self):
-        resp = self.get_response("me", "broadcasts.admin")
-        assert resp.status_code == 404
+        self.get_error_response("me", "broadcasts.admin", status_code=404)
+        # ensure we fail the scope check and call is_active_staff
+        assert mock_has_permission.call_count == 1
 
 
 class UserPermissionDetailsPostTest(UserDetailsTest):
     method = "POST"
 
-    def test_with_permission(self):
-        UserPermission.objects.create(user=self.user, permission="broadcasts.admin")
-        resp = self.get_response("me", "broadcasts.admin")
-        assert resp.status_code == 410
-        assert UserPermission.objects.filter(user=self.user, permission="broadcasts.admin").exists()
+    def test_superuser_with_permission(self):
+        self.login_as(self.superuser, superuser=True)
 
-    def test_without_permission(self):
-        resp = self.get_response("me", "broadcasts.admin")
-        assert resp.status_code == 201
-        assert UserPermission.objects.filter(user=self.user, permission="broadcasts.admin").exists()
+        self.get_success_response("me", "broadcasts.admin", status_code=201)
+        assert UserPermission.objects.filter(
+            user=self.superuser, permission="broadcasts.admin"
+        ).exists()
+
+    def test_superuser_duplicate_permission(self):
+        self.login_as(self.superuser, superuser=True)
+        self.add_user_permission(self.superuser, "broadcasts.admin")
+
+        self.get_error_response("me", "broadcasts.admin", status_code=410)
+        assert UserPermission.objects.filter(
+            user=self.superuser, permission="broadcasts.admin"
+        ).exists()
+
+    @with_feature("auth:enterprise-staff-cookie")
+    @patch.object(StaffPermission, "has_permission", wraps=StaffPermission().has_permission)
+    def test_staff_with_permission(self, mock_has_permission):
+        self.login_as(self.staff_user, staff=True)
+
+        self.get_success_response("me", "broadcasts.admin", status_code=201)
+        assert UserPermission.objects.filter(
+            user=self.staff_user, permission="broadcasts.admin"
+        ).exists()
+        # ensure we fail the scope check and call is_active_staff
+        assert mock_has_permission.call_count == 1
+
+    @with_feature("auth:enterprise-staff-cookie")
+    @patch.object(StaffPermission, "has_permission", wraps=StaffPermission().has_permission)
+    def test_staff_duplicate_permission(self, mock_has_permission):
+        self.login_as(self.staff_user, staff=True)
+        self.add_user_permission(self.staff_user, "broadcasts.admin")
+
+        self.get_error_response("me", "broadcasts.admin", status_code=410)
+        assert UserPermission.objects.filter(
+            user=self.staff_user, permission="broadcasts.admin"
+        ).exists()
+        # ensure we fail the scope check and call is_active_staff
+        assert mock_has_permission.call_count == 1
 
 
 class UserPermissionDetailsDeleteTest(UserDetailsTest):
     method = "DELETE"
 
-    def test_with_permission(self):
-        UserPermission.objects.create(user=self.user, permission="broadcasts.admin")
-        resp = self.get_response("me", "broadcasts.admin")
-        assert resp.status_code == 204
+    def test_superuser_with_permission(self):
+        self.login_as(self.superuser, superuser=True)
+        self.add_user_permission(self.superuser, "broadcasts.admin")
+
+        self.get_success_response("me", "broadcasts.admin", status_code=204)
+        assert not UserPermission.objects.filter(
+            user=self.superuser, permission="broadcasts.admin"
+        ).exists()
+
+    def test_superuser_without_permission(self):
+        self.login_as(self.superuser, superuser=True)
+
+        self.get_error_response("me", "broadcasts.admin", status_code=404)
+        assert not UserPermission.objects.filter(
+            user=self.superuser, permission="broadcasts.admin"
+        ).exists()
+
+    @with_feature("auth:enterprise-staff-cookie")
+    @patch.object(StaffPermission, "has_permission", wraps=StaffPermission().has_permission)
+    def test_staff_with_permission(self, mock_has_permission):
+        self.login_as(self.staff_user, staff=True)
+        self.add_user_permission(self.staff_user, "broadcasts.admin")
+
+        self.get_success_response("me", "broadcasts.admin", status_code=204)
         assert not UserPermission.objects.filter(
-            user=self.user, permission="broadcasts.admin"
+            user=self.staff_user, permission="broadcasts.admin"
         ).exists()
+        # ensure we fail the scope check and call is_active_staff
+        assert mock_has_permission.call_count == 1
+
+    @with_feature("auth:enterprise-staff-cookie")
+    @patch.object(StaffPermission, "has_permission", wraps=StaffPermission().has_permission)
+    def test_staff_without_permission(self, mock_has_permission):
+        self.login_as(self.staff_user, staff=True)
 
-    def test_without_permission(self):
-        resp = self.get_response("me", "broadcasts.admin")
-        assert resp.status_code == 404
+        self.get_error_response("me", "broadcasts.admin", status_code=404)
         assert not UserPermission.objects.filter(
-            user=self.user, permission="broadcasts.admin"
+            user=self.staff_user, permission="broadcasts.admin"
         ).exists()
+        # ensure we fail the scope check and call is_active_staff
+        assert mock_has_permission.call_count == 1