dev/core/454 rationalise activity case permissions & code

[eileenmcnaughton]

Overview

Per https://lab.civicrm.org/dev/core/issues/454 I have made access activities associated with cases consistent between the UI & various other places - in practice making 'access my cases and activities' OR 'access all cases and activities' required to pass the CiviCase component check and removing the 'administer CiviCase' bypass that exists in some places but not others.

Before

Via api '' required for accessing case activities requires one of

• administer CiviCase
• access my cases and activities
• access all cases and activities

UI requires one of

• access my cases and activities
• access all cases and activities

After

Via api '' required for accessing case activities requires one of

• access my cases and activities
• access all cases and activities

UI requires one of

• access my cases and activities
• access all cases and activities

Technical Details

This is a refactor step - not the final destination. I want to move from loading
all activities & then checking if each one can be accessed to determining a list
of activity types and then adding that filter to the sql query. 2 reasons

• performance
• because getcount actually hard fails when permissions are applied currently :-(

In this step I note the function activityComponents looks like it expected
the concept of showActivitiesInCore by components to take off but in
practice it is hard coded to 0 for CiviCase & CiviCase only. Oddly there is
acually handling for CiviCase in this function in a place unreachable
without the changes in this patch. In addition the few places that call this
function also do weird CiviCase handling. I've opted for relatively low
intervention since I think most places that call this function
are likely to change themselves in the short-medium term.

Comments

I think this would be easier to review if #12994 were merged first (it is incorporated in this but I would rebase it out if it were merged)

[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).

[eileenmcnaughton]

I've rebased this after @seamuslee001 merged the extraction PR

[aydun]

@eileenmcnaughton I tried to test this but could not reproduce the 'Before' scenario: I changed permissions for the 'demo' user on a buildkit instance and tested via API Explorer (logged in as demo).
'Access all case and activities' and 'Access my cases and activities' both work as expected without 'Administer CiviCase'.

Is the Description still correct?

[eileenmcnaughton]

@aydun - this issue is primarily about cleaning up the code & rationalising inconsistencies in the code around permissions - per https://github.com/civicrm/civicrm-core/pull/12995/files#diff-a35044abd0f538e4af551c5ae039692dL2787 is mostly in the interests of consistency. I wonder if in fact this removes an ability to bypass case ACLs when Administer CiviCase permission is present & it's going the opposite way to what I thought?

[aydun]

@eileenmcnaughton As I read it, any one of the three permissions was sufficient to make isContactPermittedAccessToCaseActivity() return TRUE, and the change removes 'administer CiviCase' from that list.

I guess it comes down to what the 'administer' permission should provide: is it an overriding super-permission in the root tradition, or should it mean 'can configure the CiviCase component'?

I'm not sufficiently up to speed with the nuances of other 'administer' permissions, but by itself this seems a reasonable change since anyone needing to access all cases and activities can be granted the, erm, 'access all cases and activities' permission.

[eileenmcnaughton]

@aydun going back to the original gitlab the description is more like what you hit in practice & I have updated the pr summary. @colemanw you approved the gitlab. @aydun has now reviewed - do you want to check if this is mergeable now?

[colemanw]

Concept makes a lot of sense, the code looks good and tests are passing.

[eileenmcnaughton]

thanks @colemanw