diff --git a/core/common/mixins.py b/core/common/mixins.py index d15cda8e7..72f0386fb 100644 --- a/core/common/mixins.py +++ b/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() diff --git a/core/common/permissions.py b/core/common/permissions.py index 204ea6e6a..2375715d6 100644 --- a/core/common/permissions.py +++ b/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 diff --git a/core/common/tests.py b/core/common/tests.py index 2917a50ac..84ff48292 100644 --- a/core/common/tests.py +++ b/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) diff --git a/core/fixtures/org_roles.yaml b/core/fixtures/org_roles.yaml new file mode 100644 index 000000000..6d94a11ee --- /dev/null +++ b/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 diff --git a/core/importers/models.py b/core/importers/models.py index 0f4575730..88dfa737c 100644 --- a/core/importers/models.py +++ b/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) diff --git a/core/importers/tests.py b/core/importers/tests.py index 4b032094b..8b1580aba 100644 --- a/core/importers/tests.py +++ b/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()) diff --git a/core/integration_tests/tests_access_control.py b/core/integration_tests/tests_access_control.py index 7da91566f..d6eb447e1 100644 --- a/core/integration_tests/tests_access_control.py +++ b/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,14 +201,17 @@ 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() @@ -194,7 +219,7 @@ def test_delete_clears_logo_only(self): 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') diff --git a/core/integration_tests/tests_orgs.py b/core/integration_tests/tests_orgs.py index 2a758b0b0..a778a0318 100644 --- a/core/integration_tests/tests_orgs.py +++ b/core/integration_tests/tests_orgs.py @@ -12,6 +12,8 @@ from core.collections.tests.factories import OrganizationCollectionFactory, UserCollectionFactory from core.common.constants import ACCESS_TYPE_NONE, ACCESS_TYPE_VIEW, RETIRED_ACCESS_TYPE_EDIT from core.common.tests import OCLAPITestCase, OCLAPITransactionTestCase +from core.events.models import Event +from core.orgs.constants import ROLE_ADMIN, ROLE_MEMBER from core.orgs.models import Organization from core.orgs.tests.factories import OrganizationFactory from core.sources.documents import SourceDocument @@ -175,7 +177,8 @@ def test_post_400(self): class OrganizationDetailViewTest(OCLAPITestCase): def setUp(self): self.org = OrganizationFactory(name='Stark Enterprises') - self.user = UserProfileFactory(organizations=[self.org]) + self.user = UserProfileFactory() + self.org.set_member_role(self.user, ROLE_ADMIN) self.superuser = UserProfile.objects.get(is_superuser=True) self.token = self.user.get_token() @@ -534,11 +537,12 @@ def test_delete_204(self): def test_delete_204_creator(self): creator = UserProfileFactory() org = OrganizationFactory(created_by=creator, updated_by=creator) - member = UserProfileFactory(organizations=[org]) + other_admin = UserProfileFactory() + org.set_member_role(other_admin, ROLE_ADMIN) response = self.client.delete( f'/orgs/{org.mnemonic}/members/{creator.username}/', - HTTP_AUTHORIZATION='Token ' + member.get_token(), + HTTP_AUTHORIZATION='Token ' + other_admin.get_token(), format='json' ) @@ -548,11 +552,12 @@ def test_delete_204_creator(self): def test_delete_204_last_editor(self): self.org.updated_by = self.user self.org.save() - member = UserProfileFactory(organizations=[self.org]) + admin = UserProfileFactory() + self.org.set_member_role(admin, ROLE_ADMIN) response = self.client.delete( f'/orgs/{self.org.mnemonic}/members/{self.user.username}/', - HTTP_AUTHORIZATION='Token ' + member.get_token(), + HTTP_AUTHORIZATION='Token ' + admin.get_token(), format='json' ) @@ -601,11 +606,11 @@ def test_delete_404(self): class OrganizationMemberRemovalConcurrencyTest(OCLAPITransactionTestCase): """Runs two removals at once, on separate connections, to check that every org keeps a member.""" - def test_two_members_removing_each_other_at_once_leave_one_member(self): + def test_two_members_leaving_at_once_leave_one_member(self): alice = UserProfileFactory(username='alice') bob = UserProfileFactory(username='bob') org = OrganizationFactory(mnemonic='race-org', created_by=alice, updated_by=alice) - org.members.add(bob) + org.set_member_role(bob, ROLE_ADMIN) tokens = {'alice': alice.get_token(), 'bob': bob.get_token()} is_only_member = Organization.is_only_member @@ -617,20 +622,20 @@ def slow_is_only_member(organization, user_profile): barrier = threading.Barrier(2) statuses = {} - def remove(actor, target): + def leave(actor): try: client = APIClient() barrier.wait() response = client.delete( - f'/orgs/race-org/members/{target}/', HTTP_AUTHORIZATION='Token ' + tokens[actor]) + f'/orgs/race-org/members/{actor}/', HTTP_AUTHORIZATION='Token ' + tokens[actor]) statuses[actor] = response.status_code finally: connection.close() with patch.object(Organization, 'is_only_member', slow_is_only_member): threads = [ - threading.Thread(target=remove, args=('alice', 'bob')), - threading.Thread(target=remove, args=('bob', 'alice')), + threading.Thread(target=leave, args=('alice', )), + threading.Thread(target=leave, args=('bob', )), ] for thread in threads: thread.start() @@ -641,6 +646,292 @@ def remove(actor, target): self.assertEqual(org.members.count(), 1) +class OrganizationMemberRepositoryAccessTest(OCLAPITestCase): + """A plain member keeps everything they have on the org's repositories, private ones included.""" + def setUp(self): + super().setUp() + self.org = OrganizationFactory(mnemonic='stark', public_access=ACCESS_TYPE_NONE) + self.member = UserProfileFactory(organizations=[self.org]) + self.source = OrganizationSourceFactory( + organization=self.org, mnemonic='private-source', public_access=ACCESS_TYPE_NONE) + self.auth = {'HTTP_AUTHORIZATION': 'Token ' + self.member.get_token()} + + def test_views_the_private_org_and_its_private_repository(self): + self.assertEqual(self.client.get('/orgs/stark/', **self.auth).status_code, 200) + self.assertEqual(self.client.get('/orgs/stark/sources/private-source/', **self.auth).status_code, 200) + + def test_creates_a_repository(self): + response = self.client.post( + '/orgs/stark/sources/', + {'id': 'new-source', 'name': 'New', 'short_code': 'new', 'public_access': ACCESS_TYPE_NONE}, + format='json', **self.auth + ) + + self.assertEqual(response.status_code, 201) + + def test_edits_a_private_repository(self): + collection = OrganizationCollectionFactory( + organization=self.org, mnemonic='private-collection', public_access=ACCESS_TYPE_NONE) + + for method, url, repo in ( + ('put', '/orgs/stark/sources/private-source/', self.source), + ('patch', '/orgs/stark/sources/private-source/', self.source), + ('put', '/orgs/stark/collections/private-collection/', collection), + ('patch', '/orgs/stark/collections/private-collection/', collection), + ): + with self.subTest(method=method, url=url): + name = f'Renamed by {method}' + + response = getattr(self.client, method)(url, {'name': name}, format='json', **self.auth) + + self.assertEqual(response.status_code, 200) + repo.refresh_from_db() + self.assertEqual(repo.name, name) + + def test_manages_exports_of_a_repository(self): + self.assertTrue(self.member.is_admin_for(self.source)) + self.assertFalse(UserProfileFactory().is_admin_for(self.source)) + + def test_is_refused_outside_the_org(self): + stranger = {'HTTP_AUTHORIZATION': 'Token ' + UserProfileFactory().get_token()} + + self.assertEqual(self.client.get('/orgs/stark/sources/private-source/', **stranger).status_code, 403) + + +class OrganizationMemberRoleTest(OCLAPITestCase): + def setUp(self): + super().setUp() + self.admin = UserProfileFactory() + self.org = OrganizationFactory(mnemonic='stark', created_by=self.admin, updated_by=self.admin) + self.member = UserProfileFactory(organizations=[self.org]) + + def put(self, target, data=None, actor=None): + return self.client.put( + f'/orgs/stark/members/{target.username}/', data, format='json', + HTTP_AUTHORIZATION='Token ' + (actor or self.admin).get_token()) + + def delete(self, target, actor=None): + return self.client.delete( + f'/orgs/stark/members/{target.username}/', HTTP_AUTHORIZATION='Token ' + (actor or self.admin).get_token()) + + def get_role(self, user): + return self.org.get_member_role(user) + + def test_put_adds_a_new_member_as_member_by_default(self): + user = UserProfileFactory() + + response = self.put(user) + + self.assertEqual(response.status_code, 204) + self.assertEqual(self.get_role(user), ROLE_MEMBER) + self.assertEqual(Event.objects.filter(event_type=Event.JOINED, object_url=user.uri).count(), 1) + + def test_put_adds_a_new_member_as_admin(self): + user = UserProfileFactory() + + self.assertEqual(self.put(user, {'role': ROLE_ADMIN}).status_code, 204) + self.assertEqual(self.get_role(user), ROLE_ADMIN) + + def test_put_without_a_role_keeps_an_existing_admin_as_admin(self): + self.org.set_member_role(self.member, ROLE_ADMIN) + + self.assertEqual(self.put(self.member).status_code, 204) + self.assertEqual(self.get_role(self.member), ROLE_ADMIN) + + def test_put_on_an_existing_member_records_no_second_joined_event(self): + self.assertEqual(self.put(self.member, {'role': ROLE_ADMIN}).status_code, 204) + self.assertEqual(self.put(self.member).status_code, 204) + + self.assertFalse(Event.objects.filter(event_type=Event.JOINED, object_url=self.member.uri).exists()) + + def test_put_promotes_and_demotes(self): + self.assertEqual(self.put(self.member, {'role': ROLE_ADMIN}).status_code, 204) + self.assertEqual(self.get_role(self.member), ROLE_ADMIN) + + self.assertEqual(self.put(self.member, {'role': ROLE_MEMBER}).status_code, 204) + self.assertEqual(self.get_role(self.member), ROLE_MEMBER) + + def test_put_400_for_a_role_that_does_not_exist(self): + user = UserProfileFactory() + for role in ('owner', '', 1, ['admin'], {'name': 'admin'}): + with self.subTest(role=role): + response = self.put(user, {'role': role}) + + self.assertEqual(response.status_code, 400) + self.assertEqual(list(response.data), ['role']) + self.assertFalse(self.org.is_member(user)) + + def test_put_403_for_a_member(self): + user = UserProfileFactory() + + self.assertEqual(self.put(user, actor=self.member).status_code, 403) + self.assertFalse(self.org.is_member(user)) + + def test_put_403_for_a_member_promoting_themselves(self): + self.assertEqual(self.put(self.member, {'role': ROLE_ADMIN}, actor=self.member).status_code, 403) + self.assertEqual(self.get_role(self.member), ROLE_MEMBER) + + def test_put_by_staff_who_is_not_a_member(self): + staff = UserProfileFactory(is_staff=True) + + self.assertEqual(self.put(self.member, {'role': ROLE_ADMIN}, actor=staff).status_code, 204) + self.assertEqual(self.get_role(self.member), ROLE_ADMIN) + self.assertFalse(self.org.is_member(staff)) + + def test_put_409_demoting_the_last_admin(self): + for actor in (self.admin, UserProfileFactory(is_staff=True)): + with self.subTest(actor=actor.username): + response = self.put(self.admin, {'role': ROLE_MEMBER}, actor=actor) + + self.assertEqual(response.status_code, 409) + self.assertEqual( + response.data, {'detail': 'Cannot remove or demote the last admin of an organization.'}) + self.assertEqual(self.get_role(self.admin), ROLE_ADMIN) + + def test_put_409_demoting_the_last_active_admin(self): + deactivated = UserProfileFactory(organizations=[self.org], is_active=False) + self.org.set_member_role(deactivated, ROLE_ADMIN) + + self.assertEqual(self.put(self.admin, {'role': ROLE_MEMBER}).status_code, 409) + + def test_delete_403_for_a_member_removing_someone_else(self): + other = UserProfileFactory(organizations=[self.org]) + + self.assertEqual(self.delete(other, actor=self.member).status_code, 403) + self.assertEqual(self.delete(self.admin, actor=self.member).status_code, 403) + self.assertTrue(self.org.is_member(other)) + self.assertTrue(self.org.is_member(self.admin)) + + def test_delete_409_for_the_last_admin(self): + for actor in (self.admin, UserProfileFactory(is_staff=True)): + with self.subTest(actor=actor.username): + response = self.delete(self.admin, actor=actor) + + self.assertEqual(response.status_code, 409) + self.assertEqual( + response.data, {'detail': 'Cannot remove or demote the last admin of an organization.'}) + self.assertTrue(self.org.is_member(self.admin)) + + def test_delete_removes_a_deactivated_admin(self): + deactivated = UserProfileFactory(organizations=[self.org], is_active=False) + self.org.set_member_role(deactivated, ROLE_ADMIN) + + self.assertEqual(self.delete(deactivated).status_code, 204) + self.assertFalse(self.org.is_member(deactivated)) + +class OrganizationMemberRoleReadTest(OCLAPITestCase): + def setUp(self): + super().setUp() + self.admin = UserProfileFactory(username='the-admin') + self.org = OrganizationFactory(mnemonic='stark', created_by=self.admin, updated_by=self.admin) + self.member = UserProfileFactory(username='the-member', organizations=[self.org]) + + def test_members_list_has_each_role_in_every_shape(self): + for params in ('', '?verbose=true', '?summary=true'): + with self.subTest(params=params): + response = self.client.get(f'/orgs/stark/members/{params}') + + self.assertEqual(response.status_code, 200) + self.assertEqual( + {row['username']: row['role'] for row in response.data}, + {'the-admin': 'admin', 'the-member': 'member'} + ) + + def test_role_is_from_this_org_not_another(self): + other = OrganizationFactory(mnemonic='other', created_by=self.member, updated_by=self.member) + other.members.add(self.admin) + + response = self.client.get('/orgs/other/members/') + + self.assertEqual({row['username']: row['role'] for row in response.data}, + {'the-admin': 'member', 'the-member': 'admin'}) + + def test_user_lists_outside_an_org_have_no_role(self): + response = self.client.get( + '/users/', HTTP_AUTHORIZATION='Token ' + self.admin.get_token()) + + self.assertEqual(response.status_code, 200) + self.assertTrue(response.data) + self.assertFalse(any('role' in row for row in response.data)) + + def test_org_detail_has_the_role_of_the_requester(self): + for user, expected in ( + (self.admin, 'admin'), + (self.member, 'member'), + (UserProfileFactory(), None), + (UserProfileFactory(is_staff=True), None), + ): + with self.subTest(user=user.username): + response = self.client.get('/orgs/stark/', HTTP_AUTHORIZATION='Token ' + user.get_token()) + + self.assertEqual(response.status_code, 200) + self.assertEqual(response.data['user_role'], expected) + + def test_org_detail_has_no_role_for_anonymous(self): + response = self.client.get('/orgs/stark/') + + self.assertEqual(response.status_code, 200) + self.assertIsNone(response.data['user_role']) + + def test_creating_an_org_makes_the_creator_its_admin(self): + user = UserProfileFactory() + + response = self.client.post( + '/orgs/', {'id': 'new-org', 'name': 'New'}, format='json', + HTTP_AUTHORIZATION='Token ' + user.get_token()) + + self.assertEqual(response.status_code, 201) + self.assertEqual(response.data['user_role'], 'admin') + self.assertTrue(Organization.objects.get(mnemonic='new-org').is_admin(user)) + + +class OrganizationDemotedAdminConcurrencyTest(OCLAPITransactionTestCase): + """A request that was waiting for the org's lock uses the rights its sender has once it gets the lock.""" + + def test_an_admin_being_demoted_cannot_promote_themselves_back(self): + alice = UserProfileFactory(username='alice') + bob = UserProfileFactory(username='bob') + org = OrganizationFactory(mnemonic='race-org', created_by=alice, updated_by=alice) + org.set_member_role(bob, ROLE_ADMIN) + tokens = {'alice': alice.get_token(), 'bob': bob.get_token()} + is_last_admin = Organization.is_last_admin + alice_holds_the_lock = threading.Event() + + def slow_is_last_admin(organization, user_profile): + result = is_last_admin(organization, user_profile) + alice_holds_the_lock.set() + time.sleep(1) # bob's request starts and waits for the lock + return result + + statuses = {} + + def send(actor, role): + try: + response = APIClient().put( + '/orgs/race-org/members/bob/', {'role': role}, format='json', + HTTP_AUTHORIZATION='Token ' + tokens[actor]) + statuses[actor] = response.status_code + finally: + connection.close() + + def promote_once_alice_holds_the_lock(): + alice_holds_the_lock.wait(timeout=5) + send('bob', ROLE_ADMIN) + + with patch.object(Organization, 'is_last_admin', slow_is_last_admin): + threads = [ + threading.Thread(target=send, args=('alice', ROLE_MEMBER)), + threading.Thread(target=promote_once_alice_holds_the_lock), + ] + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + self.assertEqual(statuses, {'alice': 204, 'bob': 403}) + self.assertEqual(org.get_member_role(bob), ROLE_MEMBER) + + class OrganizationExtrasViewTest(OCLAPITestCase): def test_get_200(self): org = OrganizationFactory(extras={}) @@ -673,7 +964,8 @@ def setUp(self): super().setUp() self.extras = {'foo': 'bar', 'tao': 'ching'} self.organization = OrganizationFactory(extras=self.extras) - self.user = UserProfileFactory(organizations=[self.organization]) + self.user = UserProfileFactory() + self.organization.set_member_role(self.user, ROLE_ADMIN) self.token = self.user.get_token() def test_get_200(self): @@ -753,7 +1045,8 @@ class OrganizationLogoViewTest(OCLAPITestCase): def setUp(self): super().setUp() self.organization = OrganizationFactory(mnemonic='org-1') - self.user = UserProfileFactory(organizations=[self.organization]) + self.user = UserProfileFactory() + self.organization.set_member_role(self.user, ROLE_ADMIN) self.token = self.user.get_token() @patch('core.services.storages.cloud.aws.S3.upload_base64') @@ -781,7 +1074,8 @@ def test_post_200(self, upload_base64_mock): class OrganizationOverviewViewTest(OCLAPITestCase): def setUp(self): self.org = OrganizationFactory(name='Stark Enterprises') - self.user = UserProfileFactory(organizations=[self.org]) + self.user = UserProfileFactory() + self.org.set_member_role(self.user, ROLE_ADMIN) self.superuser = UserProfile.objects.get(is_superuser=True) self.token = self.user.get_token() diff --git a/core/integration_tests/tests_users.py b/core/integration_tests/tests_users.py index 7eeaba2ac..b730e6ce4 100644 --- a/core/integration_tests/tests_users.py +++ b/core/integration_tests/tests_users.py @@ -8,6 +8,7 @@ from core.common.constants import ACCESS_TYPE_NONE, ACCESS_TYPE_VIEW, RETIRED_ACCESS_TYPE_EDIT from core.common.tests import OCLAPITestCase, PREVIEW_GROUP_NAME +from core.orgs.constants import ROLE_ADMIN, ROLE_MEMBER from core.orgs.documents import OrganizationDocument from core.orgs.tests.factories import OrganizationFactory from core.sources.tests.factories import OrganizationSourceFactory @@ -981,6 +982,42 @@ def test_hard_delete_204(self): self.assertFalse(UserProfile.objects.filter(username=random_user.username).exists()) +class UserHardDeleteOrganizationAdminTest(OCLAPITestCase): + def setUp(self): + super().setUp() + self.staff = UserProfileFactory(is_staff=True) + self.member = UserProfileFactory() + self.org = OrganizationFactory(mnemonic='stark', created_by=self.member, updated_by=self.member) + self.org.set_member_role(self.member, ROLE_MEMBER) + self.user = UserProfileFactory() + self.org.set_member_role(self.user, ROLE_ADMIN) + + def delete(self, params='?hardDelete=true'): + return self.client.delete( + f'/users/{self.user.username}/{params}', HTTP_AUTHORIZATION='Token ' + self.staff.get_token()) + + def test_refuses_the_last_admin_of_an_org(self): + response = self.delete() + + self.assertEqual(response.status_code, 409) + self.assertEqual(response.data, { + 'detail': 'Cannot remove or demote the last admin of an organization.', 'orgs': ['stark'] + }) + self.assertTrue(UserProfile.objects.filter(id=self.user.id).exists()) + + def test_allows_an_admin_when_another_remains(self): + self.org.set_member_role(self.member, ROLE_ADMIN) + + self.assertEqual(self.delete().status_code, 204) + self.assertFalse(UserProfile.objects.filter(id=self.user.id).exists()) + + def test_deactivating_the_last_admin_stays_possible(self): + self.assertEqual(self.delete(params='').status_code, 204) + self.user.refresh_from_db() + self.assertFalse(self.user.is_active) + self.assertTrue(self.org.is_admin(self.user)) + + class UserReactivateViewTest(OCLAPITestCase): def setUp(self): super().setUp() diff --git a/core/orgs/constants.py b/core/orgs/constants.py index 9db9f2359..96efb9908 100644 --- a/core/orgs/constants.py +++ b/core/orgs/constants.py @@ -4,3 +4,18 @@ DELETE_ACCEPTED = 'Successfully accepted deleted Organization.' NO_MEMBERS = 'noMembers' CANNOT_REMOVE_ONLY_MEMBER = 'Cannot remove the only member of an organization.' +CANNOT_REMOVE_LAST_ADMIN = 'Cannot remove or demote the last admin of an organization.' + +INVALID_ROLE = 'Must be admin or member.' + +ROLE_ADMIN = 'admin' +ROLE_MEMBER = 'member' + +ROLE_ADMIN_ID = 1 +ROLE_MEMBER_ID = 2 + +ROLE_ID_BY_NAME = { + ROLE_ADMIN: ROLE_ADMIN_ID, + ROLE_MEMBER: ROLE_MEMBER_ID, +} +ROLES = tuple(ROLE_ID_BY_NAME) diff --git a/core/orgs/migrations/0023_role_organizationmember.py b/core/orgs/migrations/0023_role_organizationmember.py new file mode 100644 index 000000000..f3ca71cfa --- /dev/null +++ b/core/orgs/migrations/0023_role_organizationmember.py @@ -0,0 +1,60 @@ +import django.db.models.deletion +from django.conf import settings +from django.db import migrations, models + + +def seed_roles(apps, _): + """Only what the new column needs. load_fixtures fills in the rest from core/fixtures/org_roles.yaml.""" + role = apps.get_model('orgs', 'Role') + role.objects.bulk_create([role(id=1, name='admin'), role(id=2, name='member')]) + + +class Migration(migrations.Migration): + + dependencies = [ + ('users', '0037_userprofile_sso_id'), + ('orgs', '0022_retire_public_edit_access'), + ] + + operations = [ + migrations.CreateModel( + name='Role', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('name', models.CharField(max_length=100, unique=True)), + ('description', models.TextField(blank=True, default='')), + ], + options={ + 'db_table': 'org_roles', + }, + ), + migrations.RunPython(seed_roles, migrations.RunPython.noop), + # The table exists already, as the automatic many-to-many table of UserProfile.organizations. + migrations.SeparateDatabaseAndState( + state_operations=[ + migrations.CreateModel( + name='OrganizationMember', + fields=[ + ('id', models.AutoField( + auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('organization', models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, related_name='memberships', + to='orgs.organization')), + ('userprofile', models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, to=settings.AUTH_USER_MODEL)), + ], + options={ + 'db_table': 'user_profiles_organizations', + 'unique_together': {('userprofile', 'organization')}, + }, + ), + ], + ), + # Every existing member becomes an admin, so that nobody loses what they can do today. + migrations.AddField( + model_name='organizationmember', + name='role', + field=models.ForeignKey( + db_default=1, default=2, on_delete=django.db.models.deletion.PROTECT, to='orgs.role'), + ), + ] diff --git a/core/orgs/models.py b/core/orgs/models.py index 4035e59df..1c99df8d4 100644 --- a/core/orgs/models.py +++ b/core/orgs/models.py @@ -8,10 +8,32 @@ from core.common.constants import NAMESPACE_REGEX, ACCESS_TYPE_VIEW from core.common.mixins import SourceContainerMixin from core.common.models import BaseResourceModel -from core.orgs.constants import ORG_OBJECT_TYPE +from core.orgs.constants import ORG_OBJECT_TYPE, ROLE_ADMIN_ID, ROLE_ID_BY_NAME, ROLE_MEMBER_ID from core.users.models import Follow +class Role(models.Model): + """What a member may do in an organization. The rows are in core/fixtures/org_roles.yaml.""" + class Meta: + db_table = 'org_roles' + + name = models.CharField(max_length=100, unique=True) + description = models.TextField(blank=True, default='') + + +class OrganizationMember(models.Model): + """A user's membership of an organization, with their role in it.""" + class Meta: + db_table = 'user_profiles_organizations' + unique_together = ('userprofile', 'organization') + + userprofile = models.ForeignKey('users.UserProfile', on_delete=models.CASCADE) + organization = models.ForeignKey('orgs.Organization', on_delete=models.CASCADE, related_name='memberships') + # A new member is a plain member. Only a row inserted without a role gets the column's default: code older than + # this column, as during a rollout or after a rollback, where any member could do everything an admin can. + role = models.ForeignKey(Role, on_delete=models.PROTECT, default=ROLE_MEMBER_ID, db_default=ROLE_ADMIN_ID) + + class Organization(BaseResourceModel, SourceContainerMixin, ChecksumModel): class Meta: db_table = 'organizations' @@ -70,6 +92,38 @@ def has_edit_access(self, user): return False return user.is_staff or self.is_member(user) + def is_admin(self, user_profile): + return self.memberships.filter(userprofile_id=user_profile.id, role_id=ROLE_ADMIN_ID).exists() + + def has_admin_access(self, user): + """Whether `user` may edit or delete the org and manage its members: staff and its admins.""" + if not (user and user.is_authenticated): + return False + return user.is_staff or self.is_admin(user) + + def is_last_admin(self, user_profile): + """True if no other active admin would remain without `user_profile`. Every org keeps at least one.""" + return self.is_admin(user_profile) and not self.memberships.filter( + role_id=ROLE_ADMIN_ID, userprofile__is_active=True).exclude(userprofile_id=user_profile.id).exists() + + def get_member_role(self, user_profile): + """The name of the role `user_profile` has in the org, or None if they aren't a member.""" + return self.memberships.filter( + userprofile_id=get(user_profile, 'id')).values_list('role__name', flat=True).first() + + @property + def member_roles(self): + return dict(self.memberships.values_list('userprofile_id', 'role__name')) + + def set_member_role(self, user_profile, role=None): + """ + Adds `user_profile` to the org or changes their role, and returns whether they're new to it. With no role, + a new member gets the default one and an existing member keeps theirs. + """ + _, created = self.memberships.update_or_create( + userprofile=user_profile, defaults={'role_id': ROLE_ID_BY_NAME[role]} if role else {}) + return created + def is_only_member(self, user_profile): """True if `user_profile` is the org's only member. Every org keeps at least one member.""" return bool(self.is_member(user_profile)) and not self.members.exclude(id=user_profile.id).exists() @@ -98,14 +152,14 @@ def get_visible_to(cls, user, queryset): def save(self, *args, force_insert=False, force_update=False, using=None, update_fields=None): """ - The creator becomes a member when the org is created. Later saves leave the members alone, so editing + The creator becomes an admin when the org is created. Later saves leave the members alone, so editing an org doesn't make the editor a member and removing a member sticks. """ is_new = not self.id super().save(*args, force_insert=force_insert, force_update=force_update, using=using, update_fields=update_fields) if is_new: - self.members.add(self.created_by) + self.members.add(self.created_by, through_defaults={'role_id': ROLE_ADMIN_ID}) def delete(self, using=None, keep_parents=False, sync=False): # pylint: disable=arguments-differ with transaction.atomic(): diff --git a/core/orgs/serializers.py b/core/orgs/serializers.py index b12f8db37..c2a52b767 100644 --- a/core/orgs/serializers.py +++ b/core/orgs/serializers.py @@ -91,6 +91,7 @@ class OrganizationDetailSerializer(AbstractResourceSerializer): members_url = serializers.URLField(read_only=True) extras = serializers.JSONField(required=False, allow_null=True) client_configs = SerializerMethodField() + user_role = SerializerMethodField() class Meta: model = Organization @@ -98,7 +99,7 @@ class Meta: 'type', 'uuid', 'id', 'public_access', 'name', 'company', 'website', 'location', 'members', 'created_on', 'updated_on', 'url', 'extras', 'members_url', 'created_by', 'updated_by', 'location', 'sources_url', 'public_sources', 'collections_url', 'public_collections', 'logo_url', 'description', - 'client_configs', 'text', 'overview' + 'client_configs', 'text', 'overview', 'user_role' ) def __init__(self, *args, **kwargs): @@ -119,6 +120,10 @@ def get_client_configs(self, obj): return ClientConfigSerializer(obj.client_configs.filter( is_active=True), many=True).data if self.include_client_configs else None + def get_user_role(self, obj): + """The requester's role in the org, for clients to decide which controls to show.""" + return obj.get_member_role(get(self, 'context.request.user')) + def update(self, instance, validated_data): request_user = self.context['request'].user instance.description = validated_data.get('description', instance.description) diff --git a/core/orgs/tests/tests.py b/core/orgs/tests/tests.py index 61a5eec05..66a893a05 100644 --- a/core/orgs/tests/tests.py +++ b/core/orgs/tests/tests.py @@ -3,6 +3,7 @@ from django.apps import apps from django.contrib.auth.models import AnonymousUser from django.core.exceptions import ValidationError +from django.db import connection from mock import patch, Mock from core.collections.models import Collection, CollectionReference, Expansion @@ -13,7 +14,7 @@ from core.concepts.tests.factories import ConceptFactory from core.mappings.models import Mapping from core.mappings.tests.factories import MappingFactory -from core.orgs.constants import ORG_OBJECT_TYPE +from core.orgs.constants import ORG_OBJECT_TYPE, ROLE_ADMIN from core.orgs.documents import OrganizationDocument from core.orgs.models import Organization from core.orgs.tests.factories import OrganizationFactory @@ -52,6 +53,32 @@ def test_has_edit_access(self): for user in (member, outsider): self.assertEqual(source.has_parent_edit_access(user), org.has_edit_access(user)) + def test_has_admin_access(self): + org = OrganizationFactory() + member, admin, outsider = UserProfileFactory(), UserProfileFactory(), UserProfileFactory() + org.members.add(member) + org.set_member_role(admin, ROLE_ADMIN) + + self.assertTrue(org.has_admin_access(admin)) + self.assertTrue(org.has_admin_access(UserProfileFactory(is_staff=True))) + self.assertFalse(org.has_admin_access(member)) + self.assertFalse(org.has_admin_access(outsider)) + self.assertFalse(org.has_admin_access(AnonymousUser())) + self.assertFalse(org.has_admin_access(None)) + + def test_an_insert_that_names_no_role_is_an_admin(self): + """Code older than the role column inserts this way, during a rollout or after a rollback.""" + org = OrganizationFactory() + user = UserProfileFactory() + + with connection.cursor() as cursor: + cursor.execute( + 'INSERT INTO user_profiles_organizations (userprofile_id, organization_id) VALUES (%s, %s)', + [user.id, org.id] + ) + + self.assertTrue(org.is_admin(user)) + def test_members(self): org = Organization(id=123) self.assertEqual(org.members.count(), 0) diff --git a/core/orgs/views.py b/core/orgs/views.py index 28175aab2..9f3560cf8 100644 --- a/core/orgs/views.py +++ b/core/orgs/views.py @@ -13,14 +13,15 @@ from core.collections.views import CollectionListView from core.common.constants import NOT_FOUND, MUST_SPECIFY_EXTRA_PARAM_IN_BODY, HEAD, UPDATED_BY_USERNAME_PARAM from core.common.mixins import ListWithHeadersMixin -from core.common.permissions import HasPrivateAccess, CanViewConceptDictionary +from core.common.permissions import CanAdministerOrganization, CanViewConceptDictionary, HasPrivateAccess from core.common.swagger_parameters import org_no_members_param from core.common.tasks import delete_organization from core.common.throttling import ThrottleUtil from core.common.utils import parse_updated_since_param, get_truthy_values from core.common.views import BaseAPIView, BaseLogoView from core.map_projects.views import MapProjectListView -from core.orgs.constants import NO_MEMBERS, CANNOT_REMOVE_ONLY_MEMBER +from core.orgs.constants import NO_MEMBERS, CANNOT_REMOVE_ONLY_MEMBER, CANNOT_REMOVE_LAST_ADMIN, INVALID_ROLE, \ + ROLES, ROLE_MEMBER from core.orgs.documents import OrganizationDocument from core.orgs.models import Organization from core.orgs.serializers import OrganizationDetailSerializer, OrganizationListSerializer, \ @@ -109,11 +110,16 @@ class OrganizationBaseView(BaseAPIView, RetrieveAPIView, DestroyAPIView): queryset = Organization.objects.filter(is_active=True) def get_permissions(self): - """Reads follow the org's public access. Writes need staff or membership of the org.""" + """ + Reads follow the org's public access. Writes need staff or an admin of the org. Editing one of the org's + repositories looks the org up here with the same request, and needs only a member. + """ if self.request.method in ['GET', 'HEAD']: return [CanViewConceptDictionary(), ] + if self.is_parent_lookup: + return [HasPrivateAccess(), ] - return [HasPrivateAccess(), ] + return [CanAdministerOrganization(), ] class OrganizationLogoView(OrganizationBaseView, BaseLogoView): @@ -193,7 +199,7 @@ class OrganizationClientConfigsView(ResourceClientConfigsView): lookup_field = 'org' model = Organization queryset = Organization.objects.filter(is_active=True) - edit_permission_class = HasPrivateAccess + edit_permission_class = CanAdministerOrganization class OrganizationMemberView(generics.GenericAPIView): @@ -233,32 +239,52 @@ def get(self, request, *args, **kwargs): return Response(status=status.HTTP_404_NOT_FOUND) # pragma: no cover def put(self, request, **kwargs): # pylint: disable=unused-argument + """Adds the user to the org or changes their role. With no role, an existing member keeps theirs.""" if not self.userprofile: return Response(status=status.HTTP_404_NOT_FOUND) - if not self.user_in_org: - return Response(status=status.HTTP_403_FORBIDDEN) - self.userprofile.organizations.add(self.organization) - # ES Index - self.organization.save() - self.userprofile.save() - from core.events.models import Event - Event.record(self.organization, Event.JOINED, self.userprofile, request.user) + role = get(request.data, 'role') + with transaction.atomic(): + # Locks the org so that concurrent changes can't each see another admin and leave it with none, or act on + # rights their sender has just lost. + Organization.objects.select_for_update().get(id=self.organization.id) + if not self.organization.has_admin_access(request.user): + return Response(status=status.HTTP_403_FORBIDDEN) + if role is not None and role not in ROLES: + return Response({'role': [INVALID_ROLE]}, status=status.HTTP_400_BAD_REQUEST) + if role == ROLE_MEMBER and self.organization.is_last_admin(self.userprofile): + return Response({'detail': CANNOT_REMOVE_LAST_ADMIN}, status=status.HTTP_409_CONFLICT) + joined = self.organization.set_member_role(self.userprofile, role) + + if joined: + # ES Index + self.organization.save() + self.userprofile.save() + from core.events.models import Event + Event.record(self.organization, Event.JOINED, self.userprofile, request.user) return Response(status=status.HTTP_204_NO_CONTENT) def delete(self, request, **kwargs): # pylint: disable=unused-argument - """Removes the user from the org, unless they are its only member: every org keeps at least one.""" + """ + Removes the user from the org. An admin can remove anyone and a member can leave, unless that would leave + the org with no member or no admin. + """ if not request.user.is_staff and not self.user_in_org: return Response(status=status.HTTP_403_FORBIDDEN) if not self.organization or not self.userprofile: return Response(status=status.HTTP_404_NOT_FOUND) with transaction.atomic(): - # Locks the org so that concurrent removals can't each see another member and leave it with none. + # Locks the org so that concurrent changes can't each see another member or admin and leave it with none, + # or act on rights their sender has just lost. Organization.objects.select_for_update().get(id=self.organization.id) + if request.user != self.userprofile and not self.organization.has_admin_access(request.user): + return Response(status=status.HTTP_403_FORBIDDEN) if self.organization.is_only_member(self.userprofile): return Response({'detail': CANNOT_REMOVE_ONLY_MEMBER}, status=status.HTTP_409_CONFLICT) + if self.organization.is_last_admin(self.userprofile): + return Response({'detail': CANNOT_REMOVE_LAST_ADMIN}, status=status.HTTP_409_CONFLICT) self.userprofile.organizations.remove(self.organization) # ES Index @@ -306,11 +332,11 @@ def get_throttles(self): return ThrottleUtil.get_throttles_by_user_plan(self.request.user) def get_permissions(self): - """Reads follow the org's public access. Writes need staff or membership of the org.""" + """Reads follow the org's public access. Writes need staff or an admin of the org.""" if self.request.method in ['GET', 'HEAD']: return [IsAuthenticated(), CanViewConceptDictionary()] - return [IsAuthenticated(), HasPrivateAccess()] + return [IsAuthenticated(), CanAdministerOrganization()] def get_object(self): instance = Organization.objects.filter(is_active=True, mnemonic=self.kwargs['org']).first() diff --git a/core/users/migrations/0038_alter_userprofile_organizations.py b/core/users/migrations/0038_alter_userprofile_organizations.py new file mode 100644 index 000000000..9c8f59266 --- /dev/null +++ b/core/users/migrations/0038_alter_userprofile_organizations.py @@ -0,0 +1,23 @@ +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('orgs', '0023_role_organizationmember'), + ('users', '0037_userprofile_sso_id'), + ] + + operations = [ + # The table doesn't change: orgs 0023 has already taken it over as OrganizationMember. + migrations.SeparateDatabaseAndState( + state_operations=[ + migrations.AlterField( + model_name='userprofile', + name='organizations', + field=models.ManyToManyField( + related_name='members', through='orgs.OrganizationMember', to='orgs.organization'), + ), + ], + ), + ] diff --git a/core/users/models.py b/core/users/models.py index 947561ad4..21f339a90 100644 --- a/core/users/models.py +++ b/core/users/models.py @@ -56,7 +56,8 @@ class Meta: OBJECT_TYPE = USER_OBJECT_TYPE first_name = models.CharField(max_length=100, blank=True) last_name = models.CharField(max_length=100, blank=True) - organizations = models.ManyToManyField('orgs.Organization', related_name='members') + organizations = models.ManyToManyField( + 'orgs.Organization', related_name='members', through='orgs.OrganizationMember') company = models.TextField(null=True, blank=True) sso_id = models.CharField(max_length=64, null=True, blank=True, unique=True) location = models.TextField(null=True, blank=True) @@ -170,6 +171,7 @@ def set_token(self, token): Token.objects.create(user=self, key=token) def is_admin_for(self, concept_container): # pragma: no cover + """Whether the user owns a repository or is any member of the org that does. Not an org's admin role.""" user_id = concept_container.user_id if user_id and user_id == self.id: return True @@ -187,6 +189,11 @@ def __delete_token(self): def orgs_count(self): return self.organizations.count() + @property + def orgs_needing_this_admin(self): + """The orgs that would be left with no active admin without this user.""" + return [org for org in self.organizations.all() if org.is_last_admin(self)] + @property def owned_orgs_count(self): return self.organizations.filter(created_by=self).count() diff --git a/core/users/serializers.py b/core/users/serializers.py index 9c8f26a34..d55f91f44 100644 --- a/core/users/serializers.py +++ b/core/users/serializers.py @@ -39,7 +39,17 @@ def to_representation(self, instance): return data -class UserListSerializer(AbstractResourceSerializer): +class OrganizationRoleMixin: + """On an org's member list, each member's role in that org.""" + def to_representation(self, instance): + data = super().to_representation(instance) + member_roles = get(self, 'context.member_roles') + if member_roles is not None: + data['role'] = member_roles.get(instance.id) + return data + + +class UserListSerializer(OrganizationRoleMixin, AbstractResourceSerializer): type = serializers.CharField(source='resource_type', read_only=True) class Meta: @@ -49,7 +59,7 @@ class Meta: ) -class UserSummarySerializer(PrivateUserFieldsMixin, serializers.ModelSerializer): +class UserSummarySerializer(OrganizationRoleMixin, PrivateUserFieldsMixin, serializers.ModelSerializer): sources = IntegerField(source='public_sources') collections = IntegerField(source='public_collections') organizations = IntegerField(source='orgs_count') @@ -174,7 +184,7 @@ def get_object(obj): return following.get_brief_serializer()(following).data -class UserDetailSerializer(PrivateUserFieldsMixin, AbstractResourceSerializer): +class UserDetailSerializer(OrganizationRoleMixin, PrivateUserFieldsMixin, AbstractResourceSerializer): type = serializers.CharField(source='resource_type', read_only=True) uuid = serializers.CharField(source='id', read_only=True) username = serializers.CharField(required=False, validators=[ diff --git a/core/users/views.py b/core/users/views.py index e4eb50c26..ff85f8c20 100644 --- a/core/users/views.py +++ b/core/users/views.py @@ -28,6 +28,7 @@ from core.common.utils import parse_updated_since_param, from_string_to_date, get_truthy_values, \ get_resource_class_from_resource_uri from core.common.views import BaseAPIView, BaseLogoView +from core.orgs.constants import CANNOT_REMOVE_LAST_ADMIN from core.orgs.models import Organization from core.services.auth.core import AuthService from core.services.auth.openid import OpenIDAuthService @@ -249,6 +250,7 @@ class UserListView(UserBaseView, ListWithHeadersMixin, mixins.CreateModelMixin): allow_csv_export = False # user rows carry private account fields + organization = None def get_serializer_class(self): if self.request.query_params.get('summary') in TRUTHY and self.request.method == 'GET': @@ -265,6 +267,12 @@ def get_permissions(self): return [IsAdminUser()] return [] + def get_serializer_context(self): + context = super().get_serializer_context() + if self.organization: + context['member_roles'] = self.organization.member_roles + return context + def can_view(self, organization): user = self.request.user return organization.public_can_view or user.is_staff or organization.is_member(user) @@ -284,6 +292,7 @@ def get(self, request, *args, **kwargs): if not self.can_view(organization): return Response(status=status.HTTP_403_FORBIDDEN) + self.organization = organization queryset = organization.members if not self.should_include_inactive(): queryset = queryset.filter(is_active=True) @@ -477,6 +486,12 @@ def delete(self, request, *args, **kwargs): if self.user_is_self: return Response(status=status.HTTP_405_METHOD_NOT_ALLOWED) if self.is_hard_delete_requested(): + orgs = obj.orgs_needing_this_admin + if orgs: + return Response( + {'detail': CANNOT_REMOVE_LAST_ADMIN, 'orgs': [org.mnemonic for org in orgs]}, + status=status.HTTP_409_CONFLICT + ) try: obj.delete() except Exception as ex: