From fa7b16805588cb902a5584cc5076d543ec54ab6e Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Mon, 18 Dec 2023 13:48:13 +0100 Subject: [PATCH 1/2] fix(Authorizer): Don't give WRITE permissions by default Signed-off-by: Marcel Klehr --- lib/Controller/FoldersController.php | 4 ++-- lib/Service/Authorizer.php | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/Controller/FoldersController.php b/lib/Controller/FoldersController.php index a01eb8b826..9e22710b20 100644 --- a/lib/Controller/FoldersController.php +++ b/lib/Controller/FoldersController.php @@ -247,8 +247,8 @@ public function addToFolder($folderId, $bookmarkId): JSONResponse { * @PublicPage */ public function removeFromFolder($folderId, $bookmarkId): JSONResponse { - if (!Authorizer::hasPermission(Authorizer::PERM_WRITE, $this->authorizer->getPermissionsForFolder($folderId, $this->request)) && - !Authorizer::hasPermission(Authorizer::PERM_EDIT, $this->authorizer->getPermissionsForFolder($bookmarkId, $this->request))) { + if (!Authorizer::hasPermission(Authorizer::PERM_WRITE, $this->authorizer->getPermissionsForFolder($folderId, $this->request)) || + !Authorizer::hasPermission(Authorizer::PERM_EDIT, $this->authorizer->getPermissionsForBookmark($bookmarkId, $this->request))) { return new JSONResponse(['status' => 'error', 'data' => 'Unauthorized'], Http::STATUS_FORBIDDEN); } try { diff --git a/lib/Service/Authorizer.php b/lib/Service/Authorizer.php index 085158557f..b99f7ce059 100644 --- a/lib/Service/Authorizer.php +++ b/lib/Service/Authorizer.php @@ -263,7 +263,7 @@ private function findPermissionsByUserAndItem(string $userId, string $type, int if ($share->getFolderId() === $itemId && $type === TreeMapper::TYPE_FOLDER) { // If the sought folder is the root folder of the share, we give EDIT permissions + optionally RESHARE // because the user can edit the shared folder - $perms = $this->getMaskFromFlags(true, $share->getCanShare()) | self::PERM_EDIT; + $perms = $this->getMaskFromFlags($share->getCanWrite(), $share->getCanShare()) | self::PERM_EDIT; } elseif ($this->treeMapper->hasDescendant($share->getFolderId(), $type, $itemId)) { $perms = $this->getMaskFromFlags($share->getCanWrite(), $share->getCanShare()); } else { From 774c67ff30922157a1cae2a1997bd664b930a6c2 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Mon, 18 Dec 2023 15:08:19 +0100 Subject: [PATCH 2/2] tests: Add testDeleteFromSharedFolder Signed-off-by: Marcel Klehr --- tests/FolderControllerTest.php | 85 +++++++++++++++++++++------------- 1 file changed, 53 insertions(+), 32 deletions(-) diff --git a/tests/FolderControllerTest.php b/tests/FolderControllerTest.php index fcb0865f59..49335de478 100644 --- a/tests/FolderControllerTest.php +++ b/tests/FolderControllerTest.php @@ -234,9 +234,9 @@ public function setupPublicFolder(): void { * @throws \OCA\Bookmarks\Exception\UnsupportedOperation * @throws \OCP\AppFramework\Db\DoesNotExistException */ - public function setupSharedFolder() { + public function setupSharedFolder($canWrite = true, $canShare = false) { $this->authorizer->setUserId($this->userId); - $this->share = $this->folders->createShare($this->folder1->getId(), $this->otherUser, \OCP\Share\IShare::TYPE_USER, true, false); + $this->share = $this->folders->createShare($this->folder1->getId(), $this->otherUser, \OCP\Share\IShare::TYPE_USER, $canWrite, $canShare); } /** @@ -247,7 +247,7 @@ public function setupSharedFolder() { * @throws MultipleObjectsReturnedException */ public function testRead(): void { - + $this->setupBookmarks(); $this->authorizer->setUserId($this->userId); $output = $this->controller->getFolder($this->folder1->getId()); @@ -322,7 +322,7 @@ public function testHash(bool $useCache): void { * @throws MultipleObjectsReturnedException */ public function testCreate(): void { - + $this->setupBookmarks(); $this->authorizer->setUserId($this->userId); $output = $this->controller->addFolder('foo', $this->folder1->getId()); @@ -424,7 +424,7 @@ public function testSharedEdit(bool $canWrite): void { * @throws MultipleObjectsReturnedException */ public function testDelete(): void { - + $this->setupBookmarks(); $this->authorizer->setUserId($this->userId); $output = $this->controller->deleteFolder($this->folder1->getId()); @@ -442,7 +442,7 @@ public function testDelete(): void { * @throws MultipleObjectsReturnedException */ public function testGetFullHierarchy(): void { - + $this->setupBookmarks(); $this->authorizer->setUserId($this->userId); // Using -1 here because this is the controller @@ -466,7 +466,7 @@ public function testGetFullHierarchy(): void { * @throws MultipleObjectsReturnedException */ public function testSetFullHierarchy(): void { - + $this->setupBookmarks(); $this->authorizer->setUserId($this->userId); $output = $this->controller->setFolderChildrenOrder($this->folder1->getId(), [ @@ -494,7 +494,7 @@ public function testSetFullHierarchy(): void { * @throws MultipleObjectsReturnedException */ public function testGetFolderHierarchy(): void { - + $this->setupBookmarks(); $this->authorizer->setUserId($this->userId); $output = $this->controller->getFolders(-1, -1); @@ -517,7 +517,7 @@ public function testGetFolderHierarchy(): void { * @throws MultipleObjectsReturnedException */ public function testReadNoauthFail(): void { - + $this->setupBookmarks(); $this->setupPublicFolder(); $this->authorizer->setUserId(null); @@ -533,7 +533,7 @@ public function testReadNoauthFail(): void { * @throws MultipleObjectsReturnedException */ public function testCreateNoauthFail(): void { - + $this->setupBookmarks(); $this->setupPublicFolder(); $this->authorizer->setUserId(null); @@ -550,7 +550,7 @@ public function testCreateNoauthFail(): void { * @throws MultipleObjectsReturnedException */ public function testEditNoauthFail(): void { - + $this->setupBookmarks(); $this->setupPublicFolder(); $this->authorizer->setUserId(null); @@ -567,7 +567,7 @@ public function testEditNoauthFail(): void { * @throws MultipleObjectsReturnedException */ public function testDeleteNoauthFail(): void { - + $this->setupBookmarks(); $this->setupPublicFolder(); $this->authorizer->setUserId(null); @@ -583,7 +583,7 @@ public function testDeleteNoauthFail(): void { * @throws MultipleObjectsReturnedException */ public function testGetFullHierarchyNoauthFail(): void { - + $this->setupBookmarks(); $this->authorizer->setUserId(null); $this->authorizer->setToken(null); @@ -598,7 +598,7 @@ public function testGetFullHierarchyNoauthFail(): void { * @throws MultipleObjectsReturnedException */ public function testSetFullHierarchyNoauthFail(): void { - + $this->setupBookmarks(); $this->authorizer->setUserId(null); $this->authorizer->setToken(null); @@ -616,7 +616,7 @@ public function testSetFullHierarchyNoauthFail(): void { * @throws MultipleObjectsReturnedException */ public function testGetFolderHierarchyNoauth(): void { - + $this->setupBookmarks(); $this->authorizer->setUserId(null); $this->authorizer->setToken(null); @@ -632,7 +632,7 @@ public function testGetFolderHierarchyNoauth(): void { * @throws MultipleObjectsReturnedException */ public function testReadPublic(): void { - + $this->setupBookmarks(); $this->setupPublicFolder(); $this->authorizer->setUserId(null); @@ -681,7 +681,7 @@ public function testCreatePublicFail(): void { * @throws MultipleObjectsReturnedException */ public function testEditPublicFail(): void { - + $this->setupBookmarks(); $this->setupPublicFolder(); $this->authorizer->setUserId(null); @@ -702,7 +702,7 @@ public function testEditPublicFail(): void { * @throws MultipleObjectsReturnedException */ public function testDeletePublicFail(): void { - + $this->setupBookmarks(); $this->setupPublicFolder(); $this->authorizer->setUserId(null); @@ -721,7 +721,7 @@ public function testDeletePublicFail(): void { * @throws MultipleObjectsReturnedException */ public function testGetFullHierarchyPublic(): void { - + $this->setupBookmarks(); $this->setupPublicFolder(); $this->authorizer->setUserId(null); @@ -770,7 +770,7 @@ public function testSetFullHierarchyPublicFail(): void { * @throws MultipleObjectsReturnedException */ public function testGetFolderHierarchyPublic(): void { - + $this->setupBookmarks(); $this->setupPublicFolder(); $this->authorizer->setUserId(null); @@ -789,7 +789,7 @@ public function testGetFolderHierarchyPublic(): void { * @throws MultipleObjectsReturnedException */ public function testReadShared(): void { - + $this->setupBookmarks(); $this->setupSharedFolder(); $this->authorizer->setUserId($this->otherUserId); @@ -807,7 +807,7 @@ public function testReadShared(): void { * @throws MultipleObjectsReturnedException */ public function testReadSharedFail(): void { - + $this->setupBookmarks(); $this->authorizer->setUserId($this->otherUserId); $output = $this->otherController->getFolder($this->folder1->getId()); @@ -823,7 +823,7 @@ public function testReadSharedFail(): void { * @throws MultipleObjectsReturnedException */ public function testCreateShared(): void { - + $this->setupBookmarks(); $this->setupSharedFolder(); $this->authorizer->setUserId($this->otherUserId); @@ -875,7 +875,7 @@ public function testEditShared(bool $canWrite): void { * @throws MultipleObjectsReturnedException */ public function testDeleteShared(): void { - + $this->setupBookmarks(); $this->setupSharedFolder(); $this->authorizer->setUserId($this->otherUserId); @@ -887,6 +887,27 @@ public function testDeleteShared(): void { $this->assertEquals('error', $data['status'], var_export($data, true)); } + /** + * @throws AlreadyExistsError + * @throws UrlParseError + * @throws UserLimitExceededError + * @throws \OCP\AppFramework\Db\DoesNotExistException + * @throws MultipleObjectsReturnedException + * @dataProvider shareCanWriteDataProvider + */ + public function testDeleteFromSharedFolder(bool $canWrite): void { + $this->setupBookmarks(); + $this->setupSharedFolder($canWrite); + $this->authorizer->setUserId($this->otherUserId); + $output = $this->otherController->removeFromFolder($this->folder1->getId(), $this->bookmark1Id); + $data = $output->getData(); + if ($canWrite) { + $this->assertEquals('success', $data['status'], var_export($data, true)); + } else { + $this->assertEquals('error', $data['status'], var_export($data, true)); + } + } + /** * @throws AlreadyExistsError * @throws UrlParseError @@ -894,7 +915,7 @@ public function testDeleteShared(): void { * @throws MultipleObjectsReturnedException */ public function testGetFullHierarchyShared(): void { - + $this->setupBookmarks(); $this->setupSharedFolder(); $this->authorizer->setUserId($this->otherUserId); @@ -915,7 +936,7 @@ public function testGetFullHierarchyShared(): void { * @throws MultipleObjectsReturnedException */ public function testSetFullHierarchyShared(): void { - + $this->setupBookmarks(); $this->setupSharedFolder(); @@ -947,7 +968,7 @@ public function testSetFullHierarchyShared(): void { * @throws MultipleObjectsReturnedException */ public function testGetFolderHierarchyShared(): void { - + $this->setupBookmarks(); $this->setupSharedFolder(); $this->authorizer->setUserId($this->otherUserId); @@ -971,7 +992,7 @@ public function testGetFolderHierarchyShared(): void { * @dataProvider shareDataProvider */ public function testCreateShare($participant, $type, $canWrite, $canShare): void { - + $this->setupBookmarks(); $this->authorizer->setUserid($this->userId); $res = $this->controller->createShare($this->folder1->getId(), $participant, $type, $canWrite, $canShare); @@ -992,7 +1013,7 @@ public function testCreateShare($participant, $type, $canWrite, $canShare): void * @depends testCreateShare */ public function testGetShare($participant, $type, $canWrite, $canShare): void { - + $this->setupBookmarks(); $this->authorizer->setUserId($this->userId); $res = $this->controller->createShare($this->folder1->getId(), $participant, $type, $canWrite, $canShare); @@ -1020,7 +1041,7 @@ public function testGetShare($participant, $type, $canWrite, $canShare): void { * @depends testCreateShare */ public function testEditShare($participant, $type, $canWrite, $canShare): void { - + $this->setupBookmarks(); $this->authorizer->setUserId($this->userId); $res = $this->controller->createShare($this->folder1->getId(), $participant, $type, $canWrite, $canShare); @@ -1056,7 +1077,7 @@ public function testEditShare($participant, $type, $canWrite, $canShare): void { * @depends testCreateShare */ public function testDeleteShareOwner($participant, $type, $canWrite, $canShare): void { - + $this->setupBookmarks(); $this->authorizer->setUserId($this->userId); $res = $this->controller->createShare($this->folder1->getId(), $participant, $type, $canWrite, $canShare); @@ -1083,7 +1104,7 @@ public function testDeleteShareOwner($participant, $type, $canWrite, $canShare): * @depends testCreateShare */ public function testDeleteShareSharee($participant, $type, $canWrite, $canShare): void { - + $this->setupBookmarks(); $this->authorizer->setUserId($this->userId); $res = $this->controller->createShare($this->folder1->getId(), $participant, $type, $canWrite, $canShare);