Merge pull request #141 from nextcloud/backport/138/stable20v1.9.1

[stable20] Check all nodes and all mountpoints for delete permissions

4 files changed, 162 insertions, 14 deletions

diff --git a/.github/workflows/phpunit.yml b/.github/workflows/phpunit.yml
index fb7ed02..1036f11 100644
--- a/.github/workflows/phpunit.yml
+++ b/.github/workflows/phpunit.yml
@@ -82,7 +82,7 @@ jobs:
 
     services:
       mysql:
-        image: mariadb
+        image: mariadb:10.5
         ports:
           - 4444:3306/tcp
         env:
diff --git a/lib/BackgroundJob/RetentionJob.php b/lib/BackgroundJob/RetentionJob.php
index 7a894cf..2d1215e 100644
--- a/lib/BackgroundJob/RetentionJob.php
+++ b/lib/BackgroundJob/RetentionJob.php
@@ -31,7 +31,9 @@ use OCA\Files_Retention\AppInfo\Application;
 use OCA\Files_Retention\Constants;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\BackgroundJob\IJobList;
+use OCP\Files\Config\ICachedMountFileInfo;
 use OCP\Files\Config\IUserMountCache;
+use OCP\Files\NotPermittedException;
 use OCP\IConfig;
 use OCP\IDBConnection;
 use OCP\Files\Node;
@@ -195,8 +197,21 @@ class RetentionJob extends TimedJob {
 			throw new NotFoundException("No mount points found for file $fileId");
 		}
 
-		$mountPoint = array_shift($mountPoints);
+		foreach ($mountPoints as $mountPoint) {
+			try {
+				return $this->getDeletableNodeFromMountPoint($mountPoint, $fileId);
+			} catch (NotPermittedException $e) {
+				// Check the next mount point
+				$this->logger->debug('Mount point ' . ($mountPoint->getMountId() ?? 'null') . ' has no delete permissions for file ' . $fileId);
+			} catch (NotFoundException $e) {
+				// Already logged explicitly inside
+			}
+		}
+
+		throw new NotFoundException("No mount point with delete permissions found for file $fileId");
+	}
 
