OpenConceptLab/ocl_online#648 | Organization roles: admin and member

core/common/mixins.py

@@ -384,6 +384,7 @@ class PathWalkerMixin:
     A Mixin with methods that help resolve a resource path to a resource object
     """
     path_info = None
+    is_parent_lookup = False  # this view is only finding the parent of what the request is for
 
     @staticmethod
     def get_parent_in_path(path_info, levels=1):
@@ -404,7 +405,7 @@ def get_parent_in_path(path_info, levels=1):
     @staticmethod
     def get_object_for_path(path_info, request):
         callback, _, callback_kwargs = resolve(path_info)
-        view = callback.cls(request=request, kwargs=callback_kwargs)
+        view = callback.cls(request=request, kwargs=callback_kwargs, is_parent_lookup=True)
         view.initialize(request, path_info, **callback_kwargs)
         return view.get_object()
 

core/common/permissions.py

@@ -49,6 +49,15 @@ def has_object_permission(self, request, view, obj):
         return False
 
 
+class CanAdministerOrganization(BasePermission):
+    """
+    The user is staff or an admin of the referenced organization. Its repositories don't need this: any member
+    can edit them.
+    """
+    def has_object_permission(self, request, view, obj):
+        return obj.has_admin_access(request.user)
+
+
 class CanViewConceptDictionary(HasPrivateAccess):
     """
     The user can view this source. A version is viewable only while its HEAD is too, as a version keeps the

core/common/tests.py

@@ -323,6 +323,7 @@ def setUpClass(cls):
         call_command("loaddata", "core/fixtures/base_entities.yaml", "core/fixtures/ocl_org.yaml")
         call_command("loaddata", "core/fixtures/auth_groups.yaml")
         call_command("loaddata", "core/fixtures/capabilities.yaml")
+        call_command("loaddata", "core/fixtures/org_roles.yaml")
         call_command("loaddata", "core/fixtures/toggles.json")
         call_command("seed_group_capabilities", file=TEST_GROUPS_CONFIG_FILE)
         org = Organization.objects.get(id=1)
@@ -336,6 +337,7 @@ def setUpClass(cls):
         call_command("loaddata", "core/fixtures/base_entities.yaml", "core/fixtures/ocl_org.yaml")
         call_command("loaddata", "core/fixtures/auth_groups.yaml")
         call_command("loaddata", "core/fixtures/capabilities.yaml")
+        call_command("loaddata", "core/fixtures/org_roles.yaml")
         call_command("loaddata", "core/fixtures/toggles.json")
         call_command("seed_group_capabilities", file=TEST_GROUPS_CONFIG_FILE)
         org = Organization.objects.get(id=1)
@@ -353,6 +355,7 @@ def setUpTestData(cls):
         call_command("loaddata", "core/fixtures/base_entities.yaml", "core/fixtures/ocl_org.yaml")
         call_command("loaddata", "core/fixtures/auth_groups.yaml")
         call_command("loaddata", "core/fixtures/capabilities.yaml")
+        call_command("loaddata", "core/fixtures/org_roles.yaml")
         call_command("loaddata", "core/fixtures/toggles.json")
         call_command("seed_group_capabilities", file=TEST_GROUPS_CONFIG_FILE)
 

core/fixtures/org_roles.yaml

@@ -0,0 +1,10 @@
+- model: "orgs.role"
+  pk: 1
+  fields:
+    name: admin
+    description: Can edit or delete the organization, manage its members and change their roles
+- model: "orgs.role"
+  pk: 2
+  fields:
+    name: member
+    description: Can work on the organization's repositories, private ones included

core/importers/models.py

@@ -240,7 +240,7 @@ def process(self):
     def delete(self):
         if self.exists():
             org = self.get_queryset().first()
-            if self.user and (self.user.is_staff or org.is_member(self.user)):
+            if org.has_admin_access(self.user):
                 org.updated_by = self.user
                 org.save(update_fields=['updated_by'])
                 delete_organization(org.id)

core/importers/tests.py

@@ -2088,6 +2088,20 @@ def test_organization_importer_delete_permission_denied(self):
         self.assertEqual(importer.delete(), PERMISSION_DENIED)
         self.assertTrue(Organization.objects.filter(mnemonic=org.mnemonic).exists())
 
