Skip to content

Commit

Permalink
Merge pull request #2631 from nextcloud/fix/fix-acls-on-trash2
Browse files Browse the repository at this point in the history
Fix ACL rules for trashed subfolders from groupfolders
  • Loading branch information
come-nc authored Dec 12, 2023
2 parents 5f5d7bf + 555f669 commit 96521d0
Show file tree
Hide file tree
Showing 8 changed files with 99 additions and 29 deletions.
7 changes: 6 additions & 1 deletion cypress/e2e/groupfolders.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,15 @@ describe('Groupfolders ACLs and trashbin behavior', () => {
fileOrFolderDoesNotExist('subfolder1')

// Delete files
cy.log('Deleting the files')
cy.login(managerUser)
cy.visit('/apps/files')
enterFolder(groupFolderName)
enterFolder('subfolder1')
deleteFile('file1.txt')
cy.visit('/apps/files')
enterFolder(groupFolderName)
enterFolder('subfolder1')
deleteFile('subfolder2')

// User1 sees it in trash
Expand All @@ -135,6 +139,7 @@ describe('Groupfolders ACLs and trashbin behavior', () => {
fileOrFolderDoesNotExistInTrashbin('subfolder2')

// Restore files
cy.log('Restoring the files')
cy.login(managerUser)
cy.visit('/apps/files/trashbin')
fileOrFolderExistsInTrashbin('file1.txt')
Expand All @@ -159,7 +164,7 @@ describe('Groupfolders ACLs and trashbin behavior', () => {
fileOrFolderDoesNotExist('subfolder1')
})

it.skip('ACL directly on deleted folder', () => {
it('ACL directly on deleted folder', () => {
// Create a subfolders and a file as manager
cy.login(managerUser)
cy.mkdir(managerUser, `/${groupFolderName}/subfolder1`)
Expand Down
6 changes: 6 additions & 0 deletions cypress/e2e/groupfoldersUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,17 +95,23 @@ export function fileOrFolderDoesNotExistInTrashbin(name: string) {
}

export function enterFolder(name: string) {
cy.intercept({ times: 1, method: 'PROPFIND', url: `**/dav/files/**/${name}` }).as('propFindFolder')
cy.get(`[data-cy-files-list] [data-cy-files-list-row-name="${name}"]`).click()
cy.wait('@propFindFolder')
}

export function enterFolderInTrashbin(name: string) {
cy.intercept({ times: 1, method: 'PROPFIND', url: `**/dav/trashbin/**/${name}.d*` }).as('propFindFolder')
cy.get(`[data-cy-files-list] [data-cy-files-list-row-name^="${name}.d"]`).click()
cy.wait('@propFindFolder')
}

export function deleteFile(name: string) {
cy.intercept({ times: 1, method: 'DELETE', url: `**/dav/files/**/${name}` }).as('delete')
cy.get(`[data-cy-files-list] [data-cy-files-list-row-name="${name}"] [data-cy-files-list-row-actions]`).click()
cy.get(`[data-cy-files-list] [data-cy-files-list-row-action="delete"]`).scrollIntoView()
cy.get(`[data-cy-files-list] [data-cy-files-list-row-action="delete"]`).click()
cy.wait('@delete')
}

export function restoreFile(name: string) {
Expand Down
37 changes: 28 additions & 9 deletions lib/ACL/ACLManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -24,24 +24,25 @@
namespace OCA\GroupFolders\ACL;

use OC\Cache\CappedMemoryCache;
use OCA\GroupFolders\Trash\TrashManager;
use OCP\Constants;
use OCP\Files\IRootFolder;
use OCP\IUser;

class ACLManager {
private RuleManager $ruleManager;
private CappedMemoryCache $ruleCache;
private IUser $user;
private ?int $rootStorageId;
/** @var callable */
private $rootFolderProvider;

public function __construct(RuleManager $ruleManager, IUser $user, callable $rootFolderProvider, ?int $rootStorageId = null) {
$this->ruleManager = $ruleManager;
public function __construct(
private RuleManager $ruleManager,
private TrashManager $trashManager,
private IUser $user,
callable $rootFolderProvider,
private ?int $rootStorageId = null,
) {
$this->ruleCache = new CappedMemoryCache();
$this->user = $user;
$this->rootFolderProvider = $rootFolderProvider;
$this->rootStorageId = $rootStorageId;
}

private function getRootStorageId(): int {
Expand Down Expand Up @@ -98,13 +99,31 @@ private function getRules(array $paths, bool $cache = true): array {
* @return string[]
*/
private function getRelevantPaths(string $path): array {
$paths = [$path];
$paths = [];
$fromTrashbin = str_starts_with($path, '__groupfolders/trash/');
if ($fromTrashbin) {
/* Exploded path will look like ["__groupfolders", "trash", "1", "folderName.d2345678", "rest/of/the/path.txt"] */
[,,$groupFolderId,$rootTrashedItemName] = explode('/', $path, 5);
$groupFolderId = (int)$groupFolderId;
/* Remove the date part */
$separatorPos = strrpos($rootTrashedItemName, '.d');
$rootTrashedItemDate = (int)substr($rootTrashedItemName, $separatorPos + 2);
$rootTrashedItemName = substr($rootTrashedItemName, 0, $separatorPos);
}
while ($path !== '') {
$paths[] = $path;
$path = dirname($path);
if ($fromTrashbin && ($path === '__groupfolders/trash')) {
/* We are in trash and hit the root folder, continue looking for ACLs on parent folders in original location */
$trashItemRow = $this->trashManager->getTrashItemByFileName($groupFolderId, $rootTrashedItemName, $rootTrashedItemDate);
$path = dirname('__groupfolders/' . $groupFolderId . '/' . $trashItemRow['original_location']);
$fromTrashbin = false;
continue;
}

if ($path === '.' || $path === '/') {
$path = '';
}
$paths[] = $path;
}

return $paths;
Expand Down
11 changes: 7 additions & 4 deletions lib/ACL/ACLManagerFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,21 @@

namespace OCA\GroupFolders\ACL;

use OCA\GroupFolders\Trash\TrashManager;
use OCP\IUser;

class ACLManagerFactory {
private $ruleManager;
private $rootFolderProvider;

public function __construct(RuleManager $ruleManager, callable $rootFolderProvider) {
$this->ruleManager = $ruleManager;
public function __construct(
private RuleManager $ruleManager,
private TrashManager $trashManager,
callable $rootFolderProvider,
) {
$this->rootFolderProvider = $rootFolderProvider;
}

public function getACLManager(IUser $user, ?int $rootStorageId = null): ACLManager {
return new ACLManager($this->ruleManager, $user, $this->rootFolderProvider, $rootStorageId);
return new ACLManager($this->ruleManager, $this->trashManager, $user, $this->rootFolderProvider, $rootStorageId);
}
}
1 change: 1 addition & 0 deletions lib/AppInfo/Application.php
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ public function register(IRegistrationContext $context): void {
};
return new ACLManagerFactory(
$c->get(RuleManager::class),
$c->get(TrashManager::class),
$rootFolderProvider
);
});
Expand Down
20 changes: 7 additions & 13 deletions lib/Trash/TrashBackend.php
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ public function restoreItem(ITrashItem $item) {
if ($node === null) {
throw new NotFoundException();
}
if (!$this->userHasAccessToPath($item->getUser(), $folderId . '/' . $item->getOriginalLocation(), Constants::PERMISSION_UPDATE)) {
if (!$this->userHasAccessToPath($item->getUser(), $item->getPath(), Constants::PERMISSION_UPDATE)) {
throw new NotPermittedException();
}
$folderPermissions = $this->folderManager->getFolderPermissionsForUser($item->getUser(), (int)$folderId);
Expand Down Expand Up @@ -191,7 +191,7 @@ public function removeItem(ITrashItem $item) {
if ($node->getStorage()->unlink($node->getInternalPath()) === false) {
throw new \Exception('Failed to remove item from trashbin');
}
if (!$this->userHasAccessToPath($item->getUser(), $folderId . '/' . $item->getOriginalLocation(), Constants::PERMISSION_DELETE)) {
if (!$this->userHasAccessToPath($item->getUser(), $item->getPath(), Constants::PERMISSION_DELETE)) {
throw new NotPermittedException();
}

Expand Down Expand Up @@ -253,7 +253,7 @@ private function userHasAccessToPath(
int $permission = Constants::PERMISSION_READ
): bool {
$activePermissions = $this->aclManagerFactory->getACLManager($user)
->getACLPermissionsForPath('__groupfolders/' . ltrim($path, '/'));
->getACLPermissionsForPath($path);
return (bool)($activePermissions & $permission);
}

Expand All @@ -265,7 +265,7 @@ private function getNodeForTrashItem(IUser $user, ITrashItem $trashItem): ?Node
$trashRoot = $this->getTrashFolder((int)$folderId);
try {
$node = $trashRoot->get($path);
if (!$this->userHasAccessToPath($user, $folderId . '/' . $trashItem->getOriginalLocation())) {
if (!$this->userHasAccessToPath($user, $trashItem->getPath())) {
return null;
}
return $node;
Expand Down Expand Up @@ -321,12 +321,12 @@ private function getTrashForFolders(IUser $user, array $folders): array {
$timestamp = (int)substr($pathParts['extension'], 1);
$name = $pathParts['filename'];
$key = $folderId . '/' . $name . '/' . $timestamp;
$originalLocation = isset($indexedRows[$key]) ? $indexedRows[$key]['original_location'] : '';
if (!$this->userHasAccessToPath($user, $folderId . '/' . $originalLocation)) {
if (!$this->userHasAccessToPath($user, $item->getPath())) {
continue;
}
$info = $item->getFileInfo();
$info['name'] = $name;
$originalLocation = isset($indexedRows[$key]) ? $indexedRows[$key]['original_location'] : '';
$items[] = new GroupTrashItem(
$this,
$originalLocation,
Expand Down Expand Up @@ -354,13 +354,7 @@ public function getTrashNodeById(IUser $user, int $fileId): ?Node {
$relativePath = $trashFolder->getRelativePath($absolutePath);
[, $folderId, $nameAndTime] = explode('/', $relativePath);

if (substr_count($relativePath, "/") > 2) {
// fileid is a file inside a deleted folder
$fileId = $trashFolder->get($folderId . "/" . $nameAndTime)->getId();
}
$trashItem = $this->trashManager->getTrashItemByFileId($fileId);
$originalPath = $folderId . '/' . ($trashItem ? $trashItem['original_location'] : '/');
if ($this->userHasAccessToFolder($user, (int)$folderId) && $this->userHasAccessToPath($user, $originalPath)) {
if ($this->userHasAccessToFolder($user, (int)$folderId) && $this->userHasAccessToPath($user, $absolutePath)) {
return $trashFolder->get($relativePath);
} else {
return null;
Expand Down
10 changes: 10 additions & 0 deletions lib/Trash/TrashManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,16 @@ public function getTrashItemByFileId(int $fileId): ?array {
return $query->executeQuery()->fetch() ?: null;
}

public function getTrashItemByFileName(int $folderId, string $name, int $deletedTime): ?array {
$query = $this->connection->getQueryBuilder();
$query->select(['trash_id', 'name', 'deleted_time', 'original_location', 'folder_id'])
->from('group_folders_trash')
->where($query->expr()->eq('folder_id', $query->createNamedParameter($folderId, IQueryBuilder::PARAM_INT)))
->andWhere($query->expr()->eq('name', $query->createNamedParameter($name)))
->andWhere($query->expr()->eq('deleted_time', $query->createNamedParameter($deletedTime, IQueryBuilder::PARAM_INT)));
return $query->executeQuery()->fetch() ?: null;
}

public function removeItem(int $folderId, string $name, int $deletedTime): void {
$query = $this->connection->getQueryBuilder();
$query->delete('group_folders_trash')
Expand Down
36 changes: 34 additions & 2 deletions tests/ACL/ACLManagerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,18 @@
use OCA\GroupFolders\ACL\Rule;
use OCA\GroupFolders\ACL\RuleManager;
use OCA\GroupFolders\ACL\UserMapping\IUserMapping;
use OCA\GroupFolders\Trash\TrashManager;
use OCP\Constants;
use OCP\Files\IRootFolder;
use OCP\Files\Mount\IMountPoint;
use OCP\IUser;
use Test\TestCase;

class ACLManagerTest extends TestCase {
/** @var RuleManager|\PHPUnit_Framework_MockObject_MockObject */
/** @var RuleManager */
private $ruleManager;
/** @var TrashManager */
private $trashManager;
/** @var IUser */
private $user;
/** @var ACLManager */
Expand All @@ -56,7 +59,8 @@ protected function setUp(): void {
$rootFolder = $this->createMock(IRootFolder::class);
$rootFolder->method('getMountPoint')
->willReturn($rootMountPoint);
$this->aclManager = new ACLManager($this->ruleManager, $this->user, function () use ($rootFolder) {
$this->trashManager = $this->createMock(TrashManager::class);
$this->aclManager = new ACLManager($this->ruleManager, $this->trashManager, $this->user, function () use ($rootFolder) {
return $rootFolder;
});
$this->dummyMapping = $this->createMock(IUserMapping::class);
Expand Down Expand Up @@ -95,4 +99,32 @@ public function testGetACLPermissionsForPath() {
$this->assertEquals(Constants::PERMISSION_ALL - Constants::PERMISSION_SHARE, $this->aclManager->getACLPermissionsForPath('foo/bar'));
$this->assertEquals(Constants::PERMISSION_ALL, $this->aclManager->getACLPermissionsForPath('foo/bar/sub'));
}

public function testGetACLPermissionsForPathInTrashbin() {
$this->rules = [
'__groupfolders/1' => [
new Rule($this->dummyMapping, 10, Constants::PERMISSION_READ + Constants::PERMISSION_UPDATE, Constants::PERMISSION_READ), // read only
new Rule($this->dummyMapping, 10, Constants::PERMISSION_SHARE, 0) // deny share
],
'__groupfolders/1/subfolder' => [
new Rule($this->dummyMapping, 10, Constants::PERMISSION_UPDATE, Constants::PERMISSION_UPDATE) // add write
],
'__groupfolders/trash/1/subfolder2.d1700752274' => [
new Rule($this->dummyMapping, 10, Constants::PERMISSION_SHARE, Constants::PERMISSION_SHARE) // add share
]
];

$this->trashManager
->expects($this->once())
->method('getTrashItemByFileName')
->with(1, 'subfolder2', 1700752274)
->willReturn([
'trash_id' => 3,
'name' => 'subfolder2',
'deleted_time' => '1700752274',
'original_location' => 'subfolder/subfolder2',
'folder_id' => '1',
]);
$this->assertEquals(Constants::PERMISSION_ALL, $this->aclManager->getACLPermissionsForPath('__groupfolders/trash/1/subfolder2.d1700752274/coucou.md'));
}
}

0 comments on commit 96521d0

Please sign in to comment.