plone / Products.CMFPlone

The core of the Plone content management system
https://plone.org
GNU General Public License v2.0
249 stars 188 forks source link

Lack of upgrade step message shown to a Site Administrator #3906

Open wesleybl opened 7 months ago

wesleybl commented 7 months ago

Describe the bug On sites that lack the Plone upgrade step, the message The site configuration is outdated and needs to be upgraded. Please continue with the upgrade. for a user with the Site Administrator role.

However, when clicking the link, a blank screen appears. I don't think the Site Administrator has permission to do this.

To Reproduce Steps to reproduce the behavior:

  1. Make logging into a portal with lack Plone upgrade step with a user with the Site Administrator role.
  2. Go to Site Setup (http://localhost:3000/controlpanel)
  3. See the upgrade message

Expected behavior Do not show the message to a Site Administrator.

Screenshots If applicable, add screenshots to help explain your problem. download (10)

Software (please complete the following information):

kanhaiya04 commented 7 months ago

We can incorporate the condition userHasRoles(user, ['Manager']) when rendering the message.

I will create the PR soon if this approach suits well.

davisagli commented 7 months ago

@kanhaiya04 That doesn't sound like the right solution to me.

@wesleybl I think we should give users Site Administrators permission to run upgrade steps, rather than removing the message for them. The intent of the Site Administrator role is to allow all actions that affect a single Plone site, but prevent actions that could affect another Plone site in the same Zope instance. Does this sound right for your use case?

Also, whatever we decide, we should check for a permission rather than a role. This keeps things flexible, so that the permission can be assigned to different roles in the policy for a specific deployment. For example, if @wesleybl disagrees that Site Administrators should be able to do this for his deployment, he can simply remove the permission from the Site Administrator role.

For me, checking roles instead of permissions is always a sign of an incorrect design.

wesleybl commented 7 months ago

@davisagli I don't think the Site Administrator should have permission to run upgrade step. Whoever run the upgrade step must know what they are doing. Generally we give permission to Site Administrator, "common users", who do not have this knowledge. Upgrade steps can be time-consuming and must be performed in specific windows. Upgrade steps may require permissions that only the Manager has, such as creating an index in the catalog.

I also agree that verification should be by permission.

davisagli commented 7 months ago

@wesleybl I think a reasonable argument can be made for enabling it or disabling it for Site Administrators in different scenarios. It sounds like we at least agree on adding a permission to control it, which provides that flexibility. I'm okay with keeping the permission disabled by default.

davisagli commented 7 months ago

This is a medium-level task that requires changes in a number of places:

  1. Define the permission in CMFPlone
  2. Check the permission in the plone.restapi service that does upgrades
  3. Check the permission in the Classic UI view that does upgrades
  4. Check the permission in the volto component that shows this message

I'll move this issue to CMFPlone since the scope goes beyond just volto.

wesleybl commented 7 months ago

I think a reasonable argument can be made for enabling it or disabling it for Site Administrators in different scenarios.

@davisagli I think that whoever created an update step doesn't think "will the Site Administrator have permission to do this?"

Those who would give permission to the Site Administrator and those who create the upgrade are different people. Giving him permission and him receiving Unauthorized is not a smart solution.