Skip to content
Open
Show file tree
Hide file tree
Changes from 2 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
21 changes: 14 additions & 7 deletions src/sentry/api/serializers/models/organization.py
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,11 @@ def validate_name(self, value: str) -> str:

return value

def validate_slug(self, value: str) -> str:
def _validate_slug_shape(self, value: str) -> str:
"""
Validate slug values without any DB queries.
This method is re-used across cell + control silos.
"""
# Historically, the only check just made sure there was more than 1
# character for the slug, but since then, there are many slugs that
# fit within this new imposed limit. We're not fixing existing, but
Expand All @@ -209,19 +213,22 @@ def validate_slug(self, value: str) -> str:
)
if value in RESERVED_ORGANIZATION_SLUGS:
raise serializers.ValidationError(f'This slug "{value}" is reserved and not allowed.')
qs = Organization.objects.filter(slug=value)
if "organization" in self.context:
qs = qs.exclude(id=self.context["organization"].id)
if qs.exists():
raise serializers.ValidationError(f'The slug "{value}" is already in use.')

contains_whitespace = any(c.isspace() for c in self.initial_data["slug"])
if contains_whitespace:
raise serializers.ValidationError(
f'The slug "{value}" should not contain any whitespace.'
)
return value

def validate_slug(self, value: str) -> str:
value = self._validate_slug_shape(value)
qs = Organization.objects.filter(slug=value)
if "organization" in self.context:
qs = qs.exclude(id=self.context["organization"].id)
if qs.exists():
raise serializers.ValidationError(f'The slug "{value}" is already in use.')
return value


class TrustedRelaySerializerResponse(TypedDict, total=False):
name: str
Expand Down
57 changes: 43 additions & 14 deletions src/sentry/core/endpoints/organization_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
from sentry.models.organizationmapping import OrganizationMapping
from sentry.models.organizationmember import OrganizationMember
from sentry.models.organizationmembermapping import OrganizationMemberMapping
from sentry.organizations.services.organization import organization_service
from sentry.search.utils import tokenize_query
from sentry.services.organization import (
OrganizationOptions,
Expand All @@ -41,6 +42,12 @@
)
from sentry.services.organization.provisioning import organization_provisioning_service
from sentry.silo.base import SiloMode
from sentry.types.cell import (
CellResolutionError,
RegionCategory,
get_locality_by_name,
get_new_org_cell_for_locality,
)
from sentry.users.services.user.serial import serialize_generic_user
from sentry.users.services.user.service import user_service
from sentry.utils.pagination_factory import PaginatorLike
Expand All @@ -53,6 +60,7 @@ class OrganizationPostSerializer(BaseOrganizationSerializer):
agreeTerms = serializers.BooleanField(required=True)
aggregatedDataConsent = serializers.BooleanField(required=False)
idempotencyKey = serializers.CharField(max_length=IDEMPOTENCY_KEY_LENGTH, required=False)
dataStorageLocation = serializers.CharField(required=False)

def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
Expand All @@ -66,6 +74,37 @@ def validate_agreeTerms(self, value):
raise serializers.ValidationError("This attribute is required.")
return value

def validate_slug(self, value: str) -> str:
if SiloMode.get_current_mode() == SiloMode.CONTROL:
value = self._validate_slug_shape(value)
if OrganizationMapping.objects.filter(slug=value).exists():
raise serializers.ValidationError(f'The slug "{value}" is already in use.')

return value
# TODO(cells) remove this path when cell scoped provisioning is removed.
return super().validate_slug(value)

def validate_dataStorageLocation(self, value: str) -> str:
try:
locality = get_locality_by_name(value)
except CellResolutionError:
raise serializers.ValidationError(f"Unknown data storage location {value!r}.")
if locality.category != RegionCategory.MULTI_TENANT or not locality.visible:
raise serializers.ValidationError(f"Unknown data storage location {value!r}.")
return value

def validate(self, attrs):
attrs = super().validate(attrs)

