diff --git a/apps/federatedfilesharing/lib/AppInfo/Application.php b/apps/federatedfilesharing/lib/AppInfo/Application.php index d50dfc7c9ae5..e89305162cd7 100644 --- a/apps/federatedfilesharing/lib/AppInfo/Application.php +++ b/apps/federatedfilesharing/lib/AppInfo/Application.php @@ -164,8 +164,10 @@ function ($c) { function ($c) use ($server) { if ($server->getAppManager()->isEnabledForUser('files_sharing')) { $sharingApp = new \OCA\Files_Sharing\AppInfo\Application(); + $externalManager = $sharingApp->getContainer()->query('ExternalManager'); $externalMountProvider = $sharingApp->getContainer()->query('ExternalMountProvider'); } else { + $externalManager = null; $externalMountProvider = null; } @@ -173,6 +175,7 @@ function ($c) use ($server) { $server->getDatabaseConnection(), $server->getUserManager(), \OC\Files\Filesystem::getLoader(), + $externalManager, $externalMountProvider ); } diff --git a/apps/federatedfilesharing/lib/Command/PollIncomingShares.php b/apps/federatedfilesharing/lib/Command/PollIncomingShares.php index 4ad70b6d5dd3..dfcbd19a2aeb 100644 --- a/apps/federatedfilesharing/lib/Command/PollIncomingShares.php +++ b/apps/federatedfilesharing/lib/Command/PollIncomingShares.php @@ -22,7 +22,11 @@ namespace OCA\FederatedFileSharing\Command; use OC\ServerNotAvailableException; +use OC\User\NoUserException; +use OCA\FederatedFileSharing\FederatedShareProvider; +use OCA\Files_Sharing\External\Manager; use OCA\Files_Sharing\External\MountProvider; +use OCP\Files\Mount\IMountManager; use OCP\Files\Storage\IStorage; use OCP\Files\Storage\IStorageFactory; use OCP\Files\StorageInvalidException; @@ -44,7 +48,11 @@ class PollIncomingShares extends Command { /** @var IStorageFactory */ private $loader; + /** @var Manager */ + private $externalManager; + /** @var MountProvider | null */ + private $externalMountProvider; /** @@ -55,11 +63,12 @@ class PollIncomingShares extends Command { * @param MountProvider $externalMountProvider * @param IStorageFactory $loader */ - public function __construct(IDBConnection $dbConnection, IUserManager $userManager, IStorageFactory $loader, MountProvider $externalMountProvider = null) { + public function __construct(IDBConnection $dbConnection, IUserManager $userManager, IStorageFactory $loader, Manager $externalManager = null, MountProvider $externalMountProvider = null) { parent::__construct(); $this->dbConnection = $dbConnection; $this->userManager = $userManager; $this->loader = $loader; + $this->externalManager = $externalManager; $this->externalMountProvider = $externalMountProvider; } @@ -82,6 +91,13 @@ public function execute(InputInterface $input, OutputInterface $output) { $cursor = $this->getCursor(); while ($data = $cursor->fetch()) { $user = $this->userManager->get($data['user']); + if ($user === null) { + $output->writeln( + "Skipping user \"{$data['user']}\". Reason: user manager was unable to resolve the uid into the user object" + ); + continue; + } + $userMounts = $this->externalMountProvider->getMountsForUser($user, $this->loader); /** @var \OCA\Files_Sharing\External\Mount $mount */ foreach ($userMounts as $mount) { @@ -89,9 +105,22 @@ public function execute(InputInterface $input, OutputInterface $output) { /** @var Storage $storage */ $storage = $mount->getStorage(); $this->refreshStorageRoot($storage); + } catch (NoUserException $e) { + $shareData = $this->getExternalShareData($data['user'], $mount->getMountPoint()); + $entryId = $shareData['id']; + $remote = $shareData['remote']; + // uid was null so we need to set it + $this->externalManager->setUid($data['user']); + $this->externalManager->removeShare($mount->getMountPoint()); + // and now we need to reset uid back to null + $this->externalManager->setUid(null); + $output->writeln( + "Remote \"$remote\" reports that external share with id \"$entryId\" no longer exists. Removing it.." + ); } catch (\Exception $e) { - $entryId = $this->getExternalShareId($data['user'], $mount->getMountPoint()); - $remote = $storage->getRemote(); + $shareData = $this->getExternalShareData($data['user'], $mount->getMountPoint()); + $entryId = $shareData['id']; + $remote = $shareData['remote']; $reason = $e->getMessage(); $output->writeln( "Skipping external share with id \"$entryId\" from remote \"$remote\". Reason: \"$reason\"" @@ -130,13 +159,13 @@ protected function getCursor() { return $qb->execute(); } - protected function getExternalShareId(string $userId, string $mountPoint) { + protected function getExternalShareData(string $userId, string $mountPoint) { $relativeMountPoint =\rtrim( \substr($mountPoint, \strlen("/$userId/files")), '/' ); $qb = $this->dbConnection->getQueryBuilder(); - $qb->selectDistinct('id') + $qb->select('*') ->from('share_external') ->where( $qb->expr()->eq('user', @@ -148,6 +177,6 @@ protected function getExternalShareId(string $userId, string $mountPoint) { )); $result = $qb->execute(); $externalShare = $result->fetch(); - return $externalShare['id'] ?? 0; + return $externalShare; } } diff --git a/apps/federatedfilesharing/tests/Command/PollIncomingSharesTest.php b/apps/federatedfilesharing/tests/Command/PollIncomingSharesTest.php index 3b9dd52bfc54..2c146df53b9a 100644 --- a/apps/federatedfilesharing/tests/Command/PollIncomingSharesTest.php +++ b/apps/federatedfilesharing/tests/Command/PollIncomingSharesTest.php @@ -22,9 +22,12 @@ namespace OCA\FederatedFileSharing\Tests\Command; use Doctrine\DBAL\Driver\Statement; +use OC\User\NoUserException; use OCA\FederatedFileSharing\Tests\TestCase; use OCA\FederatedFileSharing\Command\PollIncomingShares; +use OCA\Files_Sharing\External\Manager; use OCA\Files_Sharing\External\MountProvider; +use OCA\Files_Sharing\External\Storage; use OCP\DB\QueryBuilder\IExpressionBuilder; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\Files\Storage\IStorageFactory; @@ -32,6 +35,7 @@ use OCP\IDBConnection; use OCP\IUser; use OCP\IUserManager; +use PHPUnit\Framework\MockObject\MockObject; use Symfony\Component\Console\Tester\CommandTester; /** @@ -44,25 +48,31 @@ class PollIncomingSharesTest extends TestCase { /** @var CommandTester */ private $commandTester; - /** @var IDBConnection | \PHPUnit_Framework_MockObject_MockObject */ + /** @var IDBConnection | MockObject */ private $dbConnection; - /** @var IUserManager | \PHPUnit_Framework_MockObject_MockObject */ + /** @var IUserManager | MockObject */ private $userManager; - /** @var MountProvider | \PHPUnit_Framework_MockObject_MockObject */ + /** @var MountProvider | MockObject */ private $externalMountProvider; - /** @var IStorageFactory | \PHPUnit_Framework_MockObject_MockObject */ + /** @var IStorageFactory | MockObject */ private $loader; + /** + * @var Manager | MockObject + */ + private $externalManager; + protected function setUp() { parent::setUp(); $this->dbConnection = $this->createMock(IDBConnection::class); $this->userManager = $this->createMock(IUserManager::class); - $this->externalMountProvider = $this->createMock(MountProvider::class); $this->loader = $this->createMock(IStorageFactory::class); - $command = new PollIncomingShares($this->dbConnection, $this->userManager, $this->loader, $this->externalMountProvider); + $this->externalManager = $this->createMock(Manager::class); + $this->externalMountProvider = $this->createMock(MountProvider::class); + $command = new PollIncomingShares($this->dbConnection, $this->userManager, $this->loader, $this->externalManager, $this->externalMountProvider); $this->commandTester = new CommandTester($command); } @@ -92,7 +102,7 @@ public function testNoSharesPoll() { } public function testWithFilesSharingDisabled() { - $command = new PollIncomingShares($this->dbConnection, $this->userManager, $this->loader, null); + $command = new PollIncomingShares($this->dbConnection, $this->userManager, $this->loader, $this->externalManager, null); $this->commandTester = new CommandTester($command); $this->commandTester->execute([]); $output = $this->commandTester->getDisplay(); @@ -103,8 +113,13 @@ public function testUnavailableStorage() { $uid = 'foo'; $exprBuilder = $this->createMock(IExpressionBuilder::class); $statementMock = $this->createMock(Statement::class); - $statementMock->method('fetch')->willReturnOnConsecutiveCalls(['user' => $uid], false); + $statementMock->method('fetch')->willReturnOnConsecutiveCalls( + ['user' => $uid], + ['id' => 50, 'remote' => 'example.org'], + false + ); $qbMock = $this->createMock(IQueryBuilder::class); + $qbMock->method('select')->willReturnSelf(); $qbMock->method('selectDistinct')->willReturnSelf(); $qbMock->method('from')->willReturnSelf(); $qbMock->method('where')->willReturnSelf(); @@ -114,8 +129,8 @@ public function testUnavailableStorage() { $userMock = $this->createMock(IUser::class); $this->userManager->expects($this->once())->method('get') ->with($uid)->willReturn($userMock); - - $storage = $this->createMock(\OCA\Files_Sharing\External\Storage::class); + + $storage = $this->createMock(Storage::class); $storage->method('hasUpdated')->willThrowException(new StorageNotAvailableException('Ooops')); $storage->method('getRemote')->willReturn('example.org'); @@ -129,7 +144,71 @@ public function testUnavailableStorage() { $this->commandTester->execute([]); $output = $this->commandTester->getDisplay(); $this->assertContains( - 'Skipping external share with id "0" from remote "example.org". Reason: "Ooops"', + 'Skipping external share with id "50" from remote "example.org". Reason: "Ooops"', + $output + ); + } + + public function testNotExistingUser() { + $uid = 'foo'; + $exprBuilder = $this->createMock(IExpressionBuilder::class); + $statementMock = $this->createMock(Statement::class); + $statementMock->method('fetch')->willReturnOnConsecutiveCalls(['user' => $uid], false); + $qbMock = $this->createMock(IQueryBuilder::class); + $qbMock->method('selectDistinct')->willReturnSelf(); + $qbMock->method('from')->willReturnSelf(); + $qbMock->method('where')->willReturnSelf(); + $qbMock->method('expr')->willReturn($exprBuilder); + $qbMock->method('execute')->willReturn($statementMock); + + $this->externalMountProvider->expects($this->never())->method('getMountsForUser'); + + $this->dbConnection->method('getQueryBuilder')->willReturn($qbMock); + $this->commandTester->execute([]); + $output = $this->commandTester->getDisplay(); + $this->assertContains( + 'Skipping user "foo". Reason: user manager was unable to resolve the uid into the user object', + $output + ); + } + + public function testPollingUnsharedMount() { + $uid = 'foo'; + $exprBuilder = $this->createMock(IExpressionBuilder::class); + $statementMock = $this->createMock(Statement::class); + $statementMock->method('fetch')->willReturnOnConsecutiveCalls( + ['user' => $uid], + ['id' => 50, 'remote' => 'example.org'], + false + ); + $qbMock = $this->createMock(IQueryBuilder::class); + $qbMock->method('select')->willReturnSelf(); + $qbMock->method('selectDistinct')->willReturnSelf(); + $qbMock->method('from')->willReturnSelf(); + $qbMock->method('where')->willReturnSelf(); + $qbMock->method('expr')->willReturn($exprBuilder); + $qbMock->method('execute')->willReturn($statementMock); + + $userMock = $this->createMock(IUser::class); + $this->userManager->expects($this->once())->method('get') + ->with($uid)->willReturn($userMock); + + $this->externalManager->expects($this->once())->method('removeShare'); + + $storage = $this->createMock(Storage::class); + $storage->method('hasUpdated')->willThrowException(new NoUserException('Ooops')); + + $mount = $this->createMock(\OCA\Files_Sharing\External\Mount::class); + $mount->method('getStorage')->willReturn($storage); + $mount->method('getMountPoint')->willReturn("/$uid/files/point"); + $this->externalMountProvider->expects($this->once())->method('getMountsForUser') + ->willReturn([$mount]); + + $this->dbConnection->method('getQueryBuilder')->willReturn($qbMock); + $this->commandTester->execute([]); + $output = $this->commandTester->getDisplay(); + $this->assertContains( + 'Remote "example.org" reports that external share with id "50" no longer exists. Removing it..', $output ); } diff --git a/apps/files_sharing/lib/External/Manager.php b/apps/files_sharing/lib/External/Manager.php index cd25cb0cc158..4a411cad15d8 100644 --- a/apps/files_sharing/lib/External/Manager.php +++ b/apps/files_sharing/lib/External/Manager.php @@ -30,6 +30,7 @@ namespace OCA\Files_Sharing\External; use OC\Files\Filesystem; +use OC\User\NoUserException; use OCP\Files; use OCP\Notification\IManager; use OCP\Share\Events\AcceptShare; @@ -257,7 +258,7 @@ public function processNotification($remoteShare) { * @return string */ protected function stripPath($path) { - $prefix = '/' . $this->uid . '/files'; + $prefix = "/{$this->uid}/files"; return \rtrim(\substr($path, \strlen($prefix)), '/'); } @@ -308,7 +309,27 @@ public function setMountPoint($source, $target) { return $result; } + /** + * Explicitly set uid when the shares are managed in CLI + * + * @param string|null $uid + */ + public function setUid($uid) { + // FIXME: External manager should not depend on uid + $this->uid = $uid; + } + + /** + * @param $mountPoint + * @return bool + * + * @throws NoUserException + */ public function removeShare($mountPoint) { + if ($this->uid === null) { + throw new NoUserException(); + } + $mountPointObj = $this->mountManager->find($mountPoint); $id = $mountPointObj->getStorage()->getCache()->getId('');