refactor(integration): Refactor and fix federation integration tests

Signed-off-by: Carl Schwan <carl.schwan@nextcloud.com>

Author: CarlSchwan

apps/federatedfilesharing/lib/FederatedShareProvider.php

@@ -63,24 +63,19 @@ public function __construct(
 	) {
 	}
 
-	/**
-	 * Return the identifier of this provider.
-	 *
-	 * @return string Containing only [a-zA-Z0-9]
-	 */
-	public function identifier() {
+	#[Override]
+	public function identifier(): string {
 		return 'ocFederatedSharing';
 	}
 
 	/**
 	 * Share a path
 	 *
-	 * @param IShare $share
-	 * @return IShare The share object
 	 * @throws ShareNotFound
 	 * @throws \Exception
 	 */
-	public function create(IShare $share) {
+	#[Override]
+	public function create(IShare $share): IShare {
 		$shareWith = $share->getSharedWith();
 		$itemSource = $share->getNodeId();
 		$itemType = $share->getNodeType();
@@ -141,7 +136,7 @@ public function create(IShare $share) {
 		if ($remoteShare) {
 			try {
 				$ownerCloudId = $this->cloudIdManager->getCloudId($remoteShare['owner'], $remoteShare['remote']);
-				$shareId = (string)$this->addShareToDB($itemSource, $itemType, $shareWith, $sharedBy, $ownerCloudId->getId(), $permissions, 'tmp_token_' . time(), $shareType, $expirationDate);
+				$shareId = $this->addShareToDB($itemSource, $itemType, $shareWith, $sharedBy, $ownerCloudId->getId(), $permissions, 'tmp_token_' . time(), $shareType, $expirationDate);
 				$share->setId($shareId);
 				[$token, $remoteId] = $this->askOwnerToReShare($shareWith, $share, $shareId);
 				// remote share was create successfully if we get a valid token as return
@@ -176,7 +171,7 @@ public function create(IShare $share) {
 	 */
 	protected function createFederatedShare(IShare $share): string {
 		$token = $this->tokenHandler->generateToken();
-		$shareId = (string)$this->addShareToDB(
+		$shareId = $this->addShareToDB(
 			$share->getNodeId(),
 			$share->getNodeType(),
 			$share->getSharedWith(),
@@ -292,9 +287,8 @@ protected function getShareFromExternalShareTable(IShare $share) {
 	 * @param string $token
 	 * @param int $shareType
 	 * @param \DateTime $expirationDate
-	 * @return int
 	 */
-	private function addShareToDB($itemSource, $itemType, $shareWith, $sharedBy, $uidOwner, $permissions, $token, $shareType, $expirationDate) {
+	private function addShareToDB($itemSource, $itemType, $shareWith, $sharedBy, $uidOwner, $permissions, $token, $shareType, $expirationDate): string {
 		$qb = $this->dbConnection->getQueryBuilder();
 		$qb->insert('share')
 			->setValue('share_type', $qb->createNamedParameter($shareType))
@@ -316,7 +310,7 @@ private function addShareToDB($itemSource, $itemType, $shareWith, $sharedBy, $ui
 		$qb->setValue('file_target', $qb->createNamedParameter(''));
 
 		$qb->executeStatement();
-		return $qb->getLastInsertId();
+		return (string)$qb->getLastInsertId();
 	}
 
 	/**

apps/files_sharing/lib/Command/CleanupRemoteStorages.php

@@ -7,6 +7,7 @@
  */
 namespace OCA\Files_Sharing\Command;
 
+use OCA\Files_Sharing\External\ExternalShareMapper;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\Federation\ICloudIdManager;
 use OCP\IDBConnection;
@@ -22,13 +23,14 @@
 class CleanupRemoteStorages extends Command {
 
 	public function __construct(
-		protected IDBConnection $connection,
-		private ICloudIdManager $cloudIdManager,
+		protected readonly IDBConnection $connection,
+		private readonly ICloudIdManager $cloudIdManager,
+		private readonly ExternalShareMapper $externalShareMapper,
 	) {
 		parent::__construct();
 	}
 
-	protected function configure() {
+	protected function configure(): void {
 		$this
 			->setName('sharing:cleanup-remote-storages')
 			->setDescription('Cleanup shared storage entries that have no matching entry in the shares_external table')
@@ -37,6 +39,12 @@ protected function configure() {
 				null,
 				InputOption::VALUE_NONE,
 				'only show which storages would be deleted'
+			)
+			->addOption(
+				'all',
+				null,
+				InputOption::VALUE_NONE,
+				'Delete every external shares and their storage',
 			);
 	}
 
@@ -46,11 +54,12 @@ public function execute(InputInterface $input, OutputInterface $output): int {
 		$output->writeln(count($remoteStorages) . ' remote storage(s) need(s) to be checked');
 
 		$remoteShareIds = $this->getRemoteShareIds();
+		$all = $input->getOption('all');
 
 		$output->writeln(count($remoteShareIds) . ' remote share(s) exist');
 
 		foreach ($remoteShareIds as $id => $remoteShareId) {
-			if (isset($remoteStorages[$remoteShareId])) {
+			if ($all || isset($remoteStorages[$remoteShareId])) {
 				if ($input->getOption('dry-run') || $output->isVerbose()) {
 					$output->writeln("<info>$remoteShareId belongs to remote share $id</info>");
 				}
@@ -74,10 +83,14 @@ public function execute(InputInterface $input, OutputInterface $output): int {
 				}
 			}
 		}
-		return 0;
+
+		if ($all) {
+			$this->externalShareMapper->deleteAll();
+		}
+		return Command::SUCCESS;
 	}
 
-	public function countFiles($numericId, OutputInterface $output) {
+	public function countFiles($numericId, OutputInterface $output): void {
 		$queryBuilder = $this->connection->getQueryBuilder();
 		$queryBuilder->select($queryBuilder->func()->count('fileid'))
 			->from('filecache')
@@ -92,7 +105,7 @@ public function countFiles($numericId, OutputInterface $output) {
 		$output->writeln("$count files can be deleted for storage $numericId");
 	}
 
-	public function deleteStorage($id, $numericId, OutputInterface $output) {
+	public function deleteStorage($id, $numericId, OutputInterface $output): void {
 		$queryBuilder = $this->connection->getQueryBuilder();
 		$queryBuilder->delete('storages')
 			->where($queryBuilder->expr()->eq(
@@ -106,7 +119,7 @@ public function deleteStorage($id, $numericId, OutputInterface $output) {
 		$this->deleteFiles($numericId, $output);
 	}
 
-	public function deleteFiles($numericId, OutputInterface $output) {
+	public function deleteFiles($numericId, OutputInterface $output): void {
 		$queryBuilder = $this->connection->getQueryBuilder();
 		$queryBuilder->delete('filecache')
 			->where($queryBuilder->expr()->eq(
@@ -119,7 +132,7 @@ public function deleteFiles($numericId, OutputInterface $output) {
 		$output->writeln("deleted $count files");
 	}
 
-	public function getRemoteStorages() {
+	private function getRemoteStorages(): array {
 		$queryBuilder = $this->connection->getQueryBuilder();
 		$queryBuilder->select(['id', 'numeric_id'])
 			->from('storages')
@@ -148,7 +161,10 @@ public function getRemoteStorages() {
 		return $remoteStorages;
 	}
 
-	public function getRemoteShareIds() {
+	/**
+	 * @return array<string, string>
+	 */
+	private function getRemoteShareIds(): array {
 		$queryBuilder = $this->connection->getQueryBuilder();
 		$queryBuilder->select(['id', 'share_token', 'owner', 'remote'])
 			->from('share_external');
@@ -159,7 +175,6 @@ public function getRemoteShareIds() {
 		while ($row = $result->fetchAssociative()) {
 			$cloudId = $this->cloudIdManager->getCloudId($row['owner'], $row['remote']);
 			$remote = $cloudId->getRemote();
-
 			$remoteShareIds[$row['id']] = 'shared::' . md5($row['share_token'] . '@' . $remote);
 		}
 		$result->closeCursor();

apps/files_sharing/lib/Controller/RemoteController.php

@@ -17,8 +17,6 @@
 use OCP\AppFramework\OCS\OCSNotFoundException;
 use OCP\AppFramework\OCSController;
 use OCP\Files\IRootFolder;
-use OCP\Files\NotFoundException;
-use OCP\Files\NotPermittedException;
 use OCP\IRequest;
 use Psr\Log\LoggerInterface;
 
@@ -114,7 +112,7 @@ private function extendShareInfo(ExternalShare $share): array {
 
 		try {
 			$mountPointNode = $userFolder->get($share->getMountpoint());
-		} catch (NotPermittedException|NotFoundException) {
+		} catch (\Exception) {
 			return $share->jsonSerialize();
 		}
 
@@ -182,7 +180,7 @@ public function unshare(string $id): DataResponse {
 			throw new OCSNotFoundException('Share does not exist');
 		}
 
-		$mountPoint = '/' . $this->userId . '/files' . $shareInfo->getMountPoint();
+		$mountPoint = '/' . $this->userId . '/files' . $shareInfo->getMountpoint();
 
 		if ($this->externalManager->removeShare($mountPoint) === true) {
 			return new DataResponse();

apps/files_sharing/lib/External/ExternalShare.php

@@ -31,8 +31,8 @@
  * @method void setRemoteId(string $remoteId)
  * @method string getShareToken()
  * @method void setShareToken(string $shareToken)
- * @method string getPassword()
- * @method void setPassword(string $password)
+ * @method string|null getPassword()
+ * @method void setPassword(?string $password)
  * @method string getName()
  * @method string getOwner()
  * @method void setOwner(string $owner)
@@ -97,7 +97,7 @@ public function jsonSerialize(): array {
 		$parent = $this->getParent();
 		return [
 			'id' => $this->getId(),
-			'parent' => $parent === '-1' ? null : $parent,
+			'parent' => $parent,
 			'share_type' => $this->getShareType() ?? IShare::TYPE_USER, // unfortunately nullable on the DB level, but never null.
 			'remote' => $this->getRemote(),
 			'remote_id' => $this->getRemoteId(),
@@ -106,7 +106,7 @@ public function jsonSerialize(): array {
 			'owner' => $this->getOwner(),
 			'user' => $this->getUser(),
 			'mountpoint' => $this->getMountpoint(),
-			'accepted' => $this->getAccepted() === IShare::STATUS_ACCEPTED,
+			'accepted' => $this->getAccepted(),
 
 			// Added later on
 			'file_id' => null,

apps/files_sharing/lib/External/ExternalShareMapper.php

@@ -75,10 +75,6 @@ public function getUserShare(ExternalShare $parentShare, IUser $user): ExternalS
 		return $this->findEntity($qb);
 	}
 
-	public function get() {
-
-	}
-
 	public function getByMountPointAndUser(string $mountPoint, IUser $user) {
 		$qb = $this->db->getQueryBuilder();
 		$qb->select('*')
@@ -151,6 +147,17 @@ public function deleteGroupShares(IGroup $group): void {
 		}
 	}
 
+	/**
+	 * @return \Generator<ExternalShare>
+	 */
+	public function getAllShares(): \Generator {
+		$qb = $this->db->getQueryBuilder();
+		$qb->select('*')
+			->from(self::TABLE_NAME);
+
+		return $this->yieldEntities($qb);
+	}
+
 	/**
 	 * Return a list of shares for the user.
 	 *
@@ -201,4 +208,10 @@ public function getShares(IUser $user, ?int $status): array {
 		}
 		return array_values($shares);
 	}
+
+	public function deleteAll(): void {
+		$qb = $this->db->getQueryBuilder();
+		$qb->delete('share_external')
+			->executeStatement();
+	}
 }

apps/files_sharing/lib/ResponseDefinitions.php

@@ -81,15 +81,15 @@
  * }
  *
  * @psalm-type Files_SharingRemoteShare = array{
- *     accepted: bool,
+ *     accepted: int,
  *     file_id: int|null,
  *     id: string,
  *     mimetype: string|null,
  *     mountpoint: string,
  *     mtime: int|null,
  *     name: string,
  *     owner: string,
- *     parent: string|null,
+ *     parent: string,
  *     permissions: int|null,
  *     remote: string,
  *     remote_id: string,

apps/files_sharing/tests/CapabilitiesTest.php

@@ -19,11 +19,9 @@
 use OCP\IConfig;
 use OCP\IDateTimeZone;
 use OCP\IGroupManager;
-use OCP\IURLGenerator;
 use OCP\IUserManager;
 use OCP\IUserSession;
 use OCP\L10N\IFactory;
-use OCP\Mail\IMailer;
 use OCP\Security\IHasher;
 use OCP\Security\ISecureRandom;
 use OCP\Share\IProviderFactory;

apps/files_sharing/tests/Command/CleanupRemoteStoragesTest.php

@@ -15,21 +15,17 @@
 use OCP\IDBConnection;
 use OCP\Server;
 use OCP\Snowflake\IGenerator;
+use PHPUnit\Framework\Attributes\Group;
 use PHPUnit\Framework\MockObject\MockObject;
 use Symfony\Component\Console\Input\InputInterface;
 use Symfony\Component\Console\Output\OutputInterface;
 use Test\TestCase;
 
-/**
- * Class CleanupRemoteStoragesTest
- *
- *
- * @package OCA\Files_Sharing\Tests\Command
- */
-#[\PHPUnit\Framework\Attributes\Group('DB')]
+#[Group('DB')]
 class CleanupRemoteStoragesTest extends TestCase {
 
 	protected IDBConnection $connection;
+	protected ExternalShareMapper $mapper;
 	protected CleanupRemoteStorages $command;
 	private ICloudIdManager&MockObject $cloudIdManager;
 
@@ -47,6 +43,7 @@ protected function setUp(): void {
 		parent::setUp();
 
 		$this->connection = Server::get(IDBConnection::class);
+		$this->mapper = Server::get(ExternalShareMapper::class);
 
 		$storageQuery = Server::get(IDBConnection::class)->getQueryBuilder();
 		$storageQuery->insert('storages')
@@ -74,7 +71,7 @@ protected function setUp(): void {
 				$externalShare->setOwner('irrelevant');
 				$externalShare->setUser($storage['user']);
 				$externalShare->setMountpoint('irrelevant');
-				Server::get(ExternalShareMapper::class)->insert($externalShare);
+				$this->mapper->insert($externalShare);
 			}
 
 			if (isset($storage['files_count'])) {
@@ -89,7 +86,7 @@ protected function setUp(): void {
 
 		$this->cloudIdManager = $this->createMock(ICloudIdManager::class);
 
-		$this->command = new CleanupRemoteStorages($this->connection, $this->cloudIdManager);
+		$this->command = new CleanupRemoteStorages($this->connection, $this->cloudIdManager, $this->mapper);
 	}
 
 	protected function tearDown(): void {

build/integration/features/bootstrap/AppConfiguration.php

@@ -7,6 +7,7 @@
  */
 use Behat\Behat\Hook\Scope\AfterScenarioScope;
 use Behat\Behat\Hook\Scope\BeforeScenarioScope;
+use Behat\Gherkin\Node\TableNode;
 use PHPUnit\Framework\Assert;
 use Psr\Http\Message\ResponseInterface;
 
@@ -19,8 +20,8 @@ trait AppConfiguration {
 	/** @var ResponseInterface */
 	private $response = null;
 
-	abstract public function sendingTo($verb, $url);
-	abstract public function sendingToWith($verb, $url, $body);
+	abstract public function sendingTo(string $verb, string $url);
+	abstract public function sendingToWith(string $verb, string $url, ?TableNode $body);
 	abstract public function theOCSStatusCodeShouldBe($statusCode);
 	abstract public function theHTTPStatusCodeShouldBe($statusCode);