feat: release 10.16.3

.github/workflows/lint-and-codestyle.yml

@@ -59,16 +59,3 @@ jobs:
       - name: Run PHP Phan
         run: |
           make test-php-phan
-
-  git-commit-messages:
-    name: Git Commit Messages
-    runs-on: ubuntu-latest
-    steps:
-      - name: Checkout code
-        uses: actions/checkout@v4
-        with:
-          fetch-depth: 0
-
-      - name: Validate commit messages
-        uses: docker://aevea/commitsar:latest
-

apps/comments/lib/Dav/EntityCollection.php

@@ -98,6 +98,10 @@ public function getId() {
 	public function getChild($name) {
 		try {
 			$comment = $this->commentsManager->get($name);
+			// objectType/objectId identify the entity (e.g. file) the comment is attached to
+			if ($comment->getObjectType() !== $this->name || $comment->getObjectId() !== $this->id) {
+				throw new NotFound();
+			}
 			return new CommentNode(
 				$this->commentsManager,
 				$comment,
@@ -151,8 +155,10 @@ public function findChildren($limit = 0, $offset = 0, \DateTime $datetime = null
 	 */
 	public function childExists($name) {
 		try {
-			$this->commentsManager->get($name);
-			return true;
+			$comment = $this->commentsManager->get($name);
+			// objectType/objectId identify the entity (e.g. file) the comment is attached to
+			return $comment->getObjectType() === $this->name
+				&& $comment->getObjectId() === $this->id;
 		} catch (NotFoundException $e) {
 			return false;
 		}

apps/comments/tests/unit/Dav/EntityCollectionTest.php

@@ -70,10 +70,14 @@ public function testGetId(): void {
 	}
 
 	public function testGetChild(): void {
+		$comment = $this->createMock(IComment::class);
+		$comment->method('getObjectType')->willReturn('files');
+		$comment->method('getObjectId')->willReturn('19');
+
 		$this->commentsManager->expects($this->once())
 			->method('get')
 			->with('55')
-			->will($this->returnValue($this->createMock(IComment::class)));
+			->willReturn($comment);
 
 		$node = $this->collection->getChild('55');
 		$this->assertInstanceOf(\OCA\Comments\Dav\CommentNode::class, $node);
@@ -118,6 +122,15 @@ public function testFindChildren(): void {
 	}
 
 	public function testChildExistsTrue(): void {
+		$comment = $this->createMock(IComment::class);
+		$comment->method('getObjectType')->willReturn('files');
+		$comment->method('getObjectId')->willReturn('19');
+
+		$this->commentsManager->expects($this->once())
+			->method('get')
+			->with('44')
+			->willReturn($comment);
+
 		$this->assertTrue($this->collection->childExists('44'));
 	}
 
@@ -129,4 +142,60 @@ public function testChildExistsFalse(): void {
 
 		$this->assertFalse($this->collection->childExists('44'));
 	}
+
+	public function testGetChildWrongFile(): void {
+		$this->expectException(\Sabre\DAV\Exception\NotFound::class);
+
+		$comment = $this->createMock(IComment::class);
+		$comment->method('getObjectType')->willReturn('files');
+		$comment->method('getObjectId')->willReturn('999'); // different file
+
+		$this->commentsManager->expects($this->once())
+			->method('get')
+			->with('55')
+			->willReturn($comment);
+
+		$this->collection->getChild('55');
+	}
+
+	public function testGetChildWrongObjectType(): void {
+		$this->expectException(\Sabre\DAV\Exception\NotFound::class);
+
+		$comment = $this->createMock(IComment::class);
+		$comment->method('getObjectType')->willReturn('albums'); // wrong type
+		$comment->method('getObjectId')->willReturn('19');
+
+		$this->commentsManager->expects($this->once())
+			->method('get')
+			->with('55')
+			->willReturn($comment);
+
+		$this->collection->getChild('55');
+	}
+
+	public function testChildExistsWrongFile(): void {
+		$comment = $this->createMock(IComment::class);
+		$comment->method('getObjectType')->willReturn('files');
+		$comment->method('getObjectId')->willReturn('999'); // different file
+
+		$this->commentsManager->expects($this->once())
+			->method('get')
+			->with('44')
+			->willReturn($comment);
+
+		$this->assertFalse($this->collection->childExists('44'));
+	}
+
+	public function testChildExistsWrongObjectType(): void {
+		$comment = $this->createMock(IComment::class);
+		$comment->method('getObjectType')->willReturn('albums'); // wrong type
+		$comment->method('getObjectId')->willReturn('19');
+
+		$this->commentsManager->expects($this->once())
+			->method('get')
+			->with('44')
+			->willReturn($comment);
+
+		$this->assertFalse($this->collection->childExists('44'));
+	}
 }

apps/files_external/lib/Controller/GlobalStoragesController.php

@@ -86,15 +86,10 @@ public function create(
 		$applicableGroups,
 		$priority
 	) {
-		$canCreateNewLocalStorage = \OC::$server->getConfig()->getSystemValue('files_external_allow_create_new_local', false);
-
-		if ($backend === 'local' && $canCreateNewLocalStorage === false) {
-			return new DataResponse(
-				null,
-				Http::STATUS_FORBIDDEN
-			);
-		}
-
+		// NOTE: files_external_allow_create_new_local is checked during backend
+		// registration, and it will remove the related storage visibility
+		// if applicable. If the storage isn't visible, the `validate` method
+		// will fail.
 		$newStorage = $this->createStorage(
 			$mountPoint,
 			$backend,

apps/files_external/lib/Controller/StoragesController.php

@@ -183,7 +183,7 @@ protected function validate(IStorageConfig $storage) {
 						$backend->getIdentifier()
 					])
 				],
-				Http::STATUS_UNPROCESSABLE_ENTITY
+				Http::STATUS_FORBIDDEN
 			);
 		}
 		if (!$authMechanism->isVisibleFor($this->service->getVisibilityType())) {
@@ -194,7 +194,7 @@ protected function validate(IStorageConfig $storage) {
 						$authMechanism->getIdentifier()
 					])
 				],
-				Http::STATUS_UNPROCESSABLE_ENTITY
+				Http::STATUS_FORBIDDEN
 			);
 		}
 
@@ -308,7 +308,7 @@ public function show($id, $testOnly = true) {
 		} catch (NotFoundException $e) {
 			return new DataResponse(
 				[
-					'message' => (string)$this->l10n->t('Storage with id "%i" not found', [$id])
+					'message' => (string)$this->l10n->t('Storage with id "%d" not found', [$id])
 				],
 				Http::STATUS_NOT_FOUND
 			);
@@ -335,7 +335,7 @@ public function destroy($id) {
 		} catch (NotFoundException $e) {
 			return new DataResponse(
 				[
-					'message' => (string)$this->l10n->t('Storage with id "%i" not found', [$id])
+					'message' => (string)$this->l10n->t('Storage with id "%d" not found', [$id])
 				],
 				Http::STATUS_NOT_FOUND
 			);

apps/files_external/lib/Controller/UserStoragesController.php

@@ -31,7 +31,6 @@
 use OCP\Files\External\IStorageConfig;
 use OCP\Files\External\NotFoundException;
 use OCP\Files\External\Service\IUserStoragesService;
-use OCP\IConfig;
 use OCP\IL10N;
 use OCP\ILogger;
 use OCP\IRequest;
@@ -45,15 +44,13 @@ class UserStoragesController extends StoragesController {
 	 * @var IUserSession
 	 */
 	private $userSession;
-	private IConfig $config;
 
 	public function __construct(
 		$AppName,
 		IRequest $request,
 		IL10N $l10n,
 		IUserStoragesService $userStoragesService,
 		IUserSession $userSession,
-		IConfig $config,
 		ILogger $logger
 	) {
 		parent::__construct(
@@ -64,7 +61,6 @@ public function __construct(
 			$logger
 		);
 		$this->userSession = $userSession;
-		$this->config = $config;
 	}
 
 	protected function manipulateStorageConfig(IStorageConfig $storage) {
@@ -82,12 +78,6 @@ protected function manipulateStorageConfig(IStorageConfig $storage) {
 	 * @return DataResponse
 	 */
 	public function index() {
-		if (!$this->isUserMountingAllowed()) {
-			return new DataResponse(
-				null,
-				Http::STATUS_FORBIDDEN
-			);
-		}
 		return parent::index();
 	}
 
@@ -122,20 +112,11 @@ public function create(
 		$backendOptions,
 		$mountOptions
 	) {
-		if (!$this->isUserMountingAllowed()) {
-			return new DataResponse(
-				null,
-				Http::STATUS_FORBIDDEN
-			);
-		}
-		$canCreateNewLocalStorage = \OC::$server->getConfig()->getSystemValue('files_external_allow_create_new_local', false);
-		if ($backend === 'local' && $canCreateNewLocalStorage === false) {
-			return new DataResponse(
-				null,
-				Http::STATUS_FORBIDDEN
-			);
-		}
-
+		// NOTE: local backend isn't allowed for regular users regardless
+		// of the value of files_external_allow_create_new_local. The backend
+		// won't be visible for regular users, so the `validate` method will fail.
+		// Only admins can create local storages (if files_external_allow_create_new_local
+		// is true)
 		$newStorage = $this->createStorage(
 			$mountPoint,
 			$backend,
@@ -188,12 +169,6 @@ public function update(
 		$mountOptions,
 		$testOnly = true
 	) {
-		if (!$this->isUserMountingAllowed()) {
-			return new DataResponse(
-				null,
-				Http::STATUS_FORBIDDEN
-			);
-		}
 		$storage = $this->createStorage(
 			$mountPoint,
 			$backend,
@@ -241,17 +216,6 @@ public function update(
 	 * {@inheritdoc}
 	 */
 	public function destroy($id) {
-		if (!$this->isUserMountingAllowed()) {
-			return new DataResponse(
-				null,
-				Http::STATUS_FORBIDDEN
-			);
-		}
-
 		return parent::destroy($id);
 	}
-
-	private function isUserMountingAllowed(): bool {
-		return $this->config->getAppValue('files_external', 'allow_user_mounting', 'no') === 'yes';
-	}
 }

apps/files_external/tests/Controller/GlobalStoragesControllerTest.php

@@ -111,6 +111,55 @@ public function testCreate() {
 		$this->assertEquals($expectedStorage, $actual);
 	}
 
+	public function localBackendNameProvider() {
+		return [
+			['local'],
+			['\OC\Files\Storage\Local'],
+		];
+	}
+
+	/**
+	 * @dataProvider localBackendNameProvider
+	 */
+	public function testCreateLocal($localBackendName) {
+		$mount = 'randomMount';
+		$backend = "identifier:{$localBackendName}";
+		$auth = 'identifier:\Random\Missing\Auth\Class';
+		$backendOpts = [
+			'datadir' => '/tmp',
+		];
+		$priority = 3;
+
+		$storageConfig = $this->getNewStorageConfigMock([
+			'id' => 30,
+			'backendClass' => '\OCA\Files_External\Lib\Backend',
+			'backendStorageClass' => '\OC\Files\Storage\Local',
+			'authClass' => '\Random\Missing\Auth\Class',
+			'mountPoint' => $mount,
+			'backendOpts' => $backendOpts,
+			'priority' => $priority,
+			'type' => IStorageConfig::MOUNT_TYPE_ADMIN,
+		]);
+
+		$backendMock = $storageConfig->getBackend();
+		$backendMock->method('isVisibleFor')->willReturn(false);
+		$backendMock->method('validateStorage')->willReturn(true);
+
+		$authMock = $storageConfig->getAuthMechanism();
+		$authMock->method('isVisibleFor')->willReturn(true);
+		$authMock->method('validateStorage')->willReturn(true);
+
+		$this->service->expects($this->once())
+		->method('createStorage')
+		->willReturn($storageConfig);
+		$this->service->expects($this->never())
+		->method('addStorage')
+		->will($this->returnArgument(0));
+
+		$result = $this->controller->create($mount, $backend, $auth, $backendOpts, [], [], [], $priority);
+		$this->assertEquals(Http::STATUS_FORBIDDEN, $result->getStatus());
+	}
+
 	public function testUpdate() {
 		$mount = 'randomMount';
 		$backend = 'identifier:\This\Doesnt\Exist';

apps/files_external/tests/Controller/StoragesControllerTest.php

@@ -170,7 +170,7 @@ public function testAddStorageWithoutConfig() {
 
 		$data = $response->getData();
 		$this->assertEquals(Http::STATUS_FORBIDDEN, $response->getStatus());
-		$this->assertNull($data);
+		$this->assertEquals(['message' => ''], $data);  // message comes from translatable error string, which isn't set here.
 	}
 
 	public function testUpdateStorage() {