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

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

hjanott
Copy link
Member

@hjanott hjanott commented Aug 15, 2024

Closes #2522

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.

openslides_backend/shared/mixins/user_scope_mixin.py Outdated Show resolved Hide resolved
openslides_backend/shared/mixins/user_scope_mixin.py Outdated Show resolved Hide resolved
openslides_backend/shared/mixins/user_scope_mixin.py Outdated Show resolved Hide resolved
openslides_backend/shared/mixins/user_scope_mixin.py Outdated Show resolved Hide resolved
openslides_backend/shared/mixins/user_scope_mixin.py Outdated Show resolved Hide resolved
openslides_backend/shared/mixins/user_scope_mixin.py Outdated Show resolved Hide resolved
openslides_backend/shared/mixins/user_scope_mixin.py Outdated Show resolved Hide resolved
@Elblinator
Copy link
Member

I tested the PR with different scenarios and sometimes the backend returns Errors even though the action should be permitted.
What I did:

  • Create three meetings
  • Add A always as admin to all meetings
  • Add B groups do not seem to matter, I could not find a connection at least
  • Try changing B's personal information with A
    sometimes, and only sometimes, while trying to change the personal information B's personal information is not changed but an Error that A is missing the user.canUpdate permissions for he meetings {1,2,3}

@Elblinator Elblinator removed their assignment Oct 10, 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.

see comment above

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.

I found no Errors
My previous Errors probably stem from a faulty setup

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
3 participants