Fix user, group and circles change handling in bookmark folder shares

.github/workflows/phpunit-mariadb.yml

@@ -103,6 +103,14 @@ jobs:
           persist-credentials: false
           path: apps/${{ env.APP_NAME }}
 
+      - name: Checkout circles
+        uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
+        with:
+          persist-credentials: false
+          repository: nextcloud/circles
+          ref: ${{ matrix.server-versions }}
+          path: apps/circles
+
       - name: Set up php ${{ matrix.php-versions }}
         uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # 2.37.2
         with:
@@ -145,6 +153,7 @@ jobs:
         run: |
           mkdir data
           ./occ maintenance:install --verbose --database=mysql --database-name=nextcloud --database-host=127.0.0.1 --database-port=$DB_PORT --database-user=root --database-pass=rootpassword --admin-user admin --admin-pass admin
+          ./occ app:enable --force circles
           ./occ app:enable --force ${{ env.APP_NAME }}
 
       - name: Check PHPUnit script is defined

.github/workflows/phpunit-mysql.yml

@@ -101,6 +101,14 @@ jobs:
           persist-credentials: false
           path: apps/${{ env.APP_NAME }}
 
+      - name: Checkout circles
+        uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
+        with:
+          persist-credentials: false
+          repository: nextcloud/circles
+          ref: ${{ matrix.server-versions }}
+          path: apps/circles
+
       - name: Set up php ${{ matrix.php-versions }}
         uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # 2.37.2
         with:
@@ -143,6 +151,7 @@ jobs:
         run: |
           mkdir data
           ./occ maintenance:install --verbose --database=mysql --database-name=nextcloud --database-host=127.0.0.1 --database-port=$DB_PORT --database-user=root --database-pass=rootpassword --admin-user admin --admin-pass admin
+          ./occ app:enable --force circles
           ./occ app:enable --force ${{ env.APP_NAME }}
 
       - name: Check PHPUnit script is defined

.github/workflows/phpunit-pgsql.yml

@@ -104,6 +104,14 @@ jobs:
           persist-credentials: false
           path: apps/${{ env.APP_NAME }}
 
+      - name: Checkout circles
+        uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
+        with:
+          persist-credentials: false
+          repository: nextcloud/circles
+          ref: ${{ matrix.server-versions }}
+          path: apps/circles
+
       - name: Set up php ${{ matrix.php-versions }}
         uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # 2.37.2
         with:
@@ -141,6 +149,7 @@ jobs:
         run: |
           mkdir data
           ./occ maintenance:install --verbose --database=pgsql --database-name=nextcloud --database-host=127.0.0.1 --database-port=$DB_PORT --database-user=root --database-pass=rootpassword --admin-user admin --admin-pass admin
+          ./occ app:enable --force circles
           ./occ app:enable --force ${{ env.APP_NAME }}
 
       - name: Check PHPUnit script is defined

.github/workflows/phpunit-sqlite.yml

@@ -93,6 +93,14 @@ jobs:
           persist-credentials: false
           path: apps/${{ env.APP_NAME }}
 
+      - name: Checkout circles
+        uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
+        with:
+          persist-credentials: false
+          repository: nextcloud/circles
+          ref: ${{ matrix.server-versions }}
+          path: apps/circles
+
       - name: Set up php ${{ matrix.php-versions }}
         uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # 2.37.2
         with:
@@ -130,6 +138,7 @@ jobs:
         run: |
           mkdir data
           ./occ maintenance:install --verbose --database=sqlite --database-name=nextcloud --database-host=127.0.0.1 --database-port=$DB_PORT --database-user=root --database-pass=rootpassword --admin-user admin --admin-pass admin
+          ./occ app:enable --force circles
           ./occ app:enable --force ${{ env.APP_NAME }}
 
       - name: Check PHPUnit script is defined

.gitignore

@@ -5,3 +5,5 @@ _build
 node_modules
 vendor
 /.php_cs.cache
+venv
+.idea

appinfo/info.xml