+	protected function getDeletableNodeFromMountPoint(ICachedMountFileInfo $mountPoint, int $fileId): Node {
 		try {
 			$userId = $mountPoint->getUser()->getUID();
 			$userFolder = $this->rootFolder->getUserFolder($userId);
@@ -217,7 +232,14 @@ class RetentionJob extends TimedJob {
 			throw new NotFoundException('No node for file ' . $fileId . ' and user ' . $userId);
 		}
 
-		return array_shift($nodes);
+		foreach ($nodes as $node) {
+			if ($node->isDeletable()) {
+				return $node;
+			}
+			$this->logger->debug('Mount point ' . ($mountPoint->getMountId() ?? 'null') . ' has access to node ' . $node->getId() . ' but permissions are ' . $node->getPermissions());
+		}
+
+		throw new NotPermittedException();
 	}
 
 	private function expireNode(Node $node, \DateTime $deleteBefore): bool {
diff --git a/tests/bootstrap.php b/tests/bootstrap.php
index 97cde5a..86e5c45 100644
--- a/tests/bootstrap.php
+++ b/tests/bootstrap.php
@@ -31,6 +31,6 @@ require_once __DIR__ . '/../../../lib/base.php';
 if (!class_exists('\PHPUnit\Framework\TestCase')) {
 	require_once('PHPUnit/Autoload.php');
 }
-\OC_App::enable('files_retention');
+(new \OC_App())->enable('files_retention');
 \OC_App::loadApp('files_retention');
 OC_Hook::clear();
diff --git a/tests/lib/BackgroundJob/RetentionJobTest.php b/tests/lib/BackgroundJob/RetentionJobTest.php
index 1374f34..9e5fe6c 100644
--- a/tests/lib/BackgroundJob/RetentionJobTest.php
+++ b/tests/lib/BackgroundJob/RetentionJobTest.php
@@ -1,4 +1,6 @@
 <?php
+
+declare(strict_types=1);
 /**
  * @copyright 2017, Roeland Jago Douma <roeland@famdouma.nl>
  *
@@ -26,7 +28,7 @@ use OCA\Files_Retention\BackgroundJob\RetentionJob;
 use OCA\Files_Retention\Constants;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\BackgroundJob\IJobList;
-use OCP\Files\Config\ICachedMountInfo;
+use OCP\Files\Config\ICachedMountFileInfo;
 use OCP\Files\Config\IUserMountCache;
 use OCP\Files\Folder;
 use OCP\Files\Node;
@@ -39,6 +41,7 @@ use OCP\SystemTag\ISystemTagManager;
 use OCP\SystemTag\ISystemTagObjectMapper;
 use OCP\IUser;
 use OCP\SystemTag\TagNotFoundException;
+use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
@@ -47,25 +50,25 @@ use Test\TestCase;
  */
 class RetentionJobTest extends TestCase {
 
-	/** @var ISystemTagManager|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var ISystemTagManager|MockObject */
 	private $tagManager;
 
-	/** @var ISystemTagObjectMapper|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var ISystemTagObjectMapper|MockObject */
 	private $tagMapper;
 
-	/** @var IUserMountCache|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var IUserMountCache|MockObject */
 	private $userMountCache;
 
 	/** @var IDBConnection */
 	private $db;
 
-	/** @var IRootFolder|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var IRootFolder|MockObject */
 	private $rootFolder;
 
-	/** @var ITimeFactory|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var ITimeFactory|MockObject */
 	private $timeFactory;
 
-	/** @var IJobList|\PHPUnit_Framework_MockObject_MockObject */
+	/** @var IJobList|MockObject */
 	private $jobList;
 
 	/** @var RetentionJob */
@@ -164,7 +167,7 @@ class RetentionJobTest extends TestCase {
 			->with(42, 'files')
 			->willReturn([1337]);
 
-		$mountPoint = $this->createMock(ICachedMountInfo::class);
+		$mountPoint = $this->createMock(ICachedMountFileInfo::class);
 		$this->userMountCache->expects($this->once())
 			->method('getMountsForFileId')
 			->with(1337)
@@ -200,6 +203,9 @@ class RetentionJobTest extends TestCase {
 			->method('getMTime')
 			->willReturn($mtime->getTimestamp());
 
+		$node->method('isDeletable')
+			->willReturn(true);
+
 		if ($delete) {
 			$node->expects($this->once())
 				->method('delete');
@@ -255,7 +261,7 @@ class RetentionJobTest extends TestCase {
 			->with(42, 'files')
 			->willReturn([1337]);
 
-		$mountPoint = $this->createMock(ICachedMountInfo::class);
+		$mountPoint = $this->createMock(ICachedMountFileInfo::class);
 		$this->userMountCache->expects($this->once())
 			->method('getMountsForFileId')
 			->with(1337)
@@ -295,6 +301,126 @@ class RetentionJobTest extends TestCase {
 			->method('delete')
 			->will($this->throwException(new NotPermittedException()));
 
+		$node->method('isDeletable')
+			->willReturn(true);
+
+		$this->retentionJob->run(['tag' => 42]);
+	}
+
+	public function testNoDeletePermissions() {
+		$this->addTag(42, 1, Constants::DAY);
+
+		$this->tagMapper->expects($this->once())
+			->method('getObjectIdsForTags')
+			->with(42, 'files')
+			->willReturn([1337]);
+
+		$mountPoint = $this->createMock(ICachedMountFileInfo::class);
+		$this->userMountCache->expects($this->once())
+			->method('getMountsForFileId')
+			->with(1337)
+			->willReturn([$mountPoint]);
+
+		$user = $this->createMock(IUser::class);
+		$mountPoint->expects($this->once())
+			->method('getUser')
+			->willReturn($user);
+
+		$user->expects($this->once())
+			->method('getUID')
+			->willReturn('user');
+
+		$userFolder = $this->createMock(Folder::class);
+		$this->rootFolder->expects($this->once())
+			->method('getUserFolder')
+			->with('user')
+			->willReturn($userFolder);
+
+		$node = $this->createMock(Node::class);
+		$userFolder->expects($this->once())
+			->method('getById')
+			->with(1337)
+			->willReturn([$node]);
+
+		$node->method('isDeletable')
+			->willReturn(false);
+
+		$this->retentionJob->run(['tag' => 42]);
+	}
+
+	public function testNoDeletePermissionsOnFirstMountPointButOnSecond() {
+		$this->addTag(42, 1, Constants::DAY);
+
+		$this->tagMapper->expects($this->once())
+			->method('getObjectIdsForTags')
+			->with(42, 'files')
+			->willReturn([1337]);
+
+		$mountPoint1 = $this->createMock(ICachedMountFileInfo::class);
+		$mountPoint2 = $this->createMock(ICachedMountFileInfo::class);
+		$this->userMountCache->expects($this->once())
+			->method('getMountsForFileId')
+			->with(1337)
+			->willReturn([$mountPoint1, $mountPoint2]);
+
+		$user1 = $this->createMock(IUser::class);
+		$user2 = $this->createMock(IUser::class);
+		$mountPoint1->expects($this->once())
+			->method('getUser')
+			->willReturn($user1);
+		$mountPoint2->expects($this->once())
+			->method('getUser')
+			->willReturn($user2);
+
+		$user1->expects($this->once())
+			->method('getUID')
+			->willReturn('user1');
+		$user2->expects($this->once())
+			->method('getUID')
+			->willReturn('user2');
+
+		$userFolder1 = $this->createMock(Folder::class);
+		$userFolder2 = $this->createMock(Folder::class);
+		$this->rootFolder->method('getUserFolder')
+			->withConsecutive(['user1'], ['user2'])
+			->willReturnMap([
+				['user1', $userFolder1],
+				['user2', $userFolder2],
+			]);
+
+		$node1 = $this->createMock(Node::class);
+		$node2 = $this->createMock(Node::class);
+		$userFolder1->expects($this->once())
+			->method('getById')
+			->with(1337)
+			->willReturn([$node1]);
+		$userFolder2->expects($this->once())
+			->method('getById')
+			->with(1337)
+			->willReturn([$node2]);
+
+		$node1->method('isDeletable')
+			->willReturn(false);
+
+		$delta = new \DateInterval('P2D');
+		$now = new \DateTime();
+		$now->setTimestamp($this->timestampbase);
+		$mtime = $now->sub($delta);
+
+		$node2->expects($this->once())
+			->method('getMTime')
+			->willReturn($mtime->getTimestamp());
+
+		$node2->expects($this->once())
+			->method('delete')
+			->will($this->throwException(new NotPermittedException()));
+
+		$node2->method('isDeletable')
+			->willReturn(true);
+
+		$node2->expects($this->once())
+			->method('delete');
+
 		$this->retentionJob->run(['tag' => 42]);
 	}
 
@@ -322,7 +448,7 @@ class RetentionJobTest extends TestCase {
 			->with(42, 'files')
 			->willReturn([1337]);
 
-		$mountPoint = $this->createMock(ICachedMountInfo::class);
+		$mountPoint = $this->createMock(ICachedMountFileInfo::class);
 		$this->userMountCache->expects($this->once())
 			->method('getMountsForFileId')
 			->with(1337)