From 19a52ebbdd0780343c6db8be5c2c3354b605c91a Mon Sep 17 00:00:00 2001 From: Kelvin Muchiri Date: Wed, 9 Oct 2024 13:53:05 +0300 Subject: [PATCH] Add/remove user from organization synchronously (#2715) * add/remove user from organization synchronously Since an organization is a Group, adding/removing user from an organization does not take long and can be done synchronously. Sharing organization projects will take longer and remains asynchronous * fix lint error cyclic-import * add docstring * ignore items order in test * add comment * fix failing tests --- onadata/apps/api/tasks.py | 67 +------ onadata/apps/api/tests/test_tasks.py | 176 +----------------- onadata/apps/api/tests/test_tools.py | 86 +-------- .../test_organization_profile_viewset.py | 13 +- onadata/apps/api/tools.py | 91 +++++---- .../organization_member_serializer.py | 22 ++- 6 files changed, 88 insertions(+), 367 deletions(-) diff --git a/onadata/apps/api/tasks.py b/onadata/apps/api/tasks.py index 1313e2722d..3ecedc4320 100644 --- a/onadata/apps/api/tasks.py +++ b/onadata/apps/api/tasks.py @@ -11,16 +11,13 @@ 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.apps.api.tools import invalidate_organization_cache from onadata.celeryapp import app from onadata.libs.utils.email import send_generic_email from onadata.libs.utils.model_tools import queryset_iterator @@ -191,69 +188,7 @@ def regenerate_form_instance_json(xform_id: int): safe_delete(cache_key) -class ShareProjectBaseTask(app.Task): # pylint: disable=too-few-public-methods - """A Task base class for sharing a project.""" - - 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) - - invalidate_organization_cache(organization.user.username) - - 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) - - invalidate_organization_cache(organization.user.username) - - -@app.task(base=ShareProjectBaseTask) +@app.task(retry_backoff=3, autoretry_for=(DatabaseError, ConnectionError)) def share_project_async(project_id, username, role, remove=False): """Share project asynchronously""" try: diff --git a/onadata/apps/api/tests/test_tasks.py b/onadata/apps/api/tests/test_tasks.py index 7b7ad5b137..d6c6f1c42f 100644 --- a/onadata/apps/api/tests/test_tasks.py +++ b/onadata/apps/api/tests/test_tasks.py @@ -7,17 +7,13 @@ 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.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 @@ -122,180 +118,12 @@ def mock_get_full_dict( instance.refresh_from_db() self.assertFalse(instance.json) + def set_cache_for_org(org, request): """Utility to set org cache""" - org_profile_json = OrganizationSerializer( - org, context={"request": request} - ).data + org_profile_json = OrganizationSerializer(org, context={"request": request}).data cache.set(f"{ORG_PROFILE_CACHE}{org.user.username}-owner", org_profile_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 - ) - self.extra = {"HTTP_AUTHORIZATION": f"Token {self.user.auth_token}"} - - def test_user_added_to_org(self, mock_add): - """User is added to organization""" - request = self.factory.get("/", **self.extra) - request.user = self.user - set_cache_for_org(self.org, request) - cache_key = f"{ORG_PROFILE_CACHE}{self.org.user.username}-owner" - self.assertIsNotNone(cache.get(cache_key)) - 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") - self.assertEqual(cache.get(cache_key), None) - - 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 - ) - self.extra = {"HTTP_AUTHORIZATION": f"Token {self.user.auth_token}"} - - def test_user_removed_from_org(self, mock_remove): - """User is removed from organization""" - request = self.factory.get("/", **self.extra) - request.user = self.user - set_cache_for_org(self.org, request) - cache_key = f"{ORG_PROFILE_CACHE}{self.org.user.username}-owner" - self.assertIsNotNone(cache.get(cache_key)) - remove_org_user_async.delay(self.org.pk, self.user.pk) - mock_remove.assert_called_once_with(self.org, self.user) - self.assertEqual(cache.get(cache_key), None) - - @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""" diff --git a/onadata/apps/api/tests/test_tools.py b/onadata/apps/api/tests/test_tools.py index ac3c59f4f5..8e8115c5cc 100644 --- a/onadata/apps/api/tests/test_tools.py +++ b/onadata/apps/api/tests/test_tools.py @@ -7,90 +7,16 @@ Team, get_organization_members_team, ) -from onadata.apps.api.tools import ( - add_org_user_and_share_projects, - add_user_to_organization, -) +from onadata.apps.api.tools import 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""" + """Tests for add_user_to_organization""" def setUp(self) -> None: super().setUp() @@ -106,7 +32,7 @@ def setUp(self) -> None: def test_add_owner(self): """Owner added to org and projects shared""" - add_org_user_and_share_projects(self.org, self.user, "owner") + 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") @@ -129,7 +55,7 @@ def test_non_owner(self): 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") + 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") @@ -148,7 +74,7 @@ def test_project_created_by_manager(self): self.project.created_by = self.user self.project.save() - add_org_user_and_share_projects(self.org, self.user, "manager") + add_user_to_organization(self.org, self.user, "manager") self.assertTrue(ManagerRole.user_has_role(self.user, self.project)) @@ -158,7 +84,7 @@ def test_role_none(self): members_team = get_organization_members_team(self.org) DataEntryRole.add(members_team, self.project) - add_org_user_and_share_projects(self.org, self.user) + 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") 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 d86e2966f5..2110d1e840 100644 --- a/onadata/apps/api/tests/viewsets/test_organization_profile_viewset.py +++ b/onadata/apps/api/tests/viewsets/test_organization_profile_viewset.py @@ -734,7 +734,7 @@ def test_put_bad_role(self): self.assertEqual(response.status_code, 400) @override_settings(DEFAULT_FROM_EMAIL="noreply@ona.io") - @patch("onadata.apps.api.tasks.send_mail") + @patch("onadata.libs.serializers.organization_member_serializer.send_mail") def test_add_members_to_org_email(self, mock_email): self._org_create() view = OrganizationProfileViewSet.as_view({"post": "members"}) @@ -759,7 +759,7 @@ def test_add_members_to_org_email(self, mock_email): self.assertEqual(set(response.data), set(["denoinc", "aboy"])) @override_settings(DEFAULT_FROM_EMAIL="noreply@ona.io") - @patch("onadata.apps.api.tasks.send_mail") + @patch("onadata.libs.serializers.organization_member_serializer.send_mail") def test_add_members_to_org_email_custom_subj(self, mock_email): self._org_create() view = OrganizationProfileViewSet.as_view({"post": "members"}) @@ -809,8 +809,13 @@ def test_add_members_to_org_with_role(self): request = self.factory.get("/", **self.extra) response = view(request, user="denoinc") self.assertEqual(response.status_code, 200) - self.assertEqual(response.data["users"][2]["user"], "aboy") - self.assertEqual(response.data["users"][2]["role"], "editor") + # items can be in any order + self.assertTrue( + any( + item["user"] == "aboy" and item["role"] == "editor" + for item in response.data["users"] + ) + ) def test_add_members_to_owner_role(self): self._org_create() diff --git a/onadata/apps/api/tools.py b/onadata/apps/api/tools.py index 46835451c4..190218d383 100644 --- a/onadata/apps/api/tools.py +++ b/onadata/apps/api/tools.py @@ -2,6 +2,7 @@ """ API utility functions. """ +import importlib import os import tempfile from datetime import datetime @@ -48,7 +49,6 @@ from onadata.apps.main.models.user_profile import UserProfile from onadata.apps.viewer.models.parsed_instance import datetime_from_str from onadata.libs.baseviewset import DefaultBaseViewset -from onadata.libs.models.share_project import ShareProject from onadata.libs.permissions import ( ROLES, ROLES_ORDERED, @@ -195,7 +195,14 @@ def create_organization_object(org_name, creator, attrs=None): def remove_user_from_organization(organization, user): - """Remove a user from an organization""" + """Remove a user from an organization + + Remove user from organization and all projects in the organization + + :param organization: OrganizationProfile instance + :param user: User instance + :return: None + """ team = get_organization_members_team(organization) remove_user_from_team(team, user) owners_team = get_or_create_organization_owners_team(organization) @@ -209,11 +216,19 @@ def remove_user_from_organization(organization, user): role_cls.remove_obj_permissions(user, organization) role_cls.remove_obj_permissions(user, organization.userprofile_ptr) + # Invalidate organization cache + invalidate_organization_cache(organization.user.username) + + # Avoid cyclic dependency errors + api_tasks = importlib.import_module("onadata.apps.api.tasks") + # Remove user from all org projects project_qs = organization.user.project_org.all() for project in queryset_iterator(project_qs): - ShareProject(project, user.username, role, remove=True).save() + api_tasks.share_project_async.delay( + project.pk, user.username, role, remove=True + ) def remove_user_from_team(team, user): @@ -236,7 +251,15 @@ def remove_user_from_team(team, user): def add_user_to_organization(organization, user, role=None): - """Add a user to an organization""" + """Add a user to an organization + + Add user to organization and all projects in the organization + + :param organization: OrganizationProfile instance + :param user: User instance + :param role: Role name + :return: None + """ team = get_organization_members_team(organization) add_user_to_team(team, user) @@ -256,6 +279,35 @@ def add_user_to_organization(organization, user, role=None): remove_user_from_team(owners_team, user) OwnerRole.remove_obj_permissions(user, organization.userprofile_ptr) + # Invalidate organization cache + invalidate_organization_cache(organization.user.username) + + # Avoid cyclic dependency errors + api_tasks = importlib.import_module("onadata.apps.api.tasks") + + # Share all organization projects with the new user + project_qs = organization.user.project_org.all() + + if role == OwnerRole.name: + # New owners have owner role on all projects + for project in queryset_iterator(project_qs): + api_tasks.share_project_async.delay(project.pk, user.username, 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 role == ManagerRole.name and project.created_by == user: + # New managers are only granted the manager role on the + # projects they created + api_tasks.share_project_async.delay(project.pk, user.username, role) + else: + project_role = get_team_project_default_permissions(team, project) + api_tasks.share_project_async.delay( + project.pk, user.username, project_role + ) + def get_organization_members(organization): """Get members team user queryset""" @@ -845,37 +897,6 @@ def set_enketo_signed_cookies(resp, username=None, json_web_token=None): 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) - - def get_org_profile_cache_key(user, organization): """Return cache key given user and organization profile""" org_username = organization.user.username diff --git a/onadata/libs/serializers/organization_member_serializer.py b/onadata/libs/serializers/organization_member_serializer.py index 1a73e0dfb4..2b23003a52 100644 --- a/onadata/libs/serializers/organization_member_serializer.py +++ b/onadata/libs/serializers/organization_member_serializer.py @@ -2,18 +2,18 @@ """ The OrganizationMemberSerializer - manages a users access in an organization """ +from django.conf import settings 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.tools import ( _get_owners, + add_user_to_organization, get_organization_members, -) -from onadata.apps.api.tasks import ( - add_org_user_and_share_projects_async, - remove_org_user_async, + remove_user_from_organization, ) from onadata.apps.main.models.user_profile import UserProfile from onadata.libs.permissions import ( @@ -108,12 +108,18 @@ def create(self, validated_data): user = User.objects.get(username=username) if remove: - remove_org_user_async.apply_async(args=[organization.pk, user.pk]) + remove_user_from_organization(organization, user) else: - add_org_user_and_share_projects_async.apply_async( - args=[organization.pk, user.pk, role, email_subject, email_msg] - ) + add_user_to_organization(organization, user, role) + + if email_msg and email_subject and user.email: + send_mail( + email_subject, + email_msg, + settings.DEFAULT_FROM_EMAIL, + (user.email,), + ) return organization