Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(Authorizer): Don't give WRITE permissions by default #2109

Merged
merged 2 commits into from
Dec 18, 2023
Merged

fix(Authorizer): Don't give WRITE permissions by default #2109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fa7b168
fix(Authorizer): Don't give WRITE permissions by default
marcelklehr Dec 18, 2023
774c67f
tests: Add testDeleteFromSharedFolder
marcelklehr Dec 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/Controller/FoldersController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion lib/Service/Authorizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
85 changes: 53 additions & 32 deletions tests/FolderControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/**
Expand All @@ -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());
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand All @@ -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
Expand All @@ -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(), [
Expand Down Expand Up @@ -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);
Expand All @@ -517,7 +517,7 @@ public function testGetFolderHierarchy(): void {
* @throws MultipleObjectsReturnedException
*/
public function testReadNoauthFail(): void {

$this->setupBookmarks();
$this->setupPublicFolder();
$this->authorizer->setUserId(null);
Expand All @@ -533,7 +533,7 @@ public function testReadNoauthFail(): void {
* @throws MultipleObjectsReturnedException
*/
public function testCreateNoauthFail(): void {

$this->setupBookmarks();
$this->setupPublicFolder();
$this->authorizer->setUserId(null);
Expand All @@ -550,7 +550,7 @@ public function testCreateNoauthFail(): void {
* @throws MultipleObjectsReturnedException
*/
public function testEditNoauthFail(): void {

$this->setupBookmarks();
$this->setupPublicFolder();
$this->authorizer->setUserId(null);
Expand All @@ -567,7 +567,7 @@ public function testEditNoauthFail(): void {
* @throws MultipleObjectsReturnedException
*/
public function testDeleteNoauthFail(): void {

$this->setupBookmarks();
$this->setupPublicFolder();
$this->authorizer->setUserId(null);
Expand All @@ -583,7 +583,7 @@ public function testDeleteNoauthFail(): void {
* @throws MultipleObjectsReturnedException
*/
public function testGetFullHierarchyNoauthFail(): void {

$this->setupBookmarks();
$this->authorizer->setUserId(null);
$this->authorizer->setToken(null);
Expand All @@ -598,7 +598,7 @@ public function testGetFullHierarchyNoauthFail(): void {
* @throws MultipleObjectsReturnedException
*/
public function testSetFullHierarchyNoauthFail(): void {

$this->setupBookmarks();
$this->authorizer->setUserId(null);
$this->authorizer->setToken(null);
Expand All @@ -616,7 +616,7 @@ public function testSetFullHierarchyNoauthFail(): void {
* @throws MultipleObjectsReturnedException
*/
public function testGetFolderHierarchyNoauth(): void {

$this->setupBookmarks();
$this->authorizer->setUserId(null);
$this->authorizer->setToken(null);
Expand All @@ -632,7 +632,7 @@ public function testGetFolderHierarchyNoauth(): void {
* @throws MultipleObjectsReturnedException
*/
public function testReadPublic(): void {

$this->setupBookmarks();
$this->setupPublicFolder();
$this->authorizer->setUserId(null);
Expand Down Expand Up @@ -681,7 +681,7 @@ public function testCreatePublicFail(): void {
* @throws MultipleObjectsReturnedException
*/
public function testEditPublicFail(): void {

$this->setupBookmarks();
$this->setupPublicFolder();
$this->authorizer->setUserId(null);
Expand All @@ -702,7 +702,7 @@ public function testEditPublicFail(): void {
* @throws MultipleObjectsReturnedException
*/
public function testDeletePublicFail(): void {

$this->setupBookmarks();
$this->setupPublicFolder();
$this->authorizer->setUserId(null);
Expand All @@ -721,7 +721,7 @@ public function testDeletePublicFail(): void {
* @throws MultipleObjectsReturnedException
*/
public function testGetFullHierarchyPublic(): void {

$this->setupBookmarks();
$this->setupPublicFolder();
$this->authorizer->setUserId(null);
Expand Down Expand Up @@ -770,7 +770,7 @@ public function testSetFullHierarchyPublicFail(): void {
* @throws MultipleObjectsReturnedException
*/
public function testGetFolderHierarchyPublic(): void {

$this->setupBookmarks();
$this->setupPublicFolder();
$this->authorizer->setUserId(null);
Expand All @@ -789,7 +789,7 @@ public function testGetFolderHierarchyPublic(): void {
* @throws MultipleObjectsReturnedException
*/
public function testReadShared(): void {

$this->setupBookmarks();
$this->setupSharedFolder();
$this->authorizer->setUserId($this->otherUserId);
Expand All @@ -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());
Expand All @@ -823,7 +823,7 @@ public function testReadSharedFail(): void {
* @throws MultipleObjectsReturnedException
*/
public function testCreateShared(): void {

$this->setupBookmarks();
$this->setupSharedFolder();
$this->authorizer->setUserId($this->otherUserId);
Expand Down Expand Up @@ -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);
Expand All @@ -887,14 +887,35 @@ 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
* @throws UserLimitExceededError
* @throws MultipleObjectsReturnedException
*/
public function testGetFullHierarchyShared(): void {

$this->setupBookmarks();
$this->setupSharedFolder();
$this->authorizer->setUserId($this->otherUserId);
Expand All @@ -915,7 +936,7 @@ public function testGetFullHierarchyShared(): void {
* @throws MultipleObjectsReturnedException
*/
public function testSetFullHierarchyShared(): void {

$this->setupBookmarks();
$this->setupSharedFolder();

Expand Down Expand Up @@ -947,7 +968,7 @@ public function testSetFullHierarchyShared(): void {
* @throws MultipleObjectsReturnedException
*/
public function testGetFolderHierarchyShared(): void {

$this->setupBookmarks();
$this->setupSharedFolder();
$this->authorizer->setUserId($this->otherUserId);
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down
Loading