From e17f6a7e71aaf1e6b16276d03e4de38365062be7 Mon Sep 17 00:00:00 2001 From: rajpatel24 Date: Thu, 7 Nov 2024 01:04:42 +0530 Subject: [PATCH 01/13] Add endpoints to handle organization members --- kobo/apps/organizations/serializers.py | 52 ++++- .../tests/test_organization_members_api.py | 80 ++++++++ kobo/apps/organizations/views.py | 188 +++++++++++++++++- kpi/paginators.py | 9 +- kpi/urls/router_api_v2.py | 8 +- 5 files changed, 329 insertions(+), 8 deletions(-) create mode 100644 kobo/apps/organizations/tests/test_organization_members_api.py diff --git a/kobo/apps/organizations/serializers.py b/kobo/apps/organizations/serializers.py index 21d73e131d..25dd405c30 100644 --- a/kobo/apps/organizations/serializers.py +++ b/kobo/apps/organizations/serializers.py @@ -1,4 +1,8 @@ +from constance import config +from django.contrib.auth import get_user_model +from django.utils.translation import gettext as t from rest_framework import serializers +from rest_framework.reverse import reverse from kobo.apps.organizations.models import ( create_organization, @@ -11,10 +15,56 @@ class OrganizationUserSerializer(serializers.ModelSerializer): + user = serializers.HyperlinkedRelatedField( + queryset=get_user_model().objects.all(), + lookup_field='username', + view_name='user-kpi-detail', + ) + role = serializers.CharField() + has_mfa_enabled = serializers.SerializerMethodField() + url = serializers.SerializerMethodField() + date_joined = serializers.DateTimeField( + source='user.date_joined', format='%Y-%m-%dT%H:%M:%SZ' + ) + user__username = serializers.ReadOnlyField(source='user.username') + user__name = serializers.ReadOnlyField(source='user.get_full_name') + user__email = serializers.ReadOnlyField(source='user.email') + is_active = serializers.ReadOnlyField(source='user.is_active') class Meta: model = OrganizationUser - fields = ['user', 'organization'] + fields = [ + 'url', + 'user', + 'user__username', + 'user__email', + 'user__name', + 'role', + 'has_mfa_enabled', + 'date_joined', + 'is_active' + ] + + def get_has_mfa_enabled(self, obj): + return config.MFA_ENABLED + + def get_url(self, obj): + request = self.context.get('request') + return reverse( + 'organization-members-detail', + kwargs={ + 'organization_id': obj.organization.id, + 'user__username': obj.user.username + }, + request=request + ) + + def validate_role(self, role): + if role not in ['admin', 'member']: + raise serializers.ValidationError( + {'role': t("Invalid role. Only 'admin' or 'member' are allowed")} + ) + return role class OrganizationOwnerSerializer(serializers.ModelSerializer): diff --git a/kobo/apps/organizations/tests/test_organization_members_api.py b/kobo/apps/organizations/tests/test_organization_members_api.py new file mode 100644 index 0000000000..7d0045be6a --- /dev/null +++ b/kobo/apps/organizations/tests/test_organization_members_api.py @@ -0,0 +1,80 @@ +from django.urls import reverse +from model_bakery import baker +from rest_framework import status + +from kobo.apps.kobo_auth.shortcuts import User +from kobo.apps.organizations.models import Organization, OrganizationUser +from kpi.tests.kpi_test_case import BaseTestCase +from kpi.urls.router_api_v2 import URL_NAMESPACE + + +class OrganizationMemberAPITestCase(BaseTestCase): + fixtures = ['test_data'] + URL_NAMESPACE = URL_NAMESPACE + + def setUp(self): + self.organization = baker.make(Organization, id='org_12345') + self.owner_user = baker.make(User, username='owner') + self.member_user = baker.make(User, username='member') + self.invited_user = baker.make(User, username='invited') + + self.organization_user_owner = baker.make( + OrganizationUser, + organization=self.organization, + user=self.owner_user, + is_admin=True, + ) + self.organization_user_member = baker.make( + OrganizationUser, + organization=self.organization, + user=self.member_user + ) + + self.client.force_login(self.owner_user) + self.list_url = reverse( + self._get_endpoint('organization-members-list'), + kwargs={'organization_id': self.organization.id}, + ) + self.detail_url = lambda username: reverse( + self._get_endpoint('organization-members-detail'), + kwargs={ + 'organization_id': self.organization.id, + 'user__username': username + }, + ) + + def test_list_members(self): + response = self.client.get(self.list_url) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertIn( + 'owner', + [member['user__username'] for member in response.data.get('results')] + ) + self.assertIn( + 'member', + [member['user__username'] for member in response.data.get('results')] + ) + + def test_retrieve_member_details(self): + response = self.client.get(self.detail_url('member')) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.data['user__username'], 'member') + self.assertEqual(response.data['role'], 'member') + + def test_update_member_role(self): + data = {'role': 'admin'} + response = self.client.patch(self.detail_url('member'), data) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertEqual(response.data['role'], 'admin') + + def test_delete_member(self): + response = self.client.delete(self.detail_url('member')) + self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) + # Confirm deletion + response = self.client.get(self.detail_url('member')) + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + + def test_list_requires_authentication(self): + self.client.logout() + response = self.client.get(self.list_url) + self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) diff --git a/kobo/apps/organizations/views.py b/kobo/apps/organizations/views.py index 7102f513bd..7792d1a1ee 100644 --- a/kobo/apps/organizations/views.py +++ b/kobo/apps/organizations/views.py @@ -1,26 +1,34 @@ from django.conf import settings from django.contrib.postgres.aggregates import ArrayAgg -from django.db.models import QuerySet +from django.db.models import ( + QuerySet, + Case, + When, + Value, + CharField, + OuterRef, +) +from django.db.models.expressions import Exists from django.utils.decorators import method_decorator from django.views.decorators.cache import cache_page from django_dont_vary_on.decorators import only_vary_on -from kpi import filters from rest_framework import viewsets, status from rest_framework.decorators import action from rest_framework.response import Response +from kpi import filters from kpi.constants import ASSET_TYPE_SURVEY from kpi.models.asset import Asset -from kpi.paginators import AssetUsagePagination +from kpi.paginators import AssetUsagePagination, OrganizationPagination from kpi.permissions import IsAuthenticated from kpi.serializers.v2.service_usage import ( CustomAssetUsageSerializer, ServiceUsageSerializer, ) from kpi.utils.object_permission import get_database_user -from .models import Organization +from .models import Organization, OrganizationOwner, OrganizationUser from .permissions import IsOrgAdminOrReadOnly -from .serializers import OrganizationSerializer +from .serializers import OrganizationSerializer, OrganizationUserSerializer from ..stripe.constants import ACTIVE_STRIPE_STATUSES @@ -194,3 +202,173 @@ def asset_usage(self, request, pk=None, *args, **kwargs): page, many=True, context=context ) return self.get_paginated_response(serializer.data) + + +class OrganizationMemberViewSet(viewsets.ModelViewSet): + """ + * Manage organization members and their roles within an organization. + * Run a partial update on an organization member to promote or demote. + + ## Organization Members API + + This API allows authorized users to view and manage the members of an + organization, including their roles. It handles existing members. It also + allows updating roles, such as promoting a member to an admin or assigning + a new owner. + + ### List Members + + Retrieves all members in the specified organization. + + 
+    GET /api/v2/organizations/{organization_id}/members/
+
+ + > Example + > + > curl -X GET https://[kpi]/api/v2/organizations/org_12345/members/ + + > Response 200 + + > { + > "count": 2, + > "next": null, + > "previous": null, + > "results": [ + > { + > "url": "http://[kpi]/api/v2/organizations/org_12345/ \ + > members/foo_bar/", + > "user": "http://[kpi]/api/v2/users/foo_bar/", + > "user__username": "foo_bar", + > "user__email": "foo_bar@example.com", + > "user__name": "Foo Bar", + > "role": "owner", + > "has_mfa_enabled": true, + > "date_joined": "2024-08-11T12:36:32Z", + > "is_active": true + > }, + > { + > "url": "http://[kpi]/api/v2/organizations/org_12345/ \ + > members/john_doe/", + > "user": "http://[kpi]/api/v2/users/john_doe/", + > "user__username": "john_doe", + > "user__email": "john_doe@example.com", + > "user__name": "John Doe", + > "role": "admin", + > "has_mfa_enabled": false, + > "date_joined": "2024-10-21T06:38:45Z", + > "is_active": true + > } + > ] + > } + + The response includes detailed information about each member, such as their + username, email, role (owner, admin, member), and account status. + + ### Retrieve Member Details + + Retrieves the details of a specific member within an organization by username. + + 
+    GET /api/v2/organizations/{organization_id}/members/{username}/
+
+ + > Example + > + > curl -X GET https://[kpi]/api/v2/organizations/org_12345/members/foo_bar/ + + > Response 200 + + > { + > "url": "http://[kpi]/api/v2/organizations/org_12345/members/foo_bar/", + > "user": "http://[kpi]/api/v2/users/foo_bar/", + > "user__username": "foo_bar", + > "user__email": "foo_bar@example.com", + > "user__name": "Foo Bar", + > "role": "owner", + > "has_mfa_enabled": true, + > "date_joined": "2024-08-11T12:36:32Z", + > "is_active": true + > } + + ### Update Member Role + + Updates the role of a member within the organization to `owner`, `admin`, or + `member`. + + 
+    PATCH /api/v2/organizations/{organization_id}/members/{username}/
+
+ + #### Payload + > { + > "role": "admin" + > } + + - **admin**: Grants the member admin privileges within the organization + - **member**: Revokes admin privileges, setting the member as a regular user + + > Example + > + > curl -X PATCH https://[kpi]/api/v2/organizations/org_12345/ \ + > members/demo_user/ -d '{"role": "admin"}' + + ### Remove Member + + Removes a member from the organization. + + 
+    DELETE /api/v2/organizations/{organization_id}/members/{username}/
+
+ + > Example + > + > curl -X DELETE https://[kpi]/api/v2/organizations/org_12345/members/foo_bar/ + + ## Permissions + + - The user must be authenticated to perform these actions. + + ## Notes + + - **Role Validation**: Only valid roles ('admin', 'member') are accepted + in updates. + """ + serializer_class = OrganizationUserSerializer + permission_classes = [IsAuthenticated] + pagination_class = OrganizationPagination + lookup_field = 'user__username' + + def get_queryset(self): + organization_id = self.kwargs['organization_id'] + + # Subquery to check if the user is the owner + owner_subquery = OrganizationOwner.objects.filter( + organization_id=organization_id, + organization_user=OuterRef('pk') + ).values('pk') + + # Annotate with role based on organization ownership and admin status + queryset = OrganizationUser.objects.filter( + organization_id=organization_id + ).annotate( + role=Case( + When(Exists(owner_subquery), then=Value('owner')), + When(is_admin=True, then=Value('admin')), + default=Value('member'), + output_field=CharField() + ) + ) + return queryset + + def partial_update(self, request, *args, **kwargs): + instance = self.get_object() + serializer = self.get_serializer( + instance, data=request.data, partial=True + ) + serializer.is_valid(raise_exception=True) + role = serializer.validated_data.get('role') + if role: + instance.is_admin = (role == 'admin') + instance.save() + return super().partial_update(request, *args, **kwargs) diff --git a/kpi/paginators.py b/kpi/paginators.py index 4b0b9ea667..33f29da90f 100644 --- a/kpi/paginators.py +++ b/kpi/paginators.py @@ -121,7 +121,7 @@ class DataPagination(LimitOffsetPagination): offset_query_param = 'start' max_limit = settings.SUBMISSION_LIST_LIMIT - + class FastAssetPagination(Paginated): """ Pagination class optimized for faster counting for DISTINCT queries on large tables. @@ -142,3 +142,10 @@ class TinyPaginated(PageNumberPagination): Same as Paginated with a small page size """ page_size = 50 + + +class OrganizationPagination(PageNumberPagination): + """ + Pagination class for Organization + """ + page_size = 10 diff --git a/kpi/urls/router_api_v2.py b/kpi/urls/router_api_v2.py index 7c7dbd2575..411d77de2b 100644 --- a/kpi/urls/router_api_v2.py +++ b/kpi/urls/router_api_v2.py @@ -6,7 +6,7 @@ from kobo.apps.hook.views.v2.hook import HookViewSet from kobo.apps.hook.views.v2.hook_log import HookLogViewSet from kobo.apps.languages.urls import router as language_router -from kobo.apps.organizations.views import OrganizationViewSet +from kobo.apps.organizations.views import OrganizationViewSet, OrganizationMemberViewSet from kobo.apps.project_ownership.urls import router as project_ownership_router from kobo.apps.project_views.views import ProjectViewViewSet from kpi.views.v2.asset import AssetViewSet @@ -140,6 +140,12 @@ def get_urls(self, *args, **kwargs): router_api_v2.register(r'imports', ImportTaskViewSet) router_api_v2.register(r'organizations', OrganizationViewSet, basename='organizations',) +router_api_v2.register( + r'organizations/(?P[^/.]+)/members', + OrganizationMemberViewSet, + basename='organization-members', +) + router_api_v2.register(r'permissions', PermissionViewSet) router_api_v2.register(r'project-views', ProjectViewViewSet) router_api_v2.register(r'service_usage', From 0401fc479ba3e678eb186b8ca0ca5f1809cf1c61 Mon Sep 17 00:00:00 2001 From: rajpatel24 Date: Wed, 13 Nov 2024 15:33:22 +0530 Subject: [PATCH 02/13] Refactor organization member API to eliminate redundancy and optimize query for user mfa details --- kobo/apps/organizations/serializers.py | 16 ++++++++++------ .../tests/test_organization_members_api.py | 18 +++++------------- kobo/apps/organizations/views.py | 15 ++++++++++++--- kpi/paginators.py | 2 +- 4 files changed, 28 insertions(+), 23 deletions(-) diff --git a/kobo/apps/organizations/serializers.py b/kobo/apps/organizations/serializers.py index 25dd405c30..a68abeba2c 100644 --- a/kobo/apps/organizations/serializers.py +++ b/kobo/apps/organizations/serializers.py @@ -21,7 +21,9 @@ class OrganizationUserSerializer(serializers.ModelSerializer): view_name='user-kpi-detail', ) role = serializers.CharField() - has_mfa_enabled = serializers.SerializerMethodField() + user__has_mfa_enabled = serializers.BooleanField( + source='has_mfa_enabled', read_only=True + ) url = serializers.SerializerMethodField() date_joined = serializers.DateTimeField( source='user.date_joined', format='%Y-%m-%dT%H:%M:%SZ' @@ -29,7 +31,7 @@ class OrganizationUserSerializer(serializers.ModelSerializer): user__username = serializers.ReadOnlyField(source='user.username') user__name = serializers.ReadOnlyField(source='user.get_full_name') user__email = serializers.ReadOnlyField(source='user.email') - is_active = serializers.ReadOnlyField(source='user.is_active') + user__is_active = serializers.ReadOnlyField(source='user.is_active') class Meta: model = OrganizationUser @@ -40,13 +42,15 @@ class Meta: 'user__email', 'user__name', 'role', - 'has_mfa_enabled', + 'user__has_mfa_enabled', 'date_joined', - 'is_active' + 'user__is_active' ] - def get_has_mfa_enabled(self, obj): - return config.MFA_ENABLED + def update(self, instance, validated_data): + if role := validated_data.get('role', None): + validated_data['is_admin'] = role == 'admin' + return super().update(instance, validated_data) def get_url(self, obj): request = self.context.get('request') diff --git a/kobo/apps/organizations/tests/test_organization_members_api.py b/kobo/apps/organizations/tests/test_organization_members_api.py index 7d0045be6a..d9cd447a2b 100644 --- a/kobo/apps/organizations/tests/test_organization_members_api.py +++ b/kobo/apps/organizations/tests/test_organization_members_api.py @@ -13,22 +13,14 @@ class OrganizationMemberAPITestCase(BaseTestCase): URL_NAMESPACE = URL_NAMESPACE def setUp(self): - self.organization = baker.make(Organization, id='org_12345') + self.organization = baker.make( + Organization, id='org_12345', mmo_override=True + ) self.owner_user = baker.make(User, username='owner') self.member_user = baker.make(User, username='member') - self.invited_user = baker.make(User, username='invited') - self.organization_user_owner = baker.make( - OrganizationUser, - organization=self.organization, - user=self.owner_user, - is_admin=True, - ) - self.organization_user_member = baker.make( - OrganizationUser, - organization=self.organization, - user=self.member_user - ) + self.organization.add_user(self.owner_user) + self.organization.add_user(self.member_user, is_admin=False) self.client.force_login(self.owner_user) self.list_url = reverse( diff --git a/kobo/apps/organizations/views.py b/kobo/apps/organizations/views.py index 7792d1a1ee..0bbcc84551 100644 --- a/kobo/apps/organizations/views.py +++ b/kobo/apps/organizations/views.py @@ -19,7 +19,7 @@ from kpi import filters from kpi.constants import ASSET_TYPE_SURVEY from kpi.models.asset import Asset -from kpi.paginators import AssetUsagePagination, OrganizationPagination +from kpi.paginators import AssetUsagePagination, OrganizationMemberPagination from kpi.permissions import IsAuthenticated from kpi.serializers.v2.service_usage import ( CustomAssetUsageSerializer, @@ -29,6 +29,7 @@ from .models import Organization, OrganizationOwner, OrganizationUser from .permissions import IsOrgAdminOrReadOnly from .serializers import OrganizationSerializer, OrganizationUserSerializer +from ..accounts.mfa.models import MfaMethod from ..stripe.constants import ACTIVE_STRIPE_STATUSES @@ -336,12 +337,19 @@ class OrganizationMemberViewSet(viewsets.ModelViewSet): """ serializer_class = OrganizationUserSerializer permission_classes = [IsAuthenticated] - pagination_class = OrganizationPagination + pagination_class = OrganizationMemberPagination + http_method_names = ['get', 'patch', 'delete'] lookup_field = 'user__username' def get_queryset(self): organization_id = self.kwargs['organization_id'] + # Subquery to check if the user has an active MFA method + mfa_subquery = MfaMethod.objects.filter( + user=OuterRef('user_id'), + is_active=True + ).values('pk') + # Subquery to check if the user is the owner owner_subquery = OrganizationOwner.objects.filter( organization_id=organization_id, @@ -357,7 +365,8 @@ def get_queryset(self): When(is_admin=True, then=Value('admin')), default=Value('member'), output_field=CharField() - ) + ), + has_mfa_enabled=Exists(mfa_subquery) ) return queryset diff --git a/kpi/paginators.py b/kpi/paginators.py index 33f29da90f..34a7fe9ee5 100644 --- a/kpi/paginators.py +++ b/kpi/paginators.py @@ -144,7 +144,7 @@ class TinyPaginated(PageNumberPagination): page_size = 50 -class OrganizationPagination(PageNumberPagination): +class OrganizationMemberPagination(PageNumberPagination): """ Pagination class for Organization """ From 3c174df4d80e2a0e0edb9bcaee047fcbc4c7ca79 Mon Sep 17 00:00:00 2001 From: rajpatel24 Date: Thu, 14 Nov 2024 21:14:39 +0530 Subject: [PATCH 03/13] Add role-based validation tests for organization member permissions --- kobo/apps/organizations/permissions.py | 31 ++++- kobo/apps/organizations/serializers.py | 1 - .../tests/test_organization_members_api.py | 118 ++++++++++++------ kobo/apps/organizations/views.py | 20 +-- 4 files changed, 114 insertions(+), 56 deletions(-) diff --git a/kobo/apps/organizations/permissions.py b/kobo/apps/organizations/permissions.py index 7564a57eda..a29a71f37a 100644 --- a/kobo/apps/organizations/permissions.py +++ b/kobo/apps/organizations/permissions.py @@ -41,4 +41,33 @@ def has_object_permission(self, request, view, obj): return True # Instance must have an attribute named `is_admin` - return obj.is_admin(request.user) + return obj.organization.is_admin(request.user) + + +class IsOrgOwnerOrAdminOrMember(permissions.BasePermission): + def has_permission(self, request, view): + # Ensure the user is authenticated + if not request.user.is_authenticated: + return False + return True + + def has_object_permission(self, request, view, obj): + user_role = obj.organization.get_user_role(request.user) + + # Allow owners to view, update, and delete members + if user_role == 'owner': + return True + + # Allow admins to view and update, but not delete members + if user_role == 'admin': + return request.method in permissions.SAFE_METHODS or request.method == 'PATCH' + + # Allow members to only view other members + if user_role == 'member': + return request.method in permissions.SAFE_METHODS + + # Deny access to external users + if user_role == 'external': + raise Http404() + + return False diff --git a/kobo/apps/organizations/serializers.py b/kobo/apps/organizations/serializers.py index 714b4ced63..7232ba1669 100644 --- a/kobo/apps/organizations/serializers.py +++ b/kobo/apps/organizations/serializers.py @@ -1,4 +1,3 @@ -from constance import config from django.contrib.auth import get_user_model from django.utils.translation import gettext as t from rest_framework import serializers diff --git a/kobo/apps/organizations/tests/test_organization_members_api.py b/kobo/apps/organizations/tests/test_organization_members_api.py index d9cd447a2b..9766600f92 100644 --- a/kobo/apps/organizations/tests/test_organization_members_api.py +++ b/kobo/apps/organizations/tests/test_organization_members_api.py @@ -1,28 +1,25 @@ +from ddt import ddt, data, unpack from django.urls import reverse -from model_bakery import baker from rest_framework import status -from kobo.apps.kobo_auth.shortcuts import User -from kobo.apps.organizations.models import Organization, OrganizationUser -from kpi.tests.kpi_test_case import BaseTestCase +from kobo.apps.organizations.tests.test_organizations_api import ( + BaseOrganizationAssetApiTestCase +) from kpi.urls.router_api_v2 import URL_NAMESPACE - -class OrganizationMemberAPITestCase(BaseTestCase): +@ddt +class OrganizationMemberAPITestCase(BaseOrganizationAssetApiTestCase): fixtures = ['test_data'] URL_NAMESPACE = URL_NAMESPACE def setUp(self): - self.organization = baker.make( - Organization, id='org_12345', mmo_override=True - ) - self.owner_user = baker.make(User, username='owner') - self.member_user = baker.make(User, username='member') + super().setUp() + self.organization = self.someuser.organization + self.owner_user = self.someuser + self.member_user = self.alice + self.admin_user = self.anotheruser + self.external_user = self.bob - self.organization.add_user(self.owner_user) - self.organization.add_user(self.member_user, is_admin=False) - - self.client.force_login(self.owner_user) self.list_url = reverse( self._get_endpoint('organization-members-list'), kwargs={'organization_id': self.organization.id}, @@ -35,36 +32,77 @@ def setUp(self): }, ) - def test_list_members(self): + @data( + ('owner', status.HTTP_200_OK), + ('admin', status.HTTP_200_OK), + ('member', status.HTTP_200_OK), + ('external', status.HTTP_200_OK), + ) + @unpack + def test_list_members_with_different_roles(self, user_role, expected_status): + user = getattr(self, f"{user_role}_user") + self.client.force_login(user) response = self.client.get(self.list_url) - self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertIn( - 'owner', - [member['user__username'] for member in response.data.get('results')] - ) - self.assertIn( - 'member', - [member['user__username'] for member in response.data.get('results')] - ) + self.assertEqual(response.status_code, expected_status) + + @data( + ('owner', status.HTTP_200_OK), + ('admin', status.HTTP_200_OK), + ('member', status.HTTP_200_OK), + ('external', status.HTTP_404_NOT_FOUND), + ) + @unpack + def test_retrieve_member_details_with_different_roles(self, user_role, expected_status): + user = getattr(self, f"{user_role}_user") + self.client.force_login(user) + response = self.client.get(self.detail_url(self.member_user)) + self.assertEqual(response.status_code, expected_status) + + @data( + ('owner', status.HTTP_200_OK), + ('admin', status.HTTP_200_OK), + ('member', status.HTTP_403_FORBIDDEN), + ('external', status.HTTP_404_NOT_FOUND), + ) + @unpack + def test_update_member_role_with_different_roles(self, user_role, expected_status): + user = getattr(self, f"{user_role}_user") + data = {'role': 'admin'} + self.client.force_login(user) + response = self.client.patch(self.detail_url(self.member_user), data) + self.assertEqual(response.status_code, expected_status) - def test_retrieve_member_details(self): - response = self.client.get(self.detail_url('member')) - self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertEqual(response.data['user__username'], 'member') - self.assertEqual(response.data['role'], 'member') + @data( + ('owner', status.HTTP_204_NO_CONTENT), + ('admin', status.HTTP_403_FORBIDDEN), + ('member', status.HTTP_403_FORBIDDEN), + ('external', status.HTTP_404_NOT_FOUND), + ) + @unpack + def test_delete_member_with_different_roles(self, user_role, expected_status): + user = getattr(self, f"{user_role}_user") + self.client.force_login(user) + response = self.client.delete(self.detail_url(self.member_user)) + self.assertEqual(response.status_code, expected_status) + if expected_status == status.HTTP_204_NO_CONTENT: + # Confirm deletion + response = self.client.get(self.detail_url(self.member_user)) + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - def test_update_member_role(self): + @data( + ('owner', status.HTTP_405_METHOD_NOT_ALLOWED), + ('admin', status.HTTP_405_METHOD_NOT_ALLOWED), + ('member', status.HTTP_405_METHOD_NOT_ALLOWED), + ('external', status.HTTP_405_METHOD_NOT_ALLOWED), + ) + @unpack + def test_post_request_is_not_allowed(self, user_role, expected_status): + user = getattr(self, f"{user_role}_user") data = {'role': 'admin'} - response = self.client.patch(self.detail_url('member'), data) - self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertEqual(response.data['role'], 'admin') + self.client.force_login(user) + response = self.client.post(self.list_url, data) + self.assertEqual(response.status_code, expected_status) - def test_delete_member(self): - response = self.client.delete(self.detail_url('member')) - self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) - # Confirm deletion - response = self.client.get(self.detail_url('member')) - self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) def test_list_requires_authentication(self): self.client.logout() diff --git a/kobo/apps/organizations/views.py b/kobo/apps/organizations/views.py index 6230a16d7e..eff0fc2f73 100644 --- a/kobo/apps/organizations/views.py +++ b/kobo/apps/organizations/views.py @@ -30,7 +30,11 @@ from kpi.utils.object_permission import get_database_user from kpi.views.v2.asset import AssetViewSet from .models import Organization, OrganizationOwner, OrganizationUser -from .permissions import IsOrgAdmin, IsOrgAdminOrReadOnly +from .permissions import ( + IsOrgAdmin, + IsOrgAdminOrReadOnly, + IsOrgOwnerOrAdminOrMember +) from .serializers import OrganizationSerializer, OrganizationUserSerializer from ..accounts.mfa.models import MfaMethod from ..stripe.constants import ACTIVE_STRIPE_STATUSES @@ -399,7 +403,7 @@ class OrganizationMemberViewSet(viewsets.ModelViewSet): in updates. """ serializer_class = OrganizationUserSerializer - permission_classes = [IsAuthenticated] + permission_classes = [IsOrgOwnerOrAdminOrMember] pagination_class = OrganizationMemberPagination http_method_names = ['get', 'patch', 'delete'] lookup_field = 'user__username' @@ -432,15 +436,3 @@ def get_queryset(self): has_mfa_enabled=Exists(mfa_subquery) ) return queryset - - def partial_update(self, request, *args, **kwargs): - instance = self.get_object() - serializer = self.get_serializer( - instance, data=request.data, partial=True - ) - serializer.is_valid(raise_exception=True) - role = serializer.validated_data.get('role') - if role: - instance.is_admin = (role == 'admin') - instance.save() - return super().partial_update(request, *args, **kwargs) From d876fbced21f4e10e78f91dfb09e0fdf250ead73 Mon Sep 17 00:00:00 2001 From: rajpatel24 Date: Fri, 15 Nov 2024 01:48:24 +0530 Subject: [PATCH 04/13] Revert unintended change and fix linting issue --- kobo/apps/organizations/permissions.py | 7 +++++-- .../tests/test_organization_members_api.py | 16 +++++++++------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/kobo/apps/organizations/permissions.py b/kobo/apps/organizations/permissions.py index af7d2f7b56..dbb895ac0e 100644 --- a/kobo/apps/organizations/permissions.py +++ b/kobo/apps/organizations/permissions.py @@ -40,7 +40,7 @@ def has_object_permission(self, request, view, obj): return True # Instance must have an attribute named `is_admin` - return obj.organization.is_admin(request.user) + return obj.is_admin(request.user) class IsOrgOwnerOrAdminOrMember(permissions.BasePermission): @@ -59,7 +59,10 @@ def has_object_permission(self, request, view, obj): # Allow admins to view and update, but not delete members if user_role == 'admin': - return request.method in permissions.SAFE_METHODS or request.method == 'PATCH' + return ( + request.method in permissions.SAFE_METHODS or + request.method == 'PATCH' + ) # Allow members to only view other members if user_role == 'member': diff --git a/kobo/apps/organizations/tests/test_organization_members_api.py b/kobo/apps/organizations/tests/test_organization_members_api.py index 9766600f92..877d2dcb95 100644 --- a/kobo/apps/organizations/tests/test_organization_members_api.py +++ b/kobo/apps/organizations/tests/test_organization_members_api.py @@ -7,6 +7,7 @@ ) from kpi.urls.router_api_v2 import URL_NAMESPACE + @ddt class OrganizationMemberAPITestCase(BaseOrganizationAssetApiTestCase): fixtures = ['test_data'] @@ -40,7 +41,7 @@ def setUp(self): ) @unpack def test_list_members_with_different_roles(self, user_role, expected_status): - user = getattr(self, f"{user_role}_user") + user = getattr(self, f'{user_role}_user') self.client.force_login(user) response = self.client.get(self.list_url) self.assertEqual(response.status_code, expected_status) @@ -52,8 +53,10 @@ def test_list_members_with_different_roles(self, user_role, expected_status): ('external', status.HTTP_404_NOT_FOUND), ) @unpack - def test_retrieve_member_details_with_different_roles(self, user_role, expected_status): - user = getattr(self, f"{user_role}_user") + def test_retrieve_member_details_with_different_roles( + self, user_role, expected_status + ): + user = getattr(self, f'{user_role}_user') self.client.force_login(user) response = self.client.get(self.detail_url(self.member_user)) self.assertEqual(response.status_code, expected_status) @@ -66,7 +69,7 @@ def test_retrieve_member_details_with_different_roles(self, user_role, expected_ ) @unpack def test_update_member_role_with_different_roles(self, user_role, expected_status): - user = getattr(self, f"{user_role}_user") + user = getattr(self, f'{user_role}_user') data = {'role': 'admin'} self.client.force_login(user) response = self.client.patch(self.detail_url(self.member_user), data) @@ -80,7 +83,7 @@ def test_update_member_role_with_different_roles(self, user_role, expected_statu ) @unpack def test_delete_member_with_different_roles(self, user_role, expected_status): - user = getattr(self, f"{user_role}_user") + user = getattr(self, f'{user_role}_user') self.client.force_login(user) response = self.client.delete(self.detail_url(self.member_user)) self.assertEqual(response.status_code, expected_status) @@ -97,13 +100,12 @@ def test_delete_member_with_different_roles(self, user_role, expected_status): ) @unpack def test_post_request_is_not_allowed(self, user_role, expected_status): - user = getattr(self, f"{user_role}_user") + user = getattr(self, f'{user_role}_user') data = {'role': 'admin'} self.client.force_login(user) response = self.client.post(self.list_url, data) self.assertEqual(response.status_code, expected_status) - def test_list_requires_authentication(self): self.client.logout() response = self.client.get(self.list_url) From 953b7161d14fd73709a7651ca2969dd59f1f3a46 Mon Sep 17 00:00:00 2001 From: rajpatel24 Date: Sat, 16 Nov 2024 00:10:57 +0530 Subject: [PATCH 05/13] Refactor organization members API with updated permission logic and comprehensive tests --- kobo/apps/organizations/permissions.py | 56 +++---------------- kobo/apps/organizations/serializers.py | 10 ++-- .../tests/test_organization_members_api.py | 47 ++++++++++------ kobo/apps/organizations/views.py | 21 +++---- 4 files changed, 56 insertions(+), 78 deletions(-) diff --git a/kobo/apps/organizations/permissions.py b/kobo/apps/organizations/permissions.py index dbb895ac0e..c5aa6ebbd6 100644 --- a/kobo/apps/organizations/permissions.py +++ b/kobo/apps/organizations/permissions.py @@ -1,14 +1,17 @@ from django.http import Http404 from rest_framework import permissions +from rest_framework.permissions import IsAuthenticated from kobo.apps.organizations.constants import ORG_EXTERNAL_ROLE +from kobo.apps.organizations.models import Organization from kpi.mixins.validation_password_permission import ValidationPasswordPermissionMixin from kpi.utils.object_permission import get_database_user -class IsOrgAdmin(ValidationPasswordPermissionMixin, permissions.BasePermission): +class IsOrgAdminPermission(ValidationPasswordPermissionMixin, IsAuthenticated): """ - Object-level permission to only allow admin members of an object to access it. + Object-level permission to only allow admin (and owner) members of an object + to access it. Assumes the model instance has an `is_admin` attribute. """ @@ -26,50 +29,9 @@ def has_object_permission(self, request, view, obj): return obj.is_admin(user) -class IsOrgAdminOrReadOnly(IsOrgAdmin): - """ - Object-level permission to only allow admin members of an object to edit it. - Assumes the model instance has an `is_admin` attribute. - """ - - def has_object_permission(self, request, view, obj): - - # Read permissions are allowed to any request, - # so we'll always allow GET, HEAD or OPTIONS requests. - if request.method in permissions.SAFE_METHODS: - return True - - # Instance must have an attribute named `is_admin` - return obj.is_admin(request.user) - - -class IsOrgOwnerOrAdminOrMember(permissions.BasePermission): - def has_permission(self, request, view): - # Ensure the user is authenticated - if not request.user.is_authenticated: - return False - return True - +class HasOrgRolePermission(IsOrgAdminPermission): def has_object_permission(self, request, view, obj): - user_role = obj.organization.get_user_role(request.user) - - # Allow owners to view, update, and delete members - if user_role == 'owner': + obj = obj if isinstance(obj, Organization) else obj.organization + if super().has_object_permission(request, view, obj): return True - - # Allow admins to view and update, but not delete members - if user_role == 'admin': - return ( - request.method in permissions.SAFE_METHODS or - request.method == 'PATCH' - ) - - # Allow members to only view other members - if user_role == 'member': - return request.method in permissions.SAFE_METHODS - - # Deny access to external users - if user_role == 'external': - raise Http404() - - return False + return request.method in permissions.SAFE_METHODS diff --git a/kobo/apps/organizations/serializers.py b/kobo/apps/organizations/serializers.py index 7b7db8df07..df855b011a 100644 --- a/kobo/apps/organizations/serializers.py +++ b/kobo/apps/organizations/serializers.py @@ -47,11 +47,6 @@ class Meta: 'user__is_active' ] - def update(self, instance, validated_data): - if role := validated_data.get('role', None): - validated_data['is_admin'] = role == 'admin' - return super().update(instance, validated_data) - def get_url(self, obj): request = self.context.get('request') return reverse( @@ -63,6 +58,11 @@ def get_url(self, obj): request=request ) + def update(self, instance, validated_data): + if role := validated_data.get('role', None): + validated_data['is_admin'] = role == 'admin' + return super().update(instance, validated_data) + def validate_role(self, role): if role not in ['admin', 'member']: raise serializers.ValidationError( diff --git a/kobo/apps/organizations/tests/test_organization_members_api.py b/kobo/apps/organizations/tests/test_organization_members_api.py index 877d2dcb95..b885f17187 100644 --- a/kobo/apps/organizations/tests/test_organization_members_api.py +++ b/kobo/apps/organizations/tests/test_organization_members_api.py @@ -38,11 +38,15 @@ def setUp(self): ('admin', status.HTTP_200_OK), ('member', status.HTTP_200_OK), ('external', status.HTTP_200_OK), + ('anonymous', status.HTTP_401_UNAUTHORIZED), ) @unpack def test_list_members_with_different_roles(self, user_role, expected_status): - user = getattr(self, f'{user_role}_user') - self.client.force_login(user) + if user_role == 'anonymous': + self.client.logout() + else: + user = getattr(self, f'{user_role}_user') + self.client.force_login(user) response = self.client.get(self.list_url) self.assertEqual(response.status_code, expected_status) @@ -51,13 +55,17 @@ def test_list_members_with_different_roles(self, user_role, expected_status): ('admin', status.HTTP_200_OK), ('member', status.HTTP_200_OK), ('external', status.HTTP_404_NOT_FOUND), + ('anonymous', status.HTTP_401_UNAUTHORIZED), ) @unpack def test_retrieve_member_details_with_different_roles( self, user_role, expected_status ): - user = getattr(self, f'{user_role}_user') - self.client.force_login(user) + if user_role == 'anonymous': + self.client.logout() + else: + user = getattr(self, f'{user_role}_user') + self.client.force_login(user) response = self.client.get(self.detail_url(self.member_user)) self.assertEqual(response.status_code, expected_status) @@ -66,25 +74,33 @@ def test_retrieve_member_details_with_different_roles( ('admin', status.HTTP_200_OK), ('member', status.HTTP_403_FORBIDDEN), ('external', status.HTTP_404_NOT_FOUND), + ('anonymous', status.HTTP_401_UNAUTHORIZED), ) @unpack def test_update_member_role_with_different_roles(self, user_role, expected_status): - user = getattr(self, f'{user_role}_user') + if user_role == 'anonymous': + self.client.logout() + else: + user = getattr(self, f'{user_role}_user') + self.client.force_login(user) data = {'role': 'admin'} - self.client.force_login(user) response = self.client.patch(self.detail_url(self.member_user), data) self.assertEqual(response.status_code, expected_status) @data( ('owner', status.HTTP_204_NO_CONTENT), - ('admin', status.HTTP_403_FORBIDDEN), + ('admin', status.HTTP_204_NO_CONTENT), ('member', status.HTTP_403_FORBIDDEN), ('external', status.HTTP_404_NOT_FOUND), + ('anonymous', status.HTTP_401_UNAUTHORIZED), ) @unpack def test_delete_member_with_different_roles(self, user_role, expected_status): - user = getattr(self, f'{user_role}_user') - self.client.force_login(user) + if user_role == 'anonymous': + self.client.logout() + else: + user = getattr(self, f'{user_role}_user') + self.client.force_login(user) response = self.client.delete(self.detail_url(self.member_user)) self.assertEqual(response.status_code, expected_status) if expected_status == status.HTTP_204_NO_CONTENT: @@ -97,16 +113,15 @@ def test_delete_member_with_different_roles(self, user_role, expected_status): ('admin', status.HTTP_405_METHOD_NOT_ALLOWED), ('member', status.HTTP_405_METHOD_NOT_ALLOWED), ('external', status.HTTP_405_METHOD_NOT_ALLOWED), + ('anonymous', status.HTTP_401_UNAUTHORIZED), ) @unpack def test_post_request_is_not_allowed(self, user_role, expected_status): - user = getattr(self, f'{user_role}_user') + if user_role == 'anonymous': + self.client.logout() + else: + user = getattr(self, f'{user_role}_user') + self.client.force_login(user) data = {'role': 'admin'} - self.client.force_login(user) response = self.client.post(self.list_url, data) self.assertEqual(response.status_code, expected_status) - - def test_list_requires_authentication(self): - self.client.logout() - response = self.client.get(self.list_url) - self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) diff --git a/kobo/apps/organizations/views.py b/kobo/apps/organizations/views.py index 65a9dc3426..8fd6147158 100644 --- a/kobo/apps/organizations/views.py +++ b/kobo/apps/organizations/views.py @@ -22,7 +22,7 @@ from kpi.constants import ASSET_TYPE_SURVEY from kpi.filters import AssetOrderingFilter, SearchFilter from kpi.models.asset import Asset -from kpi.paginators import AssetUsagePagination, OrganizationMemberPagination +from kpi.paginators import AssetUsagePagination, OrganizationMembersPagination from kpi.permissions import IsAuthenticated from kpi.serializers.v2.service_usage import ( CustomAssetUsageSerializer, @@ -31,11 +31,7 @@ from kpi.utils.object_permission import get_database_user from kpi.views.v2.asset import AssetViewSet from .models import Organization, OrganizationOwner, OrganizationUser -from .permissions import ( - IsOrgAdmin, - IsOrgAdminOrReadOnly, - IsOrgOwnerOrAdminOrMember -) +from .permissions import HasOrgRolePermission from .serializers import OrganizationSerializer, OrganizationUserSerializer from ..accounts.mfa.models import MfaMethod from ..stripe.constants import ACTIVE_STRIPE_STATUSES @@ -91,10 +87,12 @@ class OrganizationViewSet(viewsets.ModelViewSet): queryset = Organization.objects.all() serializer_class = OrganizationSerializer lookup_field = 'id' - permission_classes = (IsAuthenticated, IsOrgAdminOrReadOnly) + permission_classes = [HasOrgRolePermission] pagination_class = AssetUsagePagination - @action(detail=True, methods=['GET'], permission_classes=[IsOrgAdmin]) + @action( + detail=True, methods=['GET'], permission_classes=[HasOrgRolePermission] + ) def assets(self, request: Request, *args, **kwargs): """ ### Retrieve Organization Assets @@ -279,6 +277,9 @@ def asset_usage(self, request, pk=None, *args, **kwargs): class OrganizationMemberViewSet(viewsets.ModelViewSet): """ + Used `ModelViewSet` instead of `NestedViewSetMixin` to maintain explicit + control over the queryset. + * Manage organization members and their roles within an organization. * Run a partial update on an organization member to promote or demote. @@ -408,8 +409,8 @@ class OrganizationMemberViewSet(viewsets.ModelViewSet): in updates. """ serializer_class = OrganizationUserSerializer - permission_classes = [IsOrgOwnerOrAdminOrMember] - pagination_class = OrganizationMemberPagination + permission_classes = [HasOrgRolePermission] + pagination_class = OrganizationMembersPagination http_method_names = ['get', 'patch', 'delete'] lookup_field = 'user__username' From f8cb95beb18301974fcfe68671644638436ac371 Mon Sep 17 00:00:00 2001 From: rajpatel24 Date: Sat, 16 Nov 2024 00:10:57 +0530 Subject: [PATCH 06/13] Refactor organization members API with updated permission logic and comprehensive tests --- kobo/apps/organizations/permissions.py | 56 +++---------------- kobo/apps/organizations/serializers.py | 10 ++-- .../tests/test_organization_members_api.py | 47 ++++++++++------ kobo/apps/organizations/views.py | 22 ++++---- kpi/paginators.py | 12 ++-- 5 files changed, 62 insertions(+), 85 deletions(-) diff --git a/kobo/apps/organizations/permissions.py b/kobo/apps/organizations/permissions.py index dbb895ac0e..c5aa6ebbd6 100644 --- a/kobo/apps/organizations/permissions.py +++ b/kobo/apps/organizations/permissions.py @@ -1,14 +1,17 @@ from django.http import Http404 from rest_framework import permissions +from rest_framework.permissions import IsAuthenticated from kobo.apps.organizations.constants import ORG_EXTERNAL_ROLE +from kobo.apps.organizations.models import Organization from kpi.mixins.validation_password_permission import ValidationPasswordPermissionMixin from kpi.utils.object_permission import get_database_user -class IsOrgAdmin(ValidationPasswordPermissionMixin, permissions.BasePermission): +class IsOrgAdminPermission(ValidationPasswordPermissionMixin, IsAuthenticated): """ - Object-level permission to only allow admin members of an object to access it. + Object-level permission to only allow admin (and owner) members of an object + to access it. Assumes the model instance has an `is_admin` attribute. """ @@ -26,50 +29,9 @@ def has_object_permission(self, request, view, obj): return obj.is_admin(user) -class IsOrgAdminOrReadOnly(IsOrgAdmin): - """ - Object-level permission to only allow admin members of an object to edit it. - Assumes the model instance has an `is_admin` attribute. - """ - - def has_object_permission(self, request, view, obj): - - # Read permissions are allowed to any request, - # so we'll always allow GET, HEAD or OPTIONS requests. - if request.method in permissions.SAFE_METHODS: - return True - - # Instance must have an attribute named `is_admin` - return obj.is_admin(request.user) - - -class IsOrgOwnerOrAdminOrMember(permissions.BasePermission): - def has_permission(self, request, view): - # Ensure the user is authenticated - if not request.user.is_authenticated: - return False - return True - +class HasOrgRolePermission(IsOrgAdminPermission): def has_object_permission(self, request, view, obj): - user_role = obj.organization.get_user_role(request.user) - - # Allow owners to view, update, and delete members - if user_role == 'owner': + obj = obj if isinstance(obj, Organization) else obj.organization + if super().has_object_permission(request, view, obj): return True - - # Allow admins to view and update, but not delete members - if user_role == 'admin': - return ( - request.method in permissions.SAFE_METHODS or - request.method == 'PATCH' - ) - - # Allow members to only view other members - if user_role == 'member': - return request.method in permissions.SAFE_METHODS - - # Deny access to external users - if user_role == 'external': - raise Http404() - - return False + return request.method in permissions.SAFE_METHODS diff --git a/kobo/apps/organizations/serializers.py b/kobo/apps/organizations/serializers.py index 7b7db8df07..df855b011a 100644 --- a/kobo/apps/organizations/serializers.py +++ b/kobo/apps/organizations/serializers.py @@ -47,11 +47,6 @@ class Meta: 'user__is_active' ] - def update(self, instance, validated_data): - if role := validated_data.get('role', None): - validated_data['is_admin'] = role == 'admin' - return super().update(instance, validated_data) - def get_url(self, obj): request = self.context.get('request') return reverse( @@ -63,6 +58,11 @@ def get_url(self, obj): request=request ) + def update(self, instance, validated_data): + if role := validated_data.get('role', None): + validated_data['is_admin'] = role == 'admin' + return super().update(instance, validated_data) + def validate_role(self, role): if role not in ['admin', 'member']: raise serializers.ValidationError( diff --git a/kobo/apps/organizations/tests/test_organization_members_api.py b/kobo/apps/organizations/tests/test_organization_members_api.py index 877d2dcb95..b885f17187 100644 --- a/kobo/apps/organizations/tests/test_organization_members_api.py +++ b/kobo/apps/organizations/tests/test_organization_members_api.py @@ -38,11 +38,15 @@ def setUp(self): ('admin', status.HTTP_200_OK), ('member', status.HTTP_200_OK), ('external', status.HTTP_200_OK), + ('anonymous', status.HTTP_401_UNAUTHORIZED), ) @unpack def test_list_members_with_different_roles(self, user_role, expected_status): - user = getattr(self, f'{user_role}_user') - self.client.force_login(user) + if user_role == 'anonymous': + self.client.logout() + else: + user = getattr(self, f'{user_role}_user') + self.client.force_login(user) response = self.client.get(self.list_url) self.assertEqual(response.status_code, expected_status) @@ -51,13 +55,17 @@ def test_list_members_with_different_roles(self, user_role, expected_status): ('admin', status.HTTP_200_OK), ('member', status.HTTP_200_OK), ('external', status.HTTP_404_NOT_FOUND), + ('anonymous', status.HTTP_401_UNAUTHORIZED), ) @unpack def test_retrieve_member_details_with_different_roles( self, user_role, expected_status ): - user = getattr(self, f'{user_role}_user') - self.client.force_login(user) + if user_role == 'anonymous': + self.client.logout() + else: + user = getattr(self, f'{user_role}_user') + self.client.force_login(user) response = self.client.get(self.detail_url(self.member_user)) self.assertEqual(response.status_code, expected_status) @@ -66,25 +74,33 @@ def test_retrieve_member_details_with_different_roles( ('admin', status.HTTP_200_OK), ('member', status.HTTP_403_FORBIDDEN), ('external', status.HTTP_404_NOT_FOUND), + ('anonymous', status.HTTP_401_UNAUTHORIZED), ) @unpack def test_update_member_role_with_different_roles(self, user_role, expected_status): - user = getattr(self, f'{user_role}_user') + if user_role == 'anonymous': + self.client.logout() + else: + user = getattr(self, f'{user_role}_user') + self.client.force_login(user) data = {'role': 'admin'} - self.client.force_login(user) response = self.client.patch(self.detail_url(self.member_user), data) self.assertEqual(response.status_code, expected_status) @data( ('owner', status.HTTP_204_NO_CONTENT), - ('admin', status.HTTP_403_FORBIDDEN), + ('admin', status.HTTP_204_NO_CONTENT), ('member', status.HTTP_403_FORBIDDEN), ('external', status.HTTP_404_NOT_FOUND), + ('anonymous', status.HTTP_401_UNAUTHORIZED), ) @unpack def test_delete_member_with_different_roles(self, user_role, expected_status): - user = getattr(self, f'{user_role}_user') - self.client.force_login(user) + if user_role == 'anonymous': + self.client.logout() + else: + user = getattr(self, f'{user_role}_user') + self.client.force_login(user) response = self.client.delete(self.detail_url(self.member_user)) self.assertEqual(response.status_code, expected_status) if expected_status == status.HTTP_204_NO_CONTENT: @@ -97,16 +113,15 @@ def test_delete_member_with_different_roles(self, user_role, expected_status): ('admin', status.HTTP_405_METHOD_NOT_ALLOWED), ('member', status.HTTP_405_METHOD_NOT_ALLOWED), ('external', status.HTTP_405_METHOD_NOT_ALLOWED), + ('anonymous', status.HTTP_401_UNAUTHORIZED), ) @unpack def test_post_request_is_not_allowed(self, user_role, expected_status): - user = getattr(self, f'{user_role}_user') + if user_role == 'anonymous': + self.client.logout() + else: + user = getattr(self, f'{user_role}_user') + self.client.force_login(user) data = {'role': 'admin'} - self.client.force_login(user) response = self.client.post(self.list_url, data) self.assertEqual(response.status_code, expected_status) - - def test_list_requires_authentication(self): - self.client.logout() - response = self.client.get(self.list_url) - self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) diff --git a/kobo/apps/organizations/views.py b/kobo/apps/organizations/views.py index 65a9dc3426..c601086a69 100644 --- a/kobo/apps/organizations/views.py +++ b/kobo/apps/organizations/views.py @@ -22,8 +22,7 @@ from kpi.constants import ASSET_TYPE_SURVEY from kpi.filters import AssetOrderingFilter, SearchFilter from kpi.models.asset import Asset -from kpi.paginators import AssetUsagePagination, OrganizationMemberPagination -from kpi.permissions import IsAuthenticated +from kpi.paginators import AssetUsagePagination, OrganizationMembersPagination from kpi.serializers.v2.service_usage import ( CustomAssetUsageSerializer, ServiceUsageSerializer, @@ -31,11 +30,7 @@ from kpi.utils.object_permission import get_database_user from kpi.views.v2.asset import AssetViewSet from .models import Organization, OrganizationOwner, OrganizationUser -from .permissions import ( - IsOrgAdmin, - IsOrgAdminOrReadOnly, - IsOrgOwnerOrAdminOrMember -) +from .permissions import HasOrgRolePermission from .serializers import OrganizationSerializer, OrganizationUserSerializer from ..accounts.mfa.models import MfaMethod from ..stripe.constants import ACTIVE_STRIPE_STATUSES @@ -91,10 +86,12 @@ class OrganizationViewSet(viewsets.ModelViewSet): queryset = Organization.objects.all() serializer_class = OrganizationSerializer lookup_field = 'id' - permission_classes = (IsAuthenticated, IsOrgAdminOrReadOnly) + permission_classes = [HasOrgRolePermission] pagination_class = AssetUsagePagination - @action(detail=True, methods=['GET'], permission_classes=[IsOrgAdmin]) + @action( + detail=True, methods=['GET'], permission_classes=[HasOrgRolePermission] + ) def assets(self, request: Request, *args, **kwargs): """ ### Retrieve Organization Assets @@ -279,6 +276,9 @@ def asset_usage(self, request, pk=None, *args, **kwargs): class OrganizationMemberViewSet(viewsets.ModelViewSet): """ + Used `ModelViewSet` instead of `NestedViewSetMixin` to maintain explicit + control over the queryset. + * Manage organization members and their roles within an organization. * Run a partial update on an organization member to promote or demote. @@ -408,8 +408,8 @@ class OrganizationMemberViewSet(viewsets.ModelViewSet): in updates. """ serializer_class = OrganizationUserSerializer - permission_classes = [IsOrgOwnerOrAdminOrMember] - pagination_class = OrganizationMemberPagination + permission_classes = [HasOrgRolePermission] + pagination_class = OrganizationMembersPagination http_method_names = ['get', 'patch', 'delete'] lookup_field = 'user__username' diff --git a/kpi/paginators.py b/kpi/paginators.py index 34a7fe9ee5..7aec997833 100644 --- a/kpi/paginators.py +++ b/kpi/paginators.py @@ -137,15 +137,15 @@ def get_count(self, queryset): return super().get_count(queryset) -class TinyPaginated(PageNumberPagination): +class OrganizationMembersPagination(PageNumberPagination): """ - Same as Paginated with a small page size + Pagination class for Organization Members """ - page_size = 50 + page_size_query_param = 'page_size' -class OrganizationMemberPagination(PageNumberPagination): +class TinyPaginated(PageNumberPagination): """ - Pagination class for Organization + Same as Paginated with a small page size """ - page_size = 10 + page_size = 50 From 906be3da208599811d25761624594acbf7ac98ee Mon Sep 17 00:00:00 2001 From: rajpatel24 Date: Sat, 16 Nov 2024 01:01:10 +0530 Subject: [PATCH 07/13] Fix failing tests --- kobo/apps/organizations/tests/test_organizations_api.py | 2 +- kobo/apps/organizations/views.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/kobo/apps/organizations/tests/test_organizations_api.py b/kobo/apps/organizations/tests/test_organizations_api.py index c97ebef66e..b1e467d5e9 100644 --- a/kobo/apps/organizations/tests/test_organizations_api.py +++ b/kobo/apps/organizations/tests/test_organizations_api.py @@ -349,7 +349,7 @@ def test_can_list(self, username, expected_status_code): def test_list_not_found_as_anonymous(self): self.client.logout() response = self.client.get(self.org_assets_list_url) - assert response.status_code == status.HTTP_404_NOT_FOUND + assert response.status_code == status.HTTP_401_UNAUTHORIZED def test_list_only_organization_assets(self): # The organization's assets endpoint only returns assets where the `owner` diff --git a/kobo/apps/organizations/views.py b/kobo/apps/organizations/views.py index c601086a69..364bf71d56 100644 --- a/kobo/apps/organizations/views.py +++ b/kobo/apps/organizations/views.py @@ -30,7 +30,7 @@ from kpi.utils.object_permission import get_database_user from kpi.views.v2.asset import AssetViewSet from .models import Organization, OrganizationOwner, OrganizationUser -from .permissions import HasOrgRolePermission +from .permissions import HasOrgRolePermission, IsOrgAdminPermission from .serializers import OrganizationSerializer, OrganizationUserSerializer from ..accounts.mfa.models import MfaMethod from ..stripe.constants import ACTIVE_STRIPE_STATUSES @@ -90,7 +90,7 @@ class OrganizationViewSet(viewsets.ModelViewSet): pagination_class = AssetUsagePagination @action( - detail=True, methods=['GET'], permission_classes=[HasOrgRolePermission] + detail=True, methods=['GET'], permission_classes=[IsOrgAdminPermission] ) def assets(self, request: Request, *args, **kwargs): """ From 01c189c49d47c16919e697e018446df5d901c3e3 Mon Sep 17 00:00:00 2001 From: rajpatel24 Date: Tue, 19 Nov 2024 17:09:35 +0530 Subject: [PATCH 08/13] Update delete logic to remove user from user table along with organization user table and enhance test coverage --- .../tests/test_organization_members_api.py | 4 + kobo/apps/organizations/views.py | 75 ++++++++++++------- 2 files changed, 53 insertions(+), 26 deletions(-) diff --git a/kobo/apps/organizations/tests/test_organization_members_api.py b/kobo/apps/organizations/tests/test_organization_members_api.py index b885f17187..a86c494341 100644 --- a/kobo/apps/organizations/tests/test_organization_members_api.py +++ b/kobo/apps/organizations/tests/test_organization_members_api.py @@ -2,6 +2,7 @@ from django.urls import reverse from rest_framework import status +from kobo.apps.kobo_auth.shortcuts import User from kobo.apps.organizations.tests.test_organizations_api import ( BaseOrganizationAssetApiTestCase ) @@ -107,6 +108,9 @@ def test_delete_member_with_different_roles(self, user_role, expected_status): # Confirm deletion response = self.client.get(self.detail_url(self.member_user)) self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + self.assertFalse( + User.objects.filter(username=f'{user_role}_user').exists() + ) @data( ('owner', status.HTTP_405_METHOD_NOT_ALLOWED), diff --git a/kobo/apps/organizations/views.py b/kobo/apps/organizations/views.py index 364bf71d56..343682f39a 100644 --- a/kobo/apps/organizations/views.py +++ b/kobo/apps/organizations/views.py @@ -276,18 +276,16 @@ def asset_usage(self, request, pk=None, *args, **kwargs): class OrganizationMemberViewSet(viewsets.ModelViewSet): """ - Used `ModelViewSet` instead of `NestedViewSetMixin` to maintain explicit - control over the queryset. - - * Manage organization members and their roles within an organization. - * Run a partial update on an organization member to promote or demote. + The API uses `ModelViewSet` instead of `NestedViewSetMixin` to maintain + explicit control over the queryset. ## Organization Members API - This API allows authorized users to view and manage the members of an - organization, including their roles. It handles existing members. It also - allows updating roles, such as promoting a member to an admin or assigning - a new owner. + This API allows authorized users to view and manage organization members and + their roles, including promoting or demoting members (eg. to admin). + + * Manage members and their roles within an organization. + * Update member roles (promote/demote). ### List Members @@ -316,9 +314,9 @@ class OrganizationMemberViewSet(viewsets.ModelViewSet): > "user__email": "foo_bar@example.com", > "user__name": "Foo Bar", > "role": "owner", - > "has_mfa_enabled": true, + > "user__has_mfa_enabled": true, > "date_joined": "2024-08-11T12:36:32Z", - > "is_active": true + > "user__is_active": true > }, > { > "url": "http://[kpi]/api/v2/organizations/org_12345/ \ @@ -328,15 +326,13 @@ class OrganizationMemberViewSet(viewsets.ModelViewSet): > "user__email": "john_doe@example.com", > "user__name": "John Doe", > "role": "admin", - > "has_mfa_enabled": false, + > "user__has_mfa_enabled": false, > "date_joined": "2024-10-21T06:38:45Z", - > "is_active": true + > "user__is_active": true > } > ] > } - The response includes detailed information about each member, such as their - username, email, role (owner, admin, member), and account status. ### Retrieve Member Details @@ -359,36 +355,51 @@ class OrganizationMemberViewSet(viewsets.ModelViewSet): > "user__email": "foo_bar@example.com", > "user__name": "Foo Bar", > "role": "owner", - > "has_mfa_enabled": true, + > "user__has_mfa_enabled": true, > "date_joined": "2024-08-11T12:36:32Z", - > "is_active": true + > "user__is_active": true > } ### Update Member Role - Updates the role of a member within the organization to `owner`, `admin`, or + Updates the role of a member within the organization to `admin` or `member`. + - **admin**: Grants the member admin privileges within the organization + - **member**: Revokes admin privileges, setting the member as a regular user + 
     PATCH /api/v2/organizations/{organization_id}/members/{username}/
