Commit 0f7634ea authored by Lukas Reschke's avatar Lukas Reschke
Browse files

Switch to a factory and add unit tests

parent 7e7dd92f
......@@ -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;
......
......@@ -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',
......
<?php
/**
* @author Lukas Reschke
* @copyright 2015 Lukas Reschke lukas@owncloud.com
*
* This file is licensed under the Affero General Public License version 3 or
* later.
* See the COPYING-README file.
*/
namespace OC\Settings\Factory;
/**
* @package OC\Settings\Factory
*/
class SubAdminFactory {
/**
* Get the groups $uid is SubAdmin of
* @param string $uid
* @return array Array of groups that $uid is subadmin of
*/
function getSubAdminsOfGroups($uid) {
return \OC_SubAdmin::getSubAdminsGroups($uid);
}
/**
* Whether the $group is accessible to $uid as subadmin
* @param string $uid
* @param string $group
* @return bool
*/
function isGroupAccessible($uid, $group) {
return \OC_SubAdmin::isGroupAccessible($uid, $group);
}
/**
* Whether $uid is accessible to $subAdmin
* @param string $subAdmin
* @param string $uid
* @return bool
*/
function isUserAccessible($subAdmin, $uid) {
return \OC_SubAdmin::isUserAccessible($subAdmin, $uid);
}
}
......@@ -33,6 +33,8 @@ class UsersControllerTest extends \Test\TestCase {
->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())