[stable11] Skip null groups in group manager

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;
 			}
 		}

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
+						$this->logger->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());

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();

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);

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,15 +184,15 @@ protected function getGroupObject($gid) {
 	 * @return bool
 	 */
 	public function groupExists($gid) {
-		return !is_null($this->get($gid));
+		return $this->get($gid) instanceof IGroup;
 	}
 
 	/**
 	 * @param string $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;
@@ -213,7 +221,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 ($aGroup instanceof IGroup) {
+					$groups[$groupId] = $aGroup;
+				} else {
+					$this->logger->debug('Group "' . $groupId . '" was returned by search but not found through direct access', ['app' => 'core']);
+				}
 			}
 			if (!is_null($limit) and $limit <= 0) {
 				return array_values($groups);
@@ -227,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());
@@ -246,7 +259,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 ($aGroup instanceof IGroup) {
+						$groups[$groupId] = $aGroup;
+					} else {
+						$this->logger->debug('User "' . $uid . '" belongs to deleted group: "' . $groupId . '"', ['app' => 'core']);
+					}
 				}
 			}
 		}

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));
 			});

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');
 			}

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');