- #### Payload + > Example + > + > curl -X PATCH https://[kpi]/api/v2/organizations/org_12345/members/foo_bar/ + + > Payload + > { > "role": "admin" > } - - **admin**: Grants the member admin privileges within the organization - - **member**: Revokes admin privileges, setting the member as a regular user + > Response 200 + + > { + > "url": "http://[kpi]/api/v2/organizations/org_12345/members/foo_bar/", + > "user": "http://[kpi]/api/v2/users/foo_bar/", + > "user__username": "foo_bar", + > "user__email": "foo_bar@example.com", + > "user__name": "Foo Bar", + > "role": "admin", + > "user__has_mfa_enabled": true, + > "date_joined": "2024-08-11T12:36:32Z", + > "user__is_active": true + > } - > Example - > - > curl -X PATCH https://[kpi]/api/v2/organizations/org_12345/ \ - > members/demo_user/ -d '{"role": "admin"}' ### Remove Member - Removes a member from the organization. + Delete an organization member and their associated user account. 
     DELETE /api/v2/organizations/{organization_id}/members/{username}/
@@ -401,6 +412,8 @@ class OrganizationMemberViewSet(viewsets.ModelViewSet):
     ## Permissions
 
     - The user must be authenticated to perform these actions.
+    - Owners and admins can manage members and roles.
+    - Members can view the list but cannot update roles or delete members.
 
     ## Notes
 
