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

Conversation

hjanott
Copy link
Member

@hjanott hjanott commented Aug 15, 2024

Closes #2522

@hjanott hjanott added this to the 4.2 milestone Aug 15, 2024
Copy link
Member

@Elblinator Elblinator left a comment

Choose a reason for hiding this comment

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

A meeting admin should not be allowed to change the information of a committee admin. The following Error should be thrown

Error: You are not allowed to perform action user.update. Missing permission: OrganizationManagementLevel can_manage_users in organization 1

@Elblinator Elblinator assigned hjanott and unassigned Elblinator Aug 16, 2024
@hjanott hjanott assigned luisa-beerboom and unassigned hjanott Aug 21, 2024
@hjanott hjanott marked this pull request as ready for review August 21, 2024 14:01
Copy link
Member

@luisa-beerboom luisa-beerboom left a comment

Choose a reason for hiding this comment

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

From the issue name I read that this is supposed to be a scope change. Given that, I would expect at least the check_permissions_for_scope function of UserScopeMixin to be changed, as well as for all actions using said mixin to have at least been looked at for the sake of considering potential necessary changes. Scope is after all something that is more generally used for user actions concerning "personal information".

Keeping with that theme, I'd also expect the password actions to probably have been changed as well and maybe user.delete (should probably discuss whether this is wanted with @MSoeb or @Elblinator )

I'd expect this wiki entry to have been changed accordingly, as well as maybe the related actions backend documentation files.
I can't really judge if the new functionality has been implemented, unless the requirements are documented in the wiki, after all.

There should be tests for the other user actions as well (maybe even one in create, just to see if the functionality actually stayed the same if current tests don't cover that).
There ought to be tests for the user imports as well: I.e. check if the participant import also newly permits what was previously forbidden and if the account import still can't be used by those who don't have organization user management rights, even if they have meeting perms for all of a users meetings.

Add tests for groups A and F fields.
@hjanott hjanott assigned luisa-beerboom and unassigned hjanott Aug 28, 2024
openslides_backend/action/actions/user/set_password.py Outdated Show resolved Hide resolved
Comment on lines 166 to 169
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.

Copy link
Member

@reiterl reiterl left a comment

Choose a reason for hiding this comment

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

Some small remarks.

Comment on lines 49 to 52
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.

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.

Comment on lines +151 to +159

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 ""
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.

Copy link
Member

@Elblinator Elblinator left a comment

Choose a reason for hiding this comment

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

Combinations that currently return the wrong value:

{
  "4": {
    "first_name": [
      false,
      "Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or CommitteeManagementLevel can_manage in committee 1 or Permission user.can_manage in meeting 1"
    ],
    "default_password": [
      false,
      "Missing permissions: OrganizationManagementLevel can_manage_users in organization 1 or CommitteeManagementLevel can_manage in committee 1 or Permission user.can_manage in meeting 1"
    ]
  }
}

The Admin has user.can_update permissions in meeting 1 and the user with the id is only in meeting 1

Combinations which return the correct value:

  • The Admin has user.can_update permissions in meeting 1 and user.can_manage in meeting 2 and the user with the id 4 is in meeting 1 and 2. Meeting 1 and 2 belong to the same committee
  • The Admin has user.can_update permissions in meeting 1 and user.can_manage in meeting 3 and the user with the id 6 is in meeting 1 and meeting 3. Meeting 1 and 3 are in different committees
  • every other from my test cases returns the correct value

@hjanott hjanott assigned Elblinator and unassigned hjanott Nov 22, 2024
@luisa-beerboom luisa-beerboom removed their assignment Nov 25, 2024
@Elblinator Elblinator assigned hjanott and unassigned Elblinator Nov 25, 2024
@hjanott hjanott enabled auto-merge (squash) November 25, 2024 15:27
@hjanott hjanott merged commit c13b26f into OpenSlides:main Nov 25, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change user.scope for Meeting admins if they are admins in several meetings
4 participants