diff --git a/src/sentry/api/endpoints/organization_access_request_details.py b/src/sentry/api/endpoints/organization_access_request_details.py index 6b5d89394ee0..87262b5ed286 100644 --- a/src/sentry/api/endpoints/organization_access_request_details.py +++ b/src/sentry/api/endpoints/organization_access_request_details.py @@ -14,8 +14,8 @@ from sentry.api.serializers import serialize from sentry.models.organization import Organization from sentry.models.organizationaccessrequest import OrganizationAccessRequest -from sentry.models.organizationmember import OrganizationMember from sentry.models.organizationmemberteam import OrganizationMemberTeam +from sentry.models.team import Team logger = logging.getLogger(__name__) @@ -35,6 +35,7 @@ class AccessRequestPermission(OrganizationPermission): ], "POST": [], "PUT": [ + "org:read", "org:write", "org:admin", "team:write", @@ -89,26 +90,38 @@ def get(self, request: Request, organization: Organization) -> Response: ).select_related("team", "member") ) - elif request.access.has_scope("team:write") and request.access.team_ids_with_membership: + else: + authorized_team_ids = [ + team.id + for team in Team.objects.filter(id__in=request.access.team_ids_with_membership) + if request.access.has_team_scope(team, "team:write") + ] + if not authorized_team_ids: + return Response([]) + access_requests = list( OrganizationAccessRequest.objects.filter( member__user_is_active=True, member__user_id__isnull=False, - team__id__in=request.access.team_ids_with_membership, + team_id__in=authorized_team_ids, ).select_related("team", "member") ) - else: - # Return empty response if user does not have access - return Response([]) - teams_by_user = OrganizationMember.objects.get_teams_by_user(organization=organization) + if not access_requests: + return Response([]) - # We omit any requests which are now redundant (i.e. the user joined that team some other way) + member_ids = {access_request.member_id for access_request in access_requests} + team_ids = {access_request.team_id for access_request in access_requests} + existing_memberships = set( + OrganizationMemberTeam.objects.filter( + organizationmember_id__in=member_ids, + team_id__in=team_ids, + ).values_list("organizationmember_id", "team_id") + ) valid_access_requests = [ access_request for access_request in access_requests - if access_request.member.user_id is not None - and access_request.team_id not in teams_by_user[access_request.member.user_id] + if (access_request.member_id, access_request.team_id) not in existing_memberships ] return Response(serialize(valid_access_requests, request.user)) diff --git a/tests/sentry/api/endpoints/test_organization_access_request_details.py b/tests/sentry/api/endpoints/test_organization_access_request_details.py index 200a81180375..5a35766e4862 100644 --- a/tests/sentry/api/endpoints/test_organization_access_request_details.py +++ b/tests/sentry/api/endpoints/test_organization_access_request_details.py @@ -3,6 +3,7 @@ from sentry.models.organizationaccessrequest import OrganizationAccessRequest from sentry.models.organizationmemberteam import OrganizationMemberTeam from sentry.testutils.cases import APITestCase +from sentry.testutils.helpers import Feature class GetOrganizationAccessRequestTest(APITestCase): @@ -102,6 +103,38 @@ def test_deny_request(self) -> None: assert not OrganizationAccessRequest.objects.filter(id=access_request.id).exists() def test_team_admin_can_approve(self) -> None: + organization = self.create_organization( + name="foo", + owner=self.user, + flags=0, + ) + user = self.create_user("bar@example.com") + member = self.create_member(organization=organization, user=user, role="member") + team = self.create_team(name="foo", organization=organization) + access_request = OrganizationAccessRequest.objects.create(member=member, team=team) + admin_user = self.create_user("admin@example.com") + self.create_member( + organization=organization, + user=admin_user, + role="member", + teams=[team], + teamRole="admin", + ) + path = reverse( + "sentry-api-0-organization-access-request-details", + args=[organization.slug, access_request.id], + ) + + self.login_as(admin_user) + with Feature({"organizations:team-roles": True}): + resp = self.client.put(path, data={"isApproved": 1}) + + assert resp.status_code == 204 + assert OrganizationMemberTeam.objects.filter( + organizationmember=member, team=team, is_active=True + ).exists() + + def test_legacy_org_admin_can_approve(self) -> None: self.login_as(user=self.user) organization = self.create_organization(name="foo", owner=self.user) diff --git a/tests/sentry/api/endpoints/test_organization_access_requests.py b/tests/sentry/api/endpoints/test_organization_access_requests.py index 64675e87f32f..1320c68b12cc 100644 --- a/tests/sentry/api/endpoints/test_organization_access_requests.py +++ b/tests/sentry/api/endpoints/test_organization_access_requests.py @@ -2,6 +2,7 @@ from sentry.models.organizationaccessrequest import OrganizationAccessRequest from sentry.testutils.cases import APITestCase +from sentry.testutils.helpers import Feature class UpdateOrganizationAccessRequestTest(APITestCase): @@ -23,6 +24,25 @@ def test_owner_can_list_access_requests(self) -> None: assert len(resp.data) == 1 assert resp.data[0]["member"]["email"] == "bar@example.com" + def test_inactive_team_membership_is_not_listed_as_pending(self) -> None: + organization = self.create_organization(name="foo", owner=self.user) + requester = self.create_member( + organization=organization, + user=self.create_user("requester@example.com"), + role="member", + ) + team = self.create_team(name="foo", organization=organization) + membership = self.create_team_membership(team=team, member=requester) + membership.update(is_active=False) + OrganizationAccessRequest.objects.create(member=requester, team=team) + path = reverse("sentry-api-0-organization-access-requests", args=[organization.slug]) + + self.login_as(self.user) + resp = self.client.get(path) + + assert resp.status_code == 200 + assert resp.data == [] + def test_admin_can_list_access_requests(self) -> None: organization = self.create_organization( name="foo", @@ -59,6 +79,41 @@ def test_admin_can_list_access_requests(self) -> None: assert resp.data[0]["member"]["id"] == str(request_1.member_id) assert resp.data[0]["team"]["id"] == str(request_1.team_id) + def test_team_admin_only_lists_requests_for_managed_teams(self) -> None: + organization = self.create_organization( + name="foo", + owner=self.user, + flags=0, + ) + managed_team = self.create_team(name="managed", organization=organization) + other_team = self.create_team(name="other", organization=organization) + team_admin = self.create_user("admin@example.com") + team_admin_member = self.create_member( + organization=organization, + user=team_admin, + role="member", + teams=[managed_team], + teamRole="admin", + ) + self.create_team_membership(team=other_team, member=team_admin_member, role="contributor") + requester = self.create_member( + organization=organization, + user=self.create_user("requester@example.com"), + role="member", + ) + managed_request = OrganizationAccessRequest.objects.create( + member=requester, team=managed_team + ) + OrganizationAccessRequest.objects.create(member=requester, team=other_team) + path = reverse("sentry-api-0-organization-access-requests", args=[organization.slug]) + + self.login_as(team_admin) + with Feature({"organizations:team-roles": True}): + resp = self.client.get(path) + + assert resp.status_code == 200 + assert [item["id"] for item in resp.data] == [str(managed_request.id)] + def test_member_empty_results(self) -> None: self.login_as(user=self.user) @@ -70,7 +125,7 @@ def test_member_empty_results(self) -> None: OrganizationAccessRequest.objects.create(member=member, team=team) user = self.create_user("foo@example.com") - member = self.create_member(organization=organization, user=user, role="member") + self.create_member(organization=organization, user=user, role="member") path = reverse("sentry-api-0-organization-access-requests", args=[organization.slug])