From dfab6c97534edf0689aa90fdb78d84b93bd4ed2f Mon Sep 17 00:00:00 2001 From: Kelvin Muchiri Date: Wed, 3 Apr 2024 15:43:37 +0300 Subject: [PATCH] Add user to organization asynchronously (#2574) * add user to organization asynchronously add user to organization asynchronously and refactor to remove redundant code * address failing tests * refactor code * add tests * add tests * suppress lint warning suppress lint warning Function name "add_org_user_and_share_projects_async" doesn't conform to '(([a-z][a-z0-9_]{2,30})|(_[a-z0-9_]*))$' pattern * fix pylint error fix pylint: unrecognized-inline-option / Unrecognized file option 'invalid-name' * add tests * share project asynchronously * add test case * add test case * retry async task incase of exception * add tests * add test case * address typos * send email after adding user to org successfully * address lint warning address pylint: no-value-for-parameter / No value for argument 'from_email' --- onadata/apps/api/tasks.py | 82 ++++++- onadata/apps/api/tests/test_tasks.py | 232 +++++++++++++++++- onadata/apps/api/tests/test_tools.py | 172 +++++++++++++ .../test_organization_profile_viewset.py | 9 +- .../tests/viewsets/test_project_viewset.py | 3 +- onadata/apps/api/tools.py | 50 +++- onadata/apps/api/viewsets/project_viewset.py | 5 +- onadata/libs/models/share_project.py | 3 + .../organization_member_serializer.py | 90 +------ .../serializers/share_project_serializer.py | 44 ++-- onadata/libs/tests/models/__init__.py | 0 .../libs/tests/models/test_share_project.py | 93 +++++++ 12 files changed, 671 insertions(+), 112 deletions(-) create mode 100644 onadata/apps/api/tests/test_tools.py create mode 100644 onadata/libs/tests/models/__init__.py create mode 100644 onadata/libs/tests/models/test_share_project.py diff --git a/onadata/apps/api/tasks.py b/onadata/apps/api/tasks.py index 09a751ae26..2b1a815f1b 100644 --- a/onadata/apps/api/tasks.py +++ b/onadata/apps/api/tasks.py @@ -11,21 +11,27 @@ from django.conf import settings from django.core.files.uploadedfile import TemporaryUploadedFile from django.core.files.storage import default_storage +from django.core.mail import send_mail from django.contrib.auth import get_user_model +from django.db import DatabaseError from django.utils import timezone from django.utils.datastructures import MultiValueDict from onadata.apps.api import tools +from onadata.apps.api.models.organization_profile import OrganizationProfile +from onadata.apps.logger.models import Instance, ProjectInvitation, XForm, Project from onadata.libs.utils.email import send_generic_email from onadata.libs.utils.model_tools import queryset_iterator from onadata.libs.utils.cache_tools import ( safe_delete, XFORM_REGENERATE_INSTANCE_JSON_TASK, ) -from onadata.apps.logger.models import Instance, ProjectInvitation, XForm +from onadata.libs.models.share_project import ShareProject from onadata.libs.utils.email import ProjectInvitationEmail from onadata.celeryapp import app +logger = logging.getLogger(__name__) + User = get_user_model() @@ -145,7 +151,7 @@ def send_project_invitation_email_async( invitation = ProjectInvitation.objects.get(id=invitation_id) except ProjectInvitation.DoesNotExist as err: - logging.exception(err) + logger.exception(err) else: email = ProjectInvitationEmail(invitation, url) @@ -161,7 +167,7 @@ def regenerate_form_instance_json(xform_id: int): try: xform: XForm = XForm.objects.get(pk=xform_id) except XForm.DoesNotExist as err: - logging.exception(err) + logger.exception(err) else: if not xform.is_instance_json_regenerated: @@ -182,3 +188,73 @@ def regenerate_form_instance_json(xform_id: int): # Clear cache used to store the task id from the AsyncResult cache_key = f"{XFORM_REGENERATE_INSTANCE_JSON_TASK}{xform_id}" safe_delete(cache_key) + + +class ShareProjectBaseTask(app.Task): + autoretry_for = ( + DatabaseError, + ConnectionError, + ) + retry_backoff = 3 + + +@app.task(base=ShareProjectBaseTask) +def add_org_user_and_share_projects_async( + org_id: int, + user_id: int, + role: str = None, + email_subject: str = None, + email_msg: str = None, +): # pylint: disable=invalid-name + """Add user to organization and share projects asynchronously""" + try: + organization = OrganizationProfile.objects.get(pk=org_id) + user = User.objects.get(pk=user_id) + + except OrganizationProfile.DoesNotExist as err: + logger.exception(err) + + except User.DoesNotExist as err: + logger.exception(err) + + else: + tools.add_org_user_and_share_projects(organization, user, role) + + if email_msg and email_subject and user.email: + send_mail( + email_subject, + email_msg, + settings.DEFAULT_FROM_EMAIL, + (user.email,), + ) + + +@app.task(base=ShareProjectBaseTask) +def remove_org_user_async(org_id, user_id): + """Remove user from organization asynchronously""" + try: + organization = OrganizationProfile.objects.get(pk=org_id) + user = User.objects.get(pk=user_id) + + except OrganizationProfile.DoesNotExist as err: + logger.exception(err) + + except User.DoesNotExist as err: + logger.exception(err) + + else: + tools.remove_user_from_organization(organization, user) + + +@app.task(base=ShareProjectBaseTask) +def share_project_async(project_id, username, role, remove=False): + """Share project asynchronously""" + try: + project = Project.objects.get(pk=project_id) + + except Project.DoesNotExist as err: + logger.exception(err) + + else: + share = ShareProject(project, username, role, remove) + share.save() diff --git a/onadata/apps/api/tests/test_tasks.py b/onadata/apps/api/tests/test_tasks.py index 0051008f32..0edcca4138 100644 --- a/onadata/apps/api/tests/test_tasks.py +++ b/onadata/apps/api/tests/test_tasks.py @@ -1,19 +1,31 @@ """Tests for module onadata.apps.api.tasks""" + import sys from unittest.mock import patch from django.core.cache import cache +from django.contrib.auth import get_user_model +from django.db import DatabaseError, OperationalError +from django.test import override_settings -from onadata.apps.main.tests.test_base import TestBase from onadata.apps.api.tasks import ( send_project_invitation_email_async, regenerate_form_instance_json, + add_org_user_and_share_projects_async, + remove_org_user_async, + share_project_async, + ShareProject, ) +from onadata.apps.api.models.organization_profile import OrganizationProfile from onadata.apps.logger.models import ProjectInvitation, Instance +from onadata.apps.main.tests.test_base import TestBase +from onadata.libs.permissions import ManagerRole from onadata.libs.utils.user_auth import get_user_default_project from onadata.libs.utils.email import ProjectInvitationEmail +User = get_user_model() + class SendProjectInivtationEmailAsyncTestCase(TestBase): """Tests for send_project_invitation_email_async""" @@ -80,7 +92,7 @@ def mock_get_full_dict( instance.refresh_from_db() self.assertFalse("foo" in instance.json) - @patch("logging.exception") + @patch("onadata.apps.api.tasks.logger.exception") def test_form_id_invalid(self, mock_log_exception): """An invalid xform_id is handled""" @@ -107,3 +119,219 @@ def mock_get_full_dict( regenerate_form_instance_json.delay(self.xform.pk) instance.refresh_from_db() self.assertFalse(instance.json) + + +@patch("onadata.apps.api.tasks.tools.add_org_user_and_share_projects") +class AddOrgUserAndShareProjectsAsyncTestCase(TestBase): + """Tests for add_org_user_and_share_projects_async""" + + def setUp(self): + super().setUp() + + self.org_user = User.objects.create(username="onaorg") + alice = self._create_user("alice", "1234&&") + self.org = OrganizationProfile.objects.create( + user=self.org_user, name="Ona Org", creator=alice + ) + + def test_user_added_to_org(self, mock_add): + """User is added to organization""" + add_org_user_and_share_projects_async.delay( + self.org.pk, self.user.pk, "manager" + ) + mock_add.assert_called_once_with(self.org, self.user, "manager") + + def test_role_optional(self, mock_add): + """role param is optional""" + add_org_user_and_share_projects_async.delay(self.org.pk, self.user.pk) + mock_add.assert_called_once_with(self.org, self.user, None) + + @patch("onadata.apps.api.tasks.logger.exception") + def test_invalid_org_id(self, mock_log, mock_add): + """Invalid org_id is handled""" + add_org_user_and_share_projects_async.delay(sys.maxsize, self.user.pk) + mock_add.assert_not_called() + mock_log.assert_called_once() + + @patch("onadata.apps.api.tasks.logger.exception") + def test_invalid_user_id(self, mock_log, mock_add): + """Invalid org_id is handled""" + add_org_user_and_share_projects_async.delay(self.org.pk, sys.maxsize) + mock_add.assert_not_called() + mock_log.assert_called_once() + + @patch("onadata.apps.api.tasks.add_org_user_and_share_projects_async.retry") + def test_database_error(self, mock_retry, mock_add): + """We retry calls if DatabaseError is raised""" + mock_add.side_effect = DatabaseError() + add_org_user_and_share_projects_async.delay(self.org.pk, self.user.pk) + self.assertTrue(mock_retry.called) + _, kwargs = mock_retry.call_args_list[0] + self.assertTrue(isinstance(kwargs["exc"], DatabaseError)) + + @patch("onadata.apps.api.tasks.add_org_user_and_share_projects_async.retry") + def test_connection_error(self, mock_retry, mock_add): + """We retry calls if ConnectionError is raised""" + mock_add.side_effect = ConnectionError() + add_org_user_and_share_projects_async.delay(self.org.pk, self.user.pk) + self.assertTrue(mock_retry.called) + _, kwargs = mock_retry.call_args_list[0] + self.assertTrue(isinstance(kwargs["exc"], ConnectionError)) + + @patch("onadata.apps.api.tasks.add_org_user_and_share_projects_async.retry") + def test_operation_error(self, mock_retry, mock_add): + """We retry calls if OperationError is raised""" + mock_add.side_effect = OperationalError() + add_org_user_and_share_projects_async.delay(self.org.pk, self.user.pk) + self.assertTrue(mock_retry.called) + _, kwargs = mock_retry.call_args_list[0] + self.assertTrue(isinstance(kwargs["exc"], OperationalError)) + + @override_settings(DEFAULT_FROM_EMAIL="noreply@ona.io") + @patch("onadata.apps.api.tasks.send_mail") + def test_send_mail(self, mock_email, mock_add): + """Send mail works""" + self.user.email = "bob@example.com" + self.user.save() + add_org_user_and_share_projects_async.delay( + self.org.pk, self.user.pk, "manager", "Subject", "Body" + ) + mock_email.assert_called_with( + "Subject", + "Body", + "noreply@ona.io", + ("bob@example.com",), + ) + mock_add.assert_called_once_with(self.org, self.user, "manager") + + @override_settings(DEFAULT_FROM_EMAIL="noreply@ona.io") + @patch("onadata.apps.api.tasks.send_mail") + def test_user_email_none(self, mock_email, mock_add): + """Email not sent if user email is None""" + add_org_user_and_share_projects_async.delay( + self.org.pk, self.user.pk, "manager", "Subject", "Body" + ) + mock_email.assert_not_called() + mock_add.assert_called_once_with(self.org, self.user, "manager") + + +@patch("onadata.apps.api.tasks.tools.remove_user_from_organization") +class RemoveOrgUserAsyncTestCase(TestBase): + """Tests for remove_org_user_async""" + + def setUp(self): + super().setUp() + + self.org_user = User.objects.create(username="onaorg") + alice = self._create_user("alice", "1234&&") + self.org = OrganizationProfile.objects.create( + user=self.org_user, name="Ona Org", creator=alice + ) + + def test_user_removed_from_org(self, mock_remove): + """User is removed from organization""" + remove_org_user_async.delay(self.org.pk, self.user.pk) + mock_remove.assert_called_once_with(self.org, self.user) + + @patch("onadata.apps.api.tasks.logger.exception") + def test_invalid_org_id(self, mock_log, mock_remove): + """Invalid org_id is handled""" + remove_org_user_async.delay(sys.maxsize, self.user.pk) + mock_remove.assert_not_called() + mock_log.assert_called_once() + + @patch("onadata.apps.api.tasks.logger.exception") + def test_invalid_user_id(self, mock_log, mock_remove): + """Invalid user_id is handled""" + remove_org_user_async.delay(self.org.pk, sys.maxsize) + mock_remove.assert_not_called() + mock_log.assert_called_once() + + @patch("onadata.apps.api.tasks.remove_org_user_async.retry") + def test_database_error(self, mock_retry, mock_remove): + """We retry calls if DatabaseError is raised""" + mock_remove.side_effect = DatabaseError() + remove_org_user_async.delay(self.org.pk, self.user.pk) + self.assertTrue(mock_retry.called) + _, kwargs = mock_retry.call_args_list[0] + self.assertTrue(isinstance(kwargs["exc"], DatabaseError)) + + @patch("onadata.apps.api.tasks.remove_org_user_async.retry") + def test_connection_error(self, mock_retry, mock_remove): + """We retry calls if ConnectionError is raised""" + mock_remove.side_effect = ConnectionError() + remove_org_user_async.delay(self.org.pk, self.user.pk) + self.assertTrue(mock_retry.called) + _, kwargs = mock_retry.call_args_list[0] + self.assertTrue(isinstance(kwargs["exc"], ConnectionError)) + + @patch("onadata.apps.api.tasks.remove_org_user_async.retry") + def test_operation_error(self, mock_retry, mock_remove): + """We retry calls if OperationError is raised""" + mock_remove.side_effect = OperationalError() + remove_org_user_async.delay(self.org.pk, self.user.pk) + self.assertTrue(mock_retry.called) + _, kwargs = mock_retry.call_args_list[0] + self.assertTrue(isinstance(kwargs["exc"], OperationalError)) + + +class ShareProjectAsyncTestCase(TestBase): + """Tests for share_project_async""" + + def setUp(self): + super().setUp() + + self._publish_transportation_form() + self.alice = self._create_user("alice", "Yuao8(-)") + + def test_share(self): + """Project is shared with user""" + share_project_async.delay(self.project.id, "alice", "manager") + + self.assertTrue(ManagerRole.user_has_role(self.alice, self.project)) + + def test_remove(self): + """User is removed from project""" + # Add user to project + ManagerRole.add(self.alice, self.project) + # Remove user + share_project_async.delay(self.project.id, "alice", "manager", True) + + self.assertFalse(ManagerRole.user_has_role(self.alice, self.project)) + + @patch("onadata.apps.api.tasks.logger.exception") + def test_invalid_project_id(self, mock_log): + """Invalid projecct_id is handled""" + share_project_async.delay(sys.maxsize, "alice", "manager") + self.assertFalse(ManagerRole.user_has_role(self.alice, self.project)) + mock_log.assert_called_once() + + @patch.object(ShareProject, "save") + @patch("onadata.apps.api.tasks.share_project_async.retry") + def test_database_error(self, mock_retry, mock_share): + """We retry calls if DatabaseError is raised""" + mock_share.side_effect = DatabaseError() + share_project_async.delay(self.project.id, self.user.pk, "manager") + self.assertTrue(mock_retry.called) + _, kwargs = mock_retry.call_args_list[0] + self.assertTrue(isinstance(kwargs["exc"], DatabaseError)) + + @patch.object(ShareProject, "save") + @patch("onadata.apps.api.tasks.share_project_async.retry") + def test_connection_error(self, mock_retry, mock_share): + """We retry calls if ConnectionError is raised""" + mock_share.side_effect = ConnectionError() + share_project_async.delay(self.project.pk, self.user.pk, "manager") + self.assertTrue(mock_retry.called) + _, kwargs = mock_retry.call_args_list[0] + self.assertTrue(isinstance(kwargs["exc"], ConnectionError)) + + @patch.object(ShareProject, "save") + @patch("onadata.apps.api.tasks.share_project_async.retry") + def test_operation_error(self, mock_retry, mock_share): + """We retry calls if OperationError is raised""" + mock_share.side_effect = OperationalError() + share_project_async.delay(self.project.pk, self.user.pk, "manager") + self.assertTrue(mock_retry.called) + _, kwargs = mock_retry.call_args_list[0] + self.assertTrue(isinstance(kwargs["exc"], OperationalError)) diff --git a/onadata/apps/api/tests/test_tools.py b/onadata/apps/api/tests/test_tools.py new file mode 100644 index 0000000000..ac3c59f4f5 --- /dev/null +++ b/onadata/apps/api/tests/test_tools.py @@ -0,0 +1,172 @@ +"""Tests for module onadata.apps.api.tools""" + +from django.contrib.auth import get_user_model + +from onadata.apps.api.models.organization_profile import ( + OrganizationProfile, + Team, + get_organization_members_team, +) +from onadata.apps.api.tools import ( + add_org_user_and_share_projects, + add_user_to_organization, +) +from onadata.apps.logger.models.project import Project +from onadata.apps.main.tests.test_base import TestBase +from onadata.libs.permissions import DataEntryRole, ManagerRole, OwnerRole + + +User = get_user_model() + + +class AddUserToOrganizationTestCase(TestBase): + """Add tests for add_user_to_organization""" + + def setUp(self) -> None: + super().setUp() + + self.org_user = User.objects.create(username="onaorg") + alice = self._create_user("alice", "1234&&") + self.org = OrganizationProfile.objects.create( + user=self.org_user, name="Ona Org", creator=alice + ) + + def test_add_owner(self): + """Owner is added to organization""" + add_user_to_organization(self.org, self.user, "owner") + + self.user.refresh_from_db() + owner_team = Team.objects.get(name=f"{self.org_user.username}#Owners") + members_team = Team.objects.get(name=f"{self.org_user.username}#members") + self.assertTrue( + owner_team.user_set.filter(username=self.user.username).exists() + ) + self.assertTrue( + members_team.user_set.filter(username=self.user.username).exists() + ) + self.assertTrue(OwnerRole.user_has_role(self.user, self.org)) + self.assertTrue(OwnerRole.user_has_role(self.user, self.org.userprofile_ptr)) + + # If role changes, user is removed from owners team + add_user_to_organization(self.org, self.user, "editor") + + self.user.refresh_from_db() + owner_team.refresh_from_db() + + self.assertFalse( + owner_team.user_set.filter(username=self.user.username).exists() + ) + self.assertFalse(OwnerRole.user_has_role(self.user, self.org)) + self.assertFalse(OwnerRole.user_has_role(self.user, self.org.userprofile_ptr)) + + def test_non_owner(self): + """Non-owners add to organization""" + add_user_to_organization(self.org, self.user, "manager") + + self.user.refresh_from_db() + owner_team = Team.objects.get(name=f"{self.org_user.username}#Owners") + members_team = Team.objects.get(name=f"{self.org_user.username}#members") + self.assertFalse( + owner_team.user_set.filter(username=self.user.username).exists() + ) + self.assertTrue( + members_team.user_set.filter(username=self.user.username).exists() + ) + self.assertTrue(ManagerRole.user_has_role(self.user, self.org)) + + def test_role_none(self): + """role param is None or not provided""" + add_user_to_organization(self.org, self.user) + + self.user.refresh_from_db() + owner_team = Team.objects.get(name=f"{self.org_user.username}#Owners") + members_team = Team.objects.get(name=f"{self.org_user.username}#members") + self.assertFalse( + owner_team.user_set.filter(username=self.user.username).exists() + ) + self.assertTrue( + members_team.user_set.filter(username=self.user.username).exists() + ) + + +class AddOrgUserAndShareProjectsTestCase(TestBase): + """Tests for add_org_user_and_share_projects""" + + def setUp(self) -> None: + super().setUp() + + self.org_user = User.objects.create(username="onaorg") + alice = self._create_user("alice", "1234&&") + self.org = OrganizationProfile.objects.create( + user=self.org_user, name="Ona Org", creator=alice + ) + self.project = Project.objects.create( + name="Demo", organization=self.org_user, created_by=alice + ) + + def test_add_owner(self): + """Owner added to org and projects shared""" + add_org_user_and_share_projects(self.org, self.user, "owner") + + self.user.refresh_from_db() + owner_team = Team.objects.get(name=f"{self.org_user.username}#Owners") + members_team = Team.objects.get(name=f"{self.org_user.username}#members") + self.assertTrue( + owner_team.user_set.filter(username=self.user.username).exists() + ) + self.assertTrue( + members_team.user_set.filter(username=self.user.username).exists() + ) + self.assertTrue(OwnerRole.user_has_role(self.user, self.project)) + self.assertTrue(OwnerRole.user_has_role(self.user, self.org)) + + def test_non_owner(self): + """Non-owners add to org and projects shared + + Non-owners should be assigned default project permissions + """ + # Set default permissions for project + members_team = get_organization_members_team(self.org) + DataEntryRole.add(members_team, self.project) + + add_org_user_and_share_projects(self.org, self.user, "manager") + + self.user.refresh_from_db() + owner_team = Team.objects.get(name=f"{self.org_user.username}#Owners") + members_team = Team.objects.get(name=f"{self.org_user.username}#members") + self.assertFalse( + owner_team.user_set.filter(username=self.user.username).exists() + ) + self.assertTrue( + members_team.user_set.filter(username=self.user.username).exists() + ) + self.assertTrue(DataEntryRole.user_has_role(self.user, self.project)) + self.assertTrue(ManagerRole.user_has_role(self.user, self.org)) + + def test_project_created_by_manager(self): + """A manager is assigned manager role on projects they created""" + self.project.created_by = self.user + self.project.save() + + add_org_user_and_share_projects(self.org, self.user, "manager") + + self.assertTrue(ManagerRole.user_has_role(self.user, self.project)) + + def test_role_none(self): + """role param is None or not provided""" + # Set default permissions for project + members_team = get_organization_members_team(self.org) + DataEntryRole.add(members_team, self.project) + + add_org_user_and_share_projects(self.org, self.user) + + self.user.refresh_from_db() + owner_team = Team.objects.get(name=f"{self.org_user.username}#Owners") + members_team = Team.objects.get(name=f"{self.org_user.username}#members") + self.assertFalse( + owner_team.user_set.filter(username=self.user.username).exists() + ) + self.assertTrue( + members_team.user_set.filter(username=self.user.username).exists() + ) + self.assertTrue(DataEntryRole.user_has_role(self.user, self.project)) diff --git a/onadata/apps/api/tests/viewsets/test_organization_profile_viewset.py b/onadata/apps/api/tests/viewsets/test_organization_profile_viewset.py index 700d071698..7ae8ae6f09 100644 --- a/onadata/apps/api/tests/viewsets/test_organization_profile_viewset.py +++ b/onadata/apps/api/tests/viewsets/test_organization_profile_viewset.py @@ -16,6 +16,7 @@ from onadata.apps.api.models.organization_profile import ( OrganizationProfile, get_organization_members_team, + Team, ) from onadata.apps.api.tests.viewsets.test_abstract_viewset import TestAbstractViewSet from onadata.apps.api.tools import ( @@ -297,6 +298,8 @@ def test_add_members_to_org(self): response = view(request, user="denoinc") self.assertEqual(response.status_code, 201) self.assertEqual(set(response.data), set(["denoinc", "aboy"])) + team = Team.objects.get(name=f"{self.organization.user.username}#members") + self.assertTrue(team.user_set.filter(username="aboy").exists()) def test_inactive_members_not_listed(self): self._org_create() @@ -681,7 +684,8 @@ def test_put_bad_role(self): response = view(request, user="denoinc") self.assertEqual(response.status_code, 400) - @patch("onadata.libs.serializers.organization_member_serializer.send_mail") + @override_settings(DEFAULT_FROM_EMAIL="noreply@ona.io") + @patch("onadata.apps.api.tasks.send_mail") def test_add_members_to_org_email(self, mock_email): self._org_create() view = OrganizationProfileViewSet.as_view({"post": "members"}) @@ -705,7 +709,8 @@ def test_add_members_to_org_email(self, mock_email): ) self.assertEqual(set(response.data), set(["denoinc", "aboy"])) - @patch("onadata.libs.serializers.organization_member_serializer.send_mail") + @override_settings(DEFAULT_FROM_EMAIL="noreply@ona.io") + @patch("onadata.apps.api.tasks.send_mail") def test_add_members_to_org_email_custom_subj(self, mock_email): self._org_create() view = OrganizationProfileViewSet.as_view({"post": "members"}) diff --git a/onadata/apps/api/tests/viewsets/test_project_viewset.py b/onadata/apps/api/tests/viewsets/test_project_viewset.py index 1b96ca546c..f7b847aa4a 100644 --- a/onadata/apps/api/tests/viewsets/test_project_viewset.py +++ b/onadata/apps/api/tests/viewsets/test_project_viewset.py @@ -2488,8 +2488,7 @@ def test_project_share_atomicity(self, mock_send_mail): mock_rm_xform_perms, ): # noqa mock_rm_xform_perms.side_effect = Exception() - with self.assertRaises(Exception): - response = view(request, pk=projectid) + response = view(request, pk=projectid) # permissions have not changed for both xform and project self.assertTrue(role_class.user_has_role(alice, self.xform)) self.assertTrue(role_class.user_has_role(alice, self.project)) diff --git a/onadata/apps/api/tools.py b/onadata/apps/api/tools.py index 64832a7a66..9aaf21bcb0 100644 --- a/onadata/apps/api/tools.py +++ b/onadata/apps/api/tools.py @@ -55,6 +55,7 @@ get_role, get_role_in_org, is_organization, + get_team_project_default_permissions, ) from onadata.libs.serializers.project_serializer import ProjectSerializer from onadata.libs.utils.api_export_tools import ( @@ -75,6 +76,7 @@ publish_form, response_with_mimetype_and_name, ) +from onadata.libs.utils.model_tools import queryset_iterator from onadata.libs.utils.project_utils import ( set_project_perms_to_xform, set_project_perms_to_xform_async, @@ -227,12 +229,27 @@ def remove_user_from_team(team, user): remove_perm(perm.codename, user, members_team) -def add_user_to_organization(organization, user): +def add_user_to_organization(organization, user, role=None): """Add a user to an organization""" team = get_organization_members_team(organization) add_user_to_team(team, user) + if role is not None: + role_cls = ROLES.get(role) + role_cls.add(user, organization) + + owners_team = get_or_create_organization_owners_team(organization) + + if role == OwnerRole.name: + role_cls.add(user, organization.userprofile_ptr) + # Add user to their respective team + add_user_to_team(owners_team, user) + + else: + remove_user_from_team(owners_team, user) + OwnerRole.remove_obj_permissions(user, organization.userprofile_ptr) + def get_organization_members(organization): """Get members team user queryset""" @@ -814,3 +831,34 @@ def set_enketo_signed_cookies(resp, username=None, json_web_token=None): resp.set_signed_cookie(ENKETO_AUTH_COOKIE, json_web_token, **enketo) return resp + + +def add_org_user_and_share_projects( + organization: OrganizationProfile, user, org_role: str = None +): + """Add user to organization and share all projects""" + add_user_to_organization(organization, user, org_role) + + def share(project, role): + share = ShareProject(project, user.username, role) + share.save() + + project_qs = organization.user.project_org.all() + + if org_role == OwnerRole.name: + # New owners have owner role on all projects + for project in queryset_iterator(project_qs): + share(project, org_role) + + else: + # New members & managers gain default team permissions on projects + team = get_organization_members_team(organization) + + for project in queryset_iterator(project_qs): + if org_role == ManagerRole.name and project.created_by == user: + # New managers are only granted the manager role on the + # projects they created + share(project, org_role) + else: + project_role = get_team_project_default_permissions(team, project) + share(project, project_role) diff --git a/onadata/apps/api/viewsets/project_viewset.py b/onadata/apps/api/viewsets/project_viewset.py index 3f72099143..c8e6797ce7 100644 --- a/onadata/apps/api/viewsets/project_viewset.py +++ b/onadata/apps/api/viewsets/project_viewset.py @@ -33,7 +33,6 @@ from onadata.libs.serializers.share_project_serializer import ( RemoveUserFromProjectSerializer, ShareProjectSerializer, - propagate_project_permissions_async, ) from onadata.libs.serializers.user_profile_serializer import UserProfileSerializer from onadata.libs.serializers.xform_serializer import ( @@ -48,6 +47,7 @@ from onadata.libs.utils.cache_tools import PROJ_OWNER_CACHE, safe_delete from onadata.libs.utils.common_tools import merge_dicts from onadata.libs.utils.export_tools import str_to_bool +from onadata.libs.utils.project_utils import propagate_project_permissions_async from onadata.settings.common import DEFAULT_FROM_EMAIL, SHARE_PROJECT_SUBJECT # pylint: disable=invalid-name @@ -64,7 +64,6 @@ class ProjectViewSet( BaseViewset, ModelViewSet, ): - """ List, Retrieve, Update, Create Project and Project Forms. """ @@ -182,7 +181,7 @@ def share(self, request, *args, **kwargs): remove = strtobool(remove) if remove: - serializer = RemoveUserFromProjectSerializer(data=data) + serializer = RemoveUserFromProjectSerializer(data={**data, remove: True}) else: serializer = ShareProjectSerializer(data=data) if serializer.is_valid(): diff --git a/onadata/libs/models/share_project.py b/onadata/libs/models/share_project.py index f23807e975..d179d15972 100644 --- a/onadata/libs/models/share_project.py +++ b/onadata/libs/models/share_project.py @@ -18,6 +18,7 @@ PROJ_PERM_CACHE, safe_delete, ) +from onadata.libs.utils.project_utils import propagate_project_permissions_async # pylint: disable=invalid-name User = get_user_model() @@ -91,6 +92,8 @@ def save(self, **kwargs): # clear cache safe_delete(f"{PROJ_OWNER_CACHE}{self.project.pk}") safe_delete(f"{PROJ_PERM_CACHE}{self.project.pk}") + # propagate KPI permissions + propagate_project_permissions_async.apply_async(args=[self.project.pk]) @transaction.atomic() def __remove_user(self): diff --git a/onadata/libs/serializers/organization_member_serializer.py b/onadata/libs/serializers/organization_member_serializer.py index 8c3520455b..22d58be9f2 100644 --- a/onadata/libs/serializers/organization_member_serializer.py +++ b/onadata/libs/serializers/organization_member_serializer.py @@ -3,85 +3,30 @@ The OrganizationMemberSerializer - manages a users access in an organization """ from django.contrib.auth import get_user_model -from django.core.mail import send_mail from django.utils.translation import gettext as _ from rest_framework import serializers -from onadata.apps.api.models.organization_profile import get_organization_members_team from onadata.apps.api.tools import ( _get_owners, - add_user_to_organization, - add_user_to_team, - get_or_create_organization_owners_team, get_organization_members, - remove_user_from_organization, - remove_user_from_team, ) -from onadata.apps.logger.models.project import Project +from onadata.apps.api.tasks import ( + add_org_user_and_share_projects_async, + remove_org_user_async, +) from onadata.apps.main.models.user_profile import UserProfile from onadata.libs.permissions import ( ROLES, - ManagerRole, OwnerRole, - get_team_project_default_permissions, is_organization, ) from onadata.libs.serializers.fields.organization_field import OrganizationField -from onadata.libs.serializers.share_project_serializer import ShareProjectSerializer -from onadata.libs.utils.project_utils import propagate_project_permissions_async -from onadata.settings.common import DEFAULT_FROM_EMAIL, SHARE_ORG_SUBJECT +from onadata.settings.common import SHARE_ORG_SUBJECT User = get_user_model() -def _compose_send_email(organization, user, email_msg, email_subject=None): - - if not email_subject: - email_subject = SHARE_ORG_SUBJECT.format(user.username, organization.name) - - # send out email message. - send_mail(email_subject, email_msg, DEFAULT_FROM_EMAIL, (user.email,)) - - -def _set_organization_role_to_user(organization, user, role): - role_cls = ROLES.get(role) - if role_cls: - role_cls.add(user, organization) - - owners_team = get_or_create_organization_owners_team(organization) - members_team = get_organization_members_team(organization) - - # add user to their respective team - if role == OwnerRole.name: - # add user to owners team - role_cls.add(user, organization.userprofile_ptr) - add_user_to_team(owners_team, user) - # add user to org projects - for project in organization.user.project_org.all(): - data = {"project": project.pk, "username": user.username, "role": role} - serializer = ShareProjectSerializer(data=data) - if serializer.is_valid(): - serializer.save() - - elif role != OwnerRole.name: - add_user_to_team(members_team, user) - # add user to org projects - for project in organization.user.project_org.all(): - if role != ManagerRole.name: - role = get_team_project_default_permissions(members_team, project) - else: - if project.created_by != user: - role = get_team_project_default_permissions(members_team, project) - - data = {"project": project.pk, "username": user.username, "role": role} - serializer = ShareProjectSerializer(data=data) - if serializer.is_valid(): - serializer.save() - # remove user from owners team - remove_user_from_team(owners_team, user) - - class OrganizationMemberSerializer(serializers.Serializer): """ The OrganizationMemberSerializer - manages a users access in an organization @@ -155,29 +100,20 @@ def create(self, validated_data): username = validated_data.get("username") role = validated_data.get("role") email_msg = validated_data.get("email_msg") - email_subject = validated_data.get("email_subject") + email_subject = validated_data.get( + "email_subject", SHARE_ORG_SUBJECT.format(username, organization.name) + ) remove = validated_data.get("remove") if username: user = User.objects.get(username=username) - add_user_to_organization(organization, user) - _set_organization_role_to_user(organization, user, role) - - if email_msg: - _compose_send_email(organization, user, email_msg, email_subject) - if remove: - remove_user_from_organization(organization, user) - - projects = Project.objects.filter( - organization=organization.user, deleted_at__isnull=True - ) - for project in projects.iterator(): - # Queue permission propagation with a - # delay for permissions to be effected - propagate_project_permissions_async.apply_async( - args=[project.id], countdown=60 + remove_org_user_async.apply_async(args=[organization.pk, user.pk]) + + else: + add_org_user_and_share_projects_async.apply_async( + args=[organization.pk, user.pk, role, email_subject, email_msg] ) return organization diff --git a/onadata/libs/serializers/share_project_serializer.py b/onadata/libs/serializers/share_project_serializer.py index 40938addad..768abcd20c 100644 --- a/onadata/libs/serializers/share_project_serializer.py +++ b/onadata/libs/serializers/share_project_serializer.py @@ -7,10 +7,10 @@ from rest_framework import serializers +from onadata.apps.api.tasks import share_project_async from onadata.libs.models.share_project import ShareProject from onadata.libs.permissions import ROLES, OwnerRole, get_object_users_with_permissions from onadata.libs.serializers.fields.project_field import ProjectField -from onadata.libs.utils.project_utils import propagate_project_permissions_async User = get_user_model() @@ -40,19 +40,18 @@ def create(self, validated_data): for username in validated_data.pop("username").split(","): validated_data["username"] = username instance = ShareProject(**validated_data) - instance.save() created_instances.append(instance) + share_project_async.apply_async( + args=[instance.project.pk, instance.username, instance.role] + ) - propagate_project_permissions_async.apply_async( - args=[validated_data.get("project").id], countdown=30 - ) return created_instances def update(self, instance, validated_data): instance = attrs_to_instance(validated_data, instance) - instance.save() - propagate_project_permissions_async.apply_async( - args=[validated_data.get("project").id], countdown=30 + + share_project_async.apply_async( + args=[instance.project.pk, instance.username, instance.role] ) return instance @@ -119,22 +118,10 @@ class RemoveUserFromProjectSerializer(ShareProjectSerializer): remove = serializers.BooleanField() def update(self, instance, validated_data): - instance = attrs_to_instance(validated_data, instance) - instance.save() - propagate_project_permissions_async.apply_async( - args=[validated_data.get("project").id], countdown=30 - ) - - return instance + return attrs_to_instance(validated_data, instance) def create(self, validated_data): - instance = ShareProject(**validated_data) - instance.save() - propagate_project_permissions_async.apply_async( - args=[validated_data.get("project").id], countdown=30 - ) - - return instance + return ShareProject(**validated_data) def validate(self, attrs): """Check and confirm that the project will be left with at least one @@ -152,3 +139,16 @@ def validate(self, attrs): ) return attrs + + def save(self, **kwargs): + instance = super().save(**kwargs) + share_project_async.apply_async( + args=[ + instance.project.pk, + instance.username, + instance.role, + instance.remove, + ] + ) + + return instance diff --git a/onadata/libs/tests/models/__init__.py b/onadata/libs/tests/models/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/onadata/libs/tests/models/test_share_project.py b/onadata/libs/tests/models/test_share_project.py new file mode 100644 index 0000000000..3f2f9d7f16 --- /dev/null +++ b/onadata/libs/tests/models/test_share_project.py @@ -0,0 +1,93 @@ +"""Tests for module onadata.libs.models.share_project""" + +from unittest.mock import patch, call + +from onadata.apps.logger.models.data_view import DataView +from onadata.apps.logger.models.project import Project +from onadata.apps.logger.models.xform import XForm +from onadata.apps.main.tests.test_base import TestBase +from onadata.libs.models.share_project import ShareProject +from onadata.libs.permissions import ManagerRole + + +@patch( + "onadata.libs.models.share_project.propagate_project_permissions_async.apply_async" +) +class ShareProjectTestCase(TestBase): + """Tests for model ShareProject""" + + def setUp(self): + super().setUp() + + self._publish_transportation_form() + md_xform = """ + | survey | + | | type | name | label | + | | text | name | Name | + | | integer | age | Age | + | | select one fruits | fruit | Fruit | + | | | | | + | choices | list name | name | label | + | | fruits | 1 | Mango | + | | fruits | 2 | Orange | + | | fruits | 3 | Apple | + """ + project = Project.objects.create( + name="Demo", organization=self.user, created_by=self.user + ) + self._publish_markdown(md_xform, self.user, project) + self.dataview_form = XForm.objects.all().order_by("-pk")[0] + DataView.objects.create( + name="Demo", + xform=self.dataview_form, + project=self.project, + matches_parent=True, + columns=[], + ) + self.alice = self._create_user("alice", "Yuao8(-)") + + @patch("onadata.libs.models.share_project.safe_delete") + def test_share(self, mock_safe_delete, mock_propagate): + """A project is shared with a user + + Permissions assigned to project, xform and dataview + """ + instance = ShareProject(self.project, self.alice, "manager") + instance.save() + self.assertTrue(ManagerRole.user_has_role(self.alice, self.project)) + self.assertTrue(ManagerRole.user_has_role(self.alice, self.xform)) + self.assertTrue(ManagerRole.user_has_role(self.alice, self.dataview_form)) + mock_propagate.assert_called_once_with(args=[self.project.pk]) + # Cache is invalidated + mock_safe_delete.assert_has_calls( + [ + call(f"ps-project_owner-{self.project.pk}"), + call(f"ps-project_permissions-{self.project.pk}"), + ] + ) + + @patch("onadata.libs.models.share_project.safe_delete") + def test_remove(self, mock_safe_delete, mock_propagate): + """A user is removed from a project""" + # Add user + ManagerRole.add(self.alice, self.project) + ManagerRole.add(self.alice, self.xform) + ManagerRole.add(self.alice, self.dataview_form) + + self.assertTrue(ManagerRole.user_has_role(self.alice, self.project)) + self.assertTrue(ManagerRole.user_has_role(self.alice, self.xform)) + self.assertTrue(ManagerRole.user_has_role(self.alice, self.dataview_form)) + # Remove user + instance = ShareProject(self.project, self.alice, "manager", True) + instance.save() + self.assertFalse(ManagerRole.user_has_role(self.alice, self.project)) + self.assertFalse(ManagerRole.user_has_role(self.alice, self.xform)) + self.assertFalse(ManagerRole.user_has_role(self.alice, self.dataview_form)) + mock_propagate.assert_called_once_with(args=[self.project.pk]) + # Cache is invalidated + mock_safe_delete.assert_has_calls( + [ + call(f"ps-project_owner-{self.project.pk}"), + call(f"ps-project_permissions-{self.project.pk}"), + ] + )