From 377fdf3860e317b68392c678fa25e081251baecc Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 19 Jan 2017 15:02:46 +0100 Subject: Skip null groups in group manager (#26871) (#26956) * Skip null groups in group manager (#26871) * Skip null groups in group manager * Also skip null groups in group manager's search function * Add more group null checks in sharing code * Add unit tests for null group safety in group manager * Add unit tests for sharing code null group checks * Added tests for null groups handling in sharing code * Ignore moveShare optional repair in mount provider In some cases, data is inconsistent in the oc_share table due to legacy data. The mount provider might attempt to make it consistent but if the target group does not exist any more it cannot work. In such case we simply ignore the exception as it is not critical. Keeping the exception would break user accounts as they would be unable to use their filesystem. * Adjust null group handing + tests * Fix new group manager tests Signed-off-by: Morris Jobke --- tests/lib/Group/ManagerTest.php | 56 ++++++++++++++++++++ tests/lib/Share20/DefaultShareProviderTest.php | 42 +++++++++++++++ tests/lib/Share20/ManagerTest.php | 73 ++++++++++++++++++++++++++ 3 files changed, 171 insertions(+) (limited to 'tests') diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index 1a7ced5f1ba..c57ef0b34cb 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -302,6 +302,32 @@ class ManagerTest extends \Test\TestCase { $this->assertEquals('group12', $group12->getGID()); } + public function testSearchResultExistsButGroupDoesNot() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend + */ + $backend = $this->createMock('\OC\Group\Database'); + $backend->expects($this->once()) + ->method('getGroups') + ->with('1') + ->will($this->returnValue(['group1'])); + $backend->expects($this->once()) + ->method('groupExists') + ->with('group1') + ->will($this->returnValue(false)); + + /** + * @var \OC\User\Manager $userManager + */ + $userManager = $this->createMock('\OC\User\Manager'); + + $manager = new \OC\Group\Manager($userManager); + $manager->addBackend($backend); + + $groups = $manager->search('1'); + $this->assertEmpty($groups); + } + public function testGetUserGroups() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend @@ -351,6 +377,36 @@ class ManagerTest extends \Test\TestCase { } } + public function testGetUserGroupsWithDeletedGroup() { + /** + * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend + */ + $backend = $this->createMock('\OC\Group\Database'); + $backend->expects($this->once()) + ->method('getUserGroups') + ->with('user1') + ->will($this->returnValue(array('group1'))); + $backend->expects($this->any()) + ->method('groupExists') + ->with('group1') + ->will($this->returnValue(false)); + + /** + * @var \OC\User\Manager $userManager + */ + $userManager = $this->createMock('\OC\User\Manager'); + $userBackend = $this->createMock('\OC_User_Backend'); + $manager = new \OC\Group\Manager($userManager); + $manager->addBackend($backend); + + /** @var \OC\User\User $user */ + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn('user1'); + + $groups = $manager->getUserGroups($user); + $this->assertEmpty($groups); + } + public function testInGroup() { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend diff --git a/tests/lib/Share20/DefaultShareProviderTest.php b/tests/lib/Share20/DefaultShareProviderTest.php index 5870da8d218..a1e78313558 100644 --- a/tests/lib/Share20/DefaultShareProviderTest.php +++ b/tests/lib/Share20/DefaultShareProviderTest.php @@ -1524,6 +1524,48 @@ class DefaultShareProviderTest extends \Test\TestCase { $this->provider->deleteFromSelf($share, 'user2'); } + /** + * @expectedException \OC\Share20\Exception\ProviderException + * @expectedExceptionMessage Group "group" does not exist + */ + public function testDeleteFromSelfGroupDoesNotExist() { + $qb = $this->dbConn->getQueryBuilder(); + $stmt = $qb->insert('share') + ->values([ + 'share_type' => $qb->expr()->literal(\OCP\Share::SHARE_TYPE_GROUP), + 'share_with' => $qb->expr()->literal('group'), + 'uid_owner' => $qb->expr()->literal('user1'), + 'uid_initiator' => $qb->expr()->literal('user1'), + 'item_type' => $qb->expr()->literal('file'), + 'file_source' => $qb->expr()->literal(1), + 'file_target' => $qb->expr()->literal('myTarget1'), + 'permissions' => $qb->expr()->literal(2) + ])->execute(); + $this->assertEquals(1, $stmt); + $id = $qb->getLastInsertId(); + + $user1 = $this->getMock('\OCP\IUser'); + $user1->method('getUID')->willReturn('user1'); + $user2 = $this->getMock('\OCP\IUser'); + $user2->method('getUID')->willReturn('user2'); + $this->userManager->method('get')->will($this->returnValueMap([ + ['user1', $user1], + ['user2', $user2], + ])); + + $this->groupManager->method('get')->with('group')->willReturn(null); + + $file = $this->getMock('\OCP\Files\File'); + $file->method('getId')->willReturn(1); + + $this->rootFolder->method('getUserFolder')->with('user1')->will($this->returnSelf()); + $this->rootFolder->method('getById')->with(1)->willReturn([$file]); + + $share = $this->provider->getShareById($id); + + $this->provider->deleteFromSelf($share, 'user2'); + } + public function testDeleteFromSelfUser() { $qb = $this->dbConn->getQueryBuilder(); $stmt = $qb->insert('share') diff --git a/tests/lib/Share20/ManagerTest.php b/tests/lib/Share20/ManagerTest.php index 7b01a8f9e70..25f406c1405 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -1139,6 +1139,39 @@ class ManagerTest extends \Test\TestCase { self::invokePrivate($this->manager, 'userCreateChecks', [$share]); } + public function testUserCreateChecksIdenticalPathSharedViaDeletedGroup() { + $share = $this->manager->newShare(); + + $sharedWith = $this->getMock('\OCP\IUser'); + $sharedWith->method('getUID')->willReturn('sharedWith'); + + $this->userManager->method('get')->with('sharedWith')->willReturn($sharedWith); + + $path = $this->getMock('\OCP\Files\Node'); + + $share->setSharedWith('sharedWith') + ->setNode($path) + ->setShareOwner('shareOwner') + ->setProviderId('foo') + ->setId('bar'); + + $share2 = $this->manager->newShare(); + $share2->setShareType(\OCP\Share::SHARE_TYPE_GROUP) + ->setShareOwner('shareOwner2') + ->setProviderId('foo') + ->setId('baz') + ->setSharedWith('group'); + + $this->groupManager->method('get')->with('group')->willReturn(null); + + $this->defaultProvider + ->method('getSharesByPath') + ->with($path) + ->willReturn([$share2]); + + $this->assertNull($this->invokePrivate($this->manager, 'userCreateChecks', [$share])); + } + public function testUserCreateChecksIdenticalPathNotSharedWithUser() { $share = $this->manager->newShare(); $sharedWith = $this->createMock(IUser::class); @@ -1216,6 +1249,29 @@ class ManagerTest extends \Test\TestCase { self::invokePrivate($this->manager, 'groupCreateChecks', [$share]); } + /** + * @expectedException Exception + * @expectedExceptionMessage Only sharing within your own groups is allowed + */ + public function testGroupCreateChecksShareWithGroupMembersOnlyNullGroup() { + $share = $this->manager->newShare(); + + $user = $this->getMock('\OCP\IUser'); + $share->setSharedBy('user')->setSharedWith('group'); + + $this->groupManager->method('get')->with('group')->willReturn(null); + $this->userManager->method('get')->with('user')->willReturn($user); + + $this->config + ->method('getAppValue') + ->will($this->returnValueMap([ + ['core', 'shareapi_only_share_with_group_members', 'no', 'yes'], + ['core', 'shareapi_allow_group_sharing', 'yes', 'yes'], + ])); + + $this->assertNull($this->invokePrivate($this->manager, 'groupCreateChecks', [$share])); + } + public function testGroupCreateChecksShareWithGroupMembersOnlyInGroup() { $share = $this->manager->newShare(); @@ -2666,6 +2722,23 @@ class ManagerTest extends \Test\TestCase { $this->manager->moveShare($share, 'recipient'); } + /** + * @expectedException \InvalidArgumentException + * @expectedExceptionMessage Group "shareWith" does not exist + */ + public function testMoveShareGroupNull() { + $share = $this->manager->newShare(); + $share->setShareType(\OCP\Share::SHARE_TYPE_GROUP); + $share->setSharedWith('shareWith'); + + $recipient = $this->getMock('\OCP\IUser'); + + $this->groupManager->method('get')->with('shareWith')->willReturn(null); + $this->userManager->method('get')->with('recipient')->willReturn($recipient); + + $this->manager->moveShare($share, 'recipient'); + } + public function testMoveShareGroup() { $share = $this->manager->newShare(); $share->setShareType(\OCP\Share::SHARE_TYPE_GROUP) -- cgit v1.2.3 From bd97b7d13049abbc74668b5455857e745a45f75e Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 17 Mar 2017 10:23:04 +0100 Subject: Use DI Signed-off-by: Joas Schilling --- apps/files_sharing/lib/MountProvider.php | 2 +- lib/private/Group/Manager.php | 24 ++++++++----- lib/private/Server.php | 2 +- tests/lib/Group/ManagerTest.php | 58 +++++++++++++++++--------------- 4 files changed, 49 insertions(+), 37 deletions(-) (limited to 'tests') diff --git a/apps/files_sharing/lib/MountProvider.php b/apps/files_sharing/lib/MountProvider.php index 93f85a888b0..5e2426ecc76 100644 --- a/apps/files_sharing/lib/MountProvider.php +++ b/apps/files_sharing/lib/MountProvider.php @@ -182,7 +182,7 @@ class MountProvider implements IMountProvider { // such issue can usually happen when dealing with // null groups which usually appear with group backend // caching inconsistencies - \OC::$server->getLogger()->debug( + $this->logger->debug( 'Could not adjust share target for share ' . $share->getId() . ' to make it consistent: ' . $e->getMessage(), ['app' => 'files_sharing'] ); diff --git a/lib/private/Group/Manager.php b/lib/private/Group/Manager.php index 8b18abd5e95..40009dbfd80 100644 --- a/lib/private/Group/Manager.php +++ b/lib/private/Group/Manager.php @@ -37,7 +37,10 @@ namespace OC\Group; use OC\Hooks\PublicEmitter; use OCP\GroupInterface; +use OCP\IGroup; use OCP\IGroupManager; +use OCP\ILogger; +use OCP\IUser; /** * Class Manager @@ -78,11 +81,16 @@ class Manager extends PublicEmitter implements IGroupManager { /** @var \OC\SubAdmin */ private $subAdmin = null; + /** @var ILogger */ + private $logger; + /** * @param \OC\User\Manager $userManager + * @param ILogger $logger */ - public function __construct(\OC\User\Manager $userManager) { + public function __construct(\OC\User\Manager $userManager, ILogger $logger) { $this->userManager = $userManager; + $this->logger = $logger; $cachedGroups = & $this->cachedGroups; $cachedUserGroups = & $this->cachedUserGroups; $this->listen('\OC\Group', 'postDelete', function ($group) use (&$cachedGroups, &$cachedUserGroups) { @@ -186,7 +194,7 @@ class Manager extends PublicEmitter implements IGroupManager { * @return bool */ public function groupExists($gid) { - return !is_null($this->get($gid)); + return $this->get($gid) instanceof IGroup; } /** @@ -194,7 +202,7 @@ class Manager extends PublicEmitter implements IGroupManager { * @return \OC\Group\Group */ public function createGroup($gid) { - if ($gid === '' || is_null($gid)) { + if ($gid === '' || $gid === null) { return false; } else if ($group = $this->get($gid)) { return $group; @@ -224,10 +232,10 @@ class Manager extends PublicEmitter implements IGroupManager { $groupIds = $backend->getGroups($search, $limit, $offset); foreach ($groupIds as $groupId) { $aGroup = $this->get($groupId); - if (!is_null($aGroup)) { + if ($aGroup instanceof IGroup) { $groups[$groupId] = $aGroup; } else { - \OC::$server->getLogger()->debug('Group "' . $groupId . '" was returned by search but not found through direct access', array('app' => 'core')); + $this->logger->debug('Group "' . $groupId . '" was returned by search but not found through direct access', ['app' => 'core']); } } if (!is_null($limit) and $limit <= 0) { @@ -242,7 +250,7 @@ class Manager extends PublicEmitter implements IGroupManager { * @return \OC\Group\Group[] */ public function getUserGroups($user) { - if (is_null($user)) { + if (!$user instanceof IUser) { return []; } return $this->getUserIdGroups($user->getUID()); @@ -262,10 +270,10 @@ class Manager extends PublicEmitter implements IGroupManager { if (is_array($groupIds)) { foreach ($groupIds as $groupId) { $aGroup = $this->get($groupId); - if (!is_null($aGroup)) { + if ($aGroup instanceof IGroup) { $groups[$groupId] = $aGroup; } else { - \OC::$server->getLogger()->debug('User "' . $uid . '" belongs to deleted group: "' . $groupId . '"', array('app' => 'core')); + $this->logger->debug('User "' . $uid . '" belongs to deleted group: "' . $groupId . '"', ['app' => 'core']); } } } diff --git a/lib/private/Server.php b/lib/private/Server.php index bcbe23b0181..2ee612a0adc 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -227,7 +227,7 @@ class Server extends ServerContainer implements IServerContainer { return new \OC\User\Manager($config); }); $this->registerService('GroupManager', function (Server $c) { - $groupManager = new \OC\Group\Manager($this->getUserManager()); + $groupManager = new \OC\Group\Manager($this->getUserManager(), $this->getLogger()); $groupManager->listen('\OC\Group', 'preCreate', function ($gid) { \OC_Hook::emit('OC_Group', 'pre_createGroup', array('run' => true, 'gid' => $gid)); }); diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index c57ef0b34cb..1fb8119b922 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -23,17 +23,21 @@ namespace Test\Group; use OC\User\Manager; +use OCP\ILogger; use OCP\IUser; use OCP\GroupInterface; class ManagerTest extends \Test\TestCase { /** @var Manager|\PHPUnit_Framework_MockObject_MockObject $userManager */ protected $userManager; + /** @var ILogger|\PHPUnit_Framework_MockObject_MockObject $userManager */ + protected $logger; protected function setUp() { parent::setUp(); $this->userManager = $this->createMock(Manager::class); + $this->logger = $this->createMock(ILogger::class); } private function getTestUser($userId) { @@ -91,7 +95,7 @@ class ManagerTest extends \Test\TestCase { ->with('group1') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $group = $manager->get('group1'); @@ -100,7 +104,7 @@ class ManagerTest extends \Test\TestCase { } public function testGetNoBackend() { - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $this->assertNull($manager->get('group1')); } @@ -115,7 +119,7 @@ class ManagerTest extends \Test\TestCase { ->with('group1') ->will($this->returnValue(false)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $this->assertNull($manager->get('group1')); @@ -125,7 +129,7 @@ class ManagerTest extends \Test\TestCase { $backend = new \Test\Util\Group\Dummy(); $backend->createGroup('group1'); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $group = $manager->get('group1'); @@ -152,7 +156,7 @@ class ManagerTest extends \Test\TestCase { ->with('group1') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -179,7 +183,7 @@ class ManagerTest extends \Test\TestCase { $backendGroupCreated = true; }));; - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $group = $manager->createGroup('group1'); @@ -198,7 +202,7 @@ class ManagerTest extends \Test\TestCase { $backend->expects($this->never()) ->method('createGroup'); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $group = $manager->createGroup('group1'); @@ -219,7 +223,7 @@ class ManagerTest extends \Test\TestCase { ->with('group1') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $groups = $manager->search('1'); @@ -253,7 +257,7 @@ class ManagerTest extends \Test\TestCase { ->method('groupExists') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -290,7 +294,7 @@ class ManagerTest extends \Test\TestCase { ->method('groupExists') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -321,7 +325,7 @@ class ManagerTest extends \Test\TestCase { */ $userManager = $this->createMock('\OC\User\Manager'); - $manager = new \OC\Group\Manager($userManager); + $manager = new \OC\Group\Manager($userManager, $this->logger); $manager->addBackend($backend); $groups = $manager->search('1'); @@ -342,7 +346,7 @@ class ManagerTest extends \Test\TestCase { ->with('group1') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $groups = $manager->getUserGroups($this->getTestUser('user1')); @@ -396,7 +400,7 @@ class ManagerTest extends \Test\TestCase { */ $userManager = $this->createMock('\OC\User\Manager'); $userBackend = $this->createMock('\OC_User_Backend'); - $manager = new \OC\Group\Manager($userManager); + $manager = new \OC\Group\Manager($userManager, $this->logger); $manager->addBackend($backend); /** @var \OC\User\User $user */ @@ -420,7 +424,7 @@ class ManagerTest extends \Test\TestCase { ->method('groupExists') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $this->assertTrue($manager->isInGroup('user1', 'group1')); @@ -439,7 +443,7 @@ class ManagerTest extends \Test\TestCase { ->method('groupExists') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $this->assertTrue($manager->isAdmin('user1')); @@ -458,7 +462,7 @@ class ManagerTest extends \Test\TestCase { ->method('groupExists') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $this->assertFalse($manager->isAdmin('user1')); @@ -489,7 +493,7 @@ class ManagerTest extends \Test\TestCase { ->method('groupExists') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -552,7 +556,7 @@ class ManagerTest extends \Test\TestCase { } })); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', 'user3'); @@ -616,7 +620,7 @@ class ManagerTest extends \Test\TestCase { } })); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', 'user3', 1); @@ -684,7 +688,7 @@ class ManagerTest extends \Test\TestCase { } })); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', 'user3', 1, 1); @@ -728,7 +732,7 @@ class ManagerTest extends \Test\TestCase { } })); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', ''); @@ -771,7 +775,7 @@ class ManagerTest extends \Test\TestCase { } })); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', '', 1); @@ -814,7 +818,7 @@ class ManagerTest extends \Test\TestCase { } })); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', '', 1, 1); @@ -842,7 +846,7 @@ class ManagerTest extends \Test\TestCase { ->with('group1') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); // prime cache @@ -885,7 +889,7 @@ class ManagerTest extends \Test\TestCase { ->method('removeFromGroup') ->will($this->returnValue(true)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); // prime cache @@ -915,7 +919,7 @@ class ManagerTest extends \Test\TestCase { ->with('user1') ->will($this->returnValue(null)); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $groups = $manager->getUserIdGroups('user1'); @@ -941,7 +945,7 @@ class ManagerTest extends \Test\TestCase { ['group2', ['gid' => 'group2']], ])); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); // group with display name -- cgit v1.2.3 From ebabf814733a136452f614dfd33404e16f9f1d42 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 17 Mar 2017 10:34:25 +0100 Subject: Clean up the test Signed-off-by: Joas Schilling --- tests/lib/Group/ManagerTest.php | 133 ++++++++++++++++------------------------ 1 file changed, 52 insertions(+), 81 deletions(-) (limited to 'tests') diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index 1fb8119b922..bd73e8f4052 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -22,12 +22,16 @@ */ namespace Test\Group; +use OC\Group\Database; use OC\User\Manager; use OCP\ILogger; use OCP\IUser; use OCP\GroupInterface; +use OCP\IUserManager; +use Test\TestCase; +use Test\Util\Group\Dummy; -class ManagerTest extends \Test\TestCase { +class ManagerTest extends TestCase { /** @var Manager|\PHPUnit_Framework_MockObject_MockObject $userManager */ protected $userManager; /** @var ILogger|\PHPUnit_Framework_MockObject_MockObject $userManager */ @@ -51,8 +55,12 @@ class ManagerTest extends \Test\TestCase { return $mockUser; } + /** + * @param null|int $implementedActions + * @return \PHPUnit_Framework_MockObject_MockObject + */ private function getTestBackend($implementedActions = null) { - if (is_null($implementedActions)) { + if ($implementedActions === null) { $implementedActions = GroupInterface::ADD_TO_GROUP | GroupInterface::REMOVE_FROM_GOUP | @@ -62,7 +70,7 @@ class ManagerTest extends \Test\TestCase { } // need to declare it this way due to optional methods // thanks to the implementsActions logic - $backend = $this->getMockBuilder(\OCP\GroupInterface::class) + $backend = $this->getMockBuilder(GroupInterface::class) ->disableOriginalConstructor() ->setMethods([ 'getGroupDetails', @@ -126,7 +134,7 @@ class ManagerTest extends \Test\TestCase { } public function testGetDeleted() { - $backend = new \Test\Util\Group\Dummy(); + $backend = new Dummy(); $backend->createGroup('group1'); $manager = new \OC\Group\Manager($this->userManager, $this->logger); @@ -166,9 +174,7 @@ class ManagerTest extends \Test\TestCase { } public function testCreate() { - /** - * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend - */ + /**@var \PHPUnit_Framework_MockObject_MockObject|\OC\Group\Backend $backend */ $backendGroupCreated = false; $backend = $this->getTestBackend(); $backend->expects($this->any()) @@ -181,7 +187,7 @@ class ManagerTest extends \Test\TestCase { ->method('createGroup') ->will($this->returnCallback(function () use (&$backendGroupCreated) { $backendGroupCreated = true; - }));; + })); $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); @@ -191,9 +197,7 @@ class ManagerTest extends \Test\TestCase { } public function testCreateExists() { - /** - * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend - */ + /** @var \PHPUnit_Framework_MockObject_MockObject|\OC\Group\Backend $backend */ $backend = $this->getTestBackend(); $backend->expects($this->any()) ->method('groupExists') @@ -227,7 +231,7 @@ class ManagerTest extends \Test\TestCase { $manager->addBackend($backend); $groups = $manager->search('1'); - $this->assertEquals(1, count($groups)); + $this->assertCount(1, $groups); $group1 = reset($groups); $this->assertEquals('group1', $group1->getGID()); } @@ -262,7 +266,7 @@ class ManagerTest extends \Test\TestCase { $manager->addBackend($backend2); $groups = $manager->search('1'); - $this->assertEquals(2, count($groups)); + $this->assertCount(2, $groups); $group1 = reset($groups); $group12 = next($groups); $this->assertEquals('group1', $group1->getGID()); @@ -299,7 +303,7 @@ class ManagerTest extends \Test\TestCase { $manager->addBackend($backend2); $groups = $manager->search('1', 2, 1); - $this->assertEquals(2, count($groups)); + $this->assertCount(2, $groups); $group1 = reset($groups); $group12 = next($groups); $this->assertEquals('group1', $group1->getGID()); @@ -307,10 +311,8 @@ class ManagerTest extends \Test\TestCase { } public function testSearchResultExistsButGroupDoesNot() { - /** - * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend - */ - $backend = $this->createMock('\OC\Group\Database'); + /** @var \PHPUnit_Framework_MockObject_MockObject|\OC\Group\Backend $backend */ + $backend = $this->createMock(Database::class); $backend->expects($this->once()) ->method('getGroups') ->with('1') @@ -320,10 +322,8 @@ class ManagerTest extends \Test\TestCase { ->with('group1') ->will($this->returnValue(false)); - /** - * @var \OC\User\Manager $userManager - */ - $userManager = $this->createMock('\OC\User\Manager'); + /** @var \OC\User\Manager $userManager */ + $userManager = $this->createMock(Manager::class); $manager = new \OC\Group\Manager($userManager, $this->logger); $manager->addBackend($backend); @@ -350,14 +350,14 @@ class ManagerTest extends \Test\TestCase { $manager->addBackend($backend); $groups = $manager->getUserGroups($this->getTestUser('user1')); - $this->assertEquals(1, count($groups)); + $this->assertCount(1, $groups); $group1 = reset($groups); $this->assertEquals('group1', $group1->getGID()); } public function testGetUserGroupIds() { /** @var \PHPUnit_Framework_MockObject_MockObject|\OC\Group\Manager $manager */ - $manager = $this->getMockBuilder('OC\Group\Manager') + $manager = $this->getMockBuilder(\OC\Group\Manager::class) ->disableOriginalConstructor() ->setMethods(['getUserGroups']) ->getMock(); @@ -368,13 +368,11 @@ class ManagerTest extends \Test\TestCase { 'abc' => 'abc', ]); - /** @var \OC\User\User $user */ - $user = $this->getMockBuilder('OC\User\User') - ->disableOriginalConstructor() - ->getMock(); + /** @var \OC\User\User|\PHPUnit_Framework_MockObject_MockObject $user */ + $user = $this->createMock(IUser::class); $groups = $manager->getUserGroupIds($user); - $this->assertEquals(2, count($groups)); + $this->assertCount(2, $groups); foreach ($groups as $group) { $this->assertInternalType('string', $group); @@ -385,7 +383,7 @@ class ManagerTest extends \Test\TestCase { /** * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend */ - $backend = $this->createMock('\OC\Group\Database'); + $backend = $this->createMock(Database::class); $backend->expects($this->once()) ->method('getUserGroups') ->with('user1') @@ -395,17 +393,14 @@ class ManagerTest extends \Test\TestCase { ->with('group1') ->will($this->returnValue(false)); - /** - * @var \OC\User\Manager $userManager - */ - $userManager = $this->createMock('\OC\User\Manager'); - $userBackend = $this->createMock('\OC_User_Backend'); - $manager = new \OC\Group\Manager($userManager, $this->logger); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); - /** @var \OC\User\User $user */ + /** @var \OC\User\User|\PHPUnit_Framework_MockObject_MockObject $user */ $user = $this->createMock(IUser::class); - $user->method('getUID')->willReturn('user1'); + $user->expects($this->atLeastOnce()) + ->method('getUID') + ->willReturn('user1'); $groups = $manager->getUserGroups($user); $this->assertEmpty($groups); @@ -498,7 +493,7 @@ class ManagerTest extends \Test\TestCase { $manager->addBackend($backend2); $groups = $manager->getUserGroups($this->getTestUser('user1')); - $this->assertEquals(2, count($groups)); + $this->assertCount(2, $groups); $group1 = reset($groups); $group2 = next($groups); $this->assertEquals('group1', $group1->getGID()); @@ -528,14 +523,10 @@ class ManagerTest extends \Test\TestCase { } })); - $userBackend = $this->getMockBuilder('\OC\User\Backend') - ->disableOriginalConstructor() - ->getMock(); - $this->userManager->expects($this->any()) ->method('searchDisplayName') ->with('user3') - ->will($this->returnCallback(function($search, $limit, $offset) use ($userBackend) { + ->will($this->returnCallback(function($search, $limit, $offset) { switch($offset) { case 0 : return ['user3' => $this->getTestUser('user3'), 'user33' => $this->getTestUser('user33')]; @@ -545,7 +536,7 @@ class ManagerTest extends \Test\TestCase { })); $this->userManager->expects($this->any()) ->method('get') - ->will($this->returnCallback(function($uid) use ($userBackend) { + ->will($this->returnCallback(function($uid) { switch($uid) { case 'user1' : return $this->getTestUser('user1'); case 'user2' : return $this->getTestUser('user2'); @@ -560,7 +551,7 @@ class ManagerTest extends \Test\TestCase { $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', 'user3'); - $this->assertEquals(1, count($users)); + $this->assertCount(1, $users); $this->assertFalse(isset($users['user1'])); $this->assertFalse(isset($users['user2'])); $this->assertFalse(isset($users['user3'])); @@ -591,14 +582,10 @@ class ManagerTest extends \Test\TestCase { } })); - $userBackend = $this->getMockBuilder('\OC\User\Backend') - ->disableOriginalConstructor() - ->getMock(); - $this->userManager->expects($this->any()) ->method('searchDisplayName') ->with('user3') - ->will($this->returnCallback(function($search, $limit, $offset) use ($userBackend) { + ->will($this->returnCallback(function($search, $limit, $offset) { switch($offset) { case 0 : return ['user3' => $this->getTestUser('user3'), 'user33' => $this->getTestUser('user33')]; @@ -608,7 +595,7 @@ class ManagerTest extends \Test\TestCase { })); $this->userManager->expects($this->any()) ->method('get') - ->will($this->returnCallback(function($uid) use ($userBackend) { + ->will($this->returnCallback(function($uid) { switch($uid) { case 'user1' : return $this->getTestUser('user1'); case 'user2' : return $this->getTestUser('user2'); @@ -624,7 +611,7 @@ class ManagerTest extends \Test\TestCase { $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', 'user3', 1); - $this->assertEquals(1, count($users)); + $this->assertCount(1, $users); $this->assertFalse(isset($users['user1'])); $this->assertFalse(isset($users['user2'])); $this->assertFalse(isset($users['user3'])); @@ -656,14 +643,10 @@ class ManagerTest extends \Test\TestCase { } })); - $userBackend = $this->getMockBuilder('\OC\User\Backend') - ->disableOriginalConstructor() - ->getMock(); - $this->userManager->expects($this->any()) ->method('searchDisplayName') ->with('user3') - ->will($this->returnCallback(function($search, $limit, $offset) use ($userBackend) { + ->will($this->returnCallback(function($search, $limit, $offset) { switch($offset) { case 0 : return [ @@ -676,7 +659,7 @@ class ManagerTest extends \Test\TestCase { })); $this->userManager->expects($this->any()) ->method('get') - ->will($this->returnCallback(function($uid) use ($userBackend) { + ->will($this->returnCallback(function($uid) { switch($uid) { case 'user1' : return $this->getTestUser('user1'); case 'user2' : return $this->getTestUser('user2'); @@ -692,7 +675,7 @@ class ManagerTest extends \Test\TestCase { $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', 'user3', 1, 1); - $this->assertEquals(1, count($users)); + $this->assertCount(1, $users); $this->assertFalse(isset($users['user1'])); $this->assertFalse(isset($users['user2'])); $this->assertFalse(isset($users['user3'])); @@ -715,13 +698,9 @@ class ManagerTest extends \Test\TestCase { ->with('testgroup', '', -1, 0) ->will($this->returnValue(array('user2', 'user33'))); - $userBackend = $this->getMockBuilder('\OC\User\Backend') - ->disableOriginalConstructor() - ->getMock(); - $this->userManager->expects($this->any()) ->method('get') - ->will($this->returnCallback(function($uid) use ($userBackend) { + ->will($this->returnCallback(function($uid) { switch($uid) { case 'user1' : return $this->getTestUser('user1'); case 'user2' : return $this->getTestUser('user2'); @@ -736,7 +715,7 @@ class ManagerTest extends \Test\TestCase { $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', ''); - $this->assertEquals(2, count($users)); + $this->assertCount(2, $users); $this->assertFalse(isset($users['user1'])); $this->assertTrue(isset($users['user2'])); $this->assertFalse(isset($users['user3'])); @@ -758,13 +737,9 @@ class ManagerTest extends \Test\TestCase { ->with('testgroup', '', 1, 0) ->will($this->returnValue(array('user2'))); - $userBackend = $this->getMockBuilder('\OC\User\Backend') - ->disableOriginalConstructor() - ->getMock(); - $this->userManager->expects($this->any()) ->method('get') - ->will($this->returnCallback(function($uid) use ($userBackend) { + ->will($this->returnCallback(function($uid) { switch($uid) { case 'user1' : return $this->getTestUser('user1'); case 'user2' : return $this->getTestUser('user2'); @@ -779,7 +754,7 @@ class ManagerTest extends \Test\TestCase { $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', '', 1); - $this->assertEquals(1, count($users)); + $this->assertCount(1, $users); $this->assertFalse(isset($users['user1'])); $this->assertTrue(isset($users['user2'])); $this->assertFalse(isset($users['user3'])); @@ -801,13 +776,9 @@ class ManagerTest extends \Test\TestCase { ->with('testgroup', '', 1, 1) ->will($this->returnValue(array('user33'))); - $userBackend = $this->getMockBuilder('\OC\User\Backend') - ->disableOriginalConstructor() - ->getMock(); - $this->userManager->expects($this->any()) ->method('get') - ->will($this->returnCallback(function($uid) use ($userBackend) { + ->will($this->returnCallback(function($uid) { switch($uid) { case 'user1' : return $this->getTestUser('user1'); case 'user2' : return $this->getTestUser('user2'); @@ -822,7 +793,7 @@ class ManagerTest extends \Test\TestCase { $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', '', 1, 1); - $this->assertEquals(1, count($users)); + $this->assertCount(1, $users); $this->assertFalse(isset($users['user1'])); $this->assertFalse(isset($users['user2'])); $this->assertFalse(isset($users['user3'])); @@ -857,11 +828,11 @@ class ManagerTest extends \Test\TestCase { // add user $group = $manager->get('group1'); $group->addUser($user1); - $expectedGroups = ['group1']; + $expectedGroups[] = 'group1'; // check result $groups = $manager->getUserGroups($user1); - $this->assertEquals(1, count($groups)); + $this->assertCount(1, $groups); $group1 = reset($groups); $this->assertEquals('group1', $group1->getGID()); } @@ -895,7 +866,7 @@ class ManagerTest extends \Test\TestCase { // prime cache $user1 = $this->getTestUser('user1'); $groups = $manager->getUserGroups($user1); - $this->assertEquals(1, count($groups)); + $this->assertCount(1, $groups); $group1 = reset($groups); $this->assertEquals('group1', $group1->getGID()); -- cgit v1.2.3 From 80d2717e5cb80615c2e75885398c26289fad463c Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 17 Mar 2017 13:54:58 +0100 Subject: Fix 5.6 duplicate class import Signed-off-by: Joas Schilling --- tests/lib/Group/ManagerTest.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) (limited to 'tests') diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index bd73e8f4052..23a35024e0a 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -27,9 +27,7 @@ use OC\User\Manager; use OCP\ILogger; use OCP\IUser; use OCP\GroupInterface; -use OCP\IUserManager; use Test\TestCase; -use Test\Util\Group\Dummy; class ManagerTest extends TestCase { /** @var Manager|\PHPUnit_Framework_MockObject_MockObject $userManager */ @@ -134,7 +132,7 @@ class ManagerTest extends TestCase { } public function testGetDeleted() { - $backend = new Dummy(); + $backend = new \Test\Util\Group\Dummy(); $backend->createGroup('group1'); $manager = new \OC\Group\Manager($this->userManager, $this->logger); -- cgit v1.2.3