From 4569c75e4c02a2a21d68e62dc8e1a640c9768291 Mon Sep 17 00:00:00 2001 From: Mikko Nieminen Date: Fri, 10 Jan 2025 13:02:40 +0100 Subject: [PATCH] refactor ownership transfer (#836), add tests (#1391) --- CHANGELOG.rst | 2 + docs/source/major_changes.rst | 8 ++ projectroles/plugins.py | 8 +- projectroles/tests/test_commands.py | 130 ++++++++++++++++++++++++++-- projectroles/views.py | 111 ++++++++++++++++-------- projectroles/views_api.py | 9 +- 6 files changed, 217 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 670b9bb9..59c2c4ed 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -31,6 +31,8 @@ Changed - Deprecate declaring app setting definitions as dict (#1456) - Allow ``scope=None`` in ``AppSettingAPI.get_definitions()`` (#1535) - Deprecate ``AppSettingAPI.get_all()`` (#1534) + - Allow no role for old owner in ``RoleAssignmentOwnerTransferMixin`` (#836, #1391) + - Allow no role for old owner in ``perform_owner_transfer()`` (#836, #1391) Removed ------- diff --git a/docs/source/major_changes.rst b/docs/source/major_changes.rst index 42ac85d2..f70d4246 100644 --- a/docs/source/major_changes.rst +++ b/docs/source/major_changes.rst @@ -34,6 +34,14 @@ instead of dict, the return data of ``AppSettingAPI.get_definition()`` and class. The return data is the same even if definitions have been provided in the deprecated dictionary format. +Old Owner Role in Project Modify API Ownership Transfer +------------------------------------------------------- + +In ``ProjectModifyPluginMixin.perform_role_modify()``, the ``old_owner_role`` +argument may be ``None``. This is used in cases where project role for the +previous owner is removed. Implementations of ``perform_role_modify()`` must be +changed accordingly. The same also applies to ``revert_role_modify()``. + Deprecated Features ------------------- diff --git a/projectroles/plugins.py b/projectroles/plugins.py index 3b99c6bc..0ae9383d 100644 --- a/projectroles/plugins.py +++ b/projectroles/plugins.py @@ -140,7 +140,7 @@ def revert_role_delete(self, role_as, request=None): pass def perform_owner_transfer( - self, project, new_owner, old_owner, old_owner_role, request=None + self, project, new_owner, old_owner, old_owner_role=None, request=None ): """ Perform additional actions to finalize project ownership transfer. @@ -148,13 +148,13 @@ def perform_owner_transfer( :param project: Project object :param new_owner: SODARUser object for new owner :param old_owner: SODARUser object for previous owner - :param old_owner_role: Role object for new role of previous owner + :param old_owner_role: Role object for new role of old owner or None :param request: Request object or None """ pass def revert_owner_transfer( - self, project, new_owner, old_owner, old_owner_role, request=None + self, project, new_owner, old_owner, old_owner_role=None, request=None ): """ Revert project ownership transfer if errors have occurred in other apps. @@ -162,7 +162,7 @@ def revert_owner_transfer( :param project: Project object :param new_owner: SODARUser object for new owner :param old_owner: SODARUser object for previous owner - :param old_owner_role: Role object for new role of previous owner + :param old_owner_role: Role object for new role of old owner or None :param request: Request object or None """ pass diff --git a/projectroles/tests/test_commands.py b/projectroles/tests/test_commands.py index 0cf41009..39ef2b59 100644 --- a/projectroles/tests/test_commands.py +++ b/projectroles/tests/test_commands.py @@ -13,6 +13,9 @@ from test_plus.test import TestCase +# Appalerts dependency +from appalerts.models import AppAlert + # Timeline dependency from timeline.models import TimelineEvent @@ -54,6 +57,7 @@ ProjectInviteMixin, AppSettingMixin, RemoteSiteMixin, + RemoteProjectMixin, ) from projectroles.utils import build_secret @@ -71,6 +75,7 @@ SITE_MODE_SOURCE = SODAR_CONSTANTS['SITE_MODE_SOURCE'] SITE_MODE_PEER = SODAR_CONSTANTS['SITE_MODE_PEER'] SITE_MODE_TARGET = SODAR_CONSTANTS['SITE_MODE_TARGET'] +REMOTE_LEVEL_READ_ROLES = SODAR_CONSTANTS['REMOTE_LEVEL_READ_ROLES'] SYSTEM_USER_GROUP = SODAR_CONSTANTS['SYSTEM_USER_GROUP'] APP_SETTING_TYPE_BOOLEAN = SODAR_CONSTANTS['APP_SETTING_TYPE_BOOLEAN'] APP_SETTING_TYPE_INTEGER = SODAR_CONSTANTS['APP_SETTING_TYPE_INTEGER'] @@ -84,6 +89,7 @@ CUSTOM_PASSWORD = 'custompass' REMOTE_SITE_NAME = 'Test Site' REMOTE_SITE_URL = 'https://example.com' +REMOTE_SITE_URL2 = 'https://another.site.org' REMOTE_SITE_SECRET = build_secret(32) LDAP_DOMAIN = 'EXAMPLE' @@ -211,14 +217,14 @@ def test_add_peer_on_target(self): def test_add_second_target(self): """Test adding second target site""" - self.make_site('Existing Site', 'https://another.site.org') + self.make_site('Existing Site', REMOTE_SITE_URL2) self.assertEqual(RemoteSite.objects.count(), 1) self.command.handle(**self.options) self.assertEqual(RemoteSite.objects.count(), 2) def test_add_second_target_existing_name(self): """Test adding second target site with existing name (should fail)""" - self.make_site(REMOTE_SITE_NAME, 'https://another.site.org') + self.make_site(REMOTE_SITE_NAME, REMOTE_SITE_URL2) self.assertEqual(RemoteSite.objects.count(), 1) with self.assertRaises(SystemExit) as cm: self.command.handle(**self.options) @@ -247,9 +253,7 @@ def test_add_second_target_existing_url_suppress(self): @override_settings(PROJECTROLES_SITE_MODE=SITE_MODE_TARGET) def test_add_second_source(self): """Test adding second source site (should fail)""" - self.make_site( - 'Existing Site', 'https://another.site.org', mode=SITE_MODE_SOURCE - ) + self.make_site('Existing Site', REMOTE_SITE_URL2, mode=SITE_MODE_SOURCE) self.assertEqual(RemoteSite.objects.count(), 1) with self.assertRaises(SystemExit) as cm: self.command.handle(**self.options) @@ -685,7 +689,6 @@ class TestCleanAppSettings( """Tests for cleanappsettings command and associated functions""" def setUp(self): - super().setUp() # Init roles self.init_roles() # Init users @@ -695,13 +698,13 @@ def setUp(self): # Init projects self.category = self.make_project( - 'top_category', PROJECT_TYPE_CATEGORY, None + 'TestCategory', PROJECT_TYPE_CATEGORY, None ) self.cat_owner_as = self.make_assignment( self.category, self.user_owner, self.role_owner ) self.project = self.make_project( - 'sub_project', PROJECT_TYPE_PROJECT, self.category + 'TestProject', PROJECT_TYPE_PROJECT, self.category ) self.owner_as = self.make_assignment( self.project, self.user_owner, self.role_owner @@ -962,7 +965,116 @@ def test_command_check(self): self.assertEqual(AppSetting.objects.count(), 8) -# TODO: Add removeroles tests +class TestRemoveRoles( + ProjectMixin, + RoleMixin, + RoleAssignmentMixin, + RemoteSiteMixin, + RemoteProjectMixin, + TestCase, +): + """Tests for removeroles command""" + + def setUp(self): + # Init roles + self.init_roles() + # Init users + self.user_owner_cat = self.make_user('user_owner_cat') + self.user_owner = self.make_user('user_owner') + self.user = self.make_user('user') + # Init projects + self.category = self.make_project( + 'TestCategory', PROJECT_TYPE_CATEGORY, None + ) + self.cat_owner_as = self.make_assignment( + self.category, self.user_owner_cat, self.role_owner + ) + self.project = self.make_project( + 'TestProject', PROJECT_TYPE_PROJECT, self.category + ) + self.owner_as = self.make_assignment( + self.project, self.user_owner, self.role_owner + ) + # Set other variables + self.cmd_name = 'removeroles' + + def test_command(self): + """Test removing non-owner roles from user""" + # Set up non-owner roles for user in category and project + self.make_assignment(self.category, self.user, self.role_contributor) + self.make_assignment(self.project, self.user, self.role_guest) + # Set up another user to ensure we keep their roles + user_keep = self.make_user('user_keep') + self.make_assignment(self.category, user_keep, self.role_contributor) + self.make_assignment(self.project, user_keep, self.role_guest) + + self.assertEqual(self.category.has_role(self.user_owner_cat), True) + self.assertEqual(self.project.has_role(self.user_owner), True) + self.assertEqual(self.category.has_role(user_keep), True) + self.assertEqual(self.project.has_role(user_keep), True) + self.assertEqual(self.category.has_role(self.user), True) + self.assertEqual(self.project.has_role(self.user), True) + self.assertEqual( + TimelineEvent.objects.filter(event_name='role_delete').count(), 0 + ) + self.assertEqual( + AppAlert.objects.filter(alert_name='role_delete').count(), 0 + ) + + call_command(self.cmd_name, user=self.user) + self.assertEqual(self.category.has_role(self.user_owner_cat), True) + self.assertEqual(self.project.has_role(self.user_owner), True) + self.assertEqual(self.category.has_role(user_keep), True) + self.assertEqual(self.project.has_role(user_keep), True) + self.assertEqual(self.category.has_role(self.user), False) + self.assertEqual(self.project.has_role(self.user), False) + self.assertEqual( + TimelineEvent.objects.filter(event_name='role_delete').count(), 2 + ) + # App alerts should not be created + self.assertEqual( + AppAlert.objects.filter(alert_name='role_delete').count(), 0 + ) + + @override_settings(PROJECTROLES_SITE_MODE=SITE_MODE_TARGET) + def test_command_remote(self): + """Test removing roles from remote project""" + self.make_assignment(self.category, self.user, self.role_contributor) + self.make_assignment(self.project, self.user, self.role_guest) + remote_site = self.make_site( + name=REMOTE_SITE_NAME, + url=REMOTE_SITE_URL, + mode=SITE_MODE_SOURCE, + ) + self.make_remote_project( + project_uuid=self.category.sodar_uuid, + site=remote_site, + level=REMOTE_LEVEL_READ_ROLES, + project=self.category, + ) + self.make_remote_project( + project_uuid=self.project.sodar_uuid, + site=remote_site, + level=REMOTE_LEVEL_READ_ROLES, + project=self.project, + ) + + self.assertEqual(self.category.has_role(self.user), True) + self.assertEqual(self.project.has_role(self.user), True) + self.assertEqual(self.category.is_remote(), True) + self.assertEqual(self.project.is_remote(), True) + + call_command(self.cmd_name, user=self.user) + # Roles should not be removed + self.assertEqual(self.category.has_role(self.user), True) + self.assertEqual(self.project.has_role(self.user), True) + + ''' + def test_command_owner(self): + """Test removing owner role without defined owner""" + self.assertEqual(self.category.has_role(self.user_owner_cat), True) + self.assertEqual(self.project.has_role(self.user_owner), True) + ''' class TestSyncGroups(TestCase): diff --git a/projectroles/views.py b/projectroles/views.py index 9903679c..237bb1c4 100644 --- a/projectroles/views.py +++ b/projectroles/views.py @@ -2143,38 +2143,44 @@ class RoleAssignmentOwnerTransferMixin(ProjectModifyPluginViewMixin): def _get_timeline_ok_status(self): timeline = get_backend_api('timeline_backend') - if not timeline: - return None - else: - return timeline.TL_STATUS_OK + return timeline.TL_STATUS_OK if timeline else None def _get_timeline_failed_status(self): timeline = get_backend_api('timeline_backend') - if not timeline: - return None - else: - return timeline.TL_STATUS_FAILED + return timeline.TL_STATUS_FAILED if timeline else None def _create_timeline_event( - self, old_owner, new_owner, old_owner_role, project + self, project, old_owner, new_owner, old_owner_role=None, issuer=None ): + """ + Create timeline event for ownership transfer. + + :param project: Project object + :param old_owner: User object for old owner + :param new_owner: User object for new_owner + :param old_owner_role: Role object for old owner's new role or None + :param issuer: User who initiated ownership transfer or None + """ timeline = get_backend_api('timeline_backend') - # Init Timeline event if not timeline: return None - tl_desc = ( - 'transfer ownership from {{{}}} to {{{}}}, set old owner as "{}"' - ).format('prev_owner', 'new_owner', old_owner_role.name) + tl_desc = 'transfer ownership from {prev_owner} to {new_owner}, ' + if old_owner_role: + tl_desc += 'set old owner as "{}"'.format(old_owner_role.name) + else: + tl_desc += 'remove old owner role' tl_event = timeline.add_event( project=project, app_name=APP_NAME, - user=self.request.user, + user=issuer, event_name='role_owner_transfer', description=tl_desc, extra_data={ 'prev_owner': old_owner.username, 'new_owner': new_owner.username, - 'old_owner_role': old_owner_role.name, + 'old_owner_role': ( + old_owner_role.name if old_owner_role else None + ), }, ) tl_event.add_object(old_owner, 'prev_owner', old_owner.username) @@ -2187,8 +2193,9 @@ def _handle_transfer( project, old_owner_as, new_owner, - old_owner_role, old_inh_owner, + old_owner_role=None, + request=None, ): """ Handle ownership transfer with atomic rollback. @@ -2196,11 +2203,12 @@ def _handle_transfer( :param project: Project object :param old_owner_as: RoleAssignment object for old owner :param new_owner: User object for new user - :param old_owner_role: Role object to set for old owner :param old_inh_owner: Whether old owner is inherited owner (boolean) + :param old_owner_role: Role object to set for old owner or None + :param request: HttpRequest object or None """ - # Inherited owner, delete local role - if old_inh_owner: + # Inherited owner or no new role for old owner: delete local role + if old_inh_owner or not old_owner_role: old_owner_as.delete() # Update old owner role else: @@ -2224,14 +2232,22 @@ def _handle_transfer( new_owner, old_owner_as.user, old_owner_role, - self.request, + request, ] self.call_project_modify_api( 'perform_owner_transfer', 'revert_owner_transfer', args ) return True - def transfer_owner(self, project, new_owner, old_owner_as, old_owner_role): + def transfer_owner( + self, + project, + new_owner, + old_owner_as, + old_owner_role=None, + request=None, + notify_old=True, + ): """ Transfer project ownership to a new user and assign a new role to the previous owner. @@ -2239,8 +2255,9 @@ def transfer_owner(self, project, new_owner, old_owner_as, old_owner_role): :param project: Project object :param new_owner: User object :param old_owner_as: RoleAssignment object - :param old_owner_role: Role object for the previous owner's new role - :return: + :param old_owner_role: Role object for old owner's new role or None + :param request: HttpRequest object or None + :param notify_old: Notify old owner (boolean, default=True) """ app_alerts = get_backend_api('appalerts_backend') self.role_owner = Role.objects.get(name=PROJECT_ROLE_OWNER) @@ -2255,13 +2272,19 @@ def transfer_owner(self, project, new_owner, old_owner_as, old_owner_role): new_inh_owner = ( True if new_inh_as and new_inh_as.role == self.role_owner else False ) + issuer = request.user if request else None tl_event = self._create_timeline_event( - old_owner, new_owner, old_owner_role, project + project, old_owner, new_owner, old_owner_role, issuer ) try: self._handle_transfer( - project, old_owner_as, new_owner, old_owner_role, old_inh_owner + project, + old_owner_as, + new_owner, + old_inh_owner, + old_owner_role, + request, ) except Exception as ex: if tl_event: @@ -2270,14 +2293,16 @@ def transfer_owner(self, project, new_owner, old_owner_as, old_owner_role): if tl_event: tl_event.set_status(self._get_timeline_ok_status()) - if not old_inh_owner and self.request.user != old_owner: + # Notify old owner + if notify_old and not old_inh_owner and issuer != old_owner: if app_alerts: app_alerts.add_alert( app_name=APP_NAME, alert_name='role_update', user=old_owner, message=ROLE_UPDATE_MSG.format( - project=project.title, role=old_owner_role.name + project=project.title, + role=old_owner_role.name if old_owner_role else 'None', ), url=reverse( 'projectroles:detail', @@ -2285,13 +2310,19 @@ def transfer_owner(self, project, new_owner, old_owner_as, old_owner_role): ), project=project, ) - if SEND_EMAIL and app_settings.get( - APP_NAME, 'notify_email_role', user=old_owner + if ( + SEND_EMAIL + and request + and app_settings.get( + APP_NAME, 'notify_email_role', user=old_owner + ) ): + # NOTE: Request is needed here email.send_role_change_mail( - 'update', project, old_owner, old_owner_role, self.request + 'update', project, old_owner, old_owner_role, request ) - if not new_inh_owner and self.request.user != new_owner: + # Notify new owner + if not new_inh_owner and issuer != new_owner: if app_alerts: app_alerts.add_alert( app_name=APP_NAME, @@ -2306,15 +2337,20 @@ def transfer_owner(self, project, new_owner, old_owner_as, old_owner_role): ), project=project, ) - if SEND_EMAIL and app_settings.get( - APP_NAME, 'notify_email_role', user=new_owner + if ( + SEND_EMAIL + and request + and app_settings.get( + APP_NAME, 'notify_email_role', user=new_owner + ) ): + # NOTE: Request is needed here email.send_role_change_mail( 'update', project, new_owner, self.role_owner, - self.request, + request, ) @@ -2361,7 +2397,12 @@ def form_valid(self, form): ) try: self.transfer_owner( - project, new_owner, old_owner_as, old_owner_role + project, + new_owner, + old_owner_as, + old_owner_role, + issuer=self.request.user, + request=self.request, ) except Exception as ex: # TODO: Add logging diff --git a/projectroles/views_api.py b/projectroles/views_api.py index 8186b8b1..06b7fc08 100644 --- a/projectroles/views_api.py +++ b/projectroles/views_api.py @@ -681,10 +681,13 @@ def post(self, request, *args, **kwargs): ) ) - # All OK, transfer owner - try: + try: # All OK, transfer owner self.transfer_owner( - project, new_owner, old_owner_as, old_owner_role + project, + new_owner, + old_owner_as, + old_owner_role, + request=request, ) except Exception as ex: raise APIException('Unable to transfer owner: {}'.format(ex))