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.

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(sync): If baseVersionEtag changed, reset frontend #5523

Merged
merged 6 commits into from
Mar 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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',
Expand Down
2 changes: 2 additions & 0 deletions composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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',
Expand Down
22 changes: 22 additions & 0 deletions cypress/e2e/api/SessionApi.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,28 @@ describe('The session Api', function() {
.then(() => connection.close())
})

it('refuses create,push,sync,save with non-matching baseVersionEtag', function() {
cy.failToCreateTextSession(undefined, 'wrongBaseVersionEtag', { filePath: '', shareToken })
.its('status')
.should('eql', 412)

connection.setBaseVersionEtag('wrongBaseVersionEtag')

cy.failToPushSteps({ connection, steps: [messages.update], version })
.its('status')
.should('equal', 412)

cy.failToSyncSteps(connection, { version: 0 })
.its('status')
.should('equal', 412)

cy.failToSave(connection)
.its('status')
.should('equal', 412)

cy.then(() => connection.close())
})

it('recovers session even if last person leaves right after create', function() {
let joining
cy.log('Initial user pushes steps')
Expand Down
3 changes: 2 additions & 1 deletion cypress/e2e/conflict.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
2 changes: 1 addition & 1 deletion cypress/e2e/share.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
})
})
Expand Down
27 changes: 23 additions & 4 deletions cypress/e2e/sync.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
Expand All @@ -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}')
Expand All @@ -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}')
Expand All @@ -126,6 +126,25 @@ describe('Sync', () => {
.should('include', 'after the lost connection')
})

it('shows warning when document session got cleaned up', () => {
cy.get('.save-status button')
.click()
cy.wait('@save')
cy.uploadTestFile('test.md')

cy.get('#editor-container .document-status', { timeout: 30000 })
.should('contain', 'Editing session has expired.')

// Reload button works
cy.get('#editor-container .document-status a.button')
.contains('Reload')
.click()

cy.getContent()
cy.get('#editor-container .document-status .notecard')
.should('not.exist')
})

it('passes the doc content from one session to the next', () => {
cy.closeFile()
cy.intercept({ method: 'PUT', url: '**/apps/text/session/*/create' })
Expand Down
30 changes: 25 additions & 5 deletions cypress/support/sessions.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,30 +36,50 @@ Cypress.Commands.add('createTextSession', (fileId, options = {}) => {
return api.open({ fileId })
})

Cypress.Commands.add('failToCreateTextSession', (fileId) => {
const api = new SessionApi()
return api.open({ fileId })
Cypress.Commands.add('failToCreateTextSession', (fileId, baseVersionEtag = null, options = {}) => {
const api = new SessionApi(options)
return api.open({ fileId, baseVersionEtag })
.then((response) => {
throw new Error('Expected request to fail - but it succeeded!')
})
.catch((err) => err.response)
}, (err) => err.response)
})

Cypress.Commands.add('pushSteps', ({ connection, steps, version, awareness = '' }) => {
return connection.push({ steps, version, awareness })
.then(response => response.data)
})

Cypress.Commands.add('failToPushSteps', ({ connection, steps, version, awareness = '' }) => {
return connection.push({ steps, version, awareness })
.then((response) => {
throw new Error('Expected request to fail - but it succeeded!')
}, (err) => err.response)
})

Cypress.Commands.add('syncSteps', (connection, options = { version: 0 }) => {
return connection.sync(options)
.then(response => response.data)
})

Cypress.Commands.add('failToSyncSteps', (connection, options = { version: 0 }) => {
return connection.sync(options)
.then((response) => {
throw new Error('Expected request to fail - but it succeeded!')
}, (err) => err.response)
})

Cypress.Commands.add('save', (connection, options = { version: 0 }) => {
return connection.save(options)
.then(response => response.data)
})

Cypress.Commands.add('failToSave', (connection, options = { version: 0 }) => {
return connection.save(options)
.then((response) => {
throw new Error('Expected request to fail - but it succeeded!')
}, (err) => err.response)
})

// Used to test for race conditions between the last push and the close request
Cypress.Commands.add('pushAndClose', ({ connection, steps, version, awareness = '' }) => {
cy.log('Race between push and close')
Expand Down
8 changes: 6 additions & 2 deletions lib/Controller/PublicSessionController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -80,8 +81,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]
Expand All @@ -92,20 +93,23 @@ 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);
}

#[NoAdminRequired]
#[PublicPage]
#[RequireDocumentBaseVersionEtag]
#[RequireDocumentSession]
public function sync(string $token, int $version = 0): DataResponse {
return $this->apiService->sync($this->getSession(), $this->getDocument(), $version, $token);
}

#[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);
Expand Down
8 changes: 6 additions & 2 deletions lib/Controller/SessionController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -57,8 +58,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]
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down
9 changes: 9 additions & 0 deletions lib/Exception/InvalidDocumentBaseVersionEtagException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php

declare(strict_types=1);

namespace OCA\Text\Exception;

class InvalidDocumentBaseVersionEtagException extends \Exception {

}
9 changes: 9 additions & 0 deletions lib/Middleware/Attribute/RequireDocumentBaseVersionEtag.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php

namespace OCA\Text\Middleware\Attribute;

use Attribute;

#[Attribute(Attribute::TARGET_METHOD)]
class RequireDocumentBaseVersionEtag {
}
33 changes: 30 additions & 3 deletions lib/Middleware/SessionMiddleware.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,21 @@

use OC\User\NoUserException;
use OCA\Text\Controller\ISessionAwareController;
use OCA\Text\Exception\InvalidDocumentBaseVersionEtagException;
use OCA\Text\Exception\InvalidSessionException;
use OCA\Text\Middleware\Attribute\RequireDocumentBaseVersionEtag;
use OCA\Text\Middleware\Attribute\RequireDocumentSession;
use OCA\Text\Middleware\Attribute\RequireDocumentSessionOrUserOrShareToken;
use OCA\Text\Service\DocumentService;
use OCA\Text\Service\SessionService;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http\DataResponse;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\JSONResponse;
use OCP\AppFramework\Http\Response;
use OCP\AppFramework\Middleware;
use OCP\Files\IRootFolder;
use OCP\Files\NotPermittedException;
use OCP\IL10N;
use OCP\IRequest;
use OCP\IUserSession;
use OCP\Share\Exceptions\ShareNotFound;
Expand All @@ -30,11 +34,13 @@ public function __construct(
private IUserSession $userSession,
private IRootFolder $rootFolder,
private ShareManager $shareManager,
private IL10N $l10n,
) {
}

/**
* @throws ReflectionException
* @throws InvalidDocumentBaseVersionEtagException
* @throws InvalidSessionException
*/
public function beforeController(Controller $controller, string $methodName): void {
Expand All @@ -52,11 +58,28 @@ public function beforeController(Controller $controller, string $methodName): vo
}
}

if (!empty($reflectionMethod->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
*/
Expand Down Expand Up @@ -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(['error' => $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);
Expand Down
Loading
Loading