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)