Skip to content

Commit ff244bb

Browse files
AltamashShaikhAltamashShaikh
committed
Update the logic to return if sync disabled and tests updated
1 parent 20720eb commit ff244bb

File tree

2 files changed

+68
-69
lines changed

2 files changed

+68
-69
lines changed

‎LdapInterop/UserSynchronizer.php‎

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
use Piwik\Plugins\UsersManager\UserUpdater;
2222
use Piwik\Site;
2323
use Piwik\Log\LoggerInterface;
24-
use Piwik\Tracker\Cache;
2524

2625
/**
2726
* Synchronizes LDAP user information with the Piwik database.
@@ -189,25 +188,13 @@ public function synchronizeLdapUser($piwikLogin, $ldapUser)
189188
*/
190189
public function synchronizePiwikAccessFromLdap($piwikLogin, $ldapUser)
191190
{
192-
if (empty($this->userAccessMapper)) {
191+
// UserSynchronizer::makeConfigured() only sets a UserAccessMapper when Config::isAccessSynchronizationEnabled()
192+
// is true, so return early in this case.
193+
if (empty($this->userAccessMapper) || !Config::isAccessSynchronizationEnabled()) {
193194
return;
194195
}
195196

196197
$userAccess = $this->userAccessMapper->getPiwikUserAccessForLdapUser($ldapUser);
197-
if (empty($userAccess)) {
198-
if (Config::isAccessSynchronizationEnabled()) {
199-
$this->logger->log('warning', "UserSynchronizer::{func}: User '{user}' has no access in LDAP, but access synchronization is enabled. Deleting access if any present.", array(
200-
'func' => __FUNCTION__,
201-
'user' => $piwikLogin
202-
));
203-
// Delete the access if isAccessSynchronizationEnabled and no userAccess is mapped
204-
$this->userModel->deleteUserAccess($piwikLogin);
205-
$this->userModel->setSuperUserAccess($piwikLogin, false);
206-
Cache::deleteTrackerCache();
207-
}
208-
209-
return;
210-
}
211198

212199
// for the synchronization, need to reset all user accesses
213200
$this->userModel->deleteUserAccess($piwikLogin);

‎tests/Unit/UserSynchronizerTest.php‎

Lines changed: 65 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
use Piwik\Plugins\LoginLdap\LdapInterop\UserSynchronizer;
1919
use Piwik\Plugins\UsersManager\Model;
2020
use Piwik\Plugins\UsersManager\UserAccessFilter;
21-
use Piwik\Tests\Framework\Mock\FakeLogger;
2221

2322
/**
2423
* @group LoginLdap
@@ -42,6 +41,15 @@ class UserSynchronizerTest extends TestCase
4241
*/
4342
public $superUserAccess;
4443

44+
/**
45+
* @var array
46+
*/
47+
public $deleteUserAccess;
48+
/**
49+
* @var array
50+
*/
51+
public $deleteSuperUserAccess;
52+
4553
public function setUp(): void
4654
{
4755
$this->userSynchronizer = new UserSynchronizer();
@@ -51,6 +59,12 @@ public function setUp(): void
5159

5260
$this->userAccess = array();
5361
$this->superUserAccess = array();
62+
$this->deleteUserAccess = array();
63+
$this->deleteSuperUserAccess = array();
64+
65+
$config = Config::getInstance()->LoginLdap;
66+
$config['enable_synchronize_access_from_ldap'] = 1;
67+
Config::getInstance()->LoginLdap = $config;
5468
}
5569

5670
public function test_makeConfigured_DoesNotThrow_WhenUserMapperCorrectlyConfigured()
@@ -140,19 +154,14 @@ public function test_synchronizePiwikAccessFromLdap_WillSetAccessCorrectly()
140154

141155
public function test_synchronizePiwikAccessFromLdap_WillNotRemoveAccessWhenAccessEmptyButSyncDisabled()
142156
{
143-
$logger = new FakeLogger();
144-
$userSynchronizer = new UserSynchronizer($logger);
145-
$this->setUserModelMock($this->getPiwikUserData(), $userSynchronizer);
146-
$this->setUserMapperMock($this->getPiwikUserData(), $userSynchronizer);
147-
$this->setUserManagerApiMock($throwsOnAdd = false, false, false, $userSynchronizer);
148-
157+
$this->setUserManagerApiMock($throwsOnAdd = false);
149158
// First iteration returns mapping
150159
$this->setUserAccessMapperMock(array(
151160
'superuser' => array(7,8,9),
152161
'view' => array(1,2,3),
153162
'admin' => array(4,5,6)
154-
), $userSynchronizer);
155-
$userSynchronizer->synchronizePiwikAccessFromLdap('piwikuser', array());
163+
));
164+
$this->userSynchronizer->synchronizePiwikAccessFromLdap('piwikuser', array());
156165
// Verify access mapping is present
157166
$this->assertEquals(array(
158167
array('piwikuser', 'view', array(1,2,3)),
@@ -167,28 +176,25 @@ public function test_synchronizePiwikAccessFromLdap_WillNotRemoveAccessWhenAcces
167176
$oldValue = $config['enable_synchronize_access_from_ldap'] ?? 0;
168177
$config['enable_synchronize_access_from_ldap'] = 0;
169178
Config::getInstance()->LoginLdap = $config;
170-
$this->setUserAccessMapperMock([], $userSynchronizer);
171-
$userSynchronizer->synchronizePiwikAccessFromLdap('piwikuser', array());
172-
$this->assertEmpty($logger->output);
179+
$this->setUserAccessMapperMock([]);
180+
$this->userSynchronizer->synchronizePiwikAccessFromLdap('piwikuser', array());
181+
$this->assertEmpty($this->deleteUserAccess);
182+
$this->assertEmpty($this->deleteSuperUserAccess);
183+
173184
$config['enable_synchronize_access_from_ldap'] = $oldValue;
174185
Config::getInstance()->LoginLdap = $config;
175186
}
176187

177188
public function test_synchronizePiwikAccessFromLdap_WillRemoveAccessWhenAccessEmptyAndSyncEnabled()
178189
{
179-
$logger = new FakeLogger();
180-
$userSynchronizer = new UserSynchronizer($logger);
181-
$this->setUserModelMock($this->getPiwikUserData(), $userSynchronizer);
182-
$this->setUserMapperMock($this->getPiwikUserData(), $userSynchronizer);
183-
$this->setUserManagerApiMock($throwsOnAdd = false, false, false, $userSynchronizer);
184-
190+
$this->setUserManagerApiMock($throwsOnAdd = false);
185191
// First iteration returns mapping
186192
$this->setUserAccessMapperMock(array(
187193
'superuser' => array(7,8,9),
188194
'view' => array(1,2,3),
189195
'admin' => array(4,5,6)
190-
), $userSynchronizer);
191-
$userSynchronizer->synchronizePiwikAccessFromLdap('piwikuser', array());
196+
));
197+
$this->userSynchronizer->synchronizePiwikAccessFromLdap('piwikuser', array());
192198
// Verify access mapping is present
193199
$this->assertEquals(array(
194200
array('piwikuser', 'view', array(1,2,3)),
@@ -198,14 +204,16 @@ public function test_synchronizePiwikAccessFromLdap_WillRemoveAccessWhenAccessEm
198204
array('piwikuser', true)
199205
), $this->superUserAccess);
200206

201-
// Second iteration returns no mapping for the user and since enable_synchronize_access_from_ldap=1, it should delete access
207+
// Second iteration returns no mapping for the user and since enable_synchronize_access_from_ldap=0, it should do nothing
202208
$config = Config::getInstance()->LoginLdap;
203209
$oldValue = $config['enable_synchronize_access_from_ldap'] ?? 0;
204210
$config['enable_synchronize_access_from_ldap'] = 1;
205211
Config::getInstance()->LoginLdap = $config;
206-
$this->setUserAccessMapperMock([], $userSynchronizer);
207-
$userSynchronizer->synchronizePiwikAccessFromLdap('piwikuser', array());
208-
$this->assertStringContainsString("UserSynchronizer::synchronizePiwikAccessFromLdap: User 'piwikuser' has no access in LDAP, but access synchronization is enabled. Deleting access if any present.", $logger->output);
212+
$this->setUserAccessMapperMock([]);
213+
$this->userSynchronizer->synchronizePiwikAccessFromLdap('piwikuser', array());
214+
$this->assertEquals(['piwikuser'], $this->deleteUserAccess);
215+
$this->assertEquals(['piwikuser'], $this->deleteSuperUserAccess);
216+
209217
$config['enable_synchronize_access_from_ldap'] = $oldValue;
210218
Config::getInstance()->LoginLdap = $config;
211219
}
@@ -220,7 +228,7 @@ public function test_synchronizePiwikAccessFromLdap_Succeeds_IfLdapUserHasNoAcce
220228
$this->assertEquals(array(), $this->superUserAccess);
221229
}
222230

