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 <hey@morrisjobke.de>

4 files changed, 45 insertions, 4 deletions

diff --git a/apps/files_sharing/lib/Controller/ShareAPIController.php b/apps/files_sharing/lib/Controller/ShareAPIController.php
index 6181cde6fe6..fa81e6364a2 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 5c5d57057b2..93f85a888b0 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 97774081b6a..2bf9a40b2cc 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 0be74a645a9..902c81c7136 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);