@@ -441,3 +454,13 @@ def get_queryset(self):
             has_mfa_enabled=Exists(mfa_subquery)
         )
         return queryset
+
+    def destroy(self, request, *args, **kwargs):
+        """
+        Delete an organization member and their associated user account
+        """
+        instance = self.get_object()
+        user = instance.user
+        instance.delete()
+        user.delete()
+        return Response(status=status.HTTP_204_NO_CONTENT)

From def1a7ae4ee4c1685b8ed6e42f9753df5ddd801b Mon Sep 17 00:00:00 2001
From: rajpatel24 
Date: Thu, 21 Nov 2024 00:44:01 +0530
Subject: [PATCH 09/13] Refactor permissions to block external users from
 listing organization members

---
 kobo/apps/organizations/permissions.py              | 13 +++++++++++++
 .../tests/test_organization_members_api.py          |  6 +++---
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/kobo/apps/organizations/permissions.py b/kobo/apps/organizations/permissions.py
index c5aa6ebbd6..f37dc36757 100644
--- a/kobo/apps/organizations/permissions.py
+++ b/kobo/apps/organizations/permissions.py
@@ -30,6 +30,19 @@ def has_object_permission(self, request, view, obj):
 
 
 class HasOrgRolePermission(IsOrgAdminPermission):
