Skip to content

Adds code to delete all the access if no access returned from Ldap and enable_synchronize_access_from_ldap=1, #PG-4868 #415

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.

Sign up for GitHub

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

Merged
AltamashShaikh merged 9 commits into from
Jan 13, 2026
Merged

Adds code to delete all the access if no access returned from Ldap and enable_synchronize_access_from_ldap=1, #PG-4868 #415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cb16418
Adds code to delete all the access if no access returned from Ldap an…
AltamashShaikh Jan 7, 2026
7c41c14
PHPCS fixes
AltamashShaikh Jan 7, 2026
47f4474
apply pr feedback
AltamashShaikh Jan 8, 2026
4dc59a6
if added back for tests to pass
AltamashShaikh Jan 8, 2026
226f686
Reset config values
AltamashShaikh Jan 8, 2026
20720eb
Default to 0
AltamashShaikh Jan 8, 2026
ff244bb
Update the logic to return if sync disabled and tests updated
AltamashShaikh Jan 9, 2026
53b25f4
Added not empty check
AltamashShaikh Jan 12, 2026
72fd2d0
Updated release date
AltamashShaikh Jan 12, 2026
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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

Expand Down
34 changes: 15 additions & 19 deletions LdapInterop/UserSynchronizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
});
});
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion plugin.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"],
Expand Down
105 changes: 105 additions & 0 deletions tests/Unit/UserSynchronizerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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()
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
});

Expand All @@ -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);
});

Expand Down Expand Up @@ -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);
}
Expand Down