Browse Source

fix(scim): prevent last owner from being demoted (#52371)

Cathy Teng 1 year ago
parent
commit
1adc87e60e

+ 1 - 0
src/sentry/receivers/__init__.py

@@ -7,6 +7,7 @@ from .features import *  # noqa: F401,F403
 from .onboarding import *  # noqa: F401,F403
 from .outbox.control import *  # noqa: F401,F403
 from .outbox.region import *  # noqa: F401,F403
+from .owners import *  # noqa: F401,F403
 from .releases import *  # noqa: F401,F403
 from .reprocessing import *  # noqa: F401,F403
 from .rules import *  # noqa: F401,F403

+ 81 - 0
src/sentry/receivers/owners.py

@@ -0,0 +1,81 @@
+from django.db.models.signals import pre_delete, pre_save
+from rest_framework.serializers import ValidationError
+
+from sentry.models.organization import Organization
+from sentry.models.organizationmember import OrganizationMember
+from sentry.models.team import Team
+from sentry.roles import organization_roles
+
+
+def _assert_org_has_owner_not_from_team(organization, top_role):
+    if not organization.member_set.filter(role=top_role).exists():
+        raise ValidationError(detail="An organization must have at least one owner")
+
+
+def prevent_demoting_last_owner(instance: OrganizationMember, **kwargs):
+    # if a member is being created
+    if instance.id is None:
+        return
+
+    try:
+        member = OrganizationMember.objects.get(id=instance.id)
+    except OrganizationMember.DoesNotExist:
+        return
+
+    # member is the last owner and the update will remove the last owner
+    if (
+        member.is_only_owner()
+        and organization_roles.get_top_dog().id not in instance.get_all_org_roles()
+    ):
+        raise ValidationError(detail="An organization must have at least one owner")
+
+
+def prevent_demoting_last_owner_team(instance: Team, **kwargs):
+    # if a team is being created
+    if instance.id is None:
+        return
+
+    try:
+        team = Team.objects.get(id=instance.id)
+    except Team.DoesNotExist:
+        return
+
+    organization = Organization.objects.get_from_cache(id=team.organization_id)
+    top_role = organization_roles.get_top_dog().id
+
+    all_owner_teams = organization.get_teams_with_org_roles(roles=[top_role])
+
+    # demoting last owner team
+    if len(all_owner_teams) == 1 and team.org_role == top_role and instance.org_role != top_role:
+        _assert_org_has_owner_not_from_team(organization, top_role)
+
+
+def prevent_removing_last_owner_team(instance: Team, **kwargs):
+    organization = Organization.objects.get_from_cache(id=instance.organization_id)
+    top_role = organization_roles.get_top_dog().id
+
+    all_owner_teams = organization.get_teams_with_org_roles(roles=[top_role])
+
+    # removing last owner team
+    if len(all_owner_teams) == 1:
+        _assert_org_has_owner_not_from_team(organization, top_role)
+
+
+pre_save.connect(
+    prevent_demoting_last_owner,
+    sender=OrganizationMember,
+    dispatch_uid="prevent_demoting_last_owner",
+    weak=False,
+)
+pre_save.connect(
+    prevent_demoting_last_owner_team,
+    sender=Team,
+    dispatch_uid="prevent_demoting_last_owner_team",
+    weak=False,
+)
+pre_delete.connect(
+    prevent_removing_last_owner_team,
+    sender=Team,
+    dispatch_uid="prevent_deleting_last_owner_team",
+    weak=False,
+)

+ 6 - 0
tests/sentry/api/endpoints/test_sentry_app_details.py

@@ -403,6 +403,9 @@ class UpdateSentryAppDetailsTest(SentryAppDetailsTest):
 
     def test_members_cant_update(self):
         with assume_test_silo_mode(SiloMode.REGION):
+            # create extra owner because we are demoting one
+            self.create_member(organization=self.org, user=self.create_user(), role="owner")
+
             member_om = OrganizationMember.objects.get(user_id=self.user.id, organization=self.org)
             member_om.role = "member"
             member_om.save()
@@ -413,6 +416,9 @@ class UpdateSentryAppDetailsTest(SentryAppDetailsTest):
 
     def test_create_integration_exceeding_scopes(self):
         with assume_test_silo_mode(SiloMode.REGION):
+            # create extra owner because we are demoting one
+            self.create_member(organization=self.org, user=self.create_user(), role="owner")
+
             member_om = OrganizationMember.objects.get(user_id=self.user.id, organization=self.org)
             member_om.role = "manager"
             member_om.save()

+ 4 - 0
tests/sentry/api/endpoints/test_sentry_apps.py

@@ -585,6 +585,8 @@ class PostSentryAppsTest(SentryAppsTest):
         self.assert_sentry_app_status_code(sentry_app, status_code=400)
 
     def test_members_cant_create(self):
+        # create extra owner because we are demoting one
+        self.create_member(organization=self.organization, user=self.create_user(), role="owner")
         member_om = OrganizationMember.objects.get(
             user_id=self.user.id, organization=self.organization
         )
@@ -594,6 +596,8 @@ class PostSentryAppsTest(SentryAppsTest):
         self.get_error_response(**self.get_data(), status_code=403)
 
     def test_create_integration_exceeding_scopes(self):
+        # create extra owner because we are demoting one
+        self.create_member(organization=self.organization, user=self.create_user(), role="owner")
         member_om = OrganizationMember.objects.get(
             user_id=self.user.id, organization=self.organization
         )

+ 10 - 0
tests/sentry/models/test_organizationmember.py

@@ -5,6 +5,7 @@ import pytest
 from django.core import mail
 from django.db import router
 from django.utils import timezone
+from rest_framework.serializers import ValidationError
 
 from sentry import roles
 from sentry.auth import manager
@@ -512,3 +513,12 @@ class OrganizationMemberTest(TestCase, HybridCloudTestMixin):
         assert roles[0][1].id == "owner"
         assert roles[-1][0] == manager_team.slug
         assert roles[-1][1].id == "manager"
+
+    def test_cannot_demote_last_owner(self):
+        org = self.create_organization()
+
+        with pytest.raises(ValidationError):
+            member = self.create_member(organization=org, role="owner", user=self.create_user())
+
+            member.role = "manager"
+            member.save()

+ 23 - 0
tests/sentry/models/test_team.py

@@ -1,4 +1,6 @@
+import pytest
 from django.test import override_settings
+from rest_framework.serializers import ValidationError
 
 from sentry.models import (
     OrganizationMember,
@@ -72,6 +74,17 @@ class TeamTest(TestCase):
         assert team.id < 1_000_000_000
         assert Team.objects.filter(id=team.id).exists()
 
+    def test_cannot_demote_last_owner_team(self):
+        org = self.create_organization()
+
+        with pytest.raises(ValidationError):
+            team = self.create_team(org, org_role="owner")
+            self.create_member(
+                organization=org, role="member", user=self.create_user(), teams=[team]
+            )
+            team.org_role = "manager"
+            team.save()
+
 
 class TransferTest(TestCase):
     def test_simple(self):
@@ -201,3 +214,13 @@ class TeamDeletionTest(TestCase):
             type=NotificationSettingTypes.ISSUE_ALERTS,
             team_id=team_id,
         ).exists()
+
+    def test_cannot_delete_last_owner_team(self):
+        org = self.create_organization()
+
+        with pytest.raises(ValidationError):
+            team = self.create_team(org, org_role="owner")
+            self.create_member(
+                organization=org, role="member", user=self.create_user(), teams=[team]
+            )
+            team.delete()