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

dev/core#4993 block Sync CMS Users form and functionality on Standalone #29351

Merged
merged 4 commits into from
Mar 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion CRM/Admin/Form/CMSUser.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,15 @@ class CRM_Admin_Form_CMSUser extends CRM_Core_Form {
*/
public $submitOnce = TRUE;

/**
* Disable on Standalone
*/
public function preProcess() {
if (!\CRM_Utils_System::allowSynchronizeUsers()) {
\CRM_Core_Error::statusBounce(ts('This framework doesn\'t allow for syncing CMS users.'));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is a draft but just noting two things:

  • A year or two ago we tried to remove most of the if Drupal from various files to make it more OO-ey. I know in standalone sometimes it hasn't booted yet and so there's no choice, but here I think it would be better to do something like if !$config->userSystem->allowsUserSync() and then CRM_Utils_System_Base::allowsUserSync would return true, and CRM_Utils_System_Standalone::allowsUserSync() would return false. In theory this also allows mocking both variations in backend unit tests (although it's not currently set up for more than one unittest mock).
  • In forms we usually use CRM_Core_Error::statusBounce instead of throwing exceptions. Even if it were to throw an exception it's preferred to use CRM_Core_Exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for these pointers 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This got me thinking -- it's specifically standaloneusers that the syncing is incompatible with. So I've made it conditional on that, leaving open the door for alternatives where syncing is a thing (external auth providers maybe?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ufundo 🤯 I think external auth providers might be dealt with in a way other than replacing the whole of standaloneusers. But ok!

}

/**
* Build the form object.
*/
Expand All @@ -47,7 +56,7 @@ public function buildQuickForm() {
* Process the form submission.
*/
public function postProcess() {
$result = CRM_Utils_System::synchronizeUsers();
$result = CRM_Utils_System::synchronizeUsersIfAllowed();

$status = ts('Checked one user record.',
[
Expand Down
11 changes: 11 additions & 0 deletions CRM/Extension/Manager.php
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,17 @@ public function getStatus($key) {
}
}

/**
* Check if a given extension is installed and enabled
*
* @param $key
*
* @return bool
*/
public function isEnabled($key) {
return ($this->getStatus($key) === self::STATUS_INSTALLED);
}

/**
* Check if a given extension is incompatible with this version of CiviCRM
*
Expand Down
21 changes: 21 additions & 0 deletions CRM/Utils/System/Base.php
Original file line number Diff line number Diff line change
Expand Up @@ -966,6 +966,27 @@ public function synchronizeUsers() {
return [];
}

/**
* Whether to allow access to CMS user sync action
* @return bool
*/
public function allowSynchronizeUsers() {
return TRUE;
}

/**
* Run CMS user sync if allowed, otherwise just returns empty array
* @return array
*/
public function synchronizeUsersIfAllowed() {
if ($this->allowSynchronizeUsers()) {
return $this->synchronizeUsers();
}
else {
return [];
}
}

/**
* Send an HTTP Response base on PSR HTTP RespnseInterface response.
*
Expand Down
8 changes: 5 additions & 3 deletions CRM/Utils/System/Standalone.php
Original file line number Diff line number Diff line change
Expand Up @@ -420,10 +420,12 @@ public function getUserIDFromUserObject($user) {
}

/**
* @inheritDoc
* CMS User Sync doesn't make sense when using standaloneusers
* (but leave open the door for other user extensions, which might have a sync method)
* @return bool
*/
public function synchronizeUsers() {
return Security::singleton()->synchronizeUsers();
public function allowSynchronizeUsers() {
return !\CRM_Extension_System::singleton()->getManager()->isEnabled('standaloneusers');
}

/**
Expand Down
2 changes: 1 addition & 1 deletion ext/authx/tests/phpunit/Civi/Authx/AbstractFlowsTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public static function setUpBeforeClass(): void {
->installMe(__DIR__)
->callback(
function() {
\CRM_Utils_System::synchronizeUsers();
\CRM_Utils_System::synchronizeUsersIfAllowed();
},
'synchronizeUsers'
)
Expand Down
27 changes: 19 additions & 8 deletions ext/standaloneusers/CRM/Standaloneusers/Upgrader.php
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
<?php
use CRM_Standaloneusers_ExtensionUtil as E;
use Civi\Api4\MessageTemplate;
use Civi\Api4\Navigation;

/**
* Collection of upgrade steps.
Expand Down Expand Up @@ -99,18 +100,28 @@ protected function createPasswordResetMessageTemplate() {
// }

/**
* Example: Run a simple query when a module is enabled.
* On enable:
* - disable the user sync menu item
*/
// public function enable() {
// CRM_Core_DAO::executeQuery('UPDATE foo SET is_active = 1 WHERE bar = "whiz"');
// }
public function enable() {
// standaloneusers is incompatible with user sync, so disable this nav menu item
Navigation::update(FALSE)
->addWhere('url', '=', 'civicrm/admin/synchUser?reset=1')
->addValue('is_active', FALSE)
->execute();
}

/**
* Example: Run a simple query when a module is disabled.
* On disable:
* - re-enable the user sync menu item
*/
// public function disable() {
// CRM_Core_DAO::executeQuery('UPDATE foo SET is_active = 0 WHERE bar = "whiz"');
// }
public function disable() {
// reinstate user sync menu item
Navigation::update(FALSE)
->addWhere('url', '=', 'civicrm/admin/synchUser?reset=1')
->addValue('is_active', TRUE)
->execute();
}

/**
* Example: Run a couple simple queries.
Expand Down
15 changes: 0 additions & 15 deletions ext/standaloneusers/Civi/Standalone/Security.php
Original file line number Diff line number Diff line change
Expand Up @@ -279,21 +279,6 @@ public function getCMSPermissionsUrlParams() {
return ['ufAccessURL' => '/civicrm/admin/roles'];
}

/**
* Since our User entity contains a FK to a contact, it's not possible for a User to exist without a contact.
*
* @todo review this (what if contact is deleted?)
*/
public function synchronizeUsers() {

$userCount = \Civi\Api4\User::get(FALSE)->selectRowCount()->execute()->countMatched();
return [
'contactCount' => $userCount,
'contactMatching' => $userCount,
'contactCreated' => 0,
];
}

/**
* High level function to encrypt password using the site-default mechanism.
*/
Expand Down
2 changes: 1 addition & 1 deletion tests/phpunit/CiviTest/CiviEndToEndTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public static function setUpBeforeClass(): void {
'name' => $GLOBALS['_CV']['ADMIN_USER'],
'pass' => $GLOBALS['_CV']['ADMIN_PASS'],
]);
CRM_Utils_System::synchronizeUsers();
CRM_Utils_System::synchronizeUsersIfAllowed();

parent::setUpBeforeClass();
}
Expand Down
2 changes: 1 addition & 1 deletion tests/phpunit/E2E/Cache/CacheTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public static function setUpBeforeClass(): void {
'name' => $GLOBALS['_CV']['ADMIN_USER'],
'pass' => $GLOBALS['_CV']['ADMIN_PASS'],
]);
CRM_Utils_System::synchronizeUsers();
\CRM_Utils_System::synchronizeUsersIfAllowed();

parent::setUpBeforeClass();
}
Expand Down
2 changes: 1 addition & 1 deletion tests/phpunit/E2E/Extern/AuthxRestTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public static function setUpBeforeClass(): void {
->install(['authx'])
->callback(
function() {
\CRM_Utils_System::synchronizeUsers();
\CRM_Utils_System::synchronizeUsersIfAllowed();
},
'synchronizeUsers'
)
Expand Down