From 992c48c89bd71971a3821143aeac7dcb8f616461 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 24 Apr 2017 09:43:44 +0200 Subject: [PATCH 1/3] Fix the storage info and other checks when the user has wrong casing Signed-off-by: Joas Schilling --- .../lib/Controller/UsersController.php | 10 +- .../tests/Controller/UsersControllerTest.php | 108 +++++++++++++----- 2 files changed, 83 insertions(+), 35 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index b1a1cf1c6b..088b0e6bdb 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -278,10 +278,10 @@ class UsersController extends OCSController { // Admin? Or SubAdmin? if($this->groupManager->isAdmin($currentLoggedInUser->getUID()) || $this->groupManager->getSubAdmin()->isUserAccessible($currentLoggedInUser, $targetUserObject)) { - $data['enabled'] = $this->config->getUserValue($userId, 'core', 'enabled', 'true'); + $data['enabled'] = $this->config->getUserValue($targetUserObject->getUID(), 'core', 'enabled', 'true'); } else { // Check they are looking up themselves - if($currentLoggedInUser->getUID() !== $userId) { + if($currentLoggedInUser->getUID() !== $targetUserObject->getUID()) { throw new OCSException('', \OCP\API::RESPOND_UNAUTHORISED); } } @@ -295,7 +295,7 @@ class UsersController extends OCSController { // Find the data $data['id'] = $targetUserObject->getUID(); - $data['quota'] = $this->fillStorageInfo($userId); + $data['quota'] = $this->fillStorageInfo($targetUserObject->getUID()); $data[AccountManager::PROPERTY_EMAIL] = $targetUserObject->getEMailAddress(); $data[AccountManager::PROPERTY_DISPLAYNAME] = $targetUserObject->getDisplayName(); $data[AccountManager::PROPERTY_PHONE] = $userAccount[AccountManager::PROPERTY_PHONE]['value']; @@ -330,7 +330,7 @@ class UsersController extends OCSController { } $permittedFields = []; - if($userId === $currentLoggedInUser->getUID()) { + if($targetUser->getUID() === $currentLoggedInUser->getUID()) { // Editing self (display, email) $permittedFields[] = 'display'; $permittedFields[] = AccountManager::PROPERTY_DISPLAYNAME; @@ -618,7 +618,7 @@ class UsersController extends OCSController { } // Check they aren't removing themselves from 'admin' or their 'subadmin; group - if ($userId === $loggedInUser->getUID()) { + if ($targetUser->getUID() === $loggedInUser->getUID()) { if ($this->groupManager->isAdmin($loggedInUser->getUID())) { if ($group->getGID() === 'admin') { throw new OCSException('Cannot remove yourself from the admin group', 105); diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index 2eb3853d33..42c94f095f 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -705,19 +705,19 @@ class UsersControllerTest extends TestCase { $this->config ->expects($this->at(0)) ->method('getUserValue') - ->with('UserToGet', 'core', 'enabled', 'true') + ->with('UID', 'core', 'enabled', 'true') ->will($this->returnValue('true')); $this->api ->expects($this->once()) ->method('fillStorageInfo') - ->with('UserToGet') + ->with('UID') ->will($this->returnValue(['DummyValue'])); $targetUser ->expects($this->once()) ->method('getDisplayName') ->will($this->returnValue('Demo User')); $targetUser - ->expects($this->once()) + ->expects($this->exactly(3)) ->method('getUID') ->will($this->returnValue('UID')); @@ -784,19 +784,19 @@ class UsersControllerTest extends TestCase { $this->config ->expects($this->at(0)) ->method('getUserValue') - ->with('UserToGet', 'core', 'enabled', 'true') + ->with('UID', 'core', 'enabled', 'true') ->will($this->returnValue('true')); $this->api ->expects($this->once()) ->method('fillStorageInfo') - ->with('UserToGet') + ->with('UID') ->will($this->returnValue(['DummyValue'])); $targetUser ->expects($this->once()) ->method('getDisplayName') ->will($this->returnValue('Demo User')); $targetUser - ->expects($this->once()) + ->expects($this->exactly(3)) ->method('getUID') ->will($this->returnValue('UID')); $this->accountManager->expects($this->any())->method('getUser') @@ -878,7 +878,7 @@ class UsersControllerTest extends TestCase { $loggedInUser ->expects($this->exactly(2)) ->method('getUID') - ->will($this->returnValue('subadmin')); + ->will($this->returnValue('UID')); $targetUser = $this->getMockBuilder(IUser::class) ->disableOriginalConstructor() ->getMock(); @@ -894,7 +894,7 @@ class UsersControllerTest extends TestCase { $this->groupManager ->expects($this->once()) ->method('isAdmin') - ->with('subadmin') + ->with('UID') ->will($this->returnValue(false)); $subAdminManager = $this->getMockBuilder('OC\SubAdmin') ->disableOriginalConstructor() @@ -915,7 +915,7 @@ class UsersControllerTest extends TestCase { $this->api ->expects($this->once()) ->method('fillStorageInfo') - ->with('subadmin') + ->with('UID') ->will($this->returnValue(['DummyValue'])); $targetUser ->expects($this->once()) @@ -926,7 +926,7 @@ class UsersControllerTest extends TestCase { ->method('getEMailAddress') ->will($this->returnValue('subadmin@owncloud.org')); $targetUser - ->expects($this->once()) + ->expects($this->exactly(3)) ->method('getUID') ->will($this->returnValue('UID')); $this->accountManager->expects($this->any())->method('getUser') @@ -961,7 +961,7 @@ class UsersControllerTest extends TestCase { $loggedInUser ->expects($this->any()) ->method('getUID') - ->will($this->returnValue('UserToEdit')); + ->will($this->returnValue('UID')); $targetUser = $this->getMockBuilder(IUser::class) ->disableOriginalConstructor() ->getMock(); @@ -978,6 +978,10 @@ class UsersControllerTest extends TestCase { ->expects($this->once()) ->method('setDisplayName') ->with('NewDisplayName'); + $targetUser + ->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('UID')); $this->assertEquals([], $this->api->editUser('UserToEdit', 'display', 'NewDisplayName')->getData()); } @@ -989,7 +993,7 @@ class UsersControllerTest extends TestCase { $loggedInUser ->expects($this->any()) ->method('getUID') - ->will($this->returnValue('UserToEdit')); + ->will($this->returnValue('UID')); $targetUser = $this->getMockBuilder(IUser::class) ->disableOriginalConstructor() ->getMock(); @@ -1006,6 +1010,10 @@ class UsersControllerTest extends TestCase { ->expects($this->once()) ->method('setEMailAddress') ->with('demo@owncloud.org'); + $targetUser + ->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('UID')); $this->assertEquals([], $this->api->editUser('UserToEdit', 'email', 'demo@owncloud.org')->getData()); } @@ -1022,7 +1030,7 @@ class UsersControllerTest extends TestCase { $loggedInUser ->expects($this->any()) ->method('getUID') - ->will($this->returnValue('UserToEdit')); + ->will($this->returnValue('UID')); $targetUser = $this->getMockBuilder(IUser::class) ->disableOriginalConstructor() ->getMock(); @@ -1035,6 +1043,10 @@ class UsersControllerTest extends TestCase { ->method('get') ->with('UserToEdit') ->will($this->returnValue($targetUser)); + $targetUser + ->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('UID')); $this->api->editUser('UserToEdit', 'email', 'demo.org'); } @@ -1046,7 +1058,7 @@ class UsersControllerTest extends TestCase { $loggedInUser ->expects($this->any()) ->method('getUID') - ->will($this->returnValue('UserToEdit')); + ->will($this->returnValue('UID')); $targetUser = $this->getMockBuilder(IUser::class) ->disableOriginalConstructor() ->getMock(); @@ -1063,6 +1075,10 @@ class UsersControllerTest extends TestCase { ->expects($this->once()) ->method('setPassword') ->with('NewPassword'); + $targetUser + ->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('UID')); $this->assertEquals([], $this->api->editUser('UserToEdit', 'password', 'NewPassword')->getData()); } @@ -1079,7 +1095,7 @@ class UsersControllerTest extends TestCase { $loggedInUser ->expects($this->any()) ->method('getUID') - ->will($this->returnValue('UserToEdit')); + ->will($this->returnValue('UID')); $targetUser = $this->getMockBuilder(IUser::class) ->disableOriginalConstructor() ->getMock(); @@ -1092,6 +1108,10 @@ class UsersControllerTest extends TestCase { ->method('get') ->with('UserToEdit') ->will($this->returnValue($targetUser)); + $targetUser + ->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('UID')); $this->api->editUser('UserToEdit', 'quota', 'NewQuota'); } @@ -1101,7 +1121,7 @@ class UsersControllerTest extends TestCase { $loggedInUser ->expects($this->any()) ->method('getUID') - ->will($this->returnValue('UserToEdit')); + ->will($this->returnValue('UID')); $targetUser = $this->getMockBuilder('\OCP\IUser')->disableOriginalConstructor()->getMock(); $targetUser->expects($this->once()) ->method('setQuota') @@ -1118,8 +1138,12 @@ class UsersControllerTest extends TestCase { $this->groupManager ->expects($this->once()) ->method('isAdmin') - ->with('UserToEdit') + ->with('UID') ->will($this->returnValue(true)); + $targetUser + ->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('UID')); $this->assertEquals([], $this->api->editUser('UserToEdit', 'quota', '3042824')->getData()); } @@ -1135,7 +1159,7 @@ class UsersControllerTest extends TestCase { $loggedInUser ->expects($this->any()) ->method('getUID') - ->will($this->returnValue('UserToEdit')); + ->will($this->returnValue('UID')); $targetUser = $this->getMockBuilder('\OCP\IUser')->disableOriginalConstructor()->getMock(); $this->userSession ->expects($this->once()) @@ -1149,8 +1173,12 @@ class UsersControllerTest extends TestCase { $this->groupManager ->expects($this->once()) ->method('isAdmin') - ->with('UserToEdit') + ->with('UID') ->will($this->returnValue(true)); + $targetUser + ->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('UID')); $this->api->editUser('UserToEdit', 'quota', 'ABC'); } @@ -1186,6 +1214,10 @@ class UsersControllerTest extends TestCase { ->expects($this->once()) ->method('getSubAdmin') ->will($this->returnValue($subAdminManager)); + $targetUser + ->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('UID')); $this->assertEquals([], $this->api->editUser('UserToEdit', 'quota', '3042824')->getData()); } @@ -1221,6 +1253,10 @@ class UsersControllerTest extends TestCase { ->expects($this->once()) ->method('getSubAdmin') ->will($this->returnValue($subAdminManager)); + $targetUser + ->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('UID')); $this->assertEquals([], $this->api->editUser('UserToEdit', 'quota', '3042824')->getData()); } @@ -1257,6 +1293,10 @@ class UsersControllerTest extends TestCase { ->expects($this->once()) ->method('getSubAdmin') ->will($this->returnValue($subAdminManager)); + $targetUser + ->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('UID')); $this->api->editUser('UserToEdit', 'quota', 'value'); } @@ -1293,12 +1333,12 @@ class UsersControllerTest extends TestCase { $loggedInUser ->expects($this->any()) ->method('getUID') - ->will($this->returnValue('UserToDelete')); + ->will($this->returnValue('UID')); $targetUser = $this->getMockBuilder('\OCP\IUser')->disableOriginalConstructor()->getMock(); $targetUser ->expects($this->once()) ->method('getUID') - ->will($this->returnValue('UserToDelete')); + ->will($this->returnValue('UID')); $this->userSession ->expects($this->once()) ->method('getUser') @@ -1322,7 +1362,7 @@ class UsersControllerTest extends TestCase { $targetUser ->expects($this->once()) ->method('getUID') - ->will($this->returnValue('UserToDelete')); + ->will($this->returnValue('UID')); $this->userSession ->expects($this->once()) ->method('getUser') @@ -1359,7 +1399,7 @@ class UsersControllerTest extends TestCase { $targetUser ->expects($this->once()) ->method('getUID') - ->will($this->returnValue('UserToDelete')); + ->will($this->returnValue('UID')); $this->userSession ->expects($this->once()) ->method('getUser') @@ -1392,7 +1432,7 @@ class UsersControllerTest extends TestCase { $targetUser ->expects($this->once()) ->method('getUID') - ->will($this->returnValue('UserToDelete')); + ->will($this->returnValue('UID')); $this->userSession ->expects($this->once()) ->method('getUser') @@ -1440,7 +1480,7 @@ class UsersControllerTest extends TestCase { $targetUser ->expects($this->once()) ->method('getUID') - ->will($this->returnValue('UserToDelete')); + ->will($this->returnValue('UID')); $this->userSession ->expects($this->once()) ->method('getUser') @@ -1488,7 +1528,7 @@ class UsersControllerTest extends TestCase { $targetUser ->expects($this->once()) ->method('getUID') - ->will($this->returnValue('UserToDelete')); + ->will($this->returnValue('UID')); $this->userSession ->expects($this->once()) ->method('getUser') @@ -1984,6 +2024,10 @@ class UsersControllerTest extends TestCase { ->method('getUID') ->will($this->returnValue('admin')); $targetUser = $this->getMockBuilder('\OCP\IUser')->disableOriginalConstructor()->getMock(); + $targetUser + ->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('admin')); $targetGroup = $this->getMockBuilder('\OCP\IGroup')->disableOriginalConstructor()->getMock(); $targetGroup ->expects($this->once()) @@ -2001,7 +2045,7 @@ class UsersControllerTest extends TestCase { $this->userManager ->expects($this->once()) ->method('get') - ->with('admin') + ->with('Admin') ->will($this->returnValue($targetUser)); $subAdminManager = $this->getMockBuilder('OC\SubAdmin') ->disableOriginalConstructor()->getMock(); @@ -2015,7 +2059,7 @@ class UsersControllerTest extends TestCase { ->with('admin') ->will($this->returnValue(true)); - $this->api->removeFromGroup('admin', 'admin'); + $this->api->removeFromGroup('Admin', 'admin'); } /** @@ -2030,6 +2074,10 @@ class UsersControllerTest extends TestCase { ->method('getUID') ->will($this->returnValue('subadmin')); $targetUser = $this->getMockBuilder('\OCP\IUser')->disableOriginalConstructor()->getMock(); + $targetUser + ->expects($this->once()) + ->method('getUID') + ->will($this->returnValue('subadmin')); $targetGroup = $this->getMockBuilder('\OCP\IGroup')->disableOriginalConstructor()->getMock(); $targetGroup ->expects($this->any()) @@ -2047,7 +2095,7 @@ class UsersControllerTest extends TestCase { $this->userManager ->expects($this->once()) ->method('get') - ->with('subadmin') + ->with('SubAdmin') ->will($this->returnValue($targetUser)); $subAdminManager = $this->getMockBuilder('OC\SubAdmin') ->disableOriginalConstructor()->getMock(); @@ -2066,7 +2114,7 @@ class UsersControllerTest extends TestCase { ->with('subadmin') ->will($this->returnValue(false)); - $this->api->removeFromGroup('subadmin', 'subadmin'); + $this->api->removeFromGroup('SubAdmin', 'subadmin'); } /** From e19126425bd24ab0a357bd6eb2673512b49dfef6 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 24 Apr 2017 10:15:03 +0200 Subject: [PATCH 2/3] Fix similar issues with the group id Signed-off-by: Joas Schilling --- apps/provisioning_api/lib/Controller/UsersController.php | 6 +++--- .../tests/Controller/UsersControllerTest.php | 6 +++++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 088b0e6bdb..ca78297a1a 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -668,10 +668,10 @@ class UsersController extends OCSController { } // Check if group exists if($group === null) { - throw new OCSException('Group:'.$groupid.' does not exist', 102); + throw new OCSException('Group does not exist', 102); } // Check if trying to make subadmin of admin group - if(strtolower($groupid) === 'admin') { + if($group->getGID() === 'admin') { throw new OCSException('Cannot create subadmins for admin group', 103); } @@ -713,7 +713,7 @@ class UsersController extends OCSController { throw new OCSException('Group does not exist', 101); } // Check if they are a subadmin of this said group - if(!$subAdminManager->isSubAdminofGroup($user, $group)) { + if(!$subAdminManager->isSubAdminOfGroup($user, $group)) { throw new OCSException('User is not a subadmin of this group', 102); } diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index 42c94f095f..766c92c964 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -2238,7 +2238,7 @@ class UsersControllerTest extends TestCase { /** * @expectedException \OCP\AppFramework\OCS\OCSException * @expectedExceptionCode 102 - * @expectedExceptionMessage Group:NotExistingGroup does not exist + * @expectedExceptionMessage Group does not exist */ public function testAddSubAdminWithNotExistingTargetGroup() { @@ -2265,6 +2265,10 @@ class UsersControllerTest extends TestCase { public function testAddSubAdminToAdminGroup() { $targetUser = $this->getMockBuilder('\OCP\IUser')->disableOriginalConstructor()->getMock(); $targetGroup = $this->getMockBuilder('\OCP\IGroup')->disableOriginalConstructor()->getMock(); + $targetGroup + ->expects($this->once()) + ->method('getGID') + ->will($this->returnValue('admin')); $this->userManager ->expects($this->once()) ->method('get') From 7a87fedac3db6de22fc35c968771a8b79cbb3397 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 24 Apr 2017 10:18:14 +0200 Subject: [PATCH 3/3] Remove dead arguments Signed-off-by: Joas Schilling --- .../lib/Controller/UsersController.php | 25 +------------------ .../tests/Controller/UsersControllerTest.php | 21 ---------------- 2 files changed, 1 insertion(+), 45 deletions(-) diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index ca78297a1a..6e34fe53eb 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -31,23 +31,20 @@ namespace OCA\Provisioning_API\Controller; use OC\Accounts\AccountManager; use OC\Settings\Mailer\NewUserMailHelper; -use \OC_Helper; +use OC_Helper; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCS\OCSException; use OCP\AppFramework\OCS\OCSForbiddenException; use OCP\AppFramework\OCSController; -use OCP\Defaults; use OCP\Files\NotFoundException; use OCP\IConfig; use OCP\IGroup; use OCP\IGroupManager; use OCP\ILogger; use OCP\IRequest; -use OCP\IURLGenerator; use OCP\IUserManager; use OCP\IUserSession; use OCP\L10N\IFactory; -use OCP\Mail\IMailer; class UsersController extends OCSController { @@ -63,14 +60,6 @@ class UsersController extends OCSController { private $accountManager; /** @var ILogger */ private $logger; - /** @var string */ - private $fromMailAddress; - /** @var IURLGenerator */ - private $urlGenerator; - /** @var IMailer */ - private $mailer; - /** @var Defaults */ - private $defaults; /** @var IFactory */ private $l10nFactory; /** @var NewUserMailHelper */ @@ -85,10 +74,6 @@ class UsersController extends OCSController { * @param IUserSession $userSession * @param AccountManager $accountManager * @param ILogger $logger - * @param string $fromMailAddress - * @param IURLGenerator $urlGenerator - * @param IMailer $mailer - * @param Defaults $defaults * @param IFactory $l10nFactory * @param NewUserMailHelper $newUserMailHelper */ @@ -100,10 +85,6 @@ class UsersController extends OCSController { IUserSession $userSession, AccountManager $accountManager, ILogger $logger, - $fromMailAddress, - IURLGenerator $urlGenerator, - IMailer $mailer, - Defaults $defaults, IFactory $l10nFactory, NewUserMailHelper $newUserMailHelper) { parent::__construct($appName, $request); @@ -114,10 +95,6 @@ class UsersController extends OCSController { $this->userSession = $userSession; $this->accountManager = $accountManager; $this->logger = $logger; - $this->fromMailAddress = $fromMailAddress; - $this->urlGenerator = $urlGenerator; - $this->mailer = $mailer; - $this->defaults = $defaults; $this->l10nFactory = $l10nFactory; $this->newUserMailHelper = $newUserMailHelper; } diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index 766c92c964..57e1d2eac6 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -70,12 +70,6 @@ class UsersControllerTest extends TestCase { protected $accountManager; /** @var IRequest|PHPUnit_Framework_MockObject_MockObject */ protected $request; - /** @var IURLGenerator|PHPUnit_Framework_MockObject_MockObject */ - private $urlGenerator; - /** @var IMailer|PHPUnit_Framework_MockObject_MockObject */ - private $mailer; - /** @var Defaults|PHPUnit_Framework_MockObject_MockObject */ - private $defaults; /** @var IFactory|PHPUnit_Framework_MockObject_MockObject */ private $l10nFactory; /** @var NewUserMailHelper|PHPUnit_Framework_MockObject_MockObject */ @@ -91,9 +85,6 @@ class UsersControllerTest extends TestCase { $this->logger = $this->createMock(ILogger::class); $this->request = $this->createMock(IRequest::class); $this->accountManager = $this->createMock(AccountManager::class); - $this->urlGenerator = $this->createMock(IURLGenerator::class); - $this->mailer = $this->createMock(IMailer::class); - $this->defaults = $this->createMock(Defaults::class); $this->l10nFactory = $this->createMock(IFactory::class); $this->newUserMailHelper = $this->createMock(NewUserMailHelper::class); @@ -107,10 +98,6 @@ class UsersControllerTest extends TestCase { $this->userSession, $this->accountManager, $this->logger, - 'test@example.org', - $this->urlGenerator, - $this->mailer, - $this->defaults, $this->l10nFactory, $this->newUserMailHelper ]) @@ -2664,10 +2651,6 @@ class UsersControllerTest extends TestCase { $this->userSession, $this->accountManager, $this->logger, - '', - $this->urlGenerator, - $this->mailer, - $this->defaults, $this->l10nFactory, $this->newUserMailHelper ]) @@ -2728,10 +2711,6 @@ class UsersControllerTest extends TestCase { $this->userSession, $this->accountManager, $this->logger, - '', - $this->urlGenerator, - $this->mailer, - $this->defaults, $this->l10nFactory, $this->newUserMailHelper ])