Merge pull request #56537 from nextcloud/refactor/files-sharing

Refactor mount providers files_sharing app

apps/files_sharing/lib/External/MountProvider.php

@@ -15,6 +15,7 @@ use OCP\Http\Client\IClientService;
 use OCP\IDBConnection;
 use OCP\IUser;
 use OCP\Server;
+use OCP\Share\IShare;
 
 class MountProvider implements IMountProvider {
 	public const STORAGE = '\OCA\Files_Sharing\External\Storage';
@@ -54,7 +55,7 @@ class MountProvider implements IMountProvider {
 		$qb->select('remote', 'share_token', 'password', 'mountpoint', 'owner')
 			->from('share_external')
 			->where($qb->expr()->eq('user', $qb->createNamedParameter($user->getUID())))
-			->andWhere($qb->expr()->eq('accepted', $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT)));
+			->andWhere($qb->expr()->eq('accepted', $qb->createNamedParameter(IShare::STATUS_ACCEPTED, IQueryBuilder::PARAM_INT)));
 		$result = $qb->executeQuery();
 		$mounts = [];
 		while ($row = $result->fetchAssociative()) {

apps/files_sharing/lib/MountProvider.php

@@ -7,6 +7,8 @@
  */
 namespace OCA\Files_Sharing;
 
+use Exception;
+use InvalidArgumentException;
 use OC\Files\View;
 use OCA\Files_Sharing\Event\ShareMountedEvent;
 use OCP\Cache\CappedMemoryCache;
@@ -18,9 +20,11 @@ use OCP\Files\Storage\IStorageFactory;
 use OCP\ICacheFactory;
 use OCP\IConfig;
 use OCP\IUser;
+use OCP\Share\IAttributes;
 use OCP\Share\IManager;
 use OCP\Share\IShare;
 use Psr\Log\LoggerInterface;
+use function count;
 
 class MountProvider implements IMountProvider {
 	/**
@@ -46,95 +50,20 @@ class MountProvider implements IMountProvider {
 	 * @return IMountPoint[]
 	 */
 	public function getMountsForUser(IUser $user, IStorageFactory $loader) {
+		$userId = $user->getUID();
 		$shares = array_merge(
-			$this->shareManager->getSharedWith($user->getUID(), IShare::TYPE_USER, null, -1),
-			$this->shareManager->getSharedWith($user->getUID(), IShare::TYPE_GROUP, null, -1),
-			$this->shareManager->getSharedWith($user->getUID(), IShare::TYPE_CIRCLE, null, -1),
-			$this->shareManager->getSharedWith($user->getUID(), IShare::TYPE_ROOM, null, -1),
-			$this->shareManager->getSharedWith($user->getUID(), IShare::TYPE_DECK, null, -1),
-			$this->shareManager->getSharedWith($user->getUID(), IShare::TYPE_SCIENCEMESH, null, -1),
+			$this->shareManager->getSharedWith($userId, IShare::TYPE_USER, null, -1),
+			$this->shareManager->getSharedWith($userId, IShare::TYPE_GROUP, null, -1),
+			$this->shareManager->getSharedWith($userId, IShare::TYPE_CIRCLE, null, -1),
+			$this->shareManager->getSharedWith($userId, IShare::TYPE_ROOM, null, -1),
+			$this->shareManager->getSharedWith($userId, IShare::TYPE_DECK, null, -1),
+			$this->shareManager->getSharedWith($userId, IShare::TYPE_SCIENCEMESH, null, -1),
 		);
 
-		// filter out excluded shares and group shares that includes self
-		$shares = array_filter($shares, function (IShare $share) use ($user) {
-			return $share->getPermissions() > 0 && $share->getShareOwner() !== $user->getUID() && $share->getSharedBy() !== $user->getUID();
-		});
-
+		$shares = $this->filterShares($shares, $userId);
 		$superShares = $this->buildSuperShares($shares, $user);
 
-		$allMounts = $this->mountManager->getAll();
-		$mounts = [];
-		$view = new View('/' . $user->getUID() . '/files');
-		$ownerViews = [];
-		$sharingDisabledForUser = $this->shareManager->sharingDisabledForUser($user->getUID());
-		/** @var CappedMemoryCache<bool> $folderExistCache */
-		$foldersExistCache = new CappedMemoryCache();
-
-		$validShareCache = $this->cacheFactory->createLocal('share-valid-mountpoint-max');
-		$maxValidatedShare = $validShareCache->get($user->getUID()) ?? 0;
-		$newMaxValidatedShare = $maxValidatedShare;
-
-		foreach ($superShares as $share) {
-			try {
-				/** @var IShare $parentShare */
-				$parentShare = $share[0];
-
-				if ($parentShare->getStatus() !== IShare::STATUS_ACCEPTED
-					&& ($parentShare->getShareType() === IShare::TYPE_GROUP
-						|| $parentShare->getShareType() === IShare::TYPE_USERGROUP
-						|| $parentShare->getShareType() === IShare::TYPE_USER)) {
-					continue;
-				}
-
-				$owner = $parentShare->getShareOwner();
-				if (!isset($ownerViews[$owner])) {
-					$ownerViews[$owner] = new View('/' . $parentShare->getShareOwner() . '/files');
-				}
-				$shareId = (int)$parentShare->getId();
-				$mount = new SharedMount(
-					'\OCA\Files_Sharing\SharedStorage',
-					$allMounts,
-					[
-						'user' => $user->getUID(),
-						// parent share
-						'superShare' => $parentShare,
-						// children/component of the superShare
-						'groupedShares' => $share[1],
-						'ownerView' => $ownerViews[$owner],
-						'sharingDisabledForUser' => $sharingDisabledForUser
-					],
-					$loader,
-					$view,
-					$foldersExistCache,
-					$this->eventDispatcher,
-					$user,
-					($shareId <= $maxValidatedShare),
-				);
-
-				$newMaxValidatedShare = max($shareId, $newMaxValidatedShare);
-
-				$event = new ShareMountedEvent($mount);
-				$this->eventDispatcher->dispatchTyped($event);
-
-				$mounts[$mount->getMountPoint()] = $allMounts[$mount->getMountPoint()] = $mount;
-				foreach ($event->getAdditionalMounts() as $additionalMount) {
-					$allMounts[$additionalMount->getMountPoint()] = $mounts[$additionalMount->getMountPoint()] = $additionalMount;
-				}
-			} catch (\Exception $e) {
-				$this->logger->error(
-					'Error while trying to create shared mount',
-					[
-						'app' => 'files_sharing',
-						'exception' => $e,
-					],
-				);
-			}
-		}
-
-		$validShareCache->set($user->getUID(), $newMaxValidatedShare, 24 * 60 * 60);
-
-		// array_filter removes the null values from the array
-		return array_values(array_filter($mounts));
+		return $this->getMountsFromSuperShares($userId, $superShares, $loader, $user);
 	}
 
 	/**
@@ -148,10 +77,11 @@ class MountProvider implements IMountProvider {
 		$tmp = [];
 
 		foreach ($shares as $share) {
-			if (!isset($tmp[$share->getNodeId()])) {
-				$tmp[$share->getNodeId()] = [];
+			$nodeId = $share->getNodeId();
+			if (!isset($tmp[$nodeId])) {
+				$tmp[$nodeId] = [];
 			}
-			$tmp[$share->getNodeId()][] = $share;
+			$tmp[$nodeId][] = $share;
 		}
 
 		$result = [];
@@ -168,25 +98,26 @@ class MountProvider implements IMountProvider {
 			$result[] = $tmp2;
 		}
 
-		return array_values($result);
+		return $result;
 	}
 
 	/**
-	 * Build super shares (virtual share) by grouping them by node id and target,
-	 * then for each group compute the super share and return it along with the matching
-	 * grouped shares. The most permissive permissions are used based on the permissions
-	 * of all shares within the group.
+	 * Groups shares by node ID and builds a new share object (super share)
+	 * which represents a summarized version of all the shares in the group.
+	 *
+	 * The permissions and attributes of the super share are accumulated from
+	 * the shares in the group, forming the most permissive combination
+	 * possible.
 	 *
 	 * @param IShare[] $allShares
 	 * @param IUser $user user
-	 * @return array Tuple of [superShare, groupedShares]
+	 * @return list<array{IShare, array<IShare>}> Tuple of [superShare, groupedShares]
 	 */
 	private function buildSuperShares(array $allShares, IUser $user) {
 		$result = [];
 
 		$groupedShares = $this->groupShares($allShares);
 
-		/** @var IShare[] $shares */
 		foreach ($groupedShares as $shares) {
 			if (count($shares) === 0) {
 				continue;
@@ -201,14 +132,7 @@ class MountProvider implements IMountProvider {
 				->setShareType($shares[0]->getShareType())
 				->setTarget($shares[0]->getTarget());
 
-			// Gather notes from all the shares.
-			// Since these are readly available here, storing them
-			// enables the DAV FilesPlugin to avoid executing many
-			// DB queries to retrieve the same information.
-			$allNotes = implode("\n", array_map(function ($sh) {
-				return $sh->getNote();
-			}, $shares));
-			$superShare->setNote($allNotes);
+			$this->combineNotes($shares, $superShare);
 
 			// use most permissive permissions
 			// this covers the case where there are multiple shares for the same
@@ -217,7 +141,6 @@ class MountProvider implements IMountProvider {
 			$superAttributes = $this->shareManager->newShare()->newAttributes();
 			$status = IShare::STATUS_PENDING;
 			foreach ($shares as $share) {
-				$superPermissions |= $share->getPermissions();
 				$status = max($status, $share->getStatus());
 				// update permissions
 				$superPermissions |= $share->getPermissions();
@@ -225,38 +148,11 @@ class MountProvider implements IMountProvider {
 				// update share permission attributes
 				$attributes = $share->getAttributes();
 				if ($attributes !== null) {
-					foreach ($attributes->toArray() as $attribute) {
-						if ($superAttributes->getAttribute($attribute['scope'], $attribute['key']) === true) {
-							// if super share attribute is already enabled, it is most permissive
-							continue;
-						}
-						// update supershare attributes with subshare attribute
-						$superAttributes->setAttribute($attribute['scope'], $attribute['key'], $attribute['value']);
-					}
+					$this->mergeAttributes($attributes, $superAttributes);
 				}
 
-				// adjust target, for database consistency if needed
-				if ($share->getTarget() !== $superShare->getTarget()) {
-					$share->setTarget($superShare->getTarget());
-					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())) {
+				$this->adjustTarget($share, $superShare, $user);
+				if ($share->getNodeCacheEntry() !== null) {
 					$superShare->setNodeCacheEntry($share->getNodeCacheEntry());
 				}
 			}
@@ -270,4 +166,192 @@ class MountProvider implements IMountProvider {
 
 		return $result;
 	}
+
+	/**
+	 * Combines $attributes into the most permissive set of attributes and
+	 * sets them in $superAttributes.
+	 */
+	private function mergeAttributes(
+		IAttributes $attributes,
+		IAttributes $superAttributes,
+	): void {
+		foreach ($attributes->toArray() as $attribute) {
+			if ($superAttributes->getAttribute(
+				$attribute['scope'],
+				$attribute['key']
+			) === true) {
+				// if super share attribute is already enabled, it is most permissive
+				continue;
+			}
+			// update super share attributes with subshare attribute
+			$superAttributes->setAttribute(
+				$attribute['scope'],
+				$attribute['key'],
+				$attribute['value']
+			);
+		}
+	}
+
+	/**
+	 * Gather notes from all the shares. Since these are readily available
+	 * here, storing them enables the DAV FilesPlugin to avoid executing many
+	 * DB queries to retrieve the same information.
+	 *
+	 * @param array<IShare> $shares
+	 * @param IShare $superShare
+	 * @return void
+	 */
+	private function combineNotes(
+		array &$shares,
+		IShare $superShare,
+	): void {
+		$allNotes = implode(
+			"\n",
+			array_map(static fn ($sh) => $sh->getNote(), $shares)
+		);
+		$superShare->setNote($allNotes);
+	}
+
+	/**
+	 * Adjusts the target in $share for DB consistency, if needed.
+	 */
+	private function adjustTarget(
+		IShare $share,
+		IShare $superShare,
+		IUser $user,
+	): void {
+		if ($share->getTarget() === $superShare->getTarget()) {
+			return;
+		}
+
+		$share->setTarget($superShare->getTarget());
+		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']
+			);
+		}
+	}
+	/**
+	 * @param string $userId
+	 * @param array $superShares
+	 * @param IStorageFactory $loader
+	 * @param IUser $user
+	 * @return array
+	 * @throws Exception
+	 */
+	private function getMountsFromSuperShares(
+		string $userId,
+		array $superShares,
+		IStorageFactory $loader,
+		IUser $user,
+	): array {
+		$allMounts = $this->mountManager->getAll();
+		$mounts = [];
+		$view = new View('/' . $userId . '/files');
+		$ownerViews = [];
+		$sharingDisabledForUser
+			= $this->shareManager->sharingDisabledForUser($userId);
+		/** @var CappedMemoryCache<bool> $folderExistCache */
+		$foldersExistCache = new CappedMemoryCache();
+
+		$validShareCache
+			= $this->cacheFactory->createLocal('share-valid-mountpoint-max');
+		$maxValidatedShare = $validShareCache->get($userId) ?? 0;
+		$newMaxValidatedShare = $maxValidatedShare;
+
+		foreach ($superShares as $share) {
+			[$parentShare, $groupedShares] = $share;
+			try {
+				if ($parentShare->getStatus() !== IShare::STATUS_ACCEPTED
+					&& ($parentShare->getShareType() === IShare::TYPE_GROUP
+						|| $parentShare->getShareType() === IShare::TYPE_USERGROUP
+						|| $parentShare->getShareType() === IShare::TYPE_USER)
+				) {
+					continue;
+				}
+
+				$owner = $parentShare->getShareOwner();
+				if (!isset($ownerViews[$owner])) {
+					$ownerViews[$owner] = new View('/' . $owner . '/files');
+				}
+				$shareId = (int)$parentShare->getId();
+				$mount = new SharedMount(
+					'\OCA\Files_Sharing\SharedStorage',
+					$allMounts,
+					[
+						'user' => $userId,
+						// parent share
+						'superShare' => $parentShare,
+						// children/component of the superShare
+						'groupedShares' => $groupedShares,
+						'ownerView' => $ownerViews[$owner],
+						'sharingDisabledForUser' => $sharingDisabledForUser
+					],
+					$loader,
+					$view,
+					$foldersExistCache,
+					$this->eventDispatcher,
+					$user,
+					$shareId <= $maxValidatedShare,
+				);
+
+				$newMaxValidatedShare = max($shareId, $newMaxValidatedShare);
+
+				$event = new ShareMountedEvent($mount);
+				$this->eventDispatcher->dispatchTyped($event);
+
+				$mounts[$mount->getMountPoint()]
+				= $allMounts[$mount->getMountPoint()] = $mount;
+				foreach ($event->getAdditionalMounts() as $additionalMount) {
+					$allMounts[$additionalMount->getMountPoint()]
+					= $mounts[$additionalMount->getMountPoint()]
+						= $additionalMount;
+				}
+			} catch (Exception $e) {
+				$this->logger->error(
+					'Error while trying to create shared mount',
+					[
+						'app' => 'files_sharing',
+						'exception' => $e,
+					],
+				);
+			}
+		}
+
+		$validShareCache->set($userId, $newMaxValidatedShare, 24 * 60 * 60);
+
+		// array_filter removes the null values from the array
+		return array_values(array_filter($mounts));
+	}
+
+	/**
+	 * Filters out shares owned or shared by the user and ones for which the
+	 * user has no permissions.
+	 *
+	 * @param IShare[] $shares
+	 * @return IShare[]
+	 */
+	private function filterShares(array $shares, string $userId): array {
+		return array_filter(
+			$shares,
+			static function (IShare $share) use ($userId) {
+				return $share->getPermissions() > 0
+					&& $share->getShareOwner() !== $userId
+					&& $share->getSharedBy() !== $userId;
+			}
+		);
+	}
 }

build/psalm-baseline.xml

@@ -1685,16 +1685,13 @@
   </file>
   <file src="apps/files_sharing/lib/MountProvider.php">
     <InternalClass>
-      <code><![CDATA[new View('/' . $parentShare->getShareOwner() . '/files')]]></code>
-      <code><![CDATA[new View('/' . $user->getUID() . '/files')]]></code>
+      <code><![CDATA[new View('/' . $owner . '/files')]]></code>
+      <code><![CDATA[new View('/' . $userId . '/files')]]></code>
     </InternalClass>
     <InternalMethod>
-      <code><![CDATA[new View('/' . $parentShare->getShareOwner() . '/files')]]></code>
-      <code><![CDATA[new View('/' . $user->getUID() . '/files')]]></code>
+      <code><![CDATA[new View('/' . $owner . '/files')]]></code>
+      <code><![CDATA[new View('/' . $userId . '/files')]]></code>
     </InternalMethod>
-    <RedundantFunctionCall>
-      <code><![CDATA[array_values]]></code>
-    </RedundantFunctionCall>
   </file>
   <file src="apps/files_sharing/lib/Scanner.php">
     <DeprecatedInterface>