dev/core#421 Fix issue where creating user driven message templates w… #12896

seamuslee001 · 2018-10-05T02:34:41Z

…as requireing the ssystem workflow message template permission as well

Overview

This fixes a regression from #11974 which mean that creating any message template including a user driven message template required all 3 permissions when the intent was clearly they should be split up

Before

Creating a user driven message template requires all 3 permissions to work

After

Now only requires the 2 specifically related permissions as per the quickform

Technical Details

This changes the permissions on the entity.action but adds in specific checks in the create action to do the targeted permissions checks needed

ping @eileenmcnaughton @mickadoo @johntwyman @ajesamson

civibot · 2018-10-05T02:34:43Z

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

monishdeb · 2018-10-05T20:05:24Z

api/v3/MessageTemplate.php

+    elseif (!CRM_Core_Permission::check('edit user-driven message templates')) {
+      throw new \Civi\API\Exception\UnauthorizedException(ts('Editing or creating user driven workflow messages requires edit user-driven message templates as well as edit message templates'));
+    }
+  }


Should we move this permission check block down to BAO? Intent is to shift such checks and other manual codes down to CRUD fns and to make API more dependent on respective BAO fns.

@monishdeb i guess so, that would be the only way to make this apply to v3 and v4 right?

Exactly 😅

Re api vs BAO - I don't feel like we've really had those conversations & are still mostly in the api & in ad hoc other places - so we're not really being systematic yet - but this makes sense here

monishdeb · 2018-10-05T20:05:42Z

tests/phpunit/api/v3/MessageTemplateTest.php

+    CRM_Core_Config::singleton()->userPermissionClass->permissions = array('edit message templates', 'edit system workflow message templates', 'edit user-driven message templates');
+    $this->callAPISuccess('MessageTemplate', 'create', ['id' => $userEntity['id'], 'msg_subject' => 'User template updated by system message permission', 'check_permissions' => TRUE]);
+    $this->callAPISuccess('MessageTemplate', 'create', ['id' => $entity['id'], 'msg_subject' => 'test msg permission subject', 'check_permissions' => TRUE]);
+  }


eileenmcnaughton · 2018-10-06T10:11:54Z

tests/phpunit/api/v3/MessageTemplateTest.php

@@ -55,6 +55,11 @@ public function setUp() {
    );
  }