+    def has_permission(self, request, view):
+        if not super().has_permission(request, view):
+            return False
+
+        organization = Organization.objects.filter(
+            id=view.kwargs.get('organization_id')
+        ).first()
+        if organization and not self.has_object_permission(
+            request, view, organization
+        ):
+            return False
+        return True
+
     def has_object_permission(self, request, view, obj):
         obj = obj if isinstance(obj, Organization) else obj.organization
         if super().has_object_permission(request, view, obj):
diff --git a/kobo/apps/organizations/tests/test_organization_members_api.py b/kobo/apps/organizations/tests/test_organization_members_api.py
index a86c494341..eb9e74b33b 100644
--- a/kobo/apps/organizations/tests/test_organization_members_api.py
+++ b/kobo/apps/organizations/tests/test_organization_members_api.py
@@ -38,7 +38,7 @@ def setUp(self):
         ('owner', status.HTTP_200_OK),
         ('admin', status.HTTP_200_OK),
         ('member', status.HTTP_200_OK),
-        ('external', status.HTTP_200_OK),
+        ('external', status.HTTP_404_NOT_FOUND),
         ('anonymous', status.HTTP_401_UNAUTHORIZED),
     )
     @unpack
@@ -115,8 +115,8 @@ def test_delete_member_with_different_roles(self, user_role, expected_status):
     @data(
         ('owner', status.HTTP_405_METHOD_NOT_ALLOWED),
         ('admin', status.HTTP_405_METHOD_NOT_ALLOWED),
-        ('member', status.HTTP_405_METHOD_NOT_ALLOWED),
-        ('external', status.HTTP_405_METHOD_NOT_ALLOWED),
+        ('member', status.HTTP_403_FORBIDDEN),
+        ('external', status.HTTP_404_NOT_FOUND),
         ('anonymous', status.HTTP_401_UNAUTHORIZED),
     )
     @unpack

