From c13c1f289d060c3c7fd776c68fb39432ee75941a Mon Sep 17 00:00:00 2001 From: Hannes Janott Date: Thu, 15 Aug 2024 11:53:44 +0200 Subject: [PATCH 01/19] Allow meeting admin user to update a non admin user that shares all his meetings with requesting user. --- .../user/create_update_permissions_mixin.py | 80 +++++++++++++++++-- tests/system/action/user/test_update.py | 48 +++++++++++ 2 files changed, 123 insertions(+), 5 deletions(-) diff --git a/openslides_backend/action/actions/user/create_update_permissions_mixin.py b/openslides_backend/action/actions/user/create_update_permissions_mixin.py index ee5270ab9..2368d4070 100644 --- a/openslides_backend/action/actions/user/create_update_permissions_mixin.py +++ b/openslides_backend/action/actions/user/create_update_permissions_mixin.py @@ -213,7 +213,9 @@ class CreateUpdatePermissionsMixin(UserMixin, UserScopeMixin, Action): def check_permissions(self, instance: dict[str, Any]) -> None: """ Checks the permissions on a per field and user.scope base, details see - https://github.com/OpenSlides/OpenSlides/wiki/user.update or user.create + https://github.com/OpenSlides/OpenSlides/wiki/Users + https://github.com/OpenSlides/OpenSlides/wiki/Permission-System + https://github.com/OpenSlides/OpenSlides/wiki/Restrictions-Overview The fields groups and their necessary permissions are also documented there. """ self.assert_not_anonymous() @@ -227,13 +229,14 @@ def check_permissions(self, instance: dict[str, Any]) -> None: ) actual_group_fields = self._get_actual_grouping_from_instance(instance) - # store scope, id and OML-permission for requested user + # store id, scope, scope id and OML-permission for requested user + self.instance_id = instance.get("id", 0) ( self.instance_user_scope, self.instance_user_scope_id, self.instance_user_oml_permission, self.instance_committee_ids, - ) = self.get_user_scope(instance.get("id") or instance) + ) = self.get_user_scope(self.instance_id or instance) if self.permstore.user_oml != OrganizationManagementLevel.SUPERADMIN: self._check_for_higher_OML(actual_group_fields, instance) @@ -247,7 +250,7 @@ def check_permissions(self, instance: dict[str, Any]) -> None: lock_result=False, ).get("locked_from_inside", False) - # Ordered by supposed velocity advantages. Changing order can only effect the sequence of detected errors for tests + # Ordered by supposed speed advantages. Changing order can only effect the sequence of detected errors for tests self.check_group_H(actual_group_fields["H"]) self.check_group_E(actual_group_fields["E"], instance) self.check_group_D(actual_group_fields["D"], instance) @@ -257,6 +260,70 @@ def check_permissions(self, instance: dict[str, Any]) -> None: self.check_group_F(actual_group_fields["F"]) self.check_group_G(actual_group_fields["G"]) + def _meeting_admin_can_manage_non_admin(self) -> bool: + """ + Checks if the requesting user has permissions to manage participants in all of requested users meetings. + Also checks if the requesting user has meeting admin rights and the requested user doesn't. + Returns true if permissions are given. False if not. Raises no Exceptions. + """ + a_meeting_ids = self.permstore.user_meetings + b_meeting_ids = set( + self.datastore.get( + fqid_from_collection_and_id("user", self.instance_id), + ["meeting_ids"], + lock_result=False, + ).get("meeting_ids", []) + ) + intersection_meeting_ids = a_meeting_ids.intersection(b_meeting_ids) + if not b_meeting_ids.issubset(intersection_meeting_ids): + return False + intersection_meetings = self.datastore.get_many( + [ + GetManyRequest( + "meeting", + list(intersection_meeting_ids), + ["meeting_user_ids", "admin_group_id"], + ) + ], + lock_result=False, + ).get("meeting", {}) + for meeting_id, meeting_dict in intersection_meetings.items(): + # get meetings admins + admin_group = self.datastore.get( + fqid_from_collection_and_id( + "group", meeting_dict.get("admin_group_id", 0) + ), + ["meeting_user_ids"], + lock_result=False, + ) + admin_meeting_users = self.datastore.get_many( + [ + GetManyRequest( + "meeting_user", + admin_group.get("meeting_user_ids", []), + ["user_id"], + ) + ], + lock_result=False, + ).get("meeting_user", {}) + # if instance/requested user is a meeting admin in this meeting. + if [ + admin_meeting_user + for admin_meeting_user in admin_meeting_users.values() + if admin_meeting_user.get("user_id") == self.instance_id + ] != []: + return False + # if requesting user is not a meeting admin in this meeting. + if not next( + iter( + admin_meeting_user + for admin_meeting_user in admin_meeting_users.values() + if admin_meeting_user.get("user_id") == self.user_id + ) + ): + return False + return True + def check_group_A( self, fields: list[str], @@ -272,7 +339,10 @@ def check_group_A( if self.instance_user_scope == UserScope.Organization: if self.permstore.user_committees.intersection(self.instance_committee_ids): return - raise MissingPermission({OrganizationManagementLevel.CAN_MANAGE_USERS: 1}) + elif not self._meeting_admin_can_manage_non_admin(): + raise MissingPermission( + {OrganizationManagementLevel.CAN_MANAGE_USERS: 1} + ) if self.instance_user_scope == UserScope.Committee: if self.instance_user_scope_id not in self.permstore.user_committees: raise MissingPermission( diff --git a/tests/system/action/user/test_update.py b/tests/system/action/user/test_update.py index cba8a0487..51d235bf2 100644 --- a/tests/system/action/user/test_update.py +++ b/tests/system/action/user/test_update.py @@ -860,6 +860,54 @@ def test_perm_group_A_meeting_manage_user(self) -> None: }, ) + def test_perm_group_A_belongs_to_same_meetings(self) -> None: + """May update group A fields on any scope as long as admin user Ann belongs to all meetings user Ben belongs to. See issue 2522.""" + self.permission_setup() # meeting 1 + logged in test user + user 111 + self.create_meeting(4) # meeting 4 + # Admin groups of meeting/1 and meeting/4 for test user + self.set_user_groups(self.user_id, [2, 5]) + self.set_user_groups(111, [1, 4]) # 111 into both meetings + response = self.request( + "user.update", + { + "id": 111, + "pronoun": "I'm gonna get updated.", + }, + ) + self.assert_status_code(response, 200) + self.assert_model_exists( + "user/111", + { + "pronoun": "I'm gonna get updated.", + }, + ) + + def test_perm_group_A_belongs_to_same_meetings_both_admin(self) -> None: + """May update group A fields on any scope as long as admin user Ann belongs to all meetings user Ben belongs to. See issue 2522.""" + self.permission_setup() # meeting 1 + logged in test user + user 111 + self.create_meeting(4) # meeting 4 + # Admin groups of meeting/1 and meeting/4 for test user + self.set_user_groups(self.user_id, [2, 5]) + self.set_user_groups(111, [1, 5]) # 111 into both meetings one admin group + response = self.request( + "user.update", + { + "id": 111, + "pronoun": "I'm gonna get updated.", + }, + ) + self.assert_status_code(response, 403) + self.assertIn( + "You are not allowed to perform action user.update. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + response.json["message"], + ) + self.assert_model_exists( + "user/111", + { + "pronoun": None, + }, + ) + def test_perm_group_A_meeting_manage_user_archived_meeting(self) -> None: self.perm_group_A_meeting_manage_user_archived_meeting( Permissions.User.CAN_UPDATE From 130fbc9fb5634c31d842c7011b2d23903096eb8d Mon Sep 17 00:00:00 2001 From: Hannes Janott Date: Wed, 21 Aug 2024 12:56:22 +0200 Subject: [PATCH 02/19] prevent success on empty meetings and changing committee admin. --- .../user/create_update_permissions_mixin.py | 28 ++++++++++------- tests/system/action/user/test_update.py | 30 ++++++++++++++++++- 2 files changed, 46 insertions(+), 12 deletions(-) diff --git a/openslides_backend/action/actions/user/create_update_permissions_mixin.py b/openslides_backend/action/actions/user/create_update_permissions_mixin.py index 2368d4070..2620531bc 100644 --- a/openslides_backend/action/actions/user/create_update_permissions_mixin.py +++ b/openslides_backend/action/actions/user/create_update_permissions_mixin.py @@ -251,6 +251,8 @@ def check_permissions(self, instance: dict[str, Any]) -> None: ).get("locked_from_inside", False) # Ordered by supposed speed advantages. Changing order can only effect the sequence of detected errors for tests + if self._meeting_admin_can_manage_non_admin(): + return self.check_group_H(actual_group_fields["H"]) self.check_group_E(actual_group_fields["E"], instance) self.check_group_D(actual_group_fields["D"], instance) @@ -266,14 +268,18 @@ def _meeting_admin_can_manage_non_admin(self) -> bool: Also checks if the requesting user has meeting admin rights and the requested user doesn't. Returns true if permissions are given. False if not. Raises no Exceptions. """ - a_meeting_ids = self.permstore.user_meetings - b_meeting_ids = set( - self.datastore.get( + b_user = self.datastore.get( fqid_from_collection_and_id("user", self.instance_id), - ["meeting_ids"], + ["meeting_ids", "committee_management_ids"], lock_result=False, - ).get("meeting_ids", []) + ) + if b_user.get("committee_management_ids"): + return False + b_meeting_ids = set(b_user.get("meeting_ids", []) ) + if not b_meeting_ids: + return False + a_meeting_ids = self.permstore.user_meetings intersection_meeting_ids = a_meeting_ids.intersection(b_meeting_ids) if not b_meeting_ids.issubset(intersection_meeting_ids): return False @@ -339,10 +345,9 @@ def check_group_A( if self.instance_user_scope == UserScope.Organization: if self.permstore.user_committees.intersection(self.instance_committee_ids): return - elif not self._meeting_admin_can_manage_non_admin(): - raise MissingPermission( - {OrganizationManagementLevel.CAN_MANAGE_USERS: 1} - ) + raise MissingPermission( + {OrganizationManagementLevel.CAN_MANAGE_USERS: 1} + ) if self.instance_user_scope == UserScope.Committee: if self.instance_user_scope_id not in self.permstore.user_committees: raise MissingPermission( @@ -545,8 +550,9 @@ def _get_actual_grouping_from_instance( """ Returns a dictionary with an entry for each field group A-E with a list of fields from payload instance. - The field groups A-F refer to https://github.com/OpenSlides/OpenSlides/wiki/user.create - or user.update + The field groups A-F refer to https://github.com/OpenSlides/openslides-meta/blob/main/models.yml + or https://github.com/OpenSlides/openslides-backend/blob/main/docs/actions/user.create.md + or https://github.com/OpenSlides/openslides-backend/blob/main/docs/actions/user.update.md """ act_grouping: dict[str, list[str]] = defaultdict(list) for key, _ in instance.items(): diff --git a/tests/system/action/user/test_update.py b/tests/system/action/user/test_update.py index 51d235bf2..026abe7b6 100644 --- a/tests/system/action/user/test_update.py +++ b/tests/system/action/user/test_update.py @@ -893,7 +893,35 @@ def test_perm_group_A_belongs_to_same_meetings_both_admin(self) -> None: "user.update", { "id": 111, - "pronoun": "I'm gonna get updated.", + "pronoun": "I'm not gonna get updated.", + }, + ) + self.assert_status_code(response, 403) + self.assertIn( + "You are not allowed to perform action user.update. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + response.json["message"], + ) + self.assert_model_exists( + "user/111", + { + "pronoun": None, + }, + ) + + def test_perm_group_A_belongs_to_same_meetings_committe_admin(self) -> None: + """May not update group A fields on any scope as long as admin user Ann belongs + to all meetings user Ben belongs to but Ben is committee admin. See issue 2522.""" + self.permission_setup() # meeting 1 + logged in test user + user 111 + self.create_meeting(4) # meeting 4 + # Admin groups of meeting/1 and meeting/4 for test user + self.set_user_groups(self.user_id, [2, 5]) + self.set_user_groups(111, [1, 4]) # 111 into both meetings + self.set_committee_management_level([60], 111) # 111 is committee admin + response = self.request( + "user.update", + { + "id": 111, + "pronoun": "I'm not gonna get updated.", }, ) self.assert_status_code(response, 403) From f2ac3062ebefd19faea0744c18e3237c80fe4527 Mon Sep 17 00:00:00 2001 From: Hannes Janott Date: Wed, 21 Aug 2024 14:15:17 +0200 Subject: [PATCH 03/19] repair user create due to missing user id --- .../action/actions/user/create_update_permissions_mixin.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/openslides_backend/action/actions/user/create_update_permissions_mixin.py b/openslides_backend/action/actions/user/create_update_permissions_mixin.py index 2620531bc..7953fe726 100644 --- a/openslides_backend/action/actions/user/create_update_permissions_mixin.py +++ b/openslides_backend/action/actions/user/create_update_permissions_mixin.py @@ -268,6 +268,8 @@ def _meeting_admin_can_manage_non_admin(self) -> bool: Also checks if the requesting user has meeting admin rights and the requested user doesn't. Returns true if permissions are given. False if not. Raises no Exceptions. """ + if not self.instance_id: + return False b_user = self.datastore.get( fqid_from_collection_and_id("user", self.instance_id), ["meeting_ids", "committee_management_ids"], From a6d9665f57c99f6d238009aaec917d1e85749172 Mon Sep 17 00:00:00 2001 From: Hannes Janott Date: Wed, 21 Aug 2024 14:33:26 +0200 Subject: [PATCH 04/19] formatting --- .../user/create_update_permissions_mixin.py | 15 ++++++--------- tests/system/action/user/test_update.py | 7 ++++--- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/openslides_backend/action/actions/user/create_update_permissions_mixin.py b/openslides_backend/action/actions/user/create_update_permissions_mixin.py index 7953fe726..3ced52dcc 100644 --- a/openslides_backend/action/actions/user/create_update_permissions_mixin.py +++ b/openslides_backend/action/actions/user/create_update_permissions_mixin.py @@ -271,14 +271,13 @@ def _meeting_admin_can_manage_non_admin(self) -> bool: if not self.instance_id: return False b_user = self.datastore.get( - fqid_from_collection_and_id("user", self.instance_id), - ["meeting_ids", "committee_management_ids"], - lock_result=False, - ) + fqid_from_collection_and_id("user", self.instance_id), + ["meeting_ids", "committee_management_ids"], + lock_result=False, + ) if b_user.get("committee_management_ids"): return False - b_meeting_ids = set(b_user.get("meeting_ids", []) - ) + b_meeting_ids = set(b_user.get("meeting_ids", [])) if not b_meeting_ids: return False a_meeting_ids = self.permstore.user_meetings @@ -347,9 +346,7 @@ def check_group_A( if self.instance_user_scope == UserScope.Organization: if self.permstore.user_committees.intersection(self.instance_committee_ids): return - raise MissingPermission( - {OrganizationManagementLevel.CAN_MANAGE_USERS: 1} - ) + raise MissingPermission({OrganizationManagementLevel.CAN_MANAGE_USERS: 1}) if self.instance_user_scope == UserScope.Committee: if self.instance_user_scope_id not in self.permstore.user_committees: raise MissingPermission( diff --git a/tests/system/action/user/test_update.py b/tests/system/action/user/test_update.py index 026abe7b6..f7bfc0fe4 100644 --- a/tests/system/action/user/test_update.py +++ b/tests/system/action/user/test_update.py @@ -909,14 +909,15 @@ def test_perm_group_A_belongs_to_same_meetings_both_admin(self) -> None: ) def test_perm_group_A_belongs_to_same_meetings_committe_admin(self) -> None: - """May not update group A fields on any scope as long as admin user Ann belongs - to all meetings user Ben belongs to but Ben is committee admin. See issue 2522.""" + """May not update group A fields on any scope as long as admin user Ann belongs + to all meetings user Ben belongs to but Ben is committee admin. See issue 2522. + """ self.permission_setup() # meeting 1 + logged in test user + user 111 self.create_meeting(4) # meeting 4 # Admin groups of meeting/1 and meeting/4 for test user self.set_user_groups(self.user_id, [2, 5]) self.set_user_groups(111, [1, 4]) # 111 into both meetings - self.set_committee_management_level([60], 111) # 111 is committee admin + self.set_committee_management_level([60], 111) # 111 is committee admin response = self.request( "user.update", { From 56928aa28c0e1a72fea1122811fc6d4a2dec36a7 Mon Sep 17 00:00:00 2001 From: Hannes Janott Date: Mon, 26 Aug 2024 14:00:47 +0200 Subject: [PATCH 05/19] draft an idea for scope change in user scope mixin. Add tests for groups A and F fields. --- .../user/create_update_permissions_mixin.py | 148 ++++----- .../shared/mixins/user_scope_mixin.py | 74 ++++- tests/system/action/user/test_update.py | 299 +++++++++++++++++- 3 files changed, 439 insertions(+), 82 deletions(-) diff --git a/openslides_backend/action/actions/user/create_update_permissions_mixin.py b/openslides_backend/action/actions/user/create_update_permissions_mixin.py index 3ced52dcc..e4c867fb5 100644 --- a/openslides_backend/action/actions/user/create_update_permissions_mixin.py +++ b/openslides_backend/action/actions/user/create_update_permissions_mixin.py @@ -203,7 +203,7 @@ class CreateUpdatePermissionsMixin(UserMixin, UserScopeMixin, Action): "is_present", # participant import ], "C": ["meeting_id", "group_ids"], - "D": ["committee_ids", "committee_management_ids"], + "D": ["committee_management_ids"], "E": ["organization_management_level"], "F": ["default_password"], "G": ["is_demo_user"], @@ -229,14 +229,13 @@ def check_permissions(self, instance: dict[str, Any]) -> None: ) actual_group_fields = self._get_actual_grouping_from_instance(instance) - # store id, scope, scope id and OML-permission for requested user - self.instance_id = instance.get("id", 0) + # store scope, scope id, OML-permission and committee ids for requested user ( self.instance_user_scope, self.instance_user_scope_id, self.instance_user_oml_permission, self.instance_committee_ids, - ) = self.get_user_scope(self.instance_id or instance) + ) = self.get_user_scope(instance.get("id") or instance) if self.permstore.user_oml != OrganizationManagementLevel.SUPERADMIN: self._check_for_higher_OML(actual_group_fields, instance) @@ -250,9 +249,10 @@ def check_permissions(self, instance: dict[str, Any]) -> None: lock_result=False, ).get("locked_from_inside", False) + # positive test for user that can manage in every meeting of updated user. + # if self._meeting_admin_can_manage_non_admin(instance.get("id") ): + # return # Ordered by supposed speed advantages. Changing order can only effect the sequence of detected errors for tests - if self._meeting_admin_can_manage_non_admin(): - return self.check_group_H(actual_group_fields["H"]) self.check_group_E(actual_group_fields["E"], instance) self.check_group_D(actual_group_fields["D"], instance) @@ -262,74 +262,74 @@ def check_permissions(self, instance: dict[str, Any]) -> None: self.check_group_F(actual_group_fields["F"]) self.check_group_G(actual_group_fields["G"]) - def _meeting_admin_can_manage_non_admin(self) -> bool: - """ - Checks if the requesting user has permissions to manage participants in all of requested users meetings. - Also checks if the requesting user has meeting admin rights and the requested user doesn't. - Returns true if permissions are given. False if not. Raises no Exceptions. - """ - if not self.instance_id: - return False - b_user = self.datastore.get( - fqid_from_collection_and_id("user", self.instance_id), - ["meeting_ids", "committee_management_ids"], - lock_result=False, - ) - if b_user.get("committee_management_ids"): - return False - b_meeting_ids = set(b_user.get("meeting_ids", [])) - if not b_meeting_ids: - return False - a_meeting_ids = self.permstore.user_meetings - intersection_meeting_ids = a_meeting_ids.intersection(b_meeting_ids) - if not b_meeting_ids.issubset(intersection_meeting_ids): - return False - intersection_meetings = self.datastore.get_many( - [ - GetManyRequest( - "meeting", - list(intersection_meeting_ids), - ["meeting_user_ids", "admin_group_id"], - ) - ], - lock_result=False, - ).get("meeting", {}) - for meeting_id, meeting_dict in intersection_meetings.items(): - # get meetings admins - admin_group = self.datastore.get( - fqid_from_collection_and_id( - "group", meeting_dict.get("admin_group_id", 0) - ), - ["meeting_user_ids"], - lock_result=False, - ) - admin_meeting_users = self.datastore.get_many( - [ - GetManyRequest( - "meeting_user", - admin_group.get("meeting_user_ids", []), - ["user_id"], - ) - ], - lock_result=False, - ).get("meeting_user", {}) - # if instance/requested user is a meeting admin in this meeting. - if [ - admin_meeting_user - for admin_meeting_user in admin_meeting_users.values() - if admin_meeting_user.get("user_id") == self.instance_id - ] != []: - return False - # if requesting user is not a meeting admin in this meeting. - if not next( - iter( - admin_meeting_user - for admin_meeting_user in admin_meeting_users.values() - if admin_meeting_user.get("user_id") == self.user_id - ) - ): - return False - return True + # def _meeting_admin_can_manage_non_admin(self, instance_id: int) -> bool: + # """ + # Checks if the requesting user has permissions to manage participants in all of requested users meetings. + # Also checks if the requesting user has meeting admin rights and the requested user doesn't. + # Returns true if permissions are given. False if not. Raises no Exceptions. + # """ + # if not instance_id: + # return False + # b_user = self.datastore.get( + # fqid_from_collection_and_id("user", instance_id), + # ["meeting_ids", "committee_management_ids"], + # lock_result=False, + # ) + # if b_user.get("committee_management_ids"): + # return False + # b_meeting_ids = set(b_user.get("meeting_ids", [])) + # if not b_meeting_ids: + # return False + # a_meeting_ids = self.permstore.user_meetings + # intersection_meeting_ids = a_meeting_ids.intersection(b_meeting_ids) + # if not b_meeting_ids.issubset(intersection_meeting_ids): + # return False + # intersection_meetings = self.datastore.get_many( + # [ + # GetManyRequest( + # "meeting", + # list(intersection_meeting_ids), + # ["meeting_user_ids", "admin_group_id"], + # ) + # ], + # lock_result=False, + # ).get("meeting", {}) + # for meeting_id, meeting_dict in intersection_meetings.items(): + # # get meetings admins + # admin_group = self.datastore.get( + # fqid_from_collection_and_id( + # "group", meeting_dict.get("admin_group_id", 0) + # ), + # ["meeting_user_ids"], + # lock_result=False, + # ) + # admin_meeting_users = self.datastore.get_many( + # [ + # GetManyRequest( + # "meeting_user", + # admin_group.get("meeting_user_ids", []), + # ["user_id"], + # ) + # ], + # lock_result=False, + # ).get("meeting_user", {}) + # # if instance/requested user is a meeting admin in this meeting. + # if [ + # admin_meeting_user + # for admin_meeting_user in admin_meeting_users.values() + # if admin_meeting_user.get("user_id") == instance_id + # ] != []: + # return False + # # if requesting user is not a meeting admin in this meeting. + # if not next( + # iter( + # admin_meeting_user + # for admin_meeting_user in admin_meeting_users.values() + # if admin_meeting_user.get("user_id") == self.user_id + # ) + # ): + # return False + # return True def check_group_A( self, diff --git a/openslides_backend/shared/mixins/user_scope_mixin.py b/openslides_backend/shared/mixins/user_scope_mixin.py index b84b6050f..543655484 100644 --- a/openslides_backend/shared/mixins/user_scope_mixin.py +++ b/openslides_backend/shared/mixins/user_scope_mixin.py @@ -48,6 +48,7 @@ def get_user_scope( set(id_or_instance.get("committee_management_ids", [])) ) oml_right = id_or_instance.get("organization_management_level", "") + meetingcheck = self.check_for_admin_in_all_meetings(id_or_instance.get("id")) else: user = self.datastore.get( fqid_from_collection_and_id("user", id_or_instance), @@ -60,6 +61,7 @@ def get_user_scope( meetings.update(user.get("meeting_ids", [])) committees_manager.update(set(user.get("committee_management_ids") or [])) oml_right = user.get("organization_management_level", "") + meetingcheck = self.check_for_admin_in_all_meetings(id_or_instance) result = self.datastore.get_many( [ GetManyRequest( @@ -76,7 +78,7 @@ def get_user_scope( if meeting_data.get("is_active_in_organization_id") } committees = committees_manager | set(meetings_committee.values()) - if len(meetings_committee) == 1 and len(committees) == 1: + if len(meetings_committee) == 1 and len(committees) == 1 or meetingcheck: return ( UserScope.Meeting, next(iter(meetings_committee)), @@ -164,3 +166,73 @@ def check_permissions_for_scope( ): return raise MissingPermission({OrganizationManagementLevel.CAN_MANAGE_USERS: 1}) + + + def check_for_admin_in_all_meetings(self, instance_id: int) -> bool: + """ + Checks if the requesting user has permissions to manage participants in all of requested users meetings. + Also checks if the requesting user has meeting admin rights and the requested user doesn't. + Returns true if permissions are given. False if not. Raises no Exceptions. + """ + if not instance_id: + return False + b_user = self.datastore.get( + fqid_from_collection_and_id("user", instance_id), + ["meeting_ids", "committee_management_ids"], + lock_result=False, + ) + if b_user.get("committee_management_ids"): + return False + b_meeting_ids = set(b_user.get("meeting_ids", [])) + if not b_meeting_ids: + return False + a_meeting_ids = self.permstore.user_meetings + intersection_meeting_ids = a_meeting_ids.intersection(b_meeting_ids) + if not b_meeting_ids.issubset(intersection_meeting_ids): + return False + intersection_meetings = self.datastore.get_many( + [ + GetManyRequest( + "meeting", + list(intersection_meeting_ids), + ["meeting_user_ids", "admin_group_id"], + ) + ], + lock_result=False, + ).get("meeting", {}) + for meeting_id, meeting_dict in intersection_meetings.items(): + # get meetings admins + admin_group = self.datastore.get( + fqid_from_collection_and_id( + "group", meeting_dict.get("admin_group_id", 0) + ), + ["meeting_user_ids"], + lock_result=False, + ) + admin_meeting_users = self.datastore.get_many( + [ + GetManyRequest( + "meeting_user", + admin_group.get("meeting_user_ids", []), + ["user_id"], + ) + ], + lock_result=False, + ).get("meeting_user", {}) + # if instance/requested user is a meeting admin in this meeting. + if [ + admin_meeting_user + for admin_meeting_user in admin_meeting_users.values() + if admin_meeting_user.get("user_id") == instance_id + ] != []: + return False + # if requesting user is not a meeting admin in this meeting. + if not next( + iter( + admin_meeting_user + for admin_meeting_user in admin_meeting_users.values() + if admin_meeting_user.get("user_id") == self.user_id + ) + ): + return False + return True \ No newline at end of file diff --git a/tests/system/action/user/test_update.py b/tests/system/action/user/test_update.py index f7bfc0fe4..91446f386 100644 --- a/tests/system/action/user/test_update.py +++ b/tests/system/action/user/test_update.py @@ -17,6 +17,134 @@ def permission_setup(self) -> None: } ) + def two_meetings_standard_fails(self, committee_id: None | int = None, group_B_success: bool = False) -> None: + #test group A + response = self.request( + "user.update", + { + "id": 111, + "pronoun": "I'm not gonna get updated.", + }, + ) + self.assert_status_code(response, 403) + self.assertIn( + "You are not allowed to perform action user.update. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + response.json["message"], + ) + self.assert_model_exists( + "user/111", + { + "pronoun": None, + }, + ) + #test group D + response = self.request( + "user.update", + { + "id": 111, + "committee_management_ids": [1,2], + }, + ) + self.assert_status_code(response, 403) + if committee_id: + self.assertIn( + f"You are not allowed to perform action user.update. Missing permission: CommitteeManagementLevel can_manage in committee {committee_id}", + response.json["message"], + ) + self.assert_model_exists( + "user/111", + { + "committee_management_ids": [committee_id], + }, + ) + else: + self.assertIn( + "You are not allowed to perform action user.update. Missing permission: CommitteeManagementLevel can_manage in committee ", + response.json["message"], + ) + self.assert_model_exists( + "user/111", + { + "committee_management_ids": None, + }, + ) + #test group E + response = self.request( + "user.update", + { + "id": 111, + "organization_management_level": OrganizationManagementLevel.CAN_MANAGE_USERS, + }, + ) + self.assert_status_code(response, 403) + self.assertIn( + "Your organization management level is not high enough to set a Level of can_manage_users.", + response.json["message"], + ) + self.assert_model_exists( + "user/111", + { + "organization_management_level": None, + }, + ) + #test group F + response = self.request( + "user.update", + { + "id": 111, + "default_password": "I'm not gonna get updated.", + }, + ) + self.assert_status_code(response, 403) + self.assertIn( + "You are not allowed to perform action user.update. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + response.json["message"], + ) + self.assert_model_exists( + "user/111", + { + "default_password": None, + }, + ) + #test group G + response = self.request( + "user.update", + { + "id": 111, + "is_demo_user": True, + }, + ) + self.assert_status_code(response, 403) + self.assertIn( + "You are not allowed to perform action user.update. Missing OrganizationManagementLevel: superadmin", + response.json["message"], + ) + self.assert_model_exists( + "user/111", + { + "is_demo_user": None, + }, + ) + #test group H + response = self.request( + "user.update", + { + "id": 111, + "saml_id": "I'm not gonna get updated.", + }, + ) + self.assert_status_code(response, 400) + self.assertIn( + "The field 'saml_id' can only be used in internal action calls", + response.json["message"], + ) + self.assert_model_exists( + "user/111", + { + "saml_id": None, + }, + ) + def test_update_correct(self) -> None: self.create_model( "user/111", @@ -866,7 +994,8 @@ def test_perm_group_A_belongs_to_same_meetings(self) -> None: self.create_meeting(4) # meeting 4 # Admin groups of meeting/1 and meeting/4 for test user self.set_user_groups(self.user_id, [2, 5]) - self.set_user_groups(111, [1, 4]) # 111 into both meetings + # 111 into both meetings + self.set_user_groups(111, [1, 4]) response = self.request( "user.update", { @@ -882,13 +1011,61 @@ def test_perm_group_A_belongs_to_same_meetings(self) -> None: }, ) + def test_perm_group_A_belongs_to_same_meetings_one_time_admin(self) -> None: + """May update group A fields on any scope as long as admin user Ann belongs to all meetings user Ben belongs to. However, is not admin in one. See issue 2522.""" + self.permission_setup() # meeting 1 + logged in test user + user 111 + self.create_meeting(4) # meeting 4 + # Admin groups of only meeting/1 for test user + self.set_user_groups(self.user_id, [2, 4]) + # 111 into both meetings + self.set_user_groups(111, [1, 4]) + self.two_meetings_standard_fails() + #test group B and C + response = self.request( + "user.update", + { + "id": 111, + "number": "I'm not gonna get updated.", + "meeting_id": 4 + }, + ) + self.assert_status_code(response, 403) + self.assertIn( + "The user needs OrganizationManagementLevel.can_manage_users or CommitteeManagementLevel.can_manage for committee of following meeting or Permission user.can_update for meeting 4", + response.json["message"], + ) + self.assert_model_exists( + "user/111", + { + "number": None, + }, + ) + def test_perm_group_A_belongs_to_same_meetings_both_admin(self) -> None: - """May update group A fields on any scope as long as admin user Ann belongs to all meetings user Ben belongs to. See issue 2522.""" + """May update group A fields on any scope as long as admin user Ann belongs to all meetings user Ben belongs to. However, Ben is admin too. See issue 2522.""" self.permission_setup() # meeting 1 + logged in test user + user 111 self.create_meeting(4) # meeting 4 # Admin groups of meeting/1 and meeting/4 for test user self.set_user_groups(self.user_id, [2, 5]) - self.set_user_groups(111, [1, 5]) # 111 into both meetings one admin group + # 111 into both meetings (one admin group) + self.set_user_groups(111, [1, 5]) + self.two_meetings_standard_fails() + #test group B and C + response = self.request( + "user.update", + { + "id": 111, + "number": "I'm not gonna get updated.", + "meeting_id": 4 + }, + ) + self.assert_status_code(response, 200) + self.assert_model_exists( + "user/111", + { + "number": None, + }, + ) response = self.request( "user.update", { @@ -908,7 +1085,7 @@ def test_perm_group_A_belongs_to_same_meetings_both_admin(self) -> None: }, ) - def test_perm_group_A_belongs_to_same_meetings_committe_admin(self) -> None: + def test_perm_group_A_belongs_to_same_meetings_committee_admin(self) -> None: """May not update group A fields on any scope as long as admin user Ann belongs to all meetings user Ben belongs to but Ben is committee admin. See issue 2522. """ @@ -916,8 +1093,28 @@ def test_perm_group_A_belongs_to_same_meetings_committe_admin(self) -> None: self.create_meeting(4) # meeting 4 # Admin groups of meeting/1 and meeting/4 for test user self.set_user_groups(self.user_id, [2, 5]) - self.set_user_groups(111, [1, 4]) # 111 into both meetings - self.set_committee_management_level([60], 111) # 111 is committee admin + # 111 into both meetings + self.set_user_groups(111, [1, 4]) + # 111 is committee admin + committee_id = 60 + self.set_committee_management_level([committee_id], 111) + self.two_meetings_standard_fails(committee_id) + #test group B and C + response = self.request( + "user.update", + { + "id": 111, + "number": "I'm not gonna get updated.", + "meeting_id": 4 + }, + ) + self.assert_status_code(response, 200) + self.assert_model_exists( + "user/111", + { + "number": None, + }, + ) response = self.request( "user.update", { @@ -1063,7 +1260,7 @@ def test_perm_group_F_default_password_for_superadmin_no_permission(self) -> Non ) def test_perm_group_F_cml_manage_user_with_two_committees(self) -> None: - """May update group A fields on committee scope. User belongs to 1 meeting in 1 committee""" + """May update group F fields on committee scope. User belongs to two meetings.""" self.permission_setup() self.create_meeting(4) self.set_committee_management_level([60], self.user_id) @@ -1085,6 +1282,94 @@ def test_perm_group_F_cml_manage_user_with_two_committees(self) -> None: }, ) + def test_perm_group_F_with_meeting_scope(self) -> None: + """May not update group F fields on meeting scope unless CML rights. User belongs to a meeting, request user having no admin rights in.""" + self.permission_setup() + self.create_meeting(4) + self.set_user_groups(111, [2]) + self.set_user_groups(self.user_id, [5]) + self.set_models({f"user/{self.user_id}": {"committee_management_ids": [63]}}) + + response = self.request( + "user.update", + { + "id": 111, + "default_password": "new_one", + }, + ) + self.assert_status_code(response, 403) + self.assertIn( + "You are not allowed to perform action user.update. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or CommitteeManagementLevel can_manage in committee 60 or Permission user.can_update in meeting 1", + response.json["message"], + ) + self.assert_model_exists( + "user/111", + { + "default_password": None, + }, + ) + # assert it works with correct CML/ MA can change MA + self.set_user_groups(111, [2]) + self.set_user_groups(self.user_id, [2]) + # self.set_models({f"user/{self.user_id}": {"committee_management_ids": [60]}}) + response = self.request( + "user.update", + { + "id": 111, + "default_password": "new_one", + }, + ) + self.assert_status_code(response, 200) + self.assert_model_exists( + "user/111", + { + "default_password": "new_one", + }, + ) + + def test_perm_group_F_with_meeting_scope_across_committees(self) -> None: + """May not update group F fields on meeting scope unless CML rights. User belongs to several meetings, request user having admin rights in.""" + self.permission_setup() + self.create_meeting(4) + self.set_user_groups(111, [1, 4]) + self.set_user_groups(self.user_id, [2, 4]) + + response = self.request( + "user.update", + { + "id": 111, + "default_password": "new_one", + }, + ) + self.assert_status_code(response, 403) + self.assertIn( + "You are not allowed to perform action user.update. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + response.json["message"], + ) + self.assert_model_exists( + "user/111", + { + "default_password": None, + }, + ) + # assert MA can change MA + self.set_user_groups(111, [1, 4]) + self.set_user_groups(self.user_id, [2, 5]) + response = self.request( + "user.update", + { + "id": 111, + "default_password": "new_one", + }, + ) + self.assert_status_code(response, 200) + self.assert_model_exists( + "user/111", + { + "default_password": "new_one", + }, + ) + def test_perm_group_B_user_can_update(self) -> None: """update group B fields for 2 meetings with simple user.can_update permissions""" self.permission_setup() From f4eec94eb1d438dcd6de67b6be94b92557e6743a Mon Sep 17 00:00:00 2001 From: Hannes Janott Date: Wed, 28 Aug 2024 14:59:15 +0200 Subject: [PATCH 06/19] use function in field group methods and check_permissions for scop --- docs/actions/user.set_password.md | 1 - docs/presenters/get_user_related_models.md | 4 +- .../user/create_update_permissions_mixin.py | 115 ++++-------------- .../action/actions/user/set_password.py | 1 + .../shared/mixins/user_scope_mixin.py | 90 ++++++++------ tests/system/action/user/test_set_password.py | 46 +++++++ tests/system/action/user/test_update.py | 52 ++++---- .../presenter/test_get_user_related_models.py | 113 +++++++++++++++++ 8 files changed, 257 insertions(+), 165 deletions(-) diff --git a/docs/actions/user.set_password.md b/docs/actions/user.set_password.md index af0a4e856..cc6495240 100644 --- a/docs/actions/user.set_password.md +++ b/docs/actions/user.set_password.md @@ -9,7 +9,6 @@ set_as_default: boolean; // default false, if not given } ``` - ## Action Sets the password of the user given by `id` to `password`. If `set_as_default` is true, the `default_password` is also updated. diff --git a/docs/presenters/get_user_related_models.md b/docs/presenters/get_user_related_models.md index dc7cfab91..ce05020d1 100644 --- a/docs/presenters/get_user_related_models.md +++ b/docs/presenters/get_user_related_models.md @@ -1,6 +1,6 @@ ## Payload -```js +``` { user_ids: Id[]; } @@ -8,7 +8,7 @@ ## Returns -```js +``` { [user_id: Id]: { organization_management_level: OML-String, diff --git a/openslides_backend/action/actions/user/create_update_permissions_mixin.py b/openslides_backend/action/actions/user/create_update_permissions_mixin.py index e4c867fb5..df430eb50 100644 --- a/openslides_backend/action/actions/user/create_update_permissions_mixin.py +++ b/openslides_backend/action/actions/user/create_update_permissions_mixin.py @@ -235,7 +235,7 @@ def check_permissions(self, instance: dict[str, Any]) -> None: self.instance_user_scope_id, self.instance_user_oml_permission, self.instance_committee_ids, - ) = self.get_user_scope(instance.get("id") or instance) + ) = self.get_user_scope(instance.get("id") or instance) if self.permstore.user_oml != OrganizationManagementLevel.SUPERADMIN: self._check_for_higher_OML(actual_group_fields, instance) @@ -249,92 +249,17 @@ def check_permissions(self, instance: dict[str, Any]) -> None: lock_result=False, ).get("locked_from_inside", False) - # positive test for user that can manage in every meeting of updated user. - # if self._meeting_admin_can_manage_non_admin(instance.get("id") ): - # return # Ordered by supposed speed advantages. Changing order can only effect the sequence of detected errors for tests self.check_group_H(actual_group_fields["H"]) self.check_group_E(actual_group_fields["E"], instance) self.check_group_D(actual_group_fields["D"], instance) self.check_group_C(actual_group_fields["C"], instance, locked_from_inside) self.check_group_B(actual_group_fields["B"], instance, locked_from_inside) - self.check_group_A(actual_group_fields["A"]) - self.check_group_F(actual_group_fields["F"]) + self.check_group_A(actual_group_fields["A"], instance) + self.check_group_F(actual_group_fields["F"], instance) self.check_group_G(actual_group_fields["G"]) - # def _meeting_admin_can_manage_non_admin(self, instance_id: int) -> bool: - # """ - # Checks if the requesting user has permissions to manage participants in all of requested users meetings. - # Also checks if the requesting user has meeting admin rights and the requested user doesn't. - # Returns true if permissions are given. False if not. Raises no Exceptions. - # """ - # if not instance_id: - # return False - # b_user = self.datastore.get( - # fqid_from_collection_and_id("user", instance_id), - # ["meeting_ids", "committee_management_ids"], - # lock_result=False, - # ) - # if b_user.get("committee_management_ids"): - # return False - # b_meeting_ids = set(b_user.get("meeting_ids", [])) - # if not b_meeting_ids: - # return False - # a_meeting_ids = self.permstore.user_meetings - # intersection_meeting_ids = a_meeting_ids.intersection(b_meeting_ids) - # if not b_meeting_ids.issubset(intersection_meeting_ids): - # return False - # intersection_meetings = self.datastore.get_many( - # [ - # GetManyRequest( - # "meeting", - # list(intersection_meeting_ids), - # ["meeting_user_ids", "admin_group_id"], - # ) - # ], - # lock_result=False, - # ).get("meeting", {}) - # for meeting_id, meeting_dict in intersection_meetings.items(): - # # get meetings admins - # admin_group = self.datastore.get( - # fqid_from_collection_and_id( - # "group", meeting_dict.get("admin_group_id", 0) - # ), - # ["meeting_user_ids"], - # lock_result=False, - # ) - # admin_meeting_users = self.datastore.get_many( - # [ - # GetManyRequest( - # "meeting_user", - # admin_group.get("meeting_user_ids", []), - # ["user_id"], - # ) - # ], - # lock_result=False, - # ).get("meeting_user", {}) - # # if instance/requested user is a meeting admin in this meeting. - # if [ - # admin_meeting_user - # for admin_meeting_user in admin_meeting_users.values() - # if admin_meeting_user.get("user_id") == instance_id - # ] != []: - # return False - # # if requesting user is not a meeting admin in this meeting. - # if not next( - # iter( - # admin_meeting_user - # for admin_meeting_user in admin_meeting_users.values() - # if admin_meeting_user.get("user_id") == self.user_id - # ) - # ): - # return False - # return True - - def check_group_A( - self, - fields: list[str], - ) -> None: + def check_group_A(self, fields: list[str], instance: dict[str, Any]) -> None: """Check Group A: Depending on scope of user to act on""" if ( self.permstore.user_oml == OrganizationManagementLevel.SUPERADMIN @@ -346,15 +271,19 @@ def check_group_A( if self.instance_user_scope == UserScope.Organization: if self.permstore.user_committees.intersection(self.instance_committee_ids): return - raise MissingPermission({OrganizationManagementLevel.CAN_MANAGE_USERS: 1}) - if self.instance_user_scope == UserScope.Committee: - if self.instance_user_scope_id not in self.permstore.user_committees: + elif not self.check_for_admin_in_all_meetings(instance.get("id", 0)): raise MissingPermission( - { - OrganizationManagementLevel.CAN_MANAGE_USERS: 1, - CommitteeManagementLevel.CAN_MANAGE: self.instance_user_scope_id, - } + {OrganizationManagementLevel.CAN_MANAGE_USERS: 1} ) + if self.instance_user_scope == UserScope.Committee: + if self.instance_user_scope_id not in self.permstore.user_committees: + if not self.check_for_admin_in_all_meetings(instance.get("id", 0)): + raise MissingPermission( + { + OrganizationManagementLevel.CAN_MANAGE_USERS: 1, + CommitteeManagementLevel.CAN_MANAGE: self.instance_user_scope_id, + } + ) elif ( self.instance_user_scope_id not in self.permstore.user_committees_meetings and self.instance_user_scope_id not in self.permstore.user_meetings @@ -434,10 +363,7 @@ def check_group_E(self, fields: list[str], instance: dict[str, Any]) -> None: f"Your organization management level is not high enough to set a Level of {instance.get('organization_management_level', OrganizationManagementLevel.CAN_MANAGE_USERS.get_verbose_type())}." ) - def check_group_F( - self, - fields: list[str], - ) -> None: + def check_group_F(self, fields: list[str], instance: dict[str, Any]) -> None: """Check F common fields: scoped permissions necessary, but if instance user has an oml-permission, that of the request user must be higher""" if ( @@ -460,7 +386,10 @@ def check_group_F( ): return expected_oml_permission = OrganizationManagementLevel.CAN_MANAGE_USERS - if expected_oml_permission > self.permstore.user_oml: + if ( + expected_oml_permission > self.permstore.user_oml + and not self.check_for_admin_in_all_meetings(instance.get("id", 0)) + ): raise MissingPermission({expected_oml_permission: 1}) else: return @@ -673,8 +602,8 @@ def get_failing_fields(self, instance: dict[str, Any]) -> list[str]: instance, locked_from_inside, ), - (self.check_group_A, actual_group_fields["A"], None, None), - (self.check_group_F, actual_group_fields["F"], None, None), + (self.check_group_A, actual_group_fields["A"], instance, None), + (self.check_group_F, actual_group_fields["F"], instance, None), (self.check_group_G, actual_group_fields["G"], None, None), ]: try: diff --git a/openslides_backend/action/actions/user/set_password.py b/openslides_backend/action/actions/user/set_password.py index 6b856c4d6..41ceaeb08 100644 --- a/openslides_backend/action/actions/user/set_password.py +++ b/openslides_backend/action/actions/user/set_password.py @@ -11,6 +11,7 @@ from .password_mixins import ClearSessionsMixin, SetPasswordMixin +# TODO auch ermöglichen @register_action("user.set_password") class UserSetPasswordAction( SetPasswordMixin, diff --git a/openslides_backend/shared/mixins/user_scope_mixin.py b/openslides_backend/shared/mixins/user_scope_mixin.py index 543655484..087cf6749 100644 --- a/openslides_backend/shared/mixins/user_scope_mixin.py +++ b/openslides_backend/shared/mixins/user_scope_mixin.py @@ -48,7 +48,6 @@ def get_user_scope( set(id_or_instance.get("committee_management_ids", [])) ) oml_right = id_or_instance.get("organization_management_level", "") - meetingcheck = self.check_for_admin_in_all_meetings(id_or_instance.get("id")) else: user = self.datastore.get( fqid_from_collection_and_id("user", id_or_instance), @@ -61,7 +60,6 @@ def get_user_scope( meetings.update(user.get("meeting_ids", [])) committees_manager.update(set(user.get("committee_management_ids") or [])) oml_right = user.get("organization_management_level", "") - meetingcheck = self.check_for_admin_in_all_meetings(id_or_instance) result = self.datastore.get_many( [ GetManyRequest( @@ -78,7 +76,7 @@ def get_user_scope( if meeting_data.get("is_active_in_organization_id") } committees = committees_manager | set(meetings_committee.values()) - if len(meetings_committee) == 1 and len(committees) == 1 or meetingcheck: + if len(meetings_committee) == 1 and len(committees) == 1: return ( UserScope.Meeting, next(iter(meetings_committee)), @@ -96,7 +94,7 @@ def get_user_scope( def check_permissions_for_scope( self, - id: int, + instance_id: int, always_check_user_oml: bool = True, meeting_permission: Permission = Permissions.User.CAN_MANAGE, ) -> None: @@ -107,7 +105,7 @@ def check_permissions_for_scope( Reason: A user with OML-level-permission has scope "meeting" or "committee" if he belongs to only 1 meeting or 1 committee. """ - scope, scope_id, user_oml, committees = self.get_user_scope(id) + scope, scope_id, user_oml, committees = self.get_user_scope(instance_id) if ( always_check_user_oml and user_oml @@ -165,8 +163,10 @@ def check_permissions_for_scope( committees, ): return - raise MissingPermission({OrganizationManagementLevel.CAN_MANAGE_USERS: 1}) - + if not self.check_for_admin_in_all_meetings(instance_id): + raise MissingPermission( + {OrganizationManagementLevel.CAN_MANAGE_USERS: 1} + ) def check_for_admin_in_all_meetings(self, instance_id: int) -> bool: """ @@ -186,7 +186,17 @@ def check_for_admin_in_all_meetings(self, instance_id: int) -> bool: b_meeting_ids = set(b_user.get("meeting_ids", [])) if not b_meeting_ids: return False - a_meeting_ids = self.permstore.user_meetings + if hasattr(self, "permstore"): + a_meeting_ids = self.permstore.user_meetings + else: + a_user = self.datastore.get( + fqid_from_collection_and_id("user", instance_id), + ["meeting_ids"], + lock_result=False, + ) + a_meeting_ids = set(a_user.get("meeting_ids", [])) + if not a_meeting_ids: + return False intersection_meeting_ids = a_meeting_ids.intersection(b_meeting_ids) if not b_meeting_ids.issubset(intersection_meeting_ids): return False @@ -202,37 +212,41 @@ def check_for_admin_in_all_meetings(self, instance_id: int) -> bool: ).get("meeting", {}) for meeting_id, meeting_dict in intersection_meetings.items(): # get meetings admins - admin_group = self.datastore.get( - fqid_from_collection_and_id( - "group", meeting_dict.get("admin_group_id", 0) - ), - ["meeting_user_ids"], - lock_result=False, - ) - admin_meeting_users = self.datastore.get_many( - [ - GetManyRequest( - "meeting_user", - admin_group.get("meeting_user_ids", []), - ["user_id"], - ) - ], - lock_result=False, - ).get("meeting_user", {}) - # if instance/requested user is a meeting admin in this meeting. - if [ - admin_meeting_user - for admin_meeting_user in admin_meeting_users.values() - if admin_meeting_user.get("user_id") == instance_id - ] != []: + if admin_group_id := meeting_dict.get("admin_group_id"): + admin_group = self.datastore.get( + fqid_from_collection_and_id("group", admin_group_id), + ["meeting_user_ids"], + lock_result=False, + ) + admin_meeting_users = self.datastore.get_many( + [ + GetManyRequest( + "meeting_user", + admin_group.get("meeting_user_ids", []), + ["user_id"], + ) + ], + lock_result=False, + ).get("meeting_user", {}) + else: return False - # if requesting user is not a meeting admin in this meeting. - if not next( - iter( + # if instance/requested user is a meeting admin in this meeting. + if admin_meeting_users: + if [ admin_meeting_user for admin_meeting_user in admin_meeting_users.values() - if admin_meeting_user.get("user_id") == self.user_id - ) - ): + if admin_meeting_user.get("user_id") == instance_id + ] != []: + return False + # if requesting user is not a meeting admin in this meeting. + if not next( + iter( + admin_meeting_user + for admin_meeting_user in admin_meeting_users.values() + if admin_meeting_user.get("user_id") == self.user_id + ) + ): + return False + else: return False - return True \ No newline at end of file + return True diff --git a/tests/system/action/user/test_set_password.py b/tests/system/action/user/test_set_password.py index 0ab5e700a..6a23a97c8 100644 --- a/tests/system/action/user/test_set_password.py +++ b/tests/system/action/user/test_set_password.py @@ -23,6 +23,52 @@ def test_update_correct(self) -> None: self.assert_history_information("user/2", ["Password changed"]) self.assert_logged_in() + def test_two_meetings(self) -> None: + self.create_meeting() + self.create_meeting(4) # meeting 4 + user_id = self.create_user("test", group_ids=[1]) + self.login(user_id) + self.create_model("user/111", {"password": "old_pw"}) + # Admin groups of meeting/1 for test user meeting/2 as normal user + self.set_user_groups(user_id, [2, 4]) + # 111 into both meetings + self.set_user_groups(111, [1, 4]) + response = self.request( + "user.set_password", {"id": 111, "password": self.PASSWORD} + ) + self.assert_status_code(response, 403) + self.assertIn( + "You are not allowed to perform action user.set_password. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + response.json["message"], + ) + model = self.get_model("user/111") + assert "old_pw" == model.get("password", "") + # Admin groups of meeting/1 for test user + self.set_user_groups(user_id, [2]) + # 111 into both meetings + self.set_user_groups(111, [1, 4]) + response = self.request( + "user.set_password", {"id": 111, "password": self.PASSWORD} + ) + self.assert_status_code(response, 403) + self.assertIn( + "You are not allowed to perform action user.set_password. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + response.json["message"], + ) + model = self.get_model("user/111") + assert "old_pw" == model.get("password", "") + # Admin groups of meeting/1 and meeting/4 for test user + self.set_user_groups(user_id, [2, 5]) + # 111 into both meetings + self.set_user_groups(111, [1, 4]) + response = self.request( + "user.set_password", {"id": 111, "password": self.PASSWORD} + ) + self.assert_status_code(response, 200) + model = self.get_model("user/111") + assert self.auth.is_equal(self.PASSWORD, model.get("password", "")) + self.assert_history_information("user/111", ["Password changed"]) + def test_update_correct_default_case(self) -> None: self.update_model("user/1", {"password": "old_pw"}) response = self.request( diff --git a/tests/system/action/user/test_update.py b/tests/system/action/user/test_update.py index 91446f386..ada0da675 100644 --- a/tests/system/action/user/test_update.py +++ b/tests/system/action/user/test_update.py @@ -17,8 +17,10 @@ def permission_setup(self) -> None: } ) - def two_meetings_standard_fails(self, committee_id: None | int = None, group_B_success: bool = False) -> None: - #test group A + def two_meetings_standard_fails( + self, committee_id: None | int = None, group_B_success: bool = False + ) -> None: + # test group A response = self.request( "user.update", { @@ -37,12 +39,12 @@ def two_meetings_standard_fails(self, committee_id: None | int = None, group_B_s "pronoun": None, }, ) - #test group D + # test group D response = self.request( "user.update", { "id": 111, - "committee_management_ids": [1,2], + "committee_management_ids": [1, 2], }, ) self.assert_status_code(response, 403) @@ -68,7 +70,7 @@ def two_meetings_standard_fails(self, committee_id: None | int = None, group_B_s "committee_management_ids": None, }, ) - #test group E + # test group E response = self.request( "user.update", { @@ -87,7 +89,7 @@ def two_meetings_standard_fails(self, committee_id: None | int = None, group_B_s "organization_management_level": None, }, ) - #test group F + # test group F response = self.request( "user.update", { @@ -106,7 +108,7 @@ def two_meetings_standard_fails(self, committee_id: None | int = None, group_B_s "default_password": None, }, ) - #test group G + # test group G response = self.request( "user.update", { @@ -125,7 +127,7 @@ def two_meetings_standard_fails(self, committee_id: None | int = None, group_B_s "is_demo_user": None, }, ) - #test group H + # test group H response = self.request( "user.update", { @@ -995,7 +997,7 @@ def test_perm_group_A_belongs_to_same_meetings(self) -> None: # Admin groups of meeting/1 and meeting/4 for test user self.set_user_groups(self.user_id, [2, 5]) # 111 into both meetings - self.set_user_groups(111, [1, 4]) + self.set_user_groups(111, [1, 4]) response = self.request( "user.update", { @@ -1018,16 +1020,12 @@ def test_perm_group_A_belongs_to_same_meetings_one_time_admin(self) -> None: # Admin groups of only meeting/1 for test user self.set_user_groups(self.user_id, [2, 4]) # 111 into both meetings - self.set_user_groups(111, [1, 4]) + self.set_user_groups(111, [1, 4]) self.two_meetings_standard_fails() - #test group B and C + # test group B and C response = self.request( "user.update", - { - "id": 111, - "number": "I'm not gonna get updated.", - "meeting_id": 4 - }, + {"id": 111, "number": "I'm not gonna get updated.", "meeting_id": 4}, ) self.assert_status_code(response, 403) self.assertIn( @@ -1048,16 +1046,12 @@ def test_perm_group_A_belongs_to_same_meetings_both_admin(self) -> None: # Admin groups of meeting/1 and meeting/4 for test user self.set_user_groups(self.user_id, [2, 5]) # 111 into both meetings (one admin group) - self.set_user_groups(111, [1, 5]) + self.set_user_groups(111, [1, 5]) self.two_meetings_standard_fails() - #test group B and C + # test group B and C response = self.request( "user.update", - { - "id": 111, - "number": "I'm not gonna get updated.", - "meeting_id": 4 - }, + {"id": 111, "number": "I'm not gonna get updated.", "meeting_id": 4}, ) self.assert_status_code(response, 200) self.assert_model_exists( @@ -1094,19 +1088,15 @@ def test_perm_group_A_belongs_to_same_meetings_committee_admin(self) -> None: # Admin groups of meeting/1 and meeting/4 for test user self.set_user_groups(self.user_id, [2, 5]) # 111 into both meetings - self.set_user_groups(111, [1, 4]) + self.set_user_groups(111, [1, 4]) # 111 is committee admin committee_id = 60 - self.set_committee_management_level([committee_id], 111) + self.set_committee_management_level([committee_id], 111) self.two_meetings_standard_fails(committee_id) - #test group B and C + # test group B and C response = self.request( "user.update", - { - "id": 111, - "number": "I'm not gonna get updated.", - "meeting_id": 4 - }, + {"id": 111, "number": "I'm not gonna get updated.", "meeting_id": 4}, ) self.assert_status_code(response, 200) self.assert_model_exists( diff --git a/tests/system/presenter/test_get_user_related_models.py b/tests/system/presenter/test_get_user_related_models.py index b4c884647..7a5425e0a 100644 --- a/tests/system/presenter/test_get_user_related_models.py +++ b/tests/system/presenter/test_get_user_related_models.py @@ -1,3 +1,4 @@ +from collections import defaultdict from typing import Any, cast from openslides_backend.permissions.management_levels import OrganizationManagementLevel @@ -137,6 +138,49 @@ def test_get_user_related_models_meeting(self) -> None: } } + def test_two_meetings(self) -> None: + user_id = 2 + self.set_models( + { + f"user/{user_id}": { + "username": "executor", + "default_password": "DEFAULT_PASSWORD", + "password": self.auth.hash("DEFAULT_PASSWORD"), + "is_active": True, + "meeting_ids": [1, 4], + }, + f"user/{111}": {"username": "untouchable", "meeting_ids": [1, 4]}, + } + ) + self.create_meeting_for_two_users(user_id, 111) + self.create_meeting_for_two_users(user_id, 111, 4) # meeting 4 + self.login(user_id) + # Admin groups of meeting/1 for executor user meeting/2 as normal user + # 111 into both meetings + meeting_user_to_group = {12: 2, 42: 4, 1111: 1, 4111: 4} + self.move_user_from_to_group(meeting_user_to_group) + status_code, data = self.request("get_user_related_models", {"user_ids": [111]}) + self.assertEqual(status_code, 403) + self.assertEqual( + "Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + data["message"], + ) + # Admin groups of meeting/1 for executor user + # 111 into both meetings + self.move_user_from_to_group({12: 2, 42: None, 1111: 1, 4111: 4}) + status_code, data = self.request("get_user_related_models", {"user_ids": [111]}) + self.assertEqual(status_code, 403) + self.assertEqual( + "Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + data["message"], + ) + # Admin groups of meeting/1 and meeting/4 for executor user + # 111 into both meetings + meeting_user_to_group = {12: 2, 42: 5, 1111: 1, 4111: 4} + self.move_user_from_to_group(meeting_user_to_group) + status_code, data = self.request("get_user_related_models", {"user_ids": [111]}) + self.assertEqual(status_code, 200) + def test_get_user_related_models_meetings_more_users(self) -> None: self.set_models( { @@ -559,3 +603,72 @@ def test_get_user_related_models_with_locked_meetings(self) -> None: ] }, } + + def create_meeting_for_two_users( + self, user1: int, user2: int, base: int = 1 + ) -> None: + """ + Creates meeting with id 1, committee 60 and groups with ids 1, 2, 3 by default. + With base you can setup other meetings, but be cautious because of group-ids + The groups have no permissions and no users by default. + Uses usernumber to create meetingusers with the concatenation of base and usernumber. + """ + committee_id = base + 59 + self.set_models( + { + f"meeting/{base}": { + "group_ids": [base, base + 1, base + 2], + "default_group_id": base, + "admin_group_id": base + 1, + "committee_id": committee_id, + "is_active_in_organization_id": 1, + }, + f"group/{base}": { + "meeting_id": base, + "default_group_for_meeting_id": base, + "name": f"group{base}", + }, + f"group/{base+1}": { + "meeting_id": base, + "admin_group_for_meeting_id": base, + "name": f"group{base+1}", + }, + f"group/{base+2}": { + "meeting_id": base, + "name": f"group{base+2}", + }, + f"committee/{committee_id}": { + "organization_id": 1, + "name": f"Commitee{committee_id}", + "meeting_ids": [base], + }, + "organization/1": { + "limit_of_meetings": 0, + "active_meeting_ids": [base], + "enable_electronic_voting": True, + }, + f"meeting_user/{base}{user1}": {"user_id": user1, "meeting_id": base}, + f"meeting_user/{base}{user2}": {"user_id": user2, "meeting_id": base}, + } + ) + + def move_user_from_to_group(self, meeting_user_to_groups: dict[int, Any]) -> None: + """ + Sets the users groups, returns the meeting_user_ids + Be careful as it does not reset previeously set groups if they are not in the data set. + """ + groups_to_meeting_user = defaultdict(list) + for meeting_user_id, group_id in meeting_user_to_groups.items(): + if group_id: + self.update_model( + f"meeting_user/{meeting_user_id}", {"group_ids": [group_id]} + ) + groups_to_meeting_user[group_id].append(meeting_user_id) + else: + self.update_model( + f"meeting_user/{meeting_user_id}", {"group_ids": None} + ) + for group_id, meeting_user_ids in groups_to_meeting_user.items(): + self.update_model( + f"group/{group_id}", {"meeting_user_ids": meeting_user_ids} + ) From 15a53656bc94b0d7310aeb9afb2d1b0d486b194b Mon Sep 17 00:00:00 2001 From: Hannes Janott Date: Fri, 30 Aug 2024 19:35:08 +0200 Subject: [PATCH 07/19] better if else structure. also search through groups other than admin group for set permissions Return meeting ids in get_user_scope. Decluttering check for method a little --- docs/presenters/get_user_scope.md | 9 ++- .../user/create_update_permissions_mixin.py | 65 +++++++++++----- .../action/actions/user/set_password.py | 1 - .../presenter/get_user_scope.py | 5 +- .../shared/mixins/user_scope_mixin.py | 76 ++++++++++++++----- 5 files changed, 114 insertions(+), 42 deletions(-) diff --git a/docs/presenters/get_user_scope.md b/docs/presenters/get_user_scope.md index 928d8fc25..8b0919fde 100644 --- a/docs/presenters/get_user_scope.md +++ b/docs/presenters/get_user_scope.md @@ -1,6 +1,6 @@ ## Payload -```js +``` { user_ids: Id[]; } @@ -8,14 +8,15 @@ ## Returns -```js +``` { user_id: Id: { collection: String, # one of "meeting", "committee" or "organization" id: Id, user_oml: String, # one of "superadmin", "can_manage_organization", "can_manage_users", "" - committee_ids: int[] // Ids of all committees the user is part of - } + committee_ids: Id[] // Ids of all committees the user is part of + }, + ... } ``` diff --git a/openslides_backend/action/actions/user/create_update_permissions_mixin.py b/openslides_backend/action/actions/user/create_update_permissions_mixin.py index df430eb50..c8cd73449 100644 --- a/openslides_backend/action/actions/user/create_update_permissions_mixin.py +++ b/openslides_backend/action/actions/user/create_update_permissions_mixin.py @@ -11,6 +11,7 @@ OrganizationManagementLevel, ) from openslides_backend.permissions.permissions import Permissions, permission_parents +from openslides_backend.permissions.permission_helper import has_committee_management_level from openslides_backend.services.datastore.commands import GetManyRequest from openslides_backend.services.datastore.interface import DatastoreService from openslides_backend.shared.exceptions import ( @@ -229,12 +230,12 @@ def check_permissions(self, instance: dict[str, Any]) -> None: ) actual_group_fields = self._get_actual_grouping_from_instance(instance) - # store scope, scope id, OML-permission and committee ids for requested user + # store scope, scope id, OML-permission and committee ids including the the respective meetings for requested user ( self.instance_user_scope, self.instance_user_scope_id, self.instance_user_oml_permission, - self.instance_committee_ids, + self.instance_committee_meeting_ids, ) = self.get_user_scope(instance.get("id") or instance) if self.permstore.user_oml != OrganizationManagementLevel.SUPERADMIN: @@ -262,28 +263,50 @@ def check_permissions(self, instance: dict[str, Any]) -> None: def check_group_A(self, fields: list[str], instance: dict[str, Any]) -> None: """Check Group A: Depending on scope of user to act on""" if ( - self.permstore.user_oml == OrganizationManagementLevel.SUPERADMIN - or not fields + not fields or self.permstore.user_oml >= OrganizationManagementLevel.CAN_MANAGE_USERS ): return if self.instance_user_scope == UserScope.Organization: - if self.permstore.user_committees.intersection(self.instance_committee_ids): + if self.permstore.user_committees.intersection( + self.instance_committee_meeting_ids + ) or self.check_for_admin_in_all_meetings(instance.get("id", 0)): + for committee_id in self.instance_committee_meeting_ids: + if has_committee_management_level( + self.datastore, + instance.get("id"), + CommitteeManagementLevel.CAN_MANAGE, + committee_id, + ): + raise MissingPermission({OrganizationManagementLevel.CAN_MANAGE_USERS: 1}) return - elif not self.check_for_admin_in_all_meetings(instance.get("id", 0)): + else: raise MissingPermission( {OrganizationManagementLevel.CAN_MANAGE_USERS: 1} ) - if self.instance_user_scope == UserScope.Committee: - if self.instance_user_scope_id not in self.permstore.user_committees: - if not self.check_for_admin_in_all_meetings(instance.get("id", 0)): - raise MissingPermission( - { - OrganizationManagementLevel.CAN_MANAGE_USERS: 1, - CommitteeManagementLevel.CAN_MANAGE: self.instance_user_scope_id, - } - ) + elif self.instance_user_scope == UserScope.Committee: + if ( + self.instance_user_scope_id in self.permstore.user_committees + or self.check_for_admin_in_all_meetings(instance.get("id", 0)) + ): + for committee_id in self.instance_committee_meeting_ids: + if has_committee_management_level( + self.datastore, + instance.get("id"), + CommitteeManagementLevel.CAN_MANAGE, + committee_id, + ): + raise MissingPermission({OrganizationManagementLevel.CAN_MANAGE_USERS: 1, + CommitteeManagementLevel.CAN_MANAGE: self.instance_user_scope_id,}) + return + else: + raise MissingPermission( + { + OrganizationManagementLevel.CAN_MANAGE_USERS: 1, + CommitteeManagementLevel.CAN_MANAGE: self.instance_user_scope_id, + } + ) elif ( self.instance_user_scope_id not in self.permstore.user_committees_meetings and self.instance_user_scope_id not in self.permstore.user_meetings @@ -382,7 +405,7 @@ def check_group_F(self, fields: list[str], instance: dict[str, Any]) -> None: ) else: if self.permstore.user_committees.intersection( - self.instance_committee_ids + self.instance_committee_meeting_ids ): return expected_oml_permission = OrganizationManagementLevel.CAN_MANAGE_USERS @@ -392,6 +415,14 @@ def check_group_F(self, fields: list[str], instance: dict[str, Any]) -> None: ): raise MissingPermission({expected_oml_permission: 1}) else: + for committee_id in self.instance_committee_meeting_ids: + if has_committee_management_level( + self.datastore, + instance.get("id"), + CommitteeManagementLevel.CAN_MANAGE, + committee_id, + ): + raise MissingPermission({OrganizationManagementLevel.CAN_MANAGE_USERS: 1,}) return else: if self.permstore.user_oml >= OrganizationManagementLevel.CAN_MANAGE_USERS: @@ -574,7 +605,7 @@ def get_failing_fields(self, instance: dict[str, Any]) -> list[str]: self.instance_user_scope, self.instance_user_scope_id, self.instance_user_oml_permission, - self.instance_committee_ids, + self.instance_committee_meeting_ids, ) = self.get_user_scope(instance.get("id") or instance) instance_meeting_id = instance.get("meeting_id") diff --git a/openslides_backend/action/actions/user/set_password.py b/openslides_backend/action/actions/user/set_password.py index 41ceaeb08..6b856c4d6 100644 --- a/openslides_backend/action/actions/user/set_password.py +++ b/openslides_backend/action/actions/user/set_password.py @@ -11,7 +11,6 @@ from .password_mixins import ClearSessionsMixin, SetPasswordMixin -# TODO auch ermöglichen @register_action("user.set_password") class UserSetPasswordAction( SetPasswordMixin, diff --git a/openslides_backend/presenter/get_user_scope.py b/openslides_backend/presenter/get_user_scope.py index 8c07f485a..fa609bad9 100644 --- a/openslides_backend/presenter/get_user_scope.py +++ b/openslides_backend/presenter/get_user_scope.py @@ -36,7 +36,10 @@ def get_result(self) -> Any: result: dict[str, Any] = {} user_ids = self.data["user_ids"] for user_id in user_ids: - scope, scope_id, user_oml, committee_ids = self.get_user_scope(user_id) + scope, scope_id, user_oml, committee_meeting_ids = self.get_user_scope( + user_id + ) + committee_ids = [ci for ci in committee_meeting_ids.keys()] result[str(user_id)] = { "collection": scope, "id": scope_id, diff --git a/openslides_backend/shared/mixins/user_scope_mixin.py b/openslides_backend/shared/mixins/user_scope_mixin.py index 087cf6749..de8b36d61 100644 --- a/openslides_backend/shared/mixins/user_scope_mixin.py +++ b/openslides_backend/shared/mixins/user_scope_mixin.py @@ -1,3 +1,4 @@ +from collections import defaultdict from enum import Enum from typing import Any @@ -15,7 +16,7 @@ ) from ...permissions.permissions import Permission, Permissions from ...services.datastore.interface import GetManyRequest -from ..exceptions import MissingPermission +from ..exceptions import MissingPermission, PermissionDenied from ..patterns import fqid_from_collection_and_id @@ -31,12 +32,14 @@ def __repr__(self) -> str: class UserScopeMixin(BaseServiceProvider): def get_user_scope( self, id_or_instance: int | dict[str, Any] - ) -> tuple[UserScope, int, str, list[int]]: + ) -> tuple[UserScope, int, str, dict[int, Any]]: """ Parameter id_or_instance: id for existing user or instance for user to create Returns the scope of the given user id together with the relevant scope id (either meeting, committee or organization), the OML level of the user as string (empty string if the user has none) and the ids of all committees that the user is either a manager in or a member of. + and their respective meetings the user is part of. #A committe can have no meetings if the + user just has committee management rights and is not part of any of its meetings. """ meetings: set[int] = set() committees_manager: set[int] = set() @@ -76,21 +79,28 @@ def get_user_scope( if meeting_data.get("is_active_in_organization_id") } committees = committees_manager | set(meetings_committee.values()) + committee_meetings: dict[int, Any] = defaultdict(list) + for meeting, committee in meetings_committee.items(): + committee_meetings[committee].append(meeting) + for committee in committees: + if committee not in committee_meetings.keys(): + committee_meetings[committee] = None + if len(meetings_committee) == 1 and len(committees) == 1: return ( UserScope.Meeting, next(iter(meetings_committee)), oml_right, - list(committees), + committee_meetings, ) elif len(committees) == 1: return ( UserScope.Committee, next(iter(committees)), oml_right, - list(committees), + committee_meetings, ) - return UserScope.Organization, 1, oml_right, list(committees) + return UserScope.Organization, 1, oml_right, committee_meetings def check_permissions_for_scope( self, @@ -105,7 +115,9 @@ def check_permissions_for_scope( Reason: A user with OML-level-permission has scope "meeting" or "committee" if he belongs to only 1 meeting or 1 committee. """ - scope, scope_id, user_oml, committees = self.get_user_scope(instance_id) + scope, scope_id, user_oml, committees_to_meetings = self.get_user_scope( + instance_id + ) if ( always_check_user_oml and user_oml @@ -160,30 +172,29 @@ def check_permissions_for_scope( self.datastore, self.user_id, CommitteeManagementLevel.CAN_MANAGE, - committees, + [ci for ci in committees_to_meetings.keys()], ): return - if not self.check_for_admin_in_all_meetings(instance_id): + meeting_ids = set() + for mi in committees_to_meetings.values(): + meeting_ids.add(*mi) + if not self.check_for_admin_in_all_meetings(instance_id, meeting_ids):# raise MissingPermission( {OrganizationManagementLevel.CAN_MANAGE_USERS: 1} ) - def check_for_admin_in_all_meetings(self, instance_id: int) -> bool: + def check_for_admin_in_all_meetings(self, instance_id: int, b_meeting_ids: set[int] = None) -> bool:# """ Checks if the requesting user has permissions to manage participants in all of requested users meetings. Also checks if the requesting user has meeting admin rights and the requested user doesn't. - Returns true if permissions are given. False if not. Raises no Exceptions. + Returns true if permissions are given. False if not. Raises no Exceptions. TODO ooops!!! """ if not instance_id: return False - b_user = self.datastore.get( - fqid_from_collection_and_id("user", instance_id), - ["meeting_ids", "committee_management_ids"], - lock_result=False, - ) - if b_user.get("committee_management_ids"): - return False - b_meeting_ids = set(b_user.get("meeting_ids", [])) + if not b_meeting_ids: + b_meeting_ids = set() + for mi in self.instance_committee_meeting_ids.values(): + b_meeting_ids.add(*mi) if not b_meeting_ids: return False if hasattr(self, "permstore"): @@ -205,13 +216,15 @@ def check_for_admin_in_all_meetings(self, instance_id: int) -> bool: GetManyRequest( "meeting", list(intersection_meeting_ids), - ["meeting_user_ids", "admin_group_id"], + ["admin_group_id", "group_ids", "meeting_user_ids"], ) ], lock_result=False, ).get("meeting", {}) for meeting_id, meeting_dict in intersection_meetings.items(): # get meetings admins + admin_meeting_users = {} + # unnecessary "if" due to admin group always existant? if admin_group_id := meeting_dict.get("admin_group_id"): admin_group = self.datastore.get( fqid_from_collection_and_id("group", admin_group_id), @@ -228,6 +241,31 @@ def check_for_admin_in_all_meetings(self, instance_id: int) -> bool: ], lock_result=False, ).get("meeting_user", {}) + # unnecessary "if" due to default group always existant? + if group_ids := meeting_dict.get("group_ids", []): + groups = self.datastore.get_many( + [GetManyRequest("group", group_ids, ["meeting_user_ids"])], + lock_result=False, + ).get("group", {}) + for group_id, group in groups.items(): + meeting_user_ids = group.get("meeting_user_ids", []) + group_permissions = group.get("permissions", []) + if meeting_user_ids and ( + "user.can_manage" in group_permissions + or "user.can_update" in group_permissions + ): # TODO test mit can manage hier nur can update + admin_meeting_users.update( + self.datastore.get_many( + [ + GetManyRequest( + "meeting_user", + meeting_user_ids, + ["user_id"], + ) + ], + lock_result=False, + ).get("meeting_user", {}) + ) else: return False # if instance/requested user is a meeting admin in this meeting. From 37ba2592c4708a7c3fd92e23eb31bb0b8904078e Mon Sep 17 00:00:00 2001 From: Hannes Janott Date: Wed, 4 Sep 2024 19:06:37 +0200 Subject: [PATCH 08/19] Add get user editable presenter plus test. Moved user create update permission mixin to shared folder deriving from BaseServiceProvider instead of UserMixin and thus Action. --- docs/presenters/get_user_editable.md | 27 +++++ .../action/actions/user/base_import.py | 6 +- .../action/actions/user/create.py | 9 +- .../action/actions/user/participant_common.py | 8 +- .../action/actions/user/update.py | 9 +- .../action/mixins/import_mixins.py | 2 +- openslides_backend/presenter/__init__.py | 1 + .../presenter/get_user_editable.py | 69 ++++++++++++ .../user_create_update_permissions_mixin.py} | 59 ++++++---- .../shared/mixins/user_scope_mixin.py | 21 ++-- .../presenter/test_get_user_editable.py | 101 ++++++++++++++++++ 11 files changed, 275 insertions(+), 37 deletions(-) create mode 100644 docs/presenters/get_user_editable.md create mode 100644 openslides_backend/presenter/get_user_editable.py rename openslides_backend/{action/actions/user/create_update_permissions_mixin.py => shared/mixins/user_create_update_permissions_mixin.py} (93%) create mode 100644 tests/system/presenter/test_get_user_editable.py diff --git a/docs/presenters/get_user_editable.md b/docs/presenters/get_user_editable.md new file mode 100644 index 000000000..79fbb68fc --- /dev/null +++ b/docs/presenters/get_user_editable.md @@ -0,0 +1,27 @@ +## Payload + +``` +{ + user_ids: Id[]; +} +``` + +## Returns + +``` +{ + user_id: Id: { + editable: boolean // true if user can be updated or deleted + message: string // error message if an exception was caught + }, + ... +} +``` + +## Logic + +It iterates over the given `user_ids` and calculates whether a user can be updated depending on permissions in shared committees and meetings, OML and the user-scope. The user scope is defined [here](https://github.com/OpenSlides/OpenSlides/wiki/Users#user-scopes). The payload field permissions are described [here](https://github.com/OpenSlides/openslides-backend/blob/main/docs/actions/user.update.md) and [here](https://github.com/OpenSlides/openslides-backend/blob/main/docs/actions/user.create.md). + +## Permissions + +There are no special permissions necessary. \ No newline at end of file diff --git a/openslides_backend/action/actions/user/base_import.py b/openslides_backend/action/actions/user/base_import.py index 33010bc60..48718107e 100644 --- a/openslides_backend/action/actions/user/base_import.py +++ b/openslides_backend/action/actions/user/base_import.py @@ -24,7 +24,9 @@ def update_instance(self, instance: dict[str, Any]) -> dict[str, Any]: rows = self.flatten_copied_object_fields( self.handle_remove_and_group_fields ) - self.create_other_actions(rows) + self.create_other_actions( + rows + ) # Hier muss doch eigentlich die collection bestimmt werden, oder nicht? return {} @@ -69,7 +71,7 @@ def handle_remove_and_group_fields(self, entry: dict[str, Any]) -> dict[str, Any ): entry.pop("gender") - # remove all fields fields marked with "remove"-state + # remove all fields marked with "remove"-state to_remove = [] for k, v in entry.items(): if isinstance(v, dict): diff --git a/openslides_backend/action/actions/user/create.py b/openslides_backend/action/actions/user/create.py index 090870035..77b3282ad 100644 --- a/openslides_backend/action/actions/user/create.py +++ b/openslides_backend/action/actions/user/create.py @@ -2,6 +2,9 @@ from typing import Any from openslides_backend.permissions.permissions import Permissions +from openslides_backend.shared.mixins.user_create_update_permissions_mixin import ( + CreateUpdatePermissionsMixin, +) from ....models.models import User from ....shared.exceptions import ActionException @@ -15,13 +18,13 @@ from ...util.register import register_action from ...util.typing import ActionResultElement from ..meeting_user.mixin import CheckLockOutPermissionMixin -from .create_update_permissions_mixin import CreateUpdatePermissionsMixin from .password_mixins import SetPasswordMixin from .user_mixins import LimitOfUserMixin, UserMixin, UsernameMixin, check_gender_helper @register_action("user.create") class UserCreate( + UserMixin, EmailCheckMixin, CreateAction, CreateUpdatePermissionsMixin, @@ -67,6 +70,10 @@ class UserCreate( history_information = "Account created" own_history_information_first = True + def check_permissions(self, instance: dict[str, Any]) -> None: + self.assert_not_anonymous() + super().check_permissions(instance) + def update_instance(self, instance: dict[str, Any]) -> dict[str, Any]: self.meeting_id: int | None = instance.get("meeting_id") diff --git a/openslides_backend/action/actions/user/participant_common.py b/openslides_backend/action/actions/user/participant_common.py index 66b5b29f2..752e6e2bc 100644 --- a/openslides_backend/action/actions/user/participant_common.py +++ b/openslides_backend/action/actions/user/participant_common.py @@ -7,14 +7,14 @@ ) from openslides_backend.permissions.permissions import Permissions from openslides_backend.shared.exceptions import MissingPermission +from openslides_backend.shared.mixins.user_create_update_permissions_mixin import ( + CreateUpdatePermissionsFailingFields, + PermissionVarStore, +) from openslides_backend.shared.patterns import fqid_from_collection_and_id from ...mixins.import_mixins import ImportRow, ImportState from ..meeting_user.mixin import CheckLockOutPermissionMixin -from ..user.create_update_permissions_mixin import ( - CreateUpdatePermissionsFailingFields, - PermissionVarStore, -) class ParticipantCommon(BaseImportJsonUploadAction, CheckLockOutPermissionMixin): diff --git a/openslides_backend/action/actions/user/update.py b/openslides_backend/action/actions/user/update.py index 657074188..0de3f40a5 100644 --- a/openslides_backend/action/actions/user/update.py +++ b/openslides_backend/action/actions/user/update.py @@ -2,6 +2,9 @@ from typing import Any from openslides_backend.permissions.permissions import Permissions +from openslides_backend.shared.mixins.user_create_update_permissions_mixin import ( + CreateUpdatePermissionsMixin, +) from ....models.models import User from ....permissions.management_levels import OrganizationManagementLevel @@ -14,7 +17,6 @@ from ...util.register import register_action from ..meeting_user.mixin import CheckLockOutPermissionMixin from .conditional_speaker_cascade_mixin import ConditionalSpeakerCascadeMixin -from .create_update_permissions_mixin import CreateUpdatePermissionsMixin from .user_mixins import ( LimitOfUserMixin, UpdateHistoryMixin, @@ -25,6 +27,7 @@ @register_action("user.update") class UserUpdate( + UserMixin, EmailCheckMixin, CreateUpdatePermissionsMixin, UpdateAction, @@ -76,6 +79,10 @@ class UserUpdate( permission = Permissions.User.CAN_UPDATE check_email_field = "email" + def check_permissions(self, instance: dict[str, Any]) -> None: + self.assert_not_anonymous() + super().check_permissions(instance) + def validate_instance(self, instance: dict[str, Any]) -> None: super().validate_instance(instance) if not self.internal and any( diff --git a/openslides_backend/action/mixins/import_mixins.py b/openslides_backend/action/mixins/import_mixins.py index 87df99e14..dfe4e68b7 100644 --- a/openslides_backend/action/mixins/import_mixins.py +++ b/openslides_backend/action/mixins/import_mixins.py @@ -317,7 +317,7 @@ def flatten_copied_object_fields( ) -> list[ImportRow]: """The self.rows will be deepcopied, flattened and returned, without changes on the self.rows. - This is necessary for using the data in the executution of actions. + This is necessary for using the data in the execution of actions. The requests response should be given with the unchanged self.rows. Parameter: hook_method: diff --git a/openslides_backend/presenter/__init__.py b/openslides_backend/presenter/__init__.py index 23c6e99ed..6dff4e598 100644 --- a/openslides_backend/presenter/__init__.py +++ b/openslides_backend/presenter/__init__.py @@ -6,6 +6,7 @@ get_forwarding_committees, get_forwarding_meetings, get_history_information, + get_user_editable, get_user_related_models, get_user_scope, get_users, diff --git a/openslides_backend/presenter/get_user_editable.py b/openslides_backend/presenter/get_user_editable.py new file mode 100644 index 000000000..f4bf38db1 --- /dev/null +++ b/openslides_backend/presenter/get_user_editable.py @@ -0,0 +1,69 @@ +from typing import Any + +import fastjsonschema + +from openslides_backend.permissions.permissions import Permissions +from openslides_backend.shared.exceptions import MissingPermission, PermissionDenied +# from openslides_backend.action.actions.user.create_update_permissions_mixin import CreateUpdatePermissionsMixin +from openslides_backend.shared.mixins.user_create_update_permissions_mixin import ( + CreateUpdatePermissionsMixin, +) +from openslides_backend.shared.schema import id_list_schema + +from ..shared.patterns import fqid_from_collection_and_id +from ..shared.schema import schema_version +from .base import BasePresenter +from .presenter import register_presenter + +get_user_editable_schema = fastjsonschema.compile( + { + "$schema": schema_version, + "type": "object", + "title": "get_user_editable", + "description": "get user editable", + "properties": { + "user_ids": id_list_schema, + }, + "required": ["user_ids"], + "additionalProperties": False, + } +) + + +@register_presenter("get_user_editable") +class GetUserEditable(CreateUpdatePermissionsMixin, BasePresenter): + """ + Checks for each user whether it is editable by calling user. + """ + + schema = get_user_editable_schema + permission = Permissions.User.CAN_MANAGE + + def get_result(self) -> Any: + result: dict[str, Any] = {} + user_ids = self.data["user_ids"] + # result["message"] = "" + for user_id in user_ids: + instance = self.datastore.get( + fqid_from_collection_and_id("user", user_id), + [ + "id", + "meeting_user_ids", + "meeting_id", + "committee_management_ids", + "organization_management_level", + ], + lock_result=False, + ) + result[str(user_id)] = {} + try: + if self.check_permissions(instance): + result[str(user_id)]["editable"] = True + else: + result[str(user_id)]["editable"] = False + except (PermissionDenied, MissingPermission) as e: + result[str(user_id)]["editable"] = False + result[str(user_id)]["message"] = e.message + + # result["message"] += e.message + return result diff --git a/openslides_backend/action/actions/user/create_update_permissions_mixin.py b/openslides_backend/shared/mixins/user_create_update_permissions_mixin.py similarity index 93% rename from openslides_backend/action/actions/user/create_update_permissions_mixin.py rename to openslides_backend/shared/mixins/user_create_update_permissions_mixin.py index c8cd73449..af2c6e53f 100644 --- a/openslides_backend/action/actions/user/create_update_permissions_mixin.py +++ b/openslides_backend/shared/mixins/user_create_update_permissions_mixin.py @@ -3,17 +3,19 @@ from functools import reduce from typing import Any, cast -from openslides_backend.action.action import Action from openslides_backend.action.relations.relation_manager import RelationManager from openslides_backend.permissions.base_classes import Permission from openslides_backend.permissions.management_levels import ( CommitteeManagementLevel, OrganizationManagementLevel, ) +from openslides_backend.permissions.permission_helper import ( + has_committee_management_level, +) from openslides_backend.permissions.permissions import Permissions, permission_parents -from openslides_backend.permissions.permission_helper import has_committee_management_level from openslides_backend.services.datastore.commands import GetManyRequest from openslides_backend.services.datastore.interface import DatastoreService +from openslides_backend.shared.base_service_provider import BaseServiceProvider from openslides_backend.shared.exceptions import ( ActionException, MissingPermission, @@ -25,8 +27,6 @@ from openslides_backend.shared.mixins.user_scope_mixin import UserScope, UserScopeMixin from openslides_backend.shared.patterns import fqid_from_collection_and_id -from .user_mixins import UserMixin - class PermissionVarStore: permission: Permission @@ -172,9 +172,11 @@ def _get_user_meetings_with_permission( return user_meetings -class CreateUpdatePermissionsMixin(UserMixin, UserScopeMixin, Action): +class CreateUpdatePermissionsMixin(UserScopeMixin, BaseServiceProvider): permstore: PermissionVarStore permission: Permission + name: str + internal: bool field_rights: dict[str, list] = { "A": [ @@ -211,15 +213,17 @@ class CreateUpdatePermissionsMixin(UserMixin, UserScopeMixin, Action): "H": ["saml_id"], } - def check_permissions(self, instance: dict[str, Any]) -> None: + def check_permissions(self, instance: dict[str, Any]) -> bool | None: """ Checks the permissions on a per field and user.scope base, details see https://github.com/OpenSlides/OpenSlides/wiki/Users https://github.com/OpenSlides/OpenSlides/wiki/Permission-System https://github.com/OpenSlides/OpenSlides/wiki/Restrictions-Overview The fields groups and their necessary permissions are also documented there. + Returns true if permissions are given. """ - self.assert_not_anonymous() + if "meta_position" in instance: # TODO still needed with all the changes? + instance.pop("meta_position") if "forwarding_committee_ids" in instance: raise PermissionDenied("forwarding_committee_ids is not allowed.") @@ -259,6 +263,7 @@ def check_permissions(self, instance: dict[str, Any]) -> None: self.check_group_A(actual_group_fields["A"], instance) self.check_group_F(actual_group_fields["F"], instance) self.check_group_G(actual_group_fields["G"]) + return True def check_group_A(self, fields: list[str], instance: dict[str, Any]) -> None: """Check Group A: Depending on scope of user to act on""" @@ -273,13 +278,17 @@ def check_group_A(self, fields: list[str], instance: dict[str, Any]) -> None: self.instance_committee_meeting_ids ) or self.check_for_admin_in_all_meetings(instance.get("id", 0)): for committee_id in self.instance_committee_meeting_ids: - if has_committee_management_level( + if ( + instance_id := instance.get("id") + ) and has_committee_management_level( self.datastore, - instance.get("id"), + instance_id, CommitteeManagementLevel.CAN_MANAGE, committee_id, ): - raise MissingPermission({OrganizationManagementLevel.CAN_MANAGE_USERS: 1}) + raise MissingPermission( + {OrganizationManagementLevel.CAN_MANAGE_USERS: 1} + ) return else: raise MissingPermission( @@ -291,14 +300,20 @@ def check_group_A(self, fields: list[str], instance: dict[str, Any]) -> None: or self.check_for_admin_in_all_meetings(instance.get("id", 0)) ): for committee_id in self.instance_committee_meeting_ids: - if has_committee_management_level( + if ( + instance_id := instance.get("id") + ) and has_committee_management_level( self.datastore, - instance.get("id"), + instance_id, CommitteeManagementLevel.CAN_MANAGE, committee_id, ): - raise MissingPermission({OrganizationManagementLevel.CAN_MANAGE_USERS: 1, - CommitteeManagementLevel.CAN_MANAGE: self.instance_user_scope_id,}) + raise MissingPermission( + { + OrganizationManagementLevel.CAN_MANAGE_USERS: 1, + CommitteeManagementLevel.CAN_MANAGE: self.instance_user_scope_id, + } + ) return else: raise MissingPermission( @@ -416,13 +431,19 @@ def check_group_F(self, fields: list[str], instance: dict[str, Any]) -> None: raise MissingPermission({expected_oml_permission: 1}) else: for committee_id in self.instance_committee_meeting_ids: - if has_committee_management_level( + if ( + instance_id := instance.get("id") + ) and has_committee_management_level( self.datastore, - instance.get("id"), + instance_id, CommitteeManagementLevel.CAN_MANAGE, committee_id, ): - raise MissingPermission({OrganizationManagementLevel.CAN_MANAGE_USERS: 1,}) + raise MissingPermission( + { + OrganizationManagementLevel.CAN_MANAGE_USERS: 1, + } + ) return else: if self.permstore.user_oml >= OrganizationManagementLevel.CAN_MANAGE_USERS: @@ -576,11 +597,7 @@ def __init__( super().__init__( services, datastore, - relation_manager, logging, - env, - skip_archived_meeting_check, - use_meeting_ids_for_archived_meeting_check, ) def get_failing_fields(self, instance: dict[str, Any]) -> list[str]: diff --git a/openslides_backend/shared/mixins/user_scope_mixin.py b/openslides_backend/shared/mixins/user_scope_mixin.py index de8b36d61..9f329973d 100644 --- a/openslides_backend/shared/mixins/user_scope_mixin.py +++ b/openslides_backend/shared/mixins/user_scope_mixin.py @@ -16,7 +16,7 @@ ) from ...permissions.permissions import Permission, Permissions from ...services.datastore.interface import GetManyRequest -from ..exceptions import MissingPermission, PermissionDenied +from ..exceptions import MissingPermission from ..patterns import fqid_from_collection_and_id @@ -30,6 +30,8 @@ def __repr__(self) -> str: class UserScopeMixin(BaseServiceProvider): + instance_committee_meeting_ids: dict + def get_user_scope( self, id_or_instance: int | dict[str, Any] ) -> tuple[UserScope, int, str, dict[int, Any]]: @@ -59,6 +61,7 @@ def get_user_scope( "organization_management_level", "committee_management_ids", ], + lock_result=False, ) meetings.update(user.get("meeting_ids", [])) committees_manager.update(set(user.get("committee_management_ids") or [])) @@ -175,26 +178,30 @@ def check_permissions_for_scope( [ci for ci in committees_to_meetings.keys()], ): return - meeting_ids = set() + meeting_ids: set = set() for mi in committees_to_meetings.values(): meeting_ids.add(*mi) - if not self.check_for_admin_in_all_meetings(instance_id, meeting_ids):# + if not self.check_for_admin_in_all_meetings(instance_id, meeting_ids): raise MissingPermission( {OrganizationManagementLevel.CAN_MANAGE_USERS: 1} ) - def check_for_admin_in_all_meetings(self, instance_id: int, b_meeting_ids: set[int] = None) -> bool:# + def check_for_admin_in_all_meetings( + self, instance_id: int, b_meeting_ids: set[int] | None = None + ) -> bool: """ Checks if the requesting user has permissions to manage participants in all of requested users meetings. Also checks if the requesting user has meeting admin rights and the requested user doesn't. - Returns true if permissions are given. False if not. Raises no Exceptions. TODO ooops!!! + Returns true if permissions are given. False if not. Raises no Exceptions. """ if not instance_id: return False if not b_meeting_ids: + if not hasattr(self, "instance_committee_meeting_ids"): + return False b_meeting_ids = set() - for mi in self.instance_committee_meeting_ids.values(): - b_meeting_ids.add(*mi) + for m_ids in self.instance_committee_meeting_ids.values(): + b_meeting_ids.update(m_ids) if not b_meeting_ids: return False if hasattr(self, "permstore"): diff --git a/tests/system/presenter/test_get_user_editable.py b/tests/system/presenter/test_get_user_editable.py new file mode 100644 index 000000000..7a68b6f14 --- /dev/null +++ b/tests/system/presenter/test_get_user_editable.py @@ -0,0 +1,101 @@ +from openslides_backend.permissions.management_levels import OrganizationManagementLevel + +from .base import BasePresenterTestCase + + +class TestGetUSerEditable(BasePresenterTestCase): + def test_good(self) -> None: + self.create_model( + "user/111", + { + "username": "Helmhut", + "last_name": "Schmidt", + "is_active": True, + "password": self.auth.hash("Kohl"), + "default_password": "Kohl", + "committee_management_ids": [2] + } + ) + self.login(111) + self.set_models( + { + "meeting/1": { + "committee_id": 2, + "is_active_in_organization_id": 1, + }, + # archived meeting + "meeting/2": { + "committee_id": 2, + "is_active_in_organization_id": None, + "is_archived_in_organization_id": 1, + }, + "committee/1": {}, + "committee/2": {"meeting_ids": [1, 2]}, + "user/2": { + "username": "only_oml_level", + "organization_management_level": OrganizationManagementLevel.CAN_MANAGE_USERS, + }, + "user/3": { + "username": "only_cml_level", + "committee_management_ids": [1], + "meeting_ids": [], + }, + "user/4": { + "username": "cml_and_meeting", + "meeting_ids": [1], + "committee_management_ids": [2], + }, + "user/5": { + "username": "no_organization", + "meeting_ids": [], + }, + "user/6": { + "username": "oml_and_meeting", + "organization_management_level": OrganizationManagementLevel.SUPERADMIN, + "meeting_ids": [1], + }, + "user/7": { + "username": "meeting_and_archived_meeting", + "meeting_ids": [1, 2], + }, + } + ) + status_code, data = self.request( + "get_user_editable", {"user_ids": [2, 3, 4, 5, 6, 7]} + ) + self.assertEqual(status_code, 200) + self.assertEqual( + data, + { + "2": { + "editable": False, + "message": 'Your organization management level is not high enough to change a user with a Level of can_manage_users!' + }, + "3": { + "editable": True, + }, + "4": { + "editable": True, + }, + "5": { + "editable": True, + }, + "6": { + "editable": False, + "message": 'Your organization management level is not high enough to change a user with a Level of superadmin!' + }, + "7": { + "editable": True, + }, + }, + ) + + def test_without_user_None(self) -> None: + status_code, data = self.request("get_user_editable", {"user_ids": [None]}) + self.assertEqual(status_code, 400) + self.assertIn("data.user_ids[0] must be integer", data["message"]) + + def test_without_user_empty_list(self) -> None: + status_code, data = self.request("get_user_editable", {"user_ids": []}) + self.assertEqual(status_code, 200) + assert data == {} From 7c242893d86ea2ba7b6699b39d3b1091dc2a0a44 Mon Sep 17 00:00:00 2001 From: Hannes Janott Date: Mon, 9 Sep 2024 18:26:02 +0200 Subject: [PATCH 09/19] Extend get user editable with payload field names to support all payload field groups. Use user.can_update and user.can_manage. More tests. Create meeting for two users method in base presenter. --- docs/presenters/get_user_editable.md | 3 +- openslides_backend/presenter/base.py | 1 + .../presenter/get_user_editable.py | 36 ++- .../user_create_update_permissions_mixin.py | 2 +- .../shared/mixins/user_scope_mixin.py | 48 ++-- tests/system/action/user/test_update.py | 78 +++++- tests/system/presenter/base.py | 80 ++++++ .../presenter/test_get_user_editable.py | 256 +++++++++++++++++- .../presenter/test_get_user_related_models.py | 76 +----- 9 files changed, 447 insertions(+), 133 deletions(-) diff --git a/docs/presenters/get_user_editable.md b/docs/presenters/get_user_editable.md index 79fbb68fc..c30e8f41a 100644 --- a/docs/presenters/get_user_editable.md +++ b/docs/presenters/get_user_editable.md @@ -3,6 +3,7 @@ ``` { user_ids: Id[]; + fields: string[] } ``` @@ -20,7 +21,7 @@ ## Logic -It iterates over the given `user_ids` and calculates whether a user can be updated depending on permissions in shared committees and meetings, OML and the user-scope. The user scope is defined [here](https://github.com/OpenSlides/OpenSlides/wiki/Users#user-scopes). The payload field permissions are described [here](https://github.com/OpenSlides/openslides-backend/blob/main/docs/actions/user.update.md) and [here](https://github.com/OpenSlides/openslides-backend/blob/main/docs/actions/user.create.md). +It iterates over the given `user_ids` and calculates whether a user can be updated depending on the given payload fields, permissions in shared committees and meetings, OML and the user-scope. The user scope is defined [here](https://github.com/OpenSlides/OpenSlides/wiki/Users#user-scopes). The payload field permissions are described [here](https://github.com/OpenSlides/openslides-backend/blob/main/docs/actions/user.update.md) and [here](https://github.com/OpenSlides/openslides-backend/blob/main/docs/actions/user.create.md). ## Permissions diff --git a/openslides_backend/presenter/base.py b/openslides_backend/presenter/base.py index 72e44e971..81a0ff799 100644 --- a/openslides_backend/presenter/base.py +++ b/openslides_backend/presenter/base.py @@ -17,6 +17,7 @@ class BasePresenter(BaseServiceProvider): Base class for presenters. """ + internal: bool = False data: Any schema: Callable[[Any], None] | None = None diff --git a/openslides_backend/presenter/get_user_editable.py b/openslides_backend/presenter/get_user_editable.py index f4bf38db1..96ae93b1c 100644 --- a/openslides_backend/presenter/get_user_editable.py +++ b/openslides_backend/presenter/get_user_editable.py @@ -3,14 +3,17 @@ import fastjsonschema from openslides_backend.permissions.permissions import Permissions -from openslides_backend.shared.exceptions import MissingPermission, PermissionDenied -# from openslides_backend.action.actions.user.create_update_permissions_mixin import CreateUpdatePermissionsMixin +from openslides_backend.shared.exceptions import ( + ActionException, + MissingPermission, + PermissionDenied, + PresenterException, +) from openslides_backend.shared.mixins.user_create_update_permissions_mixin import ( CreateUpdatePermissionsMixin, ) -from openslides_backend.shared.schema import id_list_schema +from openslides_backend.shared.schema import id_list_schema, str_list_schema -from ..shared.patterns import fqid_from_collection_and_id from ..shared.schema import schema_version from .base import BasePresenter from .presenter import register_presenter @@ -23,6 +26,7 @@ "description": "get user editable", "properties": { "user_ids": id_list_schema, + "fields": str_list_schema, }, "required": ["user_ids"], "additionalProperties": False, @@ -37,33 +41,25 @@ class GetUserEditable(CreateUpdatePermissionsMixin, BasePresenter): """ schema = get_user_editable_schema + name = "get_user_editable" permission = Permissions.User.CAN_MANAGE def get_result(self) -> Any: result: dict[str, Any] = {} - user_ids = self.data["user_ids"] - # result["message"] = "" - for user_id in user_ids: - instance = self.datastore.get( - fqid_from_collection_and_id("user", user_id), - [ - "id", - "meeting_user_ids", - "meeting_id", - "committee_management_ids", - "organization_management_level", - ], - lock_result=False, + if not self.data["fields"]: + raise PresenterException( + "Need at least one field name to check editability." ) + instance = {field_name: "" for field_name in self.data["fields"]} + for user_id in self.data["user_ids"]: + instance.update({"id": user_id}) result[str(user_id)] = {} try: if self.check_permissions(instance): result[str(user_id)]["editable"] = True else: result[str(user_id)]["editable"] = False - except (PermissionDenied, MissingPermission) as e: + except (PermissionDenied, MissingPermission, ActionException) as e: result[str(user_id)]["editable"] = False result[str(user_id)]["message"] = e.message - - # result["message"] += e.message return result diff --git a/openslides_backend/shared/mixins/user_create_update_permissions_mixin.py b/openslides_backend/shared/mixins/user_create_update_permissions_mixin.py index af2c6e53f..08eb1e4dd 100644 --- a/openslides_backend/shared/mixins/user_create_update_permissions_mixin.py +++ b/openslides_backend/shared/mixins/user_create_update_permissions_mixin.py @@ -348,7 +348,7 @@ def check_group_B( or locked_from_inside ) and fields: meeting_ids = self._meetings_from_group_B_fields_from_instance(instance) - if diff := meeting_ids - self.permstore.user_meetings: + if (diff := meeting_ids - self.permstore.user_meetings) and diff != {None}: raise MissingPermission( {self.permission: meeting_id for meeting_id in diff} ) diff --git a/openslides_backend/shared/mixins/user_scope_mixin.py b/openslides_backend/shared/mixins/user_scope_mixin.py index 9f329973d..d77576f82 100644 --- a/openslides_backend/shared/mixins/user_scope_mixin.py +++ b/openslides_backend/shared/mixins/user_scope_mixin.py @@ -178,10 +178,12 @@ def check_permissions_for_scope( [ci for ci in committees_to_meetings.keys()], ): return - meeting_ids: set = set() - for mi in committees_to_meetings.values(): - meeting_ids.add(*mi) - if not self.check_for_admin_in_all_meetings(instance_id, meeting_ids): + meeting_ids = set() + for mids in committees_to_meetings.values(): + meeting_ids = {meeting_id for meeting_id in mids} + if not meeting_ids or not self.check_for_admin_in_all_meetings( + instance_id, meeting_ids + ): raise MissingPermission( {OrganizationManagementLevel.CAN_MANAGE_USERS: 1} ) @@ -201,11 +203,15 @@ def check_for_admin_in_all_meetings( return False b_meeting_ids = set() for m_ids in self.instance_committee_meeting_ids.values(): - b_meeting_ids.update(m_ids) + if m_ids: + b_meeting_ids.update(m_ids) if not b_meeting_ids: return False + # During participant import there is no permstore. if hasattr(self, "permstore"): - a_meeting_ids = self.permstore.user_meetings + a_meeting_ids = ( + self.permstore.user_meetings + ) # returns only admin level meetings else: a_user = self.datastore.get( fqid_from_collection_and_id("user", instance_id), @@ -251,7 +257,11 @@ def check_for_admin_in_all_meetings( # unnecessary "if" due to default group always existant? if group_ids := meeting_dict.get("group_ids", []): groups = self.datastore.get_many( - [GetManyRequest("group", group_ids, ["meeting_user_ids"])], + [ + GetManyRequest( + "group", group_ids, ["meeting_user_ids", "permissions"] + ) + ], lock_result=False, ).get("group", {}) for group_id, group in groups.items(): @@ -260,7 +270,7 @@ def check_for_admin_in_all_meetings( if meeting_user_ids and ( "user.can_manage" in group_permissions or "user.can_update" in group_permissions - ): # TODO test mit can manage hier nur can update + ): admin_meeting_users.update( self.datastore.get_many( [ @@ -275,22 +285,14 @@ def check_for_admin_in_all_meetings( ) else: return False - # if instance/requested user is a meeting admin in this meeting. if admin_meeting_users: - if [ - admin_meeting_user - for admin_meeting_user in admin_meeting_users.values() - if admin_meeting_user.get("user_id") == instance_id - ] != []: - return False - # if requesting user is not a meeting admin in this meeting. - if not next( - iter( - admin_meeting_user - for admin_meeting_user in admin_meeting_users.values() - if admin_meeting_user.get("user_id") == self.user_id - ) - ): + is_admin = False + for admin_meeting_user in admin_meeting_users.values(): + if admin_meeting_user.get("user_id") == instance_id: + return False + if admin_meeting_user.get("user_id") == self.user_id: + is_admin = True + if not is_admin: return False else: return False diff --git a/tests/system/action/user/test_update.py b/tests/system/action/user/test_update.py index 379108689..8618b8236 100644 --- a/tests/system/action/user/test_update.py +++ b/tests/system/action/user/test_update.py @@ -17,7 +17,7 @@ def permission_setup(self) -> None: } ) - def two_meetings_standard_fails( + def two_meetings_test_fail_ADEFGH( self, committee_id: None | int = None, group_B_success: bool = False ) -> None: # test group A @@ -1013,6 +1013,76 @@ def test_perm_group_A_belongs_to_same_meetings(self) -> None: }, ) + def test_perm_group_A_belongs_to_same_meetings_can_update(self) -> None: + """May update group A fields on any scope as long as admin user Ann belongs to all meetings user Ben belongs to. See issue 2522.""" + self.permission_setup() # meeting 1 + logged in test user + user 111 + self.create_meeting(4) # meeting 4 + self.update_model( + "group/6", + {"permissions": ["user.can_update"]}, + ) + # Admin groups of meeting/1 and meeting/4 for test user (4 via group permission) + self.set_user_groups(self.user_id, [2, 5]) + # 111 admin in meeting 6 via group permission + self.set_user_groups(111, [1, 6]) + self.two_meetings_test_fail_ADEFGH() + self.update_model("group/5", {"meeting_user_ids": []}) + self.update_model("group/6", {"meeting_user_ids": []}) + # Admin groups of meeting/1 and meeting/4 for test user + self.set_user_groups(self.user_id, [2, 4, 6]) + # 111 into both meetings + self.set_user_groups(111, [1, 4]) + response = self.request( + "user.update", + { + "id": 111, + "pronoun": "I'm gonna get updated.", + }, + ) + self.assert_status_code(response, 200) + self.assert_model_exists( + "user/111", + { + "pronoun": "I'm gonna get updated.", + }, + ) + + def test_perm_group_A_belongs_to_same_meetings_can_manage(self) -> None: + """May update group A fields on any scope as long as admin user Ann belongs to all meetings user Ben belongs to. + Also makes sure being in multiple groups of a single meeting is no problem. See issue 2522. + """ + self.permission_setup() # meeting 1 + logged in test user + user 111 + self.create_meeting(4) # meeting 4 + self.update_model( + "group/6", + {"permissions": ["user.can_manage"]}, + ) + # Admin groups of meeting/1 and meeting/4 for test user (4 via group permission) + self.set_user_groups(self.user_id, [2, 5]) + # 111 admin in meeting 6 via group permission + self.set_user_groups(111, [1, 6]) + self.two_meetings_test_fail_ADEFGH() + self.update_model("group/5", {"meeting_user_ids": []}) + self.update_model("group/6", {"meeting_user_ids": []}) + # Admin groups of meeting/1 and meeting/4 for test user + self.set_user_groups(self.user_id, [1, 2, 4, 6]) + # 111 into both meetings + self.set_user_groups(111, [1, 4]) + response = self.request( + "user.update", + { + "id": 111, + "pronoun": "I'm gonna get updated.", + }, + ) + self.assert_status_code(response, 200) + self.assert_model_exists( + "user/111", + { + "pronoun": "I'm gonna get updated.", + }, + ) + def test_perm_group_A_belongs_to_same_meetings_one_time_admin(self) -> None: """May update group A fields on any scope as long as admin user Ann belongs to all meetings user Ben belongs to. However, is not admin in one. See issue 2522.""" self.permission_setup() # meeting 1 + logged in test user + user 111 @@ -1021,7 +1091,7 @@ def test_perm_group_A_belongs_to_same_meetings_one_time_admin(self) -> None: self.set_user_groups(self.user_id, [2, 4]) # 111 into both meetings self.set_user_groups(111, [1, 4]) - self.two_meetings_standard_fails() + self.two_meetings_test_fail_ADEFGH() # test group B and C response = self.request( "user.update", @@ -1047,7 +1117,7 @@ def test_perm_group_A_belongs_to_same_meetings_both_admin(self) -> None: self.set_user_groups(self.user_id, [2, 5]) # 111 into both meetings (one admin group) self.set_user_groups(111, [1, 5]) - self.two_meetings_standard_fails() + self.two_meetings_test_fail_ADEFGH() # test group B and C response = self.request( "user.update", @@ -1092,7 +1162,7 @@ def test_perm_group_A_belongs_to_same_meetings_committee_admin(self) -> None: # 111 is committee admin committee_id = 60 self.set_committee_management_level([committee_id], 111) - self.two_meetings_standard_fails(committee_id) + self.two_meetings_test_fail_ADEFGH(committee_id) # test group B and C response = self.request( "user.update", diff --git a/tests/system/presenter/base.py b/tests/system/presenter/base.py index 286e68c39..9a5cba740 100644 --- a/tests/system/presenter/base.py +++ b/tests/system/presenter/base.py @@ -1,3 +1,4 @@ +from collections import defaultdict from typing import Any from openslides_backend.http.application import OpenSlidesBackendWSGIApplication @@ -26,3 +27,82 @@ def request( if isinstance(response.json, list) and len(response.json) == 1: return (response.status_code, response.json[0]) return (response.status_code, response.json) + + def create_meeting_for_two_users( + self, user1: int, user2: int, base: int = 1 + ) -> None: + """ + Creates meeting with id 1, committee 60 and groups with ids 1, 2, 3 by default. + With base you can setup other meetings, but be cautious because of group-ids + The groups have no permissions and no users by default. + Uses usernumber to create meeting users with the concatenation of base and usernumber. + """ + committee_id = base + 59 + self.set_models( + { + f"meeting/{base}": { + "group_ids": [base, base + 1, base + 2], + "default_group_id": base, + "admin_group_id": base + 1, + "committee_id": committee_id, + "is_active_in_organization_id": 1, + }, + f"group/{base}": { + "meeting_id": base, + "default_group_for_meeting_id": base, + "name": f"group{base}", + }, + f"group/{base+1}": { + "meeting_id": base, + "admin_group_for_meeting_id": base, + "name": f"group{base+1}", + }, + f"group/{base+2}": { + "meeting_id": base, + "name": f"group{base+2}", + }, + f"committee/{committee_id}": { + "organization_id": 1, + "name": f"Commitee{committee_id}", + "meeting_ids": [base], + }, + "organization/1": { + "limit_of_meetings": 0, + "active_meeting_ids": [base], + "enable_electronic_voting": True, + }, + f"meeting_user/{base}{user1}": {"user_id": user1, "meeting_id": base}, + f"meeting_user/{base}{user2}": {"user_id": user2, "meeting_id": base}, + } + ) + # meet_user_ids = self.datastore.get(f"user/{user1}", ["meeting_user_ids"]).get("meeting_user_ids") + # self.update_model( + # f"user/{user1}", + # {"meeting_user_ids": meet_user_ids + int(f"{base}{user1}")} + # ) + # meet_user_ids = self.datastore.get(f"user/{user2}", ["meeting_user_ids"]).get("meeting_user_ids") + # self.update_model( + # f"user/{user2}", + # {"meeting_user_ids": meet_user_ids + int(f"{base}{user2}")} + # ) + + def move_user_to_group(self, meeting_user_to_groups: dict[int, Any]) -> None: + """ + Sets the users groups, returns the meeting_user_ids + Be careful as it does not reset previeously set groups if they are not in the data set. + """ + groups_to_meeting_user = defaultdict(list) + for meeting_user_id, group_id in meeting_user_to_groups.items(): + if group_id: + self.update_model( + f"meeting_user/{meeting_user_id}", {"group_ids": [group_id]} + ) + groups_to_meeting_user[group_id].append(meeting_user_id) + else: + self.update_model( + f"meeting_user/{meeting_user_id}", {"group_ids": None} + ) + for group_id, meeting_user_ids in groups_to_meeting_user.items(): + self.update_model( + f"group/{group_id}", {"meeting_user_ids": meeting_user_ids} + ) diff --git a/tests/system/presenter/test_get_user_editable.py b/tests/system/presenter/test_get_user_editable.py index 7a68b6f14..bd0d3350b 100644 --- a/tests/system/presenter/test_get_user_editable.py +++ b/tests/system/presenter/test_get_user_editable.py @@ -4,7 +4,8 @@ class TestGetUSerEditable(BasePresenterTestCase): - def test_good(self) -> None: + def set_up(self) -> None: + self.maxDiff = None # TODO delete self.create_model( "user/111", { @@ -13,8 +14,7 @@ def test_good(self) -> None: "is_active": True, "password": self.auth.hash("Kohl"), "default_password": "Kohl", - "committee_management_ids": [2] - } + }, ) self.login(111) self.set_models( @@ -60,16 +60,28 @@ def test_good(self) -> None: }, } ) + + def test_with_oml(self) -> None: + self.set_up() + self.update_model( + "user/111", + { + "organization_management_level": OrganizationManagementLevel.CAN_MANAGE_USERS, + }, + ) status_code, data = self.request( - "get_user_editable", {"user_ids": [2, 3, 4, 5, 6, 7]} + "get_user_editable", + { + "user_ids": [2, 3, 4, 5, 6, 7], + "fields": ["first_name", "default_password"], + }, ) self.assertEqual(status_code, 200) self.assertEqual( data, { "2": { - "editable": False, - "message": 'Your organization management level is not high enough to change a user with a Level of can_manage_users!' + "editable": True, }, "3": { "editable": True, @@ -82,7 +94,7 @@ def test_good(self) -> None: }, "6": { "editable": False, - "message": 'Your organization management level is not high enough to change a user with a Level of superadmin!' + "message": "Your organization management level is not high enough to change a user with a Level of superadmin!", }, "7": { "editable": True, @@ -90,12 +102,234 @@ def test_good(self) -> None: }, ) - def test_without_user_None(self) -> None: - status_code, data = self.request("get_user_editable", {"user_ids": [None]}) + def test_with_cml(self) -> None: + self.set_up() + self.update_model( + "user/111", + { + "committee_management_ids": [2], + }, + ) + status_code, data = self.request( + "get_user_editable", + { + "user_ids": [2, 3, 4, 5, 6, 7], + "fields": ["first_name", "default_password"], + }, + ) + self.assertEqual(status_code, 200) + self.assertEqual( + data, + { + "2": { + "editable": False, + "message": "Your organization management level is not high enough to change a user with a Level of can_manage_users!", + }, + "3": { + "editable": False, + "message": "Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or CommitteeManagementLevel can_manage in committee 1", + }, + "4": { + "editable": True, + }, + "5": { + "editable": False, + "message": "Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + }, + "6": { + "editable": False, + "message": "Your organization management level is not high enough to change a user with a Level of superadmin!", + }, + "7": { + "editable": True, + }, + }, + ) + + def test_with_same_meeting(self) -> None: + """ + User 2 can be edited because he is only in meetings which User 111 is admin of. + User 7 can not be edited because he is in two of the same meetings but User 111 is not admin in all of them. + """ + self.set_up() + self.create_meeting_for_two_users(5, 111, 1) + self.create_meeting_for_two_users(5, 111, 4) + self.create_meeting_for_two_users(7, 111, 7) + self.create_meeting_for_two_users(7, 111, 10) + self.update_model("meeting/1", {"committee_id": 1}) + self.update_model("meeting/4", {"committee_id": 2}) + self.update_model("meeting/7", {"committee_id": 1}) + self.update_model("meeting/10", {"committee_id": 2}) + # User 111 is meeting admin in meeting 1, 4 and 7 but normal user in 10 + # User 5 is normal user in meeting 1 and 4 + # User 7 is normal user in meeting 7 and 10 + meeting_user_to_group = { + 1111: 2, + 4111: 5, + 15: 1, + 45: 4, + 7111: 8, + 10111: 10, + 77: 7, + 107: 10, + } + self.move_user_to_group(meeting_user_to_group) + self.update_model( + "user/5", + { + "meeting_user_ids": [ + 15, + 45, + ], + "meeting_ids": [1, 4], + }, + ) + self.update_model( + "user/7", + { + "meeting_user_ids": [77, 107], + "meeting_ids": [7, 10], + }, + ) + self.update_model( + "user/111", + { + "meeting_user_ids": [1111, 4111, 7111, 10111], + "meeting_ids": [1, 4, 7, 10], + }, + ) + status_code, data = self.request( + "get_user_editable", + { + "user_ids": [2, 3, 4, 5, 6, 7], + "fields": ["first_name", "default_password"], + }, + ) + self.assertEqual(status_code, 200) + self.assertEqual( + data, + { + "2": { + "editable": False, + "message": "Your organization management level is not high enough to change a user with a Level of can_manage_users!", + }, + "3": { + "editable": False, + "message": "Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or CommitteeManagementLevel can_manage in committee 1", + }, + "4": { + "editable": False, + "message": "Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + }, + "5": { + "editable": True, + }, + "6": { + "editable": False, + "message": "Your organization management level is not high enough to change a user with a Level of superadmin!", + }, + "7": { + "editable": False, + "message": "Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + }, + }, + ) + + def test_with_all_payload_groups(self) -> None: + """ + Tests all user.create/update payload field groups. Especially the field 'saml_id'. + """ + self.set_up() + self.update_model( + "user/111", + { + "organization_management_level": OrganizationManagementLevel.CAN_MANAGE_USERS, + }, + ) + status_code, data = self.request( + "get_user_editable", + { + "user_ids": [2, 3, 4, 5, 6, 7], + "fields": [ + "first_name", + "default_password", + "vote_weight", + "group_ids", + "committee_management_ids", + "organization_management_level", + "is_demo_user", + "saml_id", + ], + }, + ) + self.assertEqual(status_code, 200) + self.assertEqual( + data, + { + "2": { + "editable": False, + "message": "The field 'saml_id' can only be used in internal action calls", + }, + "3": { + "editable": False, + "message": "The field 'saml_id' can only be used in internal action calls", + }, + "4": { + "editable": False, + "message": "The field 'saml_id' can only be used in internal action calls", + }, + "5": { + "editable": False, + "message": "The field 'saml_id' can only be used in internal action calls", + }, + "6": { + "editable": False, + "message": "Your organization management level is not high enough to change a user with a Level of superadmin!", + }, + "7": { + "editable": False, + "message": "The field 'saml_id' can only be used in internal action calls", + }, + }, + ) + + def test_payload_list_of_None(self) -> None: + status_code, data = self.request( + "get_user_editable", + { + "user_ids": [None], + "fields": [ + "first_name", + "default_password", + ], + }, + ) self.assertEqual(status_code, 400) self.assertIn("data.user_ids[0] must be integer", data["message"]) + status_code, data = self.request( + "get_user_editable", {"user_ids": [1, 2], "fields": [None]} + ) + self.assertEqual(status_code, 400) + self.assertIn("data.fields[0] must be string", data["message"]) - def test_without_user_empty_list(self) -> None: - status_code, data = self.request("get_user_editable", {"user_ids": []}) + def test_payload_empty_list(self) -> None: + status_code, data = self.request( + "get_user_editable", + { + "user_ids": [], + "fields": [ + "first_name", + "default_password", + ], + }, + ) self.assertEqual(status_code, 200) assert data == {} + status_code, data = self.request( + "get_user_editable", {"user_ids": [1, 2], "fields": []} + ) + self.assertEqual(status_code, 400) + assert data == { + "message": "Need at least one field name to check editability.", + "success": False, + } diff --git a/tests/system/presenter/test_get_user_related_models.py b/tests/system/presenter/test_get_user_related_models.py index 7a5425e0a..2033df27e 100644 --- a/tests/system/presenter/test_get_user_related_models.py +++ b/tests/system/presenter/test_get_user_related_models.py @@ -1,4 +1,3 @@ -from collections import defaultdict from typing import Any, cast from openslides_backend.permissions.management_levels import OrganizationManagementLevel @@ -158,7 +157,7 @@ def test_two_meetings(self) -> None: # Admin groups of meeting/1 for executor user meeting/2 as normal user # 111 into both meetings meeting_user_to_group = {12: 2, 42: 4, 1111: 1, 4111: 4} - self.move_user_from_to_group(meeting_user_to_group) + self.move_user_to_group(meeting_user_to_group) status_code, data = self.request("get_user_related_models", {"user_ids": [111]}) self.assertEqual(status_code, 403) self.assertEqual( @@ -167,7 +166,7 @@ def test_two_meetings(self) -> None: ) # Admin groups of meeting/1 for executor user # 111 into both meetings - self.move_user_from_to_group({12: 2, 42: None, 1111: 1, 4111: 4}) + self.move_user_to_group({12: 2, 42: None, 1111: 1, 4111: 4}) status_code, data = self.request("get_user_related_models", {"user_ids": [111]}) self.assertEqual(status_code, 403) self.assertEqual( @@ -177,7 +176,7 @@ def test_two_meetings(self) -> None: # Admin groups of meeting/1 and meeting/4 for executor user # 111 into both meetings meeting_user_to_group = {12: 2, 42: 5, 1111: 1, 4111: 4} - self.move_user_from_to_group(meeting_user_to_group) + self.move_user_to_group(meeting_user_to_group) status_code, data = self.request("get_user_related_models", {"user_ids": [111]}) self.assertEqual(status_code, 200) @@ -603,72 +602,3 @@ def test_get_user_related_models_with_locked_meetings(self) -> None: ] }, } - - def create_meeting_for_two_users( - self, user1: int, user2: int, base: int = 1 - ) -> None: - """ - Creates meeting with id 1, committee 60 and groups with ids 1, 2, 3 by default. - With base you can setup other meetings, but be cautious because of group-ids - The groups have no permissions and no users by default. - Uses usernumber to create meetingusers with the concatenation of base and usernumber. - """ - committee_id = base + 59 - self.set_models( - { - f"meeting/{base}": { - "group_ids": [base, base + 1, base + 2], - "default_group_id": base, - "admin_group_id": base + 1, - "committee_id": committee_id, - "is_active_in_organization_id": 1, - }, - f"group/{base}": { - "meeting_id": base, - "default_group_for_meeting_id": base, - "name": f"group{base}", - }, - f"group/{base+1}": { - "meeting_id": base, - "admin_group_for_meeting_id": base, - "name": f"group{base+1}", - }, - f"group/{base+2}": { - "meeting_id": base, - "name": f"group{base+2}", - }, - f"committee/{committee_id}": { - "organization_id": 1, - "name": f"Commitee{committee_id}", - "meeting_ids": [base], - }, - "organization/1": { - "limit_of_meetings": 0, - "active_meeting_ids": [base], - "enable_electronic_voting": True, - }, - f"meeting_user/{base}{user1}": {"user_id": user1, "meeting_id": base}, - f"meeting_user/{base}{user2}": {"user_id": user2, "meeting_id": base}, - } - ) - - def move_user_from_to_group(self, meeting_user_to_groups: dict[int, Any]) -> None: - """ - Sets the users groups, returns the meeting_user_ids - Be careful as it does not reset previeously set groups if they are not in the data set. - """ - groups_to_meeting_user = defaultdict(list) - for meeting_user_id, group_id in meeting_user_to_groups.items(): - if group_id: - self.update_model( - f"meeting_user/{meeting_user_id}", {"group_ids": [group_id]} - ) - groups_to_meeting_user[group_id].append(meeting_user_id) - else: - self.update_model( - f"meeting_user/{meeting_user_id}", {"group_ids": None} - ) - for group_id, meeting_user_ids in groups_to_meeting_user.items(): - self.update_model( - f"group/{group_id}", {"meeting_user_ids": meeting_user_ids} - ) From 3d0df2b90cc3d1d4ce41b5eb44107dd727dfd8b1 Mon Sep 17 00:00:00 2001 From: Hannes Janott Date: Wed, 11 Sep 2024 15:56:11 +0200 Subject: [PATCH 10/19] delete unnecessary comments and return types assert not anonymous in UserMixin simplified logic in GetUserEditable, CreateUpdatePermissionsMixin, UserScopeMixin Adapt tests --- docs/presenters/get_user_editable.md | 2 +- .../action/actions/user/base_import.py | 4 +- .../action/actions/user/create.py | 4 - .../action/actions/user/update.py | 4 - .../action/actions/user/user_mixins.py | 4 + .../presenter/get_user_editable.py | 6 +- .../user_create_update_permissions_mixin.py | 79 +++-------- .../shared/mixins/user_scope_mixin.py | 125 ++++++++++-------- .../action/user/scope_permissions_mixin.py | 8 +- tests/system/presenter/base.py | 10 -- .../presenter/test_get_user_editable.py | 1 - 11 files changed, 99 insertions(+), 148 deletions(-) diff --git a/docs/presenters/get_user_editable.md b/docs/presenters/get_user_editable.md index c30e8f41a..177221c0d 100644 --- a/docs/presenters/get_user_editable.md +++ b/docs/presenters/get_user_editable.md @@ -13,7 +13,7 @@ { user_id: Id: { editable: boolean // true if user can be updated or deleted - message: string // error message if an exception was caught + message?: string // error message if an exception was caught }, ... } diff --git a/openslides_backend/action/actions/user/base_import.py b/openslides_backend/action/actions/user/base_import.py index 48718107e..df5aa5964 100644 --- a/openslides_backend/action/actions/user/base_import.py +++ b/openslides_backend/action/actions/user/base_import.py @@ -24,9 +24,7 @@ def update_instance(self, instance: dict[str, Any]) -> dict[str, Any]: rows = self.flatten_copied_object_fields( self.handle_remove_and_group_fields ) - self.create_other_actions( - rows - ) # Hier muss doch eigentlich die collection bestimmt werden, oder nicht? + self.create_other_actions(rows) return {} diff --git a/openslides_backend/action/actions/user/create.py b/openslides_backend/action/actions/user/create.py index 77b3282ad..a44a3c18f 100644 --- a/openslides_backend/action/actions/user/create.py +++ b/openslides_backend/action/actions/user/create.py @@ -70,10 +70,6 @@ class UserCreate( history_information = "Account created" own_history_information_first = True - def check_permissions(self, instance: dict[str, Any]) -> None: - self.assert_not_anonymous() - super().check_permissions(instance) - def update_instance(self, instance: dict[str, Any]) -> dict[str, Any]: self.meeting_id: int | None = instance.get("meeting_id") diff --git a/openslides_backend/action/actions/user/update.py b/openslides_backend/action/actions/user/update.py index 0de3f40a5..1a70c0100 100644 --- a/openslides_backend/action/actions/user/update.py +++ b/openslides_backend/action/actions/user/update.py @@ -79,10 +79,6 @@ class UserUpdate( permission = Permissions.User.CAN_UPDATE check_email_field = "email" - def check_permissions(self, instance: dict[str, Any]) -> None: - self.assert_not_anonymous() - super().check_permissions(instance) - def validate_instance(self, instance: dict[str, Any]) -> None: super().validate_instance(instance) if not self.internal and any( diff --git a/openslides_backend/action/actions/user/user_mixins.py b/openslides_backend/action/actions/user/user_mixins.py index 5e5fbcca1..e7b3d2f7f 100644 --- a/openslides_backend/action/actions/user/user_mixins.py +++ b/openslides_backend/action/actions/user/user_mixins.py @@ -89,6 +89,10 @@ class UserMixin(CheckForArchivedMeetingMixin): "locked_out": {"type": "boolean"}, } + def check_permissions(self, instance: dict[str, Any]) -> None: + self.assert_not_anonymous() + super().check_permissions(instance) + def validate_instance(self, instance: dict[str, Any]) -> None: super().validate_instance(instance) if "meeting_id" not in instance and any( diff --git a/openslides_backend/presenter/get_user_editable.py b/openslides_backend/presenter/get_user_editable.py index 96ae93b1c..efe2fcad7 100644 --- a/openslides_backend/presenter/get_user_editable.py +++ b/openslides_backend/presenter/get_user_editable.py @@ -55,10 +55,8 @@ def get_result(self) -> Any: instance.update({"id": user_id}) result[str(user_id)] = {} try: - if self.check_permissions(instance): - result[str(user_id)]["editable"] = True - else: - result[str(user_id)]["editable"] = False + self.check_permissions(instance) + result[str(user_id)]["editable"] = True except (PermissionDenied, MissingPermission, ActionException) as e: result[str(user_id)]["editable"] = False result[str(user_id)]["message"] = e.message diff --git a/openslides_backend/shared/mixins/user_create_update_permissions_mixin.py b/openslides_backend/shared/mixins/user_create_update_permissions_mixin.py index 08eb1e4dd..ce20e2756 100644 --- a/openslides_backend/shared/mixins/user_create_update_permissions_mixin.py +++ b/openslides_backend/shared/mixins/user_create_update_permissions_mixin.py @@ -9,9 +9,6 @@ CommitteeManagementLevel, OrganizationManagementLevel, ) -from openslides_backend.permissions.permission_helper import ( - has_committee_management_level, -) from openslides_backend.permissions.permissions import Permissions, permission_parents from openslides_backend.services.datastore.commands import GetManyRequest from openslides_backend.services.datastore.interface import DatastoreService @@ -175,7 +172,6 @@ def _get_user_meetings_with_permission( class CreateUpdatePermissionsMixin(UserScopeMixin, BaseServiceProvider): permstore: PermissionVarStore permission: Permission - name: str internal: bool field_rights: dict[str, list] = { @@ -213,7 +209,7 @@ class CreateUpdatePermissionsMixin(UserScopeMixin, BaseServiceProvider): "H": ["saml_id"], } - def check_permissions(self, instance: dict[str, Any]) -> bool | None: + def check_permissions(self, instance: dict[str, Any]) -> None: """ Checks the permissions on a per field and user.scope base, details see https://github.com/OpenSlides/OpenSlides/wiki/Users @@ -222,8 +218,6 @@ def check_permissions(self, instance: dict[str, Any]) -> bool | None: The fields groups and their necessary permissions are also documented there. Returns true if permissions are given. """ - if "meta_position" in instance: # TODO still needed with all the changes? - instance.pop("meta_position") if "forwarding_committee_ids" in instance: raise PermissionDenied("forwarding_committee_ids is not allowed.") @@ -263,7 +257,6 @@ def check_permissions(self, instance: dict[str, Any]) -> bool | None: self.check_group_A(actual_group_fields["A"], instance) self.check_group_F(actual_group_fields["F"], instance) self.check_group_G(actual_group_fields["G"]) - return True def check_group_A(self, fields: list[str], instance: dict[str, Any]) -> None: """Check Group A: Depending on scope of user to act on""" @@ -274,48 +267,20 @@ def check_group_A(self, fields: list[str], instance: dict[str, Any]) -> None: return if self.instance_user_scope == UserScope.Organization: - if self.permstore.user_committees.intersection( - self.instance_committee_meeting_ids - ) or self.check_for_admin_in_all_meetings(instance.get("id", 0)): - for committee_id in self.instance_committee_meeting_ids: - if ( - instance_id := instance.get("id") - ) and has_committee_management_level( - self.datastore, - instance_id, - CommitteeManagementLevel.CAN_MANAGE, - committee_id, - ): - raise MissingPermission( - {OrganizationManagementLevel.CAN_MANAGE_USERS: 1} - ) - return - else: + if not ( + self.permstore.user_committees.intersection( + self.instance_committee_meeting_ids + ) + or self.check_for_admin_in_all_meetings(instance) + ): raise MissingPermission( {OrganizationManagementLevel.CAN_MANAGE_USERS: 1} ) elif self.instance_user_scope == UserScope.Committee: - if ( + if not ( self.instance_user_scope_id in self.permstore.user_committees - or self.check_for_admin_in_all_meetings(instance.get("id", 0)) + or self.check_for_admin_in_all_meetings(instance) ): - for committee_id in self.instance_committee_meeting_ids: - if ( - instance_id := instance.get("id") - ) and has_committee_management_level( - self.datastore, - instance_id, - CommitteeManagementLevel.CAN_MANAGE, - committee_id, - ): - raise MissingPermission( - { - OrganizationManagementLevel.CAN_MANAGE_USERS: 1, - CommitteeManagementLevel.CAN_MANAGE: self.instance_user_scope_id, - } - ) - return - else: raise MissingPermission( { OrganizationManagementLevel.CAN_MANAGE_USERS: 1, @@ -348,7 +313,7 @@ def check_group_B( or locked_from_inside ) and fields: meeting_ids = self._meetings_from_group_B_fields_from_instance(instance) - if (diff := meeting_ids - self.permstore.user_meetings) and diff != {None}: + if diff := meeting_ids - self.permstore.user_meetings: raise MissingPermission( {self.permission: meeting_id for meeting_id in diff} ) @@ -424,26 +389,12 @@ def check_group_F(self, fields: list[str], instance: dict[str, Any]) -> None: ): return expected_oml_permission = OrganizationManagementLevel.CAN_MANAGE_USERS - if ( - expected_oml_permission > self.permstore.user_oml - and not self.check_for_admin_in_all_meetings(instance.get("id", 0)) + if not ( + expected_oml_permission <= self.permstore.user_oml + or self.check_for_admin_in_all_meetings(instance) ): raise MissingPermission({expected_oml_permission: 1}) else: - for committee_id in self.instance_committee_meeting_ids: - if ( - instance_id := instance.get("id") - ) and has_committee_management_level( - self.datastore, - instance_id, - CommitteeManagementLevel.CAN_MANAGE, - committee_id, - ): - raise MissingPermission( - { - OrganizationManagementLevel.CAN_MANAGE_USERS: 1, - } - ) return else: if self.permstore.user_oml >= OrganizationManagementLevel.CAN_MANAGE_USERS: @@ -575,7 +526,9 @@ def _meetings_from_group_B_fields_from_instance( any other group B field. """ meetings: set[int] = set(instance.get("is_present_in_meeting_ids", [])) - meetings.add(cast(int, instance.get("meeting_id"))) + meeting_id = cast(int, instance.get("meeting_id")) + if meeting_id: + meetings.add(meeting_id) return meetings diff --git a/openslides_backend/shared/mixins/user_scope_mixin.py b/openslides_backend/shared/mixins/user_scope_mixin.py index d77576f82..5611954a5 100644 --- a/openslides_backend/shared/mixins/user_scope_mixin.py +++ b/openslides_backend/shared/mixins/user_scope_mixin.py @@ -31,6 +31,7 @@ def __repr__(self) -> str: class UserScopeMixin(BaseServiceProvider): instance_committee_meeting_ids: dict + name: str def get_user_scope( self, id_or_instance: int | dict[str, Any] @@ -39,8 +40,8 @@ def get_user_scope( Parameter id_or_instance: id for existing user or instance for user to create Returns the scope of the given user id together with the relevant scope id (either meeting, committee or organization), the OML level of the user as string (empty string if the user - has none) and the ids of all committees that the user is either a manager in or a member of. - and their respective meetings the user is part of. #A committe can have no meetings if the + has none) and the ids of all committees that the user is either a manager in or a member of + together with their respective meetings the user being part of. A committee can have no meetings if the user just has committee management rights and is not part of any of its meetings. """ meetings: set[int] = set() @@ -189,15 +190,32 @@ def check_permissions_for_scope( ) def check_for_admin_in_all_meetings( - self, instance_id: int, b_meeting_ids: set[int] | None = None + self, + instance_or_id: int | dict[str, Any], + b_meeting_ids: set[int] | None = None, ) -> bool: """ - Checks if the requesting user has permissions to manage participants in all of requested users meetings. - Also checks if the requesting user has meeting admin rights and the requested user doesn't. + Checks if the requesting user has permissions to manage participants in all of requested users meetings but requested user doesn't. + Also checks requested user has no committee management rights if a dict was provided instead of an id. Returns true if permissions are given. False if not. Raises no Exceptions. """ - if not instance_id: - return False + if isinstance(instance_or_id, dict): + if not (instance_id := instance_or_id.get("id")): + return False + if hasattr(self, "name") and self.name == "user.create": + if instance_or_id.get("committee_management_ids", []): + return False + else: + user = self.datastore.get( + fqid_from_collection_and_id("user", instance_id), + ["committee_management_ids"], + lock_result=False, + use_changed_models=False, + ) + if user.get("committee_management_ids", []): + return False + else: + instance_id = instance_or_id if not b_meeting_ids: if not hasattr(self, "instance_committee_meeting_ids"): return False @@ -235,56 +253,51 @@ def check_for_admin_in_all_meetings( lock_result=False, ).get("meeting", {}) for meeting_id, meeting_dict in intersection_meetings.items(): - # get meetings admins admin_meeting_users = {} - # unnecessary "if" due to admin group always existant? - if admin_group_id := meeting_dict.get("admin_group_id"): - admin_group = self.datastore.get( - fqid_from_collection_and_id("group", admin_group_id), - ["meeting_user_ids"], - lock_result=False, - ) - admin_meeting_users = self.datastore.get_many( - [ - GetManyRequest( - "meeting_user", - admin_group.get("meeting_user_ids", []), - ["user_id"], - ) - ], - lock_result=False, - ).get("meeting_user", {}) - # unnecessary "if" due to default group always existant? - if group_ids := meeting_dict.get("group_ids", []): - groups = self.datastore.get_many( - [ - GetManyRequest( - "group", group_ids, ["meeting_user_ids", "permissions"] - ) - ], - lock_result=False, - ).get("group", {}) - for group_id, group in groups.items(): - meeting_user_ids = group.get("meeting_user_ids", []) - group_permissions = group.get("permissions", []) - if meeting_user_ids and ( - "user.can_manage" in group_permissions - or "user.can_update" in group_permissions - ): - admin_meeting_users.update( - self.datastore.get_many( - [ - GetManyRequest( - "meeting_user", - meeting_user_ids, - ["user_id"], - ) - ], - lock_result=False, - ).get("meeting_user", {}) - ) - else: - return False + admin_group_id = meeting_dict.get("admin_group_id", 0) + admin_group = self.datastore.get( + fqid_from_collection_and_id("group", admin_group_id), + ["meeting_user_ids"], + lock_result=False, + ) + admin_meeting_users = self.datastore.get_many( + [ + GetManyRequest( + "meeting_user", + admin_group.get("meeting_user_ids", []), + ["user_id"], + ) + ], + lock_result=False, + ).get("meeting_user", {}) + group_ids = meeting_dict.get("group_ids", []) + groups = self.datastore.get_many( + [ + GetManyRequest( + "group", group_ids, ["meeting_user_ids", "permissions"] + ) + ], + lock_result=False, + ).get("group", {}) + for group_id, group in groups.items(): + meeting_user_ids = group.get("meeting_user_ids", []) + group_permissions = group.get("permissions", []) + if meeting_user_ids and ( + "user.can_manage" in group_permissions + or "user.can_update" in group_permissions + ): + admin_meeting_users.update( + self.datastore.get_many( + [ + GetManyRequest( + "meeting_user", + meeting_user_ids, + ["user_id"], + ) + ], + lock_result=False, + ).get("meeting_user", {}) + ) if admin_meeting_users: is_admin = False for admin_meeting_user in admin_meeting_users.values(): diff --git a/tests/system/action/user/scope_permissions_mixin.py b/tests/system/action/user/scope_permissions_mixin.py index a2e78f506..f0ddc4162 100644 --- a/tests/system/action/user/scope_permissions_mixin.py +++ b/tests/system/action/user/scope_permissions_mixin.py @@ -45,13 +45,15 @@ def setup_scoped_user(self, scope: UserScope) -> None: "meeting/1": { "user_ids": [111], "committee_id": 1, - "group_ids": [11], + "group_ids": [11, 12], + "admin_group_id": 12, "is_active_in_organization_id": 1, }, "meeting/2": { "user_ids": [111], "committee_id": 2, - "group_ids": [22], + "group_ids": [22, 23], + "admin_group_id": 23, "is_active_in_organization_id": 1, }, "user/111": { @@ -70,7 +72,9 @@ def setup_scoped_user(self, scope: UserScope) -> None: "group_ids": [22], }, "group/11": {"meeting_id": 1, "meeting_user_ids": [11]}, + "group/12": {"meeting_id": 1}, "group/22": {"meeting_id": 2, "meeting_user_ids": [22]}, + "group/23": {"meeting_id": 2}, } ) elif scope == UserScope.Committee: diff --git a/tests/system/presenter/base.py b/tests/system/presenter/base.py index 9a5cba740..63dc5b4f5 100644 --- a/tests/system/presenter/base.py +++ b/tests/system/presenter/base.py @@ -75,16 +75,6 @@ def create_meeting_for_two_users( f"meeting_user/{base}{user2}": {"user_id": user2, "meeting_id": base}, } ) - # meet_user_ids = self.datastore.get(f"user/{user1}", ["meeting_user_ids"]).get("meeting_user_ids") - # self.update_model( - # f"user/{user1}", - # {"meeting_user_ids": meet_user_ids + int(f"{base}{user1}")} - # ) - # meet_user_ids = self.datastore.get(f"user/{user2}", ["meeting_user_ids"]).get("meeting_user_ids") - # self.update_model( - # f"user/{user2}", - # {"meeting_user_ids": meet_user_ids + int(f"{base}{user2}")} - # ) def move_user_to_group(self, meeting_user_to_groups: dict[int, Any]) -> None: """ diff --git a/tests/system/presenter/test_get_user_editable.py b/tests/system/presenter/test_get_user_editable.py index bd0d3350b..967ed2610 100644 --- a/tests/system/presenter/test_get_user_editable.py +++ b/tests/system/presenter/test_get_user_editable.py @@ -5,7 +5,6 @@ class TestGetUSerEditable(BasePresenterTestCase): def set_up(self) -> None: - self.maxDiff = None # TODO delete self.create_model( "user/111", { From e46cffea06db4d0958d24c5436f37e6bec83e83f Mon Sep 17 00:00:00 2001 From: Hannes Janott Date: Thu, 19 Sep 2024 16:04:28 +0200 Subject: [PATCH 11/19] restructure check_for_admin_in_all_meetings repair tests extend exception message --- global/meta | 2 +- .../user_create_update_permissions_mixin.py | 37 ++- .../shared/mixins/user_scope_mixin.py | 228 +++++++++++------- .../action/user/scope_permissions_mixin.py | 24 +- tests/system/action/user/test_delete.py | 4 +- .../action/user/test_generate_new_password.py | 4 +- .../user/test_reset_password_to_default.py | 4 +- tests/system/action/user/test_set_password.py | 25 +- tests/system/action/user/test_update.py | 24 +- .../presenter/test_get_user_editable.py | 22 +- .../presenter/test_get_user_related_models.py | 17 +- 11 files changed, 253 insertions(+), 138 deletions(-) diff --git a/global/meta b/global/meta index 29f2e62f6..84bb58e14 160000 --- a/global/meta +++ b/global/meta @@ -1 +1 @@ -Subproject commit 29f2e62f615168a3592aae342e15f3445118ee8e +Subproject commit 84bb58e141e088f9fd0678214d5b51d040d53034 diff --git a/openslides_backend/shared/mixins/user_create_update_permissions_mixin.py b/openslides_backend/shared/mixins/user_create_update_permissions_mixin.py index ce20e2756..4a183e850 100644 --- a/openslides_backend/shared/mixins/user_create_update_permissions_mixin.py +++ b/openslides_backend/shared/mixins/user_create_update_permissions_mixin.py @@ -271,20 +271,36 @@ def check_group_A(self, fields: list[str], instance: dict[str, Any]) -> None: self.permstore.user_committees.intersection( self.instance_committee_meeting_ids ) - or self.check_for_admin_in_all_meetings(instance) + or self.check_for_admin_in_all_meetings(instance.get("id", 0)) ): raise MissingPermission( - {OrganizationManagementLevel.CAN_MANAGE_USERS: 1} + { + OrganizationManagementLevel.CAN_MANAGE_USERS: 1, + **{ + Permissions.User.CAN_UPDATE: meeting_id + for meeting_ids in self.instance_committee_meeting_ids.values() + if meeting_ids is not None + for meeting_id in meeting_ids + if meeting_id is not None + }, + } ) elif self.instance_user_scope == UserScope.Committee: if not ( self.instance_user_scope_id in self.permstore.user_committees - or self.check_for_admin_in_all_meetings(instance) + or self.check_for_admin_in_all_meetings(instance.get("id", 0)) ): raise MissingPermission( { OrganizationManagementLevel.CAN_MANAGE_USERS: 1, CommitteeManagementLevel.CAN_MANAGE: self.instance_user_scope_id, + **{ + Permissions.User.CAN_UPDATE: meeting_id + for meeting_ids in self.instance_committee_meeting_ids.values() + if meeting_ids is not None + for meeting_id in meeting_ids + if meeting_id is not None + }, } ) elif ( @@ -391,9 +407,20 @@ def check_group_F(self, fields: list[str], instance: dict[str, Any]) -> None: expected_oml_permission = OrganizationManagementLevel.CAN_MANAGE_USERS if not ( expected_oml_permission <= self.permstore.user_oml - or self.check_for_admin_in_all_meetings(instance) + or self.check_for_admin_in_all_meetings(instance.get("id", 0)) ): - raise MissingPermission({expected_oml_permission: 1}) + raise MissingPermission( + { + expected_oml_permission: 1, + **{ + Permissions.User.CAN_UPDATE: meeting_id + for meeting_ids in self.instance_committee_meeting_ids.values() + if meeting_ids is not None + for meeting_id in meeting_ids + if meeting_id is not None + }, + } + ) else: return else: diff --git a/openslides_backend/shared/mixins/user_scope_mixin.py b/openslides_backend/shared/mixins/user_scope_mixin.py index 5611954a5..786109660 100644 --- a/openslides_backend/shared/mixins/user_scope_mixin.py +++ b/openslides_backend/shared/mixins/user_scope_mixin.py @@ -44,12 +44,12 @@ def get_user_scope( together with their respective meetings the user being part of. A committee can have no meetings if the user just has committee management rights and is not part of any of its meetings. """ - meetings: set[int] = set() + meeting_ids: set[int] = set() committees_manager: set[int] = set() if isinstance(id_or_instance, dict): if "group_ids" in id_or_instance: if "meeting_id" in id_or_instance: - meetings.add(id_or_instance["meeting_id"]) + meeting_ids.add(id_or_instance["meeting_id"]) committees_manager.update( set(id_or_instance.get("committee_management_ids", [])) ) @@ -64,14 +64,14 @@ def get_user_scope( ], lock_result=False, ) - meetings.update(user.get("meeting_ids", [])) + meeting_ids.update(user.get("meeting_ids", [])) committees_manager.update(set(user.get("committee_management_ids") or [])) oml_right = user.get("organization_management_level", "") result = self.datastore.get_many( [ GetManyRequest( "meeting", - list(meetings), + list(meeting_ids), ["committee_id", "is_active_in_organization_id"], ) ] @@ -179,134 +179,180 @@ def check_permissions_for_scope( [ci for ci in committees_to_meetings.keys()], ): return - meeting_ids = set() - for mids in committees_to_meetings.values(): - meeting_ids = {meeting_id for meeting_id in mids} + meeting_ids = { + meeting_id + for mids in committees_to_meetings.values() + for meeting_id in mids + } if not meeting_ids or not self.check_for_admin_in_all_meetings( instance_id, meeting_ids ): raise MissingPermission( - {OrganizationManagementLevel.CAN_MANAGE_USERS: 1} + { + OrganizationManagementLevel.CAN_MANAGE_USERS: 1, + **{ + Permissions.User.CAN_UPDATE: meeting_id + for meeting_id in meeting_ids + }, + } ) def check_for_admin_in_all_meetings( self, - instance_or_id: int | dict[str, Any], + instance_id: int, b_meeting_ids: set[int] | None = None, ) -> bool: """ - Checks if the requesting user has permissions to manage participants in all of requested users meetings but requested user doesn't. + This is used during a permission check for scope request, user.update/create with payload fields A and F and during + user altering actions like user.delete or set_default_password. + Checks if the requesting user has permissions to manage participants in all of requested users meetings + but requested user doesn't have any of these permissions. See backend issue 2522 on github for more details. Also checks requested user has no committee management rights if a dict was provided instead of an id. + An ID will be provided during user deletion. Returns true if permissions are given. False if not. Raises no Exceptions. """ - if isinstance(instance_or_id, dict): - if not (instance_id := instance_or_id.get("id")): + if not self._check_not_committee_manager(instance_id): + return False + + if not ( + intersection_meetings := self._collect_intersected_meetings(b_meeting_ids) + ): + return False + assert isinstance(intersection_meetings, dict) + admin_meeting_users = self._collect_admin_meeting_users(intersection_meetings) + meeting_to_admin_users = self._collect_admin_users(admin_meeting_users) + return self._analyze_meetings(meeting_to_admin_users, instance_id) + + def _check_not_committee_manager(self, instance_id: int) -> bool: + if not (hasattr(self, "name") and self.name == "user.create"): + if self.datastore.get( + fqid_from_collection_and_id("user", instance_id), + ["committee_management_ids"], + lock_result=False, + use_changed_models=False, + ).get("committee_management_ids", []): return False - if hasattr(self, "name") and self.name == "user.create": - if instance_or_id.get("committee_management_ids", []): - return False - else: - user = self.datastore.get( - fqid_from_collection_and_id("user", instance_id), - ["committee_management_ids"], - lock_result=False, - use_changed_models=False, - ) - if user.get("committee_management_ids", []): - return False - else: - instance_id = instance_or_id + return True + + def _collect_intersected_meetings( + self, b_meeting_ids: set[int] | None + ) -> dict[int, Any] | bool: + """Takes the meeting ids to find intersections with the requesting users meetings. Returns False if this is not possible.""" if not b_meeting_ids: if not hasattr(self, "instance_committee_meeting_ids"): return False - b_meeting_ids = set() - for m_ids in self.instance_committee_meeting_ids.values(): - if m_ids: - b_meeting_ids.update(m_ids) - if not b_meeting_ids: - return False + if not ( + b_meeting_ids := { + m_id + for m_ids in self.instance_committee_meeting_ids.values() + for m_id in m_ids + } + ): + return False # During participant import there is no permstore. if hasattr(self, "permstore"): a_meeting_ids = ( self.permstore.user_meetings ) # returns only admin level meetings - else: - a_user = self.datastore.get( - fqid_from_collection_and_id("user", instance_id), - ["meeting_ids"], - lock_result=False, + elif not ( + a_meeting_ids := set( + self.datastore.get( + fqid_from_collection_and_id("user", self.user_id), + ["meeting_ids"], + lock_result=False, + ).get("meeting_ids", []) ) - a_meeting_ids = set(a_user.get("meeting_ids", [])) - if not a_meeting_ids: + ): return False intersection_meeting_ids = a_meeting_ids.intersection(b_meeting_ids) if not b_meeting_ids.issubset(intersection_meeting_ids): return False - intersection_meetings = self.datastore.get_many( + return self.datastore.get_many( [ GetManyRequest( "meeting", list(intersection_meeting_ids), - ["admin_group_id", "group_ids", "meeting_user_ids"], + ["admin_group_id", "group_ids"], ) ], lock_result=False, ).get("meeting", {}) - for meeting_id, meeting_dict in intersection_meetings.items(): - admin_meeting_users = {} - admin_group_id = meeting_dict.get("admin_group_id", 0) - admin_group = self.datastore.get( - fqid_from_collection_and_id("group", admin_group_id), - ["meeting_user_ids"], - lock_result=False, - ) - admin_meeting_users = self.datastore.get_many( + + def _collect_admin_meeting_users( + self, intersection_meetings: dict[int, Any] + ) -> set[int]: + """ + Gets the admin groups and those groups with permission User.CAN_UPDATE and USER.CAN_MANAGE of the meetings. + Returns a set of the groups meeting_user_ids. + """ + group_ids = [ + group_id + for meeting_id, meeting_dict in intersection_meetings.items() + for group_id in meeting_dict.get("group_ids", []) + ] + admin_group_ids = [ + meeting_dict["admin_group_id"] + for meeting_dict in intersection_meetings.values() + ] + return { + *{ + mu_id + for group_id, group in self.datastore.get_many( + [ + GetManyRequest( + "group", group_ids, ["meeting_user_ids", "permissions"] + ) + ], + lock_result=False, + ) + .get("group", {}) + .items() + if ( + "user.can_update" in group.get("permissions", []) + or "user.can_manage" in group.get("permissions", []) + ) + for mu_id in group.get("meeting_user_ids", []) + }, + *{ + mu_id + for group_id, group in self.datastore.get_many( + [GetManyRequest("group", admin_group_ids, ["meeting_user_ids"])], + lock_result=False, + ) + .get("group", {}) + .items() + for mu_id in group.get("meeting_user_ids", []) + }, + } + + def _collect_admin_users(self, meeting_user_ids: set[int]) -> dict[int, set[int]]: + """Returns the corresponding users of the groups meeting_users in a defaultdict meeting_id: user_ids.""" + meeting_to_admin_user = defaultdict(set) + for meeting_user in ( + self.datastore.get_many( [ GetManyRequest( "meeting_user", - admin_group.get("meeting_user_ids", []), - ["user_id"], + list(meeting_user_ids), + ["user_id", "meeting_id"], ) ], lock_result=False, - ).get("meeting_user", {}) - group_ids = meeting_dict.get("group_ids", []) - groups = self.datastore.get_many( - [ - GetManyRequest( - "group", group_ids, ["meeting_user_ids", "permissions"] - ) - ], - lock_result=False, - ).get("group", {}) - for group_id, group in groups.items(): - meeting_user_ids = group.get("meeting_user_ids", []) - group_permissions = group.get("permissions", []) - if meeting_user_ids and ( - "user.can_manage" in group_permissions - or "user.can_update" in group_permissions - ): - admin_meeting_users.update( - self.datastore.get_many( - [ - GetManyRequest( - "meeting_user", - meeting_user_ids, - ["user_id"], - ) - ], - lock_result=False, - ).get("meeting_user", {}) - ) - if admin_meeting_users: - is_admin = False - for admin_meeting_user in admin_meeting_users.values(): - if admin_meeting_user.get("user_id") == instance_id: - return False - if admin_meeting_user.get("user_id") == self.user_id: - is_admin = True - if not is_admin: - return False - else: + ) + .get("meeting_user", {}) + .values() + ): + meeting_to_admin_user[meeting_user["meeting_id"]].add( + meeting_user["user_id"] + ) + return meeting_to_admin_user + + def _analyze_meetings( + self, meeting_to_admin_users: dict[int, set[int]], instance_id: int + ) -> bool: + for meeting_id, admin_users in meeting_to_admin_users.items(): + if instance_id in admin_users: + return False + if self.user_id not in admin_users: return False return True diff --git a/tests/system/action/user/scope_permissions_mixin.py b/tests/system/action/user/scope_permissions_mixin.py index f0ddc4162..cbe016abc 100644 --- a/tests/system/action/user/scope_permissions_mixin.py +++ b/tests/system/action/user/scope_permissions_mixin.py @@ -32,6 +32,26 @@ def setup_admin_scope_permissions( self.set_organization_management_level(None) self.set_user_groups(1, [3]) self.set_group_permissions(3, [meeting_permission]) + self.set_models( + { + "user/777": { + "username": "admin_group_filler", + "meeting_user_ids": [666, 667], + }, + "meeting_user/666": { + "group_ids": [12, 23], + "meeting_id": 1, + "user_id": 777, + }, + "meeting_user/667": { + "group_ids": [12, 23], + "meeting_id": 2, + "user_id": 777, + }, + "group/12": {"meeting_user_ids": [666]}, + "group/23": {"meeting_user_ids": [667]}, + } + ) def setup_scoped_user(self, scope: UserScope) -> None: """ @@ -72,9 +92,9 @@ def setup_scoped_user(self, scope: UserScope) -> None: "group_ids": [22], }, "group/11": {"meeting_id": 1, "meeting_user_ids": [11]}, - "group/12": {"meeting_id": 1}, + "group/12": {"meeting_id": 1, "meeting_user_ids": [666]}, "group/22": {"meeting_id": 2, "meeting_user_ids": [22]}, - "group/23": {"meeting_id": 2}, + "group/23": {"meeting_id": 2, "meeting_user_ids": [667]}, } ) elif scope == UserScope.Committee: diff --git a/tests/system/action/user/test_delete.py b/tests/system/action/user/test_delete.py index 954802bf5..7a3c6a9b5 100644 --- a/tests/system/action/user/test_delete.py +++ b/tests/system/action/user/test_delete.py @@ -427,7 +427,7 @@ def test_delete_scope_organization_no_permission(self) -> None: response = self.request("user.delete", {"id": 111}) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.delete. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + "You are not allowed to perform action user.delete. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 2", response.json["message"], ) @@ -477,7 +477,7 @@ def test_delete_scope_organization_permission_in_meeting(self) -> None: response = self.request("user.delete", {"id": 111}) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.delete. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + "You are not allowed to perform action user.delete. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 2", response.json["message"], ) diff --git a/tests/system/action/user/test_generate_new_password.py b/tests/system/action/user/test_generate_new_password.py index 922b99fc6..bae8c8fcc 100644 --- a/tests/system/action/user/test_generate_new_password.py +++ b/tests/system/action/user/test_generate_new_password.py @@ -111,7 +111,7 @@ def test_scope_organization_no_permission(self) -> None: response = self.request("user.generate_new_password", {"id": 111}) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.generate_new_password. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + "You are not allowed to perform action user.generate_new_password. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 2", response.json["message"], ) @@ -165,7 +165,7 @@ def test_scope_organization_permission_in_meeting(self) -> None: response = self.request("user.generate_new_password", {"id": 111}) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.generate_new_password. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + "You are not allowed to perform action user.generate_new_password. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 2", response.json["message"], ) diff --git a/tests/system/action/user/test_reset_password_to_default.py b/tests/system/action/user/test_reset_password_to_default.py index dbd33d406..491f01742 100644 --- a/tests/system/action/user/test_reset_password_to_default.py +++ b/tests/system/action/user/test_reset_password_to_default.py @@ -118,7 +118,7 @@ def test_scope_organization_no_permission(self) -> None: response = self.request("user.reset_password_to_default", {"id": 111}) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.reset_password_to_default. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + "You are not allowed to perform action user.reset_password_to_default. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 2", response.json["message"], ) @@ -172,7 +172,7 @@ def test_scope_organization_permission_in_meeting(self) -> None: response = self.request("user.reset_password_to_default", {"id": 111}) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.reset_password_to_default. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + "You are not allowed to perform action user.reset_password_to_default. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 2", response.json["message"], ) diff --git a/tests/system/action/user/test_set_password.py b/tests/system/action/user/test_set_password.py index 6a23a97c8..e607d801e 100644 --- a/tests/system/action/user/test_set_password.py +++ b/tests/system/action/user/test_set_password.py @@ -28,7 +28,22 @@ def test_two_meetings(self) -> None: self.create_meeting(4) # meeting 4 user_id = self.create_user("test", group_ids=[1]) self.login(user_id) - self.create_model("user/111", {"password": "old_pw"}) + self.set_models( + { + "user/111": {"password": "old_pw"}, + "meeting_user/666": { + "group_ids": [12, 23], + "meeting_id": 12, + "user_id": 1, + }, + } + ) + self.update_model( + "user/1", + {"meeting_user_ids": [666]}, + ) + # only to make sure every meeting has an admin at all times + self.set_user_groups(1, [2, 5]) # Admin groups of meeting/1 for test user meeting/2 as normal user self.set_user_groups(user_id, [2, 4]) # 111 into both meetings @@ -38,7 +53,7 @@ def test_two_meetings(self) -> None: ) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.set_password. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + "You are not allowed to perform action user.set_password. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 4", response.json["message"], ) model = self.get_model("user/111") @@ -52,7 +67,7 @@ def test_two_meetings(self) -> None: ) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.set_password. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + "You are not allowed to perform action user.set_password. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 4", response.json["message"], ) model = self.get_model("user/111") @@ -189,7 +204,7 @@ def test_scope_organization_no_permission(self) -> None: ) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.set_password. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + "You are not allowed to perform action user.set_password. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 2", response.json["message"], ) @@ -251,7 +266,7 @@ def test_scope_organization_permission_in_meeting(self) -> None: ) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.set_password. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + "You are not allowed to perform action user.set_password. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 2", response.json["message"], ) diff --git a/tests/system/action/user/test_update.py b/tests/system/action/user/test_update.py index 8618b8236..cfe47db40 100644 --- a/tests/system/action/user/test_update.py +++ b/tests/system/action/user/test_update.py @@ -30,7 +30,7 @@ def two_meetings_test_fail_ADEFGH( ) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.update. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + "You are not allowed to perform action user.update. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 4", response.json["message"], ) self.assert_model_exists( @@ -99,7 +99,7 @@ def two_meetings_test_fail_ADEFGH( ) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.update. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + "You are not allowed to perform action user.update. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 4", response.json["message"], ) self.assert_model_exists( @@ -1139,7 +1139,7 @@ def test_perm_group_A_belongs_to_same_meetings_both_admin(self) -> None: ) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.update. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + "You are not allowed to perform action user.update. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 4", response.json["message"], ) self.assert_model_exists( @@ -1184,7 +1184,7 @@ def test_perm_group_A_belongs_to_same_meetings_committee_admin(self) -> None: ) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.update. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + "You are not allowed to perform action user.update. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 4", response.json["message"], ) self.assert_model_exists( @@ -1194,6 +1194,18 @@ def test_perm_group_A_belongs_to_same_meetings_committee_admin(self) -> None: }, ) + def test_perm_group_A_belongs_to_same_meetings_multiple_missing(self) -> None: + """May not update group A fields on any scope as long as admin user Ann belongs + to all meetings user Ben belongs to but Ben is committee admin. See issue 2522. + """ + self.permission_setup() # meeting 1 + logged in test user + user 111 + self.create_meeting(4) # meeting 4 + # Admin groups of meeting/1 and meeting/4 for test user + self.set_user_groups(self.user_id, [1, 4]) + # 111 into both meetings + self.set_user_groups(111, [1, 4]) + self.two_meetings_test_fail_ADEFGH() + def test_perm_group_A_meeting_manage_user_archived_meeting(self) -> None: self.perm_group_A_meeting_manage_user_archived_meeting( Permissions.User.CAN_UPDATE @@ -1256,7 +1268,7 @@ def test_perm_group_A_no_permission(self) -> None: ) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.update. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + "You are not allowed to perform action user.update. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 4", response.json["message"], ) @@ -1403,7 +1415,7 @@ def test_perm_group_F_with_meeting_scope_across_committees(self) -> None: ) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.update. Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + "You are not allowed to perform action user.update. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 4", response.json["message"], ) self.assert_model_exists( diff --git a/tests/system/presenter/test_get_user_editable.py b/tests/system/presenter/test_get_user_editable.py index 967ed2610..b3f4241d9 100644 --- a/tests/system/presenter/test_get_user_editable.py +++ b/tests/system/presenter/test_get_user_editable.py @@ -147,7 +147,7 @@ def test_with_cml(self) -> None: def test_with_same_meeting(self) -> None: """ - User 2 can be edited because he is only in meetings which User 111 is admin of. + User 5 can be edited because he is only in meetings which User 111 is admin of. User 7 can not be edited because he is in two of the same meetings but User 111 is not admin in all of them. """ self.set_up() @@ -200,7 +200,7 @@ def test_with_same_meeting(self) -> None: status_code, data = self.request( "get_user_editable", { - "user_ids": [2, 3, 4, 5, 6, 7], + "user_ids": [5, 7], "fields": ["first_name", "default_password"], }, ) @@ -208,28 +208,12 @@ def test_with_same_meeting(self) -> None: self.assertEqual( data, { - "2": { - "editable": False, - "message": "Your organization management level is not high enough to change a user with a Level of can_manage_users!", - }, - "3": { - "editable": False, - "message": "Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or CommitteeManagementLevel can_manage in committee 1", - }, - "4": { - "editable": False, - "message": "Missing permission: OrganizationManagementLevel can_manage_users in organization 1", - }, "5": { "editable": True, }, - "6": { - "editable": False, - "message": "Your organization management level is not high enough to change a user with a Level of superadmin!", - }, "7": { "editable": False, - "message": "Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + "message": "Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 7", }, }, ) diff --git a/tests/system/presenter/test_get_user_related_models.py b/tests/system/presenter/test_get_user_related_models.py index 2033df27e..37b602b14 100644 --- a/tests/system/presenter/test_get_user_related_models.py +++ b/tests/system/presenter/test_get_user_related_models.py @@ -153,15 +153,26 @@ def test_two_meetings(self) -> None: ) self.create_meeting_for_two_users(user_id, 111) self.create_meeting_for_two_users(user_id, 111, 4) # meeting 4 + self.set_models( + { + "user/777": {"meeting_user_ids": [666]}, + "meeting_user/666": { + "group_ids": [12, 23], + "meeting_id": 12, + "user_id": 777, + }, + } + ) + self.update_model("group/5", {"meeting_user_ids": [666]}) self.login(user_id) # Admin groups of meeting/1 for executor user meeting/2 as normal user # 111 into both meetings - meeting_user_to_group = {12: 2, 42: 4, 1111: 1, 4111: 4} + meeting_user_to_group = {12: 2, 42: 4, 1111: 1, 4111: 4, 666: 5} self.move_user_to_group(meeting_user_to_group) status_code, data = self.request("get_user_related_models", {"user_ids": [111]}) self.assertEqual(status_code, 403) self.assertEqual( - "Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + "Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 4", data["message"], ) # Admin groups of meeting/1 for executor user @@ -170,7 +181,7 @@ def test_two_meetings(self) -> None: status_code, data = self.request("get_user_related_models", {"user_ids": [111]}) self.assertEqual(status_code, 403) self.assertEqual( - "Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + "Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 4", data["message"], ) # Admin groups of meeting/1 and meeting/4 for executor user From a8085b1d507535e9567224923bb7dbbb2d1592ce Mon Sep 17 00:00:00 2001 From: Hannes Janott Date: Thu, 19 Sep 2024 17:00:35 +0200 Subject: [PATCH 12/19] running black and improving doc --- openslides_backend/presenter/__init__.py | 2 +- .../shared/mixins/user_scope_mixin.py | 19 +++++++++++++++++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/openslides_backend/presenter/__init__.py b/openslides_backend/presenter/__init__.py index be731acba..f06f93cb0 100644 --- a/openslides_backend/presenter/__init__.py +++ b/openslides_backend/presenter/__init__.py @@ -6,8 +6,8 @@ get_forwarding_committees, get_forwarding_meetings, get_history_information, - get_user_editable, get_mediafile_context, + get_user_editable, get_user_related_models, get_user_scope, get_users, diff --git a/openslides_backend/shared/mixins/user_scope_mixin.py b/openslides_backend/shared/mixins/user_scope_mixin.py index 786109660..cb9a5ff9d 100644 --- a/openslides_backend/shared/mixins/user_scope_mixin.py +++ b/openslides_backend/shared/mixins/user_scope_mixin.py @@ -224,6 +224,10 @@ def check_for_admin_in_all_meetings( return self._analyze_meetings(meeting_to_admin_users, instance_id) def _check_not_committee_manager(self, instance_id: int) -> bool: + """ + Helper function used in method check_for_admin_in_all_meetings. + Checks that requested user is not a committee manager. + """ if not (hasattr(self, "name") and self.name == "user.create"): if self.datastore.get( fqid_from_collection_and_id("user", instance_id), @@ -237,7 +241,10 @@ def _check_not_committee_manager(self, instance_id: int) -> bool: def _collect_intersected_meetings( self, b_meeting_ids: set[int] | None ) -> dict[int, Any] | bool: - """Takes the meeting ids to find intersections with the requesting users meetings. Returns False if this is not possible.""" + """ + Helper function used in method check_for_admin_in_all_meetings. + Takes the meeting ids to find intersections with the requesting users meetings. Returns False if this is not possible. + """ if not b_meeting_ids: if not hasattr(self, "instance_committee_meeting_ids"): return False @@ -326,7 +333,10 @@ def _collect_admin_meeting_users( } def _collect_admin_users(self, meeting_user_ids: set[int]) -> dict[int, set[int]]: - """Returns the corresponding users of the groups meeting_users in a defaultdict meeting_id: user_ids.""" + """ + Helper function used in method check_for_admin_in_all_meetings. + Returns the corresponding users of the groups meeting_users in a defaultdict meeting_id: user_ids. + """ meeting_to_admin_user = defaultdict(set) for meeting_user in ( self.datastore.get_many( @@ -350,6 +360,11 @@ def _collect_admin_users(self, meeting_user_ids: set[int]) -> dict[int, set[int] def _analyze_meetings( self, meeting_to_admin_users: dict[int, set[int]], instance_id: int ) -> bool: + """ + Helper function used in method check_for_admin_in_all_meetings. + Compares the admin users of all meetings with the ids of requested user and requesting user. + Requesting user must be admin in all meetings. Requested user cannot be admin in any. + """ for meeting_id, admin_users in meeting_to_admin_users.items(): if instance_id in admin_users: return False From 9f42c46e3e7dcb789aa5a829d42686f43c085546 Mon Sep 17 00:00:00 2001 From: Hannes Janott Date: Thu, 19 Sep 2024 17:37:47 +0200 Subject: [PATCH 13/19] delete unnecessary lines --- openslides_backend/shared/mixins/user_scope_mixin.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/openslides_backend/shared/mixins/user_scope_mixin.py b/openslides_backend/shared/mixins/user_scope_mixin.py index cb9a5ff9d..0bee82aa5 100644 --- a/openslides_backend/shared/mixins/user_scope_mixin.py +++ b/openslides_backend/shared/mixins/user_scope_mixin.py @@ -246,8 +246,6 @@ def _collect_intersected_meetings( Takes the meeting ids to find intersections with the requesting users meetings. Returns False if this is not possible. """ if not b_meeting_ids: - if not hasattr(self, "instance_committee_meeting_ids"): - return False if not ( b_meeting_ids := { m_id From 8ee6d4a4d9e5d4bbfa5fb5a796cf181c1a93741d Mon Sep 17 00:00:00 2001 From: Hannes Janott Date: Wed, 2 Oct 2024 16:03:12 +0200 Subject: [PATCH 14/19] do requested changes --- .../shared/mixins/user_scope_mixin.py | 116 +++++++----------- 1 file changed, 45 insertions(+), 71 deletions(-) diff --git a/openslides_backend/shared/mixins/user_scope_mixin.py b/openslides_backend/shared/mixins/user_scope_mixin.py index 0bee82aa5..041cee074 100644 --- a/openslides_backend/shared/mixins/user_scope_mixin.py +++ b/openslides_backend/shared/mixins/user_scope_mixin.py @@ -203,13 +203,13 @@ def check_for_admin_in_all_meetings( b_meeting_ids: set[int] | None = None, ) -> bool: """ - This is used during a permission check for scope request, user.update/create with payload fields A and F and during - user altering actions like user.delete or set_default_password. - Checks if the requesting user has permissions to manage participants in all of requested users meetings - but requested user doesn't have any of these permissions. See backend issue 2522 on github for more details. - Also checks requested user has no committee management rights if a dict was provided instead of an id. - An ID will be provided during user deletion. - Returns true if permissions are given. False if not. Raises no Exceptions. + This function checks the special permission condition for scope request, user.update/create with + payload fields A and F and other user altering actions like user.delete or set_default_password. + This requires all of: + * requested user is no committee manager + * requested user doesn't have any admin/user.can_update/user.can_manage rights in his meetings + * requesting user has those permissions in all of those meetings + Returns True if permissions are given. False if not. """ if not self._check_not_committee_manager(instance_id): return False @@ -220,8 +220,7 @@ def check_for_admin_in_all_meetings( return False assert isinstance(intersection_meetings, dict) admin_meeting_users = self._collect_admin_meeting_users(intersection_meetings) - meeting_to_admin_users = self._collect_admin_users(admin_meeting_users) - return self._analyze_meetings(meeting_to_admin_users, instance_id) + return self._analyze_meeting_admins(admin_meeting_users, instance_id) def _check_not_committee_manager(self, instance_id: int) -> bool: """ @@ -245,15 +244,14 @@ def _collect_intersected_meetings( Helper function used in method check_for_admin_in_all_meetings. Takes the meeting ids to find intersections with the requesting users meetings. Returns False if this is not possible. """ - if not b_meeting_ids: - if not ( - b_meeting_ids := { - m_id - for m_ids in self.instance_committee_meeting_ids.values() - for m_id in m_ids - } - ): - return False + if not b_meeting_ids and not ( + b_meeting_ids := { + m_id + for m_ids in self.instance_committee_meeting_ids.values() + for m_id in m_ids + } + ): + return False # During participant import there is no permstore. if hasattr(self, "permstore"): a_meeting_ids = ( @@ -295,53 +293,41 @@ def _collect_admin_meeting_users( for meeting_id, meeting_dict in intersection_meetings.items() for group_id in meeting_dict.get("group_ids", []) ] - admin_group_ids = [ - meeting_dict["admin_group_id"] - for meeting_dict in intersection_meetings.values() - ] return { - *{ - mu_id - for group_id, group in self.datastore.get_many( - [ - GetManyRequest( - "group", group_ids, ["meeting_user_ids", "permissions"] - ) - ], - lock_result=False, - ) - .get("group", {}) - .items() - if ( - "user.can_update" in group.get("permissions", []) - or "user.can_manage" in group.get("permissions", []) - ) - for mu_id in group.get("meeting_user_ids", []) - }, - *{ - mu_id - for group_id, group in self.datastore.get_many( - [GetManyRequest("group", admin_group_ids, ["meeting_user_ids"])], - lock_result=False, - ) - .get("group", {}) - .items() - for mu_id in group.get("meeting_user_ids", []) - }, + mu_id + for group_id, group in self.datastore.get_many( + [ + GetManyRequest( + "group", group_ids, ["meeting_user_ids", "permissions", "admin_group_for_meeting_id"] + ) + ], + lock_result=False, + ) + .get("group", {}) + .items() + if ( + group.get("admin_group_for_meeting_id") + or "user.can_update" in group.get("permissions", []) + or "user.can_manage" in group.get("permissions", []) + ) + for mu_id in group.get("meeting_user_ids", []) } - def _collect_admin_users(self, meeting_user_ids: set[int]) -> dict[int, set[int]]: + def _analyze_meeting_admins( + self, admin_meeting_user_ids: set[int], requested_user_id: int + ) -> bool: """ Helper function used in method check_for_admin_in_all_meetings. - Returns the corresponding users of the groups meeting_users in a defaultdict meeting_id: user_ids. + Compares the users of admin meeting users of all meetings with the ids of requested user and requesting user. + Requesting user must be admin in all meetings. Requested user cannot be admin in any. """ - meeting_to_admin_user = defaultdict(set) + meeting_to_admin_user_ids = defaultdict(set) for meeting_user in ( self.datastore.get_many( [ GetManyRequest( "meeting_user", - list(meeting_user_ids), + list(admin_meeting_user_ids), ["user_id", "meeting_id"], ) ], @@ -350,22 +336,10 @@ def _collect_admin_users(self, meeting_user_ids: set[int]) -> dict[int, set[int] .get("meeting_user", {}) .values() ): - meeting_to_admin_user[meeting_user["meeting_id"]].add( + meeting_to_admin_user_ids[meeting_user["meeting_id"]].add( meeting_user["user_id"] ) - return meeting_to_admin_user - - def _analyze_meetings( - self, meeting_to_admin_users: dict[int, set[int]], instance_id: int - ) -> bool: - """ - Helper function used in method check_for_admin_in_all_meetings. - Compares the admin users of all meetings with the ids of requested user and requesting user. - Requesting user must be admin in all meetings. Requested user cannot be admin in any. - """ - for meeting_id, admin_users in meeting_to_admin_users.items(): - if instance_id in admin_users: - return False - if self.user_id not in admin_users: - return False - return True + return not any( + requested_user_id in admin_users or self.user_id not in admin_users + for meeting_id, admin_users in meeting_to_admin_user_ids.items() + ) From e2e569d92c5a9b7c901d4666af8055cf06da9f0b Mon Sep 17 00:00:00 2001 From: Hannes Janott Date: Mon, 7 Oct 2024 13:15:02 +0200 Subject: [PATCH 15/19] coding style --- .../shared/mixins/user_scope_mixin.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/openslides_backend/shared/mixins/user_scope_mixin.py b/openslides_backend/shared/mixins/user_scope_mixin.py index 041cee074..d14ccd4f3 100644 --- a/openslides_backend/shared/mixins/user_scope_mixin.py +++ b/openslides_backend/shared/mixins/user_scope_mixin.py @@ -203,7 +203,7 @@ def check_for_admin_in_all_meetings( b_meeting_ids: set[int] | None = None, ) -> bool: """ - This function checks the special permission condition for scope request, user.update/create with + This function checks the special permission condition for scope request, user.update/create with payload fields A and F and other user altering actions like user.delete or set_default_password. This requires all of: * requested user is no committee manager @@ -298,7 +298,13 @@ def _collect_admin_meeting_users( for group_id, group in self.datastore.get_many( [ GetManyRequest( - "group", group_ids, ["meeting_user_ids", "permissions", "admin_group_for_meeting_id"] + "group", + group_ids, + [ + "meeting_user_ids", + "permissions", + "admin_group_for_meeting_id", + ], ) ], lock_result=False, @@ -340,6 +346,6 @@ def _analyze_meeting_admins( meeting_user["user_id"] ) return not any( - requested_user_id in admin_users or self.user_id not in admin_users - for meeting_id, admin_users in meeting_to_admin_user_ids.items() + requested_user_id in admin_users or self.user_id not in admin_users + for meeting_id, admin_users in meeting_to_admin_user_ids.items() ) From e6fc78f35470da94385c30f4984f8777be229f2d Mon Sep 17 00:00:00 2001 From: Hannes Janott Date: Mon, 7 Oct 2024 16:42:43 +0200 Subject: [PATCH 16/19] Improve method description Co-authored-by: luisa-beerboom <101706784+luisa-beerboom@users.noreply.github.com> --- openslides_backend/shared/mixins/user_scope_mixin.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/openslides_backend/shared/mixins/user_scope_mixin.py b/openslides_backend/shared/mixins/user_scope_mixin.py index d14ccd4f3..7abffc669 100644 --- a/openslides_backend/shared/mixins/user_scope_mixin.py +++ b/openslides_backend/shared/mixins/user_scope_mixin.py @@ -205,11 +205,10 @@ def check_for_admin_in_all_meetings( """ This function checks the special permission condition for scope request, user.update/create with payload fields A and F and other user altering actions like user.delete or set_default_password. - This requires all of: - * requested user is no committee manager - * requested user doesn't have any admin/user.can_update/user.can_manage rights in his meetings + This function returns true if: + * requested user is no committee manager and + * requested user doesn't have any admin/user.can_update/user.can_manage rights in his meetings and * requesting user has those permissions in all of those meetings - Returns True if permissions are given. False if not. """ if not self._check_not_committee_manager(instance_id): return False From e2582d6b2194d62716be7d1067e3f4035df40e04 Mon Sep 17 00:00:00 2001 From: Hannes Janott Date: Mon, 7 Oct 2024 18:46:58 +0200 Subject: [PATCH 17/19] add plural to MissingPermission permission output --- openslides_backend/shared/exceptions.py | 25 ++++++++++++++----- .../user_create_update_permissions_mixin.py | 12 ++++----- tests/system/action/user/test_update.py | 12 ++++----- .../presenter/test_get_user_editable.py | 2 +- 4 files changed, 32 insertions(+), 19 deletions(-) diff --git a/openslides_backend/shared/exceptions.py b/openslides_backend/shared/exceptions.py index 59404c29a..d53bf92ce 100644 --- a/openslides_backend/shared/exceptions.py +++ b/openslides_backend/shared/exceptions.py @@ -131,16 +131,29 @@ def __init__(self, action_name: str) -> None: class MissingPermission(PermissionDenied): def __init__( self, - permissions: AnyPermission | dict[AnyPermission, int], + permissions: AnyPermission | dict[AnyPermission, int | set[int]], ) -> None: if isinstance(permissions, dict): - self.message = ( - "Missing permission" + ("s" if len(permissions) > 1 else "") + ": " - ) + to_remove = [] + for permission, id_or_ids in permissions.items(): + if isinstance(id_or_ids, set) and not id_or_ids: + to_remove.append(permission) + for permission in to_remove: + del permissions[permission] + self.message = "Missing permission" + self._plural_s(permissions) + ": " self.message += " or ".join( - f"{permission.get_verbose_type()} {permission} in {permission.get_base_model()} {id}" - for permission, id in permissions.items() + f"{permission.get_verbose_type()} {permission} in {permission.get_base_model()}{self._plural_s(id_or_ids)} {id_or_ids}" + for permission, id_or_ids in permissions.items() ) else: self.message = f"Missing {permissions.get_verbose_type()}: {permissions}" super().__init__(self.message) + + def _plural_s(self, permission_or_id_or_ids: int | set[int] | dict) -> str: + if isinstance(permission_or_id_or_ids, set) or ( + isinstance(permission_or_id_or_ids, dict) + and len(permission_or_id_or_ids) > 1 + ): + return "s" + else: + return "" diff --git a/openslides_backend/shared/mixins/user_create_update_permissions_mixin.py b/openslides_backend/shared/mixins/user_create_update_permissions_mixin.py index c2776fc86..0fe562b4a 100644 --- a/openslides_backend/shared/mixins/user_create_update_permissions_mixin.py +++ b/openslides_backend/shared/mixins/user_create_update_permissions_mixin.py @@ -276,8 +276,8 @@ def check_group_A(self, fields: list[str], instance: dict[str, Any]) -> None: raise MissingPermission( { OrganizationManagementLevel.CAN_MANAGE_USERS: 1, - **{ - Permissions.User.CAN_UPDATE: meeting_id + Permissions.User.CAN_UPDATE: { + meeting_id for meeting_ids in self.instance_committee_meeting_ids.values() if meeting_ids is not None for meeting_id in meeting_ids @@ -294,8 +294,8 @@ def check_group_A(self, fields: list[str], instance: dict[str, Any]) -> None: { OrganizationManagementLevel.CAN_MANAGE_USERS: 1, CommitteeManagementLevel.CAN_MANAGE: self.instance_user_scope_id, - **{ - Permissions.User.CAN_UPDATE: meeting_id + Permissions.User.CAN_UPDATE: { + meeting_id for meeting_ids in self.instance_committee_meeting_ids.values() if meeting_ids is not None for meeting_id in meeting_ids @@ -412,8 +412,8 @@ def check_group_F(self, fields: list[str], instance: dict[str, Any]) -> None: raise MissingPermission( { expected_oml_permission: 1, - **{ - Permissions.User.CAN_UPDATE: meeting_id + Permissions.User.CAN_UPDATE: { + meeting_id for meeting_ids in self.instance_committee_meeting_ids.values() if meeting_ids is not None for meeting_id in meeting_ids diff --git a/tests/system/action/user/test_update.py b/tests/system/action/user/test_update.py index 5cc0c7753..30b4502e6 100644 --- a/tests/system/action/user/test_update.py +++ b/tests/system/action/user/test_update.py @@ -31,7 +31,7 @@ def two_meetings_test_fail_ADEFGH( ) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.update. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 4", + "You are not allowed to perform action user.update. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meetings {1, 4}", response.json["message"], ) self.assert_model_exists( @@ -100,7 +100,7 @@ def two_meetings_test_fail_ADEFGH( ) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.update. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 4", + "You are not allowed to perform action user.update. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meetings {1, 4}", response.json["message"], ) self.assert_model_exists( @@ -1146,7 +1146,7 @@ def test_perm_group_A_belongs_to_same_meetings_both_admin(self) -> None: ) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.update. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 4", + "You are not allowed to perform action user.update. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meetings {1, 4}", response.json["message"], ) self.assert_model_exists( @@ -1191,7 +1191,7 @@ def test_perm_group_A_belongs_to_same_meetings_committee_admin(self) -> None: ) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.update. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 4", + "You are not allowed to perform action user.update. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meetings {1, 4}", response.json["message"], ) self.assert_model_exists( @@ -1275,7 +1275,7 @@ def test_perm_group_A_no_permission(self) -> None: ) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.update. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 4", + "You are not allowed to perform action user.update. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meetings {1, 4}", response.json["message"], ) @@ -1421,7 +1421,7 @@ def test_perm_group_F_with_meeting_scope_across_committees(self) -> None: ) self.assert_status_code(response, 403) self.assertIn( - "You are not allowed to perform action user.update. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 4", + "You are not allowed to perform action user.update. Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meetings {1, 4}", response.json["message"], ) self.assert_model_exists( diff --git a/tests/system/presenter/test_get_user_editable.py b/tests/system/presenter/test_get_user_editable.py index b3f4241d9..7113eb8eb 100644 --- a/tests/system/presenter/test_get_user_editable.py +++ b/tests/system/presenter/test_get_user_editable.py @@ -213,7 +213,7 @@ def test_with_same_meeting(self) -> None: }, "7": { "editable": False, - "message": "Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meeting 7", + "message": "Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meetings {10, 7}", }, }, ) From 576b3f4c10766254be5db9c578206b3fc60a228a Mon Sep 17 00:00:00 2001 From: Hannes Janott Date: Tue, 8 Oct 2024 10:05:24 +0200 Subject: [PATCH 18/19] fix plural error in exception --- openslides_backend/shared/exceptions.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/openslides_backend/shared/exceptions.py b/openslides_backend/shared/exceptions.py index d53bf92ce..9ac22f1df 100644 --- a/openslides_backend/shared/exceptions.py +++ b/openslides_backend/shared/exceptions.py @@ -150,10 +150,10 @@ def __init__( super().__init__(self.message) def _plural_s(self, permission_or_id_or_ids: int | set[int] | dict) -> str: - if isinstance(permission_or_id_or_ids, set) or ( - isinstance(permission_or_id_or_ids, dict) - and len(permission_or_id_or_ids) > 1 - ): + if ( + isinstance(permission_or_id_or_ids, set) + or (isinstance(permission_or_id_or_ids, dict)) + ) and len(permission_or_id_or_ids) > 1: return "s" else: return "" From 29a0e4841f04ae623907b8d60ffe4ca437a08dea Mon Sep 17 00:00:00 2001 From: Hannes Janott Date: Mon, 14 Oct 2024 14:26:22 +0200 Subject: [PATCH 19/19] change get editable presenter on a per field basis response and a per payload fields group permission check. --- docs/presenters/get_user_editable.md | 7 +- global/meta | 2 +- openslides_backend/models/models.py | 91 +++++---- .../presenter/get_user_editable.py | 44 +++- openslides_backend/shared/exceptions.py | 2 +- .../presenter/test_get_user_editable.py | 193 +++++++++++++++--- 6 files changed, 245 insertions(+), 94 deletions(-) diff --git a/docs/presenters/get_user_editable.md b/docs/presenters/get_user_editable.md index 177221c0d..00f61489b 100644 --- a/docs/presenters/get_user_editable.md +++ b/docs/presenters/get_user_editable.md @@ -12,8 +12,11 @@ ``` { user_id: Id: { - editable: boolean // true if user can be updated or deleted - message?: string // error message if an exception was caught + field: str: ( + editable: boolean // true if user can be updated or deleted, + message?: string // error message if an exception was caught + ), + ... }, ... } diff --git a/global/meta b/global/meta index e2e3395d0..08d0088eb 160000 --- a/global/meta +++ b/global/meta @@ -1 +1 @@ -Subproject commit e2e3395d07d03ef0089747e2ba94557131a75768 +Subproject commit 08d0088ebf955ebdfda2fe512d9353ae942944c4 diff --git a/openslides_backend/models/models.py b/openslides_backend/models/models.py index 15487147d..a06c12ed6 100644 --- a/openslides_backend/models/models.py +++ b/openslides_backend/models/models.py @@ -9,7 +9,7 @@ class Organization(Model): collection = "organization" verbose_name = "organization" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) name = fields.CharField() description = fields.HTMLStrictField() legal_notice = fields.TextField() @@ -37,6 +37,7 @@ class Organization(Model): required=True, constraints={"enum": ["en", "de", "it", "es", "ru", "cs", "fr"]} ) require_duplicate_from = fields.BooleanField() + enable_anonymous = fields.BooleanField() saml_enabled = fields.BooleanField() saml_login_button_text = fields.CharField(default="SAML login") saml_attr_mapping = fields.JSONField() @@ -80,7 +81,7 @@ class User(Model): collection = "user" verbose_name = "user" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) username = fields.CharField(required=True) member_number = fields.CharField() saml_id = fields.CharField( @@ -206,7 +207,7 @@ class Gender(Model): collection = "gender" verbose_name = "gender" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) name = fields.CharField(required=True, constraints={"description": "unique"}) organization_id = fields.OrganizationField( to={"organization": "gender_ids"}, required=True @@ -218,7 +219,7 @@ class OrganizationTag(Model): collection = "organization_tag" verbose_name = "organization tag" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) name = fields.CharField(required=True) color = fields.ColorField(required=True) tagged_ids = fields.GenericRelationListField( @@ -291,7 +292,7 @@ class Committee(Model): collection = "committee" verbose_name = "committee" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) name = fields.CharField(required=True) description = fields.HTMLStrictField() external_id = fields.CharField(constraints={"description": "unique"}) @@ -326,7 +327,7 @@ class Meeting(Model, MeetingModelMixin): collection = "meeting" verbose_name = "meeting" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) external_id = fields.CharField(constraints={"description": "unique in committee"}) welcome_title = fields.CharField(default="Welcome to OpenSlides") welcome_text = fields.HTMLPermissiveField(default="Space for your welcome text.") @@ -933,7 +934,7 @@ class StructureLevel(Model): collection = "structure_level" verbose_name = "structure level" - id = fields.IntegerField(required=True) + id = fields.IntegerField(required=True, constant=True) name = fields.CharField(required=True) color = fields.ColorField() default_time = fields.IntegerField(constraints={"minimum": 0}) @@ -953,7 +954,7 @@ class Group(Model): collection = "group" verbose_name = "group" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) external_id = fields.CharField(constraints={"description": "unique in meeting"}) name = fields.CharField(required=True) permissions = fields.CharArrayField( @@ -1060,7 +1061,7 @@ class PersonalNote(Model): collection = "personal_note" verbose_name = "personal note" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) note = fields.HTMLStrictField() star = fields.BooleanField() meeting_user_id = fields.RelationField( @@ -1081,7 +1082,7 @@ class Tag(Model): collection = "tag" verbose_name = "tag" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) name = fields.CharField(required=True) tagged_ids = fields.GenericRelationListField( to={"agenda_item": "tag_ids", "assignment": "tag_ids", "motion": "tag_ids"}, @@ -1096,7 +1097,7 @@ class AgendaItem(Model, AgendaItemModelMixin): collection = "agenda_item" verbose_name = "agenda item" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) item_number = fields.CharField() comment = fields.CharField() closed = fields.BooleanField(default=False) @@ -1151,7 +1152,7 @@ class ListOfSpeakers(Model): collection = "list_of_speakers" verbose_name = "list of speakers" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) closed = fields.BooleanField(default=False) sequential_number = fields.IntegerField( required=True, @@ -1197,7 +1198,7 @@ class StructureLevelListOfSpeakers(Model): collection = "structure_level_list_of_speakers" verbose_name = "structure level list of speakers" - id = fields.IntegerField(required=True) + id = fields.IntegerField(required=True, constant=True) structure_level_id = fields.RelationField( to={"structure_level": "structure_level_list_of_speakers_ids"}, required=True, @@ -1243,7 +1244,7 @@ class PointOfOrderCategory(Model): collection = "point_of_order_category" verbose_name = "point of order category" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) text = fields.CharField(required=True) rank = fields.IntegerField(required=True) meeting_id = fields.RelationField( @@ -1258,7 +1259,7 @@ class Speaker(Model): collection = "speaker" verbose_name = "speaker" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) begin_time = fields.TimestampField() end_time = fields.TimestampField() pause_time = fields.TimestampField(read_only=True) @@ -1303,7 +1304,7 @@ class Topic(Model): collection = "topic" verbose_name = "topic" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) title = fields.CharField(required=True) text = fields.HTMLPermissiveField() sequential_number = fields.IntegerField( @@ -1350,7 +1351,7 @@ class Motion(Model): collection = "motion" verbose_name = "motion" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) number = fields.CharField() number_value = fields.IntegerField( read_only=True, @@ -1509,7 +1510,7 @@ class MotionSubmitter(Model): collection = "motion_submitter" verbose_name = "motion submitter" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) weight = fields.IntegerField() meeting_user_id = fields.RelationField( to={"meeting_user": "motion_submitter_ids"}, required=True @@ -1529,7 +1530,7 @@ class MotionEditor(Model): collection = "motion_editor" verbose_name = "motion editor" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) weight = fields.IntegerField() meeting_user_id = fields.RelationField( to={"meeting_user": "motion_editor_ids"}, required=True @@ -1549,7 +1550,7 @@ class MotionWorkingGroupSpeaker(Model): collection = "motion_working_group_speaker" verbose_name = "motion working group speaker" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) weight = fields.IntegerField() meeting_user_id = fields.RelationField( to={"meeting_user": "motion_working_group_speaker_ids"}, required=True @@ -1569,7 +1570,7 @@ class MotionComment(Model): collection = "motion_comment" verbose_name = "motion comment" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) comment = fields.HTMLStrictField() motion_id = fields.RelationField( to={"motion": "comment_ids"}, @@ -1592,7 +1593,7 @@ class MotionCommentSection(Model): collection = "motion_comment_section" verbose_name = "motion comment section" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) name = fields.CharField(required=True) weight = fields.IntegerField(default=10000) sequential_number = fields.IntegerField( @@ -1624,7 +1625,7 @@ class MotionCategory(Model): collection = "motion_category" verbose_name = "motion category" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) name = fields.CharField(required=True) prefix = fields.CharField() weight = fields.IntegerField(default=10000) @@ -1657,7 +1658,7 @@ class MotionBlock(Model): collection = "motion_block" verbose_name = "motion block" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) title = fields.CharField(required=True) internal = fields.BooleanField() sequential_number = fields.IntegerField( @@ -1696,7 +1697,7 @@ class MotionChangeRecommendation(Model): collection = "motion_change_recommendation" verbose_name = "motion change recommendation" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) rejected = fields.BooleanField(default=False) internal = fields.BooleanField(default=False) type = fields.CharField( @@ -1723,7 +1724,7 @@ class MotionState(Model): collection = "motion_state" verbose_name = "motion state" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) name = fields.CharField(required=True) weight = fields.IntegerField(required=True) recommendation_label = fields.CharField() @@ -1797,7 +1798,7 @@ class MotionWorkflow(Model): collection = "motion_workflow" verbose_name = "motion workflow" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) name = fields.CharField(required=True) sequential_number = fields.IntegerField( required=True, @@ -1835,7 +1836,7 @@ class MotionStatuteParagraph(Model): collection = "motion_statute_paragraph" verbose_name = "motion statute paragraph" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) title = fields.CharField(required=True) text = fields.HTMLStrictField() weight = fields.IntegerField(default=10000) @@ -1859,7 +1860,7 @@ class Poll(Model, PollModelMixin): collection = "poll" verbose_name = "poll" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) description = fields.TextField() title = fields.CharField(required=True) type = fields.CharField( @@ -1962,7 +1963,7 @@ class Option(Model): collection = "option" verbose_name = "option" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) weight = fields.IntegerField(default=10000) text = fields.HTMLStrictField() yes = fields.DecimalField() @@ -1997,7 +1998,7 @@ class Vote(Model): collection = "vote" verbose_name = "vote" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) weight = fields.DecimalField(constant=True) value = fields.CharField(constant=True) user_token = fields.CharField(required=True, constant=True) @@ -2018,7 +2019,7 @@ class Assignment(Model): collection = "assignment" verbose_name = "assignment" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) title = fields.CharField(required=True) description = fields.HTMLStrictField() open_posts = fields.IntegerField(default=0, constraints={"minimum": 0}) @@ -2077,7 +2078,7 @@ class AssignmentCandidate(Model): collection = "assignment_candidate" verbose_name = "assignment candidate" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) weight = fields.IntegerField(default=10000) assignment_id = fields.RelationField( to={"assignment": "candidate_ids"}, @@ -2097,7 +2098,7 @@ class PollCandidateList(Model): collection = "poll_candidate_list" verbose_name = "poll candidate list" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) poll_candidate_ids = fields.RelationListField( to={"poll_candidate": "poll_candidate_list_id"}, on_delete=fields.OnDelete.CASCADE, @@ -2118,7 +2119,7 @@ class PollCandidate(Model): collection = "poll_candidate" verbose_name = "poll candidate" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) poll_candidate_list_id = fields.RelationField( to={"poll_candidate_list": "poll_candidate_ids"}, required=True, @@ -2136,7 +2137,7 @@ class Mediafile(Model): collection = "mediafile" verbose_name = "mediafile" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) title = fields.CharField( constraints={"description": "Title and parent_id must be unique."} ) @@ -2177,7 +2178,7 @@ class MeetingMediafile(Model): collection = "meeting_mediafile" verbose_name = "meeting mediafile" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) mediafile_id = fields.RelationField( to={"mediafile": "meeting_mediafile_ids"}, required=True ) @@ -2266,7 +2267,7 @@ class Projector(Model): collection = "projector" verbose_name = "projector" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) name = fields.CharField() is_internal = fields.BooleanField(default=False) scale = fields.IntegerField(default=0) @@ -2368,7 +2369,7 @@ class Projection(Model): collection = "projection" verbose_name = "projection" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) options = fields.JSONField() stable = fields.BooleanField(default=False) weight = fields.IntegerField() @@ -2409,7 +2410,7 @@ class ProjectorMessage(Model): collection = "projector_message" verbose_name = "projector message" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) message = fields.HTMLStrictField() projection_ids = fields.RelationListField( to={"projection": "content_object_id"}, @@ -2425,7 +2426,7 @@ class ProjectorCountdown(Model): collection = "projector_countdown" verbose_name = "projector countdown" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) title = fields.CharField(required=True) description = fields.CharField(default="") default_time = fields.IntegerField() @@ -2451,7 +2452,7 @@ class ChatGroup(Model): collection = "chat_group" verbose_name = "chat group" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) name = fields.CharField(required=True) weight = fields.IntegerField(default=10000) chat_message_ids = fields.RelationListField( @@ -2474,7 +2475,7 @@ class ChatMessage(Model): collection = "chat_message" verbose_name = "chat message" - id = fields.IntegerField(constant=True) + id = fields.IntegerField(required=True, constant=True) content = fields.HTMLStrictField(required=True) created = fields.TimestampField(required=True) meeting_user_id = fields.RelationField( @@ -2492,7 +2493,7 @@ class ActionWorker(Model): collection = "action_worker" verbose_name = "action worker" - id = fields.IntegerField() + id = fields.IntegerField(required=True, constant=True) name = fields.CharField(required=True) state = fields.CharField( required=True, constraints={"enum": ["running", "end", "aborted"]} @@ -2506,7 +2507,7 @@ class ImportPreview(Model): collection = "import_preview" verbose_name = "import preview" - id = fields.IntegerField() + id = fields.IntegerField(required=True, constant=True) name = fields.CharField( required=True, constraints={ diff --git a/openslides_backend/presenter/get_user_editable.py b/openslides_backend/presenter/get_user_editable.py index efe2fcad7..578c10ff8 100644 --- a/openslides_backend/presenter/get_user_editable.py +++ b/openslides_backend/presenter/get_user_editable.py @@ -1,3 +1,4 @@ +from collections import defaultdict from typing import Any import fastjsonschema @@ -37,7 +38,7 @@ @register_presenter("get_user_editable") class GetUserEditable(CreateUpdatePermissionsMixin, BasePresenter): """ - Checks for each user whether it is editable by calling user. + Checks for each given user whether the given fields are editable by calling user on a per payload group basis. """ schema = get_user_editable_schema @@ -45,19 +46,40 @@ class GetUserEditable(CreateUpdatePermissionsMixin, BasePresenter): permission = Permissions.User.CAN_MANAGE def get_result(self) -> Any: - result: dict[str, Any] = {} - if not self.data["fields"]: + if "fields" not in self.data or not self.data["fields"]: raise PresenterException( "Need at least one field name to check editability." ) - instance = {field_name: "" for field_name in self.data["fields"]} + reversed_field_rights = { + field: group + for group, fields in self.field_rights.items() + for field in fields + } + one_field_per_group = { + group_fields[0] + for field_name in self.data["fields"] + for group_fields in self.field_rights.values() + if field_name in group_fields + } + result: defaultdict[str, dict[str, tuple[bool, str]]] = defaultdict(dict) for user_id in self.data["user_ids"]: - instance.update({"id": user_id}) result[str(user_id)] = {} - try: - self.check_permissions(instance) - result[str(user_id)]["editable"] = True - except (PermissionDenied, MissingPermission, ActionException) as e: - result[str(user_id)]["editable"] = False - result[str(user_id)]["message"] = e.message + groups_editable = {} + for field_name in one_field_per_group: + try: + self.check_permissions({"id": user_id, field_name: ""}) + groups_editable[reversed_field_rights[field_name]] = (True, "") + except (PermissionDenied, MissingPermission, ActionException) as e: + groups_editable[reversed_field_rights[field_name]] = ( + False, + e.message, + ) + result[str(user_id)].update( + { + data_field_name: groups_editable[ + reversed_field_rights[data_field_name] + ] + for data_field_name in self.data["fields"] + } + ) return result diff --git a/openslides_backend/shared/exceptions.py b/openslides_backend/shared/exceptions.py index 9ac22f1df..94ea1811d 100644 --- a/openslides_backend/shared/exceptions.py +++ b/openslides_backend/shared/exceptions.py @@ -149,7 +149,7 @@ def __init__( self.message = f"Missing {permissions.get_verbose_type()}: {permissions}" super().__init__(self.message) - def _plural_s(self, permission_or_id_or_ids: int | set[int] | dict) -> str: + def _plural_s(self, permission_or_id_or_ids: dict | int | set[int]) -> str: if ( isinstance(permission_or_id_or_ids, set) or (isinstance(permission_or_id_or_ids, dict)) diff --git a/tests/system/presenter/test_get_user_editable.py b/tests/system/presenter/test_get_user_editable.py index 7113eb8eb..edebacb47 100644 --- a/tests/system/presenter/test_get_user_editable.py +++ b/tests/system/presenter/test_get_user_editable.py @@ -80,23 +80,34 @@ def test_with_oml(self) -> None: data, { "2": { - "editable": True, + "default_password": [True, ""], + "first_name": [True, ""], }, "3": { - "editable": True, + "default_password": [True, ""], + "first_name": [True, ""], }, "4": { - "editable": True, + "default_password": [True, ""], + "first_name": [True, ""], }, "5": { - "editable": True, + "default_password": [True, ""], + "first_name": [True, ""], }, "6": { - "editable": False, - "message": "Your organization management level is not high enough to change a user with a Level of superadmin!", + "default_password": [ + False, + "Your organization management level is not high enough to change a user with a Level of superadmin!", + ], + "first_name": [ + False, + "Your organization management level is not high enough to change a user with a Level of superadmin!", + ], }, "7": { - "editable": True, + "default_password": [True, ""], + "first_name": [True, ""], }, }, ) @@ -121,26 +132,52 @@ def test_with_cml(self) -> None: data, { "2": { - "editable": False, - "message": "Your organization management level is not high enough to change a user with a Level of can_manage_users!", + "default_password": [ + False, + "Your organization management level is not high enough to change a user with a Level of can_manage_users!", + ], + "first_name": [ + False, + "Your organization management level is not high enough to change a user with a Level of can_manage_users!", + ], }, "3": { - "editable": False, - "message": "Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or CommitteeManagementLevel can_manage in committee 1", + "default_password": [ + False, + "Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or CommitteeManagementLevel can_manage in committee 1", + ], + "first_name": [ + False, + "Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or CommitteeManagementLevel can_manage in committee 1", + ], }, "4": { - "editable": True, + "default_password": [True, ""], + "first_name": [True, ""], }, "5": { - "editable": False, - "message": "Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + "default_password": [ + False, + "Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + ], + "first_name": [ + False, + "Missing permission: OrganizationManagementLevel can_manage_users in organization 1", + ], }, "6": { - "editable": False, - "message": "Your organization management level is not high enough to change a user with a Level of superadmin!", + "default_password": [ + False, + "Your organization management level is not high enough to change a user with a Level of superadmin!", + ], + "first_name": [ + False, + "Your organization management level is not high enough to change a user with a Level of superadmin!", + ], }, "7": { - "editable": True, + "default_password": [True, ""], + "first_name": [True, ""], }, }, ) @@ -208,12 +245,16 @@ def test_with_same_meeting(self) -> None: self.assertEqual( data, { - "5": { - "editable": True, - }, + "5": {"default_password": [True, ""], "first_name": [True, ""]}, "7": { - "editable": False, - "message": "Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meetings {10, 7}", + "default_password": [ + False, + "Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meetings {10, 7}", + ], + "first_name": [ + False, + "Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or Permission user.can_update in meetings {10, 7}", + ], }, }, ) @@ -250,28 +291,112 @@ def test_with_all_payload_groups(self) -> None: data, { "2": { - "editable": False, - "message": "The field 'saml_id' can only be used in internal action calls", + "committee_management_ids": [True, ""], + "default_password": [True, ""], + "first_name": [True, ""], + "group_ids": [True, ""], + "is_demo_user": [ + False, + "Missing OrganizationManagementLevel: superadmin", + ], + "organization_management_level": [True, ""], + "saml_id": [ + False, + "The field 'saml_id' can only be used in internal action calls", + ], + "vote_weight": [True, ""], }, "3": { - "editable": False, - "message": "The field 'saml_id' can only be used in internal action calls", + "committee_management_ids": [True, ""], + "default_password": [True, ""], + "first_name": [True, ""], + "group_ids": [True, ""], + "is_demo_user": [ + False, + "Missing OrganizationManagementLevel: superadmin", + ], + "organization_management_level": [True, ""], + "saml_id": [ + False, + "The field 'saml_id' can only be used in internal action calls", + ], + "vote_weight": [True, ""], }, "4": { - "editable": False, - "message": "The field 'saml_id' can only be used in internal action calls", + "committee_management_ids": [True, ""], + "default_password": [True, ""], + "first_name": [True, ""], + "group_ids": [True, ""], + "is_demo_user": [ + False, + "Missing OrganizationManagementLevel: superadmin", + ], + "organization_management_level": [True, ""], + "saml_id": [ + False, + "The field 'saml_id' can only be used in internal action calls", + ], + "vote_weight": [True, ""], }, "5": { - "editable": False, - "message": "The field 'saml_id' can only be used in internal action calls", + "committee_management_ids": [True, ""], + "default_password": [True, ""], + "first_name": [True, ""], + "group_ids": [True, ""], + "is_demo_user": [ + False, + "Missing OrganizationManagementLevel: superadmin", + ], + "organization_management_level": [True, ""], + "saml_id": [ + False, + "The field 'saml_id' can only be used in internal action calls", + ], + "vote_weight": [True, ""], }, "6": { - "editable": False, - "message": "Your organization management level is not high enough to change a user with a Level of superadmin!", + "committee_management_ids": [ + False, + "Your organization management level is not high enough to change a user with a Level of superadmin!", + ], + "default_password": [ + False, + "Your organization management level is not high enough to change a user with a Level of superadmin!", + ], + "first_name": [ + False, + "Your organization management level is not high enough to change a user with a Level of superadmin!", + ], + "group_ids": [True, ""], + "is_demo_user": [ + False, + "Your organization management level is not high enough to change a user with a Level of superadmin!", + ], + "organization_management_level": [ + False, + "Your organization management level is not high enough to change a user with a Level of superadmin!", + ], + "saml_id": [ + False, + "Your organization management level is not high enough to change a user with a Level of superadmin!", + ], + "vote_weight": [True, ""], }, "7": { - "editable": False, - "message": "The field 'saml_id' can only be used in internal action calls", + "committee_management_ids": [True, ""], + "default_password": [True, ""], + "first_name": [True, ""], + "group_ids": [True, ""], + "is_demo_user": [ + False, + "Missing OrganizationManagementLevel: superadmin", + ], + "organization_management_level": [True, ""], + "saml_id": [ + False, + "The field 'saml_id' can only be used in internal action calls", + ], + "vote_weight": [True, ""], }, }, )