Restrict read acces for public groups

arkindex/project/mixins.py

@@ -29,11 +29,15 @@ class ACLMixin(object):
     def user(self):
         return self._user or self.request.user
 
-    def has_access(self, instance, level):
+    def has_access(self, instance, level, skip_public=False):
+        """
+        Check if the user has access to a generic instance with a minimum level
+        If skip_public parameter is set to true, exclude rights on public instances
+        """
         check_level_param(level)
 
         # Handle special authentications
-        if level <= Role.Guest.value and getattr(instance, 'public', False):
+        if not skip_public and level <= Role.Guest.value and getattr(instance, 'public', False):
             return True
         if self.user.is_anonymous:
             return False

arkindex/users/api.py

@@ -521,7 +521,10 @@ class GroupDetails(RetrieveUpdateDestroyAPIView):
     }
 
     def get_membership(self, group):
-        return get_object_or_404(group.memberships, user_id=self.request.user.id)
+        try:
+            return group.memberships.get(user_id=self.request.user.id)
+        except Right.DoesNotExist:
+            raise PermissionDenied(detail='Only members of a group can retrieve their details')
 
     def get_object(self):
         if not hasattr(self, '_group'):
@@ -541,10 +544,13 @@ class GroupDetails(RetrieveUpdateDestroyAPIView):
             raise PermissionDenied(detail='Only members with an admin privilege may update or delete this group.')
 
     def get_queryset(self):
-        # Filter groups to which user belongs only
-        return Group.objects\
+        # Filter groups that are public or for which the user is a member
+        return Group.objects \
             .annotate(members_count=Count('memberships')) \
-            .filter(Q(memberships__user=self.request.user))
+            .filter(
+                Q(public=True)
+                | Q(memberships__user=self.request.user)
+            )
 
 
 class UserMemberships(ListAPIView):
@@ -666,9 +672,13 @@ class GenericMembershipsList(ACLMixin, ListAPIView):
 
     def get_queryset(self):
         content_object = self.get_content_object()
-        # Raise a permission denied if the user has no access to the group
-        if not self.has_access(content_object, Role.Guest.value):
-            raise PermissionDenied
+        # Raise a permission denied if the user has no access to the content object
+        access_params = {}
+        if isinstance(content_object, Group):
+            # A user may not be able to list group members if he is not a member
+            access_params['skip_public'] = True
+        if not self.has_access(content_object, Role.Guest.value, **access_params):
+            raise PermissionDenied(detail='You do not have the required access level to list members for this content.')
 
         qs = content_object.memberships \
             .prefetch_related(

arkindex/users/tests/test_generic_memberships.py

@@ -4,6 +4,7 @@ from django.urls import reverse
 from rest_framework import status
 
 from arkindex.dataimport.models import Repository, RepositoryType
+from arkindex.documents.models import Corpus
 from arkindex.project.tests import FixtureAPITestCase
 from arkindex.users.models import Group, Right, Role, User
 
@@ -121,33 +122,60 @@ class TestMembership(FixtureAPITestCase):
                 '__all__': ['Exactly one of those query parameters must be defined: group, corpus, repository.']
             })
 
-    def test_list_members_public_group(self):
+    def test_list_members_public_content(self):
         """
-        Any user may be able to list all members on a public group even if he is not a member
+        Any user may be able to list all members on a public content (except for groups in the test above)
         """
+        corpus = Corpus.objects.create(name='Public corpus', public=True)
+        user_right = Right.objects.create(user=self.user, content_object=corpus, level=42)
+        group_right = Right.objects.create(group=self.group, content_object=corpus, level=77)
         self.client.force_login(self.user)
         with self.assertNumQueries(8):
-            response = self.client.get(reverse('api:memberships-list'), {'group': str(self.admin_group.id)})
+            response = self.client.get(reverse('api:memberships-list'), {'corpus': str(corpus.id)})
         self.assertEqual(response.status_code, status.HTTP_200_OK)
-        self.assertEqual(response.json(), {
-            'count': 1,
+        self.assertDictEqual(response.json(), {
+            'count': 2,
             'next': None,
             'number': 1,
             'previous': None,
             'results': [
                 {
-                    'id': str(self.admin_membership.id),
-                    'level': Role.Admin.value,
+                    'id': str(user_right.id),
+                    'level': 42,
                     'user': {
-                        'display_name': self.admin.display_name,
-                        'email': self.admin.email,
-                        'id': self.admin.id,
+                        'display_name': self.user.display_name,
+                        'email': self.user.email,
+                        'id': self.user.id,
                     },
                     'group': None,
+                },
+                {
+                    'id': str(group_right.id),
+                    'level': 77,
+                    'user': None,
+                    'group': {
+                        'id': str(self.group.id),
+                        'members_count': self.group.memberships.count(),
+                        'name': self.group.name,
+                        'public': self.group.public,
+                    },
                 }
             ]
         })
 
+    def test_list_members_public_group(self):
+        """
+        Any user may not be able to list all members on a public group
+        This behavior is specific to groups
+        """
+        self.client.force_login(self.user)
+        with self.assertNumQueries(5):
+            response = self.client.get(reverse('api:memberships-list'), {'group': str(self.admin_group.id)})
+        self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
+        self.assertDictEqual(response.json(), {
+            'detail': 'You do not have the required access level to list members for this content.'
+        })
+
     def test_list_members_level_right(self):
         """
         List members of a group with their right corresponding to privileges level
@@ -230,14 +258,20 @@ class TestMembership(FixtureAPITestCase):
             ]
         )
 
+    def test_retrieve_group_wrong_id(self):
+        self.client.force_login(self.user)
+        with self.assertNumQueries(4):
+            response = self.client.get(reverse('api:group-details', kwargs={'pk': str(uuid.uuid4())}))
+        self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
+
     def test_retrieve_group_no_member(self):
         """
         A non memeber of a group may not retrieve its details, even if it is public
         """
         self.client.force_login(self.user)
-        with self.assertNumQueries(4):
+        with self.assertNumQueries(5):
             response = self.client.get(reverse('api:group-details', kwargs={'pk': str(self.admin_group.id)}))
-        self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
+        self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)
 
     def test_retrieve_group_details(self):
         self.client.force_login(self.user)