From ae77067a073992938a541be7ac6cc966e8e2f00c Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 18 Jan 2017 11:43:52 +0100 Subject: No need to check the subadmin again The user needs to be a subadmin of the group, otherwise they are not allowed to remove anyone from the group Signed-off-by: Joas Schilling --- .../lib/Controller/UsersController.php | 27 +++++++++------------- .../tests/Controller/UsersControllerTest.php | 5 ---- 2 files changed, 11 insertions(+), 21 deletions(-) (limited to 'apps') diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index cc1d63d2d34..eb09210275d 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -33,10 +33,10 @@ use \OC_Helper; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCS\OCSException; use OCP\AppFramework\OCS\OCSForbiddenException; -use OCP\AppFramework\OCS\OCSNotFoundException; use OCP\AppFramework\OCSController; use OCP\Files\NotFoundException; use OCP\IConfig; +use OCP\IGroup; use OCP\IGroupManager; use OCP\ILogger; use OCP\IRequest; @@ -275,9 +275,9 @@ class UsersController extends OCSController { break; case 'quota': $quota = $value; - if($quota !== 'none' and $quota !== 'default') { + if($quota !== 'none' && $quota !== 'default') { if (is_numeric($quota)) { - $quota = floatval($quota); + $quota = (float) $quota; } else { $quota = \OCP\Util::computerFileSize($quota); } @@ -421,6 +421,7 @@ class UsersController extends OCSController { // Looking up someone else if($subAdminManager->isUserAccessible($loggedInUser, $targetUser)) { // Return the group that the method caller is subadmin of for the user in question + /** @var IGroup[] $getSubAdminsGroups */ $getSubAdminsGroups = $subAdminManager->getSubAdminsGroups($loggedInUser); foreach ($getSubAdminsGroups as $key => $group) { $getSubAdminsGroups[$key] = $group->getGID(); @@ -492,25 +493,19 @@ class UsersController extends OCSController { // If they're not an admin, check they are a subadmin of the group in question $subAdminManager = $this->groupManager->getSubAdmin(); - if(!$this->groupManager->isAdmin($loggedInUser->getUID()) && !$subAdminManager->isSubAdminofGroup($loggedInUser, $group)) { + if (!$this->groupManager->isAdmin($loggedInUser->getUID()) && !$subAdminManager->isSubAdminOfGroup($loggedInUser, $group)) { throw new OCSException('', 104); } + // Check they aren't removing themselves from 'admin' or their 'subadmin; group - if($userId === $loggedInUser->getUID()) { - if($this->groupManager->isAdmin($loggedInUser->getUID())) { - if($group->getGID() === 'admin') { + if ($userId === $loggedInUser->getUID()) { + if ($this->groupManager->isAdmin($loggedInUser->getUID())) { + if ($group->getGID() === 'admin') { throw new OCSException('Cannot remove yourself from the admin group', 105); } } else { - // Not an admin, check they are not removing themself from their subadmin group - $subAdminGroups = $subAdminManager->getSubAdminsGroups($loggedInUser); - foreach ($subAdminGroups as $key => $group) { - $subAdminGroups[$key] = $group->getGID(); - } - - if(in_array($group->getGID(), $subAdminGroups, true)) { - throw new OCSException('Cannot remove yourself from this group as you are a SubAdmin', 105); - } + // Not an admin, so the user must be a subadmin of this group, but that is not allowed. + throw new OCSException('Cannot remove yourself from this group as you are a SubAdmin', 105); } } diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index e04ee86feae..761b5a5b1cc 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -1813,11 +1813,6 @@ class UsersControllerTest extends OriginalTest { ->method('isSubAdminofGroup') ->with($loggedInUser, $targetGroup) ->will($this->returnValue(true)); - $subAdminManager - ->expects($this->once()) - ->method('getSubAdminsGroups') - ->with($loggedInUser) - ->will($this->returnValue([$targetGroup])); $this->groupManager ->expects($this->once()) ->method('getSubAdmin') -- cgit v1.2.3 From d80a4453af8271040026b781edb3c915468f6d52 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 18 Jan 2017 11:56:24 +0100 Subject: Make sure subadmins can not delete users from their last subadmin group Signed-off-by: Joas Schilling --- .../lib/Controller/UsersController.php | 14 +++++ .../tests/Controller/UsersControllerTest.php | 62 ++++++++++++++++++++++ 2 files changed, 76 insertions(+) (limited to 'apps') diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index eb09210275d..022cbf92814 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -507,6 +507,20 @@ class UsersController extends OCSController { // Not an admin, so the user must be a subadmin of this group, but that is not allowed. throw new OCSException('Cannot remove yourself from this group as you are a SubAdmin', 105); } + + } else if (!$this->groupManager->isAdmin($loggedInUser->getUID())) { + /** @var IGroup[] $subAdminGroups */ + $subAdminGroups = $subAdminManager->getSubAdminsGroups($loggedInUser); + $subAdminGroups = array_map(function (IGroup $subAdminGroup) { + return $subAdminGroup->getGID(); + }, $subAdminGroups); + $userGroups = $this->groupManager->getUserGroupIds($targetUser); + $userSubAdminGroups = array_intersect($subAdminGroups, $userGroups); + + if (count($userSubAdminGroups) <= 1) { + // Subadmin must not be able to remove a user from all their subadmin groups. + throw new OCSException('Cannot remove user from this group as this is the only remaining group you are a SubAdmin of', 105); + } } // Remove user from group diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index 761b5a5b1cc..78dbbfdfc30 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -1826,6 +1826,68 @@ class UsersControllerTest extends OriginalTest { $this->api->removeFromGroup('subadmin', 'subadmin'); } + /** + * @expectedException \OCP\AppFramework\OCS\OCSException + * @expectedExceptionCode 105 + * @expectedExceptionMessage Cannot remove user from this group as this is the only remaining group you are a SubAdmin of + */ + public function testRemoveFromGroupAsSubAdminFromLastSubAdminGroup() { + $loggedInUser = $this->getMockBuilder('\OCP\IUser')->disableOriginalConstructor()->getMock(); + $loggedInUser + ->expects($this->any()) + ->method('getUID') + ->will($this->returnValue('subadmin')); + $targetUser = $this->getMockBuilder('\OCP\IUser')->disableOriginalConstructor()->getMock(); + $targetGroup = $this->getMockBuilder('\OCP\IGroup')->disableOriginalConstructor()->getMock(); + $targetGroup + ->expects($this->any()) + ->method('getGID') + ->will($this->returnValue('subadmin')); + $this->userSession + ->expects($this->once()) + ->method('getUser') + ->will($this->returnValue($loggedInUser)); + $this->groupManager + ->expects($this->once()) + ->method('get') + ->with('subadmin') + ->will($this->returnValue($targetGroup)); + $this->userManager + ->expects($this->once()) + ->method('get') + ->with('AnotherUser') + ->will($this->returnValue($targetUser)); + $subAdminManager = $this->getMockBuilder('OC\SubAdmin') + ->disableOriginalConstructor()->getMock(); + $subAdminManager + ->expects($this->once()) + ->method('isSubAdminofGroup') + ->with($loggedInUser, $targetGroup) + ->will($this->returnValue(true)); + $this->groupManager + ->expects($this->once()) + ->method('getSubAdmin') + ->will($this->returnValue($subAdminManager)); + $subAdminManager + ->expects($this->once()) + ->method('getSubAdminsGroups') + ->with($loggedInUser) + ->will($this->returnValue([$targetGroup])); + + $this->groupManager + ->expects($this->any()) + ->method('isAdmin') + ->with('subadmin') + ->will($this->returnValue(false)); + $this->groupManager + ->expects($this->once()) + ->method('getUserGroupIds') + ->with($targetUser) + ->willReturn(['subadmin', 'other group']); + + $this->api->removeFromGroup('AnotherUser', 'subadmin'); + } + public function testRemoveFromGroupSuccessful() { $loggedInUser = $this->getMockBuilder('\OCP\IUser')->disableOriginalConstructor()->getMock(); $loggedInUser -- cgit v1.2.3 From 5d1f7e5a7be4eba2c1cb0e601c7bc0ac43e1855f Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 18 Jan 2017 14:34:38 +0100 Subject: Allow subadmins to add people to groups via provisioning api Signed-off-by: Joas Schilling --- .../lib/Controller/UsersController.php | 9 ++ .../tests/Controller/UsersControllerTest.php | 149 ++++++++++++++++++++- 2 files changed, 151 insertions(+), 7 deletions(-) (limited to 'apps') diff --git a/apps/provisioning_api/lib/Controller/UsersController.php b/apps/provisioning_api/lib/Controller/UsersController.php index 022cbf92814..2e8a2ffe5ed 100644 --- a/apps/provisioning_api/lib/Controller/UsersController.php +++ b/apps/provisioning_api/lib/Controller/UsersController.php @@ -441,6 +441,8 @@ class UsersController extends OCSController { /** * @PasswordConfirmationRequired + * @NoAdminRequired + * * @param string $userId * @param string $groupid * @return DataResponse @@ -460,6 +462,13 @@ class UsersController extends OCSController { throw new OCSException('', 103); } + // If they're not an admin, check they are a subadmin of the group in question + $loggedInUser = $this->userSession->getUser(); + $subAdminManager = $this->groupManager->getSubAdmin(); + if (!$this->groupManager->isAdmin($loggedInUser->getUID()) && !$subAdminManager->isSubAdminOfGroup($loggedInUser, $group)) { + throw new OCSException('', 104); + } + // Add user to group $group->addUser($targetUser); return new DataResponse(); diff --git a/apps/provisioning_api/tests/Controller/UsersControllerTest.php b/apps/provisioning_api/tests/Controller/UsersControllerTest.php index 78dbbfdfc30..4d3da5fd33a 100644 --- a/apps/provisioning_api/tests/Controller/UsersControllerTest.php +++ b/apps/provisioning_api/tests/Controller/UsersControllerTest.php @@ -30,6 +30,9 @@ namespace OCA\Provisioning_API\Tests\Controller; use OCA\Provisioning_API\Controller\UsersController; +use OCP\AppFramework\Http\DataResponse; +use OCP\IGroup; +use OCP\IUser; use OCP\IUserManager; use OCP\IConfig; use OCP\IUserSession; @@ -1598,11 +1601,10 @@ class UsersControllerTest extends OriginalTest { * @expectedExceptionCode 102 */ public function testAddToGroupWithTargetGroupNotExisting() { - $this->groupManager - ->expects($this->once()) + $this->groupManager->expects($this->once()) ->method('get') ->with('GroupToAddTo') - ->will($this->returnValue(null)); + ->willReturn(null); $this->api->addToGroup('TargetUser', 'GroupToAddTo'); } @@ -1620,16 +1622,149 @@ class UsersControllerTest extends OriginalTest { * @expectedExceptionCode 103 */ public function testAddToGroupWithTargetUserNotExisting() { - $targetGroup = $this->getMockBuilder('\OCP\IGroup')->disableOriginalConstructor()->getMock(); - $this->groupManager - ->expects($this->once()) + $targetGroup = $this->createMock(IGroup::class); + $this->groupManager->expects($this->once()) ->method('get') ->with('GroupToAddTo') - ->will($this->returnValue($targetGroup)); + ->willReturn($targetGroup); + + $this->api->addToGroup('TargetUser', 'GroupToAddTo'); + } + + /** + * @expectedException \OCP\AppFramework\OCS\OCSException + * @expectedExceptionCode 104 + */ + public function testAddToGroupNoSubadmin() { + $targetUser = $this->createMock(IUser::class); + $loggedInUser = $this->createMock(IUser::class); + $loggedInUser->expects($this->once()) + ->method('getUID') + ->willReturn('subadmin'); + + $targetGroup = $this->createMock(IGroup::class); + $targetGroup->expects($this->never()) + ->method('addUser') + ->with($targetUser); + + $this->groupManager->expects($this->once()) + ->method('get') + ->with('GroupToAddTo') + ->willReturn($targetGroup); + + + $subAdminManager = $this->createMock(\OC\SubAdmin::class); + $subAdminManager->expects($this->once()) + ->method('isSubAdminOfGroup') + ->with($loggedInUser, $targetGroup) + ->willReturn(false); + + $this->groupManager->expects($this->once()) + ->method('getSubAdmin') + ->willReturn($subAdminManager); + $this->groupManager->expects($this->once()) + ->method('isAdmin') + ->with('subadmin') + ->willReturn(false); + + $this->userManager->expects($this->once()) + ->method('get') + ->with('TargetUser') + ->willReturn($targetUser); + + $this->userSession->expects($this->once()) + ->method('getUser') + ->willReturn($loggedInUser); $this->api->addToGroup('TargetUser', 'GroupToAddTo'); } + public function testAddToGroupSuccessAsSubadmin() { + $targetUser = $this->createMock(IUser::class); + $loggedInUser = $this->createMock(IUser::class); + $loggedInUser->expects($this->once()) + ->method('getUID') + ->willReturn('subadmin'); + + $targetGroup = $this->createMock(IGroup::class); + $targetGroup->expects($this->once()) + ->method('addUser') + ->with($targetUser); + + $this->groupManager->expects($this->once()) + ->method('get') + ->with('GroupToAddTo') + ->willReturn($targetGroup); + + + $subAdminManager = $this->createMock(\OC\SubAdmin::class); + $subAdminManager->expects($this->once()) + ->method('isSubAdminOfGroup') + ->with($loggedInUser, $targetGroup) + ->willReturn(true); + + $this->groupManager->expects($this->once()) + ->method('getSubAdmin') + ->willReturn($subAdminManager); + $this->groupManager->expects($this->once()) + ->method('isAdmin') + ->with('subadmin') + ->willReturn(false); + + $this->userManager->expects($this->once()) + ->method('get') + ->with('TargetUser') + ->willReturn($targetUser); + + $this->userSession->expects($this->once()) + ->method('getUser') + ->willReturn($loggedInUser); + + $this->assertEquals(new DataResponse(), $this->api->addToGroup('TargetUser', 'GroupToAddTo')); + } + + public function testAddToGroupSuccessAsAdmin() { + $targetUser = $this->createMock(IUser::class); + $loggedInUser = $this->createMock(IUser::class); + $loggedInUser->expects($this->once()) + ->method('getUID') + ->willReturn('admin'); + + $targetGroup = $this->createMock(IGroup::class); + $targetGroup->expects($this->once()) + ->method('addUser') + ->with($targetUser); + + $this->groupManager->expects($this->once()) + ->method('get') + ->with('GroupToAddTo') + ->willReturn($targetGroup); + + + $subAdminManager = $this->createMock(\OC\SubAdmin::class); + $subAdminManager->expects($this->never()) + ->method('isSubAdminOfGroup'); + + $this->groupManager->expects($this->once()) + ->method('getSubAdmin') + ->willReturn($subAdminManager); + $this->groupManager->expects($this->once()) + ->method('isAdmin') + ->with('admin') + ->willReturn(true); + + $this->userManager->expects($this->once()) + ->method('get') + ->with('TargetUser') + ->willReturn($targetUser); + + $this->userSession->expects($this->once()) + ->method('getUser') + ->willReturn($loggedInUser); + + $this->assertEquals(new DataResponse(), $this->api->addToGroup('TargetUser', 'GroupToAddTo')); + } + /** * @expectedException \OCP\AppFramework\OCS\OCSException * @expectedExceptionCode 101 -- cgit v1.2.3