From 55e0f32446de178fa96925b4f487346ea4195651 Mon Sep 17 00:00:00 2001 From: David Cramer Date: Wed, 27 Oct 2021 10:40:22 -0700 Subject: [PATCH] feat: Restrict superuser attributes to permission Add the 'users.admin' permission and restrict superuser-style mutations on users to superusers which have this permission. --- src/sentry/api/endpoints/user_details.py | 6 +- src/sentry/conf/server.py | 2 +- .../sentry/api/endpoints/test_user_details.py | 108 ++++++++++++++---- 3 files changed, 91 insertions(+), 25 deletions(-) diff --git a/src/sentry/api/endpoints/user_details.py b/src/sentry/api/endpoints/user_details.py index f5d614dfe8b17c..cf11dc9e321161 100644 --- a/src/sentry/api/endpoints/user_details.py +++ b/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) diff --git a/src/sentry/conf/server.py b/src/sentry/conf/server.py index 0f9a1e717c0bc1..200fbcdaa63329 100644 --- a/src/sentry/conf/server.py +++ b/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": { diff --git a/tests/sentry/api/endpoints/test_user_details.py b/tests/sentry/api/endpoints/test_user_details.py index fc8b02a6c9c45d..35f9b7af4c8891 100644 --- a/tests/sentry/api/endpoints/test_user_details.py +++ b/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"