+    def test_organization_importer_delete_refuses_a_plain_member(self):
+        org = OrganizationFactory(mnemonic='MemberOrg')
+        member = UserProfileFactory(organizations=[org])
+
+        self.assertEqual(OrganizationImporter({'id': 'MemberOrg'}, member).delete(), PERMISSION_DENIED)
+        self.assertTrue(Organization.objects.filter(mnemonic='MemberOrg').exists())
+
+    def test_organization_importer_delete_by_an_admin(self):
+        admin = UserProfileFactory()
+        OrganizationFactory(mnemonic='AdminOrg', created_by=admin, updated_by=admin)
+
+        self.assertEqual(OrganizationImporter({'id': 'AdminOrg'}, admin).delete(), DELETED)
+        self.assertFalse(Organization.objects.filter(mnemonic='AdminOrg').exists())
+
     def test_organization_importer_does_not_create_org_whose_name_differs_only_in_case(self):
         OrganizationFactory(mnemonic='TwinOrg')
         importer = OrganizationImporter({'id': 'twinorg', 'name': 'Twin'}, UserProfileFactory())

core/integration_tests/tests_access_control.py

@@ -15,6 +15,7 @@
 from core.concepts.models import Concept
 from core.concepts.tests.factories import ConceptFactory, ConceptNameFactory, ConceptDescriptionFactory
 from core.map_projects.tests.factories import MapProjectFactory, AutomatchRunFactory
+from core.orgs.constants import ROLE_ADMIN
 from core.orgs.models import Organization
 from core.orgs.tests.factories import OrganizationFactory
 from core.pins.models import Pin
@@ -30,14 +31,16 @@
 
 class AccessTestMixin:
     """
-    Staff, an org with one member, the owner of user-owned resources, and an outsider: a logged-in user who belongs
-    to no org and owns nothing under test.
+    Staff, an org with one member and one admin, the owner of user-owned resources, and an outsider: a logged-in user
+    who belongs to no org and owns nothing under test.
     """
     def setUp(self):
         super().setUp()
         self.staff = UserProfile.objects.get(username='ocladmin')
         self.org = OrganizationFactory(public_access=ACCESS_TYPE_VIEW)
         self.member = UserProfileFactory(organizations=[self.org])
+        self.admin = UserProfileFactory()
+        self.org.set_member_role(self.admin, ROLE_ADMIN)
         self.owner = UserProfileFactory()
         self.outsider = UserProfileFactory()
 
@@ -68,8 +71,16 @@ def test_outsider_cannot_edit(self):
     def test_anonymous_cannot_edit(self):
         self.assert_not_renamed(self.rename(None), 401)
 
-    def test_member_can_edit(self):
-        self.assert_renamed(self.rename(self.member))
+    def test_member_cannot_edit(self):
+        self.assert_not_renamed(self.rename(self.member), 403)
+        self.assert_not_renamed(self.request('patch', self.org.uri, self.member, {'name': 'Renamed'}), 403)
+
+    def test_member_cannot_delete(self):
+        self.assertEqual(self.request('delete', self.org.uri + '?inline=true', self.member).status_code, 403)
+        self.assertTrue(Organization.objects.filter(id=self.org.id).exists())
+
+    def test_admin_can_edit(self):
+        self.assert_renamed(self.rename(self.admin))
 
     def test_staff_can_edit(self):
         self.assert_renamed(self.rename(self.staff))
@@ -118,8 +129,11 @@ def test_outsider_cannot_edit(self):
     def test_anonymous_cannot_edit(self):
         self.assert_not_edited(self.edit(None), 401)
 
-    def test_member_can_edit(self):
-        self.assert_edited(self.edit(self.member))
+    def test_member_cannot_edit(self):
+        self.assert_not_edited(self.edit(self.member), 403)
+
+    def test_admin_can_edit(self):
+        self.assert_edited(self.edit(self.admin))
 
     def test_staff_can_edit(self):
         self.assert_edited(self.edit(self.staff))
@@ -130,8 +144,16 @@ def test_outsider_cannot_delete(self):
     def test_anonymous_cannot_delete(self):
         self.assert_not_deleted(self.request('delete', self.get_url()), 401)
 
-    def test_member_and_staff_delete_clears_overview_only(self):
-        for user in [self.member, self.staff]:
+    def test_member_cannot_delete(self):
+        self.org.overview = {'text': 'Existing'}
+        self.org.save()
+
+        self.assertEqual(self.request('delete', self.get_url(), self.member).status_code, 403)
+        self.org.refresh_from_db()
+        self.assertEqual(self.org.overview, {'text': 'Existing'})
+
+    def test_admin_and_staff_delete_clears_overview_only(self):
+        for user in [self.admin, self.staff]:
             self.org.overview = {'text': 'Existing'}
             self.org.save()
 
@@ -179,22 +201,25 @@ def test_outsider_cannot_upload(self):
     def test_anonymous_cannot_upload(self):
         self.assert_not_uploaded(self.upload(None), 401)
 
