Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APIv4 - Add "Permission.get" for listing available permissions #19115

Merged
merged 4 commits into from
Dec 10, 2020

Conversation

totten
Copy link
Member

@totten totten commented Dec 4, 2020

Overview

This adds a new API method "Permission.get". This is a better building-block for administrative tools that allow one to choose/assign a permission.

Before

  • There is a method CRM_Core_Permission::basicPermissions() which lists concrete permissions defined by CiviCRM (built-in as well as hook_civicrm_permission).
  • There are a number of additional permissions notations that cannot be discovered, including:
    • Constant permissions (e.g. *always allow*, *always deny*)
    • CMS-specific permissions (e.g. Drupal:post comment, WordPress:list_users)
    • Synthetic permissions which are mapped to different CMS equivalents (e.g. cms:view user account)
    • Synthetic permissions defined via extension (e.g. @afform:<form-name>)

After

The APIv4 method Permission.get provides a full accounting of all available permissions. It also supports sorting, filtering, etc.

Screen Shot 2020-12-03 at 11 45 03 PM

This captures permissions from all the sources referenced above, eg

  • CiviCRM permissions (built-in as well as hook_civicrm_permission)
  • Constant permissions (e.g. *always allow*, *always deny*)
  • CMS-specific permissions (e.g. Drupal:post comment, WordPress:list_users)
  • Synthetic permissions which are mapped to different CMS equivalents (e.g. cms:view user account)
  • Synthetic permissions defined via extension (e.g. @afform:<form-name>)

For the last case (synthetic permissions defined via extension), one must implement hook_civicrm_permissionList. There's an example in afform.php:

function afform_civicrm_permissionList(&$permissions) {
  $scanner = Civi::service('afform_scanner');
  foreach ($scanner->getMetas() as $name => $meta) {
    $permissions['@afform:' . $name] = [
      'group' => 'afform',
      'title' => ts('Afform: Inherit permission of %1', [
        1 => $name,
      ]),
    ];
  }
}

Comments

  • This patch does not add, remove, or rename any specific permissions - it makes it easier to discover existing permissions.
  • The UF layer didn't have a mechanism to enumerate the CMS-specific permissions. So...
    • I added support for Backdrop, Drupal 7, Drupal 8(+9?), and WordPress. Tested locally.
    • For Joomla, it won't list CMS-specific permissions. (I don't have a sane setup for patching Joomla.) But all the other permissions (constant & synthetic) should work.
  • The unit-test is written as end-to-end. This allows it to provide some coverage for the CMS-specific bits.

@civibot
Copy link

civibot bot commented Dec 4, 2020

(Standard links)

@civibot civibot bot added the master label Dec 4, 2020
@totten
Copy link
Member Author

totten commented Dec 4, 2020

@jaapjansma You might find this to be a useful alternative to \CRM_Core_Permission::basicPermissions() for dataprocessors admin UI. (JFYI)

Civi/Core/Container.php Outdated Show resolved Hide resolved
Civi/Api4/Permission.php Outdated Show resolved Hide resolved
@colemanw
Copy link
Member

colemanw commented Dec 4, 2020

This looks good overall. We'll need docs for the new hook.

@colemanw
Copy link
Member

colemanw commented Dec 5, 2020

This looks ready to merge @totten.
Can you squash the commits?

@eileenmcnaughton
Copy link
Contributor

@totten @colemanw my quick take is I'm not sure the commits need squashing - the idea is that if a change makes sense on it own (even incrementally) it's OK f(& possibly better) for it to be a separate commit.

@totten if after reading the comments about squashing you feel that the commits don't need squashing please merge

@colemanw
Copy link
Member

colemanw commented Dec 8, 2020

@totten it looks to me like the 2nd, 3rd, 5th and 6th commit are all basically the same area and could be squashed together.

@totten
Copy link
Member Author

totten commented Dec 9, 2020

@colemanw Squashed the commits which implement "Permission.get". Left the other the bits (CRM_Core_Permission, E2E test, afform) as they're more distinct.

@seamuslee001 seamuslee001 merged commit d27ac84 into civicrm:master Dec 10, 2020
@totten totten deleted the master-api4-perm branch December 10, 2020 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants