Merge pull request #35010 from owncloud/improve-polling

Filter unavailable users and remove unavailable shares on polling
owncloud · Apr 22, 2019 · bd98610 · bd98610
2 parents ec033b2 + 103313a
commit bd98610
Show file tree

Hide file tree

Showing 4 changed files with 150 additions and 18 deletions.
diff --git a/apps/federatedfilesharing/lib/AppInfo/Application.php b/apps/federatedfilesharing/lib/AppInfo/Application.php
@@ -164,15 +164,18 @@ 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;
 				}
 
 				return new PollIncomingShares(
 					$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
@@ -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,16 +91,36 @@ 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) {
 				try {
 					/** @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
@@ -22,16 +22,20 @@
 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;
 use OCP\Files\StorageNotAvailableException;
 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
@@ -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('');