223-
private function setUserManagerApiMock($throwsOnAddUser, $throwsOnUpdateUser = false, $throwsOnSetAccess = false, &$userSynchronizer = null)
231+
private function setUserManagerApiMock($throwsOnAddUser, $throwsOnUpdateUser = false, $throwsOnSetAccess = false)
224232
{
225233
$self = $this;
226234
$model = new Model();
@@ -238,20 +246,26 @@ private function setUserManagerApiMock($throwsOnAddUser, $throwsOnUpdateUser = f
238246
$mock->expects($this->any())->method('setUserAccess')->willThrowException(new Exception("dummy message"));
239247
} else {
240248
$mock->expects($this->any())->method('setUserAccess')->willReturnCallback(function ($login, $access, $sites) use ($self) {
249+
$key = array_search($login, $self->deleteUserAccess);
250+
if ($key !== false) {
251+
unset($self->deleteUserAccess[$key]);
252+
$self->deleteUserAccess = array_values($self->deleteUserAccess);
253+
}
241254
$self->userAccess[] = array($login, $access, $sites);
242255
});
243256
}
244257

245258
// for previous version
246259
$mock->expects($this->any())->method('setSuperUserAccess')->willReturnCallback(function ($login, $hasSuperUserAccess) use ($self) {
260+
$key = array_search($login, $self->deleteSuperUserAccess);
261+
if ($key !== false && $hasSuperUserAccess) {
262+
unset($self->deleteSuperUserAccess[$key]);
263+
$self->deleteSuperUserAccess = array_values($self->deleteSuperUserAccess);
264+
}
247265
$self->superUserAccess[] = array($login, $hasSuperUserAccess);
248266
});
249267

250-
if ($userSynchronizer) {
251-
$userSynchronizer->setUsersManagerApi($mock);
252-
} else {
253-
$this->userSynchronizer->setUsersManagerApi($mock);
254-
}
268+
$this->userSynchronizer->setUsersManagerApi($mock);
255269

256270
$mock = $this->getMockBuilder('Piwik\Plugins\UsersManager\UserUpdater')
257271
->onlyMethods(array('updateUserWithoutCurrentPassword', 'setSuperUserAccessWithoutCurrentPassword'))
@@ -263,14 +277,15 @@ private function setUserManagerApiMock($throwsOnAddUser, $throwsOnUpdateUser = f
263277
}
264278

265279
$mock->expects($this->any())->method('setSuperUserAccessWithoutCurrentPassword')->willReturnCallback(function ($login, $hasSuperUserAccess) use ($self) {
280+
$key = array_search($login, $self->deleteSuperUserAccess);
281+
if ($key !== false && $hasSuperUserAccess) {
282+
unset($self->deleteSuperUserAccess[$key]);
283+
$self->deleteSuperUserAccess = array_values($self->deleteSuperUserAccess);
284+
}
266285
$self->superUserAccess[] = array($login, $hasSuperUserAccess);
267286
});
268287

269-
if ($userSynchronizer) {
270-
$userSynchronizer->setUserUpdater($mock);
271-
} else {
272-
$this->userSynchronizer->setUserUpdater($mock);
273-
}
288+
$this->userSynchronizer->setUserUpdater($mock);
274289
}
275290

