diff --git a/CHANGELOG.md b/CHANGELOG.md index 5dfa874..d02b134 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ # LoginLdap Changelog +#### LoginLdap 5.1.4 - 2025-01-12 +* Added code to delete all the access if no access returned from Ldap and enable_synchronize_access_from_ldap=1 + #### LoginLdap 5.1.3 - 2025-07-07 * Textual changes diff --git a/LdapInterop/UserSynchronizer.php b/LdapInterop/UserSynchronizer.php index 0b60fcf..c5ec7a0 100644 --- a/LdapInterop/UserSynchronizer.php +++ b/LdapInterop/UserSynchronizer.php @@ -188,37 +188,33 @@ public function synchronizeLdapUser($piwikLogin, $ldapUser) */ public function synchronizePiwikAccessFromLdap($piwikLogin, $ldapUser) { - if (empty($this->userAccessMapper)) { + // UserSynchronizer::makeConfigured() only sets a UserAccessMapper when Config::isAccessSynchronizationEnabled() + // is true, so return early in this case. + if (empty($this->userAccessMapper) || !Config::isAccessSynchronizationEnabled()) { return; } $userAccess = $this->userAccessMapper->getPiwikUserAccessForLdapUser($ldapUser); - if (empty($userAccess)) { - $this->logger->warning("UserSynchronizer::{func}: User '{user}' has no access in LDAP, but access synchronization is enabled.", array( - 'func' => __FUNCTION__, - 'user' => $piwikLogin - )); - - return; - } // for the synchronization, need to reset all user accesses $this->userModel->deleteUserAccess($piwikLogin); $this->userModel->setSuperUserAccess($piwikLogin, false); $usersManagerApi = $this->usersManagerApi; - foreach ($userAccess as $userAccessLevel => $sites) { - Access::doAsSuperUser(function () use ($usersManagerApi, $userAccessLevel, $sites, $piwikLogin) { - if ($userAccessLevel == 'superuser') { - if (method_exists('Piwik\Plugins\UsersManager\UserUpdater', 'setSuperUserAccessWithoutCurrentPassword')) { - $this->userUpdater->setSuperUserAccessWithoutCurrentPassword($piwikLogin, true); + if (!empty($userAccess)) { + foreach ($userAccess as $userAccessLevel => $sites) { + Access::doAsSuperUser(function () use ($usersManagerApi, $userAccessLevel, $sites, $piwikLogin) { + if ($userAccessLevel == 'superuser') { + if (method_exists('Piwik\Plugins\UsersManager\UserUpdater', 'setSuperUserAccessWithoutCurrentPassword')) { + $this->userUpdater->setSuperUserAccessWithoutCurrentPassword($piwikLogin, true); + } else { + $usersManagerApi->setSuperUserAccess($piwikLogin, true); + } } else { - $usersManagerApi->setSuperUserAccess($piwikLogin, true); + $usersManagerApi->setUserAccess($piwikLogin, $userAccessLevel, $sites); } - } else { - $usersManagerApi->setUserAccess($piwikLogin, $userAccessLevel, $sites); - } - }); + }); + } } } diff --git a/plugin.json b/plugin.json index 52c819f..154590e 100644 --- a/plugin.json +++ b/plugin.json @@ -1,6 +1,6 @@ { "name": "LoginLdap", - "version": "5.1.3", + "version": "5.1.4", "description": "LDAP authentication and synchronization for Matomo.", "theme": false, "keywords": ["ldap", "login", "authentication", "active", "directory", "kerberos", "sso"], diff --git a/tests/Unit/UserSynchronizerTest.php b/tests/Unit/UserSynchronizerTest.php index 604b458..1870a9c 100644 --- a/tests/Unit/UserSynchronizerTest.php +++ b/tests/Unit/UserSynchronizerTest.php @@ -41,6 +41,15 @@ class UserSynchronizerTest extends TestCase */ public $superUserAccess; + /** + * @var array + */ + public $deleteUserAccess; + /** + * @var array + */ + public $deleteSuperUserAccess; + public function setUp(): void { $this->userSynchronizer = new UserSynchronizer(); @@ -50,6 +59,12 @@ public function setUp(): void $this->userAccess = array(); $this->superUserAccess = array(); + $this->deleteUserAccess = array(); + $this->deleteSuperUserAccess = array(); + + $config = Config::getInstance()->LoginLdap; + $config['enable_synchronize_access_from_ldap'] = 1; + Config::getInstance()->LoginLdap = $config; } public function test_makeConfigured_DoesNotThrow_WhenUserMapperCorrectlyConfigured() @@ -137,6 +152,72 @@ public function test_synchronizePiwikAccessFromLdap_WillSetAccessCorrectly() ), $this->superUserAccess); } + public function test_synchronizePiwikAccessFromLdap_WillNotRemoveAccessWhenAccessEmptyButSyncDisabled() + { + $this->setUserManagerApiMock($throwsOnAdd = false); + // First iteration returns mapping + $this->setUserAccessMapperMock(array( + 'superuser' => array(7,8,9), + 'view' => array(1,2,3), + 'admin' => array(4,5,6) + )); + $this->userSynchronizer->synchronizePiwikAccessFromLdap('piwikuser', array()); + // Verify access mapping is present + $this->assertEquals(array( + array('piwikuser', 'view', array(1,2,3)), + array('piwikuser', 'admin', array(4,5,6)) + ), $this->userAccess); + $this->assertEquals(array( + array('piwikuser', true) + ), $this->superUserAccess); + + // Second iteration returns no mapping for the user and since enable_synchronize_access_from_ldap=0, it should do nothing + $config = Config::getInstance()->LoginLdap; + $oldValue = $config['enable_synchronize_access_from_ldap'] ?? 0; + $config['enable_synchronize_access_from_ldap'] = 0; + Config::getInstance()->LoginLdap = $config; + $this->setUserAccessMapperMock([]); + $this->userSynchronizer->synchronizePiwikAccessFromLdap('piwikuser', array()); + $this->assertEmpty($this->deleteUserAccess); + $this->assertEmpty($this->deleteSuperUserAccess); + + $config['enable_synchronize_access_from_ldap'] = $oldValue; + Config::getInstance()->LoginLdap = $config; + } + + public function test_synchronizePiwikAccessFromLdap_WillRemoveAccessWhenAccessEmptyAndSyncEnabled() + { + $this->setUserManagerApiMock($throwsOnAdd = false); + // First iteration returns mapping + $this->setUserAccessMapperMock(array( + 'superuser' => array(7,8,9), + 'view' => array(1,2,3), + 'admin' => array(4,5,6) + )); + $this->userSynchronizer->synchronizePiwikAccessFromLdap('piwikuser', array()); + // Verify access mapping is present + $this->assertEquals(array( + array('piwikuser', 'view', array(1,2,3)), + array('piwikuser', 'admin', array(4,5,6)) + ), $this->userAccess); + $this->assertEquals(array( + array('piwikuser', true) + ), $this->superUserAccess); + + // Second iteration returns no mapping for the user and since enable_synchronize_access_from_ldap=0, it should do nothing + $config = Config::getInstance()->LoginLdap; + $oldValue = $config['enable_synchronize_access_from_ldap'] ?? 0; + $config['enable_synchronize_access_from_ldap'] = 1; + Config::getInstance()->LoginLdap = $config; + $this->setUserAccessMapperMock([]); + $this->userSynchronizer->synchronizePiwikAccessFromLdap('piwikuser', array()); + $this->assertEquals(['piwikuser'], $this->deleteUserAccess); + $this->assertEquals(['piwikuser'], $this->deleteSuperUserAccess); + + $config['enable_synchronize_access_from_ldap'] = $oldValue; + Config::getInstance()->LoginLdap = $config; + } + public function test_synchronizePiwikAccessFromLdap_Succeeds_IfLdapUserHasNoAccess() { $this->setUserManagerApiMock($throwsOnAdd = false); @@ -165,12 +246,22 @@ private function setUserManagerApiMock($throwsOnAddUser, $throwsOnUpdateUser = f $mock->expects($this->any())->method('setUserAccess')->willThrowException(new Exception("dummy message")); } else { $mock->expects($this->any())->method('setUserAccess')->willReturnCallback(function ($login, $access, $sites) use ($self) { + $key = array_search($login, $self->deleteUserAccess); + if ($key !== false) { + unset($self->deleteUserAccess[$key]); + $self->deleteUserAccess = array_values($self->deleteUserAccess); + } $self->userAccess[] = array($login, $access, $sites); }); } // for previous version $mock->expects($this->any())->method('setSuperUserAccess')->willReturnCallback(function ($login, $hasSuperUserAccess) use ($self) { + $key = array_search($login, $self->deleteSuperUserAccess); + if ($key !== false && $hasSuperUserAccess) { + unset($self->deleteSuperUserAccess[$key]); + $self->deleteSuperUserAccess = array_values($self->deleteSuperUserAccess); + } $self->superUserAccess[] = array($login, $hasSuperUserAccess); }); @@ -186,6 +277,11 @@ private function setUserManagerApiMock($throwsOnAddUser, $throwsOnUpdateUser = f } $mock->expects($this->any())->method('setSuperUserAccessWithoutCurrentPassword')->willReturnCallback(function ($login, $hasSuperUserAccess) use ($self) { + $key = array_search($login, $self->deleteSuperUserAccess); + if ($key !== false && $hasSuperUserAccess) { + unset($self->deleteSuperUserAccess[$key]); + $self->deleteSuperUserAccess = array_values($self->deleteSuperUserAccess); + } $self->superUserAccess[] = array($login, $hasSuperUserAccess); }); @@ -229,6 +325,15 @@ private function setUserModelMock($returnValue) ->onlyMethods(array('getUser', 'deleteUserAccess', 'setSuperUserAccess')) ->getMock(); $mock->expects($this->any())->method('getUser')->will($this->returnValue($returnValue)); + $self = $this; + $mock->expects($this->any())->method('deleteUserAccess')->willReturnCallback(function ($login) use ($self) { + $self->deleteUserAccess[] = $login; + }); + $mock->expects($this->any())->method('setSuperUserAccess')->willReturnCallback(function ($login, $hasSuperUserAccess) use ($self) { + if (!$hasSuperUserAccess && !in_array($login, $self->deleteSuperUserAccess)) { + $self->deleteSuperUserAccess[] = $login; + } + }); $this->userSynchronizer->setUserModel($mock); }