DispatchPolicy - Actively report any upgrade problems with hook_civicrm_permission

Overview
--------

This is a preventive/diagnostic revision which would bring to light potential
problems with firing `cleanupPermissions()`/`hook_civicrm_permission` at the
wrong moment during an upgrade.

Before
------

If `cleanupPermissions()` (and its `hook_civicrm_permission`) are fired too
early during an upgrade, then the cleanup runs quietly but omits important
results (because the hook is dropped).  On several UF's, the
`cleanupPermissions()` **revokes access** for any omitted permissions.  The
sysadmin would have manually re-grant access.

After
-----

If `cleanupPermissions()` (and its `hook_civicrm_permission`) are fired too
early during an upgrade, then it raises an error during the upgrade.

Surely, it's better to report the error rather than silently drop the data.

Comments
--------

* This revision is preventive/diagnostic/speculative.  It's based on rumor that
  somebody had a problem with permissions in an upgrade.  I don't actually have
  steps to reproduce a probelmatic case.

* To see this preventive mechanism in action, I provoked a trivial violation:
    * Setup a DB with 5.32
    * Checkout the code for 5.34.
    * Add a local hack https://gist.github.com/totten/e1448b343f94ff9c3d971402a1b3db3d
      which has the effect of running `cleanupPermissions()` prematurely.
    * Observe: `cv upgrade:db` fails (cv v0.3.5+)

* It's tempting to think of `hook_civicrm_permission` as returning a static list of
  strings -- in which case, there's no real harm to firing during the defensive
  phase.  The problem is that `hook_civicrm_permission` can also be used for
  dynamic scenarios.  (The power of hooks!) That's useful for any site-building
  extension (e.g.  bespoke forms/views/APIs/dashlets) where you generate new
  permissions based on site configuration.  That will depend on the
  correct-functioning of the extension+configuration...  which cannot be
  ensured during the defensive upgrade-phase...  and which creates a parallel
  choice between incorrect-results (data-loss) or fatal-error.  This is the
  real reason why one shouldn't run `cleanupPermissions()` during the defensive
  phase.  (Of course, we should run it...  during the liberal phase...)

diff --git a/CRM/Upgrade/DispatchPolicy.php b/CRM/Upgrade/DispatchPolicy.php
index de0fa72511d554bb988d2100a4ea0eeae97f93d9..fa450cc2bb60d5a87ada67925e98a81fe9d48d33 100644 (file)
--- a/CRM/Upgrade/DispatchPolicy.php
+++ b/CRM/Upgrade/DispatchPolicy.php
@@ -59,6 +59,8 @@ class CRM_Upgrade_DispatchPolicy {
     // It's more restrictive, preventing interference from unexpected callpaths.
     $policies['upgrade.main'] = [
       'hook_civicrm_config' => 'run',
+      // cleanupPermissions() in some UF's can be destructive. Running prematurely could be actively harmful.
+      'hook_civicrm_permission' => 'fail',
       '/^hook_civicrm_(pre|post)$/' => 'drop',
       '/^hook_civicrm_/' => $strict ? 'warn-drop' : 'drop',
       '/^civi\./' => 'run',