From a9033ca787ca3013e64716e887b79106c7a5103c Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Mon, 14 Dec 2020 13:13:21 -0800 Subject: [PATCH] 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 | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CRM/Upgrade/DispatchPolicy.php b/CRM/Upgrade/DispatchPolicy.php index de0fa72511..fa450cc2bb 100644 --- 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', -- 2.25.1