fix(teams): Allow team admins to review access requests

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))

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)

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])