From c986b032a385174252da18e0dc0dc01316a121f6 Mon Sep 17 00:00:00 2001 From: geoffg-sentry <165922362+geoffg-sentry@users.noreply.github.com> Date: Wed, 18 Mar 2026 16:13:34 -0400 Subject: [PATCH 1/2] fix(has_release_permission): refactor a three year old weakness --- src/sentry/api/bases/organization.py | 29 +++-- .../organization_release_assemble.py | 4 +- .../endpoints/organization_release_details.py | 15 +-- .../organization_release_file_details.py | 8 +- .../endpoints/organization_release_files.py | 4 +- .../releases/endpoints/release_deploys.py | 6 +- .../test_organization_release_assemble.py | 33 ++++++ .../test_organization_release_file_details.py | 112 ++++++++++++++++++ .../test_organization_release_files.py | 38 ++++++ .../endpoints/test_release_deploys.py | 51 ++++++++ 10 files changed, 277 insertions(+), 23 deletions(-) diff --git a/src/sentry/api/bases/organization.py b/src/sentry/api/bases/organization.py index fa1cc07e32f1c3..b761ced3dfe5b7 100644 --- a/src/sentry/api/bases/organization.py +++ b/src/sentry/api/bases/organization.py @@ -706,14 +706,26 @@ def has_release_permission( organization: Organization | RpcOrganization, release: Release | None = None, project_ids: set[int] | None = None, + require_all_projects: bool = False, ) -> bool: """ Does the given request have permission to access this release, based on the projects to which the release is attached? + By default (require_all_projects=False), access is granted if the user + has access to *any* project linked to the release. This is appropriate + for read operations. + + When require_all_projects=True and a release is provided, access is + only granted if the user has access to *all* projects linked to the + release. Use this for mutations (PUT, DELETE, POST) that affect the + entire release across all its projects. This check respects Open + Membership: when allow_joinleave is True, has_project_access grants + access to all active org projects via has_global_access. + If the given request has an actor (user or ApiKey), cache the results - for a minute on the unique combination of actor,org,release, and project - ids. + for a minute on the unique combination of actor, org, release, project + ids, and require_all_projects. """ actor_id = None has_perms = None @@ -727,20 +739,23 @@ def has_release_permission( if requested_project_ids is None: requested_project_ids = self.get_requested_project_ids_unchecked(request) key = "release_perms:1:%s" % hash_values( - [actor_id, organization.id, release.id if release is not None else 0] + [ + actor_id, + organization.id, + release.id if release is not None else 0, + int(require_all_projects), + ] + sorted(requested_project_ids) ) has_perms = cache.get(key) if has_perms is None: projects = self.get_projects(request, organization, project_ids=project_ids) - # XXX(iambriccardo): The logic here is that you have access to this release if any of your projects - # associated with this release you have release permissions to. This is a bit of - # a problem because anyone can add projects to a release, so this check is easy - # to defeat. if release is not None: has_perms = ReleaseProject.objects.filter( release=release, project__in=projects ).exists() + if has_perms and require_all_projects: + has_perms = request.access.has_projects_access(list(release.projects.all())) else: has_perms = len(projects) > 0 diff --git a/src/sentry/releases/endpoints/organization_release_assemble.py b/src/sentry/releases/endpoints/organization_release_assemble.py index e429323c9ffaf5..5c6e53bdfb13f9 100644 --- a/src/sentry/releases/endpoints/organization_release_assemble.py +++ b/src/sentry/releases/endpoints/organization_release_assemble.py @@ -36,7 +36,9 @@ def post(self, request: Request, organization, version) -> Response: except Release.DoesNotExist: raise ResourceDoesNotExist - if not self.has_release_permission(request, organization, release): + if not self.has_release_permission( + request, organization, release, require_all_projects=True + ): raise ResourceDoesNotExist schema = { diff --git a/src/sentry/releases/endpoints/organization_release_details.py b/src/sentry/releases/endpoints/organization_release_details.py index b9a77e33d1685a..896b9a0dc42305 100644 --- a/src/sentry/releases/endpoints/organization_release_details.py +++ b/src/sentry/releases/endpoints/organization_release_details.py @@ -446,14 +446,12 @@ def put(self, request: Request, organization: Organization, version) -> Response scope.set_tag("failure_reason", "Release.DoesNotExist") raise ResourceDoesNotExist - if not self.has_release_permission(request, organization, release): + if not self.has_release_permission( + request, organization, release, require_all_projects=True + ): scope.set_tag("failure_reason", "no_release_permission") raise ResourceDoesNotExist - if not request.access.has_projects_access(list(projects)): - scope.set_tag("failure_reason", "no_access_to_all_projects") - raise ResourceDoesNotExist - serializer = OrganizationReleaseSerializer(data=request.data) if not serializer.is_valid(): @@ -562,10 +560,9 @@ def delete(self, request: Request, organization, version) -> Response: except Release.DoesNotExist: raise ResourceDoesNotExist - if not self.has_release_permission(request, organization, release): - raise ResourceDoesNotExist - - if not request.access.has_projects_access(list(release.projects.all())): + if not self.has_release_permission( + request, organization, release, require_all_projects=True + ): raise ResourceDoesNotExist try: diff --git a/src/sentry/releases/endpoints/organization_release_file_details.py b/src/sentry/releases/endpoints/organization_release_file_details.py index fdb1739650e53b..aaef4eb8c36915 100644 --- a/src/sentry/releases/endpoints/organization_release_file_details.py +++ b/src/sentry/releases/endpoints/organization_release_file_details.py @@ -76,7 +76,9 @@ def put(self, request: Request, organization: Organization, version, file_id) -> except Release.DoesNotExist: raise ResourceDoesNotExist - if not self.has_release_permission(request, organization, release): + if not self.has_release_permission( + request, organization, release, require_all_projects=True + ): raise ResourceDoesNotExist return self.update_releasefile(request, release, file_id) @@ -101,7 +103,9 @@ def delete(self, request: Request, organization, version, file_id) -> Response: except Release.DoesNotExist: raise ResourceDoesNotExist - if not self.has_release_permission(request, organization, release): + if not self.has_release_permission( + request, organization, release, require_all_projects=True + ): raise ResourceDoesNotExist return self.delete_releasefile(release, file_id) diff --git a/src/sentry/releases/endpoints/organization_release_files.py b/src/sentry/releases/endpoints/organization_release_files.py index e50860f557ba6b..fc07d7a652efbb 100644 --- a/src/sentry/releases/endpoints/organization_release_files.py +++ b/src/sentry/releases/endpoints/organization_release_files.py @@ -103,7 +103,9 @@ def post(self, request: Request, organization, version) -> Response: logger = logging.getLogger("sentry.files") logger.info("organizationreleasefile.start") - if not self.has_release_permission(request, organization, release): + if not self.has_release_permission( + request, organization, release, require_all_projects=True + ): raise ResourceDoesNotExist return self.post_releasefile(request, release, logger) diff --git a/src/sentry/releases/endpoints/release_deploys.py b/src/sentry/releases/endpoints/release_deploys.py index 2a30c256b7c565..9195fbf3fbc2f6 100644 --- a/src/sentry/releases/endpoints/release_deploys.py +++ b/src/sentry/releases/endpoints/release_deploys.py @@ -207,9 +207,9 @@ def post(self, request: Request, organization, version) -> Response: ) raise ResourceDoesNotExist - if not self.has_release_permission(request, organization, release): - # Logic here copied from `has_release_permission` (lightly edited for results to be more - # human-readable) + if not self.has_release_permission( + request, organization, release, require_all_projects=True + ): if request.user.is_authenticated: auth = f"user.id: {request.user.id}" elif request.auth is not None: diff --git a/tests/sentry/releases/endpoints/test_organization_release_assemble.py b/tests/sentry/releases/endpoints/test_organization_release_assemble.py index 710ae4b9f2fd69..6371f945690a0c 100644 --- a/tests/sentry/releases/endpoints/test_organization_release_assemble.py +++ b/tests/sentry/releases/endpoints/test_organization_release_assemble.py @@ -26,6 +26,39 @@ def setUp(self) -> None: args=[self.organization.slug, self.release.version], ) + def test_no_access_to_all_projects(self) -> None: + user = self.create_user(is_staff=False, is_superuser=False) + org = self.organization + org.flags.allow_joinleave = False + org.save() + + team1 = self.create_team(organization=org) + team2 = self.create_team(organization=org) + project1 = self.create_project(teams=[team1], organization=org) + project2 = self.create_project(teams=[team2], organization=org) + + release = self.create_release( + version="restricted-release.1", + project=project1, + additional_projects=[project2], + ) + + self.create_member(teams=[team1], user=user, organization=org) + self.login_as(user=user) + + url = reverse( + "sentry-api-0-organization-release-assemble", + args=[org.slug, release.version], + ) + + checksum = sha1(b"1").hexdigest() + response = self.client.post( + url, + data={"checksum": checksum, "chunks": [checksum]}, + ) + + assert response.status_code == 404 + def test_assemble_json_schema(self) -> None: response = self.client.post( self.url, data={"lol": "test"}, HTTP_AUTHORIZATION=f"Bearer {self.token.token}" diff --git a/tests/sentry/releases/endpoints/test_organization_release_file_details.py b/tests/sentry/releases/endpoints/test_organization_release_file_details.py index cd44002e1b83bf..9d222fe3153dac 100644 --- a/tests/sentry/releases/endpoints/test_organization_release_file_details.py +++ b/tests/sentry/releases/endpoints/test_organization_release_file_details.py @@ -120,6 +120,80 @@ def test_archived_with_dist(self) -> None: class ReleaseFileUpdateTest(APITestCase): + def test_no_access_to_all_projects(self) -> None: + user = self.create_user(is_staff=False, is_superuser=False) + org = self.organization + org.flags.allow_joinleave = False + org.save() + + team1 = self.create_team(organization=org) + team2 = self.create_team(organization=org) + project1 = self.create_project(teams=[team1], organization=org) + project2 = self.create_project(teams=[team2], organization=org) + + release = Release.objects.create(organization_id=org.id, version="1") + release.add_project(project1) + release.add_project(project2) + + releasefile = ReleaseFile.objects.create( + organization_id=org.id, + release_id=release.id, + file=File.objects.create(name="application.js", type="release.file"), + name="http://example.com/application.js", + ) + + self.create_member(teams=[team1], user=user, organization=org) + self.login_as(user=user) + + url = reverse( + "sentry-api-0-organization-release-file-details", + kwargs={ + "organization_id_or_slug": org.slug, + "version": release.version, + "file_id": releasefile.id, + }, + ) + response = self.client.put(url, {"name": "foobar"}) + + assert response.status_code == 404 + + def test_no_access_to_all_projects_open_membership(self) -> None: + user = self.create_user(is_staff=False, is_superuser=False) + org = self.organization + org.flags.allow_joinleave = True + org.save() + + team1 = self.create_team(organization=org) + team2 = self.create_team(organization=org) + project1 = self.create_project(teams=[team1], organization=org) + project2 = self.create_project(teams=[team2], organization=org) + + release = Release.objects.create(organization_id=org.id, version="1") + release.add_project(project1) + release.add_project(project2) + + releasefile = ReleaseFile.objects.create( + organization_id=org.id, + release_id=release.id, + file=File.objects.create(name="application.js", type="release.file"), + name="http://example.com/application.js", + ) + + self.create_member(teams=[team1], user=user, organization=org) + self.login_as(user=user) + + url = reverse( + "sentry-api-0-organization-release-file-details", + kwargs={ + "organization_id_or_slug": org.slug, + "version": release.version, + "file_id": releasefile.id, + }, + ) + response = self.client.put(url, {"name": "foobar"}) + + assert response.status_code == 200 + def test_simple(self) -> None: self.login_as(user=self.user) @@ -155,6 +229,44 @@ def test_simple(self) -> None: class ReleaseFileDeleteTest(APITestCase): + def test_no_access_to_all_projects(self) -> None: + user = self.create_user(is_staff=False, is_superuser=False) + org = self.organization + org.flags.allow_joinleave = False + org.save() + + team1 = self.create_team(organization=org) + team2 = self.create_team(organization=org) + project1 = self.create_project(teams=[team1], organization=org) + project2 = self.create_project(teams=[team2], organization=org) + + release = Release.objects.create(organization_id=org.id, version="1") + release.add_project(project1) + release.add_project(project2) + + releasefile = ReleaseFile.objects.create( + organization_id=org.id, + release_id=release.id, + file=File.objects.create(name="application.js", type="release.file"), + name="http://example.com/application.js", + ) + + self.create_member(teams=[team1], user=user, organization=org) + self.login_as(user=user) + + url = reverse( + "sentry-api-0-organization-release-file-details", + kwargs={ + "organization_id_or_slug": org.slug, + "version": release.version, + "file_id": releasefile.id, + }, + ) + response = self.client.delete(url) + + assert response.status_code == 404 + assert ReleaseFile.objects.filter(id=releasefile.id).exists() + def test_simple(self) -> None: self.login_as(user=self.user) diff --git a/tests/sentry/releases/endpoints/test_organization_release_files.py b/tests/sentry/releases/endpoints/test_organization_release_files.py index 73a0d909558d81..01f8a7c58717fb 100644 --- a/tests/sentry/releases/endpoints/test_organization_release_files.py +++ b/tests/sentry/releases/endpoints/test_organization_release_files.py @@ -206,6 +206,44 @@ def test_queries_should_be_narrowing_search(self) -> None: class ReleaseFileCreateTest(APITestCase): + def test_no_access_to_all_projects(self) -> None: + user = self.create_user(is_staff=False, is_superuser=False) + org = self.organization + org.flags.allow_joinleave = False + org.save() + + team1 = self.create_team(organization=org) + team2 = self.create_team(organization=org) + project1 = self.create_project(teams=[team1], organization=org) + project2 = self.create_project(teams=[team2], organization=org) + + release = Release.objects.create(organization_id=org.id, version="1") + release.add_project(project1) + release.add_project(project2) + + self.create_member(teams=[team1], user=user, organization=org) + self.login_as(user=user) + + url = reverse( + "sentry-api-0-organization-release-files", + kwargs={ + "organization_id_or_slug": org.slug, + "version": release.version, + }, + ) + response = self.client.post( + url, + { + "name": "http://example.com/application.js", + "file": SimpleUploadedFile( + "application.js", b"function() { }", content_type="application/javascript" + ), + }, + format="multipart", + ) + + assert response.status_code == 404 + def test_simple(self) -> None: project = self.create_project(name="foo") diff --git a/tests/sentry/releases/endpoints/test_release_deploys.py b/tests/sentry/releases/endpoints/test_release_deploys.py index 343c85bf9e85f4..8321c152f73aa9 100644 --- a/tests/sentry/releases/endpoints/test_release_deploys.py +++ b/tests/sentry/releases/endpoints/test_release_deploys.py @@ -152,6 +152,57 @@ def setUp(self) -> None: self.create_member(teams=[team], user=user, organization=self.org) self.login_as(user=user) + def test_no_access_to_all_projects(self) -> None: + self.org.flags.allow_joinleave = False + self.org.save() + + team2 = self.create_team(organization=self.org) + project2 = self.create_project(teams=[team2], organization=self.org) + + release = Release.objects.create(organization_id=self.org.id, version="1", total_deploys=0) + release.add_project(self.project) + release.add_project(project2) + + url = reverse( + "sentry-api-0-organization-release-deploys", + kwargs={ + "organization_id_or_slug": self.org.slug, + "version": release.version, + }, + ) + response = self.client.post( + url, + data={"name": "foo", "environment": "production", "url": "https://www.example.com"}, + ) + + assert response.status_code == 404 + assert not Deploy.objects.filter(release=release).exists() + + def test_no_access_to_all_projects_open_membership(self) -> None: + self.org.flags.allow_joinleave = True + self.org.save() + + team2 = self.create_team(organization=self.org) + project2 = self.create_project(teams=[team2], organization=self.org) + + release = Release.objects.create(organization_id=self.org.id, version="1", total_deploys=0) + release.add_project(self.project) + release.add_project(project2) + + url = reverse( + "sentry-api-0-organization-release-deploys", + kwargs={ + "organization_id_or_slug": self.org.slug, + "version": release.version, + }, + ) + response = self.client.post( + url, + data={"name": "foo", "environment": "production", "url": "https://www.example.com"}, + ) + + assert response.status_code == 201 + def test_simple(self) -> None: release = Release.objects.create(organization_id=self.org.id, version="1", total_deploys=0) release.add_project(self.project) From 6659753eb300daa6f9e5eefd1cc7d9f11e30a7f9 Mon Sep 17 00:00:00 2001 From: geoffg-sentry <165922362+geoffg-sentry@users.noreply.github.com> Date: Wed, 18 Mar 2026 16:16:01 -0400 Subject: [PATCH 2/2] better comment --- src/sentry/api/bases/organization.py | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/sentry/api/bases/organization.py b/src/sentry/api/bases/organization.py index b761ced3dfe5b7..24bc36b1ed0a3d 100644 --- a/src/sentry/api/bases/organization.py +++ b/src/sentry/api/bases/organization.py @@ -712,20 +712,15 @@ def has_release_permission( Does the given request have permission to access this release, based on the projects to which the release is attached? - By default (require_all_projects=False), access is granted if the user - has access to *any* project linked to the release. This is appropriate - for read operations. - - When require_all_projects=True and a release is provided, access is - only granted if the user has access to *all* projects linked to the - release. Use this for mutations (PUT, DELETE, POST) that affect the - entire release across all its projects. This check respects Open - Membership: when allow_joinleave is True, has_project_access grants - access to all active org projects via has_global_access. - - If the given request has an actor (user or ApiKey), cache the results - for a minute on the unique combination of actor, org, release, project - ids, and require_all_projects. + By default, access is granted if the user has access to *any* project + on the release (suitable for reads). When require_all_projects=True, + the user must have access to *all* projects on the release (use for + mutations). Without this, a user with access to one project on a + multi-project release could modify or delete it, affecting projects + they cannot access. The all-projects check respects Open Membership + via has_global_access. + + Results are cached for 60s per actor/org/release/project-ids/mode. """ actor_id = None has_perms = None