From 2d7f6d4dffba028755b73841418a69fda16b5e95 Mon Sep 17 00:00:00 2001 From: Sujith H Date: Tue, 12 Jun 2018 18:49:33 +0530 Subject: [PATCH] [stable10] Restrict group admin create groups using userprovision API Using userprovision API, restrict group admin create groups. Its against the design. Also made minor changes in the method deleteGroup for better code readability. Signed-off-by: Sujith H --- apps/provisioning_api/lib/Groups.php | 22 +++-- apps/provisioning_api/tests/GroupsTest.php | 80 +++++++++++++++++++ .../apiProvisioning-v1/addGroup.feature | 5 +- .../apiProvisioning-v2/addGroup.feature | 3 +- 4 files changed, 98 insertions(+), 12 deletions(-) diff --git a/apps/provisioning_api/lib/Groups.php b/apps/provisioning_api/lib/Groups.php index b78e16038012..a85b5aeb470f 100644 --- a/apps/provisioning_api/lib/Groups.php +++ b/apps/provisioning_api/lib/Groups.php @@ -116,9 +116,8 @@ public function getGroup($parameters) { }, $users); $users = array_values($users); return new OC_OCS_Result(['users' => $users]); - } else { - return new OC_OCS_Result(null, \OCP\API::RESPOND_UNAUTHORISED, 'User does not have access to specified group'); } + return new OC_OCS_Result(null, \OCP\API::RESPOND_UNAUTHORISED, 'User does not have access to specified group'); } /** @@ -138,8 +137,17 @@ public function addGroup($parameters) { if($this->groupManager->groupExists($groupId)){ return new OC_OCS_Result(null, 102); } - $this->groupManager->createGroup($groupId); - return new OC_OCS_Result(null, 100); + $user = $this->userSession->getUser(); + if ($user === null) { + return new OC_OCS_Result(null, 102); + } + // Only admin has got privilege to create group + if ($this->groupManager->isAdmin($user->getUID())) { + $this->groupManager->createGroup($groupId); + return new OC_OCS_Result(null, 100); + } + + return new OC_OCS_Result(null, 997); } /** @@ -150,12 +158,12 @@ public function deleteGroup($parameters) { // Check it exists if(!$this->groupManager->groupExists($parameters['groupid'])){ return new OC_OCS_Result(null, 101); - } else if($parameters['groupid'] === 'admin' || !$this->groupManager->get($parameters['groupid'])->delete()){ + } + if($parameters['groupid'] === 'admin' || !$this->groupManager->get($parameters['groupid'])->delete()){ // Cannot delete admin group return new OC_OCS_Result(null, 102); - } else { - return new OC_OCS_Result(null, 100); } + return new OC_OCS_Result(null, 100); } /** diff --git a/apps/provisioning_api/tests/GroupsTest.php b/apps/provisioning_api/tests/GroupsTest.php index 06b5d5eac20e..cbd1e893f359 100644 --- a/apps/provisioning_api/tests/GroupsTest.php +++ b/apps/provisioning_api/tests/GroupsTest.php @@ -30,6 +30,7 @@ use OCP\API; use OCP\IGroupManager; use OCP\IRequest; +use OCP\IUser; use OCP\IUserSession; class GroupsTest extends \Test\TestCase { @@ -365,6 +366,58 @@ public function testAddGroupExistingGroup() { $this->assertEquals(102, $result->getStatusCode()); } + public function testAddGroupUserDoesNotExist() { + $this->request + ->method('getParam') + ->with('groupid') + ->willReturn('NewGroup'); + + $this->groupManager + ->method('groupExists') + ->with('NewGroup') + ->willReturn(false); + + $this->userSession + ->expects($this->once()) + ->method('getUser') + ->willReturn(null); + + $result = $this->api->addGroup([]); + $this->assertInstanceOf('OC_OCS_Result', $result); + $this->assertFalse($result->succeeded()); + $this->assertEquals(102, $result->getStatusCode()); + } + + public function testAddGroupUserNotAdmin() { + $this->request + ->method('getParam') + ->with('groupid') + ->willReturn('NewGroup'); + + $this->groupManager + ->method('groupExists') + ->with('NewGroup') + ->willReturn(false); + + $this->groupManager->expects($this->once()) + ->method('isAdmin') + ->willReturn(false); + + $iUser = $this->createMock(IUser::class); + $iUser->expects($this->once()) + ->method('getUID') + ->willReturn('user1'); + $this->userSession + ->expects($this->once()) + ->method('getUser') + ->willReturn($iUser); + + $result = $this->api->addGroup([]); + $this->assertInstanceOf('OC_OCS_Result', $result); + $this->assertFalse($result->succeeded()); + $this->assertEquals(997, $result->getStatusCode()); + } + public function testAddGroup() { $this->request ->method('getParam') @@ -381,6 +434,19 @@ public function testAddGroup() { ->method('createGroup') ->with('NewGroup'); + $this->groupManager->expects($this->once()) + ->method('isAdmin') + ->willReturn(true); + + $iUser = $this->createMock(IUser::class); + $iUser->expects($this->once()) + ->method('getUID') + ->willReturn('user1'); + $this->userSession + ->expects($this->once()) + ->method('getUser') + ->willReturn($iUser); + $result = $this->api->addGroup([]); $this->assertInstanceOf('OC_OCS_Result', $result); $this->assertTrue($result->succeeded()); @@ -402,6 +468,19 @@ public function testAddGroupWithSpecialChar() { ->method('createGroup') ->with('Iñtërnâtiônàlizætiøn'); + $this->groupManager->expects($this->once()) + ->method('isAdmin') + ->willReturn(true); + + $iUser = $this->createMock(IUser::class); + $iUser->expects($this->once()) + ->method('getUID') + ->willReturn('user1'); + $this->userSession + ->expects($this->once()) + ->method('getUser') + ->willReturn($iUser); + $result = $this->api->addGroup([]); $this->assertInstanceOf('OC_OCS_Result', $result); $this->assertTrue($result->succeeded()); @@ -451,5 +530,6 @@ public function testDeleteGroup() { ]); $this->assertInstanceOf('OC_OCS_Result', $result); $this->assertTrue($result->succeeded()); + $this->assertEquals(100, $result->getStatusCode()); } } diff --git a/tests/acceptance/features/apiProvisioning-v1/addGroup.feature b/tests/acceptance/features/apiProvisioning-v1/addGroup.feature index 6aafc1230c84..5f04afe5ed45 100644 --- a/tests/acceptance/features/apiProvisioning-v1/addGroup.feature +++ b/tests/acceptance/features/apiProvisioning-v1/addGroup.feature @@ -47,13 +47,12 @@ So that I can more easily manage access to resources by groups rather than indiv And the HTTP status code should be "401" And group "new-group" should not exist - @skip @issue-31283 Scenario: subadmin tries to create a group Given user "subadmin" has been created And group "new-group" has been created And user "subadmin" has been made a subadmin of group "new-group" And user "subadmin" sends HTTP method "POST" to API endpoint "/cloud/groups" with body | groupid | another-group | - Then the OCS status code should be "102" - And the HTTP status code should be "200" + Then the OCS status code should be "997" + And the HTTP status code should be "401" And group "another-group" should not exist \ No newline at end of file diff --git a/tests/acceptance/features/apiProvisioning-v2/addGroup.feature b/tests/acceptance/features/apiProvisioning-v2/addGroup.feature index 96e29581f93a..2e8bb50116f8 100644 --- a/tests/acceptance/features/apiProvisioning-v2/addGroup.feature +++ b/tests/acceptance/features/apiProvisioning-v2/addGroup.feature @@ -48,13 +48,12 @@ So that I can more easily manage access to resources by groups rather than indiv And the HTTP status code should be "401" And group "new-group" should not exist - @skip @issue-31283 Scenario: subadmin tries to create a group Given user "subadmin" has been created And group "new-group" has been created And user "subadmin" has been made a subadmin of group "new-group" And user "subadmin" sends HTTP method "POST" to API endpoint "/cloud/groups" with body | groupid | another-group | - Then the OCS status code should be "401" + Then the OCS status code should be "997" And the HTTP status code should be "401" And group "another-group" should not exist \ No newline at end of file