DispatchPolicy - Actively report any upgrade problems with hook_civicrm_permission
authorTim Otten <totten@civicrm.org>
Mon, 14 Dec 2020 21:13:21 +0000 (13:13 -0800)
committerTim Otten <totten@civicrm.org>
Tue, 15 Dec 2020 00:16:03 +0000 (16:16 -0800)
commita9033ca787ca3013e64716e887b79106c7a5103c
treedcf2f62c74b60d297c68bc3de7a8f853b28bfc94
parent42a440e48094a231e7c6cfbb527b82b48cfd19c1
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...)
CRM/Upgrade/DispatchPolicy.php