diff --git a/lib/Service/CollectiveService.php b/lib/Service/CollectiveService.php index 5695e1c25..172a1e5c1 100644 --- a/lib/Service/CollectiveService.php +++ b/lib/Service/CollectiveService.php @@ -337,14 +337,23 @@ public function trashCollective(int $id, string $userId): Collective { */ public function deleteCollective(int $id, string $userId, bool $deleteCircle): Collective { $collective = $this->getCollectiveFromTrash($id, $userId); + $circleId = $collective->getCircleId(); + + // Check permission up front to not purge the collective and then fail to destroy the team + if ($deleteCircle && !$this->circleHelper->isOwner($circleId, $userId)) { + throw new NotPermittedException('Not allowed to destroy team ' . $circleId); + } + + // Purge before touching the team: the collective can only be found in trash while its team exists. + $collective = $this->purgeCollective($collective); if ($deleteCircle) { - $this->circleHelper->destroyCircle($collective->getCircleId(), $userId); + $this->circleHelper->destroyCircle($circleId, $userId); } else { - $this->circleHelper->unflagCircleAsAppManaged($collective->getCircleId()); + $this->circleHelper->unflagCircleAsAppManaged($circleId); } - return $this->purgeCollective($collective); + return $collective; } /** diff --git a/lib/Versions/VersionsBackend.php b/lib/Versions/VersionsBackend.php index 23e33a839..67b25d3ee 100644 --- a/lib/Versions/VersionsBackend.php +++ b/lib/Versions/VersionsBackend.php @@ -333,11 +333,8 @@ public function deleteVersionsFolder(int $folderId): void { /** @var Folder $versionsFolder */ $versionsFolder = $this->getVersionsFolder($folderId); foreach ($versionsFolder->getDirectoryListing() as $fileFolder) { - if (!($fileFolder instanceof Folder)) { - continue; - } - foreach ($fileFolder->getDirectoryListing() as $file) { - $this->deleteAllVersionsForFile($folderId, $file->getId()); + if ($fileFolder instanceof Folder) { + $this->collectiveVersionMapper->deleteAllVersionsForFileId((int)$fileFolder->getName()); } } $versionsFolder->delete(); diff --git a/playwright/e2e/page-links-viewer.spec.ts b/playwright/e2e/page-links-viewer.spec.ts index 9ae42c21b..b48f7f4af 100644 --- a/playwright/e2e/page-links-viewer.spec.ts +++ b/playwright/e2e/page-links-viewer.spec.ts @@ -32,14 +32,12 @@ const links: ViewerLinkTestCaseData[] = [ getLinkUrl: ({ fileId }) => `/index.php/f/${fileId}`, fixtureName: 'test.md', mimetype: 'text/markdown', - getPath: () => '', }, { description: 'absolute files app path to image file', getLinkUrl: ({ fileId }) => `/index.php/f/${fileId}`, fixtureName: 'test.png', mimetype: 'image/png', - getPath: () => '', }, { description: 'absolute files app path to PDF file', @@ -57,9 +55,12 @@ test.describe('Links to viewer', () => { test(`Opens link with ${linkData.description} in viewer (${modeLabel} mode)`, async ({ collective, editor, page, user }) => { const sourcePage = collective.getPageByTitle('Link Source') + // Upload into a fresh folder, as the Text editor of a previous test may still hold a lock on a file with the same name. + const path = linkData.getPath?.({ sourcePage }) + ?? await user.createFolder({ path: randomString() }, page) const fileId = await user.uploadFixture({ name: linkData.fixtureName, - path: linkData.getPath({ sourcePage }), + path, mimetype: linkData.mimetype, }, page) diff --git a/playwright/e2e/page-picker.spec.ts b/playwright/e2e/page-picker.spec.ts index 72c3ecad3..de370fff6 100644 --- a/playwright/e2e/page-picker.spec.ts +++ b/playwright/e2e/page-picker.spec.ts @@ -90,12 +90,18 @@ test.describe('Custom page picker - cross-collective search', () => { editor.setMode(true) await editor.clickMenu('Insert link', 'Link to page') await page.waitForTimeout(200) - }) - test('link to page from other collective', async ({ page, editor }) => { - await editor.pagePicker.locator('.searchbar [aria-haspopup="menu"]').click() + const filterMenu = editor.pagePicker.locator('.searchbar [aria-haspopup="menu"]') + await filterMenu.click() await page.getByText('Limit to current collective').click() + // The menu stays open with focus on the checkbox. Close it, else typing + // a space into the search field may toggle the checkbox again. + await filterMenu.click() + await expect(page.getByText('Limit to current collective')).toBeHidden() + }) + + test('link to page from other collective', async ({ page, editor }) => { // Should already be listed without searching const otherPageItem = editor.pagePicker.locator('.page-preview-item').filter({ hasText: 'Other collective page' }) await expect(otherPageItem).toBeVisible() @@ -114,10 +120,7 @@ test.describe('Custom page picker - cross-collective search', () => { await expect(pageWidget).toHaveAttribute('href', origin + otherTargetPage.getPageUrl()) }) - test('searching "Landing page" lists landing page', async ({ page, editor }) => { - await editor.pagePicker.locator('.searchbar [aria-haspopup="menu"]').click() - await page.getByText('Limit to current collective').click() - + test('searching "Landing page" lists landing page', async ({ editor }) => { await editor.pagePickerSearch.fill('Landing page') await expect(editor.pagePicker.locator('.page-preview-item').filter({ hasText: 'Source page' })).not.toBeVisible() @@ -125,10 +128,7 @@ test.describe('Custom page picker - cross-collective search', () => { await expect(landingPageItem).toHaveCount(2) }) - test('searching collective title lists landing page', async ({ page, collectives, editor }) => { - await editor.pagePicker.locator('.searchbar [aria-haspopup="menu"]').click() - await page.getByText('Limit to current collective').click() - + test('searching collective title lists landing page', async ({ collectives, editor }) => { await editor.pagePickerSearch.fill(collectives[1].data.name) await expect(editor.pagePicker.locator('.page-preview-item').filter({ hasText: 'Source page' })).not.toBeVisible() diff --git a/playwright/support/fixtures/User.ts b/playwright/support/fixtures/User.ts index d6ff48908..3b52c5903 100644 --- a/playwright/support/fixtures/User.ts +++ b/playwright/support/fixtures/User.ts @@ -23,6 +23,14 @@ export class User { return await trashAndDeleteCollective({ ...options, page }) } + async createFolder(options: { path: string }, page: Page) { + await page.request.fetch( + webdavUrl(this.account.userId, options.path), + { method: 'MKCOL', failOnStatusCode: true }, + ) + return options.path + } + async uploadFixture(options: { name: string, path: string, mimetype: string }, page: Page) { const filepath = resolve(import.meta.dirname, 'files', options.name) await page.request.put( diff --git a/playwright/support/helpers/links.ts b/playwright/support/helpers/links.ts index 59fd31428..cf2b9961f 100644 --- a/playwright/support/helpers/links.ts +++ b/playwright/support/helpers/links.ts @@ -26,7 +26,8 @@ export type ViewerLinkTestCaseData = { getLinkUrl: (params: { fileId: number }) => string fixtureName: string mimetype: string - getPath: (params: { sourcePage: CollectivePage }) => string + // Defaults to a new folder in the user's home + getPath?: (params: { sourcePage: CollectivePage }) => string } export type SameTabLinkTestCaseData = { diff --git a/tests/Unit/Service/CollectiveServiceTest.php b/tests/Unit/Service/CollectiveServiceTest.php index 2e4c24ef1..8a5e9d400 100644 --- a/tests/Unit/Service/CollectiveServiceTest.php +++ b/tests/Unit/Service/CollectiveServiceTest.php @@ -24,11 +24,14 @@ use OCA\Collectives\Service\CollectiveService; use OCA\Collectives\Service\CollectiveShareService; use OCA\Collectives\Service\NotFoundException; +use OCA\Collectives\Service\NotPermittedException; use OCA\Collectives\Service\UnprocessableEntityException; use OCP\App\IAppManager; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\File; use OCP\Files\Folder; +use OCP\Files\InvalidPathException; +use OCP\Files\NotFoundException as FilesNotFoundException; use OCP\IL10N; use Symfony\Component\String\Slugger\SluggerInterface; use Symfony\Component\String\UnicodeString; @@ -38,6 +41,7 @@ class CollectiveServiceTest extends TestCase { private string $userId = 'jane'; private CollectiveMapper $collectiveMapper; private CollectiveHelper $collectiveHelper; + private CollectiveFolderManager $collectiveFolderManager; private CircleHelper $circleHelper; private IL10N $l10n; private CollectiveService $service; @@ -48,13 +52,13 @@ protected function setUp(): void { $this->collectiveMapper = $this->createMock(CollectiveMapper::class); $this->collectiveHelper = $this->createMock(CollectiveHelper::class); - $collectiveFolderManager = $this->createMock(CollectiveFolderManager::class); + $this->collectiveFolderManager = $this->createMock(CollectiveFolderManager::class); $folder = $this->createMock(Folder::class); $file = $this->createMock(File::class); $folder->method('get') ->willReturn($file); - $collectiveFolderManager->method('initializeFolder') + $this->collectiveFolderManager->method('initializeFolder') ->willReturn($folder); $this->circleHelper = $this->createMock(CircleHelper::class); @@ -77,7 +81,7 @@ protected function setUp(): void { $appManager, $this->collectiveMapper, $this->collectiveHelper, - $collectiveFolderManager, + $this->collectiveFolderManager, $this->circleHelper, $shareService, $collectiveUserSettingsMapper, @@ -209,4 +213,101 @@ public function testCreate(): void { 'canLeave' => true, ], $collective->jsonSerialize()); } + + private function mockTrashedCollective(): Collective { + $collective = new Collective(); + $collective->setId(123); + $collective->setCircleId('CircleId'); + $collective->setTrashTimestamp(1700000000); + $this->collectiveMapper->method('findTrashByIdAndUser') + ->with(123, $this->userId) + ->willReturn($collective); + return $collective; + } + + public function testDeleteCollectiveDestroysCircleAfterPurge(): void { + $collective = $this->mockTrashedCollective(); + $this->circleHelper->method('isOwner') + ->willReturn(true); + $this->collectiveFolderManager->method('getFolder') + ->willThrowException(new FilesNotFoundException()); + + $calls = []; + $this->collectiveMapper->expects(self::once()) + ->method('delete') + ->with($collective) + ->willReturnCallback(function (Collective $collective) use (&$calls): Collective { + $calls[] = 'purge'; + return $collective; + }); + $this->circleHelper->expects(self::once()) + ->method('destroyCircle') + ->with('CircleId', $this->userId) + ->willReturnCallback(function () use (&$calls): void { + $calls[] = 'destroyCircle'; + }); + + $this->service->deleteCollective(123, $this->userId, true); + self::assertSame(['purge', 'destroyCircle'], $calls); + } + + public function testDeleteCollectiveKeepCircleUnflagsCircleAfterPurge(): void { + $collective = $this->mockTrashedCollective(); + $this->collectiveFolderManager->method('getFolder') + ->willThrowException(new FilesNotFoundException()); + + $calls = []; + $this->collectiveMapper->expects(self::once()) + ->method('delete') + ->with($collective) + ->willReturnCallback(function (Collective $collective) use (&$calls): Collective { + $calls[] = 'purge'; + return $collective; + }); + $this->circleHelper->expects(self::never()) + ->method('destroyCircle'); + $this->circleHelper->expects(self::once()) + ->method('unflagCircleAsAppManaged') + ->with('CircleId') + ->willReturnCallback(function () use (&$calls): void { + $calls[] = 'unflagCircle'; + }); + + $this->service->deleteCollective(123, $this->userId, false); + self::assertSame(['purge', 'unflagCircle'], $calls); + } + + public function testDeleteCollectiveKeepsCircleIfPurgeFails(): void { + $this->mockTrashedCollective(); + $this->circleHelper->method('isOwner') + ->willReturn(true); + $this->collectiveFolderManager->method('getFolder') + ->willThrowException(new InvalidPathException()); + + $this->collectiveMapper->expects(self::never()) + ->method('delete'); + $this->circleHelper->expects(self::never()) + ->method('destroyCircle'); + $this->circleHelper->expects(self::never()) + ->method('unflagCircleAsAppManaged'); + + $this->expectException(NotFoundException::class); + $this->service->deleteCollective(123, $this->userId, true); + } + + public function testDeleteCollectiveAndCircleAsNonOwnerFailsBeforePurge(): void { + $this->mockTrashedCollective(); + $this->circleHelper->method('isOwner') + ->willReturn(false); + + $this->collectiveFolderManager->expects(self::never()) + ->method('getFolder'); + $this->collectiveMapper->expects(self::never()) + ->method('delete'); + $this->circleHelper->expects(self::never()) + ->method('destroyCircle'); + + $this->expectException(NotPermittedException::class); + $this->service->deleteCollective(123, $this->userId, true); + } }