Organizations: take into account the user when listing members (#11212)

* Organizations: take into account the user when listing members

Users shouldn't be able to see members from other teams they are not part of.

Ref readthedocs/readthedocs-corporate#1736

* Format

* Also don't allow seeing profiles from others

* Tests

* Tests

* Format

* Update readthedocs/profiles/views.py

Co-authored-by: Manuel Kaufmann <humitos@gmail.com>

---------

Co-authored-by: Manuel Kaufmann <humitos@gmail.com>

readthedocs/core/permissions.py

@@ -132,14 +132,31 @@ def admins(cls, obj):
             return obj.owners.all()
 
     @classmethod
-    def members(cls, obj):
-        from readthedocs.organizations.models import Organization
+    def members(cls, obj, user=None):
+        """
+        Return the users that are members of `obj`.
+
+        If `user` is provided, we return the members of `obj` that are visible to `user`.
+        For a project this means the users that have access to the project,
+        and for an organization, this means the users that are on the same teams as `user`,
+        including the organization owners.
+        """
+        from readthedocs.organizations.models import Organization, Team
         from readthedocs.projects.models import Project
 
         if isinstance(obj, Project):
-            return obj.users.all()
+            project_owners = obj.users.all()
+            if user and user not in project_owners:
+                return User.objects.none()
+            return project_owners
 
         if isinstance(obj, Organization):
+            if user:
+                teams = Team.objects.member(user, organization=obj)
+                return User.objects.filter(
+                    Q(teams__in=teams) | Q(owner_organizations=obj),
+                ).distinct()
+
             return User.objects.filter(
                 Q(teams__organization=obj) | Q(owner_organizations=obj),
             ).distinct()

readthedocs/core/tests/test_permissions.py

@@ -0,0 +1,98 @@
+from django.contrib.auth.models import User
+from django.test import TestCase, override_settings
+from django_dynamic_fixture import get
+
+from readthedocs.core.permissions import AdminPermission
+from readthedocs.organizations.constants import ADMIN_ACCESS, READ_ONLY_ACCESS
+from readthedocs.organizations.models import Organization, Team
+from readthedocs.projects.models import Project
+
+
+@override_settings(RTD_ALLOW_ORGANIZATIONS=True)
+class TestPermissionsWithOrganizations(TestCase):
+    def setUp(self):
+        self.owner = get(User)
+        self.project = get(Project)
+        self.organization = get(
+            Organization, owners=[self.owner], projects=[self.project]
+        )
+        self.team = get(Team, organization=self.organization, access=ADMIN_ACCESS)
+        self.team_read_only = get(
+            Team, organization=self.organization, access=READ_ONLY_ACCESS
+        )
+
+        self.user_admin = get(User)
+        self.user_on_same_team = get(User)
+        self.user_read_only = get(User)
+
+        self.organization.add_member(self.user_admin, self.team)
+        self.organization.add_member(self.user_on_same_team, self.team)
+        self.organization.add_member(self.user_read_only, self.team_read_only)
+
+        self.another_owner = get(User)
+        self.another_organization = get(Organization, owners=[self.another_owner])
+        self.another_team = get(
+            Team, organization=self.another_organization, access=ADMIN_ACCESS
+        )
+        self.another_team_read_only = get(
+            Team, organization=self.another_organization, access=READ_ONLY_ACCESS
+        )
+        self.another_user_admin = get(User)
+        self.another_user_read_only = get(User)
+
+        self.another_organization.add_member(self.another_user_admin, self.another_team)
+        self.another_organization.add_member(
+            self.another_user_read_only, self.another_team_read_only
+        )
+
+    def test_members(self):
+        users = AdminPermission.members(self.organization)
+        self.assertQuerySetEqual(
+            users,
+            [self.owner, self.user_admin, self.user_read_only, self.user_on_same_team],
+            ordered=False,
+            transform=lambda x: x,
+        )
+
+        users = AdminPermission.members(self.another_organization)
+        self.assertQuerySetEqual(
+            users,
+            [self.another_owner, self.another_user_admin, self.another_user_read_only],
+            ordered=False,
+            transform=lambda x: x,
+        )
+
+    def test_members_for_user(self):
+        # Owner should be able to see all members.
+        users = AdminPermission.members(self.organization, user=self.owner)
+        self.assertQuerySetEqual(
+            users,
+            [self.owner, self.user_admin, self.user_read_only, self.user_on_same_team],
+            ordered=False,
+            transform=lambda x: x,
+        )
+
+        # User is able to see users that are on the same team, and owners.
+        users = AdminPermission.members(self.organization, user=self.user_admin)
+        self.assertQuerySetEqual(
+            users,
+            [self.owner, self.user_admin, self.user_on_same_team],
+            ordered=False,
+            transform=lambda x: x,
+        )
+
+        users = AdminPermission.members(self.organization, user=self.user_on_same_team)
+        self.assertQuerySetEqual(
+            users,
+            [self.owner, self.user_admin, self.user_on_same_team],
+            ordered=False,
+            transform=lambda x: x,
+        )
+
+        users = AdminPermission.members(self.organization, user=self.user_read_only)
+        self.assertQuerySetEqual(
+            users,
+            [self.owner, self.user_read_only],
+            ordered=False,
+            transform=lambda x: x,
+        )

readthedocs/organizations/views/public.py

@@ -8,6 +8,7 @@
 from vanilla import DetailView, GenericView, ListView
 
 from readthedocs.core.filters import FilterContextMixin
+from readthedocs.core.permissions import AdminPermission
 from readthedocs.notifications.models import Notification
 from readthedocs.organizations.filters import (
     OrganizationProjectListFilterSet,
@@ -93,7 +94,9 @@ def get_context_data(self, **kwargs):
         return context
 
     def get_queryset(self):
-        return self.get_organization().members
+        return AdminPermission.members(
+            obj=self.get_organization(), user=self.request.user
+        )
 
     def get_success_url(self):
         return reverse_lazy(

readthedocs/profiles/views.py

@@ -120,8 +120,9 @@ def get_object(self):
         if request_user == user:
             return user
 
+        # Don't allow members to see another user profile if they don't share the same team.
         for org in Organization.objects.for_user(request_user):
-            if AdminPermission.is_member(user=user, obj=org):
+            if user in AdminPermission.members(obj=org, user=request_user):
                 return user
         raise Http404()
 

readthedocs/rtd_tests/tests/test_profile_views.py

@@ -267,6 +267,7 @@ def setUp(self):
         super().setUp()
         self.owner = get(User, username="owner")
         self.team_mate = get(User, username="teammate")
+        self.member_from_another_team = get(User, username="member_from_another_team")
         self.org = get(Organization, owners=[self.owner])
 
         self.team = get(
@@ -295,6 +296,10 @@ def setUp(self):
             self.team_mate,
             self.team2,
         )
+        self.org.add_member(
+            self.member_from_another_team,
+            self.team2,
+        )
 
         self.another_owner = get(User, username="another_owner")
         self.another_user = get(User, username="another_user")
@@ -320,10 +325,14 @@ def test_user_can_see_the_profile(self):
         self.assertEqual(resp.status_code, 200)
 
     def test_unrelated_user_can_not_see_the_profile(self):
-        self.client.force_login(self.another_user)
         url = reverse(
             "profiles_profile_detail", kwargs={"username": self.user.username}
         )
+        self.client.force_login(self.another_user)
+        resp = self.client.get(url)
+        self.assertEqual(resp.status_code, 404)
+
+        self.client.force_login(self.member_from_another_team)
         resp = self.client.get(url)
         self.assertEqual(resp.status_code, 404)
 
@@ -332,17 +341,28 @@ def test_unrelated_user_can_not_see_the_profile(self):
         self.assertEqual(resp.status_code, 404)
 
     def test_related_user_can_see_the_profile(self):
-        self.client.force_login(self.owner)
         url = reverse(
             "profiles_profile_detail", kwargs={"username": self.user.username}
         )
+        self.client.force_login(self.owner)
         resp = self.client.get(url)
         self.assertEqual(resp.status_code, 200)
 
         self.client.force_login(self.team_mate)
         resp = self.client.get(url)
         self.assertEqual(resp.status_code, 200)
 
+        url = reverse(
+            "profiles_profile_detail", kwargs={"username": self.owner.username}
+        )
+        self.client.force_login(self.user)
+        resp = self.client.get(url)
+        self.assertEqual(resp.status_code, 200)
+
+        self.client.force_login(self.member_from_another_team)
+        resp = self.client.get(url)
+        self.assertEqual(resp.status_code, 200)
+
     def test_user_without_orgs_can_see_their_own_profile(self):
         new_user = get(User)
         self.client.force_login(new_user)