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 31 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
31 changes: 31 additions & 0 deletions docs/presenters/get_user_editable.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
## Payload

```
{
user_ids: Id[];
fields: string[]
}
```

## Returns

```
{
user_id: Id: {
field: str: (
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 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

There are no special permissions necessary.
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
9 changes: 5 additions & 4 deletions docs/presenters/get_user_scope.md
Original file line number Diff line number Diff line change
@@ -1,21 +1,22 @@
## Payload

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

## 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
},
...
}
```

Expand Down
5 changes: 4 additions & 1 deletion openslides_backend/action/actions/user/create.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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_exists


@register_action("user.create")
class UserCreate(
UserMixin,
luisa-beerboom marked this conversation as resolved.
Show resolved Hide resolved
EmailCheckMixin,
CreateAction,
CreateUpdatePermissionsMixin,
Expand Down
9 changes: 5 additions & 4 deletions openslides_backend/action/actions/user/participant_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,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 ....shared.filters import And, FilterOperator, Or
from ..meeting_user.mixin import CheckLockOutPermissionMixin
from ..user.create_update_permissions_mixin import (
CreateUpdatePermissionsFailingFields,
PermissionVarStore,
)


class ParticipantCommon(BaseImportJsonUploadAction, CheckLockOutPermissionMixin):
Expand Down Expand Up @@ -51,6 +51,7 @@ def check_permissions(self, instance: dict[str, Any]) -> None:
)

self.permission_check = CreateUpdatePermissionsFailingFields(
self.user_id,
permstore,
self.services,
self.datastore,
Expand Down
5 changes: 4 additions & 1 deletion openslides_backend/action/actions/user/update.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 ....action.action import original_instances
from ....action.util.typing import ActionData
Expand All @@ -17,7 +20,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 (
AdminIntegrityCheckMixin,
LimitOfUserMixin,
Expand All @@ -29,6 +31,7 @@

@register_action("user.update")
class UserUpdate(
UserMixin,
EmailCheckMixin,
CreateUpdatePermissionsMixin,
UpdateAction,
Expand Down
4 changes: 4 additions & 0 deletions openslides_backend/action/actions/user/user_mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,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(
Expand Down
2 changes: 1 addition & 1 deletion openslides_backend/action/mixins/import_mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
1 change: 1 addition & 0 deletions openslides_backend/presenter/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
get_forwarding_meetings,
get_history_information,
get_mediafile_context,
get_user_editable,
get_user_related_models,
get_user_scope,
get_users,
Expand Down
1 change: 1 addition & 0 deletions openslides_backend/presenter/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class BasePresenter(BaseServiceProvider):
Base class for presenters.
"""

internal: bool = False
data: Any
schema: Callable[[Any], None] | None = None

Expand Down
85 changes: 85 additions & 0 deletions openslides_backend/presenter/get_user_editable.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
from collections import defaultdict
from typing import Any

import fastjsonschema

from openslides_backend.permissions.permissions import Permissions
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, str_list_schema

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,
"fields": str_list_schema,
},
"required": ["user_ids"],
"additionalProperties": False,
}
)


@register_presenter("get_user_editable")
class GetUserEditable(CreateUpdatePermissionsMixin, BasePresenter):
"""
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
name = "get_user_editable"
permission = Permissions.User.CAN_MANAGE

def get_result(self) -> Any:
if "fields" not in self.data or not self.data["fields"]:
raise PresenterException(
"Need at least one field name to check editability."
)
Copy link
Member

Choose a reason for hiding this comment

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

Why not make fields required?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good idea. Didn't think of it. So I removed that part of the check, but still checking for the list to have an entry.

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"]:
result[str(user_id)] = {}
groups_editable = {}
for field_name in one_field_per_group:
try:
self.check_permissions({"id": user_id, field_name: ""})
Copy link
Member

Choose a reason for hiding this comment

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

What, if the field is a integer field ? "" is not a good value for an integer field.

Copy link
Member Author

Choose a reason for hiding this comment

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

Only the keys are used by the 'check_permissions'. So there shouldn't be any problems. But I agree that 'None' is more beautiful.

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
5 changes: 4 additions & 1 deletion openslides_backend/presenter/get_user_scope.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
25 changes: 19 additions & 6 deletions openslides_backend/shared/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -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: dict | int | set[int]) -> 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 ""
Comment on lines +151 to +159
Copy link
Member

Choose a reason for hiding this comment

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

Why just write "permission(s)" instead of this code stuff?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, one could make it simple, but I like to make the error message to be more specific.

Loading