From a78b13da82bafa281d273f9889f1611d17f3c0f5 Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 19 Jan 2017 15:02:46 +0100 Subject: [PATCH 1/4] 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 --- .../lib/Controller/ShareAPIController.php | 2 +- apps/files_sharing/lib/MountProvider.php | 18 ++++- .../Controller/ShareAPIControllerTest.php | 7 +- .../files_sharing/tests/MountProviderTest.php | 22 +++++- lib/private/Group/Manager.php | 14 +++- lib/private/Share20/DefaultShareProvider.php | 4 + lib/private/Share20/Manager.php | 13 +++- tests/lib/Group/ManagerTest.php | 56 ++++++++++++++ .../lib/Share20/DefaultShareProviderTest.php | 42 +++++++++++ tests/lib/Share20/ManagerTest.php | 73 +++++++++++++++++++ 10 files changed, 241 insertions(+), 10 deletions(-) diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php index 90274beba492c..1a13f134d245e 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -777,7 +777,7 @@ protected function canAccessShare(\OCP\Share\IShare $share, $checkGroups = true) if ($checkGroups && $share->getShareType() === \OCP\Share::SHARE_TYPE_GROUP) { $sharedWith = $this->groupManager->get($share->getSharedWith()); $user = $this->userManager->get($this->currentUser); - if ($user !== null && $sharedWith->inGroup($user)) { + if ($user !== null && $sharedWith !== null && $sharedWith->inGroup($user)) { return true; } } diff --git a/apps/files_sharing/lib/MountProvider.php b/apps/files_sharing/lib/MountProvider.php index 40d2fb27535c0..f65c1b043c98b 100644 --- a/apps/files_sharing/lib/MountProvider.php +++ b/apps/files_sharing/lib/MountProvider.php @@ -170,7 +170,23 @@ private function buildSuperShares(array $allShares, \OCP\IUser $user) { if ($share->getTarget() !== $superShare->getTarget()) { // adjust target, for database consistency $share->setTarget($superShare->getTarget()); - $this->shareManager->moveShare($share, $user->getUID()); + try { + $this->shareManager->moveShare($share, $user->getUID()); + } catch (\InvalidArgumentException $e) { + // ignore as it is not important and we don't want to + // block FS setup + + // the subsequent code anyway only uses the target of the + // super share + + // such issue can usually happen when dealing with + // null groups which usually appear with group backend + // caching inconsistencies + \OC::$server->getLogger()->debug( + 'Could not adjust share target for share ' . $share->getId() . ' to make it consistent: ' . $e->getMessage(), + ['app' => 'files_sharing'] + ); + } } if (!is_null($share->getNodeCacheEntry())) { $superShare->setNodeCacheEntry($share->getNodeCacheEntry()); diff --git a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php index ed4aa1dba9e5d..972902739bc3b 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -545,14 +545,19 @@ public function testCanAccessShare() { $this->groupManager->method('get')->will($this->returnValueMap([ ['group', $group], ['group2', $group2], + ['groupnull', null], ])); $this->assertTrue($this->invokePrivate($this->ocs, 'canAccessShare', [$share])); $share = $this->getMockBuilder('OCP\Share\IShare')->getMock(); $share->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_GROUP); $share->method('getSharedWith')->willReturn('group2'); + $this->assertFalse($this->invokePrivate($this->ocs, 'canAccessShare', [$share])); - $this->groupManager->method('get')->with('group2')->willReturn($group); + // null group + $share = $this->getMock('OCP\Share\IShare'); + $share->method('getShareType')->willReturn(\OCP\Share::SHARE_TYPE_GROUP); + $share->method('getSharedWith')->willReturn('groupnull'); $this->assertFalse($this->invokePrivate($this->ocs, 'canAccessShare', [$share])); $share = $this->getMockBuilder('OCP\Share\IShare')->getMock(); diff --git a/apps/files_sharing/tests/MountProviderTest.php b/apps/files_sharing/tests/MountProviderTest.php index 0be74a645a9bb..902c81c7136ee 100644 --- a/apps/files_sharing/tests/MountProviderTest.php +++ b/apps/files_sharing/tests/MountProviderTest.php @@ -263,6 +263,20 @@ public function mergeSharesDataProvider() { ['1', 100, 'user2', '/share2-renamed', 31], ], ], + // #9: share as outsider with "nullgroup" and "user1" where recipient renamed in between + [ + [ + [2, 100, 'user2', '/share2', 31], + ], + [ + [1, 100, 'nullgroup', '/share2-renamed', 31], + ], + [ + // use target of least recent share + ['1', 100, 'nullgroup', '/share2-renamed', 31], + ], + true + ], ]; } @@ -278,7 +292,7 @@ public function mergeSharesDataProvider() { * @param array $groupShares array of group share specs * @param array $expectedShares array of expected supershare specs */ - public function testMergeShares($userShares, $groupShares, $expectedShares) { + public function testMergeShares($userShares, $groupShares, $expectedShares, $moveFails = false) { $rootFolder = $this->createMock(IRootFolder::class); $userManager = $this->createMock(IUserManager::class); @@ -307,6 +321,12 @@ public function testMergeShares($userShares, $groupShares, $expectedShares) { return new \OC\Share20\Share($rootFolder, $userManager); })); + if ($moveFails) { + $this->shareManager->expects($this->any()) + ->method('moveShare') + ->will($this->throwException(new \InvalidArgumentException())); + } + $mounts = $this->provider->getMountsForUser($this->user, $this->loader); $this->assertCount(count($expectedShares), $mounts); diff --git a/lib/private/Group/Manager.php b/lib/private/Group/Manager.php index e7f0acc95a1c9..30fde1ad1938b 100644 --- a/lib/private/Group/Manager.php +++ b/lib/private/Group/Manager.php @@ -213,7 +213,12 @@ public function search($search, $limit = null, $offset = null) { foreach ($this->backends as $backend) { $groupIds = $backend->getGroups($search, $limit, $offset); foreach ($groupIds as $groupId) { - $groups[$groupId] = $this->get($groupId); + $aGroup = $this->get($groupId); + if (!is_null($aGroup)) { + $groups[$groupId] = $aGroup; + } else { + \OC::$server->getLogger()->debug('Group "' . $groupId . '" was returned by search but not found through direct access', array('app' => 'core')); + } } if (!is_null($limit) and $limit <= 0) { return array_values($groups); @@ -246,7 +251,12 @@ public function getUserIdGroups($uid) { $groupIds = $backend->getUserGroups($uid); if (is_array($groupIds)) { foreach ($groupIds as $groupId) { - $groups[$groupId] = $this->get($groupId); + $aGroup = $this->get($groupId); + if (!is_null($aGroup)) { + $groups[$groupId] = $aGroup; + } else { + \OC::$server->getLogger()->debug('User "' . $uid . '" belongs to deleted group: "' . $groupId . '"', array('app' => 'core')); + } } } } diff --git a/lib/private/Share20/DefaultShareProvider.php b/lib/private/Share20/DefaultShareProvider.php index f7ef3875e296d..b1863f6b08d7b 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -329,6 +329,10 @@ public function deleteFromSelf(\OCP\Share\IShare $share, $recipient) { $group = $this->groupManager->get($share->getSharedWith()); $user = $this->userManager->get($recipient); + if (is_null($group)) { + throw new ProviderException('Group "' . $share->getSharedWith() . '" does not exist'); + } + if (!$group->inGroup($user)) { throw new ProviderException('Recipient not in receiving group'); } diff --git a/lib/private/Share20/Manager.php b/lib/private/Share20/Manager.php index cd1d52c3bbf46..d6d5db324bb32 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -393,10 +393,12 @@ protected function userCreateChecks(\OCP\Share\IShare $share) { // The share is already shared with this user via a group share if ($existingShare->getShareType() === \OCP\Share::SHARE_TYPE_GROUP) { $group = $this->groupManager->get($existingShare->getSharedWith()); - $user = $this->userManager->get($share->getSharedWith()); + if (!is_null($group)) { + $user = $this->userManager->get($share->getSharedWith()); - if ($group->inGroup($user) && $existingShare->getShareOwner() !== $share->getShareOwner()) { - throw new \Exception('Path already shared with this user'); + if ($group->inGroup($user) && $existingShare->getShareOwner() !== $share->getShareOwner()) { + throw new \Exception('Path already shared with this user'); + } } } } @@ -418,7 +420,7 @@ protected function groupCreateChecks(\OCP\Share\IShare $share) { if ($this->shareWithGroupMembersOnly()) { $sharedBy = $this->userManager->get($share->getSharedBy()); $sharedWith = $this->groupManager->get($share->getSharedWith()); - if (!$sharedWith->inGroup($sharedBy)) { + if (is_null($sharedWith) || !$sharedWith->inGroup($sharedBy)) { throw new \Exception('Only sharing within your own groups is allowed'); } } @@ -887,6 +889,9 @@ public function moveShare(\OCP\Share\IShare $share, $recipientId) { if ($share->getShareType() === \OCP\Share::SHARE_TYPE_GROUP) { $sharedWith = $this->groupManager->get($share->getSharedWith()); + if (is_null($sharedWith)) { + throw new \InvalidArgumentException('Group "' . $share->getSharedWith() . '" does not exist'); + } $recipient = $this->userManager->get($recipientId); if (!$sharedWith->inGroup($recipient)) { throw new \InvalidArgumentException('Invalid recipient'); diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index ece99eb569db5..4c37a67973547 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -286,6 +286,32 @@ public function testSearchMultipleBackendsLimitAndOffset() { $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 @@ -340,6 +366,36 @@ public function testGetUserGroupIds() { } } + 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 5870da8d2188f..a1e783135589d 100644 --- a/tests/lib/Share20/DefaultShareProviderTest.php +++ b/tests/lib/Share20/DefaultShareProviderTest.php @@ -1524,6 +1524,48 @@ public function testDeleteFromSelfGroupUserNotInGroup() { $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 bd85e3c73aa92..2383c0d290a5d 100644 --- a/tests/lib/Share20/ManagerTest.php +++ b/tests/lib/Share20/ManagerTest.php @@ -1138,6 +1138,39 @@ public function testUserCreateChecksIdenticalPathSharedViaGroup() { $this->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); @@ -1215,6 +1248,29 @@ public function testGroupCreateChecksShareWithGroupMembersOnlyNotInGroup() { $this->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(); @@ -2510,6 +2566,23 @@ public function testMoveShareGroupNotRecipient() { $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) From 564b2f974f25c7a382f6f0ce4cd8c274d914bc4d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 17 Mar 2017 10:23:04 +0100 Subject: [PATCH 2/4] 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 | 57 +++++++++++++----------- 4 files changed, 49 insertions(+), 36 deletions(-) diff --git a/apps/files_sharing/lib/MountProvider.php b/apps/files_sharing/lib/MountProvider.php index f65c1b043c98b..21b0ca88f015b 100644 --- a/apps/files_sharing/lib/MountProvider.php +++ b/apps/files_sharing/lib/MountProvider.php @@ -182,7 +182,7 @@ private function buildSuperShares(array $allShares, \OCP\IUser $user) { // 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 30fde1ad1938b..2481ebf173602 100644 --- a/lib/private/Group/Manager.php +++ b/lib/private/Group/Manager.php @@ -37,7 +37,10 @@ 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) { @@ -176,7 +184,7 @@ protected function getGroupObject($gid) { * @return bool */ public function groupExists($gid) { - return !is_null($this->get($gid)); + return $this->get($gid) instanceof IGroup; } /** @@ -184,7 +192,7 @@ public function groupExists($gid) { * @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; @@ -214,10 +222,10 @@ public function search($search, $limit = null, $offset = null) { $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) { @@ -232,7 +240,7 @@ public function search($search, $limit = null, $offset = null) { * @return \OC\Group\Group[] */ public function getUserGroups($user) { - if (is_null($user)) { + if (!$user instanceof IUser) { return []; } return $this->getUserIdGroups($user->getUID()); @@ -252,10 +260,10 @@ public function getUserIdGroups($uid) { 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 0e9b2605efb3f..ffb34d70eb9e0 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -223,7 +223,7 @@ public function __construct($webRoot, \OC\Config $config) { 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 4c37a67973547..925bd1e2cf5fe 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -11,10 +11,14 @@ use OC\User\Manager; use OC\User\User; +use OCP\ILogger; +use OCP\IUser; class ManagerTest extends \Test\TestCase { /** @var Manager */ private $userManager; + /** @var ILogger|\PHPUnit_Framework_MockObject_MockObject $userManager */ + protected $logger; public function setUp() { parent::setUp(); @@ -22,6 +26,7 @@ public function setUp() { $this->userManager = $this->getMockBuilder('\OC\User\Manager') ->disableOriginalConstructor() ->getMock(); + $this->logger = $this->createMock(ILogger::class); } /** @@ -52,7 +57,7 @@ public function testGet() { ->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'); @@ -61,7 +66,7 @@ public function testGet() { } public function testGetNoBackend() { - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $this->assertNull($manager->get('group1')); } @@ -78,7 +83,7 @@ public function testGetNotExists() { ->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')); @@ -88,7 +93,7 @@ public function testGetDeleted() { $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'); @@ -119,7 +124,7 @@ public function testGetMultipleBackends() { ->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); @@ -151,7 +156,7 @@ public function testCreate() { $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'); @@ -172,7 +177,7 @@ public function testCreateExists() { $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'); @@ -195,7 +200,7 @@ public function testSearch() { ->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'); @@ -233,7 +238,7 @@ public function testSearchMultipleBackends() { ->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); @@ -274,7 +279,7 @@ public function testSearchMultipleBackendsLimitAndOffset() { ->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); @@ -305,7 +310,7 @@ public function testSearchResultExistsButGroupDoesNot() { */ $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'); @@ -331,7 +336,7 @@ public function testGetUserGroups() { $userBackend = $this->getMockBuilder('\OC\Group\Database') ->disableOriginalConstructor() ->getMock(); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $groups = $manager->getUserGroups($this->newUser('user1', $userBackend)); @@ -385,7 +390,7 @@ public function testGetUserGroupsWithDeletedGroup() { */ $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 */ @@ -411,7 +416,7 @@ public function testInGroup() { ->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')); @@ -432,7 +437,7 @@ public function testIsAdmin() { ->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')); @@ -453,7 +458,7 @@ public function testNotAdmin() { ->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')); @@ -491,7 +496,7 @@ public function testGetUserGroupsMultipleBackends() { $userBackend = $this->getMockBuilder('\OC\User\Backend') ->disableOriginalConstructor() ->getMock(); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend1); $manager->addBackend($backend2); @@ -556,7 +561,7 @@ public function testDisplayNamesInGroupWithOneUserBackend() { } })); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', 'user3'); @@ -622,7 +627,7 @@ public function testDisplayNamesInGroupWithOneUserBackendWithLimitSpecified() { } })); - $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); @@ -692,7 +697,7 @@ public function testDisplayNamesInGroupWithOneUserBackendWithLimitAndOffsetSpeci } })); - $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); @@ -738,7 +743,7 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmpty() { } })); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', ''); @@ -783,7 +788,7 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitS } })); - $manager = new \OC\Group\Manager($this->userManager); + $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); $users = $manager->displayNamesInGroup('testgroup', '', 1); @@ -828,7 +833,7 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitA } })); - $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); @@ -861,7 +866,7 @@ public function testGetUserGroupsWithAddUser() { ->method('implementsActions') ->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 @@ -909,7 +914,7 @@ public function testGetUserGroupsWithRemoveUser() { ->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 @@ -941,7 +946,7 @@ public function testGetUserIdGroups() { ->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'); From e2354109cda666f9c0899a12d82075145b2fc0c1 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 17 Mar 2017 10:34:25 +0100 Subject: [PATCH 3/4] Clean up the test Signed-off-by: Joas Schilling --- tests/lib/Group/ManagerTest.php | 82 +++++++++++++++------------------ 1 file changed, 36 insertions(+), 46 deletions(-) diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index 925bd1e2cf5fe..339d813c69dec 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -9,12 +9,15 @@ namespace Test\Group; +use OC\Group\Database; use OC\User\Manager; use OC\User\User; use OCP\ILogger; use OCP\IUser; +use Test\TestCase; +use Test\Util\Group\Dummy; -class ManagerTest extends \Test\TestCase { +class ManagerTest extends TestCase { /** @var Manager */ private $userManager; /** @var ILogger|\PHPUnit_Framework_MockObject_MockObject $userManager */ @@ -90,7 +93,7 @@ public function testGetNotExists() { } public function testGetDeleted() { - $backend = new \Test\Util\Group\Dummy(); + $backend = new Dummy(); $backend->createGroup('group1'); $manager = new \OC\Group\Manager($this->userManager, $this->logger); @@ -134,9 +137,7 @@ public function testGetMultipleBackends() { } 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->getMockBuilder('\OC\Group\Database') ->disableOriginalConstructor() @@ -154,7 +155,7 @@ public function testCreate() { ->method('createGroup') ->will($this->returnCallback(function () use (&$backendGroupCreated) { $backendGroupCreated = true; - }));; + })); $manager = new \OC\Group\Manager($this->userManager, $this->logger); $manager->addBackend($backend); @@ -164,9 +165,7 @@ public function testCreate() { } public function testCreateExists() { - /** - * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Group\Backend $backend - */ + /** @var \PHPUnit_Framework_MockObject_MockObject|\OC\Group\Backend $backend */ $backend = $this->getMockBuilder('\OC\Group\Database') ->disableOriginalConstructor() ->getMock(); @@ -204,7 +203,7 @@ public function testSearch() { $manager->addBackend($backend); $groups = $manager->search('1'); - $this->assertEquals(1, count($groups)); + $this->assertCount(1, $groups); $group1 = reset($groups); $this->assertEquals('group1', $group1->getGID()); } @@ -243,7 +242,7 @@ public function testSearchMultipleBackends() { $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()); @@ -284,7 +283,7 @@ public function testSearchMultipleBackendsLimitAndOffset() { $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()); @@ -292,10 +291,8 @@ public function testSearchMultipleBackendsLimitAndOffset() { } 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') @@ -305,10 +302,8 @@ public function testSearchResultExistsButGroupDoesNot() { ->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); @@ -340,14 +335,14 @@ public function testGetUserGroups() { $manager->addBackend($backend); $groups = $manager->getUserGroups($this->newUser('user1', $userBackend)); - $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(); @@ -358,13 +353,11 @@ public function testGetUserGroupIds() { '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); @@ -375,7 +368,7 @@ public function testGetUserGroupsWithDeletedGroup() { /** * @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') @@ -385,17 +378,14 @@ public function testGetUserGroupsWithDeletedGroup() { ->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); @@ -501,7 +491,7 @@ public function testGetUserGroupsMultipleBackends() { $manager->addBackend($backend2); $groups = $manager->getUserGroups($this->newUser('user1', $userBackend)); - $this->assertEquals(2, count($groups)); + $this->assertCount(2, $groups); $group1 = reset($groups); $group2 = next($groups); $this->assertEquals('group1', $group1->getGID()); @@ -565,7 +555,7 @@ public function testDisplayNamesInGroupWithOneUserBackend() { $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'])); @@ -631,7 +621,7 @@ public function testDisplayNamesInGroupWithOneUserBackendWithLimitSpecified() { $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'])); @@ -701,7 +691,7 @@ public function testDisplayNamesInGroupWithOneUserBackendWithLimitAndOffsetSpeci $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'])); @@ -747,7 +737,7 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmpty() { $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'])); @@ -792,7 +782,7 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitS $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'])); @@ -837,7 +827,7 @@ public function testDisplayNamesInGroupWithOneUserBackendAndSearchEmptyAndLimitA $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'])); @@ -877,11 +867,11 @@ public function testGetUserGroupsWithAddUser() { // add user $group = $manager->get('group1'); $group->addUser($user1); - $expectedGroups = array('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()); } @@ -920,7 +910,7 @@ public function testGetUserGroupsWithRemoveUser() { // prime cache $user1 = $this->newUser('user1', null); $groups = $manager->getUserGroups($user1); - $this->assertEquals(1, count($groups)); + $this->assertCount(1, $groups); $group1 = reset($groups); $this->assertEquals('group1', $group1->getGID()); From 783a46cfce817faac315c017681a1f8ff36226a2 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 17 Mar 2017 13:54:58 +0100 Subject: [PATCH 4/4] Fix 5.6 duplicate class import Signed-off-by: Joas Schilling --- tests/lib/Group/ManagerTest.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/lib/Group/ManagerTest.php b/tests/lib/Group/ManagerTest.php index 339d813c69dec..e35966c4cc8ff 100644 --- a/tests/lib/Group/ManagerTest.php +++ b/tests/lib/Group/ManagerTest.php @@ -15,7 +15,6 @@ use OCP\ILogger; use OCP\IUser; use Test\TestCase; -use Test\Util\Group\Dummy; class ManagerTest extends TestCase { /** @var Manager */ @@ -93,7 +92,7 @@ public function testGetNotExists() { } public function testGetDeleted() { - $backend = new Dummy(); + $backend = new \Test\Util\Group\Dummy(); $backend->createGroup('group1'); $manager = new \OC\Group\Manager($this->userManager, $this->logger);