From 2124540d1d9f500423e5149b1490ed489788ff1d Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Tue, 20 Jan 2015 12:59:57 +0100 Subject: Dont remove a file from cache if the delete operation failed --- lib/private/files/view.php | 2 +- tests/lib/files/view.php | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/lib/private/files/view.php b/lib/private/files/view.php index a2717ce4321..5ed7af9222c 100644 --- a/lib/private/files/view.php +++ b/lib/private/files/view.php @@ -803,7 +803,7 @@ class View { $result = \OC_FileProxy::runPostProxies($operation, $this->getAbsolutePath($path), $result); - if (in_array('delete', $hooks)) { + if (in_array('delete', $hooks) and $result) { $this->updater->remove($path); } if (in_array('write', $hooks)) { diff --git a/tests/lib/files/view.php b/tests/lib/files/view.php index 5e42e5ffd0f..9ddc9c80475 100644 --- a/tests/lib/files/view.php +++ b/tests/lib/files/view.php @@ -849,4 +849,27 @@ class View extends \Test\TestCase { $this->assertEquals($time, $view->filemtime('/test/sub/storage/foo/bar.txt')); } + + public function testDeleteFailKeepCache() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Files\Storage\Temporary $storage + */ + $storage = $this->getMockBuilder('\OC\Files\Storage\Temporary') + ->setConstructorArgs(array(array())) + ->setMethods(array('unlink')) + ->getMock(); + $storage->expects($this->once()) + ->method('unlink') + ->will($this->returnValue(false)); + $scanner = $storage->getScanner(); + $cache = $storage->getCache(); + $storage->file_put_contents('foo.txt', 'asd'); + $scanner->scan(''); + \OC\Files\Filesystem::mount($storage, array(), '/test/'); + + $view = new \OC\Files\View('/test'); + + $this->assertFalse($view->unlink('foo.txt')); + $this->assertTrue($cache->inCache('foo.txt')); + } } -- cgit v1.2.3 From d4c4e2a3221bdba82c60e5428a5b151b4f21ad10 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 28 Jan 2015 15:35:49 +0100 Subject: Detect failed deletes in the trashbin --- apps/files_trashbin/lib/trashbin.php | 5 +++++ apps/files_trashbin/tests/storage.php | 39 ++++++++++++++++++++++++++++++++--- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/apps/files_trashbin/lib/trashbin.php b/apps/files_trashbin/lib/trashbin.php index 0576be66b4b..ead7f3e09ca 100644 --- a/apps/files_trashbin/lib/trashbin.php +++ b/apps/files_trashbin/lib/trashbin.php @@ -180,6 +180,11 @@ class Trashbin { } \OC_FileProxy::$enabled = $proxyStatus; + if ($view->file_exists('/files/' . $file_path)) { // failed to delete the original file, abort + $view->unlink($trashPath); + return false; + } + if ($sizeOfAddedFiles !== false) { $size = $sizeOfAddedFiles; $query = \OC_DB::prepare("INSERT INTO `*PREFIX*files_trash` (`id`,`timestamp`,`location`,`user`) VALUES (?,?,?,?)"); diff --git a/apps/files_trashbin/tests/storage.php b/apps/files_trashbin/tests/storage.php index d9a18e5a15c..24a04e68b2a 100644 --- a/apps/files_trashbin/tests/storage.php +++ b/apps/files_trashbin/tests/storage.php @@ -71,7 +71,7 @@ class Storage extends \Test\TestCase { public function testSingleStorageDelete() { $this->assertTrue($this->userView->file_exists('test.txt')); $this->userView->unlink('test.txt'); - list($storage, ) = $this->userView->resolvePath('test.txt'); + list($storage,) = $this->userView->resolvePath('test.txt'); $storage->getScanner()->scan(''); // make sure we check the storage $this->assertFalse($this->userView->getFileInfo('test.txt')); @@ -123,7 +123,7 @@ class Storage extends \Test\TestCase { $this->userView->unlink('test.txt'); // rescan trash storage - list($rootStorage, ) = $this->rootView->resolvePath($this->user . '/files_trashbin'); + list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin'); $rootStorage->getScanner()->scan(''); // check if versions are in trashbin @@ -158,7 +158,7 @@ class Storage extends \Test\TestCase { $this->userView->file_exists('substorage/test.txt'); // rescan trash storage - list($rootStorage, ) = $this->rootView->resolvePath($this->user . '/files_trashbin'); + list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin'); $rootStorage->getScanner()->scan(''); // versions were moved too @@ -173,4 +173,37 @@ class Storage extends \Test\TestCase { $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/versions/'); $this->assertEquals(0, count($results)); } + + /** + * Delete should fail is the source file cant be deleted + */ + public function testSingleStorageDeleteFail() { + /** + * @var \OC\Files\Storage\Temporary | \PHPUnit_Framework_MockObject_MockObject $storage + */ + $storage = $this->getMockBuilder('\OC\Files\Storage\Temporary') + ->setConstructorArgs([[]]) + ->setMethods(['rename', 'unlink']) + ->getMock(); + + $storage->expects($this->any()) + ->method('rename') + ->will($this->returnValue(false)); + $storage->expects($this->any()) + ->method('unlink') + ->will($this->returnValue(false)); + + $cache = $storage->getCache(); + + Filesystem::mount($storage, [], '/' . $this->user . '/files'); + $this->userView->file_put_contents('test.txt', 'foo'); + $this->assertTrue($storage->file_exists('test.txt')); + $this->assertFalse($this->userView->unlink('test.txt')); + $this->assertTrue($storage->file_exists('test.txt')); + $this->assertTrue($cache->inCache('test.txt')); + + // file should not be in the trashbin + $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/'); + $this->assertEquals(0, count($results)); + } } -- cgit v1.2.3 From 215388f4e06ab229ffaccc052f8d2b07faa45892 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 28 Jan 2015 16:30:42 +0100 Subject: Make sure we delete the file when doing a cross storage trashbin move --- apps/files_trashbin/lib/storage.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apps/files_trashbin/lib/storage.php b/apps/files_trashbin/lib/storage.php index 21b4e56d0bb..019fd490b52 100644 --- a/apps/files_trashbin/lib/storage.php +++ b/apps/files_trashbin/lib/storage.php @@ -85,6 +85,8 @@ class Storage extends Wrapper { $result = $this->storage->unlink($path); } unset($this->deletedFiles[$normalized]); + } else if ($this->storage->file_exists($path)) { + $result = $this->storage->unlink($path); } return $result; -- cgit v1.2.3 From ce0aa02aacee6b4e2641bdb1666f5650507ed28a Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 29 Jan 2015 15:52:40 +0100 Subject: Dont do a cache rename if we cant delete the source file --- apps/files_trashbin/lib/storage.php | 4 +++- lib/private/files/view.php | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/apps/files_trashbin/lib/storage.php b/apps/files_trashbin/lib/storage.php index 019fd490b52..175889ef95d 100644 --- a/apps/files_trashbin/lib/storage.php +++ b/apps/files_trashbin/lib/storage.php @@ -80,7 +80,9 @@ class Storage extends Wrapper { $result = \OCA\Files_Trashbin\Trashbin::move2trash($filesPath); // in cross-storage cases the file will be copied // but not deleted, so we delete it here - $this->storage->unlink($path); + if ($result) { + $this->storage->unlink($path); + } } else { $result = $this->storage->unlink($path); } diff --git a/lib/private/files/view.php b/lib/private/files/view.php index 5ed7af9222c..6c720a6f5c0 100644 --- a/lib/private/files/view.php +++ b/lib/private/files/view.php @@ -527,7 +527,7 @@ class View { fclose($target); if ($result !== false) { - $storage1->unlink($internalPath1); + $result &= $storage1->unlink($internalPath1); } } } @@ -537,7 +537,7 @@ class View { if ($this->shouldEmitHooks()) { $this->emit_file_hooks_post($exists, $path2); } - } elseif ($result !== false) { + } elseif ($result) { $this->updater->rename($path1, $path2); if ($this->shouldEmitHooks($path1) and $this->shouldEmitHooks($path2)) { \OC_Hook::emit( -- cgit v1.2.3 From 734dcc82dd48bc06a2152a45f9d62546e122b822 Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Fri, 30 Jan 2015 12:00:57 +0100 Subject: Fix subadmin listing of group Without this patch filtering for the "_everyone" (empty) group did not work for subadmins. Fixes itself. --- settings/controller/userscontroller.php | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/settings/controller/userscontroller.php b/settings/controller/userscontroller.php index be1b26f86ad..fd88692d777 100644 --- a/settings/controller/userscontroller.php +++ b/settings/controller/userscontroller.php @@ -161,7 +161,7 @@ class UsersController extends Controller { private function getUsersForUID(array $userIDs) { $users = []; foreach ($userIDs as $uid => $displayName) { - $users[] = $this->userManager->get($uid); + $users[$uid] = $this->userManager->get($uid); } return $users; } @@ -196,7 +196,7 @@ class UsersController extends Controller { } } - $users = array(); + $users = []; if ($this->isAdmin) { if($gid !== '') { @@ -210,16 +210,31 @@ class UsersController extends Controller { } } else { + /** @var array $subAdminOf List of groups the user is subadmin */ + $subAdminOf = \OC_SubAdmin::getSubAdminsGroups($this->userSession->getUser()->getUID()); + // Set the $gid parameter to an empty value if the subadmin has no rights to access a specific group - if($gid !== '' && !in_array($gid, \OC_SubAdmin::getSubAdminsGroups($this->userSession->getUser()->getUID()))) { + if($gid !== '' && !in_array($gid, $subAdminOf)) { $gid = ''; } - $batch = $this->getUsersForUID($this->groupManager->displayNamesInGroup($gid, $pattern, $limit, $offset)); + // Batch all groups the user is subadmin of when a group is specified + $batch = []; + if($gid === '') { + foreach($subAdminOf as $group) { + $groupUsers = $this->groupManager->displayNamesInGroup($group, $pattern, $limit, $offset); + foreach($groupUsers as $uid => $displayName) { + $batch[$uid] = $displayName; + } + } + } else { + $batch = $this->groupManager->displayNamesInGroup($gid, $pattern, $limit, $offset); + } + $batch = $this->getUsersForUID($batch); + foreach ($batch as $user) { // Only add the groups, this user is a subadmin of - $userGroups = array_intersect($this->groupManager->getUserGroupIds($user), - \OC_SubAdmin::getSubAdminsGroups($this->userSession->getUser()->getUID())); + $userGroups = array_intersect($this->groupManager->getUserGroupIds($user), $subAdminOf); $users[] = $this->formatUserForIndex($user, $userGroups); } } -- cgit v1.2.3 From 7e7dd92f6b7c255ea7a94cbcf0e2e762ef49f8ee Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Fri, 30 Jan 2015 14:16:16 +0100 Subject: Add unit tests --- settings/application.php | 9 +- settings/controller/userscontroller.php | 24 ++- .../controller/logsettingscontrollertest.php | 1 + tests/settings/controller/userscontrollertest.php | 196 ++++++++++++++++++++- 4 files changed, 211 insertions(+), 19 deletions(-) diff --git a/settings/application.php b/settings/application.php index d5516a1eefd..3b2c77ab849 100644 --- a/settings/application.php +++ b/settings/application.php @@ -1,7 +1,7 @@ query('Mail'), $c->query('DefaultMailAddress'), $c->query('URLGenerator'), - $c->query('OCP\\App\\IAppManager') + $c->query('OCP\\App\\IAppManager'), + $c->query('SubAdminOfGroups') ); }); $container->registerService('LogSettingsController', function(IContainer $c) { @@ -145,6 +146,10 @@ class Application extends App { $container->registerService('IsSubAdmin', function(IContainer $c) { return \OC_Subadmin::isSubAdmin(\OC_User::getUser()); }); + /** FIXME: Remove once OC_SubAdmin is non-static and mockable */ + $container->registerService('SubAdminOfGroups', function(IContainer $c) { + return \OC_SubAdmin::getSubAdminsGroups(\OC_User::getUser()); + }); $container->registerService('Mail', function(IContainer $c) { return new \OC_Mail; }); diff --git a/settings/controller/userscontroller.php b/settings/controller/userscontroller.php index fd88692d777..39d94fd2e18 100644 --- a/settings/controller/userscontroller.php +++ b/settings/controller/userscontroller.php @@ -1,7 +1,7 @@ userManager = $userManager; $this->groupManager = $groupManager; @@ -98,6 +103,7 @@ class UsersController extends Controller { $this->mail = $mail; $this->fromMailAddress = $fromMailAddress; $this->urlGenerator = $urlGenerator; + $this->subAdminOfGroups = $subAdminOfGroups; // check for encryption state - TODO see formatUserForIndex $this->isEncryptionAppEnabled = $appManager->isEnabledForUser('files_encryption'); @@ -210,18 +216,15 @@ class UsersController extends Controller { } } else { - /** @var array $subAdminOf List of groups the user is subadmin */ - $subAdminOf = \OC_SubAdmin::getSubAdminsGroups($this->userSession->getUser()->getUID()); - // Set the $gid parameter to an empty value if the subadmin has no rights to access a specific group - if($gid !== '' && !in_array($gid, $subAdminOf)) { + if($gid !== '' && !in_array($gid, $this->subAdminOfGroups)) { $gid = ''; } // Batch all groups the user is subadmin of when a group is specified $batch = []; if($gid === '') { - foreach($subAdminOf as $group) { + foreach($this->subAdminOfGroups as $group) { $groupUsers = $this->groupManager->displayNamesInGroup($group, $pattern, $limit, $offset); foreach($groupUsers as $uid => $displayName) { $batch[$uid] = $displayName; @@ -234,7 +237,10 @@ class UsersController extends Controller { foreach ($batch as $user) { // Only add the groups, this user is a subadmin of - $userGroups = array_intersect($this->groupManager->getUserGroupIds($user), $subAdminOf); + $userGroups = array_values(array_intersect( + $this->groupManager->getUserGroupIds($user), + $this->subAdminOfGroups + )); $users[] = $this->formatUserForIndex($user, $userGroups); } } @@ -274,7 +280,7 @@ class UsersController extends Controller { } } if (empty($groups)) { - $groups = \OC_SubAdmin::getSubAdminsGroups($this->userSession->getUser()->getUID()); + $groups = $this->subAdminOfGroups; } } diff --git a/tests/settings/controller/logsettingscontrollertest.php b/tests/settings/controller/logsettingscontrollertest.php index e80acfa75b5..84581bf5782 100644 --- a/tests/settings/controller/logsettingscontrollertest.php +++ b/tests/settings/controller/logsettingscontrollertest.php @@ -10,6 +10,7 @@ namespace Test\Settings\Controller; use \OC\Settings\Application; +use OC\Settings\Controller\LogSettingsController; /** * @package OC\Settings\Controller diff --git a/tests/settings/controller/userscontrollertest.php b/tests/settings/controller/userscontrollertest.php index 7dc2d066a5c..eb85573d3fe 100644 --- a/tests/settings/controller/userscontrollertest.php +++ b/tests/settings/controller/userscontrollertest.php @@ -1,7 +1,7 @@ disableOriginalConstructor()->getMock(); $this->container['Config'] = $this->getMockBuilder('\OCP\IConfig') ->disableOriginalConstructor()->getMock(); - $this->container['IsAdmin'] = true; $this->container['L10N'] ->expects($this->any()) ->method('t') @@ -55,11 +54,9 @@ class UsersControllerTest extends \Test\TestCase { ->disableOriginalConstructor()->getMock(); } - /** - * TODO: Since the function uses the static OC_Subadmin class it can't be mocked - * to test for subadmins. Thus the test always assumes you have admin permissions... - */ - public function testIndex() { + public function testIndexAdmin() { + $this->container['IsAdmin'] = true; + $foo = $this->getMockBuilder('\OC\User\User') ->disableOriginalConstructor()->getMock(); $foo @@ -198,11 +195,167 @@ class UsersControllerTest extends \Test\TestCase { $this->assertEquals($expectedResponse, $response); } + public function testIndexSubAdmin() { + $this->container['IsAdmin'] = false; + $this->container['SubAdminOfGroups'] = ['SubGroup1', 'SubGroup2']; + + $foo = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $foo + ->expects($this->exactly(4)) + ->method('getUID') + ->will($this->returnValue('foo')); + $foo + ->expects($this->once()) + ->method('getDisplayName') + ->will($this->returnValue('M. Foo')); + $foo + ->method('getLastLogin') + ->will($this->returnValue(500)); + $foo + ->method('getHome') + ->will($this->returnValue('/home/foo')); + $foo + ->expects($this->once()) + ->method('getBackendClassName') + ->will($this->returnValue('OC_User_Database')); + $admin = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $admin + ->expects($this->exactly(4)) + ->method('getUID') + ->will($this->returnValue('admin')); + $admin + ->expects($this->once()) + ->method('getDisplayName') + ->will($this->returnValue('S. Admin')); + $admin + ->expects($this->once()) + ->method('getLastLogin') + ->will($this->returnValue(12)); + $admin + ->expects($this->once()) + ->method('getHome') + ->will($this->returnValue('/home/admin')); + $admin + ->expects($this->once()) + ->method('getBackendClassName') + ->will($this->returnValue('OC_User_Dummy')); + $bar = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $bar + ->expects($this->exactly(4)) + ->method('getUID') + ->will($this->returnValue('bar')); + $bar + ->expects($this->once()) + ->method('getDisplayName') + ->will($this->returnValue('B. Ar')); + $bar + ->method('getLastLogin') + ->will($this->returnValue(3999)); + $bar + ->method('getHome') + ->will($this->returnValue('/home/bar')); + $bar + ->expects($this->once()) + ->method('getBackendClassName') + ->will($this->returnValue('OC_User_Dummy')); + + $this->container['GroupManager'] + ->expects($this->at(0)) + ->method('displayNamesInGroup') + ->with('SubGroup1', 'pattern') + ->will($this->returnValue(['foo' => 'M. Foo', 'admin' => 'S. Admin'])); + $this->container['GroupManager'] + ->expects($this->at(1)) + ->method('displayNamesInGroup') + ->with('SubGroup2', 'pattern') + ->will($this->returnValue(['bar' => 'B. Ar'])); + $this->container['GroupManager'] + ->expects($this->exactly(3)) + ->method('getUserGroupIds') + ->will($this->onConsecutiveCalls( + ['SubGroup2', 'SubGroup1'], + ['SubGroup2', 'Foo'], + ['admin', 'SubGroup1', 'testGroup'] + )); + $this->container['UserManager'] + ->expects($this->at(0)) + ->method('get') + ->with('foo') + ->will($this->returnValue($foo)); + $this->container['UserManager'] + ->expects($this->at(1)) + ->method('get') + ->with('admin') + ->will($this->returnValue($admin)); + $this->container['UserManager'] + ->expects($this->at(2)) + ->method('get') + ->with('bar') + ->will($this->returnValue($bar)); + $this->container['Config'] + ->expects($this->exactly(6)) + ->method('getUserValue') + ->will($this->onConsecutiveCalls( + 1024, 'foo@bar.com', + 404, 'admin@bar.com', + 2323, 'bar@dummy.com' + )); + + $expectedResponse = new DataResponse( + [ + 0 => [ + 'name' => 'foo', + 'displayname' => 'M. Foo', + 'groups' => ['SubGroup2', 'SubGroup1'], + 'subadmin' => [], + 'quota' => 1024, + 'storageLocation' => '/home/foo', + 'lastLogin' => 500, + 'backend' => 'OC_User_Database', + 'email' => 'foo@bar.com', + 'isRestoreDisabled' => false, + ], + 1 => [ + 'name' => 'admin', + 'displayname' => 'S. Admin', + 'groups' => ['SubGroup2'], + 'subadmin' => [], + 'quota' => 404, + 'storageLocation' => '/home/admin', + 'lastLogin' => 12, + 'backend' => 'OC_User_Dummy', + 'email' => 'admin@bar.com', + 'isRestoreDisabled' => false, + ], + 2 => [ + 'name' => 'bar', + 'displayname' => 'B. Ar', + 'groups' => ['SubGroup1'], + 'subadmin' => [], + 'quota' => 2323, + 'storageLocation' => '/home/bar', + 'lastLogin' => 3999, + 'backend' => 'OC_User_Dummy', + 'email' => 'bar@dummy.com', + 'isRestoreDisabled' => false, + ], + ] + ); + + $response = $this->container['UsersController']->index(0, 10, '', 'pattern'); + $this->assertEquals($expectedResponse, $response); + } + /** * TODO: Since the function uses the static OC_Subadmin class it can't be mocked * to test for subadmins. Thus the test always assumes you have admin permissions... */ public function testIndexWithSearch() { + $this->container['IsAdmin'] = true; + $foo = $this->getMockBuilder('\OC\User\User') ->disableOriginalConstructor()->getMock(); $foo @@ -326,8 +479,9 @@ class UsersControllerTest extends \Test\TestCase { $this->assertEquals($expectedResponse, $response); } - public function testIndexWithBackend() { + $this->container['IsAdmin'] = true; + $user = $this->getMockBuilder('\OC\User\User') ->disableOriginalConstructor()->getMock(); $user @@ -386,6 +540,8 @@ class UsersControllerTest extends \Test\TestCase { } public function testIndexWithBackendNoUser() { + $this->container['IsAdmin'] = true; + $this->container['UserManager'] ->expects($this->once()) ->method('getBackends') @@ -406,6 +562,8 @@ class UsersControllerTest extends \Test\TestCase { * to test for subadmins. Thus the test always assumes you have admin permissions... */ public function testCreateSuccessfulWithoutGroup() { + $this->container['IsAdmin'] = true; + $user = $this->getMockBuilder('\OC\User\User') ->disableOriginalConstructor()->getMock(); $user @@ -449,6 +607,8 @@ class UsersControllerTest extends \Test\TestCase { * to test for subadmins. Thus the test always assumes you have admin permissions... */ public function testCreateSuccessfulWithGroup() { + $this->container['IsAdmin'] = true; + $user = $this->getMockBuilder('\OC\User\User') ->disableOriginalConstructor()->getMock(); $user @@ -520,6 +680,8 @@ class UsersControllerTest extends \Test\TestCase { * to test for subadmins. Thus the test always assumes you have admin permissions... */ public function testCreateUnsuccessful() { + $this->container['IsAdmin'] = true; + $this->container['UserManager'] ->method('createUser') ->will($this->throwException(new \Exception())); @@ -539,6 +701,8 @@ class UsersControllerTest extends \Test\TestCase { * to test for subadmins. Thus the test always assumes you have admin permissions... */ public function testDestroySelf() { + $this->container['IsAdmin'] = true; + $user = $this->getMockBuilder('\OC\User\User') ->disableOriginalConstructor()->getMock(); $user @@ -567,6 +731,8 @@ class UsersControllerTest extends \Test\TestCase { * to test for subadmins. Thus the test always assumes you have admin permissions... */ public function testDestroy() { + $this->container['IsAdmin'] = true; + $user = $this->getMockBuilder('\OC\User\User') ->disableOriginalConstructor()->getMock(); $user @@ -604,6 +770,8 @@ class UsersControllerTest extends \Test\TestCase { * to test for subadmins. Thus the test always assumes you have admin permissions... */ public function testDestroyUnsuccessful() { + $this->container['IsAdmin'] = true; + $user = $this->getMockBuilder('\OC\User\User') ->disableOriginalConstructor()->getMock(); $user @@ -641,6 +809,8 @@ class UsersControllerTest extends \Test\TestCase { * test if an invalid mail result in a failure response */ public function testCreateUnsuccessfulWithInvalidEMail() { + $this->container['IsAdmin'] = true; + /** * FIXME: Disabled due to missing DI on mail class. * TODO: Re-enable when https://github.com/owncloud/core/pull/12085 is merged. @@ -666,6 +836,8 @@ class UsersControllerTest extends \Test\TestCase { * test if a valid mail result in a successful mail send */ public function testCreateSuccessfulWithValidEMail() { + $this->container['IsAdmin'] = true; + /** * FIXME: Disabled due to missing DI on mail class. * TODO: Re-enable when https://github.com/owncloud/core/pull/12085 is merged. @@ -737,6 +909,8 @@ class UsersControllerTest extends \Test\TestCase { } public function testRestorePossibleWithoutEncryption() { + $this->container['IsAdmin'] = true; + list($user, $expectedResult) = $this->mockUser(); $result = \Test_Helper::invokePrivate($this->container['UsersController'], 'formatUserForIndex', [$user]); @@ -744,6 +918,8 @@ class UsersControllerTest extends \Test\TestCase { } public function testRestorePossibleWithAdminAndUserRestore() { + $this->container['IsAdmin'] = true; + list($user, $expectedResult) = $this->mockUser(); $this->container['OCP\\App\\IAppManager'] @@ -779,6 +955,8 @@ class UsersControllerTest extends \Test\TestCase { } public function testRestoreNotPossibleWithoutAdminRestore() { + $this->container['IsAdmin'] = true; + list($user, $expectedResult) = $this->mockUser(); $this->container['OCP\\App\\IAppManager'] @@ -795,6 +973,8 @@ class UsersControllerTest extends \Test\TestCase { } public function testRestoreNotPossibleWithoutUserRestore() { + $this->container['IsAdmin'] = true; + list($user, $expectedResult) = $this->mockUser(); $this->container['OCP\\App\\IAppManager'] -- cgit v1.2.3 From 0f7634eadc2eee3a32116e2b2686ca462dfaaafa Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Fri, 30 Jan 2015 17:24:42 +0100 Subject: Switch to a factory and add unit tests --- settings/application.php | 7 +- settings/controller/userscontroller.php | 43 ++- settings/factory/subadminfactory.php | 45 +++ tests/settings/controller/userscontrollertest.php | 406 ++++++++++++++++++++-- 4 files changed, 443 insertions(+), 58 deletions(-) create mode 100644 settings/factory/subadminfactory.php diff --git a/settings/application.php b/settings/application.php index 3b2c77ab849..6fe23447a72 100644 --- a/settings/application.php +++ b/settings/application.php @@ -16,6 +16,7 @@ use OC\Settings\Controller\LogSettingsController; use OC\Settings\Controller\MailSettingsController; use OC\Settings\Controller\SecuritySettingsController; use OC\Settings\Controller\UsersController; +use OC\Settings\Factory\SubAdminFactory; use OC\Settings\Middleware\SubadminMiddleware; use \OCP\AppFramework\App; use OCP\IContainer; @@ -92,7 +93,7 @@ class Application extends App { $c->query('DefaultMailAddress'), $c->query('URLGenerator'), $c->query('OCP\\App\\IAppManager'), - $c->query('SubAdminOfGroups') + $c->query('SubAdminFactory') ); }); $container->registerService('LogSettingsController', function(IContainer $c) { @@ -147,8 +148,8 @@ class Application extends App { return \OC_Subadmin::isSubAdmin(\OC_User::getUser()); }); /** FIXME: Remove once OC_SubAdmin is non-static and mockable */ - $container->registerService('SubAdminOfGroups', function(IContainer $c) { - return \OC_SubAdmin::getSubAdminsGroups(\OC_User::getUser()); + $container->registerService('SubAdminFactory', function(IContainer $c) { + return new SubAdminFactory(); }); $container->registerService('Mail', function(IContainer $c) { return new \OC_Mail; diff --git a/settings/controller/userscontroller.php b/settings/controller/userscontroller.php index 39d94fd2e18..b1caaa17991 100644 --- a/settings/controller/userscontroller.php +++ b/settings/controller/userscontroller.php @@ -11,6 +11,7 @@ namespace OC\Settings\Controller; use OC\AppFramework\Http; +use OC\Settings\Factory\SubAdminFactory; use OC\User\User; use OCP\App\IAppManager; use OCP\AppFramework\Controller; @@ -56,8 +57,8 @@ class UsersController extends Controller { private $isEncryptionAppEnabled; /** @var bool contains the state of the admin recovery setting */ private $isRestoreEnabled = false; - /** @var string[] Array of groups the user is sub-admin of */ - private $subAdminOfGroups = []; + /** @var SubAdminFactory */ + private $subAdminFactory; /** * @param string $appName @@ -74,7 +75,7 @@ class UsersController extends Controller { * @param string $fromMailAddress * @param IURLGenerator $urlGenerator * @param IAppManager $appManager - * @param array $subAdminOfGroups + * @param SubAdminFactory $subAdminFactory */ public function __construct($appName, IRequest $request, @@ -90,7 +91,7 @@ class UsersController extends Controller { $fromMailAddress, IURLGenerator $urlGenerator, IAppManager $appManager, - array $subAdminOfGroups) { + SubAdminFactory $subAdminFactory) { parent::__construct($appName, $request); $this->userManager = $userManager; $this->groupManager = $groupManager; @@ -103,7 +104,7 @@ class UsersController extends Controller { $this->mail = $mail; $this->fromMailAddress = $fromMailAddress; $this->urlGenerator = $urlGenerator; - $this->subAdminOfGroups = $subAdminOfGroups; + $this->subAdminFactory = $subAdminFactory; // check for encryption state - TODO see formatUserForIndex $this->isEncryptionAppEnabled = $appManager->isEnabledForUser('files_encryption'); @@ -216,15 +217,18 @@ class UsersController extends Controller { } } else { + $subAdminOfGroups = $this->subAdminFactory->getSubAdminsOfGroups( + $this->userSession->getUser()->getUID() + ); // Set the $gid parameter to an empty value if the subadmin has no rights to access a specific group - if($gid !== '' && !in_array($gid, $this->subAdminOfGroups)) { + if($gid !== '' && !in_array($gid, $subAdminOfGroups)) { $gid = ''; } // Batch all groups the user is subadmin of when a group is specified $batch = []; if($gid === '') { - foreach($this->subAdminOfGroups as $group) { + foreach($subAdminOfGroups as $group) { $groupUsers = $this->groupManager->displayNamesInGroup($group, $pattern, $limit, $offset); foreach($groupUsers as $uid => $displayName) { $batch[$uid] = $displayName; @@ -239,7 +243,7 @@ class UsersController extends Controller { // Only add the groups, this user is a subadmin of $userGroups = array_values(array_intersect( $this->groupManager->getUserGroupIds($user), - $this->subAdminOfGroups + $subAdminOfGroups )); $users[] = $this->formatUserForIndex($user, $userGroups); } @@ -256,8 +260,6 @@ class UsersController extends Controller { * @param array $groups * @param string $email * @return DataResponse - * - * TODO: Tidy up and write unit tests - code is mainly static method calls */ public function create($username, $password, array $groups=array(), $email='') { @@ -270,17 +272,17 @@ class UsersController extends Controller { ); } - // TODO FIXME get rid of the static calls to OC_Subadmin if (!$this->isAdmin) { + $uid = $this->userSession->getUser()->getUID(); if (!empty($groups)) { foreach ($groups as $key => $group) { - if (!\OC_SubAdmin::isGroupAccessible($this->userSession->getUser()->getUID(), $group)) { + if (!$this->subAdminFactory->isGroupAccessible($uid, $group)) { unset($groups[$key]); } } } if (empty($groups)) { - $groups = $this->subAdminOfGroups; + $groups = $this->subAdminFactory->getSubAdminsOfGroups($uid); } } @@ -297,7 +299,7 @@ class UsersController extends Controller { if($user instanceof User) { if($groups !== null) { - foreach( $groups as $groupName ) { + foreach($groups as $groupName) { $group = $this->groupManager->get($groupName); if(empty($group)) { @@ -363,11 +365,10 @@ class UsersController extends Controller { * * @param string $id * @return DataResponse - * - * TODO: Tidy up and write unit tests - code is mainly static method calls */ public function destroy($id) { - if($this->userSession->getUser()->getUID() === $id) { + $UserId = $this->userSession->getUser()->getUID(); + if($UserId === $id) { return new DataResponse( array( 'status' => 'error', @@ -379,8 +380,7 @@ class UsersController extends Controller { ); } - // FIXME: Remove this static function call at some point… - if(!$this->isAdmin && !\OC_SubAdmin::isUserAccessible($this->userSession->getUser()->getUID(), $id)) { + if(!$this->isAdmin && !$this->subAdminFactory->isUserAccessible($UserId, $id)) { return new DataResponse( array( 'status' => 'error', @@ -427,14 +427,13 @@ class UsersController extends Controller { * @param string $id * @param string $mailAddress * @return DataResponse - * - * TODO: Tidy up and write unit tests - code is mainly static method calls */ public function setMailAddress($id, $mailAddress) { + $UserId = $this->userSession->getUser()->getUID(); // FIXME: Remove this static function call at some point… if($this->userSession->getUser()->getUID() !== $id && !$this->isAdmin - && !\OC_SubAdmin::isUserAccessible($this->userSession->getUser()->getUID(), $id)) { + && !$this->subAdminFactory->isUserAccessible($UserId, $id)) { return new DataResponse( array( 'status' => 'error', diff --git a/settings/factory/subadminfactory.php b/settings/factory/subadminfactory.php new file mode 100644 index 00000000000..12a45527ae1 --- /dev/null +++ b/settings/factory/subadminfactory.php @@ -0,0 +1,45 @@ +disableOriginalConstructor()->getMock(); $this->container['L10N'] = $this->getMockBuilder('\OCP\IL10N') ->disableOriginalConstructor()->getMock(); + $this->container['SubAdminFactory'] = $this->getMockBuilder('\OC\Settings\Factory\SubAdminFactory') + ->disableOriginalConstructor()->getMock(); $this->container['Config'] = $this->getMockBuilder('\OCP\IConfig') ->disableOriginalConstructor()->getMock(); $this->container['L10N'] @@ -197,7 +199,22 @@ class UsersControllerTest extends \Test\TestCase { public function testIndexSubAdmin() { $this->container['IsAdmin'] = false; - $this->container['SubAdminOfGroups'] = ['SubGroup1', 'SubGroup2']; + $this->container['SubAdminFactory'] + ->expects($this->once()) + ->method('getSubAdminsOfGroups') + ->with('username') + ->will($this->returnValue(['SubGroup1', 'SubGroup2'])); + + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user + ->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('username')); + $this->container['UserSession'] + ->expects($this->once()) + ->method('getUser') + ->will($this->returnValue($user)); $foo = $this->getMockBuilder('\OC\User\User') ->disableOriginalConstructor()->getMock(); @@ -557,11 +574,7 @@ class UsersControllerTest extends \Test\TestCase { $this->assertEquals($expectedResponse, $response); } - /** - * TODO: Since the function uses the static OC_Subadmin class it can't be mocked - * to test for subadmins. Thus the test always assumes you have admin permissions... - */ - public function testCreateSuccessfulWithoutGroup() { + public function testCreateSuccessfulWithoutGroupAdmin() { $this->container['IsAdmin'] = true; $user = $this->getMockBuilder('\OC\User\User') @@ -602,11 +615,86 @@ class UsersControllerTest extends \Test\TestCase { $this->assertEquals($expectedResponse, $response); } - /** - * TODO: Since the function uses the static OC_Subadmin class it can't be mocked - * to test for subadmins. Thus the test always assumes you have admin permissions... - */ - public function testCreateSuccessfulWithGroup() { + public function testCreateSuccessfulWithoutGroupSubAdmin() { + $this->container['IsAdmin'] = false; + $this->container['SubAdminFactory'] + ->expects($this->once()) + ->method('getSubAdminsOfGroups') + ->with('username') + ->will($this->returnValue(['SubGroup1', 'SubGroup2'])); + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user + ->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('username')); + $this->container['UserSession'] + ->expects($this->once()) + ->method('getUser') + ->will($this->returnValue($user)); + + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user + ->method('getHome') + ->will($this->returnValue('/home/user')); + $user + ->method('getHome') + ->will($this->returnValue('/home/user')); + $user + ->method('getUID') + ->will($this->returnValue('foo')); + $user + ->expects($this->once()) + ->method('getBackendClassName') + ->will($this->returnValue('bar')); + $subGroup1 = $this->getMockBuilder('\OCP\IGroup') + ->disableOriginalConstructor()->getMock(); + $subGroup1 + ->expects($this->once()) + ->method('addUser') + ->with($user); + $subGroup2 = $this->getMockBuilder('\OCP\IGroup') + ->disableOriginalConstructor()->getMock(); + $subGroup2 + ->expects($this->once()) + ->method('addUser') + ->with($user); + + $this->container['UserManager'] + ->expects($this->once()) + ->method('createUser') + ->will($this->onConsecutiveCalls($user)); + $this->container['GroupManager'] + ->expects($this->exactly(2)) + ->method('get') + ->will($this->onConsecutiveCalls($subGroup1, $subGroup2)); + $this->container['GroupManager'] + ->expects($this->once()) + ->method('getUserGroupIds') + ->with($user) + ->will($this->onConsecutiveCalls(['SubGroup1', 'SubGroup2'])); + + $expectedResponse = new DataResponse( + array( + 'name' => 'foo', + 'groups' => ['SubGroup1', 'SubGroup2'], + 'storageLocation' => '/home/user', + 'backend' => 'bar', + 'lastLogin' => null, + 'displayname' => null, + 'quota' => null, + 'subadmin' => [], + 'email' => null, + 'isRestoreDisabled' => false, + ), + Http::STATUS_CREATED + ); + $response = $this->container['UsersController']->create('foo', 'password'); + $this->assertEquals($expectedResponse, $response); + } + + public function testCreateSuccessfulWithGroupAdmin() { $this->container['IsAdmin'] = true; $user = $this->getMockBuilder('\OC\User\User') @@ -675,11 +763,86 @@ class UsersControllerTest extends \Test\TestCase { $this->assertEquals($expectedResponse, $response); } - /** - * TODO: Since the function uses the static OC_Subadmin class it can't be mocked - * to test for subadmins. Thus the test always assumes you have admin permissions... - */ - public function testCreateUnsuccessful() { + public function testCreateSuccessfulWithGroupSubAdmin() { + $this->container['IsAdmin'] = false; + $this->container['SubAdminFactory'] + ->expects($this->once()) + ->method('getSubAdminsOfGroups') + ->with('username') + ->will($this->returnValue(['SubGroup1', 'SubGroup2'])); + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user + ->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('username')); + $this->container['UserSession'] + ->expects($this->once()) + ->method('getUser') + ->will($this->returnValue($user)); + + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user + ->method('getHome') + ->will($this->returnValue('/home/user')); + $user + ->method('getHome') + ->will($this->returnValue('/home/user')); + $user + ->method('getUID') + ->will($this->returnValue('foo')); + $user + ->expects($this->once()) + ->method('getBackendClassName') + ->will($this->returnValue('bar')); + $subGroup1 = $this->getMockBuilder('\OCP\IGroup') + ->disableOriginalConstructor()->getMock(); + $subGroup1 + ->expects($this->once()) + ->method('addUser') + ->with($user); + $subGroup2 = $this->getMockBuilder('\OCP\IGroup') + ->disableOriginalConstructor()->getMock(); + $subGroup2 + ->expects($this->once()) + ->method('addUser') + ->with($user); + + $this->container['UserManager'] + ->expects($this->once()) + ->method('createUser') + ->will($this->onConsecutiveCalls($user)); + $this->container['GroupManager'] + ->expects($this->exactly(2)) + ->method('get') + ->will($this->onConsecutiveCalls($subGroup1, $subGroup2)); + $this->container['GroupManager'] + ->expects($this->once()) + ->method('getUserGroupIds') + ->with($user) + ->will($this->onConsecutiveCalls(['SubGroup1'])); + + $expectedResponse = new DataResponse( + array( + 'name' => 'foo', + 'groups' => ['SubGroup1'], + 'storageLocation' => '/home/user', + 'backend' => 'bar', + 'lastLogin' => null, + 'displayname' => null, + 'quota' => null, + 'subadmin' => [], + 'email' => null, + 'isRestoreDisabled' => false, + ), + Http::STATUS_CREATED + ); + $response = $this->container['UsersController']->create('foo', 'password', ['SubGroup1', 'ExistingGroup']); + $this->assertEquals($expectedResponse, $response); + } + + public function testCreateUnsuccessfulAdmin() { $this->container['IsAdmin'] = true; $this->container['UserManager'] @@ -696,11 +859,39 @@ class UsersControllerTest extends \Test\TestCase { $this->assertEquals($expectedResponse, $response); } - /** - * TODO: Since the function uses the static OC_Subadmin class it can't be mocked - * to test for subadmins. Thus the test always assumes you have admin permissions... - */ - public function testDestroySelf() { + public function testCreateUnsuccessfulSubAdmin() { + $this->container['IsAdmin'] = false; + $this->container['SubAdminFactory'] + ->expects($this->once()) + ->method('getSubAdminsOfGroups') + ->with('username') + ->will($this->returnValue(['SubGroup1', 'SubGroup2'])); + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user + ->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('username')); + $this->container['UserSession'] + ->expects($this->once()) + ->method('getUser') + ->will($this->returnValue($user)); + + $this->container['UserManager'] + ->method('createUser') + ->will($this->throwException(new \Exception())); + + $expectedResponse = new DataResponse( + [ + 'message' => 'Unable to create user.' + ], + Http::STATUS_FORBIDDEN + ); + $response = $this->container['UsersController']->create('foo', 'password', array()); + $this->assertEquals($expectedResponse, $response); + } + + public function testDestroySelfAdmin() { $this->container['IsAdmin'] = true; $user = $this->getMockBuilder('\OC\User\User') @@ -726,11 +917,33 @@ class UsersControllerTest extends \Test\TestCase { $this->assertEquals($expectedResponse, $response); } - /** - * TODO: Since the function uses the static OC_Subadmin class it can't be mocked - * to test for subadmins. Thus the test always assumes you have admin permissions... - */ - public function testDestroy() { + public function testDestroySelfSubadmin() { + $this->container['IsAdmin'] = false; + + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user + ->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('myself')); + $this->container['UserSession'] + ->method('getUser') + ->will($this->returnValue($user)); + + $expectedResponse = new DataResponse( + array( + 'status' => 'error', + 'data' => array( + 'message' => 'Unable to delete user.' + ) + ), + Http::STATUS_FORBIDDEN + ); + $response = $this->container['UsersController']->destroy('myself'); + $this->assertEquals($expectedResponse, $response); + } + + public function testDestroyAdmin() { $this->container['IsAdmin'] = true; $user = $this->getMockBuilder('\OC\User\User') @@ -765,11 +978,54 @@ class UsersControllerTest extends \Test\TestCase { $response = $this->container['UsersController']->destroy('UserToDelete'); $this->assertEquals($expectedResponse, $response); } - /** - * TODO: Since the function uses the static OC_Subadmin class it can't be mocked - * to test for subadmins. Thus the test always assumes you have admin permissions... - */ - public function testDestroyUnsuccessful() { + + public function testDestroySubAdmin() { + $this->container['IsAdmin'] = false; + $this->container['SubAdminFactory'] + ->expects($this->once()) + ->method('isUserAccessible') + ->with('myself', 'UserToDelete') + ->will($this->returnValue(true)); + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user + ->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('myself')); + $this->container['UserSession'] + ->method('getUser') + ->will($this->returnValue($user)); + + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $toDeleteUser = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $toDeleteUser + ->expects($this->once()) + ->method('delete') + ->will($this->returnValue(true)); + $this->container['UserSession'] + ->method('getUser') + ->will($this->returnValue($user)); + $this->container['UserManager'] + ->method('get') + ->with('UserToDelete') + ->will($this->returnValue($toDeleteUser)); + + $expectedResponse = new DataResponse( + [ + 'status' => 'success', + 'data' => [ + 'username' => 'UserToDelete' + ] + ], + Http::STATUS_NO_CONTENT + ); + $response = $this->container['UsersController']->destroy('UserToDelete'); + $this->assertEquals($expectedResponse, $response); + } + + public function testDestroyUnsuccessfulAdmin() { $this->container['IsAdmin'] = true; $user = $this->getMockBuilder('\OC\User\User') @@ -805,10 +1061,94 @@ class UsersControllerTest extends \Test\TestCase { $this->assertEquals($expectedResponse, $response); } + public function testDestroyUnsuccessfulSubAdmin() { + $this->container['IsAdmin'] = false; + $this->container['SubAdminFactory'] + ->expects($this->once()) + ->method('isUserAccessible') + ->with('myself', 'UserToDelete') + ->will($this->returnValue(true)); + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user + ->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('myself')); + $this->container['UserSession'] + ->method('getUser') + ->will($this->returnValue($user)); + + $toDeleteUser = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $toDeleteUser + ->expects($this->once()) + ->method('delete') + ->will($this->returnValue(false)); + $this->container['UserSession'] + ->method('getUser') + ->will($this->returnValue($user)); + $this->container['UserManager'] + ->method('get') + ->with('UserToDelete') + ->will($this->returnValue($toDeleteUser)); + + $expectedResponse = new DataResponse( + [ + 'status' => 'error', + 'data' => [ + 'message' => 'Unable to delete user.' + ] + ], + Http::STATUS_FORBIDDEN + ); + $response = $this->container['UsersController']->destroy('UserToDelete'); + $this->assertEquals($expectedResponse, $response); + } + + public function testDestroyNotAccessibleToSubAdmin() { + $this->container['IsAdmin'] = false; + $this->container['SubAdminFactory'] + ->expects($this->once()) + ->method('isUserAccessible') + ->with('myself', 'UserToDelete') + ->will($this->returnValue(false)); + $user = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $user + ->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('myself')); + $this->container['UserSession'] + ->method('getUser') + ->will($this->returnValue($user)); + + $toDeleteUser = $this->getMockBuilder('\OC\User\User') + ->disableOriginalConstructor()->getMock(); + $this->container['UserSession'] + ->method('getUser') + ->will($this->returnValue($user)); + $this->container['UserManager'] + ->method('get') + ->with('UserToDelete') + ->will($this->returnValue($toDeleteUser)); + + $expectedResponse = new DataResponse( + [ + 'status' => 'error', + 'data' => [ + 'message' => 'Authentication error' + ] + ], + Http::STATUS_FORBIDDEN + ); + $response = $this->container['UsersController']->destroy('UserToDelete'); + $this->assertEquals($expectedResponse, $response); + } + /** * test if an invalid mail result in a failure response */ - public function testCreateUnsuccessfulWithInvalidEMail() { + public function testCreateUnsuccessfulWithInvalidEmailAdmin() { $this->container['IsAdmin'] = true; /** @@ -835,7 +1175,7 @@ class UsersControllerTest extends \Test\TestCase { /** * test if a valid mail result in a successful mail send */ - public function testCreateSuccessfulWithValidEMail() { + public function testCreateSuccessfulWithValidEmailAdmin() { $this->container['IsAdmin'] = true; /** -- cgit v1.2.3 From fcd5056376c441611bcb925ff4b7e3fa892fcf3e Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Fri, 30 Jan 2015 18:31:04 +0100 Subject: Consistent variable naming --- settings/controller/userscontroller.php | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/settings/controller/userscontroller.php b/settings/controller/userscontroller.php index b1caaa17991..80fb81600df 100644 --- a/settings/controller/userscontroller.php +++ b/settings/controller/userscontroller.php @@ -273,16 +273,16 @@ class UsersController extends Controller { } if (!$this->isAdmin) { - $uid = $this->userSession->getUser()->getUID(); + $userId = $this->userSession->getUser()->getUID(); if (!empty($groups)) { foreach ($groups as $key => $group) { - if (!$this->subAdminFactory->isGroupAccessible($uid, $group)) { + if (!$this->subAdminFactory->isGroupAccessible($userId, $group)) { unset($groups[$key]); } } } if (empty($groups)) { - $groups = $this->subAdminFactory->getSubAdminsOfGroups($uid); + $groups = $this->subAdminFactory->getSubAdminsOfGroups($userId); } } @@ -367,8 +367,8 @@ class UsersController extends Controller { * @return DataResponse */ public function destroy($id) { - $UserId = $this->userSession->getUser()->getUID(); - if($UserId === $id) { + $userId = $this->userSession->getUser()->getUID(); + if($userId === $id) { return new DataResponse( array( 'status' => 'error', @@ -380,7 +380,7 @@ class UsersController extends Controller { ); } - if(!$this->isAdmin && !$this->subAdminFactory->isUserAccessible($UserId, $id)) { + if(!$this->isAdmin && !$this->subAdminFactory->isUserAccessible($userId, $id)) { return new DataResponse( array( 'status' => 'error', @@ -429,11 +429,10 @@ class UsersController extends Controller { * @return DataResponse */ public function setMailAddress($id, $mailAddress) { - $UserId = $this->userSession->getUser()->getUID(); - // FIXME: Remove this static function call at some point… - if($this->userSession->getUser()->getUID() !== $id + $userId = $this->userSession->getUser()->getUID(); + if($userId !== $id && !$this->isAdmin - && !$this->subAdminFactory->isUserAccessible($UserId, $id)) { + && !$this->subAdminFactory->isUserAccessible($userId, $id)) { return new DataResponse( array( 'status' => 'error', -- cgit v1.2.3