From 85795d6a32dc0cf1cefcc1f1b2f538eb6e0776d4 Mon Sep 17 00:00:00 2001 From: Jonas Date: Wed, 13 Mar 2024 17:21:15 +0100 Subject: [PATCH 01/18] fix(backend): Reset document session and yjs file when file is deleted Signed-off-by: Jonas --- lib/Listeners/BeforeNodeDeletedListener.php | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/Listeners/BeforeNodeDeletedListener.php b/lib/Listeners/BeforeNodeDeletedListener.php index 2f1143b4e0d..a1e0cc0864b 100644 --- a/lib/Listeners/BeforeNodeDeletedListener.php +++ b/lib/Listeners/BeforeNodeDeletedListener.php @@ -26,6 +26,7 @@ namespace OCA\Text\Listeners; use OCA\Text\Service\AttachmentService; +use OCA\Text\Service\DocumentService; use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; use OCP\Files\Events\Node\BeforeNodeDeletedEvent; @@ -35,10 +36,13 @@ * @template-implements IEventListener */ class BeforeNodeDeletedListener implements IEventListener { - private $attachmentService; + private AttachmentService $attachmentService; + private DocumentService $documentService; - public function __construct(AttachmentService $attachmentService) { + public function __construct(AttachmentService $attachmentService, + DocumentService $documentService) { $this->attachmentService = $attachmentService; + $this->documentService = $documentService; } public function handle(Event $event): void { @@ -48,6 +52,7 @@ public function handle(Event $event): void { $node = $event->getNode(); if ($node instanceof File && $node->getMimeType() === 'text/markdown') { $this->attachmentService->deleteAttachments($node); + $this->documentService->resetDocument($node->getId(), true); } } } From 8e741dec2a794b7537dc426e0547e9f229c048e5 Mon Sep 17 00:00:00 2001 From: Jonas Date: Wed, 13 Mar 2024 20:54:34 +0100 Subject: [PATCH 02/18] fix(backend): Reset document session when updated from outside editor When a text file is updated via other means than from the editor (e.g. when uploaded/synced via webdav) and there is no unsaved steps in the document session, reset the document session This will prevent conflict resolution dialogs in this case. Client frontends will have to reload the document afterwards though. Signed-off-by: Jonas --- composer/composer/autoload_classmap.php | 1 + composer/composer/autoload_static.php | 1 + lib/AppInfo/Application.php | 3 ++ lib/Listeners/BeforeNodeWrittenListener.php | 56 +++++++++++++++++++++ lib/Service/DocumentService.php | 7 +++ 5 files changed, 68 insertions(+) create mode 100644 lib/Listeners/BeforeNodeWrittenListener.php diff --git a/composer/composer/autoload_classmap.php b/composer/composer/autoload_classmap.php index da85631f4b9..dcff792ea42 100644 --- a/composer/composer/autoload_classmap.php +++ b/composer/composer/autoload_classmap.php @@ -38,6 +38,7 @@ 'OCA\\Text\\Listeners\\BeforeAssistantNotificationListener' => $baseDir . '/../lib/Listeners/BeforeAssistantNotificationListener.php', 'OCA\\Text\\Listeners\\BeforeNodeDeletedListener' => $baseDir . '/../lib/Listeners/BeforeNodeDeletedListener.php', 'OCA\\Text\\Listeners\\BeforeNodeRenamedListener' => $baseDir . '/../lib/Listeners/BeforeNodeRenamedListener.php', + 'OCA\\Text\\Listeners\\BeforeNodeWrittenListener' => $baseDir . '/../lib/Listeners/BeforeNodeWrittenListener.php', 'OCA\\Text\\Listeners\\FilesLoadAdditionalScriptsListener' => $baseDir . '/../lib/Listeners/FilesLoadAdditionalScriptsListener.php', 'OCA\\Text\\Listeners\\FilesSharingLoadAdditionalScriptsListener' => $baseDir . '/../lib/Listeners/FilesSharingLoadAdditionalScriptsListener.php', 'OCA\\Text\\Listeners\\LoadEditorListener' => $baseDir . '/../lib/Listeners/LoadEditorListener.php', diff --git a/composer/composer/autoload_static.php b/composer/composer/autoload_static.php index 8732ef4a1dc..a172981b0ef 100644 --- a/composer/composer/autoload_static.php +++ b/composer/composer/autoload_static.php @@ -53,6 +53,7 @@ class ComposerStaticInitText 'OCA\\Text\\Listeners\\BeforeAssistantNotificationListener' => __DIR__ . '/..' . '/../lib/Listeners/BeforeAssistantNotificationListener.php', 'OCA\\Text\\Listeners\\BeforeNodeDeletedListener' => __DIR__ . '/..' . '/../lib/Listeners/BeforeNodeDeletedListener.php', 'OCA\\Text\\Listeners\\BeforeNodeRenamedListener' => __DIR__ . '/..' . '/../lib/Listeners/BeforeNodeRenamedListener.php', + 'OCA\\Text\\Listeners\\BeforeNodeWrittenListener' => __DIR__ . '/..' . '/../lib/Listeners/BeforeNodeWrittenListener.php', 'OCA\\Text\\Listeners\\FilesLoadAdditionalScriptsListener' => __DIR__ . '/..' . '/../lib/Listeners/FilesLoadAdditionalScriptsListener.php', 'OCA\\Text\\Listeners\\FilesSharingLoadAdditionalScriptsListener' => __DIR__ . '/..' . '/../lib/Listeners/FilesSharingLoadAdditionalScriptsListener.php', 'OCA\\Text\\Listeners\\LoadEditorListener' => __DIR__ . '/..' . '/../lib/Listeners/LoadEditorListener.php', diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 7fdf15251a7..563117f61b0 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -32,6 +32,7 @@ use OCA\Text\Listeners\BeforeAssistantNotificationListener; use OCA\Text\Listeners\BeforeNodeDeletedListener; use OCA\Text\Listeners\BeforeNodeRenamedListener; +use OCA\Text\Listeners\BeforeNodeWrittenListener; use OCA\Text\Listeners\FilesLoadAdditionalScriptsListener; use OCA\Text\Listeners\FilesSharingLoadAdditionalScriptsListener; use OCA\Text\Listeners\LoadEditorListener; @@ -51,6 +52,7 @@ use OCP\DirectEditing\RegisterDirectEditorEvent; use OCP\Files\Events\Node\BeforeNodeDeletedEvent; use OCP\Files\Events\Node\BeforeNodeRenamedEvent; +use OCP\Files\Events\Node\BeforeNodeWrittenEvent; use OCP\Files\Events\Node\NodeCopiedEvent; use OCP\Files\Template\ITemplateManager; use OCP\Files\Template\TemplateFileCreator; @@ -71,6 +73,7 @@ public function register(IRegistrationContext $context): void { $context->registerEventListener(LoadEditor::class, LoadEditorListener::class); // for attachments $context->registerEventListener(NodeCopiedEvent::class, NodeCopiedListener::class); + $context->registerEventListener(BeforeNodeWrittenEvent::class, BeforeNodeWrittenListener::class); $context->registerEventListener(BeforeNodeRenamedEvent::class, BeforeNodeRenamedListener::class); $context->registerEventListener(BeforeNodeDeletedEvent::class, BeforeNodeDeletedListener::class); $context->registerEventListener(AddMissingIndicesEvent::class, AddMissingIndicesListener::class); diff --git a/lib/Listeners/BeforeNodeWrittenListener.php b/lib/Listeners/BeforeNodeWrittenListener.php new file mode 100644 index 00000000000..ec6f28e48e5 --- /dev/null +++ b/lib/Listeners/BeforeNodeWrittenListener.php @@ -0,0 +1,56 @@ + + * + * @author Jonas + * + * @license GNU AGPL version 3 or any later version + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + * + */ + +namespace OCA\Text\Listeners; + +use OCA\Text\Service\DocumentService; +use OCP\EventDispatcher\Event; +use OCP\EventDispatcher\IEventListener; +use OCP\Files\Events\Node\BeforeNodeWrittenEvent; +use OCP\Files\File; + +/** + * @template-implements IEventListener + */ +class BeforeNodeWrittenListener implements IEventListener { + private DocumentService $documentService; + + public function __construct(DocumentService $documentService) { + $this->documentService = $documentService; + } + + public function handle(Event $event): void { + if (!$event instanceof BeforeNodeWrittenEvent) { + return; + } + $node = $event->getNode(); + if ($node instanceof File && $node->getMimeType() === 'text/markdown') { + if (!$this->documentService->isSaveFromText()) { + // Reset document session to avoid manual conflict resolution if there's no unsaved steps + $this->documentService->resetDocument($node->getId()); + } + } + } +} diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index 6291cb706a9..656dc6c5e9e 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -74,6 +74,8 @@ class DocumentService { */ public const AUTOSAVE_MINIMUM_DELAY = 10; + private bool $saveFromText = false; + private ?string $userId; private DocumentMapper $documentMapper; private SessionMapper $sessionMapper; @@ -118,6 +120,10 @@ public function getDocument(int $id): ?Document { } } + public function isSaveFromText(): bool { + return $this->saveFromText; + } + /** * @throws NotFoundException * @throws InvalidPathException @@ -389,6 +395,7 @@ public function autosave(Document $document, ?File $file, int $version, ?string ILock::TYPE_APP, Application::APP_NAME ), function () use ($file, $autoSaveDocument, $documentState) { + $this->saveFromText = true; $file->putContent($autoSaveDocument); if ($documentState) { $this->writeDocumentState($file->getId(), $documentState); From 8e1ad169ec24ffad4468535ff6c091d994f2e26b Mon Sep 17 00:00:00 2001 From: Jonas Date: Wed, 13 Mar 2024 21:06:06 +0100 Subject: [PATCH 03/18] fix(backend): Remove yjs file and all steps when resetting document session Instead of just deleting the newest steps, always remove all session data: document, sessions and steps from the database as well as the yjs (document state) file. Without the `--force` option, don't reset document sessions with unsaved steps. Signed-off-by: Jonas --- lib/Command/ResetDocument.php | 45 ++++++++++++--------------------- lib/Cron/Cleanup.php | 15 ++++------- lib/Service/DocumentService.php | 4 +-- 3 files changed, 22 insertions(+), 42 deletions(-) diff --git a/lib/Command/ResetDocument.php b/lib/Command/ResetDocument.php index 65a614e2f1a..0864ccb68ec 100644 --- a/lib/Command/ResetDocument.php +++ b/lib/Command/ResetDocument.php @@ -23,9 +23,7 @@ namespace OCA\Text\Command; -use OCA\Text\Db\DocumentMapper; -use OCA\Text\Db\SessionMapper; -use OCA\Text\Db\StepMapper; +use OCA\Text\Exception\DocumentHasUnsavedChangesException; use OCA\Text\Service\DocumentService; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; @@ -34,33 +32,26 @@ class ResetDocument extends Command { protected DocumentService $documentService; - protected DocumentMapper $documentMapper; - protected StepMapper $stepMapper; - protected SessionMapper $sessionMapper; - public function __construct(DocumentService $documentService, DocumentMapper $documentMapper, StepMapper $stepMapper, SessionMapper $sessionMapper) { + public function __construct(DocumentService $documentService) { parent::__construct(); - $this->documentService = $documentService; - $this->documentMapper = $documentMapper; - $this->stepMapper = $stepMapper; - $this->sessionMapper = $sessionMapper; } protected function configure(): void { $this ->setName('text:reset') - ->setDescription('Reset a text document') + ->setDescription('Reset a text document session to the current file content') ->addArgument( 'file-id', InputArgument::REQUIRED, - 'File id of the document to rest' + 'File id of the document to reset' ) ->addOption( - 'full', + 'force', 'f', null, - 'Drop all existing steps and use the currently saved version' + 'Reset the document session even with unsaved changes' ) ; } @@ -72,27 +63,23 @@ protected function configure(): void { */ protected function execute(InputInterface $input, OutputInterface $output): int { $fileId = $input->getArgument('file-id'); - $fullReset = $input->getOption('full'); + $fullReset = $input->getOption('force'); if ($fullReset) { - $output->writeln('Full document reset'); + $output->writeln('Force-reset the document session for file ' . $fileId); $this->documentService->resetDocument($fileId, true); return 0; - } else { - $output->writeln('Trying to restore to last saved version'); - $document = $this->documentMapper->find($fileId); - $deleted = $this->stepMapper->deleteAfterVersion($fileId, $document->getLastSavedVersion()); - if ($deleted > 0) { - $this->sessionMapper->deleteByDocumentId($fileId); - $output->writeln('Reverted document to the last saved version'); - - return 0; - } else { - $output->writeln('Failed revert changes that are newer than the last saved version'); - } + } + $output->writeln('Reset the document session for file ' . $fileId); + try { + $this->documentService->resetDocument($fileId); + } catch (DocumentHasUnsavedChangesException) { + $output->writeln('Not resetting due to unsaved changes'); return 1; } + + return 0; } } diff --git a/lib/Cron/Cleanup.php b/lib/Cron/Cleanup.php index 19933cfc17c..e374e742f10 100644 --- a/lib/Cron/Cleanup.php +++ b/lib/Cron/Cleanup.php @@ -28,7 +28,7 @@ namespace OCA\Text\Cron; -use OCA\Text\Service\AttachmentService; +use OCA\Text\Exception\DocumentHasUnsavedChangesException; use OCA\Text\Service\DocumentService; use OCA\Text\Service\SessionService; use OCP\AppFramework\Utility\ITimeFactory; @@ -39,26 +39,22 @@ class Cleanup extends TimedJob { private SessionService $sessionService; private DocumentService $documentService; private LoggerInterface $logger; - private AttachmentService $attachmentService; public function __construct(ITimeFactory $time, SessionService $sessionService, DocumentService $documentService, - AttachmentService $attachmentService, LoggerInterface $logger) { parent::__construct($time); $this->sessionService = $sessionService; $this->documentService = $documentService; - $this->attachmentService = $attachmentService; $this->logger = $logger; $this->setInterval(SessionService::SESSION_VALID_TIME); } /** * @param array $argument - * @return void */ - protected function run($argument) { + protected function run($argument): void { $this->logger->debug('Run cleanup job for text documents'); $documents = $this->documentService->getAll(); foreach ($documents as $document) { @@ -69,11 +65,10 @@ protected function run($argument) { continue; } - if ($this->documentService->hasUnsavedChanges($document)) { - continue; + try { + $this->documentService->resetDocument($document->getId()); + } catch (DocumentHasUnsavedChangesException) { } - - $this->documentService->resetDocument($document->getId()); } $this->logger->debug('Run cleanup job for text sessions'); diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index 656dc6c5e9e..cb1b216e069 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -432,10 +432,8 @@ public function resetDocument(int $documentId, bool $force = false): void { $this->stepMapper->deleteAll($documentId); $this->sessionMapper->deleteByDocumentId($documentId); $this->documentMapper->delete($document); + $this->getStateFile($documentId)->delete(); - if ($force) { - $this->getStateFile($documentId)->delete(); - } $this->logger->debug('document reset for ' . $documentId); } catch (DoesNotExistException|NotFoundException $e) { // Ignore if document not found or state file not found From cc2e6c30c2260116fe34a78b9625404175d04824 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Fri, 15 Mar 2024 16:04:44 +0100 Subject: [PATCH 04/18] fix: catch expected exception in event handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Listeners/BeforeNodeWrittenListener.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/Listeners/BeforeNodeWrittenListener.php b/lib/Listeners/BeforeNodeWrittenListener.php index ec6f28e48e5..1079d7c1b9c 100644 --- a/lib/Listeners/BeforeNodeWrittenListener.php +++ b/lib/Listeners/BeforeNodeWrittenListener.php @@ -25,6 +25,7 @@ namespace OCA\Text\Listeners; +use OCA\Text\Exception\DocumentHasUnsavedChangesException; use OCA\Text\Service\DocumentService; use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; @@ -49,7 +50,11 @@ public function handle(Event $event): void { if ($node instanceof File && $node->getMimeType() === 'text/markdown') { if (!$this->documentService->isSaveFromText()) { // Reset document session to avoid manual conflict resolution if there's no unsaved steps - $this->documentService->resetDocument($node->getId()); + try { + $this->documentService->resetDocument($node->getId()); + } catch (DocumentHasUnsavedChangesException) { + // Do not throw during event handling in this is expected to happen + } } } } From aa236cf8d8f177edaf14ccaafea8661bb9ed02b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Wed, 13 Mar 2024 17:08:23 +0100 Subject: [PATCH 05/18] fix: Clean up logic to return document state file or file content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- cypress/e2e/api/SessionApi.spec.js | 4 ++-- lib/Service/ApiService.php | 14 ++++---------- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/cypress/e2e/api/SessionApi.spec.js b/cypress/e2e/api/SessionApi.spec.js index 6b8ffe74314..6a102316a71 100644 --- a/cypress/e2e/api/SessionApi.spec.js +++ b/cypress/e2e/api/SessionApi.spec.js @@ -315,7 +315,7 @@ describe('The session Api', function() { }) }) - it('sends initial content if other session is alive but did not push any steps', function() { + it('does not send initial content if other session is alive but did not push any steps', function() { let joining cy.createTextSession(undefined, { filePath: '', shareToken }) .then(con => { @@ -323,7 +323,7 @@ describe('The session Api', function() { return con }) .its('state.documentSource') - .should('not.eql', '') + .should('eql', '') .then(() => joining.close()) .then(() => connection.close()) }) diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php index bcd89e4ded6..9d3fd6410e3 100644 --- a/lib/Service/ApiService.php +++ b/lib/Service/ApiService.php @@ -136,25 +136,19 @@ public function create(?int $fileId = null, ?string $filePath = null, ?string $t $session = $this->sessionService->initSession($document->getId(), $guestName); + $documentState = null; + $content = null; if ($freshSession) { $this->logger->debug('Starting a fresh editing session for ' . $file->getId()); - $documentState = null; $content = $this->loadContent($file); } else { $this->logger->debug('Loading existing session for ' . $file->getId()); - $content = null; try { $stateFile = $this->documentService->getStateFile($document->getId()); $documentState = $stateFile->getContent(); + $this->logger->debug('Existing document, state file loaded ' . $file->getId()); } catch (NotFoundException $e) { - $this->logger->debug('State file not found for ' . $file->getId()); - $documentState = ''; // no state saved yet. - // If there are no steps yet we might still need the content. - $steps = $this->documentService->getSteps($document->getId(), 0); - if (empty($steps)) { - $this->logger->debug('Empty steps, loading content for ' . $file->getId()); - $content = $this->loadContent($file); - } + $this->logger->debug('Existing document, but state file not found for ' . $file->getId()); } } From c2798f20ff8637dbf52b78dfd07be629c312e32d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Julius=20H=C3=A4rtl?= Date: Wed, 13 Mar 2024 17:08:54 +0100 Subject: [PATCH 06/18] fix: Set base version etag to a unique id per document creation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Julius Härtl --- lib/Service/DocumentService.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php index cb1b216e069..38e6c4f2cba 100644 --- a/lib/Service/DocumentService.php +++ b/lib/Service/DocumentService.php @@ -156,7 +156,7 @@ public function createDocument(File $file): Document { $document->setLastSavedVersion(0); $document->setLastSavedVersionTime($file->getMTime()); $document->setLastSavedVersionEtag($file->getEtag()); - $document->setBaseVersionEtag($file->getEtag()); + $document->setBaseVersionEtag(uniqid()); try { /** @var Document $document */ $document = $this->documentMapper->insert($document); From ae78252ee7e8a472841e6424ddc224f86c22e112 Mon Sep 17 00:00:00 2001 From: Jonas Date: Fri, 15 Mar 2024 11:53:24 +0100 Subject: [PATCH 07/18] fix(sync): If `baseVersionEtag` changed, reset frontend `baseVersionEtag` changes when a new document session got initialized, e.g. after an old document session without session clients got cleaned up, or because the markdown file got changed via webdav. Detect this in the client and ask the user to reload the page for resetting the session. Signed-off-by: Jonas --- lib/Controller/PublicSessionController.php | 4 ++-- lib/Controller/SessionController.php | 4 ++-- lib/Service/ApiService.php | 5 ++++- src/components/Editor.vue | 1 + src/services/SessionApi.js | 3 ++- src/services/SyncService.js | 6 ++++-- 6 files changed, 15 insertions(+), 8 deletions(-) diff --git a/lib/Controller/PublicSessionController.php b/lib/Controller/PublicSessionController.php index 889f6634eed..8d50478a414 100644 --- a/lib/Controller/PublicSessionController.php +++ b/lib/Controller/PublicSessionController.php @@ -80,8 +80,8 @@ protected function isPasswordProtected(): bool { #[NoAdminRequired] #[PublicPage] - public function create(string $token, string $file = null, ?string $guestName = null): DataResponse { - return $this->apiService->create(null, $file, $token, $guestName); + public function create(string $token, ?string $file = null, ?string $baseVersionEtag = null, ?string $guestName = null): DataResponse { + return $this->apiService->create(null, $file, $baseVersionEtag, $token, $guestName); } #[NoAdminRequired] diff --git a/lib/Controller/SessionController.php b/lib/Controller/SessionController.php index 9de585d394b..b5de48b19b5 100644 --- a/lib/Controller/SessionController.php +++ b/lib/Controller/SessionController.php @@ -57,8 +57,8 @@ public function __construct( } #[NoAdminRequired] - public function create(int $fileId = null, string $file = null): DataResponse { - return $this->apiService->create($fileId, $file, null, null); + public function create(?int $fileId = null, ?string $file = null, ?string $baseVersionEtag = null): DataResponse { + return $this->apiService->create($fileId, $file, $baseVersionEtag, null, null); } #[NoAdminRequired] diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php index 9d3fd6410e3..95dc3310215 100644 --- a/lib/Service/ApiService.php +++ b/lib/Service/ApiService.php @@ -71,7 +71,7 @@ public function __construct(IRequest $request, $this->l10n = $l10n; } - public function create(?int $fileId = null, ?string $filePath = null, ?string $token = null, ?string $guestName = null): DataResponse { + public function create(?int $fileId = null, ?string $filePath = null, ?string $baseVersionEtag = null, ?string $token = null, ?string $guestName = null): DataResponse { try { if ($token) { $file = $this->documentService->getFileByShareToken($token, $this->request->getParam('filePath')); @@ -115,6 +115,9 @@ public function create(?int $fileId = null, ?string $filePath = null, ?string $t $this->sessionService->removeInactiveSessionsWithoutSteps($file->getId()); $document = $this->documentService->getDocument($file->getId()); $freshSession = $document === null; + if ($baseVersionEtag && $baseVersionEtag !== $document?->getBaseVersionEtag()) { + return new DataResponse($this->l10n->t('Editing session has expired. Please reload the page.'), Http::STATUS_CONFLICT); + } if ($freshSession) { $this->logger->info('Create new document of ' . $file->getId()); diff --git a/src/components/Editor.vue b/src/components/Editor.vue index 66d565bf4a1..952505902b1 100644 --- a/src/components/Editor.vue +++ b/src/components/Editor.vue @@ -370,6 +370,7 @@ export default { guestName, shareToken: this.shareToken, filePath: this.relativePath, + baseVersionEtag: this.$syncService?.baseVersionEtag, forceRecreate: this.forceRecreate, serialize: this.isRichEditor ? (content) => createMarkdownSerializer(this.$editor.schema).serialize(content ?? this.$editor.state.doc) diff --git a/src/services/SessionApi.js b/src/services/SessionApi.js index 69d848b3cd3..96bf299ad0a 100644 --- a/src/services/SessionApi.js +++ b/src/services/SessionApi.js @@ -38,9 +38,10 @@ class SessionApi { this.#options = options } - open({ fileId }) { + open({ fileId, baseVersionEtag }) { return axios.put(this.#url(`session/${fileId}/create`), { fileId, + baseVersionEtag, filePath: this.#options.filePath, token: this.#options.shareToken, guestName: this.#options.guestName, diff --git a/src/services/SyncService.js b/src/services/SyncService.js index fe6ca02737b..427179a80f6 100644 --- a/src/services/SyncService.js +++ b/src/services/SyncService.js @@ -68,7 +68,7 @@ class SyncService { #sendIntervalId - constructor({ serialize, getDocumentState, ...options }) { + constructor({ baseVersionEtag, serialize, getDocumentState, ...options }) { /** @type {import('mitt').Emitter} _bus */ this._bus = mitt() @@ -85,6 +85,7 @@ class SyncService { this.lastStepPush = Date.now() this.version = null + this.baseVersionEtag = baseVersionEtag this.sending = false this.#sendIntervalId = null @@ -101,7 +102,7 @@ class SyncService { const connect = initialSession ? Promise.resolve(new Connection({ data: initialSession }, {})) - : this._api.open({ fileId }) + : this._api.open({ fileId, baseVersionEtag: this.baseVersionEtag }) .catch(error => this._emitError(error)) this.connection = await connect @@ -112,6 +113,7 @@ class SyncService { } this.backend = new PollingBackend(this, this.connection) this.version = this.connection.docStateVersion + this.baseVersionEtag = this.connection.document.baseVersionEtag this.emit('opened', { ...this.connection.state, version: this.version, From f72fb5b03de7512854d5821173c4eb36ccdc35c6 Mon Sep 17 00:00:00 2001 From: Jonas Date: Mon, 18 Mar 2024 19:05:30 +0100 Subject: [PATCH 08/18] fix(Middleware): Response with 412 if `baseVersionEtag` doesn't match Signed-off-by: Jonas --- composer/composer/autoload_classmap.php | 2 ++ composer/composer/autoload_static.php | 2 ++ lib/Controller/PublicSessionController.php | 4 +++ lib/Controller/SessionController.php | 4 +++ ...nvalidDocumentBaseVersionEtagException.php | 9 +++++ .../RequireDocumentBaseVersionEtag.php | 9 +++++ lib/Middleware/SessionMiddleware.php | 33 +++++++++++++++++-- lib/Service/ApiService.php | 6 ++-- src/services/PollingBackend.js | 3 ++ src/services/SessionApi.js | 3 ++ src/services/SyncService.js | 4 ++- 11 files changed, 72 insertions(+), 7 deletions(-) create mode 100644 lib/Exception/InvalidDocumentBaseVersionEtagException.php create mode 100644 lib/Middleware/Attribute/RequireDocumentBaseVersionEtag.php diff --git a/composer/composer/autoload_classmap.php b/composer/composer/autoload_classmap.php index dcff792ea42..dd4f029eb45 100644 --- a/composer/composer/autoload_classmap.php +++ b/composer/composer/autoload_classmap.php @@ -31,6 +31,7 @@ 'OCA\\Text\\Event\\LoadEditor' => $baseDir . '/../lib/Event/LoadEditor.php', 'OCA\\Text\\Exception\\DocumentHasUnsavedChangesException' => $baseDir . '/../lib/Exception/DocumentHasUnsavedChangesException.php', 'OCA\\Text\\Exception\\DocumentSaveConflictException' => $baseDir . '/../lib/Exception/DocumentSaveConflictException.php', + 'OCA\\Text\\Exception\\InvalidDocumentBaseVersionEtagException' => $baseDir . '/../lib/Exception/InvalidDocumentBaseVersionEtagException.php', 'OCA\\Text\\Exception\\InvalidSessionException' => $baseDir . '/../lib/Exception/InvalidSessionException.php', 'OCA\\Text\\Exception\\UploadException' => $baseDir . '/../lib/Exception/UploadException.php', 'OCA\\Text\\Exception\\VersionMismatchException' => $baseDir . '/../lib/Exception/VersionMismatchException.php', @@ -45,6 +46,7 @@ 'OCA\\Text\\Listeners\\LoadViewerListener' => $baseDir . '/../lib/Listeners/LoadViewerListener.php', 'OCA\\Text\\Listeners\\NodeCopiedListener' => $baseDir . '/../lib/Listeners/NodeCopiedListener.php', 'OCA\\Text\\Listeners\\RegisterDirectEditorEventListener' => $baseDir . '/../lib/Listeners/RegisterDirectEditorEventListener.php', + 'OCA\\Text\\Middleware\\Attribute\\RequireDocumentBaseVersionEtag' => $baseDir . '/../lib/Middleware/Attribute/RequireDocumentBaseVersionEtag.php', 'OCA\\Text\\Middleware\\Attribute\\RequireDocumentSession' => $baseDir . '/../lib/Middleware/Attribute/RequireDocumentSession.php', 'OCA\\Text\\Middleware\\Attribute\\RequireDocumentSessionOrUserOrShareToken' => $baseDir . '/../lib/Middleware/Attribute/RequireDocumentSessionOrUserOrShareToken.php', 'OCA\\Text\\Middleware\\SessionMiddleware' => $baseDir . '/../lib/Middleware/SessionMiddleware.php', diff --git a/composer/composer/autoload_static.php b/composer/composer/autoload_static.php index a172981b0ef..aac06275a14 100644 --- a/composer/composer/autoload_static.php +++ b/composer/composer/autoload_static.php @@ -46,6 +46,7 @@ class ComposerStaticInitText 'OCA\\Text\\Event\\LoadEditor' => __DIR__ . '/..' . '/../lib/Event/LoadEditor.php', 'OCA\\Text\\Exception\\DocumentHasUnsavedChangesException' => __DIR__ . '/..' . '/../lib/Exception/DocumentHasUnsavedChangesException.php', 'OCA\\Text\\Exception\\DocumentSaveConflictException' => __DIR__ . '/..' . '/../lib/Exception/DocumentSaveConflictException.php', + 'OCA\\Text\\Exception\\InvalidDocumentBaseVersionEtagException' => __DIR__ . '/..' . '/../lib/Exception/InvalidDocumentBaseVersionEtagException.php', 'OCA\\Text\\Exception\\InvalidSessionException' => __DIR__ . '/..' . '/../lib/Exception/InvalidSessionException.php', 'OCA\\Text\\Exception\\UploadException' => __DIR__ . '/..' . '/../lib/Exception/UploadException.php', 'OCA\\Text\\Exception\\VersionMismatchException' => __DIR__ . '/..' . '/../lib/Exception/VersionMismatchException.php', @@ -60,6 +61,7 @@ class ComposerStaticInitText 'OCA\\Text\\Listeners\\LoadViewerListener' => __DIR__ . '/..' . '/../lib/Listeners/LoadViewerListener.php', 'OCA\\Text\\Listeners\\NodeCopiedListener' => __DIR__ . '/..' . '/../lib/Listeners/NodeCopiedListener.php', 'OCA\\Text\\Listeners\\RegisterDirectEditorEventListener' => __DIR__ . '/..' . '/../lib/Listeners/RegisterDirectEditorEventListener.php', + 'OCA\\Text\\Middleware\\Attribute\\RequireDocumentBaseVersionEtag' => __DIR__ . '/..' . '/../lib/Middleware/Attribute/RequireDocumentBaseVersionEtag.php', 'OCA\\Text\\Middleware\\Attribute\\RequireDocumentSession' => __DIR__ . '/..' . '/../lib/Middleware/Attribute/RequireDocumentSession.php', 'OCA\\Text\\Middleware\\Attribute\\RequireDocumentSessionOrUserOrShareToken' => __DIR__ . '/..' . '/../lib/Middleware/Attribute/RequireDocumentSessionOrUserOrShareToken.php', 'OCA\\Text\\Middleware\\SessionMiddleware' => __DIR__ . '/..' . '/../lib/Middleware/SessionMiddleware.php', diff --git a/lib/Controller/PublicSessionController.php b/lib/Controller/PublicSessionController.php index 8d50478a414..8dd157dbd93 100644 --- a/lib/Controller/PublicSessionController.php +++ b/lib/Controller/PublicSessionController.php @@ -25,6 +25,7 @@ namespace OCA\Text\Controller; +use OCA\Text\Middleware\Attribute\RequireDocumentBaseVersionEtag; use OCA\Text\Middleware\Attribute\RequireDocumentSession; use OCA\Text\Service\ApiService; use OCP\AppFramework\Http\Attribute\NoAdminRequired; @@ -92,6 +93,7 @@ public function close(int $documentId, int $sessionId, string $sessionToken): Da #[NoAdminRequired] #[PublicPage] + #[RequireDocumentBaseVersionEtag] #[RequireDocumentSession] public function push(int $documentId, int $sessionId, string $sessionToken, int $version, array $steps, string $awareness, string $token): DataResponse { return $this->apiService->push($this->getSession(), $this->getDocument(), $version, $steps, $awareness, $token); @@ -99,6 +101,7 @@ public function push(int $documentId, int $sessionId, string $sessionToken, int #[NoAdminRequired] #[PublicPage] + #[RequireDocumentBaseVersionEtag] #[RequireDocumentSession] public function sync(string $token, int $version = 0): DataResponse { return $this->apiService->sync($this->getSession(), $this->getDocument(), $version, $token); @@ -106,6 +109,7 @@ public function sync(string $token, int $version = 0): DataResponse { #[NoAdminRequired] #[PublicPage] + #[RequireDocumentBaseVersionEtag] #[RequireDocumentSession] public function save(string $token, int $version = 0, string $autosaveContent = null, string $documentState = null, bool $force = false, bool $manualSave = false): DataResponse { return $this->apiService->save($this->getSession(), $this->getDocument(), $version, $autosaveContent, $documentState, $force, $manualSave, $token); diff --git a/lib/Controller/SessionController.php b/lib/Controller/SessionController.php index b5de48b19b5..71452a79bd7 100644 --- a/lib/Controller/SessionController.php +++ b/lib/Controller/SessionController.php @@ -25,6 +25,7 @@ namespace OCA\Text\Controller; +use OCA\Text\Middleware\Attribute\RequireDocumentBaseVersionEtag; use OCA\Text\Middleware\Attribute\RequireDocumentSession; use OCA\Text\Service\ApiService; use OCA\Text\Service\NotificationService; @@ -69,6 +70,7 @@ public function close(int $documentId, int $sessionId, string $sessionToken): Da #[NoAdminRequired] #[PublicPage] + #[RequireDocumentBaseVersionEtag] #[RequireDocumentSession] public function push(int $version, array $steps, string $awareness): DataResponse { try { @@ -81,6 +83,7 @@ public function push(int $version, array $steps, string $awareness): DataRespons #[NoAdminRequired] #[PublicPage] + #[RequireDocumentBaseVersionEtag] #[RequireDocumentSession] public function sync(int $version = 0): DataResponse { try { @@ -93,6 +96,7 @@ public function sync(int $version = 0): DataResponse { #[NoAdminRequired] #[PublicPage] + #[RequireDocumentBaseVersionEtag] #[RequireDocumentSession] public function save(int $version = 0, string $autosaveContent = null, string $documentState = null, bool $force = false, bool $manualSave = false): DataResponse { try { diff --git a/lib/Exception/InvalidDocumentBaseVersionEtagException.php b/lib/Exception/InvalidDocumentBaseVersionEtagException.php new file mode 100644 index 00000000000..1bb13ca65a2 --- /dev/null +++ b/lib/Exception/InvalidDocumentBaseVersionEtagException.php @@ -0,0 +1,9 @@ +getAttributes(RequireDocumentBaseVersionEtag::class))) { + $this->assertDocumentBaseVersionEtag(); + } + if (!empty($reflectionMethod->getAttributes(RequireDocumentSession::class))) { $this->assertDocumentSession($controller); } } + /** + * @throws InvalidDocumentBaseVersionEtagException + */ + private function assertDocumentBaseVersionEtag(): void { + $documentId = (int)$this->request->getParam('documentId'); + $baseVersionEtag = $this->request->getParam('baseVersionEtag'); + + $document = $this->documentService->getDocument($documentId); + if ($baseVersionEtag && $document?->getBaseVersionEtag() !== $baseVersionEtag) { + throw new InvalidDocumentBaseVersionEtagException(); + } + } + /** * @throws InvalidSessionException */ @@ -118,9 +141,13 @@ private function assertUserOrShareToken(ISessionAwareController $controller): vo $controller->setDocument($document); } - public function afterException($controller, $methodName, \Exception $exception): DataResponse|Response { + public function afterException($controller, $methodName, \Exception $exception): JSONResponse|Response { + if ($exception instanceof InvalidDocumentBaseVersionEtagException) { + return new JSONResponse($this->l10n->t('Editing session has expired. Please reload the page.'), Http::STATUS_PRECONDITION_FAILED); + } + if ($exception instanceof InvalidSessionException) { - return new DataResponse([], 403); + return new JSONResponse([], 403); } return parent::afterException($controller, $methodName, $exception); diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php index 95dc3310215..1be3bf51d97 100644 --- a/lib/Service/ApiService.php +++ b/lib/Service/ApiService.php @@ -116,7 +116,7 @@ public function create(?int $fileId = null, ?string $filePath = null, ?string $b $document = $this->documentService->getDocument($file->getId()); $freshSession = $document === null; if ($baseVersionEtag && $baseVersionEtag !== $document?->getBaseVersionEtag()) { - return new DataResponse($this->l10n->t('Editing session has expired. Please reload the page.'), Http::STATUS_CONFLICT); + return new DataResponse($this->l10n->t('Editing session has expired. Please reload the page.'), Http::STATUS_PRECONDITION_FAILED); } if ($freshSession) { @@ -193,7 +193,7 @@ public function push(Session $session, Document $document, int $version, array $ $session = $this->sessionService->updateSessionAwareness($session, $awareness); } catch (DoesNotExistException $e) { // Session was removed in the meantime. #3875 - return new DataResponse([], 403); + return new DataResponse($this->l10n->t('Editing session has expired. Please reload the page.'), Http::STATUS_PRECONDITION_FAILED); } if (empty($steps)) { return new DataResponse([]); @@ -204,7 +204,7 @@ public function push(Session $session, Document $document, int $version, array $ return new DataResponse($e->getMessage(), 422); } catch (DoesNotExistException|NotPermittedException) { // Either no write access or session was removed in the meantime (#3875). - return new DataResponse([], 403); + return new DataResponse($this->l10n->t('Editing session has expired. Please reload the page.'), Http::STATUS_PRECONDITION_FAILED); } return new DataResponse($result); } diff --git a/src/services/PollingBackend.js b/src/services/PollingBackend.js index 39595db05cd..3cbb3537f5e 100644 --- a/src/services/PollingBackend.js +++ b/src/services/PollingBackend.js @@ -170,6 +170,9 @@ class PollingBackend { outsideChange: e.response.data.outsideChange, }, }) + } else if (e.response.status === 412) { + this.#syncService.emit('error', { type: ERROR_TYPE.LOAD_ERROR, data: e.response }) + this.disconnect() } else if (e.response.status === 403) { this.#syncService.emit('error', { type: ERROR_TYPE.SOURCE_NOT_FOUND, data: {} }) this.disconnect() diff --git a/src/services/SessionApi.js b/src/services/SessionApi.js index 96bf299ad0a..c8f49e0a77b 100644 --- a/src/services/SessionApi.js +++ b/src/services/SessionApi.js @@ -114,6 +114,7 @@ export class Connection { return this.#post(this.#url(`session/${this.#document.id}/sync`), { ...this.#defaultParams, filePath: this.#options.filePath, + baseVersionEtag: this.#document.baseVersionEtag, version, }) } @@ -122,6 +123,7 @@ export class Connection { return this.#post(this.#url(`session/${this.#document.id}/save`), { ...this.#defaultParams, filePath: this.#options.filePath, + baseVersionEtag: this.#document.baseVersionEtag, version, autosaveContent, documentState, @@ -134,6 +136,7 @@ export class Connection { return this.#post(this.#url(`session/${this.#document.id}/push`), { ...this.#defaultParams, filePath: this.#options.filePath, + baseVersionEtag: this.#document.baseVersionEtag, steps, version, awareness, diff --git a/src/services/SyncService.js b/src/services/SyncService.js index 427179a80f6..a68e55f90d2 100644 --- a/src/services/SyncService.js +++ b/src/services/SyncService.js @@ -187,7 +187,9 @@ class SyncService { if (!response || code === 'ECONNABORTED') { this.emit('error', { type: ERROR_TYPE.CONNECTION_FAILED, data: {} }) } - if (response?.status === 403) { + if (response?.status === 412) { + this.emit('error', { type: ERROR_TYPE.LOAD_ERROR, data: response }) + } else if (response?.status === 403) { if (!data.document) { // either the session is invalid or the document is read only. logger.error('failed to write to document - not allowed') From af37fe48341a67d4388ae72453aa2c9ef0192777 Mon Sep 17 00:00:00 2001 From: Jonas Date: Mon, 18 Mar 2024 19:58:16 +0100 Subject: [PATCH 09/18] fix(DocumentStatus): Refactor and migrate to `NcNoteCard` Fixes: #4905 Signed-off-by: Jonas --- cypress/e2e/conflict.spec.js | 3 +- cypress/e2e/share.spec.js | 2 +- cypress/e2e/sync.spec.js | 8 +-- src/components/Editor/DocumentStatus.vue | 85 +++++++++++------------- 4 files changed, 47 insertions(+), 51 deletions(-) diff --git a/cypress/e2e/conflict.spec.js b/cypress/e2e/conflict.spec.js index 9e5021dc6c1..1396f6f9bef 100644 --- a/cypress/e2e/conflict.spec.js +++ b/cypress/e2e/conflict.spec.js @@ -54,7 +54,8 @@ variants.forEach(function({ fixture, mime }) { cy.get('#viewer .modal-header button.header-close').click() cy.get('#viewer').should('not.exist') cy.openFile(fileName) - cy.get('.text-editor .document-status .icon-error') + cy.get('.text-editor .document-status') + .should('contain', 'Document has been changed outside of the editor.') getWrapper() .find('#read-only-editor') .should('contain', 'Hello world') diff --git a/cypress/e2e/share.spec.js b/cypress/e2e/share.spec.js index 5eb124baf8c..e8282d5b775 100644 --- a/cypress/e2e/share.spec.js +++ b/cypress/e2e/share.spec.js @@ -151,7 +151,7 @@ describe('Open test.md in viewer', function() { cy.login(recipient) cy.visit('/apps/files') cy.openFile('test.md') - cy.getModal().find('.empty-content__name').should('contain', 'Failed to load file') + cy.getModal().find('.document-status').should('contain', 'This file cannot be displayed as download is disabled by the share') cy.getModal().getContent().should('not.exist') }) }) diff --git a/cypress/e2e/sync.spec.js b/cypress/e2e/sync.spec.js index 01a68ab7035..582157cff3a 100644 --- a/cypress/e2e/sync.spec.js +++ b/cypress/e2e/sync.spec.js @@ -74,7 +74,7 @@ describe('Sync', () => { }).as('sessionRequests') cy.wait('@dead', { timeout: 30000 }) cy.get('#editor-container .document-status', { timeout: 30000 }) - .should('contain', 'File could not be loaded') + .should('contain', 'Document could not be loaded.') .then(() => { reconnect = true }) @@ -83,7 +83,7 @@ describe('Sync', () => { .as('syncAfterRecovery') cy.wait('@syncAfterRecovery', { timeout: 30000 }) cy.get('#editor-container .document-status', { timeout: 30000 }) - .should('not.contain', 'File could not be loaded') + .should('not.contain', 'Document could not be loaded.') // FIXME: There seems to be a bug where typed words maybe lost if not waiting for the new session cy.wait('@syncAfterRecovery', { timeout: 10000 }) cy.getContent().type('* more content added after the lost connection{enter}') @@ -109,12 +109,12 @@ describe('Sync', () => { cy.wait('@sessionRequests', { timeout: 30000 }) cy.get('#editor-container .document-status', { timeout: 30000 }) - .should('contain', 'File could not be loaded') + .should('contain', 'Document could not be loaded.') cy.wait('@syncAfterRecovery', { timeout: 60000 }) cy.get('#editor-container .document-status', { timeout: 30000 }) - .should('not.contain', 'File could not be loaded') + .should('not.contain', 'Document could not be loaded.') // FIXME: There seems to be a bug where typed words maybe lost if not waiting for the new session cy.wait('@syncAfterRecovery', { timeout: 10000 }) cy.getContent().type('* more content added after the lost connection{enter}') diff --git a/src/components/Editor/DocumentStatus.vue b/src/components/Editor/DocumentStatus.vue index 3782b765197..014ac7a25ee 100644 --- a/src/components/Editor/DocumentStatus.vue +++ b/src/components/Editor/DocumentStatus.vue @@ -22,27 +22,34 @@