locality_name = attrs.get("dataStorageLocation")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come it's not mandatory? Is this for self-hosted/ST or some specific environment?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This serializer and logic is also used for the current cell scoped endpoint which doesn't receive dataStorageLocation. Once we are fully control, dataStorageLocation will be required.

if locality_name is None and SiloMode.get_current_mode() == SiloMode.CONTROL:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few cases of SiloMode.get_current_mode() == SiloMode.CONTROL throughout this class... one option would be a separate subclass so the conditional is moved up a level. it might make it clearer what should stay and what should go when cell based provisioning is deprecated

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of the silo mode checks in this serializer can be removed once cell provisioning is removed. I'll add some TODO's for future me.

locality_name = settings.SENTRY_MONOLITH_REGION
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SENTRY_MONOLITH_REGION is technically a cell name...
Right now they are the same everywhere but i think we should avoid that assumption here

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block is intended to handle the transition from cell -> control requests. I'm planning on removing this once we don't have any more cell scoped requests.


if locality_name is not None:
attrs["cell_name"] = get_new_org_cell_for_locality(locality_name).name
Comment thread
cursor[bot] marked this conversation as resolved.

return attrs


@extend_schema(tags=["Users"])
@all_silo_endpoint
Expand Down Expand Up @@ -312,7 +351,6 @@ def _get_from_control(self, request: Request) -> Response:
paginator_cls=paginator_cls,
)

# XXX: endpoint useless for end-users as it needs user context.
def post(self, request: Request) -> Response:
"""
Create a New Organization
Expand All @@ -329,13 +367,6 @@ def post(self, request: Request) -> Response:
terms of service and privacy policy.
:auth: required, user-context-needed
"""
# TODO(cells): Move org creation to control as part of the broader
# org-listing/org-provisioning cutover. Since POST is private, the
# legacy cell-side path can be removed once the control implementation
# is ready.
if SiloMode.get_current_mode() == SiloMode.CONTROL:
return Response(status=404)

if not request.user.is_authenticated:
return Response({"detail": "This endpoint requires user info"}, status=401)

Expand Down Expand Up @@ -403,16 +434,14 @@ def post(self, request: Request) -> Response:
)

rpc_org = organization_provisioning_service.provision_organization_in_cell(
cell_name=settings.SENTRY_LOCAL_CELL or settings.SENTRY_MONOLITH_REGION,
cell_name=result.get("cell_name"),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cell_name=result.get("cell_name"),
cell_name=result["cell_name"],

how come it's nullable?

provisioning_options=provision_args,
)
org = Organization.objects.get(id=rpc_org.id)

# TODO(hybrid-cloud): We'll need to catch a more generic error
# when the internal RPC is implemented.
Comment on lines -411 to -412
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this applies for the cell-provisioned version right? is the comment removed since it's different on control?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This TODO is a few years old now, and mentions internal RPC being added (which it has). RPC calls don't raise IntegrityError and this catch block can be removed once we remove the cell scope compatibility.

except IntegrityError:
return Response(
{"detail": "An organization with this slug already exists."}, status=409
)

return Response(serialize(org, request.user), status=201)
org_data = organization_service.serialize_organization(id=rpc_org.id, as_user=rpc_user)

return Response(org_data, status=201)
Comment thread
sentry[bot] marked this conversation as resolved.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unchecked None from serialize_organization returns empty 201

Medium Severity

organization_service.serialize_organization() has return type Any | None and returns None when the organization isn't found. The result is passed directly to Response(org_data, status=201) without a None check. If org_data is None, the endpoint returns a 201 success with an empty body, silently indicating success while providing no data. The previous code used Organization.objects.get() which would raise on missing orgs. In a cross-silo RPC context, transient issues could make this more likely than in the old single-silo path.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 858c4e8. Configure here.

