Skip to content
Open
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
3 changes: 1 addition & 2 deletions src/sentry/core/endpoints/organization_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@
from sentry.silo.base import SiloMode
from sentry.types.cell import (
CellResolutionError,
RegionCategory,
get_locality_by_name,
)
from sentry.users.services.user.serial import serialize_generic_user
Comment thread
sentry-warden[bot] marked this conversation as resolved.
Expand Down Expand Up @@ -106,8 +105,8 @@
if "request" in self.context and is_active_staff(self.context["request"]):
# Staff users are allowed to create orgs in hidden cells/localities.
return value
if locality.category != RegionCategory.MULTI_TENANT or not locality.visible:
Comment thread
cursor[bot] marked this conversation as resolved.
if not locality.visible or not locality.signup_visible:
raise serializers.ValidationError(f"Unknown data storage location {value!r}.")

Check warning on line 109 in src/sentry/core/endpoints/organization_index.py

View check run for this annotation

@sentry/warden / warden: security-review

Single-tenant locality org-creation guard dropped for default-open signup_visible

Non-staff org creation now allows any locality with visible and signup_visible true, and no longer rejects SINGLE_TENANT by category. Because signup_visible defaults to true, a visible single-tenant locality name in dataStorageLocation can be used to provision orgs into dedicated cells unless every ST locality is explicitly configured signup_visible=false.

Check warning on line 109 in src/sentry/core/endpoints/organization_index.py

View check run for this annotation

@sentry/warden / warden: wrdn-authz

Single-tenant locality org-creation guard dropped for default-open signup_visible

In deployments retaining legacy `SINGLE_TENANT` locality configurations without an explicit `signup_visible` value, non-staff users can submit that locality through `dataStorageLocation` and provision organizations into its dedicated cell. The locality parser defaults omitted `signup_visible` to true, while the org-creation validator checks only `visible` and `signup_visible` and no longer enforces the single-tenant category boundary.
return value

Comment thread
sentry[bot] marked this conversation as resolved.
def validate(self, attrs):
Expand Down
3 changes: 1 addition & 2 deletions src/sentry/testutils/cell.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

from django.test import override_settings

from sentry.types.cell import Cell, CellDirectory, Locality, RegionCategory, get_global_directory
from sentry.types.cell import Cell, CellDirectory, Locality, get_global_directory


