Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 35 additions & 26 deletions src/sentry/integrations/api/endpoints/data_forwarding_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -169,19 +170,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

Expand Down Expand Up @@ -295,14 +301,7 @@ 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

Expand All @@ -312,13 +311,23 @@ 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 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
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 provider, config, project_ids, etc. for main config update, "
"'project_ids' for bulk enrollment, or 'project_id' for single project update."
)

@extend_schema(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,105 @@ 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
teams=[self.team],
teamRole="admin",
)
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):
Expand Down
Loading