Rationalise Activity api ACLs for consistency, to respect the hook & improve performance

[eileenmcnaughton]

Overview

This PR alters the way permissions are handled on sites that use ACLs when the activity.get api is called with the 'check_permissions' flag set to TRUE. This would usually be the case for js calls

Before

• 'view all activities' permissions bypasses all contact ACLs in the api but not anywhere in the UI
• when performance checks ARE applied they are slow & because they retrieve & then filter a request for 50 rows could return less than 50 even though more exist
• getcount doesn't apply permissions
• api permissions cannot be altered by hooks
• api v4 not aligned to api v3 behaviour

After

After this patch

1. the 'view all activities' permission will no longer by-pass all other ACLs. One could argue that's exactly
   what it means - but it doesn't do that in the UI which seems like the standard elsewhere.
2. a user will be able to view an activity via the api if they have permission to view ANY contact linked to it (before it was ALL contacts via the api)
3. a user will not see the names of any contacts they do not have permission over when requesting activity contact details in return parameters
4. getcount will no longer by-pass the api
5. performance is improved

Technical Details

We have a lot of inconsistency about how (and if) activity ACLs are applied. Note that permissions only apply
when the api is being called with check_permissions = TRUE - e.g from the js layer.

This PR changes the logic used for the activity.get api to be consistent with the report logic
which
a) is the most performant variant
b) is the one with the least code complexity
c) is more consistent with CiviCase
d) allows hooks to modify the permissions applies
e) creates consistency between api v3 & v4
f) is consistent with some site user expectations but not others - the presence of all this inconsistency
is an indicator not everyone wants the same thing but given that choosing a performant &
maintainable option for core seems like a good criteria.

After this patch

1. the 'view all activities' permission will no longer by-pass all other ACLs. One could argue that's exactly
   what it means - but it doesn't do that in the UI which seems like the standard elsewhere.
2. a user will be able to view an activity via the api if they have permission to view ANY contact linked to it (before it was ALL contacts via the api)
3. a user will not see the names of any contacts they do not have permission over when requesting activity contact details in return parameters
4. getcount will no longer by-pass the api
5. performance is improved

Places where permissioning applies to activities

• activities listing on contact - shows actitivies & related contact names regardless of permission to view the contacts
• activity search results -- shows actitivies & related contact names regardless of permission to view the contacts
• activity view page - links to view the activity exist on the above 2 screens but will give access denied unless they
  can see ALL related contacts
• activity reports - shows activities if ANY related contacts are permitted, suppresses names of unpermitted contacts

Potential follow on steps

1. make the activity tab listing consistent by switching from the unperformant deprecatedGetActivities fn
   to the performance getActivities fn - there are no remaining blockers to that.
2. align the activity view screen & add in hook call there too
3. align activity search results screen, address performance issues there too....

Comments

I did a more detailed current breakdown over here https://docs.google.com/spreadsheets/d/1JHzYDiyssnGOAIEwnSQof_mrZ-Mh9E-grMnSsuZMQGE/edit#gid=0

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

[colemanw]

This would be more efficient if we skip adding this clause if getPermittedActivityTypes returns the full set of types.

[eileenmcnaughton]

It's filtered by component

[jmcclelland]

Thanks Eileen for doing this deep dive. I don't have any specific comments on the code changes but fully support the goals.

[MegaphoneJon]

I'm also a +1 on the goals here. Maintaining compatibility with Activity Type ACLs extension is important, but you specifically mentioned hooks altering the permissions so I assume this is fine.

I should downgrade "maintaining compatibility" to "comparable compatibility is possible with a new version of the extension".

[eileenmcnaughton]

Just noting that I sent an email to the dev list about this (hence the 2 comments above)

[eileenmcnaughton]

@MegaphoneJon WRT the activityTypeACLS they don't apply in most places at the moment but this

1. extends them to v3 api and
2. agrees the std for other places so we can stdise elsewhere later

[colemanw]

Merging based on positive feedback and passing tests.