diff --git a/Makefile b/Makefile index 6f10869..73bf287 100644 --- a/Makefile +++ b/Makefile @@ -20,6 +20,14 @@ cert_dir=$(HOME)/.nextcloud/certificates # change. ds_repo=Euro-Office/DocumentServer ds_version=v9.3.4-hotfix.1 +# sha256 of the x86_64 rpm for ds_version, checked after download. This is +# ~600 MB of native code including the x2t binary the web server executes, so +# it is worth knowing that what arrived is what was reviewed - a pinned hash +# also catches the release being re-uploaded under the same tag, which the +# digest the API reports alongside the url cannot. Override it together with +# ds_version; set it empty to build an unverified tree. +# make ds_version=v9.3.5 ds_sha256=$(curl -sfL .../releases/tags/v9.3.5 | ...) +ds_sha256=f2b7748d6d45090b07428ca5c27cbb34f8bef89651fa4d568eee2d65dd02e0ad oo_dir=$(CURDIR)/3rdparty/onlyoffice/documentserver all: 3rdparty/onlyoffice/documentserver version @@ -54,14 +62,29 @@ appstore: # one fixed filename per release, while Euro-Office puts the version in the # filename and it does not always match the tag (the v9.3.2 release ships a # 9.3.1-dev.1 rpm). + # Matched against browser_download_url rather than anywhere in the json: + # the first https://…x86_64.rpm in the response is not necessarily an + # asset of it. url=$$(curl -sfL $${GITHUB_TOKEN:+-H "Authorization: Bearer $$GITHUB_TOKEN"} \ https://api.github.com/repos/$(ds_repo)/releases/tags/$(ds_version) \ - | grep -o 'https://[^"]*x86_64[.]rpm' | head -1); \ + | grep -o '"browser_download_url": *"[^"]*x86_64[.]rpm"' \ + | grep -o 'https://[^"]*' | head -1); \ if [ -z "$$url" ]; then echo "no x86_64 rpm asset for $(ds_repo) $(ds_version)" >&2; exit 1; fi; \ echo "fetching $$url"; \ - curl -sL -o documentserver.x86_64.rpm "$$url" + curl -fsSL -o documentserver.x86_64.rpm "$$url" + # Before anything reads it, and before it is unpacked: this is where a + # truncated download, a substituted asset or a re-uploaded release stops. + bash -c 'if [ -n "$(ds_sha256)" ]; then \ + echo "$(ds_sha256) documentserver.x86_64.rpm" | sha256sum -c - || \ + { echo "!! the downloaded document server does not match ds_sha256; refusing to build" >&2; \ + rm -f documentserver.x86_64.rpm; exit 1; }; \ + else echo "!! ds_sha256 is empty: building an unverified document server tree" >&2; fi' cd oo-extract && rpm2cpio ../documentserver.x86_64.rpm | cpio -idm - chmod -R 777 oo-extract/ + # Readable and traversable by the web server, writable only by the user + # doing the build. This used to be 777, which shipped a world-writable + # tree - so any local account could replace the x2t binary the web server + # runs. + chmod -R u+rwX,go+rX,go-w oo-extract/ # var/www//documentserver: "onlyoffice" upstream, "euro-office" # on the fork. src=$$(find oo-extract/var/www -mindepth 2 -maxdepth 2 -type d -name documentserver | head -1); \ diff --git a/appinfo/info.xml b/appinfo/info.xml index 1a94f81..4527d7f 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -16,7 +16,7 @@ The community document server will automatically be configured if no other docum Additionally, the community document server only supports running on x86-64 Linux servers.]]> - 0.3.2 + 0.3.4 agpl Robin Appelman DocumentServer @@ -43,6 +43,7 @@ Additionally, the community document server only supports running on x86-64 Linu OCA\DocumentServer\Migration\RebuildFonts + OCA\DocumentServer\Migration\RepairFormatSettings diff --git a/js/close-beacon.js b/js/close-beacon.js index 2ec6f7b..559aa8d 100644 --- a/js/close-beacon.js +++ b/js/close-beacon.js @@ -18,6 +18,7 @@ 'use strict'; var sessionId = null; + var goodbyeSent = null; function publish(id) { sessionId = id; @@ -62,9 +63,14 @@ // back, and saying goodbye for it would end a session still editing return; } - if (!sessionId || !navigator.sendBeacon) { + if (!sessionId || !navigator.sendBeacon || goodbyeSent === sessionId) { + // the id we have already said goodbye for, rather than a flag: + // pagehide and unload both fire for the same departure, and the + // server holds the second one for its grace period before finding + // there is nothing left to do return; } + goodbyeSent = sessionId; try { navigator.sendBeacon( window.__documentServerCloseUrl + '?sid=' + encodeURIComponent(sessionId) diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index f78c51c..e77bf94 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -28,6 +28,7 @@ use OCA\DocumentServer\IPC\RedisIPCFactory; use OCA\DocumentServer\JSSettingsHelper; use OCA\DocumentServer\OnlyOffice\AutoConfig; +use OCA\DocumentServer\OnlyOffice\BundledFormats; use OCA\DocumentServer\OnlyOffice\URLDecoder; use OCA\Onlyoffice\AppConfig; use OCA\Onlyoffice\Crypt; @@ -73,12 +74,21 @@ public function register(IRegistrationContext $context): void { ); }); + // Registered rather than left to autowiring: its one constructor + // argument is a string, and the container resolves those by parameter + // name before falling back to the default - so a service registered + // under the name "path" would be handed to it as a file path. + $context->registerService(BundledFormats::class, function () { + return new BundledFormats(); + }); + $context->registerService(AutoConfig::class, function (IAppContainer $container) { $server = $container->getServer(); $appConfig = $this->buildAppConfig(); return new AutoConfig( $server->get(IURLGenerator::class), - $appConfig + $appConfig, + $container->get(BundledFormats::class) ); }); } diff --git a/lib/BackgroundJob/Cleanup.php b/lib/BackgroundJob/Cleanup.php index 27a2f46..e61c519 100644 --- a/lib/BackgroundJob/Cleanup.php +++ b/lib/BackgroundJob/Cleanup.php @@ -75,7 +75,14 @@ protected function run($argument) { // server going away (#100). A document nobody has typed // into since the last write returns without running the // converter at all. - $this->saveHandler->saveSnapshot($documentId); + // + // Through the same interval the editing path uses, not past + // it: this is the periodic write arriving by another road, + // and an admin who set `autosave_interval 0` to stop the + // converter running under their editors did not mean + // "except from cron". It also stops a busy instance turning + // one cron pass into a converter run per open document. + $this->saveHandler->saveSnapshotIfDue($documentId); } else { $this->saveHandler->flushChanges($documentId); } diff --git a/lib/Channel/SessionCloser.php b/lib/Channel/SessionCloser.php index dab0289..78266c3 100644 --- a/lib/Channel/SessionCloser.php +++ b/lib/Channel/SessionCloser.php @@ -73,6 +73,19 @@ public function sessionStoppedCoAuthoring(Session $session): void { $this->announceParticipants($session); } + /** + * An editor that is gone for good: the page said so on its way out. + * + * The session is dropped rather than expired, and it has to be: the poll it + * left behind keeps running server-side and marking it as seen, so an + * expired row revives itself and the document is never seen to be empty - + * see SessionManager::removeSession(). What makes acting on an + * unauthenticated message safe is not that this is reversible, then, but + * that the only irreversible part of it is guarded: flushChanges() checks + * again, under the lock and after the converter has run, that nobody is in + * the document, and a session that appeared in the meantime keeps its + * changes and its baseline. + */ public function sessionLeft(Session $session): void { $documentId = $session->getDocumentId(); diff --git a/lib/Channel/SessionManager.php b/lib/Channel/SessionManager.php index ef2f444..d45f7c9 100644 --- a/lib/Channel/SessionManager.php +++ b/lib/Channel/SessionManager.php @@ -148,12 +148,20 @@ private function getExpiredSessions(): array { * open: it stops being a participant, but if it turns out to still be * polling, its next poll marks it as seen and it simply carries on. Nothing * is disposed of on the strength of a message a live page can send. + * + * Backdated past the timeout rather than set to zero. Zero happens to be + * far enough in the past against a real clock, but it says nothing about + * why, and it only works as long as the clock is large - which is a thing + * to know rather than a thing to rely on. One second past the cutoff is the + * same answer to both queries that ask, and it means it. */ public function expireSession(string $sessionId): void { $query = $this->connection->getQueryBuilder(); + $expiredAt = $this->timeFactory->getTime() - self::EXPIRED_SESSION_TIMEOUT - 1; + $query->update('documentserver_sess') - ->set('last_seen', $query->createNamedParameter(0, \PDO::PARAM_INT)) + ->set('last_seen', $query->createNamedParameter($expiredAt, \PDO::PARAM_INT)) ->where($query->expr()->eq('session_id', $query->createNamedParameter($sessionId))); QueryHelper::executeStatement($query); } @@ -161,6 +169,16 @@ public function expireSession(string $sessionId): void { /** * Drop a single session, for a client that said it was leaving rather than * one that stopped polling. + * + * Deleted rather than expired, and the abandoned poll is why. The request + * that session left behind keeps running server-side for up to + * Channel::TIMEOUT seconds and marks it as seen every + * Channel::SEEN_INTERVAL while it does - so an expired row revives itself, + * from nothing but its own orphaned poll, and the document is not disposed + * of when the last editor leaves. Deleting is what makes that marking a + * no-op. (A client that only stopped co-authoring is a different case: its + * page is still there and still polling, and expireSession() is right for + * it precisely because reviving is what should happen.) */ public function removeSession(string $sessionId): void { $this->ipcFactory->cleanupChannel($sessionId); @@ -191,15 +209,35 @@ public function isDocumentActive(int $documentId): bool { } /** + * Who is currently in a document. + * + * Expired rows are left out rather than waited for. They are only deleted + * by cleanSessions(), which runs from the background job, so between two + * job runs the table holds every session that ever stopped polling - a + * browser that was killed, a laptop that was closed. Counting those as + * participants meant a document was never seen to be empty: + * SessionCloser::sessionLeft() would not write the file when the last + * editor left, and the write fell back to whenever the job next ran, which + * is exactly the wait #100 exists to remove. It also made expireSession() + * do nothing observable. + * + * The cutoff is the one cleanSessions() deletes by, so this only stops + * counting a session that was already on its way out. A session that is + * genuinely polling says so every Channel::SEEN_INTERVAL seconds, six times + * over inside the timeout. + * * @param int $documentId * @return Session[] */ public function getSessionsForDocument(int $documentId): array { $query = $this->connection->getQueryBuilder(); + $cutoffTime = $this->timeFactory->getTime() - self::EXPIRED_SESSION_TIMEOUT; + $query->select('session_id', 'document_id', 'user', 'user_original', 'last_seen', 'readonly', 'user_index', 'username') ->from('documentserver_sess') - ->where($query->expr()->eq('document_id', $query->createNamedParameter($documentId, \PDO::PARAM_INT))); + ->where($query->expr()->eq('document_id', $query->createNamedParameter($documentId, \PDO::PARAM_INT))) + ->andWhere($query->expr()->gte('last_seen', $query->createNamedParameter($cutoffTime, \PDO::PARAM_INT))); return array_map(function (array $row) { return Session::fromRow($row); diff --git a/lib/Command/FlushChanges.php b/lib/Command/FlushChanges.php index a2c85a4..3518851 100644 --- a/lib/Command/FlushChanges.php +++ b/lib/Command/FlushChanges.php @@ -61,13 +61,49 @@ protected function configure() { null, InputOption::VALUE_NONE, 'Flush only inactive pages' + ) + ->addOption( + 'snapshot', + null, + InputOption::VALUE_NONE, + 'Write every open document out to its file without ending any editing session' ); parent::configure(); } protected function execute(InputInterface $input, OutputInterface $output) { $documents = $this->documentStore->getOpenDocuments(); + + // One document that will not assemble - a converter that chokes on it, + // a file the web server may not write - must not decide the fate of + // every document after it in the list. Returning at the first failure + // meant a single bad document left the rest unwritten, which is the + // opposite of what somebody running this before a backup is asking for. + // So each is attempted, and the exit code reports whether all of them + // worked. + $failed = 0; + foreach ($documents as $documentId) { + // A snapshot writes the file and leaves the editing session + // running, which is the only safe thing to do to a document + // somebody is still typing into. The background job does this on + // its own schedule; the option is here to drive it from cron at a + // chosen interval, or to get everything on disk on demand - before + // a backup, say. + if ($input->getOption('snapshot')) { + try { + $this->saveHandler->saveSnapshot($documentId); + } catch (\Exception $e) { + $failed++; + $this->logger->error( + 'Error while saving a snapshot of document ' . $documentId, + ['exception' => $e, 'app' => 'documentserver_community'] + ); + $output->writeln("could not save a snapshot of document $documentId: {$e->getMessage()}"); + } + continue; + } + if (!$input->getOption('inactive-pages') || !$this->sessionManager->isDocumentActive($documentId)) { try { @@ -76,16 +112,18 @@ protected function execute(InputInterface $input, OutputInterface $output) { // SaveHandler::flushChanges(). $this->saveHandler->flushChanges($documentId); } catch (\Exception $e) { + $failed++; $this->logger->error( 'Error while applying changes for document ' . $documentId, ['exception' => $e, 'app' => 'documentserver_community'] ); - // the exit codes were the wrong way round, which makes the - // command unusable from cron or a && chain - return 1; + $output->writeln("could not flush document $documentId: {$e->getMessage()}"); } } } - return 0; + + // the exit codes were the wrong way round, which makes the command + // unusable from cron or a && chain + return $failed === 0 ? 0 : 1; } } diff --git a/lib/Controller/DocumentController.php b/lib/Controller/DocumentController.php index 5805d69..3738c01 100644 --- a/lib/Controller/DocumentController.php +++ b/lib/Controller/DocumentController.php @@ -38,6 +38,7 @@ use OCA\DocumentServer\XHRCommand\CloseSession; use OCA\DocumentServer\XHRCommand\CommandDispatcher; use OCA\DocumentServer\XHRCommand\CursorCommand; +use OCA\DocumentServer\XHRCommand\ForceSave; use OCA\DocumentServer\XHRCommand\GetLock; use OCA\DocumentServer\XHRCommand\GetMessages; use OCA\DocumentServer\XHRCommand\IsSaveLock; @@ -48,6 +49,7 @@ use OCA\DocumentServer\XHRCommand\UnlockDocument; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\DataResponse; +use OCP\AppFramework\Http\Attribute\AnonRateLimit; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; use OCP\AppFramework\Http\Attribute\PublicPage; @@ -82,6 +84,7 @@ class DocumentController extends Controller { UnlockDocument::class, CursorCommand::class, OpenDocument::class, + ForceSave::class, ChatMessage::class, GetMessages::class, CloseSession::class, @@ -170,6 +173,11 @@ private function getInitialResponses(): array { #[NoAdminRequired] #[NoCSRFRequired] #[PublicPage] + // Nothing authenticates this but the session id, and it holds a worker for + // the grace period, so cap what one caller can spend. Generous next to what + // an editor does - a session says goodbye once - and low enough that + // guessing at session ids is not free. + #[AnonRateLimit(limit: 20, period: 60)] public function sessionClosed(string $sid): Response { $session = $this->sessionManager->getSession($sid); if (!$session) { diff --git a/lib/Controller/StaticController.php b/lib/Controller/StaticController.php index d15db4d..36d254f 100644 --- a/lib/Controller/StaticController.php +++ b/lib/Controller/StaticController.php @@ -131,10 +131,17 @@ public function webApps(string $path) { // the page that embeds the editor is the one that can still say // goodbye when the editor's own frame is being removed - return $this->createFileResponseWithContent( - $localPath, - file_get_contents($localPath) . "\n" . $this->closeBeacon('close-beacon-host.js') - ); + // + // Checked here as well as in createFileResponse(), because this + // branch reads the file itself: on a build where api.js was never + // rendered from its .tpl, file_get_contents() on a missing file + // turned a 404 into a warning and a 200 with nothing in it. + if (file_exists($localPath)) { + return $this->createFileResponseWithContent( + $localPath, + file_get_contents($localPath) . "\n" . $this->closeBeacon('close-beacon-host.js') + ); + } } return $this->createFileResponse($localPath); diff --git a/lib/Document/ChangeStore.php b/lib/Document/ChangeStore.php index c54722d..400ebde 100644 --- a/lib/Document/ChangeStore.php +++ b/lib/Document/ChangeStore.php @@ -66,16 +66,26 @@ public function __construct( * collides: by then the other writer has committed, so the fresh max is * past its changes. * + * The index the batch was actually stored at is returned rather than left to + * the caller to guess. A caller that reads the max itself before calling + * this is reading it outside the transaction, so a concurrent save - the + * very thing the retry exists for - leaves it describing changes by numbers + * they were not stored under. + * + * @return int the change index of the first change stored, or the index the + * batch would have started at when there were no changes to + * store * @throws DBException */ - public function addChangesForDocument(int $documentId, array $changes, string $user, string $userOriginal) { + public function addChangesForDocument(int $documentId, array $changes, string $user, string $userOriginal): int { $time = $this->timeFactory->getTime(); for ($attempt = 1;; $attempt++) { $this->connection->beginTransaction(); try { - $changeIndex = $this->getMaxChangeIndexForDocument($documentId) + 1; + $startIndex = $this->getMaxChangeIndexForDocument($documentId) + 1; + $changeIndex = $startIndex; foreach ($changes as $change) { $this->addChangeForDocument($documentId, $change, $user, $userOriginal, $time, $changeIndex); @@ -83,7 +93,7 @@ public function addChangesForDocument(int $documentId, array $changes, string $u } $this->connection->commit(); - return; + return $startIndex; } catch (\Throwable $e) { if ($this->connection->inTransaction()) { $this->connection->rollBack(); diff --git a/lib/Document/SaveHandler.php b/lib/Document/SaveHandler.php index 11b0ff6..1931b7f 100644 --- a/lib/Document/SaveHandler.php +++ b/lib/Document/SaveHandler.php @@ -94,8 +94,30 @@ public function saveSnapshotIfDue(int $documentId): bool { return false; } + return $this->saveSnapshotThrottled($documentId, $interval); + } + + /** + * Write the document out unless it was written less than $minInterval + * seconds ago. + * + * Separate from saveSnapshotIfDue() because the two answer to different + * things: that one is the periodic write and the admin can turn it off, + * this one is a floor under a write somebody asked for. A client command + * must not be able to turn a keystroke into a converter run, and it must + * not be silenced by `autosave_interval 0` either - the admin turning off + * the periodic write did not ask for the Save button to stop working. + * + * The last-write time is the one the snapshot state already keeps, and + * writeDocument() stamps it before running the converter as well as after, + * so a document that fails to assemble backs off rather than being retried + * on every request. + * + * @return bool whether the document was written + */ + public function saveSnapshotThrottled(int $documentId, int $minInterval): bool { $state = $this->documentStore->getSnapshotState($documentId); - if (($this->timeFactory->getTime() - $state['time']) < $interval) { + if (($this->timeFactory->getTime() - $state['time']) < $minInterval) { return false; } diff --git a/lib/IPC/MemcacheIPCBackend.php b/lib/IPC/MemcacheIPCBackend.php index f62f6e5..1342ce7 100644 --- a/lib/IPC/MemcacheIPCBackend.php +++ b/lib/IPC/MemcacheIPCBackend.php @@ -25,6 +25,7 @@ use OCA\DocumentServer\Channel\Channel; use OCP\IMemcache; +use OCP\IMemcacheTTL; /** * IPC Channels built on top of memcache concurrency primitives @@ -89,6 +90,7 @@ public function popMessage(string $channel, int $timeout): ?string { // can take is written off, so a writer that died mid-publish does // not stall the channel. $misses = $this->memcache->inc("$channel::miss_$readKey"); + $this->expire("$channel::miss_$readKey"); if (is_int($misses) && $misses < self::MAX_PUBLISH_MISSES) { return null; } @@ -104,4 +106,21 @@ public function popMessage(string $channel, int $timeout): ?string { return $message; } + + /** + * Put an expiry on a key that inc() created without one. + * + * `inc()` is the only atomic counter IMemcache offers and it takes no TTL, + * so a miss counter for a slot that is never resolved - the session ended + * while a push was in flight - would otherwise sit in the cache for the + * lifetime of the process. The messages themselves already expire, and this + * gives their bookkeeping the same lifetime - long enough to outlive any + * push it is counting, short enough that an abandoned one goes away. + * Backends with no TTL support keep the old behaviour. + */ + private function expire(string $key): void { + if ($this->memcache instanceof IMemcacheTTL) { + $this->memcache->setTTL($key, Channel::TIMEOUT * 4); + } + } } diff --git a/lib/Migration/RepairFormatSettings.php b/lib/Migration/RepairFormatSettings.php new file mode 100644 index 0000000..239a2ab --- /dev/null +++ b/lib/Migration/RepairFormatSettings.php @@ -0,0 +1,96 @@ + + * + * @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\DocumentServer\Migration; + +use OCA\DocumentServer\OnlyOffice\AutoConfig; +use OCP\App\IAppManager; +use OCP\Migration\IOutput; +use OCP\Migration\IRepairStep; +use Psr\Container\ContainerInterface; +use Psr\Log\LoggerInterface; + +/** + * Hand an existing install the format matrix a fresh one now gets. + * + * Seeding runs from AutoConfig::autoConfig(), which only runs while the + * connector has no document server url - so it reaches new installs and nothing + * else. An instance configured before the seed was read from the bundled + * package keeps what the old hardcoded list wrote: twelve editable formats, + * three of which (doc, ppt, xls) the bundled server cannot edit at all, and + * none of the twenty-nine it can. + * + * The decision of whether an install may be re-seeded is AutoConfig's, and it + * is a conservative one: exactly the old lists and nothing else, so an admin + * who has changed anything keeps their own settings. + */ +class RepairFormatSettings implements IRepairStep { + private IAppManager $appManager; + private ContainerInterface $container; + private LoggerInterface $logger; + + /** + * AutoConfig is resolved in run() rather than injected, because building it + * needs the connector's own AppConfig class - so an instance with the + * connector disabled, or not installed at all, cannot construct one. That is + * an ordinary state for this app, and an upgrade must not fall over in it; + * boot() takes the same care for the same reason. + */ + public function __construct(IAppManager $appManager, ContainerInterface $container, LoggerInterface $logger) { + $this->appManager = $appManager; + $this->container = $container; + $this->logger = $logger; + } + + public function getName(): string { + return 'Update the OnlyOffice format settings to what the bundled document server supports'; + } + + public function run(IOutput $output): void { + if (!$this->appManager->isEnabledForUser('onlyoffice')) { + // nothing to configure, and AutoConfig would not be constructible + // against a connector that is not there + return; + } + + try { + /** @var AutoConfig $autoConfig */ + $autoConfig = $this->container->get(AutoConfig::class); + $reseeded = $autoConfig->reseedFormatsIfUntouched(); + } catch (\Throwable $e) { + // An upgrade must not fail over a settings default. The old matrix + // keeps working - it is a subset of what the server can do, not a + // broken state - so this is worth a line in the log and nothing + // more. + $this->logger->warning('documentserver could not update the OnlyOffice format settings: {error}', [ + 'error' => $e->getMessage(), + 'exception' => $e, + ]); + return; + } + + if ($reseeded) { + $output->info('OnlyOffice format settings updated to what the bundled document server supports'); + } + } +} diff --git a/lib/Migration/Version001600Date20260909120000.php b/lib/Migration/Version001600Date20260909120000.php new file mode 100644 index 0000000..2c8c414 --- /dev/null +++ b/lib/Migration/Version001600Date20260909120000.php @@ -0,0 +1,51 @@ += ?` + * both have to fall back to an index that only narrows by document and then + * scan what it finds. + * + * That was affordable when the max was read once per save. It is read several + * times per save now, plus once per snapshot, so the same pair the other way + * round turns both into a lookup - and costs nothing, because it replaces an + * index rather than adding one. + */ +class Version001600Date20260909120000 extends SimpleMigrationStep { + private const TABLE = 'documentserver_changes'; + private const INDEX = 'documentserver_change_doc_id'; + private const COLUMNS = ['document_id', 'change_index']; + + public function changeSchema(IOutput $output, Closure $schemaClosure, array $options) { + /** @var Schema $schema */ + $schema = $schemaClosure(); + + if (!$schema->hasTable(self::TABLE)) { + return null; + } + + $table = $schema->getTable(self::TABLE); + if (!$table->hasIndex(self::INDEX) + || $table->getIndex(self::INDEX)->getColumns() === self::COLUMNS) { + return null; + } + + $table->dropIndex(self::INDEX); + $table->addUniqueIndex(self::COLUMNS, self::INDEX); + + return $schema; + } +} diff --git a/lib/OnlyOffice/AutoConfig.php b/lib/OnlyOffice/AutoConfig.php index 2758f64..ad4f52d 100644 --- a/lib/OnlyOffice/AutoConfig.php +++ b/lib/OnlyOffice/AutoConfig.php @@ -25,7 +25,14 @@ use OCP\IURLGenerator; class AutoConfig { - private const SUPPORTED_DEFAULT_FORMATS = [ + /** + * The formats OnlyOffice becomes the default opener for on a fresh install. + * + * A product choice, not a capability list: the bundled server can open far + * more than this, but taking .txt, .csv or .html away from Nextcloud's own + * handlers is not what installing a document server is asking for. + */ + private const DEFAULT_OPEN_FORMATS = [ 'doc', 'docx', 'odp', @@ -38,37 +45,89 @@ class AutoConfig { 'xlsx', ]; - private const SUPPORTED_EDIT_FORMATS = [ - 'csv', - 'doc', - 'docx', - 'odp', - 'ods', - 'odt', - 'ppt', - 'pptx', - 'rtf', - 'txt', - 'xls', - 'xlsx', - ]; - private $urlGenerator; private $appConfig; + private $bundledFormats; - public function __construct(IURLGenerator $urlGenerator, AppConfig $appConfig) { + public function __construct(IURLGenerator $urlGenerator, AppConfig $appConfig, BundledFormats $bundledFormats) { $this->urlGenerator = $urlGenerator; $this->appConfig = $appConfig; + $this->bundledFormats = $bundledFormats; } + /** + * What the format seed produced before it was read from the package: the + * hardcoded lists AutoConfig used to re-apply on every request. + * + * Kept so that an install carrying exactly this can be told apart from one + * an admin has since changed - see reseedFormatsIfUntouched(). Frozen + * history, not a list to maintain: nothing else may read these. + */ + private const LEGACY_SEED_DEFAULT_FORMATS = [ + 'doc', 'docx', 'odp', 'ods', 'odt', 'pdf', 'ppt', 'pptx', 'xls', 'xlsx', + ]; + private const LEGACY_SEED_EDIT_FORMATS = [ + 'csv', 'doc', 'docx', 'odp', 'ods', 'odt', 'ppt', 'pptx', 'rtf', 'txt', 'xls', 'xlsx', + ]; + public function autoConfigIfNeeded() { if ($this->shouldAutoConfig()) { $this->autoConfig(); - } elseif ($this->isCommunityDocumentServerConfigured()) { - $this->syncSupportedFormats(false); } } + /** + * Re-seed the format settings of an install that still carries what the old + * hardcoded seed wrote, and leave every other install alone. + * + * Existing installs are the ones the seed cannot reach: seeding runs from + * autoConfig(), which only runs while the connector has no document server + * url, so an instance configured before this change keeps the matrix the + * old code left behind - twelve editable formats, three of which (doc, ppt, + * xls) the bundled server cannot edit at all, and none of the twenty-nine + * it can. + * + * The old code force-wrote the matrix on every request, so an install that + * ran it holds exactly the hardcoded lists - except where the admin + * *disabled* something, which survived because the write was an AND against + * what was already there. That asymmetry is what makes this safe to decide: + * anything other than the two lists exactly is an admin's own choice, and + * is left as it is. + * + * @return bool whether the formats were re-seeded + */ + public function reseedFormatsIfUntouched(): bool { + if (!$this->isCommunityDocumentServerConfigured()) { + return false; + } + + $enabled = ['def' => [], 'edit' => []]; + foreach ($this->appConfig->FormatsSetting() as $format => $settings) { + foreach (['def', 'edit'] as $action) { + if ($settings[$action] ?? false) { + $enabled[$action][] = $format; + } + } + } + + sort($enabled['def']); + sort($enabled['edit']); + $legacyDefault = self::LEGACY_SEED_DEFAULT_FORMATS; + $legacyEdit = self::LEGACY_SEED_EDIT_FORMATS; + sort($legacyDefault); + sort($legacyEdit); + + if ($enabled['def'] !== $legacyDefault || $enabled['edit'] !== $legacyEdit) { + return false; + } + + return $this->seedSupportedFormats(); + } + + public function isCommunityDocumentServerConfigured(): bool { + return strpos((string)$this->appConfig->GetDocumentServerUrl(), 'apps/documentserver_community') !== false; + } + /** * Check if onlyoffice is not configured and we should fill our defaults * @@ -78,10 +137,6 @@ private function shouldAutoConfig(): bool { return !$this->appConfig->GetDocumentServerUrl(); } - private function isCommunityDocumentServerConfigured(): bool { - return strpos((string)$this->appConfig->GetDocumentServerUrl(), 'apps/documentserver_community') !== false; - } - /** * Fill the documentserver url and other defaults */ @@ -90,46 +145,58 @@ private function autoConfig() { ['path' => '_']), 0, -strlen('/web-apps/_')); $this->appConfig->SetDocumentServerUrl($url); - $this->syncSupportedFormats(true); + $this->seedSupportedFormats(); $this->appConfig->SetSameTab(true); } - private function syncSupportedFormats(bool $forceWrite): void { - $formatSettings = $this->appConfig->FormatsSetting(); - $defaultFormats = []; - $editFormats = []; - $hasUnsupportedFormats = false; - - foreach ($formatSettings as $format => $settings) { - if (!in_array($format, self::SUPPORTED_DEFAULT_FORMATS, true) && ($settings['def'] ?? false)) { - $hasUnsupportedFormats = true; - } - if (!in_array($format, self::SUPPORTED_EDIT_FORMATS, true) && ($settings['edit'] ?? false)) { - $hasUnsupportedFormats = true; - } - - $defaultFormats[$format] = in_array($format, self::SUPPORTED_DEFAULT_FORMATS, true) - && ($settings['def'] ?? false); - $editFormats[$format] = in_array($format, self::SUPPORTED_EDIT_FORMATS, true) - && ($settings['edit'] ?? false); + /** + * Write the format defaults, once, while the connector is still unconfigured. + * + * A seed and nothing more: from here on the admin owns these two settings. + * This used to run from boot() on every request against a hardcoded list, + * force-disabling anything outside it - so a format the admin enabled in + * the settings UI was switched back off by the next page load, PDF among + * them, which 9.x has a dedicated editor for. + * + * Seeding still earns its keep, because the connector leaves the + * lossy-editable formats (odt, ods, odp, csv, rtf, txt) off by default; + * that was the point of doing this at all. + * + * @return bool whether the settings were written + */ + private function seedSupportedFormats(): bool { + $bundled = $this->bundledFormats->actions(); + if (!$bundled) { + // no package to ask, so nothing to say about it: leave the + // connector's own defaults alone rather than writing every format + // off + return false; } - if (!$forceWrite && !$hasUnsupportedFormats) { - return; - } + $editable = array_fill_keys($this->bundledFormats->editable(), true); - // On initial config, enable all supported formats regardless of what FormatsSetting returns, - // so a fresh install does not end up with zero formats enabled. - if ($forceWrite) { - foreach (self::SUPPORTED_DEFAULT_FORMATS as $format) { - $defaultFormats[$format] = true; - } - foreach (self::SUPPORTED_EDIT_FORMATS as $format) { - $editFormats[$format] = true; - } + // Every format the connector has an opinion about, plus anything the + // bundled server knows that it does not: an explicit answer for each, + // so the seed does not half-depend on which formats the connector's own + // copy of the matrix happens to default on. + $formats = array_unique(array_merge( + array_keys($this->appConfig->FormatsSetting()), + array_keys($bundled) + )); + + $defaultFormats = []; + $editFormats = []; + foreach ($formats as $format) { + $known = isset($bundled[$format]); + $editFormats[$format] = $known && isset($editable[$format]); + // a default opener for something the bundled server cannot open + // would just be a file that fails to load + $defaultFormats[$format] = $known && in_array($format, self::DEFAULT_OPEN_FORMATS, true); } $this->appConfig->SetDefaultFormats($defaultFormats); $this->appConfig->SetEditableFormats($editFormats); + + return true; } } diff --git a/lib/OnlyOffice/BundledFormats.php b/lib/OnlyOffice/BundledFormats.php new file mode 100644 index 0000000..ec0ed06 --- /dev/null +++ b/lib/OnlyOffice/BundledFormats.php @@ -0,0 +1,108 @@ + + * + * @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\DocumentServer\OnlyOffice; + +/** + * The format matrix the bundled document server ships, as it declares it. + * + * The onlyoffice connector carries its own copy of the same file, but the two + * can be different versions, and it is this one that says what the converter + * we actually run can do. Read from the package rather than restated as a + * list, because a list goes stale silently: the one this replaced predated + * 9.x and named twelve editable formats where the bundled server declares + * twenty-nine. + */ +class BundledFormats { + public const FORMATS_FILE = __DIR__ . '/../../3rdparty/onlyoffice/documentserver/document-formats/onlyoffice-docs-formats.json'; + + /** Editing that keeps everything; "lossy-edit" is editing through a conversion. */ + private const EDIT_ACTIONS = ['edit', 'lossy-edit']; + + private string $path; + + /** @var array>|null extension => actions, read once */ + private ?array $actions = null; + + public function __construct(string $path = self::FORMATS_FILE) { + $this->path = $path; + } + + /** + * What the bundled server says it can do with each format, keyed by + * extension. Empty when the package is not there or does not parse - the + * callers of this treat that as "nothing known", never as "nothing + * supported". + * + * @return array> + */ + public function actions(): array { + if ($this->actions !== null) { + return $this->actions; + } + + $this->actions = []; + + // asked before reading, not left to the read failing: a missing package + // is the ordinary state of a checkout that has not run `make` yet, and + // a warning about it in the log would be noise + if (!is_readable($this->path)) { + return $this->actions; + } + + $json = @file_get_contents($this->path); + if ($json === false) { + return $this->actions; + } + + $formats = json_decode($json, true); + if (!is_array($formats)) { + return $this->actions; + } + + foreach ($formats as $format) { + if (!is_array($format) || !isset($format['name']) || !is_string($format['name'])) { + continue; + } + $actions = $format['actions'] ?? []; + $this->actions[$format['name']] = is_array($actions) ? array_values($actions) : []; + } + + return $this->actions; + } + + /** + * The formats the bundled server can open in an editor. + * + * @return list + */ + public function editable(): array { + $editable = []; + foreach ($this->actions() as $name => $actions) { + if (array_intersect(self::EDIT_ACTIONS, $actions)) { + $editable[] = $name; + } + } + return $editable; + } +} diff --git a/lib/XHRCommand/ChatMessage.php b/lib/XHRCommand/ChatMessage.php index 2e171c0..7fd86cc 100644 --- a/lib/XHRCommand/ChatMessage.php +++ b/lib/XHRCommand/ChatMessage.php @@ -46,6 +46,20 @@ class ChatMessage implements ICommandHandler { private const LOCK_ATTEMPTS = 5; private const LOCK_RETRY_US = 50 * 1000; + /** + * The longest message that will be stored and relayed, in characters. + * + * sdkjs c_oAscMaxCellOrCommentLength, which is what the chat panel puts in + * the textarea's maxlength - so this is the editor's own limit, enforced + * where it counts rather than only in the browser. Without it the length is + * whatever a client puts on the socket, and the history is capped at a + * hundred *messages* with nothing said about their size, so a scripted + * client could park as much in a document's appdata as the request limit + * allows. Truncated rather than dropped: a message that is too long is a + * client that is not the editor, and there is nothing to report to. + */ + private const MAX_MESSAGE_LENGTH = 32767; + private $documentStore; private $lockingProvider; private $logger; @@ -73,7 +87,7 @@ public function handle(array $command, Session $session, IIPCChannel $sessionCha 'user' => $session->getUserId(), 'useridoriginal' => $session->getUserOriginal(), 'username' => $session->getUserName(), - 'message' => $command['message'], + 'message' => mb_substr($command['message'], 0, self::MAX_MESSAGE_LENGTH), 'time' => time() * 1000, ]; diff --git a/lib/XHRCommand/ForceSave.php b/lib/XHRCommand/ForceSave.php new file mode 100644 index 0000000..7bb4638 --- /dev/null +++ b/lib/XHRCommand/ForceSave.php @@ -0,0 +1,156 @@ + + * + * @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\DocumentServer\XHRCommand; + +use OCA\DocumentServer\Channel\Session; +use OCA\DocumentServer\Document\SaveHandler; +use OCA\DocumentServer\IPC\IIPCChannel; +use OCP\AppFramework\Utility\ITimeFactory; +use Psr\Log\LoggerInterface; + +/** + * Write the document out now, because the editor asked. + * + * This is what the connector's "Keep intermediate versions when editing" + * setting turns into: it sets editorConfig.customization.forcesave, which + * gives the editor a Save button, and pressing it sends forceSaveStart. + * Without a handler the command was dropped and the button did nothing, so the + * setting looked broken from the settings UI. + * + * A snapshot, not a flush: people are still typing, and consuming the change + * list here would leave the next session to open the document looking at the + * version from before the save - see SaveHandler::saveSnapshot(). + */ +class ForceSave implements ICommandHandler { + /** sdkjs c_oAscForceSaveTypes.Button */ + private const TYPE_BUTTON = 1; + /** sdkjs c_oAscServerCommandErrors */ + private const ERROR_NONE = 0; + private const ERROR_UNKNOWN = 3; + private const ERROR_NOT_MODIFIED = 4; + + /** + * The shortest gap between two writes this command will ask for, in + * seconds. + * + * Assembling a document is a converter run, and this is the one place a + * client can ask for one directly: the periodic write is floored at + * `autosave_interval`, but a command is whatever arrives on the socket. + * Without a floor a client that sends forceSaveStart after every change - + * scripted, or looping on an error - turns each keystroke into an x2t run + * for as long as it keeps typing, because a stored change is exactly what + * stops the write being skipped as unmodified. + * + * Short enough that a person pressing the button does not notice it: it + * only refuses a second write within three seconds of the last one, and it + * refuses it the same way an unmodified document is refused, so the button + * ends its action rather than hanging. Nothing is lost by the refusal - the + * changes are in the change store either way, and the next write picks + * them up. + */ + private const MIN_INTERVAL = 3; + + private $saveHandler; + private $timeFactory; + private $logger; + + public function __construct(SaveHandler $saveHandler, ITimeFactory $timeFactory, LoggerInterface $logger) { + $this->saveHandler = $saveHandler; + $this->timeFactory = $timeFactory; + $this->logger = $logger; + } + + public function getType(): string { + return 'forceSaveStart'; + } + + public function handle(array $command, Session $session, IIPCChannel $sessionChannel, IIPCChannel $documentChannel, CommandDispatcher $commandDispatcher): void { + $documentId = $session->getDocumentId(); + // the same clock the save acks are stamped with (SaveChangesCommand's + // unSaveLock), which is what the editor compares this against before it + // will send another forceSaveStart + $time = $this->timeFactory->getTime() * 1000; + + // A session that cannot edit has nothing to save, and no Save button + // either - so this is a client that is not the editor's own, and the + // only thing it can achieve here is a converter run. Refused the way an + // unmodified document is: there is nothing for the editor to be told + // about, and no error to log on every poll. + if ($session->isReadOnly()) { + $sessionChannel->pushMessage(json_encode([ + 'type' => 'forceSaveStart', + 'messages' => [ + 'code' => self::ERROR_NOT_MODIFIED, + 'time' => $time, + ], + ])); + return; + } + + try { + $written = $this->saveHandler->saveSnapshotThrottled($documentId, self::MIN_INTERVAL); + $code = $written ? self::ERROR_NONE : self::ERROR_NOT_MODIFIED; + } catch (\Exception $e) { + $this->logger->warning('documentserver force save failed for document {doc}: {error}', [ + 'doc' => $documentId, + 'error' => $e->getMessage(), + 'exception' => $e, + ]); + $code = self::ERROR_UNKNOWN; + } + + // Reported after the fact rather than before: the save is synchronous + // here, so announcing a save that started and then failing it a moment + // later would only leave the button in the saving state for as long as + // it takes to say so. NotModified is a state of its own to the editor, + // and the right answer when the snapshot wrote nothing: nothing has been + // typed since the last write, another write of the same document is + // already running, or one finished less than MIN_INTERVAL ago. The file + // was written moments ago in all three, and this ends the button's + // action without claiming a save that did not happen. + $sessionChannel->pushMessage(json_encode([ + 'type' => 'forceSaveStart', + 'messages' => [ + 'code' => $code, + 'time' => $time, + ], + ])); + + if ($code !== self::ERROR_NONE) { + return; + } + + // The second half of the reply, and it has to carry the same timestamp: + // the client stores the one from forceSaveStart and ignores a forceSave + // whose time does not match it. + $sessionChannel->pushMessage(json_encode([ + 'type' => 'forceSave', + 'messages' => [ + 'type' => self::TYPE_BUTTON, + 'time' => $time, + 'success' => true, + ], + ])); + } +} diff --git a/lib/XHRCommand/SaveChangesCommand.php b/lib/XHRCommand/SaveChangesCommand.php index cd92ee2..d7e630a 100644 --- a/lib/XHRCommand/SaveChangesCommand.php +++ b/lib/XHRCommand/SaveChangesCommand.php @@ -80,27 +80,54 @@ public function handle(array $command, Session $session, IIPCChannel $sessionCha // deleteIndex is repeated on every chunk of the same save, and it is // relative to the change store as it was before the save started, so // re-applying it once we have stored a chunk would delete that chunk. - if ($isFirstChunk && $command['deleteIndex']) { - $this->changeStore->deleteChangesByIndex($session->getDocumentId(), (int)$command['deleteIndex']); + // + // It is absent from most saves and -1 when the client explicitly has + // nothing to discard (sdkjs guards its own arithmetic with + // `-1 !== this.deleteIndex`). Both used to reach the delete: the key was + // read unguarded, and the test was truthiness, so -1 asked for + // `change_index >= -1` - every change in the document. + $deleteIndex = $command['deleteIndex'] ?? null; + if ($isFirstChunk && is_numeric($deleteIndex) && (int)$deleteIndex >= 0) { + $this->changeStore->deleteChangesByIndex($session->getDocumentId(), (int)$deleteIndex); } + // Relayed for completeness and nothing else: sdkjs never reads + // startIndex out of this message. Which is why it is left as the max + // read before the insert, stale though that is when a save races + // another one - there is nothing on the other end for a corrected value + // to be correct *for*, and changesIndex below is the field that is + // actually interpreted. $startIndex = $this->changeStore->getMaxChangeIndexForDocument($session->getDocumentId()); - $this->changeStore->addChangesForDocument($session->getDocumentId(), $changes, $session->getUserId(), $session->getUserOriginal()); + $firstIndex = $this->changeStore->addChangesForDocument( + $session->getDocumentId(), $changes, $session->getUserId(), $session->getUserOriginal()); - $changeIndex = $this->changeStore->getMaxChangeIndexForDocument($session->getDocumentId()); + // How many changes the document now holds, not the highest index in it. + // + // sdkjs stores this as its "synced" position and derives absolute + // positions from it by adding its own offsets - on the wire, + // `deleteIndex += this.changesIndex`, and locally the same shape, + // `Changes.length = SyncIndex + deleteIndex`, where a length is plainly + // a count. Sending the max made every index the client derived one too + // low. It is harmless while a save carries changes, because the result + // still lands past the end of the store and deletes nothing - but sdkjs + // also sends a saveChanges with an *empty* change array when all it has + // to say is a deleteIndex (`0 < aChanges.length || null !== deleteIndex`), + // and there the off-by-one is a request to delete one change that + // should have survived. + $changeCount = $this->changeStore->getMaxChangeIndexForDocument($session->getDocumentId()) + 1; $documentChannel->pushMessage(json_encode([ 'type' => 'saveChanges', 'docId' => $session->getDocumentId(), 'userId' => $session->getUserId(), - 'changes' => array_map(function (string $changeString, int $offset) use ($session, $startIndex) { - // the store numbers the changes it just stored from $startIndex + 1 up - $change = new Change($session->getDocumentId(), time(), $changeString, $session->getUserId(), $session->getUserOriginal(), $startIndex + 1 + $offset); + 'changes' => array_map(function (string $changeString, int $offset) use ($session, $firstIndex) { + // the store numbered the changes it just stored from $firstIndex up + $change = new Change($session->getDocumentId(), time(), $changeString, $session->getUserId(), $session->getUserOriginal(), $firstIndex + $offset); return $change->formatForClient(); }, $changes, array_keys($changes)), 'startIndex' => $startIndex, - 'changesIndex' => $changeIndex, + 'changesIndex' => $changeCount, 'locks' => [], // Relayed, not synthesised: for spreadsheets this carries the // recalcIndexRows/recalcIndexColumns the receiver needs to shift @@ -120,7 +147,7 @@ public function handle(array $command, Session $session, IIPCChannel $sessionCha if (!$isLastChunk) { // Mid-save: the client waits for savePartChanges before sending the // next chunk, and the locks stay held until the save completes. - $sessionChannel->pushMessage('{"type":"savePartChanges","changesIndex":' . $changeIndex . '}'); + $sessionChannel->pushMessage('{"type":"savePartChanges","changesIndex":' . $changeCount . '}'); return; } @@ -139,7 +166,7 @@ public function handle(array $command, Session $session, IIPCChannel $sessionCha } $now = time() * 1000; - $sessionChannel->pushMessage('{"type":"unSaveLock","index":' . $changeIndex . ',"time":' . $now . '}'); + $sessionChannel->pushMessage('{"type":"unSaveLock","index":' . $changeCount . ',"time":' . $now . '}'); // Write the document out if it has been long enough since the last // time. The editor's "all changes are saved" only ever meant that the diff --git a/tests/BackgroundJob/CleanupTest.php b/tests/BackgroundJob/CleanupTest.php new file mode 100644 index 0000000..24cbd83 --- /dev/null +++ b/tests/BackgroundJob/CleanupTest.php @@ -0,0 +1,126 @@ + + * + * @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\DocumentServer\Tests\BackgroundJob; + +use OCA\DocumentServer\BackgroundJob\Cleanup; +use OCA\DocumentServer\Channel\SessionManager; +use OCA\DocumentServer\Document\DocumentStore; +use OCA\DocumentServer\Document\LockStore; +use OCA\DocumentServer\Document\SaveHandler; +use OCA\DocumentServer\IPC\DatabaseIPCBackend; +use OCP\AppFramework\Utility\ITimeFactory; +use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; +use Test\TestCase; + +/** + * The cron job, and which of the two ways of writing a document it picks. + * + * A document nobody is in is flushed: written and disposed of, change list + * consumed. A document somebody is still typing into may only be snapshotted - + * consuming the change list under a live session strands the document, since + * Editor.bin stays at the version it was opened at. + */ +class CleanupTest extends TestCase { + /** @var SaveHandler|MockObject */ + private $saveHandler; + /** @var SessionManager|MockObject */ + private $sessionManager; + /** @var DocumentStore|MockObject */ + private $documentStore; + private Cleanup $job; + + protected function setUp(): void { + parent::setUp(); + + $this->saveHandler = $this->createMock(SaveHandler::class); + $this->sessionManager = $this->createMock(SessionManager::class); + $this->documentStore = $this->createMock(DocumentStore::class); + + $this->job = new Cleanup( + $this->createMock(ITimeFactory::class), + $this->sessionManager, + $this->documentStore, + $this->saveHandler, + $this->createMock(LockStore::class), + $this->createMock(DatabaseIPCBackend::class), + $this->createMock(LoggerInterface::class) + ); + } + + private function runJob(): void { + $run = new \ReflectionMethod(Cleanup::class, 'run'); + $run->setAccessible(true); + $run->invoke($this->job, null); + } + + /** + * Through the same interval the editing path uses, not past it. The job + * calling saveSnapshot() directly ignored `autosave_interval` - including + * the 0 that is documented as turning the periodic write off - and turned + * one cron pass into a converter run for every open document. + */ + public function testALiveDocumentIsWrittenThroughTheAutosaveInterval() { + $this->documentStore->method('getOpenDocuments')->willReturn([1]); + $this->sessionManager->method('isDocumentActive')->willReturn(true); + + $this->saveHandler->expects($this->once())->method('saveSnapshotIfDue')->with(1); + $this->saveHandler->expects($this->never())->method('saveSnapshot'); + $this->saveHandler->expects($this->never())->method('flushChanges'); + + $this->runJob(); + } + + public function testADocumentNobodyIsInIsFlushed() { + $this->documentStore->method('getOpenDocuments')->willReturn([1]); + $this->sessionManager->method('isDocumentActive')->willReturn(false); + + $this->saveHandler->expects($this->once())->method('flushChanges')->with(1); + $this->saveHandler->expects($this->never())->method('saveSnapshotIfDue'); + + $this->runJob(); + } + + /** + * The job is the safety net for every open document, so one it cannot write + * must not stop it reaching the others. + */ + public function testOneUnwritableDocumentDoesNotStopTheSweep() { + $this->documentStore->method('getOpenDocuments')->willReturn([1, 2, 3]); + $this->sessionManager->method('isDocumentActive')->willReturn(false); + + $attempted = []; + $this->saveHandler->method('flushChanges') + ->willReturnCallback(function (int $documentId) use (&$attempted) { + $attempted[] = $documentId; + if ($documentId === 2) { + throw new \Exception('will not assemble'); + } + }); + + $this->runJob(); + + $this->assertSame([1, 2, 3], $attempted); + } +} diff --git a/tests/Channel/SessionCloserTest.php b/tests/Channel/SessionCloserTest.php new file mode 100644 index 0000000..37f08d3 --- /dev/null +++ b/tests/Channel/SessionCloserTest.php @@ -0,0 +1,151 @@ + + * + * @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\DocumentServer\Tests\Channel; + +use OCA\DocumentServer\Channel\Session; +use OCA\DocumentServer\Channel\SessionCloser; +use OCA\DocumentServer\Channel\SessionManager; +use OCA\DocumentServer\Document\SaveHandler; +use OCA\DocumentServer\IPC\IIPCFactory; +use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; +use Test\TestCase; + +/** + * What happens to a document when an editor says it is leaving. + * + * The document must be written as soon as the last participant is gone, because + * otherwise it waits for a background job and a browser closed at the wrong + * moment loses the session's work (#100). The two ways a client can say it is + * going are not the same thing, though, and swapping one for the other breaks + * this in a way no unit test would notice on its own - which is why the first + * two tests here say which is which, and why. + */ +class SessionCloserTest extends TestCase { + private const DOCUMENT = 42; + + /** @var SessionManager|MockObject */ + private $sessionManager; + /** @var SaveHandler|MockObject */ + private $saveHandler; + private SessionCloser $closer; + + protected function setUp(): void { + parent::setUp(); + + $this->sessionManager = $this->createMock(SessionManager::class); + $this->saveHandler = $this->createMock(SaveHandler::class); + + $this->closer = new SessionCloser( + $this->sessionManager, + $this->saveHandler, + $this->createMock(IIPCFactory::class), + $this->createMock(LoggerInterface::class) + ); + } + + private function session(string $id = 'sid'): Session { + return new Session($id, self::DOCUMENT, 'user', 'user', 'user', 100, false, 1); + } + + /** + * Dropped, not expired - and this is not interchangeable. The poll the + * departing session left behind keeps running server-side for up to + * Channel::TIMEOUT seconds, marking it as seen every + * Channel::SEEN_INTERVAL, so an expired row revives itself out of its own + * orphaned request: the document is then never seen to be empty and is + * never disposed of. Deleting is what makes that marking a no-op. + */ + public function testALeavingSessionIsDroppedRatherThanExpired() { + $this->sessionManager->expects($this->once()) + ->method('removeSession') + ->with('sid'); + $this->sessionManager->expects($this->never())->method('expireSession'); + $this->sessionManager->method('getSessionsForDocument')->willReturn([]); + + $this->closer->sessionLeft($this->session()); + } + + /** + * The whole point of the beacon: the file is written there and then, rather + * than whenever cron next runs. + */ + public function testTheDocumentIsWrittenWhenTheLastParticipantLeaves() { + $this->sessionManager->method('getSessionsForDocument')->willReturn([]); + + $this->saveHandler->expects($this->once()) + ->method('flushChanges') + ->with(self::DOCUMENT); + + $this->closer->sessionLeft($this->session()); + } + + /** + * One of several leaving is not the document closing. Flushing consumes the + * change list, which is the only record of what the others have typed. + */ + public function testTheDocumentIsNotWrittenWhileSomebodyIsStillInIt() { + $this->sessionManager->method('getSessionsForDocument') + ->willReturn([$this->session('other')]); + + $this->saveHandler->expects($this->never())->method('flushChanges'); + + $this->closer->sessionLeft($this->session()); + } + + /** + * A failed save must not escape: on the beacon path there is nobody left to + * see it, and on the command path it would fail the command. The changes + * are still in the store, so the background job will try again. + */ + public function testAFailedSaveIsLoggedRatherThanThrown() { + $this->sessionManager->method('getSessionsForDocument')->willReturn([]); + $this->saveHandler->method('flushChanges') + ->willThrowException(new \Exception('x2t said no')); + + $this->closer->sessionLeft($this->session()); + + $this->addToAssertionCount(1); + } + + /** + * sdkjs sends `close` whenever it drops the editor to view mode - a licence + * verdict, rights taken away, a critical error - with the page still open. + * It is not a departure, so it must not write the document out and take its + * folder away. + */ + public function testStoppingCoAuthoringDoesNotDisposeOfTheDocument() { + // expired here, deliberately: the page is still open and still polling, + // and reviving on its next poll is exactly what should happen + $this->sessionManager->expects($this->once()) + ->method('expireSession') + ->with('sid'); + $this->sessionManager->expects($this->never())->method('removeSession'); + $this->sessionManager->method('getSessionsForDocument')->willReturn([]); + + $this->saveHandler->expects($this->never())->method('flushChanges'); + + $this->closer->sessionStoppedCoAuthoring($this->session()); + } +} diff --git a/tests/Channel/SessionManagerTest.php b/tests/Channel/SessionManagerTest.php index 3e24eb1..9e7dd29 100644 --- a/tests/Channel/SessionManagerTest.php +++ b/tests/Channel/SessionManagerTest.php @@ -153,6 +153,62 @@ public function testIsDocumentActive() { $this->assertFalse($this->manager->isDocumentActive(6)); } + /** + * A session that stopped polling stops counting as a participant straight + * away, rather than when the background job next deletes it. + * + * Rows only go away in cleanSessions(), which runs from cron - so between + * two runs the table holds every browser that was ever killed mid-edit. + * Counting those meant a document was never seen to be empty, and the write + * that happens when the last editor leaves never happened. + */ + public function testAnExpiredSessionIsNotAParticipant() { + $this->time = 10; + $this->manager->newSession('foo', 5); + $this->assertTrue($this->manager->isDocumentActive(5)); + + $this->time = 10 + SessionManager::EXPIRED_SESSION_TIMEOUT + 1; + + $this->assertFalse($this->manager->isDocumentActive(5), + 'a session nobody has polled for longer than the timeout still counts'); + $this->assertEquals([], $this->manager->getSessionsForDocument(5)); + // still there to be revived, just not a participant + $this->assertNotNull($this->manager->getSession('foo')); + } + + public function testPollingKeepsASessionAParticipant() { + $this->time = 10; + $this->manager->newSession('foo', 5); + + $this->time = 10 + SessionManager::EXPIRED_SESSION_TIMEOUT + 1; + $this->manager->markAsSeen('foo'); + + $this->assertTrue($this->manager->isDocumentActive(5)); + $this->assertCount(1, $this->manager->getSessionsForDocument(5)); + } + + /** + * expireSession() is how a client that said it is going away is dropped + * without deleting the row, so a page that turns out to still be there + * revives on its next poll. That only means anything if an expired session + * stops counting immediately. + */ + public function testExpiringASessionDropsItFromTheDocumentAtOnce() { + $this->time = 10; + $this->manager->newSession('foo', 5); + $this->manager->newSession('bar', 5); + + $this->manager->expireSession('foo'); + + $this->assertEquals(['bar'], array_map( + fn ($session) => $session->getSessionId(), + $this->manager->getSessionsForDocument(5))); + + // and it comes back if it was still polling after all + $this->manager->markAsSeen('foo'); + $this->assertCount(2, $this->manager->getSessionsForDocument(5)); + } + public function testGetSessionCount() { $this->time = 10; diff --git a/tests/Command/FlushChangesTest.php b/tests/Command/FlushChangesTest.php new file mode 100644 index 0000000..91b1c54 --- /dev/null +++ b/tests/Command/FlushChangesTest.php @@ -0,0 +1,128 @@ + + * + * @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\DocumentServer\Tests\Command; + +use OCA\DocumentServer\Channel\SessionManager; +use OCA\DocumentServer\Command\FlushChanges; +use OCA\DocumentServer\Document\DocumentStore; +use OCA\DocumentServer\Document\SaveHandler; +use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; +use Symfony\Component\Console\Tester\CommandTester; +use Test\TestCase; + +/** + * `occ documentserver:flush`, and mostly one thing about it: what a document it + * cannot write does to the documents after it in the list. + * + * The command sweeps every open document, and it is the thing an admin runs + * before a backup or from cron. Returning at the first failure meant one + * document the converter chokes on left every document after it unwritten - + * silently, since the exit code was all that said so. The sweep has to finish, + * and still report that it did not go cleanly. + */ +class FlushChangesTest extends TestCase { + /** @var SaveHandler|MockObject */ + private $saveHandler; + /** @var DocumentStore|MockObject */ + private $documentStore; + /** @var SessionManager|MockObject */ + private $sessionManager; + private CommandTester $command; + /** @var int[] the documents the sweep actually got to */ + private array $attempted = []; + + protected function setUp(): void { + parent::setUp(); + + $this->saveHandler = $this->createMock(SaveHandler::class); + $this->documentStore = $this->createMock(DocumentStore::class); + $this->sessionManager = $this->createMock(SessionManager::class); + + $this->command = new CommandTester(new FlushChanges( + $this->saveHandler, + $this->documentStore, + $this->sessionManager, + $this->createMock(LoggerInterface::class) + )); + } + + /** + * Every document is handed to $work, in order, recording which ones the + * sweep got to; the ones named in $failing throw when their turn comes. + */ + private function documents(array $ids, string $work, array $failing = []): void { + $this->documentStore->method('getOpenDocuments')->willReturn($ids); + + $this->saveHandler->method($work) + ->willReturnCallback(function (int $documentId) use ($failing) { + $this->attempted[] = $documentId; + if (in_array($documentId, $failing, true)) { + throw new \Exception("document $documentId will not assemble"); + } + return true; + }); + } + + public function testASnapshotSweepFinishesPastADocumentThatWillNotAssemble() { + $this->documents([1, 2, 3], 'saveSnapshot', [2]); + + $this->assertSame(1, $this->command->execute(['--snapshot' => true])); + $this->assertSame([1, 2, 3], $this->attempted, 'the sweep stopped at the bad document'); + $this->assertStringContainsString('2', $this->command->getDisplay()); + } + + public function testAFlushSweepFinishesPastADocumentThatWillNotAssemble() { + $this->documents([1, 2, 3], 'flushChanges', [2]); + $this->sessionManager->method('isDocumentActive')->willReturn(false); + + $this->assertSame(1, $this->command->execute([])); + $this->assertSame([1, 2, 3], $this->attempted, 'the sweep stopped at the bad document'); + } + + /** + * The exit codes used to be the wrong way round, which makes the command + * unusable from cron or a && chain. + */ + public function testASweepThatWroteEverythingSucceeds() { + $this->documents([1, 2, 3], 'saveSnapshot'); + + $this->assertSame(0, $this->command->execute(['--snapshot' => true])); + $this->assertSame([1, 2, 3], $this->attempted); + } + + /** + * A snapshot leaves the editing session running, which is the only safe + * thing to do to a document somebody is still typing into. Flushing + * consumes the change list, and that is the record of everything typed. + */ + public function testASnapshotSweepNeverFlushes() { + $this->documentStore->method('getOpenDocuments')->willReturn([1]); + + $this->saveHandler->expects($this->once())->method('saveSnapshot')->with(1); + $this->saveHandler->expects($this->never())->method('flushChanges'); + + $this->assertSame(0, $this->command->execute(['--snapshot' => true])); + } +} diff --git a/tests/Document/ChangeStoreTest.php b/tests/Document/ChangeStoreTest.php new file mode 100644 index 0000000..f81358d --- /dev/null +++ b/tests/Document/ChangeStoreTest.php @@ -0,0 +1,134 @@ + + * + * @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\DocumentServer\Tests\Document; + +use OCA\DocumentServer\Document\Change; +use OCA\DocumentServer\Document\ChangeStore; +use OCP\AppFramework\Utility\ITimeFactory; +use OCP\IDBConnection; +use PHPUnit\Framework\Attributes\Group; +use Test\TestCase; + +/** + * Where a batch of changes lands in the change log, and how the caller learns + * it. + * + * SaveChangesCommand broadcasts the indexes of the changes it just stored, and + * it used to work them out from a max it read *before* the insert. The store + * reads its own max inside the transaction and retries a batch that collides, + * so the two disagree exactly when it matters: when somebody else saved at the + * same moment. + */ +#[Group('DB')] +class ChangeStoreTest extends TestCase { + private const DOCUMENT = 987654; + + private ChangeStore $store; + private IDBConnection $connection; + + protected function setUp(): void { + parent::setUp(); + + $this->connection = \OCP\Server::get(IDBConnection::class); + + $timeFactory = $this->createMock(ITimeFactory::class); + $timeFactory->method('getTime')->willReturn(1700000000); + + $this->store = new ChangeStore($this->connection, $timeFactory); + + $this->clear(); + } + + protected function tearDown(): void { + $this->clear(); + parent::tearDown(); + } + + private function clear(): void { + $query = $this->connection->getQueryBuilder(); + $query->delete('documentserver_changes') + ->where($query->expr()->eq('document_id', $query->createNamedParameter(self::DOCUMENT, \PDO::PARAM_INT))); + $query->executeStatement(); + } + + /** @return int[] the change_index of every stored change, in order */ + private function storedIndexes(): array { + return array_map( + fn (Change $change) => $change->getChangeIndex(), + $this->store->getChangesForDocument(self::DOCUMENT) + ); + } + + private function add(array $changes): int { + return $this->store->addChangesForDocument(self::DOCUMENT, $changes, 'user', 'user'); + } + + public function testAnEmptyDocumentStartsAtZero() { + $this->assertSame(-1, $this->store->getMaxChangeIndexForDocument(self::DOCUMENT)); + + $this->assertSame(0, $this->add(['a', 'b'])); + $this->assertSame([0, 1], $this->storedIndexes()); + } + + /** + * The returned index is the one the batch was actually given, which is what + * lets the caller describe its own changes without reading the max itself. + */ + public function testTheReturnedIndexIsWhereTheBatchWasStored() { + $this->assertSame(0, $this->add(['a'])); + $this->assertSame(1, $this->add(['b', 'c'])); + $this->assertSame(3, $this->add(['d'])); + + $this->assertSame([0, 1, 2, 3], $this->storedIndexes()); + $this->assertSame(3, $this->store->getMaxChangeIndexForDocument(self::DOCUMENT)); + } + + /** + * A batch stored after somebody else's is numbered past it, and says so. + * This is the case a max read before the call gets wrong: it would still + * describe these changes as starting at 1. + */ + public function testABatchStoredAfterAnotherReportsItsRealPosition() { + $staleMax = $this->store->getMaxChangeIndexForDocument(self::DOCUMENT); + $this->assertSame(-1, $staleMax); + + // somebody else's save, between the caller reading the max and its own + // insert + $this->add(['theirs']); + + $firstIndex = $this->add(['ours']); + + $this->assertNotSame($staleMax + 1, $firstIndex, + 'the index a caller would have guessed from the stale max'); + $this->assertSame(1, $firstIndex); + $this->assertSame([0, 1], $this->storedIndexes()); + } + + public function testStoringNothingDoesNotMoveTheIndex() { + $this->add(['a']); + + $this->assertSame(1, $this->add([])); + $this->assertSame([0], $this->storedIndexes()); + } +} diff --git a/tests/Document/SaveHandlerTest.php b/tests/Document/SaveHandlerTest.php index 90be117..b248dbd 100644 --- a/tests/Document/SaveHandlerTest.php +++ b/tests/Document/SaveHandlerTest.php @@ -204,4 +204,64 @@ public function testAutosaveCanBeTurnedOff() { $this->assertFalse($saveHandler->saveSnapshotIfDue(self::DOCUMENT)); } + + /** + * The floor under a write somebody asked for, which is what stops a client + * command turning every keystroke into a converter run. Separate from the + * autosave interval on purpose: this one is not the admin's to switch off. + */ + public function testAThrottledWriteIsRefusedInsideItsInterval() { + $this->documentStore->method('getSnapshotState') + ->willReturn($this->snapshotState(1, $this->now - 2)); + + $this->documentStore->expects($this->never())->method('saveChanges'); + + $this->assertFalse($this->saveHandler->saveSnapshotThrottled(self::DOCUMENT, 3)); + } + + public function testAThrottledWriteHappensOnceItsIntervalHasPassed() { + $this->documentStore->method('getSnapshotState') + ->willReturn($this->snapshotState(1, $this->now - 4)); + $this->changeStore->method('getMaxChangeIndexForDocument')->willReturn(4); + $this->changeStore->method('getChangesForDocument')->willReturn([$this->change()]); + + $this->documentStore->expects($this->once())->method('saveChanges'); + + $this->assertTrue($this->saveHandler->saveSnapshotThrottled(self::DOCUMENT, 3)); + } + + /** + * `autosave_interval 0` turns off the periodic write. It must not also + * silence a write a client asked for: the admin said they did not want the + * document assembled on a timer, not that the Save button should stop + * working. + */ + public function testTurningAutosaveOffDoesNotDisableAThrottledWrite() { + $config = $this->createMock(IConfig::class); + $config->method('getAppValue')->willReturn('0'); + + $timeFactory = $this->createMock(ITimeFactory::class); + $timeFactory->method('getTime')->willReturnCallback(function () { + return $this->now; + }); + + $saveHandler = new SaveHandler( + $this->documentStore, + $this->changeStore, + $this->createMock(DocumentConverter::class), + $this->createMock(ILockingProvider::class), + $this->sessionManager, + $config, + $timeFactory + ); + + $this->documentStore->method('getSnapshotState') + ->willReturn($this->snapshotState(1, $this->now - 3600)); + $this->changeStore->method('getMaxChangeIndexForDocument')->willReturn(4); + $this->changeStore->method('getChangesForDocument')->willReturn([$this->change()]); + + $this->documentStore->expects($this->once())->method('saveChanges'); + + $this->assertTrue($saveHandler->saveSnapshotThrottled(self::DOCUMENT, 3)); + } } diff --git a/tests/OnlyOffice/BundledFormatsTest.php b/tests/OnlyOffice/BundledFormatsTest.php new file mode 100644 index 0000000..c83f774 --- /dev/null +++ b/tests/OnlyOffice/BundledFormatsTest.php @@ -0,0 +1,129 @@ + + * + * @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\DocumentServer\Tests\OnlyOffice; + +use OCA\DocumentServer\OnlyOffice\BundledFormats; +use Test\TestCase; + +/** + * Which formats the bundled document server says it can edit. + * + * This is what AutoConfig seeds the connector's format settings from, and it + * replaces a hardcoded list that had gone stale without anybody noticing - + * twelve formats where the bundled server declares twenty-nine, and three of + * the twelve (doc, ppt, xls) it cannot edit at all. So the point of these is + * that the answer keeps coming from the package rather than from a list. + */ +class BundledFormatsTest extends TestCase { + /** @var string[] */ + private array $fixtures = []; + + protected function tearDown(): void { + foreach ($this->fixtures as $path) { + @unlink($path); + } + $this->fixtures = []; + + parent::tearDown(); + } + + private function fixture(string $json): BundledFormats { + $path = tempnam(sys_get_temp_dir(), 'formats'); + file_put_contents($path, $json); + $this->fixtures[] = $path; + return new BundledFormats($path); + } + + public function testEditableCoversLossyEditing() { + // "lossy-edit" is editing through a conversion, which is exactly the + // set the connector leaves off by default - odt, ods, csv, txt - and + // therefore the set seeding exists to turn on. + $formats = $this->fixture(json_encode([ + ['name' => 'docx', 'actions' => ['view', 'edit', 'comment']], + ['name' => 'odt', 'actions' => ['view', 'lossy-edit']], + ['name' => 'doc', 'actions' => ['view', 'auto-convert']], + ['name' => 'vsdx', 'actions' => ['view']], + ])); + + $this->assertEquals(['docx', 'odt'], $formats->editable()); + } + + public function testAFormatWithNoActionsIsNotEditable() { + $formats = $this->fixture(json_encode([ + ['name' => 'docx', 'actions' => ['edit']], + ['name' => 'bmp'], + ['name' => 'gif', 'actions' => null], + ])); + + $this->assertEquals(['docx'], $formats->editable()); + $this->assertEquals(['docx' => ['edit'], 'bmp' => [], 'gif' => []], $formats->actions()); + } + + /** + * A missing or unreadable package must not read as "this server can edit + * nothing": AutoConfig would then seed every format off, which is worse + * than leaving the connector's own defaults alone. + */ + public function testAMissingPackageIsNotAnEmptyServer() { + $formats = new BundledFormats('/nonexistent/onlyoffice-docs-formats.json'); + + $this->assertSame([], $formats->actions()); + $this->assertSame([], $formats->editable()); + } + + public function testGarbageIsTreatedAsAMissingPackage() { + $this->assertSame([], $this->fixture('not json at all')->actions()); + $this->assertSame([], $this->fixture('"a string"')->actions()); + } + + /** + * The shape of the real file, when there is one - a build machine has it, + * the unit-test job does not build the document server. Catches an upstream + * release renaming the fields this reads, which would otherwise show up as + * a fresh install seeding nothing. + */ + public function testTheBundledPackageDeclaresTheEditorsItShips() { + if (!file_exists(BundledFormats::FORMATS_FILE)) { + $this->markTestSkipped('the document server is not built in this checkout'); + } + + $formats = new BundledFormats(); + $editable = $formats->editable(); + + // the four editors the bundled server ships, one format each + foreach (['docx', 'xlsx', 'pptx', 'pdf'] as $format) { + $this->assertContains($format, $editable, "$format should be editable"); + } + // lossy-edit, the reason seeding exists + foreach (['odt', 'ods', 'odp', 'csv', 'rtf', 'txt'] as $format) { + $this->assertContains($format, $editable, "$format should be editable"); + } + // known to the matrix, but view only: the old hardcoded list claimed + // doc, ppt and xls were editable + foreach (['doc', 'ppt', 'xls', 'vsdx'] as $format) { + $this->assertArrayHasKey($format, $formats->actions()); + $this->assertNotContains($format, $editable, "$format should not be editable"); + } + } +} diff --git a/tests/XHRCommand/ChatMessageTest.php b/tests/XHRCommand/ChatMessageTest.php new file mode 100644 index 0000000..c587b82 --- /dev/null +++ b/tests/XHRCommand/ChatMessageTest.php @@ -0,0 +1,132 @@ + + * + * @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\DocumentServer\Tests\XHRCommand; + +use OCA\DocumentServer\Channel\Session; +use OCA\DocumentServer\Document\DocumentStore; +use OCA\DocumentServer\IPC\IIPCChannel; +use OCA\DocumentServer\XHRCommand\ChatMessage; +use OCA\DocumentServer\XHRCommand\CommandDispatcher; +use OCP\Lock\ILockingProvider; +use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; +use Test\TestCase; + +/** + * A message from the editor's chat panel. + * + * The interesting part is not the happy path but the size of what gets stored: + * the history is kept in the document's appdata and capped at a hundred + * *messages*, with nothing said about how big one may be, and the editor's own + * limit is a maxlength on a textarea - which is to say, no limit at all for + * anything that is not the editor. + */ +class ChatMessageTest extends TestCase { + private const DOCUMENT = 42; + /** sdkjs c_oAscMaxCellOrCommentLength, the editor's own limit */ + private const MAX_LENGTH = 32767; + + /** @var DocumentStore|MockObject */ + private $documentStore; + private ChatMessage $handler; + + protected function setUp(): void { + parent::setUp(); + + $this->documentStore = $this->createMock(DocumentStore::class); + + $this->handler = new ChatMessage( + $this->documentStore, + $this->createMock(ILockingProvider::class), + $this->createMock(LoggerInterface::class) + ); + } + + /** + * @return array{stored: ?array, broadcast: array} what reached the history + * and what was relayed + */ + private function send(array $command): array { + $stored = null; + $this->documentStore->method('addChatMessage') + ->willReturnCallback(function (int $documentId, array $message) use (&$stored) { + $stored = $message; + }); + + $broadcast = []; + $channel = $this->createMock(IIPCChannel::class); + $channel->method('pushMessage')->willReturnCallback(function (string $m) use (&$broadcast) { + $broadcast[] = json_decode($m, true); + }); + + $this->handler->handle( + $command, + new Session('sid', self::DOCUMENT, 'user', 'user', 'user', 100, false, 1), + $channel, + $channel, + new CommandDispatcher() + ); + + return ['stored' => $stored, 'broadcast' => $broadcast]; + } + + public function testAnOrdinaryMessageIsStoredAndRelayedWhole() { + $result = $this->send(['type' => 'message', 'message' => 'hello everyone']); + + $this->assertSame('hello everyone', $result['stored']['message']); + $this->assertSame('hello everyone', $result['broadcast'][0]['messages'][0]['message']); + } + + /** + * The length has to be capped where it is stored, not only in the browser. + * Otherwise a client that is not the editor can park a hundred messages of + * whatever the request limit allows in one document's appdata. + */ + public function testAnOverlongMessageIsCutToTheEditorsOwnLimit() { + $result = $this->send(['type' => 'message', 'message' => str_repeat('a', self::MAX_LENGTH * 2)]); + + $this->assertSame(self::MAX_LENGTH, mb_strlen($result['stored']['message'])); + $this->assertSame(self::MAX_LENGTH, mb_strlen($result['broadcast'][0]['messages'][0]['message']), + 'the relayed copy is capped too, or every other participant gets the whole thing'); + } + + /** + * Cut by characters rather than bytes: mid-sequence truncation would put + * invalid UTF-8 into the history file, and json_encode() returns false on + * that - which would lose the whole backlog, not just one message. + */ + public function testCuttingAMessageLeavesValidUtf8() { + $result = $this->send(['type' => 'message', 'message' => str_repeat('é', self::MAX_LENGTH * 2)]); + + $this->assertSame(self::MAX_LENGTH, mb_strlen($result['stored']['message'])); + $this->assertNotFalse(json_encode($result['stored'])); + } + + public function testACommandWithNoMessageIsIgnored() { + $this->documentStore->expects($this->never())->method('addChatMessage'); + + $this->assertSame([], $this->send(['type' => 'message'])['broadcast']); + $this->assertSame([], $this->send(['type' => 'message', 'message' => ['not', 'a', 'string']])['broadcast']); + } +} diff --git a/tests/XHRCommand/ForceSaveTest.php b/tests/XHRCommand/ForceSaveTest.php new file mode 100644 index 0000000..17288fe --- /dev/null +++ b/tests/XHRCommand/ForceSaveTest.php @@ -0,0 +1,215 @@ + + * + * @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\DocumentServer\Tests\XHRCommand; + +use OCA\DocumentServer\Channel\Session; +use OCA\DocumentServer\Controller\DocumentController; +use OCA\DocumentServer\Document\SaveHandler; +use OCA\DocumentServer\IPC\IIPCChannel; +use OCA\DocumentServer\XHRCommand\CommandDispatcher; +use OCA\DocumentServer\XHRCommand\ForceSave; +use OCP\AppFramework\Utility\ITimeFactory; +use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; +use Test\TestCase; + +/** + * The Save button the connector's "Keep intermediate versions when editing" + * setting puts in the editor. + * + * Its command was dropped by the dispatcher, so the button did nothing and the + * setting looked broken. The reply is in two parts and the editor is fussy + * about both: it stores the timestamp from forceSaveStart and ignores a + * forceSave whose time does not match, and it will not send another + * forceSaveStart until a save has been acknowledged since the last one. + */ +class ForceSaveTest extends TestCase { + private const DOCUMENT = 42; + + /** @var SaveHandler|MockObject */ + private $saveHandler; + private $now = 1700000000; + /** @var ForceSave */ + private $handler; + + protected function setUp(): void { + parent::setUp(); + + $this->saveHandler = $this->createMock(SaveHandler::class); + + $timeFactory = $this->createMock(ITimeFactory::class); + $timeFactory->method('getTime')->willReturnCallback(function () { + return $this->now; + }); + + $this->handler = new ForceSave( + $this->saveHandler, + $timeFactory, + $this->createMock(LoggerInterface::class) + ); + } + + private function session(bool $readOnly = false): Session { + return new Session('sid', self::DOCUMENT, 'user', 'user', 'user', 100, $readOnly, 1); + } + + /** + * @return array the messages pushed to the session's own channel, decoded + */ + private function pressSave(?Session $session = null): array { + $pushed = []; + $sessionChannel = $this->createMock(IIPCChannel::class); + $sessionChannel->method('pushMessage')->willReturnCallback(function (string $message) use (&$pushed) { + $pushed[] = json_decode($message, true); + }); + + $this->handler->handle( + ['type' => 'forceSaveStart'], + $session ?? $this->session(), + $sessionChannel, + $this->createMock(IIPCChannel::class), + new CommandDispatcher() + ); + + return $pushed; + } + + /** + * Without this registration the command is dropped, which is the whole bug: + * everything below describes a handler nothing would ever call. + */ + public function testTheDispatcherKnowsTheCommand() { + $this->assertContains(ForceSave::class, DocumentController::COMMAND_HANDLERS); + $this->assertSame('forceSaveStart', $this->handler->getType()); + } + + public function testPressingSaveWritesTheDocumentAndReportsItSaved() { + $this->saveHandler->expects($this->once()) + ->method('saveSnapshotThrottled') + // the floor is the handler's own; what matters here is that it + // asks for a throttled write rather than an unconditional one + ->with(self::DOCUMENT, $this->greaterThan(0)) + ->willReturn(true); + + $pushed = $this->pressSave(); + + $this->assertCount(2, $pushed); + $this->assertSame('forceSaveStart', $pushed[0]['type']); + $this->assertSame(0, $pushed[0]['messages']['code'], 'c_oAscServerCommandErrors.NoError'); + $this->assertSame('forceSave', $pushed[1]['type']); + $this->assertSame(1, $pushed[1]['messages']['type'], 'c_oAscForceSaveTypes.Button'); + $this->assertTrue($pushed[1]['messages']['success']); + + // the editor stores the first time and ignores a forceSave that does + // not carry the same one, so the button would hang on a mismatch + $this->assertSame($this->now * 1000, $pushed[0]['messages']['time']); + $this->assertSame($pushed[0]['messages']['time'], $pushed[1]['messages']['time']); + } + + /** + * A snapshot writes the file and leaves the change list alone. Flushing + * here would consume it, and since Editor.bin stays at the version the + * document was opened at, the next session to open it would show the + * document from before the save. + */ + public function testPressingSaveDoesNotEndTheEditingSession() { + $this->saveHandler->expects($this->never())->method('flushChanges'); + $this->saveHandler->method('saveSnapshotThrottled')->willReturn(true); + + $this->pressSave(); + } + + /** + * Nothing typed since the last write: the file already holds what the + * button was pressed for. NotModified is the editor's own state for that + * and it ends the button's action without claiming a save. + */ + public function testNothingToSaveIsReportedAsNotModified() { + $this->saveHandler->method('saveSnapshotThrottled')->willReturn(false); + + $pushed = $this->pressSave(); + + $this->assertCount(1, $pushed); + $this->assertSame('forceSaveStart', $pushed[0]['type']); + $this->assertSame(4, $pushed[0]['messages']['code'], 'c_oAscServerCommandErrors.NotModified'); + } + + /** + * A failure has to be answered too. An unanswered forceSaveStart leaves the + * button spinning until the editor's conversion timeout, and then reports a + * save that started and never landed. + */ + public function testAFailedSaveIsReportedRatherThanThrown() { + $this->saveHandler->method('saveSnapshotThrottled') + ->willThrowException(new \Exception('x2t said no')); + + $pushed = $this->pressSave(); + + $this->assertCount(1, $pushed); + $this->assertSame('forceSaveStart', $pushed[0]['type']); + $this->assertSame(3, $pushed[0]['messages']['code'], 'c_oAscServerCommandErrors.UnknownError'); + } + + /** + * A session that cannot edit has no Save button, so a forceSaveStart from + * one is not the editor asking: it is a client that has nothing to save and + * can only make the server run the converter. Answered, so nothing hangs, + * but not acted on. + */ + public function testAReadOnlySessionCannotAskForAWrite() { + $this->saveHandler->expects($this->never())->method('saveSnapshotThrottled'); + $this->saveHandler->expects($this->never())->method('saveSnapshot'); + + $pushed = $this->pressSave($this->session(true)); + + $this->assertCount(1, $pushed); + $this->assertSame('forceSaveStart', $pushed[0]['type']); + $this->assertSame(4, $pushed[0]['messages']['code'], 'c_oAscServerCommandErrors.NotModified'); + } + + /** + * The write has to be floored, because this is the one place a client asks + * for a converter run directly - the periodic write is floored by + * `autosave_interval`, a command is whatever arrives on the socket. A + * client that sends forceSaveStart after every change would otherwise turn + * each keystroke into an x2t run, since a stored change is exactly what + * stops a write being skipped as unmodified. + * + * Asserted as "does not call saveSnapshot()", because that is the shape the + * bug had: the unthrottled call is the one that has to stay gone. + */ + public function testTheWriteIsFlooredRatherThanRunOnDemand() { + $this->saveHandler->expects($this->never())->method('saveSnapshot'); + $this->saveHandler->expects($this->exactly(3)) + ->method('saveSnapshotThrottled') + ->with(self::DOCUMENT, $this->greaterThan(0)) + ->willReturn(false); + + // three presses in the same second, as a script would send them + for ($i = 0; $i < 3; $i++) { + $pushed = $this->pressSave(); + $this->assertSame(4, $pushed[0]['messages']['code'], 'c_oAscServerCommandErrors.NotModified'); + } + } +} diff --git a/tests/XHRCommand/SaveChangesCommandTest.php b/tests/XHRCommand/SaveChangesCommandTest.php new file mode 100644 index 0000000..e213367 --- /dev/null +++ b/tests/XHRCommand/SaveChangesCommandTest.php @@ -0,0 +1,185 @@ + + * + * @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\DocumentServer\Tests\XHRCommand; + +use OCA\DocumentServer\Channel\Session; +use OCA\DocumentServer\Document\ChangeStore; +use OCA\DocumentServer\Document\LockStore; +use OCA\DocumentServer\Document\SaveHandler; +use OCA\DocumentServer\IPC\IIPCChannel; +use OCA\DocumentServer\XHRCommand\CommandDispatcher; +use OCA\DocumentServer\XHRCommand\SaveChangesCommand; +use OCP\AppFramework\Utility\ITimeFactory; +use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; +use Test\TestCase; + +/** + * The two numbers in a save that sdkjs does arithmetic on. + * + * `changesIndex` is the client's "synced" position, and it derives absolute + * positions from it by adding its own offsets - on the wire + * `deleteIndex += this.changesIndex`, and locally `Changes.length = SyncIndex + + * deleteIndex`, where a length is a count. So it has to be a count of the + * changes stored, not the highest index among them. `deleteIndex` comes back + * the other way and asks for changes to be dropped, which makes believing it + * blindly expensive. + */ +class SaveChangesCommandTest extends TestCase { + private const DOCUMENT = 42; + + /** @var ChangeStore|MockObject */ + private $changeStore; + private SaveChangesCommand $handler; + /** @var array the messages broadcast to the document */ + private array $broadcast = []; + /** @var array the messages sent back to the saving session */ + private array $toSession = []; + + protected function setUp(): void { + parent::setUp(); + + $this->changeStore = $this->createMock(ChangeStore::class); + $this->changeStore->method('addChangesForDocument')->willReturn(0); + + $lockStore = $this->createMock(LockStore::class); + $lockStore->method('releaseLocks')->willReturn([]); + + $this->handler = new SaveChangesCommand( + $this->changeStore, + $lockStore, + $this->createMock(ITimeFactory::class), + $this->createMock(SaveHandler::class), + $this->createMock(LoggerInterface::class) + ); + } + + /** The document holds $count changes, numbered 0..$count-1. */ + private function storeHolds(int $count): void { + $this->changeStore->method('getMaxChangeIndexForDocument')->willReturn($count - 1); + } + + private function save(array $extra = []): void { + $this->broadcast = []; + $this->toSession = []; + + $documentChannel = $this->createMock(IIPCChannel::class); + $documentChannel->method('pushMessage')->willReturnCallback(function (string $m) { + $this->broadcast[] = json_decode($m, true); + }); + $sessionChannel = $this->createMock(IIPCChannel::class); + $sessionChannel->method('pushMessage')->willReturnCallback(function (string $m) { + $this->toSession[] = json_decode($m, true); + }); + + $this->handler->handle( + array_merge(['type' => 'saveChanges', 'changes' => '[]', 'releaseLocks' => false], $extra), + new Session('sid', self::DOCUMENT, 'user', 'user', 'user', 100, false, 1), + $sessionChannel, + $documentChannel, + new CommandDispatcher() + ); + } + + private function message(array $messages, string $type): ?array { + foreach ($messages as $message) { + if (($message['type'] ?? null) === $type) { + return $message; + } + } + return null; + } + + /** + * A count, not the highest index. Sending the max made every position the + * client derived one too low, and a save whose only content is a + * deleteIndex then asked for one change too many to be dropped. + */ + public function testTheClientIsToldHowManyChangesTheDocumentHolds() { + $this->storeHolds(267); + + $this->save(); + + $this->assertSame(267, $this->message($this->broadcast, 'saveChanges')['changesIndex']); + $this->assertSame(267, $this->message($this->toSession, 'unSaveLock')['index']); + } + + public function testAnEmptyDocumentReportsNoChanges() { + $this->storeHolds(0); + + $this->save(); + + $this->assertSame(0, $this->message($this->broadcast, 'saveChanges')['changesIndex']); + } + + public function testAMidSaveChunkReportsTheSameCount() { + $this->storeHolds(5); + + $this->save(['startSaveChanges' => true, 'endSaveChanges' => false]); + + $this->assertSame(5, $this->message($this->toSession, 'savePartChanges')['changesIndex']); + $this->assertNull($this->message($this->toSession, 'unSaveLock'), 'the save is not finished'); + } + + /** + * -1 is sdkjs saying it has nothing to discard - it guards its own + * arithmetic with `-1 !== this.deleteIndex`. Read as truthy it asked for + * `change_index >= -1`, which is every change in the document. + */ + public function testMinusOneDoesNotDeleteTheWholeDocument() { + $this->storeHolds(10); + $this->changeStore->expects($this->never())->method('deleteChangesByIndex'); + + $this->save(['deleteIndex' => -1]); + } + + /** Most saves carry no deleteIndex at all; reading the key unguarded warned. */ + public function testAnAbsentDeleteIndexDeletesNothing() { + $this->storeHolds(10); + $this->changeStore->expects($this->never())->method('deleteChangesByIndex'); + + $this->save(); + } + + public function testADeleteIndexIsHonouredWhenTheClientMeansIt() { + $this->storeHolds(10); + $this->changeStore->expects($this->once()) + ->method('deleteChangesByIndex') + ->with(self::DOCUMENT, 4); + + $this->save(['deleteIndex' => 4]); + } + + /** + * deleteIndex is repeated on every chunk of one save and is relative to the + * store as it was before the save started, so applying it again after a + * chunk has been stored would delete that chunk. + */ + public function testALaterChunkDoesNotReapplyTheDelete() { + $this->storeHolds(10); + $this->changeStore->expects($this->never())->method('deleteChangesByIndex'); + + $this->save(['deleteIndex' => 4, 'startSaveChanges' => false, 'endSaveChanges' => true]); + } +} diff --git a/tests/rig/README.md b/tests/rig/README.md index fdda1b6..682f677 100644 --- a/tests/rig/README.md +++ b/tests/rig/README.md @@ -38,12 +38,14 @@ nothing about a particular instance is hardcoded in a test. | test | covers | | --- | --- | +| `formats` | a fresh install is seeded with every format the bundled server can edit, and from then on the admin's own choice survives page loads — `AutoConfig` runs from `boot()`, so on every request, and it used to re-apply a hardcoded list over whatever the settings UI had written. Also checks the setting reaches the editor, by reading the connector's own config response rather than sdkjs internals; and that an install configured *before* any of this is repaired on upgrade, since seeding only ever runs while the connector has no url — but only when it still carries exactly what the old code wrote, so an admin who has changed anything keeps their settings. Runs `occ upgrade` twice, which is the only thing that carries an app's repair steps | | `smoke` | every format opens with no JS exception or failed request, takes an edit, and that edit is in the file after a flush. Catches the whole class of "the editor does not come up": an unrendered `api.js`, stylesheets killed by the CSP nonce, fonts the converter cannot find, appdata the file cache never heard about | -| `flush-live` | flushing a document somebody is still editing writes the file and leaves the change list and the document folder alone. The change list is the only record of what was typed — `Editor.bin` stays at the version the document was opened at — so consuming it mid-session strands the document | +| `flush-live` | flushing a document somebody is still editing writes the file and leaves the change list and the document folder alone, and `--snapshot` does that write on its own. The change list is the only record of what was typed — `Editor.bin` stays at the version the document was opened at — so consuming it mid-session strands the document | | `autosave` | edits reach the file while the document is open, with no cron and no flush; a save inside the interval does not re-assemble the document; `autosave_interval 0` turns it off | | `leave` | closing the editor and closing the tab both write the document out and dispose of it, one participant leaving does not end the document for the other, and reopening shows all of it | | `twotab` | two tabs of *one* browser — which share a cookie jar, a Nextcloud session and a connection pool, as two separate browsers do not — see each other type; and one of them dropped to view mode (`asc_coAuthoringDisconnect`, which is what sdkjs does on a licence verdict or a rights change) does not take the document away from the other | | `coedit` | two users in one document each see what the other types, in a text document and in a spreadsheet, with one socket session per browser — a second one means the transport reconnected mid-edit | +| `forcesave` | the Save button the "Keep intermediate versions when editing" setting puts in the editor: the two-part reply the editor accepts, the text in the file with no cron and no flush, and a session opened afterwards still seeing everything — the button must snapshot, not flush | | `chat` | the chat panel carries messages both ways, and a late joiner reads the backlog | ## Things that will mislead you @@ -59,7 +61,29 @@ nothing about a particular instance is hardcoded in a test. cannot upgrade, and the 400 is what makes the client fall back to long polling. - An app config value written with `occ` takes a few seconds to be visible to - web requests, which is what `harness.APP_CONFIG_PROPAGATION` waits out. + web requests, which is what `harness.APP_CONFIG_PROPAGATION` waits out, and + `harness.wait_for()` polls out where the wait can be shorter. +- **`occ` boots this app too**, so an `occ` call made while the connector is + unconfigured re-runs auto-configuration — which is why `formats_test` deletes + the connector's url *last* and puts it back afterwards. +- **The php image caches compiled code for up to a minute** + (`opcache.revalidate_freq=60`), so a code change you just made may not be in + the next web request even though the mount is live. `docker compose restart + app` clears it; `occ` does not use opcache at all, which is how the same + request can take the old path from the browser and the new one from the + command line. +- **A failed run leaves polls in flight, and they put their sessions back.** + Each editor holds a long poll for up to `Channel::TIMEOUT` (25 s), and + `Channel::getResponse()` re-creates a session whose row it cannot find — so a + `harness.reset()` right after a test that died mid-edit can be followed by + rows appearing from nowhere, and the next test counts editors that are not + there. Wait it out, or `docker compose restart app` to drop the requests, + before trusting a session count. +- **An app's repair steps only run when its installed version is behind + `appinfo/info.xml`.** `occ maintenance:repair` runs core's, not the app's, so + `formats_test` lowers `installed_version` and runs `occ upgrade` — and a + change that needs a repair step reaches a real instance only if the version in + `info.xml` was bumped along with it. - Deleting appdata behind Nextcloud's back leaves stale file cache rows and the next conversion fails with `Could not create path .../Editor.bin`; `harness.reset()` follows it with `files:scan-app-data` for that reason. diff --git a/tests/rig/flush_live_test.py b/tests/rig/flush_live_test.py index ce86381..0de09d6 100755 --- a/tests/rig/flush_live_test.py +++ b/tests/rig/flush_live_test.py @@ -11,8 +11,10 @@ 1. type, then flush while the session is live 2. the file has the text, and the change list and document are still there - 3. type again, flush again with everybody gone - 4. the file must hold both edits - the second flush must not have dropped the + 3. `--snapshot`, which is that write on its own: for cron at a chosen + interval, or to get everything on disk on demand + 4. type again, flush again with everybody gone + 5. the file must hold both edits - the second flush must not have dropped the first one """ import argparse, asyncio, os, subprocess, sys, time @@ -50,6 +52,10 @@ async def main(): print('==> resetting rig state') harness.reset() + # from the blank document, not whatever earlier runs left in the sample: + # this types three times, and a click into a document that already has text + # in it lands in the middle of it rather than after it + harness.restore_document(args.kind) # off, so that what the file holds is the flush's doing and nothing else harness.set_app_config('autosave_interval', 0) time.sleep(harness.APP_CONFIG_PROPAGATION) @@ -84,6 +90,33 @@ async def main(): ok = False print(' FAIL - the flush disposed of a document that is still open') + print(f'\n==> [1b] type {t}s, then flush --snapshot') + await A.type(f'{t}s ', row_offset=1) + await A.drain(10) + before = harness.changes() + print(' flush --snapshot: exit=%d %s' % harness.flush('--snapshot')) + got = harness.file_markers(args.file, args.member, [f'{t}1', f'{t}s']) + print(' file:', got, '| changes:', harness.changes(), '(was', str(before) + ')', + '| doc folders:', harness.doc_folders(), + '\n snapshot state:', harness.snapshot_state()) + if not all(got.values()): + ok = False + print(' FAIL - --snapshot did not write the document') + if harness.changes() < before: + ok = False + print(' FAIL - --snapshot consumed the change list of a live session') + if harness.doc_folders() == 0: + ok = False + print(' FAIL - --snapshot disposed of a document that is still open') + # a second one with nothing typed since is the common case for a + # document left open in a tab, and has to be a no-op rather than an + # error + code, out = harness.flush('--snapshot') + print(' again, with nothing typed since: exit=%d %s' % (code, out)) + if code != 0: + ok = False + print(' FAIL - a redundant --snapshot reports failure') + print('\n==> [2] a second session joins: it must see the first one\'s work') B = Session('B', 9333, ub, pb, args.base, args.fileid, args.chromium, (420, 470)) try: @@ -91,8 +124,8 @@ async def main(): await B.login_and_open() await B.drain(25) text = str(await B.eval(TEXT_JS)) - print(' joiner sees the marker:', f'{t}1' in text) - if f'{t}1' not in text: + print(' joiner sees the markers:', f'{t}1' in text, f'{t}s' in text) + if f'{t}1' not in text or f'{t}s' not in text: ok = False print(' FAIL - the joiner replayed an empty change list against the ' 'baseline and got the document as it was before the edits') @@ -110,7 +143,7 @@ async def main(): harness.drop_sessions() print(' flush: exit=%d %s' % harness.flush()) got = harness.file_markers(args.file, args.member, - [f'{t}1', f'{t}2']) + [f'{t}1', f'{t}s', f'{t}2']) print(' file:', got, '| doc folders:', harness.doc_folders(), '| changes:', harness.changes()) if not all(got.values()): ok = False diff --git a/tests/rig/forcesave_test.py b/tests/rig/forcesave_test.py new file mode 100755 index 0000000..5e3ddbf --- /dev/null +++ b/tests/rig/forcesave_test.py @@ -0,0 +1,218 @@ +#!/usr/bin/env python3 +"""Press the editor's Save button and check the save from every angle. + +"Keep intermediate versions when editing" sets +editorConfig.customization.forcesave, which gives the editor a Save button. +Pressing it sends a forceSaveStart command the dispatcher had no handler for, +so the button did nothing and the setting looked broken from the settings UI. + +The reply is in two parts and the editor is fussy about both: it stores the +timestamp from forceSaveStart and ignores a forceSave whose time does not +match, so a mismatch leaves the button spinning until the conversion timeout. +Pressing Save with nothing typed since the last write is a state of its own +(NotModified), not a failure. + +And the save has to be a snapshot, not a flush. Writing the file while +consuming the change list looks fine if you only download the file - the bytes +are right at that moment - and breaks everything after: the change list is the +whole record of what was typed, Editor.bin stays at the version the document +was opened at, so a session opening next renders the document from before the +save and the eventual flush writes a file that has lost the earlier edits. So +after the save this checks, in order: what the editor was told, the file on +disk, a freshly opened session, and the file again once everyone has left. +""" +import argparse, asyncio, json, os, sys, time + +sys.path.insert(0, os.path.join(os.path.dirname(os.path.abspath(__file__)), 'lib')) +import config +import harness +from driver import Session, DISMISS_JS, TEXT_JS + +# The real Save button path, not CoAuthoringApi.forceSave() directly: with +# autosave off and the "Keep intermediate versions" customization on, +# asc_Save(false) is what the toolbar button and Ctrl+S run, and it only reaches +# forceSave() through isForceSaveOnUserSave. Calling forceSave() straight would +# hide the setting not arriving. +SAVE_JS = r"""(() => { + try { + const w = document.querySelector('iframe').contentWindow; + const api = w.Asc.editor; + const inner = api.CoAuthoringApi._CoAuthoringApi; + if (!w.__rigForceSave) { + w.__rigForceSave = []; + const orig = inner.onForceSave; + inner.onForceSave = function (e) { w.__rigForceSave.push(e); if (orig) orig.call(this, e); }; + } + w.__rigForceSave.length = 0; + api.asc_setAutoSaveGap(0); + const res = api.asc_Save(false); + return JSON.stringify({isForceSaveOnUserSave: api.isForceSaveOnUserSave, + docCanSave: api.asc_isDocumentCanSave(), ascSave: res}); + } catch (e) { return 'err: ' + e; } +})()""" + +EVENTS_JS = r"""(() => { + const w = document.querySelector('iframe').contentWindow; + return JSON.stringify(w.__rigForceSave || 'no recorder'); +})()""" + + +def markers(document, names): + return harness.file_markers(document['name'], document['member'], names) + + +async def press_save(session, what): + print(f'\n==> pressing Save {what}:', await session.eval(SAVE_JS)) + await session.drain(25) + events = json.loads(await session.eval(EVENTS_JS)) + print(' the editor was told:', events) + return events + + +async def main(): + p = argparse.ArgumentParser() + p.add_argument('--base', default=config.BASE) + p.add_argument('--a', default=config.ADMIN) + p.add_argument('--kind', default='docx', choices=sorted(config.DOCUMENTS)) + p.add_argument('--fileid', default=None) + p.add_argument('--chromium', default=config.CHROMIUM) + p.add_argument('--tag', default='SAVE') + args = p.parse_args() + document = config.DOCUMENTS[args.kind] + fileid = args.fileid or config.document(args.kind)['fileid'] + + ua, pa = args.a.split(':', 1) + # unique per run: the sample file keeps what earlier runs typed into it, and + # a leftover marker looks exactly like one that was just written + t = args.tag + str(int(time.time()) % 100000) + before, after = t + 'a', t + 'b' + ok = True + + print('==> resetting rig state') + harness.reset() + # from the blank document, not whatever earlier runs left in the sample: + # this test types twice, and a second click into a document that already + # has text in it lands in the middle of it rather than after it + harness.restore_document(args.kind) + # the setting whose button this is, and no autosave, so nothing but the + # button can put the text in the file + harness.set_app_config('customizationForcesave', 'true', harness.CONNECTOR_ID) + harness.set_app_config('autosave_interval', 0) + forcesave = harness.wait_for( + lambda: harness.editor_config(fileid)['editorConfig']['customization'].get('forcesave'), + True) + print(' forcesave in the editor config:', forcesave) + if forcesave is not True: + harness.delete_app_config('autosave_interval') + print(' FAIL - the editor will not even have a Save button') + return 1 + time.sleep(harness.APP_CONFIG_PROPAGATION) + # a rig with background jobs in AJAX mode runs the Cleanup job whenever a + # browser makes a request, and that job writes live documents too - so "the + # file has not been written yet" has to be checked against it + job_before = harness.job_last_run() + + A = Session('A', 9222, ua, pa, args.base, fileid, args.chromium) + try: + await A.start() + print(f'==> opening file {fileid} as {ua}') + await A.login_and_open() + await A.drain(25) + await A.eval(DISMISS_JS) + await A.type(before + ' ', row_offset=0) + await A.drain(8) + + got = markers(document, [before]) + print(f' file before any save: {got}') + if got[before]: + if harness.job_last_run() != job_before: + print(' (the Cleanup job ran in this window and wrote it; the ' + 'button is not the only writer this run)') + else: + ok = False + print(' FAIL - something else wrote the file, with autosave off ' + 'and the job not having run') + + events = await press_save(A, 'once') + # {start: true} then {success: true}, both for the Button type: the two + # halves of the reply, and the second only arrives if its timestamp + # matches the one the first handed over + if not any(e.get('start') for e in events): + ok = False + print(' FAIL - the editor was never told the save started; the ' + 'command was dropped') + if not any(e.get('success') for e in events): + ok = False + print(' FAIL - no forceSave reply the editor accepted: a missing ' + 'reply, or a timestamp that did not match forceSaveStart') + + got = markers(document, [before]) + print(f'\n 1. file on disk right after the save: {got}') + if not got[before]: + ok = False + print(' FAIL - the save did not reach the file') + + print('\n==> pressing Save again with nothing typed since') + events = await press_save(A, 'with nothing to save') + if any(e.get('success') is False for e in events): + ok = False + print(' FAIL - a redundant save is reported as an error') + if not any(e.get('refuse') for e in events): + print(' (not answered as NotModified; harmless as long as it ' + 'is not an error)') + + print('\n==> typing again, which is what makes consuming the change ' + 'list dangerous') + await A.type(after + ' ', row_offset=1) + await A.drain(8) + await press_save(A, 'after the second edit') + + print('\n==> a second session opens the document while the first still edits') + B = Session('B', 9333, ua, pa, args.base, fileid, args.chromium) + try: + await B.start() + await B.login_and_open() + await B.drain(25) + text = str(await B.eval(TEXT_JS) or '') + missing = [m for m in (before, after) if m not in text] + print(f' 2. the new session renders: missing {missing or "nothing"}') + if missing: + ok = False + print(' FAIL - it shows the document from before the save; ' + 'the change list was consumed') + print(' it renders:', text[:160]) + finally: + await B.stop() + finally: + await A.stop() + + print('\n==> everyone has left; expiring the sessions and flushing as cron would') + harness.drop_sessions() + code, out = harness.flush() + print(f' documentserver:flush exit={code} {out}') + if code != 0: + ok = False + print(' FAIL - the flush failed') + + got = markers(document, [before, after]) + print(f' 3. file on disk after the flush: {got}') + # the pre-save edit is the one at risk: a flush replaying a change list that + # no longer holds it, against a baseline that was never rebased, drops it + if not all(got.values()): + ok = False + print(' FAIL - the flush lost an edit the Save button had written') + + harness.delete_app_config('autosave_interval') + + errors = harness.app_log_errors() + if errors: + print('\n===== errors the app logged =====') + for line in errors[:5]: + print(' ', line[:200]) + + print('\n VERDICT:', 'the Save button saves' if ok else 'FAIL') + return 0 if ok else 1 + + +if __name__ == '__main__': + sys.exit(asyncio.run(main())) diff --git a/tests/rig/formats_test.py b/tests/rig/formats_test.py new file mode 100755 index 0000000..43349b2 --- /dev/null +++ b/tests/rig/formats_test.py @@ -0,0 +1,296 @@ +#!/usr/bin/env python3 +"""Check that the onlyoffice format settings are seeded, and then left alone. + +`AutoConfig::autoConfigIfNeeded()` runs from `boot()`, so on every request, and +it used to re-apply a hardcoded format list - force-disabling anything the +admin had enabled that the list did not mention. Enabling pdf, docm or html +editing in the settings UI therefore worked until the next page load, which is +as good as not working at all. The list had also gone stale: twelve formats +where the bundled server declares twenty-nine editable, three of the twelve +(doc, ppt, xls) not editable at all. + +Four things, in order: + + 1. the seed - a fresh install gets every format the bundled server can edit, + and only the curated set as default openers + 2. no reversion - an admin's own choice survives page loads + 3. the setting reaches the editor - the connector opens a file in view mode + when its format is not enabled for editing, and in edit mode when it is + 4. the repair - an install configured before any of this gets the corrected + matrix on upgrade, unless the admin has changed it + +No browser needed: what the editor is told is the connector's own config +response, which is a cheaper and less ambiguous reading than sdkjs internals. +""" +import argparse, json, os, re, subprocess, sys + +sys.path.insert(0, os.path.join(os.path.dirname(os.path.abspath(__file__)), 'lib')) +import config +import harness + +# The four editors the bundled server ships, and the lossy-editable formats the +# connector leaves off by default - which is the reason seeding exists at all. +MUST_EDIT = ['docx', 'xlsx', 'pptx', 'pdf', 'odt', 'ods', 'odp', 'csv', 'rtf', 'txt'] +# Known to the format matrix, but view-only. The old hardcoded list claimed doc, +# ppt and xls were editable; they only auto-convert. +MUST_NOT_EDIT = ['doc', 'ppt', 'xls', 'vsdx'] +# Formats the old list did not mention, so an admin who enabled them had them +# switched back off by the next page load. +WAS_REVERTED = ['pdf', 'docm', 'html', 'epub', 'ott', 'tsv', 'xlsm'] + +DEFAULT_OPENERS = ['doc', 'docx', 'odp', 'ods', 'odt', 'pdf', 'ppt', 'pptx', 'xls', 'xlsx'] + +# What the old hardcoded seed wrote, and therefore what an install configured +# before this change still carries. Kept here as its own copy rather than +# imported from anywhere: the point of the check is that the repair recognises +# these exact lists, so a test that read them from the code under test would +# agree with it no matter what it said. +LEGACY_DEFAULT = ['doc', 'docx', 'odp', 'ods', 'odt', 'pdf', 'ppt', 'pptx', 'xls', 'xlsx'] +LEGACY_EDIT = ['csv', 'doc', 'docx', 'odp', 'ods', 'odt', 'ppt', 'pptx', 'rtf', 'txt', + 'xls', 'xlsx'] + +INFO_XML = os.path.join(os.path.dirname(os.path.abspath(__file__)), '..', '..', + 'appinfo', 'info.xml') + + +def page_loads(times=3): + """Ordinary web requests, which is all it took to lose the setting.""" + user, password = config.credentials(config.ADMIN) + for _ in range(times): + subprocess.run(['curl', '-s', '-o', '/dev/null', '-u', f'{user}:{password}', + f'{config.BASE}/apps/files/'], capture_output=True) + + +def seed(ok): + """What a fresh install gets. + + The connector being unconfigured is what makes this the initial + auto-config, so its url has to go last: occ boots this app too, and any occ + call made while the url is missing re-runs the seed - which would then be + undone by the deletes that were meant to come first. + """ + print('\n==> [1] the seed a fresh install gets') + url = harness.get_app_config('DocumentServerUrl', harness.CONNECTOR_ID) + harness.delete_app_config('editFormats', harness.CONNECTOR_ID) + harness.delete_app_config('defFormats', harness.CONNECTOR_ID) + harness.delete_app_config('DocumentServerUrl', harness.CONNECTOR_ID) + try: + harness.occ('status') # boots the app with the connector unconfigured + edit = harness.get_json_app_config('editFormats') or {} + default = harness.get_json_app_config('defFormats') or {} + editable = sorted(name for name, on in edit.items() if on) + openers = sorted(name for name, on in default.items() if on) + print(f' {len(editable)} editable: {editable}') + print(f' {len(openers)} default openers: {openers}') + + missing = [name for name in MUST_EDIT if not edit.get(name)] + if missing: + ok = False + print(f' FAIL - the seed left these off: {missing}') + wrong = [name for name in MUST_NOT_EDIT if edit.get(name)] + if wrong: + ok = False + print(f' FAIL - the seed enabled editing for formats the server ' + f'cannot edit: {wrong}') + if openers != sorted(DEFAULT_OPENERS): + ok = False + print(f' FAIL - default openers should be {sorted(DEFAULT_OPENERS)}') + # the whole point of reading the matrix out of the package instead of + # restating it: the number goes up with the bundled server + if len(editable) < 20: + ok = False + print(f' FAIL - only {len(editable)} editable formats seeded; the ' + f'bundled server declares far more') + finally: + # the provisioned url, not the one auto-config just wrote: that one + # comes from overwrite.cli.url, and an editor iframe on a different + # origin than the page cannot be read by the other tests + harness.set_app_config('DocumentServerUrl', url, harness.CONNECTOR_ID) + print(' url restored to', harness.get_app_config( + 'DocumentServerUrl', harness.CONNECTOR_ID)) + return ok + + +def no_reversion(ok): + print("\n==> [2] an admin's own format choice survives page loads") + before = harness.get_json_app_config('editFormats') or {} + choice = dict(before) + # an admin turning things on, including the ones the hardcoded list used to + # revert, and one thing off, so a wholesale rewrite shows up either way + for name in WAS_REVERTED: + choice[name] = True + choice['docx'] = True + choice['rtf'] = False + harness.set_json_app_config('editFormats', choice) + + print(f' set {len(WAS_REVERTED)} formats the old code reverted, plus rtf off') + stored = harness.wait_for(lambda: harness.get_json_app_config('editFormats'), choice) + lost = sorted(name for name, on in choice.items() if bool(stored.get(name)) != bool(on)) + if lost: + # not "the write failed": reading it back is an occ call, which boots + # this app, which is all it used to take to have the value rewritten + ok = False + print(f' FAIL - the choice did not even survive being read back: {lost}') + + page_loads() + after = harness.get_json_app_config('editFormats') + reverted = sorted(name for name, on in choice.items() if bool(after.get(name)) != bool(on)) + if reverted: + ok = False + print(f' FAIL - page loads changed these back: {reverted}') + elif not lost: + print(' nothing was rewritten') + + harness.set_json_app_config('editFormats', before) + harness.wait_for(lambda: harness.get_json_app_config('editFormats'), before) + return ok + + +def reaches_the_editor(ok, fileid, kind): + print(f'\n==> [3] the {kind} setting reaches the editor') + before = harness.get_json_app_config('editFormats') or {} + + off = dict(before, **{kind: False}) + harness.set_json_app_config('editFormats', off) + mode = harness.wait_for(lambda: harness.editor_mode(fileid), 'view') + print(f' editing {kind} off -> the connector opens it in {mode!r} mode') + if mode != 'view': + ok = False + print(' FAIL - the setting does not reach the editor at all') + + harness.set_json_app_config('editFormats', dict(before, **{kind: True})) + mode = harness.wait_for(lambda: harness.editor_mode(fileid), 'edit') + print(f' editing {kind} on -> the connector opens it in {mode!r} mode') + if mode != 'edit': + ok = False + print(' FAIL - the file cannot be edited with its format enabled') + + harness.set_json_app_config('editFormats', before) + harness.wait_for(lambda: harness.get_json_app_config('editFormats'), before) + return ok + + +def write_matrix(default_on, edit_on): + """The stored matrix as the old code left it: an entry per known format. + + An explicit true or false for every format the connector knows, because + that is what the old seed wrote - and what the repair has to recognise. A + map with only the enabled formats in it would read back differently, since + the connector fills the gaps from its own defaults. + """ + keys = list((harness.get_json_app_config('defFormats') or {}).keys()) + harness.set_json_app_config('defFormats', {k: k in default_on for k in keys}) + harness.set_json_app_config('editFormats', {k: k in edit_on for k in keys}) + return keys + + +def enabled(key): + return sorted(name for name, on in (harness.get_json_app_config(key) or {}).items() if on) + + +def previous_version(): + """A version below the one in info.xml, so the app's upgrade path runs.""" + version = re.search(r'([^<]+)', open(INFO_XML).read()).group(1) + parts = [int(part) for part in version.split('.')] + for i in range(len(parts) - 1, -1, -1): + if parts[i]: + parts[i] -= 1 + return '.'.join(str(part) for part in parts) + raise RuntimeError(f'no version below {version}') + + +def upgrade_the_app(): + """Run the app's upgrade path, which is what carries its repair steps. + + Repair steps are not part of `occ maintenance:repair` - that runs core's - + so the only way to reach one is an app whose installed version is behind + what info.xml declares. Which is also the reason the version has to be + bumped for any of this to reach a real instance. + """ + harness.occ('config:app:set', harness.APP_ID, 'installed_version', + '--value', previous_version(), check=True) + harness.occ('upgrade', check=True) + + +def repair(ok): + print('\n==> [4] an existing install is repaired on upgrade') + before_default = harness.get_json_app_config('defFormats') or {} + before_edit = harness.get_json_app_config('editFormats') or {} + if not before_default: + print(' FAIL - no stored format matrix to work from') + return False + + try: + # (a) an install still carrying exactly what the old seed wrote. Seeding + # only ever runs while the connector has no url, so without the repair + # this instance would keep the stale matrix for good. + write_matrix(LEGACY_DEFAULT, LEGACY_EDIT) + print(f' wrote the old hardcoded matrix ({len(LEGACY_EDIT)} editable)') + upgrade_the_app() + + editable, openers = enabled('editFormats'), enabled('defFormats') + print(f' after upgrade: {len(editable)} editable, {len(openers)} default openers') + if len(editable) < 20: + ok = False + print(f' FAIL - the matrix was not repaired: still {editable}') + wrong = [name for name in MUST_NOT_EDIT if name in editable] + if wrong: + ok = False + print(f' FAIL - repair left formats the server cannot edit: {wrong}') + missing = [name for name in MUST_EDIT if name not in editable] + if missing: + ok = False + print(f' FAIL - repair left these off: {missing}') + if openers != sorted(DEFAULT_OPENERS): + ok = False + print(f' FAIL - repair changed the default openers to {openers}') + + # (b) the same install, with one format an admin turned on themselves. + # The old code force-wrote the matrix on every request, so anything + # other than the two lists exactly is a choice somebody made after it + # stopped doing that - and it has to survive. + customised = sorted(LEGACY_EDIT + ['html']) + write_matrix(LEGACY_DEFAULT, customised) + print(' wrote the old matrix plus one format an admin enabled (html)') + upgrade_the_app() + + editable = enabled('editFormats') + if editable != customised: + ok = False + print(f" FAIL - the admin's choice was overwritten: {editable}") + else: + print(f' after upgrade: unchanged, {len(editable)} editable') + finally: + harness.set_json_app_config('defFormats', before_default) + harness.set_json_app_config('editFormats', before_edit) + harness.wait_for(lambda: harness.get_json_app_config('editFormats'), before_edit) + print(' matrix restored') + + return ok + + +def main(): + p = argparse.ArgumentParser() + p.add_argument('--kind', default='docx', choices=sorted(config.DOCUMENTS)) + p.add_argument('--fileid', default=None) + args = p.parse_args() + fileid = args.fileid or config.document(args.kind)['fileid'] + + ok = True + ok = seed(ok) + ok = no_reversion(ok) + ok = reaches_the_editor(ok, fileid, args.kind) + ok = repair(ok) + + errors = harness.app_log_errors() + if errors: + print('\n===== errors the app logged =====') + for line in errors[:5]: + print(' ', line[:200]) + + print('\n VERDICT:', 'the format settings hold' if ok else 'FAIL') + return 0 if ok else 1 + + +if __name__ == '__main__': + sys.exit(main()) diff --git a/tests/rig/lib/harness.py b/tests/rig/lib/harness.py index c6de18d..7c54281 100644 --- a/tests/rig/lib/harness.py +++ b/tests/rig/lib/harness.py @@ -5,15 +5,23 @@ for the saving tests, since the editor saying "all changes are saved" only ever meant they reached the server. """ +import base64 +import json import os import subprocess import sys import tempfile +import time +import urllib.request sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) import config APP_ID = 'documentserver_community' +# The onlyoffice connector. Its app config is where the admin settings this app +# has to honour live - the format matrix, the Save button - so the tests read +# and write it as an admin would. +CONNECTOR_ID = 'onlyoffice' def occ(*args, check=False): @@ -41,23 +49,84 @@ def sql(query): return r.stdout.strip() -def set_app_config(key, value): +def set_app_config(key, value, app=APP_ID): """An app config value, and the wait for it to be visible to web requests. occ writes it through the local cache the web server does not share, so a test that reads it back straight away sees the old one and concludes the setting does nothing. """ - occ('config:app:set', APP_ID, key, '--value', str(value)) + occ('config:app:set', app, key, '--value', str(value)) -def delete_app_config(key): - occ('config:app:delete', APP_ID, key) +def get_app_config(key, app=APP_ID): + return occ('config:app:get', app, key) + + +def delete_app_config(key, app=APP_ID): + occ('config:app:delete', app, key) APP_CONFIG_PROPAGATION = 14 # seconds; see set_app_config +def get_json_app_config(key, app=CONNECTOR_ID): + """A json-valued app config setting, or None when it is not set at all. + + The connector keeps the format matrix like this: one setting holding an + extension -> bool map. + """ + raw = get_app_config(key, app) + try: + return json.loads(raw) + except ValueError: + return None + + +def set_json_app_config(key, value, app=CONNECTOR_ID): + set_app_config(key, json.dumps(value, sort_keys=True), app) + + +def editor_config(fileid, user_pair=None): + """What the connector decides about a file, as the editor page is told it. + + The connector's own answer, rather than a guess from sdkjs internals: this + is where a format setting shows up. editorConfig.mode is "view" when it has + concluded the file is not editable, and document.permissions.edit is only + the user's write access, not the format decision. + """ + user, password = config.credentials(user_pair or config.ADMIN) + # the header outright, not an auth handler: those only send credentials + # after a challenge, and an unauthenticated OCS call answers 200 with an + # error envelope rather than the 401 that would trigger one + credentials = base64.b64encode(f'{user}:{password}'.encode()).decode() + request = urllib.request.Request( + f'{config.BASE}/ocs/v2.php/apps/onlyoffice/api/v1/config/{fileid}?format=json', + headers={'OCS-APIRequest': 'true', 'Authorization': f'Basic {credentials}'}) + with urllib.request.urlopen(request, timeout=30) as response: + return json.loads(response.read().decode()) + + +def editor_mode(fileid, user_pair=None): + """"edit" or "view", as the connector would open the file.""" + return editor_config(fileid, user_pair)['editorConfig'].get('mode', 'edit') + + +def wait_for(reading, expected, timeout=30, interval=2): + """Poll until a reading matches, and return the last one either way. + + For settings rather than documents: how long a value written with occ takes + to reach a web request depends on which caches are in play, so a test that + sleeps a fixed time is either slow or flaky. + """ + deadline = time.time() + timeout + while True: + got = reading() + if got == expected or time.time() >= deadline: + return got + time.sleep(interval) + + def reset(): """Drop per-document state so the next run starts from the file on disk.""" sql('truncate oc_documentserver_sess; truncate oc_documentserver_changes; ' @@ -111,8 +180,23 @@ def changes(): return int(sql('select count(*) from oc_documentserver_changes;') or 0) +# SessionManager::EXPIRED_SESSION_TIMEOUT +SESSION_TIMEOUT = 30 + + def sessions(): - return int(sql('select count(*) from oc_documentserver_sess;') or 0) + """How many editors are in a document, which is not how many rows there are. + + A session that has gone is expired rather than deleted - so that a page + that turns out to still be there revives on its next poll instead of + losing its editing session - and the row is only removed later, by the + Cleanup job. So the row count answers "what has not been garbage-collected + yet", and every test that asks this means "who is still editing", which is + the question the app asks too (SessionManager::getSessionsForDocument). + """ + return int(sql( + 'select count(*) from oc_documentserver_sess ' + f'where last_seen >= unix_timestamp() - {SESSION_TIMEOUT};') or 0) def drop_sessions(): diff --git a/tests/rig/run_all.py b/tests/rig/run_all.py index c8680be..846bea2 100755 --- a/tests/rig/run_all.py +++ b/tests/rig/run_all.py @@ -19,10 +19,14 @@ # name -> (script, args, what it covers) SUITE = [ + ('formats', 'formats_test.py', [], + 'the onlyoffice format settings are seeded, then left to the admin, and an ' + 'install configured before any of that is repaired on upgrade'), ('smoke', 'smoke_test.py', [], 'every format opens, edits and reaches the file'), ('flush-live', 'flush_live_test.py', [], - 'flushing a document somebody is still editing must not empty its change list'), + 'flushing a document somebody is still editing must not empty its change ' + 'list, and --snapshot does that write on its own'), ('autosave', 'autosave_test.py', [], 'edits reach the file while editing, with no cron and no flush'), ('leave', 'leave_test.py', [], @@ -33,6 +37,8 @@ 'two users see each other typing'), ('coedit-xlsx', 'coedit_test.py', ['--kind', 'xlsx', '--rounds', '1', '--tag', 'X'], 'the same, in a spreadsheet, where changes carry recalc indexes'), + ('forcesave', 'forcesave_test.py', [], + "the editor's Save button writes the file without ending the session"), ('chat', 'chat_test.py', [], 'the chat panel, including a late joiner reading the backlog'), ]