move validation to validate_* methods & return correct HTTP status codes

Author: shellmayr

src/sentry/core/endpoints/organization_details.py

@@ -5,12 +5,14 @@
 from datetime import datetime, timedelta, timezone
 from typing import TypedDict
 
+from django.core.exceptions import PermissionDenied
 from django.db import models, router, transaction
 from django.db.models.query_utils import DeferredAttribute
 from django.urls import reverse
 from django.utils import timezone as django_timezone
 from drf_spectacular.utils import OpenApiResponse, extend_schema, extend_schema_serializer
 from rest_framework import serializers, status
+from rest_framework.exceptions import NotFound
 from sentry_sdk import capture_exception
 
 from bitfield.types import BitHandler
@@ -453,6 +455,26 @@ def validate_samplingMode(self, value):
 
         return value
 
+    def validate_hasGranularReplayPermissions(self, value):
+        self._validate_granular_replay_permissions()
+        return value
+
+    def validate_replayAccessMembers(self, value):
+        self._validate_granular_replay_permissions()
+        return value
+
+    def _validate_granular_replay_permissions(self):
+        organization = self.context["organization"]
+        request = self.context["request"]
+
+        if not features.has("organizations:granular-replay-permissions", organization):
+            raise NotFound("This feature is not enabled for your organization.")
+
+        if not request.access.has_scope("org:admin"):
+            raise PermissionDenied(
+                "You do not have permission to modify granular replay permissions."
+            )
+
     def validate(self, attrs):
         attrs = super().validate(attrs)
         if attrs.get("avatarType") == "upload":
@@ -567,85 +589,65 @@ def save(self, **kwargs):
         if trusted_relay_info is not None:
             self.save_trusted_relays(trusted_relay_info, changed_data, org)
 