210 changes: 209 additions & 1 deletion tests/sentry/core/endpoints/test_organization_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,19 @@
from sentry.silo.base import SiloMode
from sentry.testutils.asserts import assert_org_audit_log_exists
from sentry.testutils.cases import APITestCase, TwoFactorAPITestCase
from sentry.testutils.cell import get_test_env_directory
from sentry.testutils.helpers.analytics import assert_any_analytics_event
from sentry.testutils.helpers.options import override_options
from sentry.testutils.hybrid_cloud import HybridCloudTestMixin
from sentry.testutils.outbox import outbox_runner
from sentry.testutils.silo import (
assume_test_silo_mode,
assume_test_silo_mode_of,
cell_silo_test,
control_silo_test,
create_test_cells,
)
from sentry.types.cell import Cell, Locality, RegionCategory
from sentry.users.models.authenticator import Authenticator
from sentry.utils.slug import ORG_SLUG_PATTERN

Expand Down Expand Up @@ -234,7 +237,212 @@ def test_response_compatible_with_cell(self) -> None:
}


class OrganizationsCreateTest(OrganizationIndexTest, HybridCloudTestMixin):
@control_silo_test(cells=create_test_cells("us", "de"))
class OrganizationsCreateControlTest(OrganizationIndexTest, HybridCloudTestMixin):
method = "post"

def test_implicit_data_storage_location(self) -> None:
response = self.get_success_response(name="implicit org", slug="implicit-org")

organization_id = response.data["id"]
with assume_test_silo_mode(SiloMode.CONTROL):
mapping = OrganizationMapping.objects.get(organization_id=organization_id)
assert mapping.cell_name == "us"

def test_invalid_data_storage_location(self) -> None:
response = self.get_error_response(
name="bad locality",
slug="bad-locality",
dataStorageLocation="atlantis",
status_code=400,
)
assert "dataStorageLocation" in response.data

def test_locality_to_cell_resolution(self) -> None:
cells = [
Cell(
name="us",
snowflake_id=1,
category=RegionCategory.MULTI_TENANT,
address="10.0.0.1",
visible=True,
),
Cell(
name="us2",
snowflake_id=3,
category=RegionCategory.MULTI_TENANT,
address="10.0.0.2",
visible=True,
),
Cell(
name="de",
snowflake_id=4,
category=RegionCategory.MULTI_TENANT,
address="10.0.0.4",
visible=True,
),
]

localities = [
Locality(
name="us",
cells=frozenset(["us", "us2"]),
category=RegionCategory.MULTI_TENANT,
new_org_cell="us2",
visible=True,
),
Locality(
name="de",
cells=frozenset(["de"]),
category=RegionCategory.MULTI_TENANT,
new_org_cell="de",
visible=True,
),
]
with get_test_env_directory().swap_state(cells, localities):
data = {"name": "Acme co", "slug": "acme-co", "dataStorageLocation": "us"}
resp = self.get_success_response(**data)
assert "acme-co" == resp.data["slug"]
org_id = resp.data["id"]

with assume_test_silo_mode_of(OrganizationMapping):
org_mapping = OrganizationMapping.objects.get(slug="acme-co", organization_id=org_id)
assert org_mapping.cell_name == "us2", "Should be created in us2 cell, not us"

with assume_test_silo_mode_of(Organization):
org = Organization.objects.get(id=org_id)
assert org.slug == org_mapping.slug

# Validate ownership of the new org
owners = [owner.id for owner in org.get_owners()]
assert [self.user.id] == owners

def test_with_default_team_true(self) -> None:
data = {"name": "hello world", "slug": "foobar", "defaultTeam": True}
response = self.get_success_response(**data)

organization_id = response.data["id"]

with assume_test_silo_mode_of(Organization):
Organization.objects.get(id=organization_id)
team = Team.objects.get(organization_id=organization_id)
assert team.name == "hello world"

org_member = OrganizationMember.objects.get(
organization_id=organization_id, user_id=self.user.id
)
OrganizationMemberTeam.objects.get(organizationmember_id=org_member.id, team_id=team.id)

def test_invalid_slug_values(self) -> None:
with self.options({"api.rate-limit.org-create": 9001}):
self.get_error_response(name="name", slug=" i have whitespace ", status_code=400)
self.get_error_response(name="name", slug="bird-company!", status_code=400)

