From 0f7634eadc2eee3a32116e2b2686ca462dfaaafa Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Fri, 30 Jan 2015 17:24:42 +0100 Subject: [PATCH] Switch to a factory and add unit tests --- settings/application.php | 7 +- settings/controller/userscontroller.php | 43 +- settings/factory/subadminfactory.php | 45 ++ .../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; /** -- 2.39.5