Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow meeting admin user to update a non admin user that shares all his meetings with requesting user. #2576

Merged
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
c13c1f2
Allow meeting admin user to update a non admin user that shares all h…
hjanott Aug 15, 2024
130fbc9
prevent success on empty meetings and changing committee admin.
hjanott Aug 21, 2024
f2ac306
repair user create due to missing user id
hjanott Aug 21, 2024
a6d9665
formatting
hjanott Aug 21, 2024
56928aa
draft an idea for scope change in user scope mixin.
hjanott Aug 26, 2024
f4eec94
use function in field group methods and check_permissions for scop
hjanott Aug 28, 2024
86208da
Merge remote-tracking branch 'upstream/main' into 2522_meeting_admin_…
hjanott Aug 28, 2024
15a5365
better if else structure.
hjanott Aug 30, 2024
37ba259
Add get user editable presenter plus test.
hjanott Sep 4, 2024
7c24289
Extend get user editable with payload field names to support all payl…
hjanott Sep 9, 2024
3d0df2b
delete unnecessary comments and return types
hjanott Sep 11, 2024
e46cffe
restructure check_for_admin_in_all_meetings
hjanott Sep 19, 2024
2a3398f
merge main
hjanott Sep 19, 2024
a8085b1
running black and improving doc
hjanott Sep 19, 2024
9f42c46
delete unnecessary lines
hjanott Sep 19, 2024
84398e4
Merge branch 'main' into 2522_meeting_admin_can_manage_non_admin
Elblinator Sep 25, 2024
8ee6d4a
do requested changes
hjanott Oct 2, 2024
1d07d85
Merge branch 'main' of https://github.com/OpenSlides/openslides-backe…
hjanott Oct 7, 2024
e2e569d
coding style
hjanott Oct 7, 2024
e6fc78f
Improve method description
hjanott Oct 7, 2024
e2582d6
add plural to MissingPermission permission output
hjanott Oct 7, 2024
576b3f4
fix plural error in exception
hjanott Oct 8, 2024
29a0e48
change get editable presenter on a per field basis response and a per…
hjanott Oct 14, 2024
011f63d
Merge branch 'main' of https://github.com/OpenSlides/openslides-backe…
hjanott Oct 14, 2024
d05d4cc
code improvement and names
hjanott Oct 14, 2024
91b6d77
Merge branch 'main' of https://github.com/OpenSlides/openslides-backe…
hjanott Nov 13, 2024
6287de2
cleanup
hjanott Nov 13, 2024
3dec54d
fix error with missing user id
hjanott Nov 13, 2024
87e80b4
repair test for get user scope
hjanott Nov 14, 2024
90abbb0
allow requested user to be meeting admin, bug fix on default password…
hjanott Nov 14, 2024
b2827cc
Merge branch 'main' into 2522_meeting_admin_can_manage_non_admin
hjanott Nov 15, 2024
b0c1179
make fields required
hjanott Nov 19, 2024
cb64e6c
add to presenter overview
hjanott Nov 19, 2024
c3056ec
same behavior on meeting scope and refactoring group A and F check
hjanott Nov 22, 2024
de6f279
Merge branch 'main' into 2522_meeting_admin_can_manage_non_admin
hjanott Nov 22, 2024
42eb249
move test to user update tests
hjanott Nov 22, 2024
371d408
typing
hjanott Nov 22, 2024
39f3d1e
Merge branch 'main' into 2522_meeting_admin_can_manage_non_admin
hjanott Nov 25, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion docs/actions/user.set_password.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
4 changes: 2 additions & 2 deletions docs/presenters/get_user_related_models.md
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
## Payload

```js
```
{
user_ids: Id[];
}
```

## Returns

```js
```
{
[user_id: Id]: {
organization_management_level: OML-String,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
Expand All @@ -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()
Expand All @@ -227,7 +229,7 @@ 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 scope, scope id, OML-permission and committee ids for requested user
(
self.instance_user_scope,
self.instance_user_scope_id,
Expand All @@ -247,20 +249,17 @@ 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)
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 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
luisa-beerboom marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -272,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}
)
luisa-beerboom marked this conversation as resolved.
Show resolved Hide resolved
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,
}
)
luisa-beerboom marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down Expand Up @@ -360,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 (
Expand All @@ -386,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
Expand Down Expand Up @@ -475,8 +478,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():
Expand Down Expand Up @@ -598,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:
Expand Down
1 change: 1 addition & 0 deletions openslides_backend/action/actions/user/set_password.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from .password_mixins import ClearSessionsMixin, SetPasswordMixin


# TODO auch ermöglichen
luisa-beerboom marked this conversation as resolved.
Show resolved Hide resolved
@register_action("user.set_password")
class UserSetPasswordAction(
SetPasswordMixin,
Expand Down
92 changes: 89 additions & 3 deletions openslides_backend/shared/mixins/user_scope_mixin.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,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:
Expand All @@ -105,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
Expand Down Expand Up @@ -163,4 +163,90 @@ 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}
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is good that you have worked it into the scope, but if the way the scope checking works has been changed in the backend, the other services (mainly the client) need to be able to get the information required to calculate the same result via the scope presenter. This means meeting memberships and committee_management_ids need to somehow be part of what the scope presenter returns.

Look into how that presenters return value would optimally look after the new changes, change the wiki entry accordingly and change the get_user_scope_function in this presenter.
It is then only appropriate that the check_for_admin_in_all_meetings function should not calculate everything and get all data from scratch, but use the type of data calculated by get_user_scope in the previous part of the calling function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Moved the function to the user scope mixin. However, in some specific cases the meeting_ids cannot be obtained from permstore.


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
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", []))
luisa-beerboom marked this conversation as resolved.
Show resolved Hide resolved
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
luisa-beerboom marked this conversation as resolved.
Show resolved Hide resolved
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
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 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
)
):
return False
else:
return False
return True
46 changes: 46 additions & 0 deletions tests/system/action/user/test_set_password.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Loading