From 377fdf3860e317b68392c678fa25e081251baecc Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Thu, 19 Jan 2017 15:02:46 +0100 Subject: [PATCH] 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 6181cde6fe..fa81e6364a 100644 --- a/apps/files_sharing/lib/Controller/ShareAPIController.php +++ b/apps/files_sharing/lib/Controller/ShareAPIController.php @@ -779,7 +779,7 @@ class ShareAPIController extends OCSController { 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 5c5d57057b..93f85a888b 100644 --- a/apps/files_sharing/lib/MountProvider.php +++ b/apps/files_sharing/lib/MountProvider.php @@ -170,7 +170,23 @@ class MountProvider implements IMountProvider { 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 97774081b6..2bf9a40b2c 100644 --- a/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php +++ b/apps/files_sharing/tests/Controller/ShareAPIControllerTest.php @@ -544,14 +544,19 @@ class ShareAPIControllerTest extends \Test\TestCase { $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 0be74a645a..902c81c713 100644 --- a/apps/files_sharing/tests/MountProviderTest.php +++ b/apps/files_sharing/tests/MountProviderTest.php @@ -263,6 +263,20 @@ class MountProviderTest extends \Test\TestCase { ['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 @@ class MountProviderTest extends \Test\TestCase { * @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 @@ class MountProviderTest extends \Test\TestCase { 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 944598a829..8b18abd5e9 100644 --- a/lib/private/Group/Manager.php +++ b/lib/private/Group/Manager.php @@ -223,7 +223,12 @@ class Manager extends PublicEmitter implements IGroupManager { 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); @@ -256,7 +261,12 @@ class Manager extends PublicEmitter implements IGroupManager { $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 fe6472c31a..e4ae26be13 100644 --- a/lib/private/Share20/DefaultShareProvider.php +++ b/lib/private/Share20/DefaultShareProvider.php @@ -329,6 +329,10 @@ class DefaultShareProvider implements IShareProvider { $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 acc142f62b..6b86ec4f45 100644 --- a/lib/private/Share20/Manager.php +++ b/lib/private/Share20/Manager.php @@ -393,10 +393,12 @@ class Manager implements IManager { // 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 @@ class Manager implements IManager { 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'); } } @@ -886,6 +888,9 @@ class Manager implements IManager { 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 1a7ced5f1b..c57ef0b34c 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 5870da8d21..a1e7831355 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 7b01a8f9e7..25f406c140 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)