@@ -61,6 +61,7 @@ Requirements:
 			<step>OCA\Bookmarks\Migration\OrphanedSharesRepairStep</step>
 			<step>OCA\Bookmarks\Migration\OrphanedTreeItemsRepairStep</step>
 			<step>OCA\Bookmarks\Migration\GroupSharesUpdateRepairStep</step>
+			<step>OCA\Bookmarks\Migration\CircleSharesUpdateRepairStep</step>
 		</post-migration>
 	</repair-steps>
 

lib/AppInfo/Application.php

@@ -86,9 +86,9 @@ public function register(IRegistrationContext $context): void {
 		$context->registerEventListener(UserAddedEvent::class, UsersGroupsCirclesListener::class);
 		$context->registerEventListener(UserRemovedEvent::class, UsersGroupsCirclesListener::class);
 		$context->registerEventListener(BeforeGroupDeletedEvent::class, UsersGroupsCirclesListener::class);
-		$context->registerEventListener('\OCA\Circles\Events\CircleMemberAddedEvent', UsersGroupsCirclesListener::class);
-		$context->registerEventListener('\OCA\Circles\Events\CircleMemberRemovedEvent', UsersGroupsCirclesListener::class);
-		$context->registerEventListener('\OCA\Circles\Events\CircleDestroyedEvent', UsersGroupsCirclesListener::class);
+		$context->registerEventListener('OCA\Circles\Events\MembershipsCreatedEvent', UsersGroupsCirclesListener::class);
+		$context->registerEventListener('OCA\Circles\Events\MembershipsRemovedEvent', UsersGroupsCirclesListener::class);
+		$context->registerEventListener('OCA\Circles\Events\DestroyingCircleEvent', UsersGroupsCirclesListener::class);
 
 		$context->registerEventListener(BeforeTemplateRenderedEvent::class, BeforeTemplateRenderedListener::class);
 

lib/Controller/FoldersController.php

@@ -125,10 +125,9 @@ private function _returnFolderAsArray($folder): array {
 			return $returnFolder;
 		}
 		if ($folder instanceof SharedFolder) {
-			$share = $this->shareMapper->findByFolderAndUser($folder->getFolderId(), $folder->getUserId());
 			$returnFolder = $folder->toArray();
 			$returnFolder['id'] = $folder->getFolderId();
-			$returnFolder['userId'] = $share->getOwner();
+			$returnFolder['userId'] = $this->folderMapper->find($folder->getFolderId())->getUserId();
 			$parent = $this->treeMapper->findParentOf(TreeMapper::TYPE_SHARE, $folder->getId());
 			$returnFolder['parent_folder'] = $this->toExternalFolderId($parent->getId());
 			$returnFolder['userDisplayName'] = $this->userManager->get($returnFolder['userId'])->getDisplayName();
@@ -735,11 +734,13 @@ public function findSharedFolders(): DataResponse {
 		} catch (Exception $e) {
 			return new Http\DataResponse(['status' => 'error', 'data' => ['Internal error']], Http::STATUS_INTERNAL_SERVER_ERROR);
 		}
-		return new Http\DataResponse(['status' => 'success', 'data' => array_map(function (Share $share) {
+		// The user may have the same folder through several shares
+		$folderIds = array_values(array_unique(array_map(static fn (Share $share) => $share->getFolderId(), $shares)));
+		return new Http\DataResponse(['status' => 'success', 'data' => array_map(static function (int $folderId) {
 			return [
-				'id' => $share->getFolderId(),
+				'id' => $folderId,
 			];
-		}, $shares)]);
+		}, $folderIds)]);
 	}
 
 	/**
@@ -798,15 +799,20 @@ public function getShares(int $folderId): DataResponse {
 		if (Authorizer::hasPermission(Authorizer::PERM_READ, $permissions) && $this->authorizer->getUserId() !== null) {
 			try {
 				$this->folderMapper->find($folderId);
-				$share = $this->shareMapper->findByFolderAndUser($folderId, $this->authorizer->getUserId());
+				$shares = $this->shareMapper->findByFolderAndUser($folderId, $this->authorizer->getUserId());
 			} catch (MultipleObjectsReturnedException|Exception) {
 				return new DataResponse(['status' => 'error', 'data' => ['Internal error']], Http::STATUS_INTERNAL_SERVER_ERROR);
 			} catch (DoesNotExistException $e) {
 				$res = new DataResponse(['status' => 'error', 'data' => ['Could not find folder']], Http::STATUS_NOT_FOUND);
 				$res->throttle(['action' => 'getShares']);
 				return $res;
 			}
-			return new Http\DataResponse(['status' => 'success', 'data' => [$share->toArray()]]);
+			if (count($shares) === 0) {
+				$res = new DataResponse(['status' => 'error', 'data' => ['Could not find folder']], Http::STATUS_NOT_FOUND);
+				$res->throttle(['action' => 'getShares']);
+				return $res;
+			}
+			return new Http\DataResponse(['status' => 'success', 'data' => array_map(static fn (Share $share) => $share->toArray(), $shares)]);
 		}
 		$res = new DataResponse(['status' => 'error', 'data' => ['Could not find folder']], Http::STATUS_NOT_FOUND);
 		$res->throttle(['action' => 'getShares']);

lib/Db/Share.php

@@ -79,18 +79,18 @@ public function toArray(): array {
 	}
 
 	private static function getParticipantDisplayName($type, $participant) {
+		// The participant may not exist anymore, e.g. if the user, group or circle was deleted,
+		// or the circles app was disabled
 		try {
-			if ($type === IShare::TYPE_USER) {
-				return \OCP\Server::get(IUserManager::class)->get($participant)->getDisplayName();
-			}
-			if ($type === IShare::TYPE_GROUP) {
-				return \OCP\Server::get(IGroupManager::class)->get($participant)->getDisplayName();
-			}
-			if ($type === IShare::TYPE_CIRCLE) {
-				return \OCP\Server::get(CirclesService::class)->getCircle($participant)->getName();
-			}
+			$displayName = match ($type) {
+				IShare::TYPE_USER => \OCP\Server::get(IUserManager::class)->get($participant)?->getDisplayName(),
+				IShare::TYPE_GROUP => \OCP\Server::get(IGroupManager::class)->get($participant)?->getDisplayName(),
+				IShare::TYPE_CIRCLE => \OCP\Server::get(CirclesService::class)->getCircle($participant)?->getName(),
+				default => null,
+			};
 		} catch (NotFoundExceptionInterface|ContainerExceptionInterface $e) {
-			return $participant;
+			$displayName = null;
 		}
+		return $displayName ?? $participant;
 	}
 }

lib/Db/ShareMapper.php

@@ -118,13 +118,14 @@ public function findByFolderAndParticipant(int $folderId, int $type, string $par
 	}
 
 	/**
+	 * Finds all shares of the folder that the user's shared folder is part of
+	 *
 	 * @param int $folderId
 	 * @param string $userId
-	 * @return Share
-	 * @throws DoesNotExistException
-	 * @throws MultipleObjectsReturnedException
+	 * @return Share[]
+	 * @throws Exception
 	 */
-	public function findByFolderAndUser(int $folderId, string $userId): Share {
+	public function findByFolderAndUser(int $folderId, string $userId): array {
 		$qb = $this->db->getQueryBuilder();
 		$qb->select(array_map(static function ($c) {
 			return 's.' . $c;
@@ -134,7 +135,7 @@ public function findByFolderAndUser(int $folderId, string $userId): Share {
 			->leftJoin('t', 'bookmarks_shared_folders', 'sf', 'sf.id = t.shared_folder_id')
 			->where($qb->expr()->eq('s.folder_id', $qb->createPositionalParameter($folderId, IQueryBuilder::PARAM_INT)))
 			->andWhere($qb->expr()->eq('sf.user_id', $qb->createPositionalParameter($userId)));
-		return $this->findEntity($qb);
+		return $this->findEntities($qb);
 	}
 
 	/**
@@ -178,19 +179,20 @@ public function insertOrUpdate(Entity $entity): Share {
 	}
 
 	/**
-	 * @throws DoesNotExistException
-	 * @throws MultipleObjectsReturnedException
+	 * Finds all shares that the shared folder is part of
+	 *
+	 * @return Share[]
 	 * @throws Exception
 	 */
-	public function findBySharedFolder(int $id): Share {
+	public function findBySharedFolder(int $id): array {
 		$qb = $this->db->getQueryBuilder();
 		$qb->select(array_map(static function ($c) {
 			return 's.' . $c;
 		}, Share::$columns))
 			->from('bookmarks_shares', 's')
 			->innerJoin('s', 'bookmarks_shared_to_shares', 't', 's.id = t.share_id')
 			->where($qb->expr()->eq('t.shared_folder_id', $qb->createPositionalParameter($id, IQueryBuilder::PARAM_INT)));
-		return $this->findEntity($qb);
+		return $this->findEntities($qb);
 	}
 
 	/**

lib/Db/SharedFolderMapper.php

@@ -9,6 +9,7 @@
 namespace OCA\Bookmarks\Db;
 
 use OCA\Bookmarks\Events\CreateEvent;
+use OCA\Bookmarks\Events\UpdateEvent;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Db\Entity;
 use OCP\AppFramework\Db\MultipleObjectsReturnedException;
@@ -96,7 +97,8 @@ public function findByFolder(int $folderId): array {
 	 */
 	public function findByOwner(string $userId): array {
 		$qb = $this->db->getQueryBuilder();
-		$qb->select(array_map(static function ($c) {
+		// A shared folder can be part of several shares of the same owner
+		$qb->selectDistinct(array_map(static function ($c) {
 			return 'p.' . $c;
 		}, SharedFolder::$columns))
 			->from('bookmarks_shared_folders', 'p')
@@ -174,7 +176,8 @@ public function findByFolderAndUser(int $folderId, string $userId): SharedFolder
 	 */
 	public function findByOwnerAndUser(string $owner, string $userId): array {
 		$qb = $this->db->getQueryBuilder();
-		$qb->select(array_map(static function ($c) {
+		// A shared folder can be part of several shares of the same owner
+		$qb->selectDistinct(array_map(static function ($c) {
 			return 'p.' . $c;
 		}, SharedFolder::$columns))
 			->from('bookmarks_shared_folders', 'p')
@@ -188,12 +191,12 @@ public function findByOwnerAndUser(string $owner, string $userId): array {
 	/**
 	 * @param int $shareId
 	 * @param string $userId
-	 * @return SharedFolder
-	 * @throws DoesNotExistException
-	 * @throws MultipleObjectsReturnedException
+	 * @return SharedFolder[]
 	 * @throws Exception
+	 *
+	 * @psalm-return array<array-key, SharedFolder>
 	 */
-	public function findByShareAndUser(int $shareId, string $userId): SharedFolder {
+	public function findByShareAndUser(int $shareId, string $userId): array {
 		$qb = $this->db->getQueryBuilder();
 		$qb->select(array_map(static function ($c) {
 			return 'p.' . $c;
@@ -202,7 +205,7 @@ public function findByShareAndUser(int $shareId, string $userId): SharedFolder {
 			->leftJoin('p', 'bookmarks_shared_to_shares', 't', 't.shared_folder_id = p.id')
 			->where($qb->expr()->eq('t.share_id', $qb->createPositionalParameter($shareId, IQueryBuilder::PARAM_INT)))
 			->andWhere($qb->expr()->eq('p.user_id', $qb->createPositionalParameter($userId)));
-		return $this->findEntity($qb);
+		return $this->findEntities($qb);
 	}
 
 	/**
@@ -244,6 +247,23 @@ public function mount(int $id, int $share_id): void {
 		));
 	}
 
+	/**
+	 * Removes the shared folder from a share. It may still be part of other shares of the same folder.
+	 *
+	 * @throws Exception
+	 */
+	public function unmount(int $id, int $shareId): void {
+		$qb = $this->db->getQueryBuilder();
+		$qb->delete('bookmarks_shared_to_shares')
+			->where($qb->expr()->eq('shared_folder_id', $qb->createPositionalParameter($id, IQueryBuilder::PARAM_INT)))
+			->andWhere($qb->expr()->eq('share_id', $qb->createPositionalParameter($shareId, IQueryBuilder::PARAM_INT)))
+			->executeStatement();
+		$this->eventDispatcher->dispatch(UpdateEvent::class, new UpdateEvent(
+			TreeMapper::TYPE_SHARE,
+			$id
+		));
+	}
+
 	/**
 	 * @return SharedFolder[]
 	 *

lib/Db/TreeMapper.php

@@ -650,12 +650,28 @@ public function deleteShare(int $shareId): void {
 		$share = $this->shareMapper->find($shareId);
 		$sharedFolders = $this->sharedFolderMapper->findByShare($shareId);
 		foreach ($sharedFolders as $sharedFolder) {
-			$this->sharedFolderMapper->delete($sharedFolder);
-			$this->deleteEntry(TreeMapper::TYPE_SHARE, $sharedFolder->getId());
+			$this->removeSharedFolderFromShare($sharedFolder, $shareId);
 		}
 		$this->shareMapper->delete($share);
 	}
 
+	/**
+	 * Removes the shared folder from the share. The shared folder is only deleted
+	 * if it isn't part of another share of the same folder.
+	 *
+	 * @throws DoesNotExistException
+	 * @throws MultipleObjectsReturnedException
+	 * @throws UnsupportedOperation|Exception
+	 */
+	public function removeSharedFolderFromShare(SharedFolder $sharedFolder, int $shareId): void {
+		$this->sharedFolderMapper->unmount($sharedFolder->getId(), $shareId);
+		if (count($this->shareMapper->findBySharedFolder($sharedFolder->getId())) > 0) {
+			return;
+		}
+		$this->sharedFolderMapper->delete($sharedFolder);
+		$this->deleteEntry(TreeMapper::TYPE_SHARE, $sharedFolder->getId());
+	}
+
 	/**
 	 * @param string $type
 	 * @psalm-param TreeMapper::TYPE_* $type
@@ -683,12 +699,12 @@ public function move(string $type, int $itemId, int $newParentFolderId, ?int $in
 		} else {
 			$sharedFolder = $this->sharedFolderMapper->find($itemId);
 			$folderId = $sharedFolder->getFolderId();
-			$share = $this->shareMapper->findBySharedFolder($sharedFolder->getId());
+			$owner = $this->folderMapper->find($folderId)->getUserId();
 
 			// Make sure that the sharer of this share doesn't have a share of the target folder or one of its parents
 			// would make a share loop very probable, which would be very bad. Breaks the whole app.
 
-			if ($this->isFolderSharedWithUser($newParentFolderId, $share->getOwner())) {
+			if ($this->isFolderSharedWithUser($newParentFolderId, $owner)) {
 				throw new UnsupportedOperation('Cannot nest a folder shared from user A inside a folder shared with user A');
 			}
 		}
@@ -996,18 +1012,18 @@ public function getSubFolders(int $folderId, $layers = 0, ?bool $isSoftDeleted =
 		}, $this->findChildren(TreeMapper::TYPE_FOLDER, $folderId, $isSoftDeleted));
 		$shares = array_map(function (SharedFolder $sharedFolder) use ($layers, $folderId, $isSoftDeleted) {
 			try {
-				$share = $this->shareMapper->findBySharedFolder($sharedFolder->getId());
+				$folder = $this->folderMapper->find($sharedFolder->getFolderId());
 			} catch (DoesNotExistException|MultipleObjectsReturnedException|Exception $e) {
 				$this->logger->error('Failed to load a shared folder', ['exception' => $e]);
 				return null;
 			}
 			$array = $sharedFolder->toArray();
-			$array['id'] = $share->getFolderId();
-			$array['userId'] = $share->getOwner();
+			$array['id'] = $folder->getId();
+			$array['userId'] = $folder->getUserId();
 			$array['userDisplayName'] = $this->userManager->get($array['userId'])->getDisplayName();
 			$array['parent_folder'] = $folderId;
 			if ($layers !== 0) {
-				$array['children'] = $this->getSubFolders($share->getFolderId(), $layers - 1, $isSoftDeleted);
+				$array['children'] = $this->getSubFolders($folder->getId(), $layers - 1, $isSoftDeleted);
 			}
 			return $array;
 		}, $this->findChildren(TreeMapper::TYPE_SHARE, $folderId, $isSoftDeleted));