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

ajesamson · 2018-04-13T14:13:59Z

Overview

The edit message templates permission currently grants access to user-driven messages and system workflow message. The implication of this is that, there is no way of restricting a particular user to just user-driven messages or system workflow messages.

This PR adds 2 new granular permissions called edit user-driven message templates and edit system workflow message templates. 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.

Before

After

Technical Details

New permissions were added to existing ones

      'edit system workflow message templates' => array(
        $prefix . ts('edit system workflow message templates'),
      ),
      'edit user-driven message templates' => array(
        $prefix . ts('edit user-driven message templates'),
      ),

and applied to message template urls.

<item>
     <path>civicrm/admin/messageTemplates</path>
     <title>Message Templates</title>
     <desc>Message templates allow you to save and re-use messages with layouts which you can use when sending email to one or more contacts.</desc>
     <page_callback>CRM_Admin_Page_MessageTemplates</page_callback>
     <adminGroup>Communications</adminGroup>
     <icon>admin/small/template.png</icon>
     <access_arguments>edit message templates;edit user-driven message templates;edit system workflow message templates</access_arguments>
     <weight>30</weight>
  </item>

Permission checks were applied on the view page, restricting tab access. Checks were also carried out while building the form to ensure users with one permission, does not edit forms belonging to other permission group.

  private function checkUserPermission($workflowId) {
    if (isset($workflowId)) {
      $canView = CRM_Core_Permission::check('edit system workflow message templates');
    } else {
      $canView = CRM_Core_Permission::check('edit user-driven message templates');
    }

    if (! $canView && ! CRM_Core_Permission::check('edit message templates')) {
      CRM_Core_Session::setStatus(ts('You do not have permission to view requested page.'), ts('Access Denied'));
      $url = CRM_Utils_System::url('civicrm/admin/messageTemplates', "reset=1");
      CRM_Utils_System::redirect($url);
    }
  }

For existing users to be aware of these changes, a pre-upgrade message notification was provided. This affords them the opportunity of deciding whether to upgrade existing user permissions or not.

if ($rev == '5.2.alpha1') {
  $params = array(
    1 => 'edit user-driven message templates',
    2 => 'edit system workflow message templates',
    3 => 'edit message templates',
  );
  $preUpgradeMessage .= '<p>' . ts('New granular permissions called %1 and %2 have been added for %3 permission. These permissions help to limit user access per template', $params) . '</p>';
}

civicrm-builder · 2018-04-13T14:14:01Z

Can one of the admins verify this patch?

seamuslee001 · 2018-04-13T22:12:28Z

Jenkins ok to test

eileenmcnaughton · 2018-04-16T07:33:59Z

@mattwire perhaps you can trade with Compucorp to review one of yours? I know you want review on your membership tidy up one.

I think the idea of splitting up the permissions makes sense in principle. I'm a bit alarmed to see those php calls from the tpl file but I'm sure there are ample other places where we check permissions to compare to. If we really don't have an existing smarty permission check maybe we should add one in CRM_Core_Smarty_Plugins?

eileenmcnaughton · 2018-04-16T07:35:05Z

CRM/Core/xml/Menu/Admin.xml

@@ -262,15 +262,15 @@
     <page_callback>CRM_Admin_Page_MessageTemplates</page_callback>
     <adminGroup>Communications</adminGroup>
     <icon>admin/small/template.png</icon>
-     <access_arguments>edit message templates</access_arguments>
+     <access_arguments>edit message templates;edit user-driven message templates;edit system workflow message templates</access_arguments>


I see the xml sometimes uses commas & sometimes semi-colons - do they have the same meaning?

I think it’s the diff between or and I think ; == or , == and

I think Seamus is right

mickadoo

looks pretty good overall, some minor comments

mickadoo · 2018-04-16T11:19:21Z

templates/CRM/Admin/Page/MessageTemplates.tpl

-      <li id='tab_user'>    <a href='#user'     title='{ts}User-driven Messages{/ts}'>    {ts}User-driven Messages{/ts}    </a></li>
-      <li id='tab_workflow'><a href='#workflow' title='{ts}System Workflow Messages{/ts}'>{ts}System Workflow Messages{/ts}</a></li>
+      {if $canEditUserDrivenMessageTemplates or $canEditMessageTemplates}
+        <li id='tab_user'>    <a href='#user'     title='{ts}User-driven Messages{/ts}'>    {ts}User-driven Messages{/ts}    </a></li>


