Error out when subadmin doesn't select any group

Signed-off-by: Joas Schilling <coding@schilljs.com>

diff --git a/settings/Controller/UsersController.php b/settings/Controller/UsersController.php
index 28b8d2648d99de90358fa370dd9dba02a0417ee1..43a3843249911bd33ce986294cc388ce5c49f688 100644 (file)
--- a/settings/Controller/UsersController.php
+++ b/settings/Controller/UsersController.php
@@ -346,13 +346,12 @@ class UsersController extends Controller {
                        }
 
                        if (empty($groups)) {
-                               $groups = $this->groupManager->getSubAdmin()->getSubAdminsGroups($currentUser);
-                               // New class returns IGroup[] so convert back
-                               $gids = [];
-                               foreach ($groups as $group) {
-                                       $gids[] = $group->getGID();
-                               }
-                               $groups = $gids;
+                               return new DataResponse(
+                                       array(
+                                               'message' => $this->l10n->t('No valid group selected'),
+                                       ),
+                                       Http::STATUS_FORBIDDEN
+                               );
                        }
                }
 
@@ -380,7 +379,7 @@ class UsersController extends Controller {
                        );
                }
 
-               if($user instanceof User) {
+               if($user instanceof IUser) {
                        if($groups !== null) {
                                foreach($groups as $groupName) {
                                        $group = $this->groupManager->get($groupName);

diff --git a/tests/Settings/Controller/UsersControllerTest.php b/tests/Settings/Controller/UsersControllerTest.php
index b85667740f7c4908e23a7d04a13f8c8a796b01f6..36ca09175241b4825f2080e644bfbbd3bed4af7e 100644 (file)
--- a/tests/Settings/Controller/UsersControllerTest.php
+++ b/tests/Settings/Controller/UsersControllerTest.php
@@ -19,6 +19,7 @@ use OCP\AppFramework\Http\DataResponse;
 use OCP\IAvatar;
 use OCP\IAvatarManager;
 use OCP\IConfig;
+use OCP\IGroup;
 use OCP\IGroupManager;
 use OCP\IL10N;
 use OCP\ILogger;
@@ -852,95 +853,6 @@ class UsersControllerTest extends \Test\TestCase {
                $this->assertEquals($expectedResponse, $response);
        }
 
-       public function testCreateSuccessfulWithoutGroupSubAdmin() {
-               $controller = $this->getController(false);
-               $user = $this->getMockBuilder('\OC\User\User')
-                       ->disableOriginalConstructor()->getMock();
-               $this->userSession
-                       ->expects($this->once())
-                       ->method('getUser')
-                       ->will($this->returnValue($user));
-
-               $newUser = $this->getMockBuilder('\OC\User\User')
-                       ->disableOriginalConstructor()->getMock();
-               $newUser
-                       ->method('getUID')
-                       ->will($this->returnValue('foo'));
-               $newUser
-                       ->method('getHome')
-                       ->will($this->returnValue('/home/user'));
-               $newUser
-                       ->method('getHome')
-                       ->will($this->returnValue('/home/user'));
-               $newUser
-                       ->expects($this->once())
-                       ->method('getBackendClassName')
-                       ->will($this->returnValue('bar'));
-               $user = $this->getMockBuilder('\OC\User\User')
-                       ->disableOriginalConstructor()->getMock();
-               $subGroup1 = $this->getMockBuilder('\OCP\IGroup')
-                       ->disableOriginalConstructor()->getMock();
-               $subGroup1
-                       ->expects($this->once())
-                       ->method('addUser')
-                       ->with($newUser);
-               $subGroup2 = $this->getMockBuilder('\OCP\IGroup')
-                       ->disableOriginalConstructor()->getMock();
-               $subGroup2
-                       ->expects($this->once())
-                       ->method('addUser')
-                       ->with($newUser);
-
-               $this->userManager
-                       ->expects($this->once())
-                       ->method('createUser')
-                       ->will($this->returnValue($newUser));
-               $this->groupManager
-                       ->expects($this->exactly(2))
-                       ->method('get')
-                       ->will($this->onConsecutiveCalls($subGroup1, $subGroup2));
-               $this->groupManager
-                       ->expects($this->once())
-                       ->method('getUserGroupIds')
-                       ->with($user)
-                       ->will($this->onConsecutiveCalls(['SubGroup1', 'SubGroup2']));
-
-               $subadmin = $this->getMockBuilder('\OC\SubAdmin')
-                       ->disableOriginalConstructor()
-                       ->getMock();
-               $subadmin
-                       ->expects($this->at(0))
-                       ->method('getSubAdminsGroups')
-                       ->will($this->returnValue([$subGroup1, $subGroup2]));
-               $subadmin
-                       ->expects($this->at(1))
-                       ->method('getSubAdminsGroups')
-                       ->will($this->returnValue([]));
-               $this->groupManager
-                       ->expects($this->any())
-                       ->method('getSubAdmin')
-                       ->will($this->returnValue($subadmin));
-
-               $expectedResponse = new DataResponse(
-                       array(
-                               'name' => 'foo',
-                               'groups' => ['SubGroup1', 'SubGroup2'],
-                               'storageLocation' => '/home/user',
-                               'backend' => 'bar',
-                               'lastLogin' => 0,
-                               'displayname' => null,
-                               'quota' => null,
-                               'subadmin' => [],
-                               'email' => null,
-                               'isRestoreDisabled' => false,
-                               'isAvatarAvailable' => true,
-                       ),
-                       Http::STATUS_CREATED
-               );
-               $response = $controller->create('foo', 'password');
-               $this->assertEquals($expectedResponse, $response);
-       }
-
        public function testCreateSuccessfulWithGroupAdmin() {
                $controller = $this->getController(true);
 
@@ -1026,16 +938,12 @@ class UsersControllerTest extends \Test\TestCase {
 
        public function testCreateSuccessfulWithGroupSubAdmin() {
                $controller = $this->getController(false);
-               $user = $this->getMockBuilder('\OC\User\User')
-                       ->disableOriginalConstructor()->getMock();
+               $user = $this->createMock(IUser::class);
                $this->userSession
                        ->expects($this->once())
                        ->method('getUser')
                        ->will($this->returnValue($user));
-               $user = $this->getMockBuilder('\OC\User\User')
-                       ->disableOriginalConstructor()->getMock();
-               $newUser = $this->getMockBuilder('\OC\User\User')
-                       ->disableOriginalConstructor()->getMock();
+               $newUser = $this->createMock(IUser::class);
                $newUser
                        ->method('getHome')
                        ->will($this->returnValue('/home/user'));
@@ -1049,8 +957,7 @@ class UsersControllerTest extends \Test\TestCase {
                        ->expects($this->once())
                        ->method('getBackendClassName')
                        ->will($this->returnValue('bar'));
-               $subGroup1 = $this->getMockBuilder('\OCP\IGroup')
-                       ->disableOriginalConstructor()->getMock();
+               $subGroup1 = $this->createMock(IGroup::class);
                $subGroup1
                        ->expects($this->any())
                        ->method('getGID')
@@ -1063,16 +970,6 @@ class UsersControllerTest extends \Test\TestCase {
                        ->expects($this->once())
                        ->method('createUser')
                        ->will($this->returnValue($newUser));
-               $this->groupManager
-                       ->expects($this->at(0))
-                       ->method('get')
-                       ->with('SubGroup1')
-                       ->will($this->returnValue($subGroup1));
-               $this->groupManager
-                       ->expects($this->at(4))
-                       ->method('get')
-                       ->with('SubGroup1')
-                       ->will($this->returnValue($subGroup1));
                $this->groupManager
                        ->expects($this->once())
                        ->method('getUserGroupIds')
@@ -1084,21 +981,28 @@ class UsersControllerTest extends \Test\TestCase {
                        ->with($newUser)
                        ->will($this->onConsecutiveCalls(['SubGroup1']));
 
-               $subadmin = $this->getMockBuilder('\OC\SubAdmin')
-                       ->disableOriginalConstructor()
-                       ->getMock();
-               $subadmin->expects($this->at(1))
+               $subadmin = $this->createMock(\OC\SubAdmin::class);
+               $subadmin->expects($this->atLeastOnce())
                        ->method('getSubAdminsGroups')
                        ->with($user)
-                       ->will($this->returnValue([$subGroup1]));
-               $subadmin->expects($this->at(2))
-                       ->method('getSubAdminsGroups')
-                       ->with($newUser)
-                       ->will($this->returnValue([]));
+                       ->willReturnMap([
+                               [$user, [$subGroup1]],
+                               [$newUser, []],
+                       ]);
+               $subadmin->expects($this->atLeastOnce())
+                       ->method('isSubAdminofGroup')
+                       ->willReturnMap([
+                               [$user, $subGroup1, true],
+                       ]);
                $this->groupManager
                        ->expects($this->any())
                        ->method('getSubAdmin')
                        ->will($this->returnValue($subadmin));
+               $this->groupManager->expects($this->atLeastOnce())
+                       ->method('get')
+                       ->willReturnMap([
+                               ['SubGroup1', $subGroup1],
+                       ]);
 
                $expectedResponse = new DataResponse(
                        array(
@@ -1137,16 +1041,36 @@ class UsersControllerTest extends \Test\TestCase {
                $this->assertEquals($expectedResponse, $response);
        }
 
+       public function testCreateUnsuccessfulSubAdminNoGroup() {
+               $controller = $this->getController(false);
+               $user = $this->createMock(IUser::class);
+               $user->expects($this->any())
+                       ->method('getUID')
+                       ->will($this->returnValue('username'));
+               $this->userSession->expects($this->once())
+                       ->method('getUser')
+                       ->will($this->returnValue($user));
+
+               $this->userManager->expects($this->never())
+                       ->method('createUser');
+
+               $expectedResponse = new DataResponse(
+                       [
+                               'message' => 'No valid group selected'
+                       ],
+                       Http::STATUS_FORBIDDEN
+               );
+               $response = $controller->create('foo', 'password', []);
+               $this->assertEquals($expectedResponse, $response);
+       }
+
        public function testCreateUnsuccessfulSubAdmin() {
                $controller = $this->getController(false);
-               $user = $this->getMockBuilder('\OC\User\User')
-                       ->disableOriginalConstructor()->getMock();
-               $user
-                       ->expects($this->any())
+               $user = $this->createMock(IUser::class);
+               $user->expects($this->any())
                        ->method('getUID')
                        ->will($this->returnValue('username'));
-               $this->userSession
-                       ->expects($this->once())
+               $this->userSession->expects($this->once())
                        ->method('getUser')
                        ->will($this->returnValue($user));
 
@@ -1154,29 +1078,24 @@ class UsersControllerTest extends \Test\TestCase {
                        ->method('createUser')
                        ->will($this->throwException(new \Exception()));
 
-               $subgroup1 = $this->getMockBuilder('\OCP\IGroup')
-                       ->disableOriginalConstructor()
-                       ->getMock();
-               $subgroup1->expects($this->once())
-                       ->method('getGID')
-                       ->will($this->returnValue('SubGroup1'));
-               $subgroup2 = $this->getMockBuilder('\OCP\IGroup')
-                       ->disableOriginalConstructor()
-                       ->getMock();
-               $subgroup2->expects($this->once())
-                       ->method('getGID')
-                       ->will($this->returnValue('SubGroup2'));
-               $subadmin = $this->getMockBuilder('\OC\SubAdmin')
-                       ->disableOriginalConstructor()
-                       ->getMock();
-               $subadmin->expects($this->once())
-                       ->method('getSubAdminsGroups')
-                       ->with($user)
-                       ->will($this->returnValue([$subgroup1, $subgroup2]));
-               $this->groupManager
-                       ->expects($this->any())
+               $subgroup1 = $this->createMock(IGroup::class);
+               $subgroup2 = $this->createMock(IGroup::class);
+               $subadmin = $this->createMock(\OC\SubAdmin::class);
+               $subadmin->expects($this->atLeastOnce())
+                       ->method('isSubAdminofGroup')
+                       ->willReturnMap([
+                               [$user, $subgroup1, true],
+                               [$user, $subgroup2, true],
+                       ]);
+               $this->groupManager->expects($this->any())
                        ->method('getSubAdmin')
-                       ->will($this->returnValue($subadmin));
+                       ->willReturn($subadmin);
+               $this->groupManager->expects($this->atLeastOnce())
+                       ->method('get')
+                       ->willReturnMap([
+                               ['SubGroup1', $subgroup1],
+                               ['SubGroup2', $subgroup2],
+                       ]);
 
                $expectedResponse = new DataResponse(
                        [
@@ -1184,7 +1103,7 @@ class UsersControllerTest extends \Test\TestCase {
                        ],
                        Http::STATUS_FORBIDDEN
                );
-               $response = $controller->create('foo', 'password', array());
+               $response = $controller->create('foo', 'password', array('SubGroup1', 'SubGroup2'));
                $this->assertEquals($expectedResponse, $response);
        }