Fix caching issues with Event permissions #12769

mattwire · 2018-09-04T14:27:02Z

Overview

Follow up to #12424

Calling checkPermission() with an event ID and then with no ID (to get all permissions) only returns permission for the first event, meaning all subsequent events are returned as having no permission when they probably do.

Before

If CRM_Event_BAO_Event::checkPermission() is called with a single event ID the static cache is "populated" but actually only has a single event ID in. If the same function is called again with no ID the cache is returned rather than building an array of all events with permissions.

After

Refactor CRM_Event_BAO_Event::checkPermission() into two separate functions as they return two different things - an array of permissions in the case of all events, and TRUE/FALSE in the case of a single event ID.
A separate cache is implemented for all event permissions and for a single event permission.
So if you:

Call checkPermission() with an event ID you will get a TRUE/FALSE response and that will be cached.
Call checkPermission() with no event ID you will be "redirected" to getAllPermissions() which will build it's own cache and return an array of all event permissions.

Technical Details

If CRM_Event_BAO_Event::checkPermission() is called with a single event ID the static cache is "populated" but actually only has a single event ID in. If the same function is called again with no ID the cache is returned rather than building an array of all events with permissions.

Comments

@alifrumin Would you be able to review this one? It may need some further work on the existing unit tests as well if you have chance to take a look?

alifrumin · 2018-09-05T14:28:04Z

Hey @mattwire, I can take a look at this later this week.

alifrumin · 2018-09-07T22:39:09Z

I tested this lightly locally,

I logged in as a user with the permission "CiviEvent: access CiviEvent" and could create but not edit an event.

I got a permissions error immediately after creating the event:

Expected behaviour as outlined in #12424 is that users with the permission "CiviEvent: access CiviEvent" can create events and then edit the events they create.

I like the concept of the code @mattwire, thoughts?

eileenmcnaughton · 2018-09-10T05:35:51Z

