feat: Restrict superuser attributes to permission

src/sentry/api/endpoints/user_details.py

@@ -94,7 +94,7 @@ def validate(self, attrs):
         return super().validate(attrs)
 
 
-class SuperuserUserSerializer(BaseUserSerializer):
+class PrivilegedUserSerializer(BaseUserSerializer):
     isActive = serializers.BooleanField(source="is_active")
     isStaff = serializers.BooleanField(source="is_staff")
     isSuperuser = serializers.BooleanField(source="is_superuser")
@@ -140,8 +140,8 @@ def put(self, request, user):
         :auth: required
         """
 
-        if is_active_superuser(request):
-            serializer_cls = SuperuserUserSerializer
+        if is_active_superuser(request) and request.access.has_permission("users.admin"):
+            serializer_cls = PrivilegedUserSerializer
         else:
             serializer_cls = UserSerializer
         serializer = serializer_cls(user, data=request.data, partial=True)

src/sentry/conf/server.py

@@ -2175,7 +2175,7 @@ def build_cdc_postgres_init_db_volume(settings):
 
 # This is customizable for sentry.io, but generally should only be additive
 # (currently the values not used anymore so this is more for documentation purposes)
-SENTRY_USER_PERMISSIONS = ("broadcasts.admin",)
+SENTRY_USER_PERMISSIONS = ("broadcasts.admin", "users.admin")
 
 KAFKA_CLUSTERS = {
     "default": {

tests/sentry/api/endpoints/test_user_details.py

@@ -1,4 +1,4 @@
-from sentry.models import Organization, OrganizationStatus, User, UserOption
+from sentry.models import Organization, OrganizationStatus, User, UserOption, UserPermission
 from sentry.testutils import APITestCase
 
 
@@ -82,26 +82,6 @@ def test_saving_changes_value(self):
 
         assert UserOption.objects.get_value(user=self.user, key="language") == "en"
 
-    def test_superuser(self):
-        # superuser should be able to change self.user's name
-        superuser = self.create_user(email="b@example.com", is_superuser=True)
-        self.login_as(user=superuser, superuser=True)
-
-        resp = self.get_valid_response(
-            self.user.id,
-            name="hello world",
-            email="c@example.com",
-            isActive="false",
-        )
-        assert resp.data["id"] == str(self.user.id)
-
-        user = User.objects.get(id=self.user.id)
-        assert user.name == "hello world"
-        # note: email should not change, removed support for email changing from this endpoint
-        assert user.email == "a@example.com"
-        assert user.username == "a@example.com"
-        assert not user.is_active
-
     def test_managed_fields(self):
         assert self.user.name == "example name"
         with self.settings(SENTRY_MANAGED_USER_FIELDS=("name",)):
@@ -137,6 +117,92 @@ def test_change_username_when_same(self):
         assert user.username == "new@example.com"
 
 
+class UserDetailsSuperuserUpdateTest(UserDetailsTest):
+    method = "put"
+
+    def test_superuser_cannot_change_is_active(self):
+        superuser = self.create_user(email="b@example.com", is_superuser=True)
+        self.login_as(user=superuser, superuser=True)
+
+        resp = self.get_valid_response(
+            self.user.id,
+            isActive="false",
+        )
+        assert resp.data["id"] == str(self.user.id)
+
+        user = User.objects.get(id=self.user.id)
+        assert user.is_active
+
+    def test_superuser_with_permission_can_change_is_active(self):
+        superuser = self.create_user(email="b@example.com", is_superuser=True)
+        UserPermission.objects.create(user=superuser, permission="users.admin")
+        self.login_as(user=superuser, superuser=True)
+
+        resp = self.get_valid_response(
+            self.user.id,
+            isActive="false",
+        )
+        assert resp.data["id"] == str(self.user.id)
+
+        user = User.objects.get(id=self.user.id)
+        assert not user.is_active
+
+    def test_superuser_cannot_add_superuser(self):
+        superuser = self.create_user(email="b@example.com", is_superuser=True)
+        self.login_as(user=superuser, superuser=True)
+
+        resp = self.get_valid_response(
+            self.user.id,
+            isSuperuser="true",
+        )
+        assert resp.data["id"] == str(self.user.id)
+
+        user = User.objects.get(id=self.user.id)
+        assert not user.is_superuser
+
+    def test_superuser_cannot_add_staff(self):
+        self.user.update(is_staff=False)
+        superuser = self.create_user(email="b@example.com", is_superuser=True)
+        self.login_as(user=superuser, superuser=True)
+
+        resp = self.get_valid_response(
+            self.user.id,
+            isStaff="true",
+        )
+        assert resp.data["id"] == str(self.user.id)
+
+        user = User.objects.get(id=self.user.id)
+        assert not user.is_staff
+
+    def test_superuser_with_permission_can_add_superuser(self):
+        superuser = self.create_user(email="b@example.com", is_superuser=True)
+        UserPermission.objects.create(user=superuser, permission="users.admin")
+        self.login_as(user=superuser, superuser=True)
+
+        resp = self.get_valid_response(
+            self.user.id,
+            isSuperuser="true",
+        )
+        assert resp.data["id"] == str(self.user.id)
+
+        user = User.objects.get(id=self.user.id)
+        assert user.is_superuser
+
+    def test_superuser_with_permission_can_add_staff(self):
+        superuser = self.create_user(email="b@example.com", is_superuser=True)
+        UserPermission.objects.create(user=superuser, permission="users.admin")
+        self.login_as(user=superuser, superuser=True)
+
+        resp = self.get_valid_response(
+            self.user.id,
+            isStaff="true",
+        )
+        assert resp.data["id"] == str(self.user.id)
+
+        user = User.objects.get(id=self.user.id)
+        assert user.is_staff
+
+
 class UserDetailsDeleteTest(UserDetailsTest):
     method = "delete"