From cd63d302f2f4568dbf88cb38a56876bb34b06c61 Mon Sep 17 00:00:00 2001 From: Irene Liu Date: Fri, 14 Nov 2025 11:29:19 -0800 Subject: [PATCH 1/5] bug fixes --- .../api/endpoints/data_forwarding_details.py | 52 +++++----- .../api/serializers/models/data_forwarder.py | 1 + .../endpoints/test_data_forwarding_details.py | 97 +++++++++++++++++++ 3 files changed, 128 insertions(+), 22 deletions(-) diff --git a/src/sentry/integrations/api/endpoints/data_forwarding_details.py b/src/sentry/integrations/api/endpoints/data_forwarding_details.py index 1afec9fc9aadf9..d8f3d8136e38da 100644 --- a/src/sentry/integrations/api/endpoints/data_forwarding_details.py +++ b/src/sentry/integrations/api/endpoints/data_forwarding_details.py @@ -169,19 +169,24 @@ def _validate_enrollment_changes( ) # Validate permissions on all projects - unauthorized_project_ids: set[int] = { - project_id - for project_id in all_projects_by_id.keys() - if not request.access.has_project_scope(all_projects_by_id[project_id], "project:write") - } - if unauthorized_project_ids: - raise PermissionDenied( - detail={ - "project_ids": [ - f"Insufficient access to projects: {', '.join(map(str, unauthorized_project_ids))}" - ] - } - ) + # org:write users can enroll/unenroll any project in the organization + # project:write users need explicit permission on each project + if not request.access.has_scope("org:write"): + unauthorized_project_ids: set[int] = { + project_id + for project_id in all_projects_by_id.keys() + if not request.access.has_project_scope( + all_projects_by_id[project_id], "project:write" + ) + } + if unauthorized_project_ids: + raise PermissionDenied( + detail={ + "project_ids": [ + f"Insufficient access to projects: {', '.join(map(str, unauthorized_project_ids))}" + ] + } + ) return project_ids_to_enroll, project_ids_to_unenroll @@ -295,16 +300,13 @@ def _update_single_project_configuration( def put( self, request: Request, organization: Organization, data_forwarder: DataForwarder ) -> Response: - # org:write users can update the main data forwarder configuration - if request.access.has_scope("org:write"): - return self._update_data_forwarder_config(request, organization, data_forwarder) - - # project:write users have two operation types: - # 1. Bulk enrollment/unenrollment: {"project_ids": [...]} - # 2. Single project override update: {"project_id": X, "overrides": {...}} - + # Determine operation type based on request body has_project_ids = "project_ids" in request.data has_project_id = "project_id" in request.data + has_config_fields = any( + key in request.data + for key in ["provider", "config", "is_enabled", "enroll_new_projects"] + ) if has_project_ids and has_project_id: raise serializers.ValidationError( @@ -312,13 +314,19 @@ def put( "Use 'project_ids' for bulk enrollment or 'project_id' with 'overrides' for single project update." ) + # org:write users can perform all operations, try main config update first + if request.access.has_scope("org:write") and has_config_fields: + return self._update_data_forwarder_config(request, organization, data_forwarder) + + # Project-specific operations (both org:write and project:write users) if has_project_ids: return self._update_enrollment(request, organization, data_forwarder) elif has_project_id: return self._update_single_project_configuration(request, organization, data_forwarder) else: raise serializers.ValidationError( - "Must specify either 'project_ids' for bulk enrollment or 'project_id' with 'overrides' for single project update." + "Must specify one of: main config fields (provider, config, etc.), " + "'project_ids' for bulk enrollment, or 'project_id' with 'overrides' for single project update." ) @extend_schema( diff --git a/src/sentry/integrations/api/serializers/models/data_forwarder.py b/src/sentry/integrations/api/serializers/models/data_forwarder.py index ac7727af55db9f..fae81726ee8d14 100644 --- a/src/sentry/integrations/api/serializers/models/data_forwarder.py +++ b/src/sentry/integrations/api/serializers/models/data_forwarder.py @@ -74,6 +74,7 @@ def serialize( ) -> dict[str, Any]: return { "id": str(obj.id), + "isEnabled": obj.is_enabled, "dataForwarderId": str(obj.data_forwarder_id), "project": { "id": str(obj.project_id), diff --git a/tests/sentry/integrations/api/endpoints/test_data_forwarding_details.py b/tests/sentry/integrations/api/endpoints/test_data_forwarding_details.py index 7dbad601bab247..fec8037bd84a84 100644 --- a/tests/sentry/integrations/api/endpoints/test_data_forwarding_details.py +++ b/tests/sentry/integrations/api/endpoints/test_data_forwarding_details.py @@ -502,6 +502,103 @@ def test_update_single_project_creates_new_config(self) -> None: assert project_config.overrides == {"custom": "value"} assert project_config.is_enabled + def test_org_write_can_bulk_enroll(self) -> None: + data_forwarder = self.create_data_forwarder( + provider=DataForwarderProviderSlug.SEGMENT, + config={"write_key": "test_key"}, + ) + + project1 = self.create_project(organization=self.organization) + project2 = self.create_project(organization=self.organization) + + user = self.create_user() + self.create_member( + user=user, + organization=self.organization, + role="manager", # Has org:write + ) + self.login_as(user=user) + + payload = { + "project_ids": [project1.id, project2.id], + } + + self.get_success_response( + self.organization.slug, data_forwarder.id, status_code=200, **payload + ) + + enrolled_projects = set( + DataForwarderProject.objects.filter( + data_forwarder=data_forwarder, is_enabled=True + ).values_list("project_id", flat=True) + ) + assert enrolled_projects == {project1.id, project2.id} + + def test_org_write_can_update_project_overrides(self) -> None: + data_forwarder = self.create_data_forwarder( + provider=DataForwarderProviderSlug.SEGMENT, + config={"write_key": "test_key"}, + ) + + project = self.create_project(organization=self.organization) + + user = self.create_user() + self.create_member( + user=user, + organization=self.organization, + role="manager", # Has org:write + ) + self.login_as(user=user) + + payload = { + "project_id": project.id, + "overrides": {"custom": "value"}, + "is_enabled": True, + } + + self.get_success_response( + self.organization.slug, data_forwarder.id, status_code=200, **payload + ) + + project_config = DataForwarderProject.objects.get( + data_forwarder=data_forwarder, project=project + ) + assert project_config.overrides == {"custom": "value"} + assert project_config.is_enabled + + def test_project_write_cannot_update_main_config(self) -> None: + """Test that project:write users cannot update main data forwarder config""" + data_forwarder = self.create_data_forwarder( + provider=DataForwarderProviderSlug.SEGMENT, + config={"write_key": "old_key"}, + is_enabled=True, + ) + + user = self.create_user() + self.create_member( + user=user, + organization=self.organization, + role="member", # Has project:write but not org:write + teams=[self.team], + teamRole="admin", + ) + self.login_as(user=user) + + payload = { + "provider": DataForwarderProviderSlug.SEGMENT, + "config": {"write_key": "new_key"}, + "is_enabled": False, + } + + self.get_error_response( + self.organization.slug, data_forwarder.id, status_code=400, **payload + ) + + # Config should remain unchanged + data_forwarder.refresh_from_db() + assert data_forwarder.config == {"write_key": "old_key"} + assert data_forwarder.is_enabled + @region_silo_test class DataForwardingDetailsDeleteTest(DataForwardingDetailsEndpointTest): From 3bd13a4d30ea8151dc0a5f5493596c6a26ba61ee Mon Sep 17 00:00:00 2001 From: Irene Liu Date: Fri, 14 Nov 2025 11:58:43 -0800 Subject: [PATCH 2/5] give project write perms to test --- .../integrations/api/endpoints/test_data_forwarding_details.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/sentry/integrations/api/endpoints/test_data_forwarding_details.py b/tests/sentry/integrations/api/endpoints/test_data_forwarding_details.py index fec8037bd84a84..4c5e61bd9b8e9e 100644 --- a/tests/sentry/integrations/api/endpoints/test_data_forwarding_details.py +++ b/tests/sentry/integrations/api/endpoints/test_data_forwarding_details.py @@ -547,6 +547,8 @@ def test_org_write_can_update_project_overrides(self) -> None: user=user, organization=self.organization, role="manager", # Has org:write + teams=[self.team], + teamRole="admin", ) self.login_as(user=user) From 307bcfaa6d223e92b208b9f9bd542f6a25cca42d Mon Sep 17 00:00:00 2001 From: Irene Liu Date: Fri, 14 Nov 2025 12:34:30 -0800 Subject: [PATCH 3/5] change request body parsing logic --- .../api/endpoints/data_forwarding_details.py | 25 ++++++++++--------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/sentry/integrations/api/endpoints/data_forwarding_details.py b/src/sentry/integrations/api/endpoints/data_forwarding_details.py index d8f3d8136e38da..7085062dd88013 100644 --- a/src/sentry/integrations/api/endpoints/data_forwarding_details.py +++ b/src/sentry/integrations/api/endpoints/data_forwarding_details.py @@ -100,13 +100,13 @@ def convert_args( def _update_data_forwarder_config( self, request: Request, organization: Organization, data_forwarder: DataForwarder - ) -> Response: + ) -> Response | None: """ Request body: {"is_enabled": true, "enroll_new_projects": true, "provider": "segment", "config": {...}, "project_ids": [1, 2, 3]} Returns: - Response: 200 OK with serialized data forwarder on success, - 400 Bad Request with validation errors on failure + Response: 200 OK with serialized data forwarder on success + None: If validation fails (signals caller to try other operations) """ data: dict[str, Any] = request.data data["organization_id"] = organization.id @@ -120,7 +120,8 @@ def _update_data_forwarder_config( serialize(data_forwarder, request.user), status=status.HTTP_200_OK, ) - return self.respond(serializer.errors, status=status.HTTP_400_BAD_REQUEST) + # Validation failed - return None to signal caller ddto try other operations + return None def _validate_enrollment_changes( self, @@ -303,10 +304,6 @@ def put( # Determine operation type based on request body has_project_ids = "project_ids" in request.data has_project_id = "project_id" in request.data - has_config_fields = any( - key in request.data - for key in ["provider", "config", "is_enabled", "enroll_new_projects"] - ) if has_project_ids and has_project_id: raise serializers.ValidationError( @@ -314,11 +311,15 @@ def put( "Use 'project_ids' for bulk enrollment or 'project_id' with 'overrides' for single project update." ) - # org:write users can perform all operations, try main config update first - if request.access.has_scope("org:write") and has_config_fields: - return self._update_data_forwarder_config(request, organization, data_forwarder) + # org:write users can perform all operations + # Try to update main config first - if serializer is valid, use that + # Otherwise fall through to project-specific operations + if request.access.has_scope("org:write"): + response = self._update_data_forwarder_config(request, organization, data_forwarder) + if response is not None: + return response - # Project-specific operations (both org:write and project:write users) + # Project-specific operations if has_project_ids: return self._update_enrollment(request, organization, data_forwarder) elif has_project_id: From 16057a6beeb97d70b618ad80dac225ea9fed3582 Mon Sep 17 00:00:00 2001 From: Irene Liu Date: Fri, 14 Nov 2025 12:41:02 -0800 Subject: [PATCH 4/5] update error message --- .../integrations/api/endpoints/data_forwarding_details.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/integrations/api/endpoints/data_forwarding_details.py b/src/sentry/integrations/api/endpoints/data_forwarding_details.py index 7085062dd88013..d806904695d86a 100644 --- a/src/sentry/integrations/api/endpoints/data_forwarding_details.py +++ b/src/sentry/integrations/api/endpoints/data_forwarding_details.py @@ -326,7 +326,7 @@ def put( return self._update_single_project_configuration(request, organization, data_forwarder) else: raise serializers.ValidationError( - "Must specify one of: main config fields (provider, config, etc.), " + "Must specify main config fields (provider, config, project_ids, etc.), " "'project_ids' for bulk enrollment, or 'project_id' with 'overrides' for single project update." ) From 3f675e347018f9d1d12c41fbc04a912b31821a16 Mon Sep 17 00:00:00 2001 From: Irene Liu Date: Fri, 14 Nov 2025 12:42:40 -0800 Subject: [PATCH 5/5] fix msg again --- .../integrations/api/endpoints/data_forwarding_details.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sentry/integrations/api/endpoints/data_forwarding_details.py b/src/sentry/integrations/api/endpoints/data_forwarding_details.py index d806904695d86a..7b47233cb9b9ab 100644 --- a/src/sentry/integrations/api/endpoints/data_forwarding_details.py +++ b/src/sentry/integrations/api/endpoints/data_forwarding_details.py @@ -326,8 +326,8 @@ def put( return self._update_single_project_configuration(request, organization, data_forwarder) else: raise serializers.ValidationError( - "Must specify main config fields (provider, config, project_ids, etc.), " - "'project_ids' for bulk enrollment, or 'project_id' with 'overrides' for single project update." + "Must specify provider, config, project_ids, etc. for main config update, " + "'project_ids' for bulk enrollment, or 'project_id' for single project update." ) @extend_schema(