-    def test_member_can_upload(self):
-        self.assert_uploaded(self.upload(self.member))
+    def test_member_cannot_upload(self):
+        self.assert_not_uploaded(self.upload(self.member), 403)
+
+    def test_admin_can_upload(self):
+        self.assert_uploaded(self.upload(self.admin))
 
     def test_staff_can_upload(self):
         self.assert_uploaded(self.upload(self.staff))
 
     def test_delete_clears_logo_only(self):
-        for user, status_code in [(None, 401), (self.outsider, 403)]:
+        for user, status_code in [(None, 401), (self.outsider, 403), (self.member, 403)]:
             self.org.logo_path = f'orgs/{self.org.mnemonic}/logo.png'
             self.org.save()
 
             self.assertEqual(self.request('delete', self.org.uri + 'logo/', user).status_code, status_code)
             self.org.refresh_from_db()
             self.assertEqual(self.org.logo_path, f'orgs/{self.org.mnemonic}/logo.png')
 
-        for user in [self.member, self.staff]:
+        for user in [self.admin, self.staff]:
             self.org.logo_path = f'orgs/{self.org.mnemonic}/logo.png'
             self.org.save()
 
@@ -747,10 +772,15 @@ def test_anonymous_cannot_edit_or_delete(self):
         self.assertEqual(self.delete(None).status_code, 401)
         self.assert_extras({'foo': 'bar'})
 
-    def test_member_can_edit_and_delete(self):
-        self.assertEqual(self.edit(self.member).status_code, 200)
+    def test_member_cannot_edit_or_delete(self):
+        self.assertEqual(self.edit(self.member).status_code, 403)
+        self.assertEqual(self.delete(self.member).status_code, 403)
+        self.assert_extras({'foo': 'bar'})
+
+    def test_admin_can_edit_and_delete(self):
+        self.assertEqual(self.edit(self.admin).status_code, 200)
         self.assert_extras({'foo': 'changed'})
-        self.assertEqual(self.delete(self.member).status_code, 204)
+        self.assertEqual(self.delete(self.admin).status_code, 204)
         self.assert_extras({})
 
     def test_staff_can_edit(self):
@@ -825,8 +855,8 @@ def test_staff_can_upload(self):
 
 class ClientConfigAccessTest(AccessTestMixin, OCLAPITestCase):
     """
-    Adding a config to an org needs staff or membership, and to a repo staff or edit access. Changing a config needs
-    staff or its creator.
+    Adding a config to an org needs staff or an admin of it, and to a repo staff or edit access. Changing a config
+    needs staff or its creator.
     """
     payload = {'name': 'Home', 'type': 'home', 'config': {'tabs': [{'type': 'concepts', 'default': True}]}}
 
@@ -850,7 +880,9 @@ def assert_cannot_create(self, resource):
 
     def test_create_on_org(self):
         self.assert_cannot_create(self.org)
-        self.assert_can_create(self.org, self.member)
+        self.assertEqual(self.create(self.org, self.member).status_code, 403)
+        self.assertFalse(self.org.client_configs.exists())
+        self.assert_can_create(self.org, self.admin)
         self.assert_can_create(self.org, self.staff)
 
     def test_create_on_source(self):
@@ -862,16 +894,16 @@ def test_create_on_collection(self):
         self.assert_can_create(self.collection, self.member)
 
     def test_edit(self):
-        config_id = self.assert_can_create(self.org, self.member)
+        config_id = self.assert_can_create(self.org, self.admin)
         url = f'/client-configs/{config_id}/'
 
-        for user in [self.outsider, self.owner]:
+        for user in [self.outsider, self.owner, self.member]:
             self.assertEqual(self.request('put', url, user, {'name': 'Renamed'}).status_code, 403)
             self.assertEqual(self.request('patch', url, user, {'name': 'Renamed'}).status_code, 403)
         self.assertEqual(self.request('put', url, None, {'name': 'Renamed'}).status_code, 401)
         self.assertEqual(ClientConfig.objects.get(id=config_id).name, 'Home')
 
-        self.assertEqual(self.request('put', url, self.member, {'name': 'Renamed'}).status_code, 200)
+        self.assertEqual(self.request('put', url, self.admin, {'name': 'Renamed'}).status_code, 200)
         self.assertEqual(ClientConfig.objects.get(id=config_id).name, 'Renamed')
         self.assertEqual(self.request('put', url, self.staff, {'name': 'Renamed again'}).status_code, 200)
         self.assertEqual(ClientConfig.objects.get(id=config_id).name, 'Renamed again')