def test_conflicting_slug(self) -> None:
self.create_organization(slug="acme-co")

resp = self.get_error_response(name="name", slug="acme-co", status_code=400)
assert 'The slug "acme-co" is already in use' in str(resp.data)

def test_name_with_url_scheme_rejected(self) -> None:
with self.options({"api.rate-limit.org-create": 9001}):
self.get_error_response(
name="https://evil.com Click Here", slug="legit-slug", status_code=400
)
self.get_error_response(name="http://evil.com", slug="legit-slug-2", status_code=400)

def test_name_with_spam_signals_rejected(self) -> None:
response = self.get_error_response(
name="Win $50 ETH bit.ly/offer Claim Now",
slug="spam-org",
status_code=400,
)
assert "disallowed content" in str(response.data)

def test_name_with_single_signal_allowed(self) -> None:
response = self.get_success_response(name="BTC Analytics", slug="btc-analytics")
with assume_test_silo_mode_of(Organization):
org = Organization.objects.get(id=response.data["id"])
assert org.name == "BTC Analytics"

def test_required_terms_with_terms_url(self) -> None:
data: dict[str, Any] = {"name": "hello world"}
with self.settings(PRIVACY_URL=None, TERMS_URL="https://example.com/terms"):
self.get_success_response(**data)

with self.settings(TERMS_URL=None, PRIVACY_URL="https://example.com/privacy"):
self.get_success_response(**data)

with self.settings(
TERMS_URL="https://example.com/terms", PRIVACY_URL="https://example.com/privacy"
):
data = {"name": "hello world", "agreeTerms": False}
self.get_error_response(status_code=400, **data)

data = {"name": "hello world", "agreeTerms": True}
self.get_success_response(**data)

@mock.patch("sentry.analytics.record")
def test_success_analytics_in_rpc_call(self, mock_record: mock.MagicMock) -> None:
self.login_as(user=self.user)

with outbox_runner():
data = {
"name": "org name",
"aggregatedDataConsent": True,
"agreeTerms": True,
"defaultTeam": True,
}
response = self.get_success_response(**data)
assert response.status_code == 201

with assume_test_silo_mode_of(Organization):
org = Organization.objects.get(slug="org-name")

assert_any_analytics_event(
mock_record,
OrganizationCreatedEvent(
id=org.id,
actor_id=self.user.id,
name=org.name,
slug=org.slug,
),
)
assert_any_analytics_event(
mock_record, AggregatedDataConsentOrganizationCreatedEvent(organization_id=org.id)
)
assert_org_audit_log_exists(
organization=org,
event=audit_log.get_event_id("ORG_ADD"),
)
with assume_test_silo_mode_of(Organization):
assert org.get_option("sentry:aggregated_data_consent") is True
assert org.get_option("sentry:streamline_ui_only") is True
assert OrganizationMember.objects.filter(
organization_id=org.id, user_id=self.user.id
).exists()
assert Team.objects.filter(organization_id=org.id).exists()

def test_demo_user_cannot_create_organization(self) -> None:
demo_user = self.create_user("demo@example.com")
self.login_as(demo_user)
with override_options({"demo-mode.enabled": True, "demo-mode.users": [demo_user.id]}):
self.get_error_response(name="demo org", slug="demo-org", status_code=403)

with assume_test_silo_mode_of(Organization):
assert not Organization.objects.filter(slug="demo-org").exists()

def test_demo_user_cannot_create_organization_when_demo_mode_disabled(self) -> None:
demo_user = self.create_user("demo@example.com")
self.login_as(demo_user)
with override_options({"demo-mode.enabled": False, "demo-mode.users": [demo_user.id]}):
self.get_error_response(name="demo org", slug="demo-org", status_code=403)

with assume_test_silo_mode_of(Organization):
assert not Organization.objects.filter(slug="demo-org").exists()


class OrganizationsCreateInCellTest(OrganizationIndexTest, HybridCloudTestMixin):
method = "post"

def test_missing_params(self) -> None:
Expand Down
Loading