CRM/Event/BAO/Event.php

   */
  public static function checkPermission($eventId = NULL, $type = CRM_Core_Permission::VIEW) {
+    if (empty($eventId)) {
+      return self::getAllPermissions($type);


agree - ideally add a deprecation notice

eileenmcnaughton · 2018-09-10T05:37:42Z

CRM/Event/BAO/Event.php

+      // Note: for a multisite setup, a user with edit all events, can edit all events
+      // including those from other sites
+      if (($type == CRM_Core_Permission::EDIT) && CRM_Core_Permission::check('edit all events')) {
+        Civi::$statics[__CLASS__]['permission'][$eventId] = TRUE;


I think you need an action nuance in your keys? ie

Civi::$statics[CLASS]['permission']['edit'][$eventId] = TRUE;
Civi::$statics[CLASS]['permission']['view'][$eventId] = TRUE;

(set both on edit perm, only one on view )

mattwire · 2018-10-23T21:31:30Z

@alifrumin Do you have time to test this again? I've refactored it since you found the original error but it could really do with some more tests as it was a very messy function before.

mattwire · 2018-10-23T21:40:41Z

@spalmstr Are you able to test this please? It relates to #11716

alifrumin · 2018-10-24T14:36:22Z

Taking a look now

alifrumin · 2018-10-24T14:57:57Z

I tested this by:

logging into the demo site created by jenkins.
creating a new drupal user "event" with the role "authenticated user"
giving the "authenticated user" role the following permissions in civi:

CiviEvent: access CiviEvent
CiviCRM: access CiviCRM backend and API
CiviEvent: view event info -- I was prompted to when trying to view event info however according to the description of CiviEvent: access CiviEvent - "Create events, view all events, and view participant records (for visible contacts)" I should not need it.

Logging in as the user "event"
Attempting to create an event - I am able to see the form to create an event, however on save of the event, it seems the event is created but I do not have permission to edit it the event, see screenshot below. This is the same behavior as described on September 7th.

mattwire · 2018-10-24T15:21:59Z

@alifrumin Thanks. I just found the same thing :-( and realised how fragile it all is. So I've just written a pile of tests. testDeleteOwnEvent() will fail currently... is that even possible? And, if so, what permissions do they need to do that?

mattwire · 2018-10-24T15:23:52Z

@alifrumin And it looks like we may need to "improve" the descriptions of some of the event permissions once we've worked out how they are actually meant to work!

eileenmcnaughton · 2018-10-24T23:21:46Z

I worked through this & it looks pretty good -some thoughts

I'd like to see calling checkPermission deprecated - at least to the extent of a code comment saying that - I realise more deprecation in this PR might be scope creep.
I think you could get some efficiencies by setting 'view' keys when you set edit keys.
I find the 2 statics key names kinda confusing 'permission' vs 'permissions'
I think you could check the 'permissions' statics before doing a query - since it's like that 'all events a contact can access' would be loaded in one call & then 'can they access x event ' in a second one
I think the rule for delete should be that they have 'delete in CiviEvent' AND edit access to the event in question. Arguably they should be able to delete events they created without 'delete in CiviEvent' but I think I'd be more worried about accidentally opening up delete permissions than accidentally reducing them

e.g

        if (CRM_Core_Permission::check('delete in CiviEvent') && CRM_Event_BAO_Event::checkPermission($eventId, CRM_Core_Permission::EDIT)) {
          Civi::$statics[__CLASS__]['permission']['delete'][$eventId] = TRUE;
        }

(you'd need to change the tests slightly for this assumption)

NOTE - other than resolving the test issue my comments are non-blocking & it would be fine to leave those thoughts for later

mattwire · 2018-10-29T21:27:45Z

I'd like to see calling checkPermission deprecated - at least to the extent of a code comment saying that - I realise more deprecation in this PR might be scope creep.

Deprecated warning added when we should be calling checkAllPermissions instead.

I think you could get some efficiencies by setting 'view' keys when you set edit keys.

Yes, good idea. Done.

I find the 2 statics key names kinda confusing 'permission' vs 'permissions'

So do I, but for now it solves the main issue here, that caching for a single event was preventing caching from all from happening. And because of the difference in the way each of the caches are generated and the general confusion about what each permission actually does... this seemed most sensible for now. But I've added a code comment.

I think you could check the 'permissions' statics before doing a query - since it's like that 'all events a contact can access' would be loaded in one call & then 'can they access x event ' in a second one

I think there are definitely further improvements but my head starts to spin if I improve it too much.

I think the rule for delete should be that they have 'delete in CiviEvent' AND edit access to the event in question. Arguably they should be able to delete events they created without 'delete in CiviEvent' but I think I'd be more worried about accidentally opening up delete permissions than accidentally reducing them

Yes. So I've updated the tests so that 'Delete in CiviEvent' is required. I can see someone submitting a PR with either a new permission 'Delete own events' or an adjustment to how the existing permissions work.

@eileenmcnaughton @alifrumin Assuming tests pass are you both happy with this now from a "it's less broken than it was before" point of view?

eileenmcnaughton · 2018-10-29T21:58:44Z

@mattwire style error

alifrumin · 2018-10-30T14:01:51Z

I tested this and it looks good to me!

There is an error that shows up on the events dashboard for a user with the permissions:

CiviEvent: access CiviEvent
CiviCRM: access CiviCRM backend and API
CiviEvent: view event info

but it sounds like that might be on purpose.

Error text:

"User deprecated function: Deprecated function CRM_Event_BAO_Event::checkPermission, use CRM_Event_BAO_Event::getAllPermissions. Array ( [civi.tag] => deprecated ) in CRM_Core_Error_Log->log() (line 69 of /home/jenkins/bknix-dfl/build/core-12769-pu8u/sites/all/modules/civicrm/CRM/Core/Error/Log.php)."

seamuslee001 · 2018-10-30T20:32:55Z

Given Eileen's comments and Alice's test i believe this is good to merge

(CiviCRM Review Template WORD-1.1)

(r-explain) Pass
(r-test) Pass - Expands unit tests as well
(r-code) Pass
(r-doc) Pass
(r-maint) Pass
(r-run) Pass - Tested by Alice and Eileen
(r-user) Pass
(r-tech) Pass

eileenmcnaughton · 2018-10-30T20:34:52Z

Thanks @seamuslee001 - @mattwire @alifrumin

We should try to fix those places where it calls the now-deprecated version of the function

totten added the master label Sep 5, 2018

eileenmcnaughton reviewed Sep 10, 2018

View reviewed changes

mattwire changed the title ~~Fix caching issues with Event permissions~~ WIP Fix caching issues with Event permissions Sep 13, 2018

mattwire force-pushed the civievent_permissions branch from aae6cec to f20dc79 Compare October 20, 2018 22:28

mattwire changed the title ~~WIP Fix caching issues with Event permissions~~ Fix caching issues with Event permissions Oct 23, 2018

mattwire mentioned this pull request Oct 23, 2018

CRM-21798 updates CRM_Event_BAO_Event::checkPermission so it can be called repeatedly to check permissions on a list of even #11716

Closed

mattwire force-pushed the civievent_permissions branch from f20dc79 to 01e5965 Compare October 24, 2018 15:20

mattwire force-pushed the civievent_permissions branch 2 times, most recently from e96aa1d to b263942 Compare October 24, 2018 15:47

mattwire force-pushed the civievent_permissions branch from b263942 to 544571f Compare October 25, 2018 10:31

mattwire changed the title ~~Fix caching issues with Event permissions~~ WIP Fix caching issues with Event permissions Oct 25, 2018

Refactor Event::checkPermissions to resolve issues with caching

35b3710

mattwire force-pushed the civievent_permissions branch from 544571f to 069f4ab Compare October 29, 2018 21:22

mattwire changed the title ~~WIP Fix caching issues with Event permissions~~ Fix caching issues with Event permissions Oct 29, 2018

Add more unit tests for Event permissions

d0853f7

mattwire force-pushed the civievent_permissions branch from 069f4ab to d0853f7 Compare October 29, 2018 21:59

seamuslee001 merged commit acaaa16 into civicrm:master Oct 30, 2018

alifrumin mentioned this pull request Nov 15, 2018

Replace deprecated event function #13093

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix caching issues with Event permissions #12769

Fix caching issues with Event permissions #12769

mattwire commented Sep 4, 2018

alifrumin commented Sep 5, 2018 •

edited

Loading

alifrumin commented Sep 7, 2018 •

edited

Loading

eileenmcnaughton Sep 10, 2018

eileenmcnaughton Sep 10, 2018

mattwire commented Oct 23, 2018

mattwire commented Oct 23, 2018

alifrumin commented Oct 24, 2018

alifrumin commented Oct 24, 2018

mattwire commented Oct 24, 2018

mattwire commented Oct 24, 2018

eileenmcnaughton commented Oct 24, 2018 •

edited

Loading

mattwire commented Oct 29, 2018

eileenmcnaughton commented Oct 29, 2018

alifrumin commented Oct 30, 2018

seamuslee001 commented Oct 30, 2018

eileenmcnaughton commented Oct 30, 2018

Fix caching issues with Event permissions #12769

Fix caching issues with Event permissions #12769

Conversation

mattwire commented Sep 4, 2018

Overview

Before

After

Technical Details

Comments

alifrumin commented Sep 5, 2018 • edited Loading

alifrumin commented Sep 7, 2018 • edited Loading

eileenmcnaughton Sep 10, 2018

Choose a reason for hiding this comment

eileenmcnaughton Sep 10, 2018

Choose a reason for hiding this comment

mattwire commented Oct 23, 2018

mattwire commented Oct 23, 2018

alifrumin commented Oct 24, 2018

alifrumin commented Oct 24, 2018

mattwire commented Oct 24, 2018

mattwire commented Oct 24, 2018

eileenmcnaughton commented Oct 24, 2018 • edited Loading

mattwire commented Oct 29, 2018

eileenmcnaughton commented Oct 29, 2018

alifrumin commented Oct 30, 2018

seamuslee001 commented Oct 30, 2018

eileenmcnaughton commented Oct 30, 2018

alifrumin commented Sep 5, 2018 •

edited

Loading

alifrumin commented Sep 7, 2018 •

edited

Loading

eileenmcnaughton commented Oct 24, 2018 •

edited

Loading