From d706056555d31eecfda1756c43100876cbe75b7b Mon Sep 17 00:00:00 2001
From: rajpatel24 
Date: Thu, 21 Nov 2024 13:07:08 +0530
Subject: [PATCH 10/13] Refactor serializer to retrieve user name from extra
 details and enable pagination with LimitOffsetPagination

---
 kobo/apps/organizations/serializers.py |  6 ++++--
 kobo/apps/organizations/views.py       | 13 -------------
 kpi/paginators.py                      |  7 -------
 3 files changed, 4 insertions(+), 22 deletions(-)

diff --git a/kobo/apps/organizations/serializers.py b/kobo/apps/organizations/serializers.py
index df855b011a..ae13041394 100644
--- a/kobo/apps/organizations/serializers.py
+++ b/kobo/apps/organizations/serializers.py
@@ -29,7 +29,9 @@ class OrganizationUserSerializer(serializers.ModelSerializer):
         source='user.date_joined', format='%Y-%m-%dT%H:%M:%SZ'
     )
     user__username = serializers.ReadOnlyField(source='user.username')
-    user__name = serializers.ReadOnlyField(source='user.get_full_name')
+    user__extra_details__name = serializers.ReadOnlyField(
+        source='user.extra_details.data.name'
+    )
     user__email = serializers.ReadOnlyField(source='user.email')
     user__is_active = serializers.ReadOnlyField(source='user.is_active')
 
