dev/core#1327 Implement new view status checks permission (reduce noize)

[seamuslee001]

Add in upgrade notice

Overview

This implements the new View Status Checks permission to allow for some more granularity of who gets to see the status checks

Before

Anyone with CiviCRM administration gets the status checks

After

Only roles with the view status checks permission get the status checks

ping @mattwire @eileenmcnaughton @jusfreeman

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[agh1]

My assumption per our conversation last night was that this would require both this new permission and Administer CiviCRM. The reason is that most things the status check would warn about require Administer access to fix, and some might be sensitive enough that you wouldn't want to share beyond those with Administer access.

The context was that in some organizations a senior user might have Administer CiviCRM but really shouldn't have the status checks on blast. We never really envisioned a status check user that lacks Administer CiviCRM.

That all said, maybe this would be a great permission for an API user in a monitoring system to check statuses without needing to grant that system the ability to mess with things.

[seamuslee001]

@agh1 I was just going off the google doc which suggested it was just a new permission. I think having a standalone permission makes more sense to me than having the combo tbh but open for discussion

[eileenmcnaughton]

I think it just requires the new permission - the complexity is that we really want this permission to be opt out not opt in - so default on in new installs for whoever would normally get 'administer CiviCRM'. In a perfect world we would also add it on upgrade to all users who have 'administer CiviCRM'. However we've never figured out how to do that on all CMS on upgrade (we possibly could create the structure for a function on the CMSes to add a permission & populate it drupal as a minimum & work on the others over time).

So not being able to add the permission we run the risk that people suddenly stop seeing the system checks without realising, when we really want it to be a more active decision.

One thing talked about was idea of meta-permissions but still the transitioning is an issue

Also we should flag this to the dev list - I think the phasing in is tricky & people may take actions in response so more eyes on = more better

[eileenmcnaughton]

@totten @colemanw

[jusfreeman]

I think having this permission is a great first step, thanks @seamuslee001

I think we need to agree on 1) when status checks should be performed, 2) where they are accessible from as well as 3) who status alerts are shown too.

I've quickly created this lab issue for this discussion, https://lab.civicrm.org/dev/core/issues/1041 and to link any associated PRs.

[seamuslee001]

Thanks for all the comments.

I have now added in a transitional arrangement that should be removed at the end of 2019 to ensure that administer CiviCRM is still enough for the checks to show however, i have also included a mechanism by which system administrators (or their shops etc) can immediately transition to the new arrangement. I think this is the best of both worlds.

ping @eileenmcnaughton @totten @colemanw @agh1

[eileenmcnaughton]

@seamuslee001 as . to where this stalled - apart from timing (ie. we finished the sprint & everyone had a tonne of catch up to do) the other 2 things were

1. emailing the dev list & getting more people looking / agreeing (maybe disagreeing)
2. install permissions - new sites should get the new permission for anyone with 'administer CiviCRM' on install

If we are doing the above it might be a good time to add in the new 'role permissions' - the idea here was that we would have 2 permissions that would imply a set of permissions ie

'administer CiviCRM system' and 'administer CiviCRM data' - the idea being that if you give one of those permissions it's like a 'package' - the former including things like seeing file permission status checks & the latter being able to edit profiles & option values & such like. Things like administering mailing settings might make more sense for the former but currently is just 'administer CiviCRM'

I think the way it would work is we would add another key to the values in the array

self::getCorePermissions

e.g

      'administer reserved groups' => [
        $prefix . ts('administer reserved groups'),
        ts('Edit and disable Reserved Groups (Needs Edit Groups)'),
      'permission_bundle' => 'Adminster CiviCRM Data',
      ],

In the check function it would still be Permission::check('administer reserved groups') but within that function it would also look for any bundles holding that permission & return true if the granular permission or the bundle exists. Existing hooks would allow extensions to edit

@totten might have other ideas on this

[eileenmcnaughton]

@seamuslee001 so coming back to this I logged https://lab.civicrm.org/dev/core/issues/1327 but I think the 2 things I mentioned above are still the minimum - ie. ensure the new permission is being added by default on new installs and email the dev list - it's probably best if they discuss on the Phab.

It seems working back through it this IS different to what was agreed - I think we could easily merge something that adds 'View Status Checks' and makes it so you can see the checks if you have that OR 'Administer CIviCRM' and it might be worth changing this PR to do only that as a step in the right direction - but until we make it so people can start to remove 'Administer CiviCRM' it doesn't achieve that much

[jaapjansma]

Betty and I are reviewing PR's and we came across this one.

Since November 1st nothing has happened on this PR and it needs work. @seamuslee001 are you going to work on this PR? If not we would propose to close it.

[seamuslee001]

@eileenmcnaughton This should handle the transition i would have thought?

[eileenmcnaughton]

@seamuslee001 my understanding of @totten's proposal is this - #16482 - note that if we go that way the removing this permission (or any others) from Administer CiviCRM is not in the plan

[seamuslee001]

@eileenmcnaughton I was under the impression that it was all about removing permissions from administer civicrm so you could then more finer grained allow for access by specific permission be it system or data. I have had a client request this style of functionality and I'm not sure if we are going to resolve #16482 any time soon tbh and this does implement the strategy you had requested in your previous comment of falling back by default to allowing for people with administer CiviCRM to see the status checks as they have before. I can certainly go either way but I guess i feel like we should be doing something more here.

[eileenmcnaughton]

Well when I thought through what I understood @totten to be suggesting it was NOT removing permissions from administer CiviCRM - but rather creating new permissions that would allow us not to give Administer CiviCRM to users unless we really wanted them to have 'root' permission

[seamuslee001]

@eileenmcnaughton if you can confirm your ok with the transition arrangements in this PR then i'll update the upgrade notice

[jaapjansma]

Betty and I are reviewing PRs again and since February 19 nothing has happened.
@seamuslee001 and @eileenmcnaughton do you think you still have time for this PR? If not we will propose to close it.

[eileenmcnaughton]

Let's close & discuss further in gitlab