276291
private function getPiwikUserData()
@@ -282,7 +297,7 @@ private function getPiwikUserData()
282297
);
283298
}
284299

285-
private function setUserMapperMock($value, $throws = false, &$userSynchronizer = null)
300+
private function setUserMapperMock($value, $throws = false)
286301
{
287302
$mock = $this->getMockBuilder('Piwik\Plugins\LoginLdap\LdapInterop\UserMapper')
288303
->onlyMethods(array('createPiwikUserFromLdapUser', 'markUserAsLdapUser'))
@@ -292,37 +307,34 @@ private function setUserMapperMock($value, $throws = false, &$userSynchronizer =
292307
} else {
293308
$mock->expects($this->any())->method('createPiwikUserFromLdapUser')->will($this->returnValue($value));
294309
}
295-
if ($userSynchronizer) {
296-
$userSynchronizer->setUserMapper($mock);
297-
} else {
298-
$this->userSynchronizer->setUserMapper($mock);
299-
}
310+
$this->userSynchronizer->setUserMapper($mock);
300311
}
301312

302-
private function setUserAccessMapperMock($value, &$userSynchronizer = null)
313+
private function setUserAccessMapperMock($value)
303314
{
304315
$mock = $this->getMockBuilder('Piwik\Plugins\LoginLdap\LdapInterop\UserAccessMapper')
305316
->onlyMethods(array('getPiwikUserAccessForLdapUser'))
306317
->getMock();
307318
$mock->expects($this->any())->method('getPiwikUserAccessForLdapUser')->will($this->returnValue($value));
308-
if ($userSynchronizer) {
309-
$userSynchronizer->setUserAccessMapper($mock);
310-
} else {
311-
$this->userSynchronizer->setUserAccessMapper($mock);
312-
}
319+
$this->userSynchronizer->setUserAccessMapper($mock);
313320
}
314321

315-
private function setUserModelMock($returnValue, &$userSynchronizer = null)
322+
private function setUserModelMock($returnValue)
316323
{
317324
$mock = $this->getMockBuilder('Piwik\Plugins\UsersManager\Model')
318325
->onlyMethods(array('getUser', 'deleteUserAccess', 'setSuperUserAccess'))
319326
->getMock();
320327
$mock->expects($this->any())->method('getUser')->will($this->returnValue($returnValue));
328+
$self = $this;
329+
$mock->expects($this->any())->method('deleteUserAccess')->willReturnCallback(function ($login) use ($self) {
330+
$self->deleteUserAccess[] = $login;
331+
});
332+
$mock->expects($this->any())->method('setSuperUserAccess')->willReturnCallback(function ($login, $hasSuperUserAccess) use ($self) {
333+
if (!$hasSuperUserAccess && !in_array($login, $self->deleteSuperUserAccess)) {
334+
$self->deleteSuperUserAccess[] = $login;
335+
}
336+
});
321337

322-
if ($userSynchronizer) {
323-
$userSynchronizer->setUserModel($mock);
324-
} else {
325-
$this->userSynchronizer->setUserModel($mock);
326-
}
338+
$this->userSynchronizer->setUserModel($mock);
327339
}
328340
}

0 commit comments

Comments
 (0)