I know it wasn't something you introduced, but can you remove the extra spaces here like in the second link below?

Alright i will.

mickadoo · 2018-04-16T11:26:34Z

CRM/Admin/Form/MessageTemplates.php

+   * @param int $workflowId
+   */
+  private function checkUserPermission($workflowId) {
+    if (isset($workflowId)) {


I'm not really familiar with message templates so just a question here, is it impossible for user driven message templates to have a workflow ID?

User driven message templates do not have workflow ID. A template becomes system workflow message template the moment it has workflow ID.

mickadoo

Looks ok to me, it'll require someone (non compucorp) to review too though

mickadoo · 2018-04-16T13:37:33Z

Explain (r-explain)
- PASS : The goal/problem/solution have been adequately explained in the PR.
Test results (r-test)
- UNREVIEWED
Code quality (r-code)
- PASS: The functionality, purpose, and style of the code seems clear+sensible.
Documentation (r-doc)
- UNREVIEWED
- COMMENTS: Do you think this needs a documentation change @ajesamson?
Maintainability (r-maint)
- UNREVIEWED
Run it (r-run)
- UNREVIEWED
User impact (r-user)
- UNREVIEWED

eileenmcnaughton · 2018-04-16T20:51:47Z

test this please

colemanw · 2018-04-17T17:23:41Z

@eileenmcnaughton

If we really don't have an existing smarty permission check maybe we should add one in CRM_Core_Smarty_Plugins?

I just looked into this because it's a very sensible suggestion, but the problem is that smarty functions can't be called from within {if} statements, as far as I can tell. So you can create a smarty function that behaves like this {crmCheck permission="administer CiviCRM"} but then how would you use it? I'm pretty sure you can't do {if {foo} } Conditional something {/if}

colemanw · 2018-04-17T17:26:49Z

Hi @ajesamson thanks for this contribution. I'm just looking at it for the first time and my initial question is about whether the original permission indeed ought to be kept around. That seems needlessly confusing, as it will have significant overlap with the new permissions.

eileenmcnaughton · 2018-04-17T20:51:33Z

@colemanw ah yeah - you'd need to add in a capture I suppose which starts to get a bit complex

ajesamson · 2018-04-17T21:12:01Z

Hi @colemanw the reason why it was left behind is more or less for backward compatibility. It was meant to allow existing users with permission continue working without impacting their process on upgrade. Your suggestions are highly welcomed on how best to handle this.

mattwire · 2018-04-19T16:36:08Z

@ajesamson I would have thought we could remove the old permission, making sure we add an upgrader message (there was one when batch permissions were added) telling the user that they will need to reconfigure permissions for message templates. The only other place it may cause a problem is if that permission is used in extensions, @totten normally says "grep universe" to find out.

colemanw · 2018-04-19T16:41:55Z

I agree. As part of the upgrade, I think all users with the old permission should be granted the 2 new ones. And the upgrade message can give admins a heads up that they now have finer control if they want it.

eileenmcnaughton · 2018-04-22T02:21:22Z

Do we actually have an effective methodology for adding permissions during upgrade?

ajesamson · 2018-05-08T11:15:43Z

@colemanw Please can you provide a guide on how best to migrate the old permission for existing users to 2 new ones. I have spent some time looking through the code base but could not get a method that provides easy access to CMS permission table for such upgrade.

The suggested sample code by @mattwire used setPreUpgradeMessage and left decision to the user. Also, i discovered there is another extension depending on edit message template after grep universe. It might be advisable to just add upgrade notification.

eileenmcnaughton · 2018-05-08T20:20:01Z

@ajesamson @colemanw I think adding perms on upgrade across the CMS is too hard looking at it. I think historically we have set a message on upgrade & that will have to do

  /**
   * Compute any messages which should be displayed beforeupgrade.
   *
   * Note: This function is called iteratively for each upcoming
   * revision to the database.
   *
   * @param string $preUpgradeMessage
   * @param string $rev
   *   a version number, e.g. '4.4.alpha1', '4.4.beta3', '4.4.0'.
   * @param null $currentVer
   */
  public function setPreUpgradeMessage(&$preUpgradeMessage, $rev, $currentVer = NULL) {
    if ($rev == '4.7.alpha1') {
      // CRM-16478 Remove custom fatal error template path option
      $config = CRM_Core_Config::singleton();
      if (!empty($config->fatalErrorTemplate) && $config->fatalErrorTemplate != 'CRM/common/fatal.tpl') {
        $preUpgradeMessage .= '<p>' . ts('The custom fatal error template setting will be removed during the upgrade. You are currently using this custom template: %1 . Following the upgrade you will need to use the standard approach to overriding template files, as described in the documentation.', array(1 => $config->fatalErrorTemplate)) . '</p>';
      }
    }
    if ($rev == '4.7.alpha4') {
      // CRM-17004 Warn of Moneris removal
      $count = 1;
      // Query only works in 4.3+
      if (version_compare($currentVer, "4.3.0") > 0) {
        $count = CRM_Core_DAO::singleValueQuery("SELECT COUNT(id) FROM civicrm_payment_processor WHERE payment_processor_type_id IN (SELECT id FROM civicrm_payment_processor_type WHERE name = 'Moneris')");
      }
      if ($count && !function_exists('moneris_civicrm_managed')) {
        $preUpgradeMessage .= '<p>' . ts('The %1 payment processor is no longer bundled with CiviCRM. After upgrading you will need to install the extension to continue using it.', array(1 => 'Moneris')) . '</p>';
      }
    }
    if ($rev == '4.7.13') {
      $preUpgradeMessage .= '<p>' . ts('A new permission has been added called %1 This Permission is now used to control access to the Manage Tags screen', array(1 => 'manage tags')) . '</p>';
    }
    if ($rev == '4.7.22') {
      // Based on support inquiries for 4.7.21, show message during 4.7.22.
      // For affected users, this issue prevents loading the regular status screen.
      if (!$this->checkImageUploadDir()) {
        $preUpgradeMessage .= '<p>' . ts('There appears to be an inconsistency in the configuration of "Image Upload URL" and "Image Upload Directory".') . '</p>'
          . '<p>'
          . ts('Further advice will be displayed at the end of the upgrade.')
          . '</p>';
      }
    }
    if ($rev == '4.7.27') {
      $params = array(
        1 => 'Close accounting batches created by user',
        2 => 'Close all accounting batches',
        3 => 'Reopen accounting batches created by user',
        4 => 'Reopen all accounting batches',
        5 => 'https://wiki.civicrm.org/confluence/display/CRMDOC/Default+Permissions+and+Roles',
      );
      $preUpgradeMessage .= '<p>' . ts('A new set of batch permissions has been added called "%1", "%2", "%3" and "%4". These permissions are now used to control access to the Accounting Batches tasks. If your users need to be able to Reopen or Close batches you may need to give them additional permissions. <a href=%5>Read more</a>', $params) . '</p>';
    }
    if ($rev == '4.7.32') {
      $preUpgradeMessage .= '<p>' . ts('A new %1 permission has been added. It is not granted by default. If you use SMS, you may wish to review your permissions.', array(1 => 'send SMS')) . '</p>';
    }
  }

…r permissions

mickadoo

re-approving after revision changes

eileenmcnaughton · 2018-05-31T11:41:27Z

OK - I'm happy to go with @mickadoo's re-review on this. It seems to have been conceptually agreed & the code makes sense from a quick skim so I don't have any concerns over & above @mickadoo having tested /reviewed

seamuslee001 · 2018-05-31T11:45:02Z

CRM/Upgrade/Incremental/php/FiveTwo.php

-    // if ($rev == '5.12.34') {
-    //   $preUpgradeMessage .= '<p>' . ts('A new permission has been added called %1 This Permission is now used to control access to the Manage Tags screen', array(1 => 'manage tags')) . '</p>';
-    // }
+    if ($rev == '5.3.0') {


@eileenmcnaughton can you move this code into FiveThree.php

ah dang - didn't spot that!

eileenmcnaughton · 2018-10-06T10:13:35Z

This appears to have caused a regression - see #12896

dev/core#61 split edit message templates permission

e86f2eb

ajesamson mentioned this pull request Apr 13, 2018

PCHR-3567: Restrict Message Template Form Access Based on Permission compucorp/civihr#2570

Closed

eileenmcnaughton reviewed Apr 16, 2018

View reviewed changes

civicrm-builder added the master label Apr 16, 2018

ajesamson force-pushed the 61-split-edit-message-templates-permission branch from 2665aa1 to 4766edc Compare April 16, 2018 10:13

mickadoo suggested changes Apr 16, 2018

View reviewed changes

dev/core#61: Code refactor and jenkins build failure resolution

3b23dae

ajesamson force-pushed the 61-split-edit-message-templates-permission branch from 4766edc to 3b23dae Compare April 16, 2018 11:56

mickadoo approved these changes Apr 16, 2018

View reviewed changes

ajesamson added 2 commits May 28, 2018 17:48

dev/core#61: Added pre-upgrade message notification for added granula…

9faa733

…r permissions

dev/core#61: Updated revision number

50e2850

mickadoo approved these changes May 31, 2018

View reviewed changes

eileenmcnaughton merged commit 40a732a into civicrm:master May 31, 2018

seamuslee001 reviewed May 31, 2018

View reviewed changes

eileenmcnaughton mentioned this pull request May 31, 2018

Move upgrade message to right file #12242

Merged

This was referenced Jun 4, 2018

PCHR-3521: Split CiviCRM Edit Message Template Permission compucorp/civicrm-core#23

Closed

PCHR-3521: Split CiviCRM Edit Message Template compucorp/civicrm-core#24

Closed

seamuslee001 mentioned this pull request Oct 6, 2018

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

Merged

alifrumin mentioned this pull request Jan 7, 2021

Update permissions for "CiviCRM Admin Menu -> Mailings -> Message Templates" nav item #19342

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ajesamson commented Apr 13, 2018 •

edited

Loading

civicrm-builder commented Apr 13, 2018

seamuslee001 commented Apr 13, 2018

eileenmcnaughton commented Apr 16, 2018

eileenmcnaughton Apr 16, 2018

seamuslee001 Apr 16, 2018

mickadoo Apr 16, 2018

mickadoo left a comment

mickadoo Apr 16, 2018

ajesamson Apr 16, 2018

mickadoo Apr 16, 2018

ajesamson Apr 16, 2018

mickadoo left a comment

mickadoo commented Apr 16, 2018

eileenmcnaughton commented Apr 16, 2018

colemanw commented Apr 17, 2018

colemanw commented Apr 17, 2018

eileenmcnaughton commented Apr 17, 2018

ajesamson commented Apr 17, 2018

mattwire commented Apr 19, 2018

colemanw commented Apr 19, 2018

eileenmcnaughton commented Apr 22, 2018

ajesamson commented May 8, 2018 •

edited

Loading

eileenmcnaughton commented May 8, 2018

mickadoo left a comment

eileenmcnaughton commented May 31, 2018

seamuslee001 May 31, 2018

eileenmcnaughton May 31, 2018

eileenmcnaughton commented Oct 6, 2018

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

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

Conversation

ajesamson commented Apr 13, 2018 • edited Loading

Overview

Before

After

Technical Details

civicrm-builder commented Apr 13, 2018

seamuslee001 commented Apr 13, 2018

eileenmcnaughton commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickadoo left a 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

mickadoo left a comment

Choose a reason for hiding this comment

mickadoo commented Apr 16, 2018

eileenmcnaughton commented Apr 16, 2018

colemanw commented Apr 17, 2018

colemanw commented Apr 17, 2018

eileenmcnaughton commented Apr 17, 2018

ajesamson commented Apr 17, 2018

mattwire commented Apr 19, 2018

colemanw commented Apr 19, 2018

eileenmcnaughton commented Apr 22, 2018

ajesamson commented May 8, 2018 • edited Loading

eileenmcnaughton commented May 8, 2018

mickadoo left a comment

Choose a reason for hiding this comment

eileenmcnaughton commented May 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Oct 6, 2018

ajesamson commented Apr 13, 2018 •

edited

Loading

ajesamson commented May 8, 2018 •

edited

Loading