Fix leaving the organization (#6422)

Users should have the option to leave the organization.

CHANGELOG.md

@@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 - Downloading additional data from cloud storage if use_cache=true and job_file_mapping are specified
   (<https://github.com/opencv/cvat/pull/6879>)
+- Leaving an organization (<https://github.com/opencv/cvat/pull/6422>)
 
 ### Security
 - TDB

cvat-core/src/organization.ts

@@ -332,7 +332,7 @@ Object.defineProperties(Organization.prototype.leave, {
                 const result = await serverProxy.organizations.members(this.slug, 1, 10, {
                     filter: JSON.stringify({
                         and: [{
-                            '==': [{ var: 'user' }, user.id],
+                            '==': [{ var: 'user' }, user.username],
                         }],
                     }),
                 });

cvat-ui/package.json

@@ -1,6 +1,6 @@
 {
   "name": "cvat-ui",
-  "version": "1.56.1",
+  "version": "1.56.2",
   "description": "CVAT single-page application",
   "main": "src/index.tsx",
   "scripts": {

cvat-ui/src/actions/organization-actions.ts

@@ -212,14 +212,17 @@ export function inviteOrganizationMembersAsync(
     };
 }
 
-export function leaveOrganizationAsync(organization: any): ThunkAction {
+export function leaveOrganizationAsync(
+    organization: any,
+    onLeaveSuccess?: () => void,
+): ThunkAction {
     return async function (dispatch, getState) {
         const { user } = getState().auth;
         dispatch(organizationActions.leaveOrganization());
         try {
             await organization.leave(user);
             dispatch(organizationActions.leaveOrganizationSuccess());
-            localStorage.removeItem('currentOrganization');
+            if (onLeaveSuccess) onLeaveSuccess();
         } catch (error) {
             dispatch(organizationActions.leaveOrganizationFailed(error));
         }

cvat-ui/src/components/organization-page/member-item.tsx

@@ -3,12 +3,14 @@
 // SPDX-License-Identifier: MIT
 
 import React from 'react';
+import { useSelector } from 'react-redux';
 import Select from 'antd/lib/select';
 import Text from 'antd/lib/typography/Text';
 import { Row, Col } from 'antd/lib/grid';
 import moment from 'moment';
 import { DeleteOutlined } from '@ant-design/icons';
 import Modal from 'antd/lib/modal';
+import { CombinedState } from 'reducers';
 
 export interface Props {
     membershipInstance: any;
@@ -24,6 +26,7 @@ function MemberItem(props: Props): JSX.Element {
         user, joined_date: joinedDate, role, invitation,
     } = membershipInstance;
     const { username, firstName, lastName } = user;
+    const { username: selfUserName } = useSelector((state: CombinedState) => state.auth.user);
 
     return (
         <Row className='cvat-organization-member-item' justify='space-between'>
@@ -61,7 +64,7 @@ function MemberItem(props: Props): JSX.Element {
                 </Select>
             </Col>
             <Col span={1} className='cvat-organization-member-item-remove'>
-                {role !== 'owner' ? (
+                {(role === 'owner' || selfUserName === username) ? null : (
                     <DeleteOutlined
                         onClick={() => {
                             Modal.confirm({
@@ -78,7 +81,7 @@ function MemberItem(props: Props): JSX.Element {
                             });
                         }}
                     />
-                ) : null}
+                )}
             </Col>
         </Row>
     );

cvat-ui/src/components/organization-page/top-bar.tsx

@@ -281,7 +281,10 @@ function OrganizationTopBar(props: Props): JSX.Element {
                                 onClick={() => {
                                     Modal.confirm({
                                         onOk: () => {
-                                            dispatch(leaveOrganizationAsync(organizationInstance));
+                                            dispatch(leaveOrganizationAsync(organizationInstance, () => {
+                                                localStorage.removeItem('currentOrganization');
+                                                window.location.reload();
+                                            }));
                                         },
                                         className: 'cvat-modal-organization-leave-confirm',
                                         content: (

cvat/apps/iam/rules/memberships.rego

@@ -71,6 +71,7 @@ allow {
     input.resource.organization.id == input.auth.organization.id
 }
 
+# maintainer of the organization can change the role of any member and remove any member except himself/another maintainer/owner
 allow {
     { utils.CHANGE_ROLE, utils.DELETE }[input.scope]
     input.resource.is_active
@@ -81,22 +82,28 @@ allow {
         organizations.OWNER,
         organizations.MAINTAINER
     }[input.resource.role]
+    input.resource.user.id != input.auth.user.id
 }
 
+
+# owner of the organization can change the role of any member and remove any member except himself
 allow {
     { utils.CHANGE_ROLE, utils.DELETE }[input.scope]
     input.resource.is_active
     input.resource.organization.id == input.auth.organization.id
     utils.has_perm(utils.USER)
     organizations.is_owner
+    input.resource.user.id != input.auth.user.id
     input.resource.role != organizations.OWNER
 }
 
+# member can leave the organization except case when member is the owner
 allow {
     input.scope == utils.DELETE
     input.resource.is_active
-    utils.is_sandbox
-    input.resource.role != organizations.OWNER
+    organizations.is_member
+    input.resource.organization.id == input.auth.organization.id
     input.resource.user.id == input.auth.user.id
+    input.resource.role != organizations.OWNER
     utils.has_perm(utils.WORKER)
 }

cvat/apps/iam/rules/tests/configs/memberships.csv

@@ -4,8 +4,8 @@ list,Membership,Organization,N/A,,GET,/memberships,None,Worker
 view,Membership,Sandbox,None,,GET,/membership/{id},Admin,N/A
 view,Membership,Sandbox,Self,,GET,/membership/{id},None,N/A
 view,Membership,Organization,"None, Self",,GET,/membership/{id},None,Worker
-change:role,Membership,Organization,"None, Self","resource[""role""] not in [""maintainer"", ""owner""]",PATCH,/membership/{id},User,Maintainer
-change:role,Membership,Organization,"None, Self","resource[""role""] != ""owner""",PATCH,/membership/{id},User,Owner
-delete,Membership,Organization,"None, Self","resource[""role""] not in [""maintainer"", ""owner""]",DELETE,/membership/{id},User,Maintainer
-delete,Membership,Organization,"None, Self","resource[""role""] != ""owner""",DELETE,/membership/{id},User,Owner
-delete,Membership,Sandbox,Self,"resource[""role""] != ""owner""",DELETE,/membership/{id},Worker,N/A
+change:role,Membership,Organization,None,"resource[""role""] != ""owner""",PATCH,/membership/{id},User,Owner
+change:role,Membership,Organization,None,"resource[""role""] not in [""maintainer"", ""owner""]",PATCH,/membership/{id},User,Maintainer
+delete,Membership,Organization,None,"resource[""role""] != ""owner""",DELETE,/membership/{id},User,Owner
+delete,Membership,Organization,None,"resource[""role""] not in [""maintainer"", ""owner""]",DELETE,/membership/{id},User,Maintainer
+delete,Membership,Organization,Self,"resource[""role""] != ""owner""",DELETE,/membership/{id},Worker,Worker

cvat/apps/organizations/serializers.py

@@ -112,6 +112,11 @@ class Meta:
         fields = ['id', 'user', 'organization', 'is_active', 'joined_date', 'role',
             'invitation']
         read_only_fields = fields
+        extra_kwargs = {
+            'invitation': {
+                'allow_null': True, # owner of an organization does not have an invitation
+            }
+        }
 
 class MembershipWriteSerializer(serializers.ModelSerializer):
     def to_representation(self, instance):

cvat/schema.yml

@@ -7965,6 +7965,7 @@ components:
         invitation:
           type: string
           readOnly: true
+          nullable: true
       required:
       - user
     MetaUser:

tests/python/rest_api/test_memberships.py

@@ -4,12 +4,13 @@
 # SPDX-License-Identifier: MIT
 
 from http import HTTPStatus
+from typing import ClassVar, List
 
 import pytest
 from cvat_sdk.api_client.api_client import ApiClient, Endpoint
 from deepdiff import DeepDiff
 
-from shared.utils.config import get_method, patch_method
+from shared.utils.config import get_method, make_api_client, patch_method
 
 from .utils import CollectionSimpleFilterTestBase
 
@@ -77,7 +78,8 @@ def test_can_use_simple_filter_for_object_list(self, field):
 
 @pytest.mark.usefixtures("restore_db_per_function")
 class TestPatchMemberships:
-    _ORG = 2
+    _ORG: ClassVar[int] = 1
+    ROLES: ClassVar[List[str]] = ["worker", "supervisor", "maintainer", "owner"]
 
     def _test_can_change_membership(self, user, membership_id, new_role):
         response = patch_method(
@@ -97,11 +99,16 @@ def _test_cannot_change_membership(self, user, membership_id, new_role):
     @pytest.mark.parametrize(
         "who, whom, new_role, is_allow",
         [
-            ("supervisor", "worker", "supervisor", False),
-            ("supervisor", "maintainer", "supervisor", False),
+            ("worker", "worker", "supervisor", False),
             ("worker", "supervisor", "worker", False),
             ("worker", "maintainer", "worker", False),
+            ("worker", "owner", "worker", False),
+            ("supervisor", "worker", "supervisor", False),
+            ("supervisor", "supervisor", "worker", False),
+            ("supervisor", "maintainer", "supervisor", False),
+            ("supervisor", "owner", "worker", False),
             ("maintainer", "maintainer", "worker", False),
+            ("maintainer", "owner", "worker", False),
             ("maintainer", "supervisor", "worker", True),
             ("maintainer", "worker", "supervisor", True),
             ("owner", "maintainer", "worker", True),
@@ -111,9 +118,85 @@ def _test_cannot_change_membership(self, user, membership_id, new_role):
     )
     def test_user_can_change_role_of_member(self, who, whom, new_role, is_allow, find_users):
         user = find_users(org=self._ORG, role=who)[0]["username"]
-        membership_id = find_users(org=self._ORG, role=whom)[1]["membership_id"]
+        membership_id = find_users(org=self._ORG, role=whom, exclude_username=user)[0][
+            "membership_id"
+        ]
 
         if is_allow:
             self._test_can_change_membership(user, membership_id, new_role)
         else:
             self._test_cannot_change_membership(user, membership_id, new_role)
+
+    @pytest.mark.parametrize(
+        "who",
+        ROLES,
+    )
+    def test_user_cannot_change_self_role(self, who: str, find_users):
+        user = find_users(org=self._ORG, role=who)[0]
+        self._test_cannot_change_membership(
+            user["username"], user["membership_id"], self.ROLES[abs(self.ROLES.index(who) - 1)]
+        )
+
+
+@pytest.mark.usefixtures("restore_db_per_function")
+class TestDeleteMemberships:
+    _ORG: ClassVar[int] = 1
+
+    def _test_delete_membership(
+        self,
+        who: str,
+        membership_id: int,
+        is_allow: bool,
+    ) -> None:
+        expected_status = HTTPStatus.NO_CONTENT if is_allow else HTTPStatus.FORBIDDEN
+
+        with make_api_client(who) as api_client:
+            (_, response) = api_client.memberships_api.destroy(membership_id, _check_status=False)
+            assert response.status == expected_status
+
+    @pytest.mark.parametrize(
+        "who, is_allow",
+        [
+            ("worker", True),
+            ("supervisor", True),
+            ("maintainer", True),
+            ("owner", False),
+        ],
+    )
+    def test_member_can_leave_organization(self, who, is_allow, find_users):
+        user = find_users(role=who, org=self._ORG)[0]
+
+        self._test_delete_membership(user["username"], user["membership_id"], is_allow)
+
+    @pytest.mark.parametrize(
+        "who, whom, is_allow",
+        [
+            ("worker", "worker", False),
+            ("worker", "supervisor", False),
+            ("worker", "maintainer", False),
+            ("worker", "owner", False),
+            ("supervisor", "worker", False),
+            ("supervisor", "supervisor", False),
+            ("supervisor", "maintainer", False),
+            ("supervisor", "owner", False),
+            ("maintainer", "worker", True),
+            ("maintainer", "supervisor", True),
+            ("maintainer", "maintainer", False),
+            ("maintainer", "owner", False),
+            ("owner", "worker", True),
+            ("owner", "supervisor", True),
+            ("owner", "maintainer", True),
+        ],
+    )
+    def test_member_can_exclude_another_member(
+        self,
+        who: str,
+        whom: str,
+        is_allow: bool,
+        find_users,
+    ):
+        user = find_users(role=who, org=self._ORG)[0]["username"]
+        membership_id = find_users(role=whom, org=self._ORG, exclude_username=user)[0][
+            "membership_id"
+        ]
+        self._test_delete_membership(user, membership_id, is_allow)

tests/python/rest_api/test_projects.py

@@ -266,14 +266,14 @@ def test_org_supervisor_can_get_project_backup(
     def test_org_supervisor_cannot_get_project_backup(
         self, find_users, projects, is_project_staff, is_org_member
     ):
-        users = find_users(role="supervisor", exclude_privilege="admin")
+        users = find_users(exclude_privilege="admin")
 
         user, project = next(
             (user, project)
             for user, project in product(users, projects)
             if not is_project_staff(user["id"], project["id"])
             and project["organization"]
-            and is_org_member(user["id"], project["organization"])
+            and is_org_member(user["id"], project["organization"], role="supervisor")
         )
 
         self._test_cannot_get_project_backup(user["username"], project["id"])
@@ -890,14 +890,14 @@ def test_non_project_staff_org_members_cannot_add_label(
         is_org_member,
         role,
     ):
-        users = find_users(role=role, exclude_privilege="admin")
+        users = find_users(exclude_privilege="admin")
 
         user, project = next(
             (user, project)
             for user, project in product(users, projects)
             if not is_project_staff(user["id"], project["id"])
             and project["organization"]
-            and is_org_member(user["id"], project["organization"])
+            and is_org_member(user["id"], project["organization"], role=role)
         )
 
         new_label = {"name": "new name"}
@@ -913,14 +913,14 @@ def test_non_project_staff_org_members_cannot_add_label(
     def test_project_staff_org_members_can_add_label(
         self, find_users, projects, is_project_staff, is_org_member, labels, role
     ):
-        users = find_users(role=role, exclude_privilege="admin")
+        users = find_users(exclude_privilege="admin")
 
         user, project = next(
             (user, project)
             for user, project in product(users, projects)
             if is_project_staff(user["id"], project["id"])
             and project["organization"]
-            and is_org_member(user["id"], project["organization"])
+            and is_org_member(user["id"], project["organization"], role=role)
             and any(label.get("project_id") == project["id"] for label in labels)
         )
 

tests/python/rest_api/test_tasks.py

@@ -1609,14 +1609,14 @@ def test_non_task_staff_org_members_cannot_add_label(
         is_org_member,
         role,
     ):
-        users = find_users(role=role, exclude_privilege="admin")
+        users = find_users(exclude_privilege="admin")
 
         user, task = next(
             (user, task)
             for user, task in product(users, tasks)
             if not is_task_staff(user["id"], task["id"])
             and task["organization"]
-            and is_org_member(user["id"], task["organization"])
+            and is_org_member(user["id"], task["organization"], role=role)
         )
 
         new_label = {"name": "new name"}