Give Site Administrator permission to manage users

wesleybl commented 9 months ago

it was necessary to give permission plone.restapi: Access Plone user information and Manage users to Site Administrator.
An upgrade step has not yet been carried out to give these permissions to an already created portal. I don't know if I should do this here in plone.restapi or in plone.app.upgrade.
Several checks were made to prevent the Site Administrator from giving Manager permission to any user.
I will do a PR on Volto, to remove options from the Site Administrator to give Manager permission. To do this, roles, groups and users endpoints send information relevant to this.
A good way to test blocking the Site Administrator from giving Manager permission is to use this PR on the current Volto. The options are still available in the Volto, but the backend blocks them.
Maybe reviewing the commits might be easier.

Fixes plone/volto#3917

netlify[bot] commented 9 months ago

Deploy Preview for plone-restapi canceled.

Name	Link
Latest commit	8a156f3b64c60a74860f568b4bb3ec2167d9708b
Latest deploy log	https://app.netlify.com/sites/plone-restapi/deploys/6598744a1c7f4e000899b6ca

mister-roboto commented 9 months ago

@wesleybl thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

wesleybl commented 9 months ago

@jenkins-plone-org please run jobs

erral commented 9 months ago

how does this work in classic?

wesleybl commented 9 months ago

how does this work in classic?

In Plone Classic, users with the Site Administrator role are allowed to manage users. See:

https://github.com/plone/Products.CMFPlone/blob/1c1a0445a4a042af20a3602dda629999a8bf1186/Products/CMFPlone/profiles/default/rolemap.xml#L194-L199

jensens commented 9 months ago

I would appreciate a second review before merge, since this is a security issue.

davisagli commented 9 months ago

I will review this, but I want to take the time to do it carefully, so I haven't gotten to it yet.

wesleybl commented 8 months ago

I will review this, but I want to take the time to do it carefully, so I haven't gotten to it yet.

@davisagli could you please take a look here? I really need this. On our Sites we do not provide Manager permission to clients, but they need to manage users.

wesleybl commented 8 months ago

@jenkins-plone-org please run jobs

wesleybl commented 8 months ago

@gforcada when I requested @jenkins-plone-org to run the Jenkins Jobs, Job Plone Jenkins CI - pull-request-6.1-3.11 did not run. Even after the other jobs finished it didn't start. Do you know why?

wesleybl commented 8 months ago

@davisagli I made all the requested changes. Can you take a look please?

davisagli commented 8 months ago

@wesleybl @gforcada I think it's intentional that the Jenkins build is not triggered for 3.11 -- we run on the oldest and latest supported Python versions only, to manage the load. I just need to update the repository branch protection settings so that the 3.12 check is required instead of the 3.11 check, and then the 3.11 check will not show up here.

wesleybl commented 7 months ago

@jenkins-plone-org please run jobs

wesleybl commented 7 months ago

@jenkins-plone-org please run jobs

wesleybl commented 7 months ago

IMHO, the user control panel and groups control panel should know which role the current user has and act accordingly. So the question seems to be, how the frontend can know the role of the current user.

@ksuess yes. The frontend does not know what the current user's roles are. So I thought it would be better to do it this way. Do you have any suggestions for doing this differently?

We could have a "@current_user_roles" endpoint, but I don't like that idea. It would be more complex and less informative.

@davisagli opinions?

tisto commented 7 months ago

FYI: @davisagli is on holiday until mid-next week...

davisagli commented 7 months ago

@wesleybl I think @ksuess has a reasonable point that "can_assign" in the @roles endpoint is really more information about the current user's abilities than about the role.

We should already have the current user's roles in the frontend redux state -- the PersonalTools component fetches the user data from /@users/[userid] using the userid decoded from the auth JWT token, and the response includes the user's global roles. https://6.docs.plone.org/plone.restapi/docs/source/endpoints/users.html#read-user

ksuess commented 7 months ago

@wesleybl I think @ksuess has a reasonable point that "can_assign" in the @roles endpoint is really more information about the current user's abilities than about the role.

We should already have the current user's roles in the frontend redux state -- the PersonalTools component fetches the user data from /@users/[userid] using the userid decoded from the auth JWT token, and the response includes the user's global roles. https://6.docs.plone.org/plone.restapi/docs/source/endpoints/users.html#read-user

current user's roles are not fetched unless the personal tools are visited. But the fetch of the user data of the authenticated user can be done in the users control panel component. This would make the change of @roles endpoint dispensable. The users control panel component can act according the roles of the authenticated user:

disable checkboxes and hide delete menu for users with Manager role if the authenticated user is only Site Administrator.
disable checkboxes for Manager role if the authenticated user is only Site Administrator.

wesleybl commented 7 months ago

@ksuess @davisagli Okay. Let's continue like this then. Although I think we would be transferring responsibility from the backend to the frontend.

If we have the authenticated user's roles on the frontend, we can remove the can_assign and can_delete keys from the endpoints.

wesleybl commented 6 months ago

@ksuess @davisagli I removed the can_delete and can_assign key from endpoints. Can you check if it's ok now?

wesleybl commented 6 months ago

@jenkins-plone-org please run jobs

davisagli commented 6 months ago

@wesleybl Thanks -- this is on my list to review, but I'm also currently on a holiday break. Please remind me about it in a week if I haven't gotten to it yet.

davisagli commented 6 months ago

@jenkins-plone-org please run jobs

davisagli commented 6 months ago

@wesleybl Thanks for your hard work and patience on this one!

tisto commented 5 months ago

@wesleybl thank you for your hard work on this!

@davisagli just to double-check: this is a non-breaking feature, correct? I can cut a new release...

davisagli commented 5 months ago

@tisto yes, as far as I can tell. It adds the ability for site admins to do some user management actions that were previously only available for Managers

tisto commented 5 months ago

@wesleybl @davisagli released https://pypi.org/project/plone.restapi/9.3.0/

plone / plone.restapi