+  public function tareDown() {


tare it down

eileenmcnaughton · 2018-10-06T10:17:31Z

CRM/Core/Permission.php

@@ -1486,8 +1486,8 @@ public static function getEntityActionPermissions() {

    $permissions['message_template'] = array(
      'get' => array('access CiviCRM'),
-      'create' => array('edit message templates', 'edit user-driven message templates', 'edit system workflow message templates'),
-      'update' => array('edit message templates', 'edit user-driven message templates', 'edit system workflow message templates'),
+      'create' => array('edit message templates'),


This would have 'worked' had the array structure been different - see the double array in this place. However, I can see that your version precise than this

'default' => array( // At minimum the user needs one of the following. Finer-grained access is controlled by CRM_Case_BAO_Case::addSelectWhereClause array('access my cases and activities', 'access all cases and activities'), ), );

eileenmcnaughton · 2018-10-06T10:19:54Z

CRM/Core/BAO/MessageTemplate.php

@@ -83,6 +83,27 @@ public static function setIsActive($id, $is_active) {
   * @return object
   */
  public static function add(&$params) {
+    // System Workflow Templates have a specific wodkflow_id in them but normal user end message templates don't
+    // If we have an id check to see if we are update, and need to check if original is a system workflow or not.
+    if (!empty($params['id']) && !empty($params['check_permissions'])) {


So my understanding is that if they have ''edit message templates'' then we don't need to check the workflow id as that permission is more like 'either'

@eileenmcnaughton my understanding was that the idea was that it would have to be edit message templates + either system or user edit permissions. i.e. getting away from just having edit message templates.

@ajesamson @guanhuan this was a Compucorp PR so comments very welcome

@seamuslee001 see #11974 - " This helps to further limit which of the templates a user can view and edit, while retaining exiting edit message templates permission for backward compability."

My understanding is that 'edit message templates' should work as it always has - to use more granular this should be removed & the more granular used instead

@seamuslee001 What @eileenmcnaughton said is correct. It is not meant to be

edit message templates + either system workflow or user-driven permissions

The essence of leaving edit message templates permission is for backward compatibility. Users are notified on upgrade (as suggested by @colemanw due to some implementation complexity) of the new permissions. They are then expected; at their own discretion, to disable edit message templates while assigning either system workflow or user-driven message templates permission (or both) to users as applicable to their system.

cc @mickadoo

yep understand now, i'm working on an update just now, that should implement that concept as well.

seamuslee001 · 2018-10-08T09:19:08Z

ok @eileenmcnaughton @ajesamson @guanhuan etal my latest fix should prove that having just edit message templates permission is enough however the API will also accept one of the granular permissions + properly validate if just the granular permission is granted

eileenmcnaughton · 2018-10-08T11:38:32Z

@seamuslee001 this is good to merge now - although it makes sense to merge to the rc

…as requireing the ssystem workflow message template permission as well Wrap permission checking in the check_permissions param Move Permission checking to BAO level from API Allow for the fact that edit message templates permission should still be able to work on both sets of templates without the granular permissions. Also reduce duplication of code abit

eileenmcnaughton · 2018-10-08T21:02:21Z

looks good

seamuslee001 · 2018-10-08T23:48:40Z

Test failures unrelated Merging as per the tag

civibot bot added the master label Oct 5, 2018

seamuslee001 force-pushed the dev_core_421 branch from 73c35ef to 1d60e7f Compare October 5, 2018 05:34

monishdeb reviewed Oct 5, 2018

View reviewed changes

seamuslee001 force-pushed the dev_core_421 branch from 1d60e7f to 330ec60 Compare October 5, 2018 21:40

eileenmcnaughton reviewed Oct 6, 2018

View reviewed changes

eileenmcnaughton mentioned this pull request Oct 6, 2018

dev/core#61 Split Edit Message Templates Permission #11974

Merged

eileenmcnaughton reviewed Oct 6, 2018

View reviewed changes

eileenmcnaughton added the sig:bug label Oct 7, 2018

seamuslee001 force-pushed the dev_core_421 branch from 330ec60 to 35372d8 Compare October 8, 2018 09:18

seamuslee001 changed the base branch from master to 5.7 October 8, 2018 21:00

civibot bot added 5.7 and removed master labels Oct 8, 2018

seamuslee001 force-pushed the dev_core_421 branch from 35372d8 to 781ed31 Compare October 8, 2018 21:01

eileenmcnaughton added the merge on pass label Oct 8, 2018

seamuslee001 merged commit 585d646 into civicrm:5.7 Oct 8, 2018

eileenmcnaughton deleted the dev_core_421 branch October 17, 2018 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dev/core#421 Fix issue where creating user driven message templates w… #12896

dev/core#421 Fix issue where creating user driven message templates w… #12896

seamuslee001 commented Oct 5, 2018 •

edited by eileenmcnaughton

Loading

civibot bot commented Oct 5, 2018

monishdeb Oct 5, 2018

seamuslee001 Oct 5, 2018

monishdeb Oct 6, 2018

eileenmcnaughton Oct 6, 2018

monishdeb Oct 5, 2018

eileenmcnaughton Oct 6, 2018

eileenmcnaughton Oct 6, 2018

eileenmcnaughton Oct 6, 2018

seamuslee001 Oct 6, 2018

eileenmcnaughton Oct 7, 2018

ajesamson Oct 8, 2018

seamuslee001 Oct 8, 2018

seamuslee001 commented Oct 8, 2018

eileenmcnaughton commented Oct 8, 2018

eileenmcnaughton commented Oct 8, 2018

seamuslee001 commented Oct 8, 2018

dev/core#421 Fix issue where creating user driven message templates w… #12896

dev/core#421 Fix issue where creating user driven message templates w… #12896

Conversation

seamuslee001 commented Oct 5, 2018 • edited by eileenmcnaughton Loading

Overview

Before

After

Technical Details

civibot bot commented Oct 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seamuslee001 commented Oct 8, 2018

eileenmcnaughton commented Oct 8, 2018

eileenmcnaughton commented Oct 8, 2018

seamuslee001 commented Oct 8, 2018

seamuslee001 commented Oct 5, 2018 •

edited by eileenmcnaughton

Loading