@@ -40,7 +42,7 @@ class Meta:
             'user',
             'user__username',
             'user__email',
-            'user__name',
+            'user__extra_details__name',
             'role',
             'user__has_mfa_enabled',
             'date_joined',
diff --git a/kobo/apps/organizations/views.py b/kobo/apps/organizations/views.py
index 343682f39a..35d6afa81e 100644
--- a/kobo/apps/organizations/views.py
+++ b/kobo/apps/organizations/views.py
@@ -22,7 +22,6 @@
 from kpi.constants import ASSET_TYPE_SURVEY
 from kpi.filters import AssetOrderingFilter, SearchFilter
 from kpi.models.asset import Asset
-from kpi.paginators import AssetUsagePagination, OrganizationMembersPagination
 from kpi.serializers.v2.service_usage import (
     CustomAssetUsageSerializer,
     ServiceUsageSerializer,
@@ -87,7 +86,6 @@ class OrganizationViewSet(viewsets.ModelViewSet):
     serializer_class = OrganizationSerializer
     lookup_field = 'id'
     permission_classes = [HasOrgRolePermission]
-    pagination_class = AssetUsagePagination
 
     @action(
         detail=True, methods=['GET'], permission_classes=[IsOrgAdminPermission]
@@ -422,7 +420,6 @@ class OrganizationMemberViewSet(viewsets.ModelViewSet):
     """
     serializer_class = OrganizationUserSerializer
     permission_classes = [HasOrgRolePermission]
-    pagination_class = OrganizationMembersPagination
     http_method_names = ['get', 'patch', 'delete']
     lookup_field = 'user__username'
 
@@ -454,13 +451,3 @@ def get_queryset(self):
             has_mfa_enabled=Exists(mfa_subquery)
         )
         return queryset
-
-    def destroy(self, request, *args, **kwargs):
-        """
-        Delete an organization member and their associated user account
-        """
-        instance = self.get_object()
-        user = instance.user
-        instance.delete()
-        user.delete()
-        return Response(status=status.HTTP_204_NO_CONTENT)
diff --git a/kpi/paginators.py b/kpi/paginators.py
index 7aec997833..29823ea1a4 100644
--- a/kpi/paginators.py
+++ b/kpi/paginators.py
@@ -137,13 +137,6 @@ def get_count(self, queryset):
         return super().get_count(queryset)
 
 
-class OrganizationMembersPagination(PageNumberPagination):
-    """
-    Pagination class for Organization Members
-    """
-    page_size_query_param = 'page_size'
-
-
 class TinyPaginated(PageNumberPagination):
     """
     Same as Paginated with a small page size

From 0e2840873aa599334d8d6fd49dcfcd46435a9aa6 Mon Sep 17 00:00:00 2001
From: Raj Patel <51355159+rajpatel24@users.noreply.github.com>
Date: Thu, 21 Nov 2024 15:00:28 +0530
Subject: [PATCH 11/13] feat(organizations): update organizations endpoint to
 block post and delete requests TASK-964 (#5274)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

### 📣 Summary
Restricted POST and DELETE methods for `organizations` endpoint to
prevent unintended changes.


### 📖 Description
Previously, the organizations endpoint allowed POST and DELETE requests,
which could lead to accidental changes or deletions. This update
restricts these methods, ensuring that organizations data remains intact
and secure.


### 👀 Preview steps
- Send a GET request to the organizations endpoint to retrieve a list of
organizations.
- Attempt to send a POST request to create a new organization (this
should fail).
- Attempt to send a DELETE request to delete an existing organization
(this should fail).
- Send a PATCH request to update an existing organization (this should
succeed).


### 💭 Notes
- This change is a security enhancement to prevent accidental or
malicious modifications to organizations data.
- Tests have been updated to reflect the new behaviour.
---
 kobo/apps/organizations/tests/test_organizations_api.py | 7 ++++++-
 kobo/apps/organizations/views.py                        | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/kobo/apps/organizations/tests/test_organizations_api.py b/kobo/apps/organizations/tests/test_organizations_api.py
index b1e467d5e9..a3fa635543 100644
--- a/kobo/apps/organizations/tests/test_organizations_api.py
+++ b/kobo/apps/organizations/tests/test_organizations_api.py
@@ -61,7 +61,12 @@ def test_anonymous_user(self):
     def test_create(self):
         data = {'name': 'my org'}
         res = self.client.post(self.url_list, data)
-        self.assertContains(res, data['name'], status_code=201)
+        self.assertEqual(res.status_code, status.HTTP_405_METHOD_NOT_ALLOWED)
+
+    def test_delete(self):
+        self._insert_data()
+        res = self.client.delete(self.url_detail)
+        self.assertEqual(res.status_code, status.HTTP_405_METHOD_NOT_ALLOWED)
 
     def test_list(self):
         self._insert_data()
diff --git a/kobo/apps/organizations/views.py b/kobo/apps/organizations/views.py
index 35d6afa81e..704eeec0c6 100644
--- a/kobo/apps/organizations/views.py
+++ b/kobo/apps/organizations/views.py
@@ -86,6 +86,7 @@ class OrganizationViewSet(viewsets.ModelViewSet):
     serializer_class = OrganizationSerializer
     lookup_field = 'id'
     permission_classes = [HasOrgRolePermission]
+    http_method_names = ['get', 'patch']
 
     @action(
         detail=True, methods=['GET'], permission_classes=[IsOrgAdminPermission]

From 4d67c505ed36318a12686b9e339ac82c7d4ef3e9 Mon Sep 17 00:00:00 2001
From: rajpatel24 
Date: Thu, 21 Nov 2024 21:13:52 +0530
Subject: [PATCH 12/13] Optimize organization members API by prefetching
 related user details to improve performance

---
 kobo/apps/organizations/views.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kobo/apps/organizations/views.py b/kobo/apps/organizations/views.py
index 704eeec0c6..7cb1310c9f 100644
--- a/kobo/apps/organizations/views.py
+++ b/kobo/apps/organizations/views.py
@@ -442,7 +442,7 @@ def get_queryset(self):
         # Annotate with role based on organization ownership and admin status
         queryset = OrganizationUser.objects.filter(
             organization_id=organization_id
-        ).annotate(
+        ).select_related('user__extra_details').annotate(
             role=Case(
                 When(Exists(owner_subquery), then=Value('owner')),
                 When(is_admin=True, then=Value('admin')),

From 5c0ef26fcb4f0e271e49ee17a5c9416af1e23e22 Mon Sep 17 00:00:00 2001
From: rajpatel24 
Date: Fri, 22 Nov 2024 11:33:06 +0530
Subject: [PATCH 13/13] Update organization members API doc

---
 kobo/apps/organizations/views.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kobo/apps/organizations/views.py b/kobo/apps/organizations/views.py
index 7cb1310c9f..7fd0e59470 100644
--- a/kobo/apps/organizations/views.py
+++ b/kobo/apps/organizations/views.py
@@ -398,7 +398,7 @@ class OrganizationMemberViewSet(viewsets.ModelViewSet):
 
     ### Remove Member
 
-    Delete an organization member and their associated user account.
+    Delete an organization member.
 
     
     DELETE /api/v2/organizations/{organization_id}/members/{username}/