class TestEnvCellDirectory(CellDirectory):
Expand All @@ -24,7 +24,6 @@ def _apply_cells(
Locality(
name=c.name,
cells=frozenset([c.name]),
category=RegionCategory.MULTI_TENANT,
visible=c.visible,
new_org_cell=c.name,
)
Expand Down
15 changes: 5 additions & 10 deletions src/sentry/types/cell.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ class Locality:
cells: frozenset[str]
"""The set of cell names that belong to this locality."""

category: RegionCategory

new_org_cell: str
"""The cell within this locality where new organizations are provisioned."""

Expand All @@ -45,6 +43,9 @@ class Locality:
signup_visible: bool = True
"""Whether or not a locality should be visible for org signup/relocation."""

category: RegionCategory = RegionCategory.MULTI_TENANT
"""Deprecated. Visibility is defined via `visible` and `signup_visible`."""

def to_url(self, path: str) -> str:
"""Resolve a path into a customer facing URL on this locality.

Expand Down Expand Up @@ -495,19 +496,13 @@ def find_all_multitenant_locality_names() -> list[str]:
"""
Return all visible multi-tenant localities.
"""
return [
loc.name
for loc in get_global_directory().localities
if loc.category == RegionCategory.MULTI_TENANT and loc.visible
]
return [loc.name for loc in get_global_directory().localities if loc.visible]
Comment on lines 498 to +499

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.

Bug: Removing the category check in find_all_multitenant_locality_names may incorrectly expose single-tenant localities in the UI if they are configured with visible=True.
Severity: HIGH

Suggested Fix

Verify that all production configurations for single-tenant localities have been updated to set visible=False. To mitigate risk, consider adding logging to detect and alert on any single-tenant localities with visible=True during a transition period. Ensure the breaking nature of this change is clearly communicated.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: src/sentry/types/cell.py#L498-L499

Potential issue: The function `find_all_multitenant_locality_names` was changed to
filter localities solely based on the `visible` flag, removing the previous check for
`category == RegionCategory.MULTI_TENANT`. This change assumes that all single-tenant
localities in production configurations have been updated to `visible=False`. If any
single-tenant locality is configured with `visible=True`, it will now be incorrectly
exposed in the frontend UI and included in DNS prefetching. This represents a breaking
change and a migration risk, as the required configuration updates are not mentioned or
enforced, potentially leading to unintended exposure of single-tenant environments.

Also affects:

  • src/sentry/web/client_config.py:367~372



def find_all_signup_locality_names() -> list[str]:
"""
Return all locality names that are visible to org signup.
"""
return [
loc.name
for loc in get_global_directory().localities
if loc.category == RegionCategory.MULTI_TENANT and loc.visible and loc.signup_visible
loc.name for loc in get_global_directory().localities if loc.visible and loc.signup_visible
]
4 changes: 1 addition & 3 deletions src/sentry/web/client_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
from sentry.types.cell import (
Cell,
Locality,
RegionCategory,
find_all_cell_names,
find_all_multitenant_locality_names,
find_all_signup_locality_names,
Expand Down Expand Up @@ -365,10 +364,9 @@ def localities(self) -> list[Mapping[str, Any]]:

monolith_locality = get_locality_name_for_cell(settings.SENTRY_FALLBACK_CELL)

def region_display_order(region: Locality) -> tuple[bool, bool, str]:
def region_display_order(region: Locality) -> tuple[bool, str]:
return (
region.name != monolith_locality, # default locality comes first
region.category != RegionCategory.MULTI_TENANT, # multi-tenant before single
region.name, # then sort alphabetically
)

Expand Down
4 changes: 1 addition & 3 deletions tests/sentry/core/endpoints/test_organization_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -289,14 +289,12 @@ def test_locality_to_cell_resolution(self) -> None:
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,
),
Expand Down Expand Up @@ -339,7 +337,6 @@ def test_staff_user_override_cell_visiblity(self) -> None:
Locality(
name="ja",
cells=frozenset(["ja"]),
category=RegionCategory.MULTI_TENANT,
new_org_cell="ja",
visible=False,
),
Expand All @@ -349,6 +346,7 @@ def test_staff_user_override_cell_visiblity(self) -> None:
category=RegionCategory.SINGLE_TENANT,
new_org_cell="acme",
visible=True,
signup_visible=False,
),
]
with get_test_env_directory().swap_state(cells, localities):
Expand Down
6 changes: 3 additions & 3 deletions tests/sentry/middleware/integrations/parsers/test_jira.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@
from sentry.testutils.cell import override_cells
from sentry.testutils.outbox import assert_no_webhook_payloads, assert_webhook_payloads_for_mailbox
from sentry.testutils.silo import control_silo_test
from sentry.types.cell import Cell, Locality, RegionCategory
from sentry.types.cell import Cell, Locality

cell = Cell("us", 1, "http://us.testserver")
eu_cell = Cell("eu", 2, "http://eu.testserver")
locality = Locality("us", frozenset(["us"]), RegionCategory.MULTI_TENANT, new_org_cell="us")
eu_locality = Locality("eu", frozenset(["eu"]), RegionCategory.MULTI_TENANT, new_org_cell="eu")
locality = Locality("us", frozenset(["us"]), new_org_cell="us")
eu_locality = Locality("eu", frozenset(["eu"]), new_org_cell="eu")

cell_config = (cell, eu_cell)

Expand Down
7 changes: 1 addition & 6 deletions tests/sentry/types/test_cell.py
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ def test_validate_cell(self) -> None:
cell.validate()

def test_locality_to_url(self) -> None:
locality = Locality("us", frozenset(["us"]), RegionCategory.MULTI_TENANT, new_org_cell="us")
locality = Locality("us", frozenset(["us"]), new_org_cell="us")
with override_settings(SILO_MODE=SiloMode.CELL, SENTRY_LOCAL_CELL="us"):
assert locality.to_url("/avatar/abcdef/") == "http://us.testserver/avatar/abcdef/"
with override_settings(SILO_MODE=SiloMode.CONTROL, SENTRY_LOCAL_CELL=""):
Expand Down Expand Up @@ -345,14 +345,12 @@ def test_get_new_org_cell_for_locality(self) -> None:
Locality(
name="us",
cells=frozenset(["us", "us2"]),
category=RegionCategory.MULTI_TENANT,
new_org_cell="us2",
visible=True,
),
Locality(
name="de",
cells=frozenset(["de1", "de2"]),
category=RegionCategory.MULTI_TENANT,
new_org_cell="de2",
visible=True,
),
Expand Down Expand Up @@ -392,22 +390,19 @@ def test_find_all_signup_locality_names(self) -> None:
Locality(
name="us",
cells=frozenset(["us"]),
category=RegionCategory.MULTI_TENANT,
new_org_cell="us",
visible=True,
),
Locality(
name="de",
cells=frozenset(["de"]),
category=RegionCategory.MULTI_TENANT,
new_org_cell="de",
visible=True,
signup_visible=False,
),
Locality(
name="ja",
cells=frozenset(["ja"]),
category=RegionCategory.MULTI_TENANT,
new_org_cell="de",
visible=False,
signup_visible=True,
Expand Down
8 changes: 2 additions & 6 deletions tests/sentry/users/api/endpoints/test_user_regions.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,8 @@
cell_config = (us, de, st)
SECRET = "test-seer-api-shared-secret-thirty-two-bytes!"

us_locality = Locality(
name="us", cells=frozenset(["us"]), category=RegionCategory.MULTI_TENANT, new_org_cell="us"
)
de_locality = Locality(
name="de", cells=frozenset(["de"]), category=RegionCategory.MULTI_TENANT, new_org_cell="de"
)
us_locality = Locality(name="us", cells=frozenset(["us"]), new_org_cell="us")
de_locality = Locality(name="de", cells=frozenset(["de"]), new_org_cell="de")
st_locality = Locality(
name="acme",
cells=frozenset(["acme"]),
Expand Down
6 changes: 1 addition & 5 deletions tests/sentry/web/test_client_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,8 @@ def create_test_localities(
cell.Locality(
name=c.name,
cells=frozenset([c.name]),
category=(
cell.RegionCategory.SINGLE_TENANT
if c.name in single_tenants
else cell.RegionCategory.MULTI_TENANT
),
new_org_cell=c.name,
visible=c.name not in single_tenants,
)
for c in cells
)
Expand Down
Loading