-        if "hasGranularReplayPermissions" in data or "replayAccessMembers" in data:
-            if not features.has("organizations:granular-replay-permissions", org):
-                raise serializers.ValidationError(
-                    {
-                        "hasGranularReplayPermissions": "This feature is not enabled for your organization."
-                    }
-                )
-
-            if not self.context["request"].access.has_scope("org:admin"):
-                raise serializers.ValidationError(
-                    {
-                        "hasGranularReplayPermissions": "You do not have permission to modify granular replay permissions."
-                    }
-                )
-
-            if "hasGranularReplayPermissions" in data:
-                option_key = "sentry:granular-replay-permissions"
-                new_value = data["hasGranularReplayPermissions"]
-
-                option_inst, created = OrganizationOption.objects.update_or_create(
-                    organization=org, key=option_key, defaults={"value": new_value}
+        if "hasGranularReplayPermissions" in data:
+            option_key = "sentry:granular-replay-permissions"
+            new_value = data["hasGranularReplayPermissions"]
+            option_inst, created = OrganizationOption.objects.update_or_create(
+                organization=org, key=option_key, defaults={"value": new_value}
+            )
+            changed_data["hasGranularReplayPermissions"] = f"to {new_value}"
+
+        if "replayAccessMembers" in data:
+            member_ids = data["replayAccessMembers"]
+            if member_ids is None:
+                member_ids = []
+            current_member_ids = set(
+                OrganizationMemberReplayAccess.objects.filter(organization=org).values_list(
+                    "organizationmember_id", flat=True
                 )
-
-                changed_data["hasGranularReplayPermissions"] = f"to {new_value}"
-
-            if "replayAccessMembers" in data:
-                member_ids = data["replayAccessMembers"]
-
-                if member_ids is None:
-                    member_ids = []
-
-                current_member_ids = set(
-                    OrganizationMemberReplayAccess.objects.filter(organization=org).values_list(
-                        "organizationmember_id", flat=True
+            )
+            new_member_ids = set(member_ids)
+
+            to_add = new_member_ids - current_member_ids
+            to_remove = current_member_ids - new_member_ids
+            if to_add:
+                valid_member_ids = set(
+                    OrganizationMember.objects.filter(organization=org, id__in=to_add).values_list(
+                        "id", flat=True
                     )
                 )
-                new_member_ids = set(member_ids)
-
-                to_add = new_member_ids - current_member_ids
-                to_remove = current_member_ids - new_member_ids
-
-                if to_add:
-                    valid_member_ids = set(
-                        OrganizationMember.objects.filter(
-                            organization=org, id__in=to_add
-                        ).values_list("id", flat=True)
+                invalid_member_ids = to_add - valid_member_ids
+                if invalid_member_ids:
+                    raise serializers.ValidationError(
+                        {
+                            "replayAccessMembers": f"Invalid organization member IDs: {sorted(invalid_member_ids)}"
+                        }
                     )
-                    invalid_member_ids = to_add - valid_member_ids
-                    if invalid_member_ids:
-                        raise serializers.ValidationError(
-                            {
-                                "replayAccessMembers": f"Invalid organization member IDs: {sorted(invalid_member_ids)}"
-                            }
+
+                OrganizationMemberReplayAccess.objects.bulk_create(
+                    [
+                        OrganizationMemberReplayAccess(
+                            organization=org, organizationmember_id=member_id
                         )
+                        for member_id in to_add
+                    ],
+                    ignore_conflicts=True,
+                )
 
-                    OrganizationMemberReplayAccess.objects.bulk_create(
-                        [
-                            OrganizationMemberReplayAccess(
-                                organization=org, organizationmember_id=member_id
-                            )
-                            for member_id in to_add
-                        ],
-                        ignore_conflicts=True,
-                    )
+            if to_remove:
+                OrganizationMemberReplayAccess.objects.filter(
+                    organization=org, organizationmember_id__in=to_remove
+                ).delete()
 
+            if to_add or to_remove:
+                changes = []
+                if to_add:
+                    changes.append(f"added {len(to_add)} member(s)")
                 if to_remove:
-                    OrganizationMemberReplayAccess.objects.filter(
-                        organization=org, organizationmember_id__in=to_remove
-                    ).delete()
-
-                if to_add or to_remove:
-                    changes = []
-                    if to_add:
-                        changes.append(f"added {len(to_add)} member(s)")
-                    if to_remove:
-                        changes.append(f"removed {len(to_remove)} member(s)")
-                    changed_data["replayAccessMembers"] = (
-                        f"{' and '.join(changes)} (total: {len(new_member_ids)} member(s) with access)"
-                    )
+                    changes.append(f"removed {len(to_remove)} member(s)")
+                changed_data["replayAccessMembers"] = (
+                    f"{' and '.join(changes)} (total: {len(new_member_ids)} member(s) with access)"
+                )
 
         if "openMembership" in data:
             org.flags.allow_joinleave = data["openMembership"]

tests/sentry/core/endpoints/test_organization_details.py

@@ -1517,8 +1517,7 @@ def test_granular_replay_permissions_flag_unset(self) -> None:
 
     def test_granular_replay_permissions_flag_requires_feature(self) -> None:
         data = {"hasGranularReplayPermissions": True}
-        response = self.get_error_response(self.organization.slug, **data, status_code=400)
-        assert "hasGranularReplayPermissions" in response.data
+        self.get_error_response(self.organization.slug, **data, status_code=404)
 
     @with_feature("organizations:granular-replay-permissions")
     def test_granular_replay_permissions_flag_requires_admin_scope(self) -> None:
@@ -1656,8 +1655,7 @@ def test_replay_access_members_requires_feature(self) -> None:
             organization=self.organization, user=self.create_user(), role="member"
         )
         data = {"replayAccessMembers": [member1.id]}
-        response = self.get_error_response(self.organization.slug, **data, status_code=400)
-        assert "hasGranularReplayPermissions" in response.data
+        self.get_error_response(self.organization.slug, **data, status_code=404)
 
     @with_feature("organizations:granular-replay-permissions")
     def test_replay_access_members_requires_admin_scope(self) -> None: