Make the onlyoffice settings take effect: formats and the Save button

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/<vendor>/documentserver: "onlyoffice" upstream, "euro-office"
 	# on the fork.
 	src=$$(find oo-extract/var/www -mindepth 2 -maxdepth 2 -type d -name documentserver | head -1); \

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.]]>
 	</description>
-	<version>0.3.2</version>
+	<version>0.3.4</version>
 	<licence>agpl</licence>
 	<author>Robin Appelman</author>
 	<namespace>DocumentServer</namespace>
@@ -43,6 +43,7 @@ Additionally, the community document server only supports running on x86-64 Linu
 	<repair-steps>
 		<post-migration>
 			<step>OCA\DocumentServer\Migration\RebuildFonts</step>
+			<step>OCA\DocumentServer\Migration\RepairFormatSettings</step>
 		</post-migration>
 	</repair-steps>
 

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)

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)
 			);
 		});
 	}

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);
 				}

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();
 

lib/Channel/SessionManager.php

@@ -148,19 +148,37 @@ 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);
 	}
 
 	/**
 	 * 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);

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("<error>could not save a snapshot of document $documentId: {$e->getMessage()}</error>");
+				}
+				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("<error>could not flush document $documentId: {$e->getMessage()}</error>");
 				}
 			}
 		}
-		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;
 	}
 }

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) {

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);