Merge pull request #5466 from nextcloud/fix/read_only_pushs

fix(backend): Accept pushs with only step1 messages by read-only clients
nextcloud · Mar 12, 2024 · c9f3af6 · c9f3af6
2 parents ac126df + e794653
commit c9f3af6
Show file tree

Hide file tree

Showing 3 changed files with 13 additions and 13 deletions.
diff --git a/.github/workflows/cypress-e2e.yml b/.github/workflows/cypress-e2e.yml
@@ -59,6 +59,7 @@ jobs:
       - name: Set up node ${{ steps.versions.outputs.nodeVersion }}
         uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2
         with:
+          cache: 'npm'
           node-version: ${{ steps.versions.outputs.nodeVersion }}
 
       - name: Set up npm ${{ steps.versions.outputs.npmVersion }}
@@ -119,6 +120,7 @@ jobs:
       - name: Set up node ${{ needs.init.outputs.nodeVersion }}
         uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2
         with:
+          cache: 'npm'
           node-version: ${{ needs.init.outputs.nodeVersion }}
 
       - name: Set up npm ${{ needs.init.outputs.npmVersion }}

diff --git a/lib/Service/ApiService.php b/lib/Service/ApiService.php
@@ -184,11 +184,8 @@ public function close(int $documentId, int $sessionId, string $sessionToken): Da
 
 	/**
 	 * @throws NotFoundException
-	 * @throws DoesNotExistException
-	 *
-	 * @param null|string $token
 	 */
-	public function push(Session $session, Document $document, int $version, array $steps, string $awareness, string|null $token = null): DataResponse {
+	public function push(Session $session, Document $document, int $version, array $steps, string $awareness, ?string $token = null): DataResponse {
 		try {
 			$session = $this->sessionService->updateSessionAwareness($session, $awareness);
 		} catch (DoesNotExistException $e) {
@@ -198,16 +195,12 @@ public function push(Session $session, Document $document, int $version, array $
 		if (empty($steps)) {
 			return new DataResponse([]);
 		}
-		$file = $this->documentService->getFileForSession($session, $token);
-		if ($this->documentService->isReadOnly($file, $token)) {
-			return new DataResponse([], 403);
-		}
 		try {
-			$result = $this->documentService->addStep($document, $session, $steps, $version);
+			$result = $this->documentService->addStep($document, $session, $steps, $version, $token);
 		} catch (InvalidArgumentException $e) {
 			return new DataResponse($e->getMessage(), 422);
-		} catch (DoesNotExistException $e) {
-			// Session was removed in the meantime. #3875
+		} catch (DoesNotExistException|NotPermittedException) {
+			// Either no write access or session was removed in the meantime (#3875).
 			return new DataResponse([], 403);
 		}
 		return new DataResponse($result);

diff --git a/lib/Service/DocumentService.php b/lib/Service/DocumentService.php
@@ -206,10 +206,12 @@ public function writeDocumentState(int $documentId, string $content): void {
 	}
 
 	/**
-	 * @throws DoesNotExistException
 	 * @throws InvalidArgumentException
+	 * @throws NotFoundException
+	 * @throws NotPermittedException
+	 * @throws DoesNotExistException
 	 */
-	public function addStep(Document $document, Session $session, array $steps, int $version): array {
+	public function addStep(Document $document, Session $session, array $steps, int $version, ?string $shareToken): array {
 		$documentId = $session->getDocumentId();
 		$stepsToInsert = [];
 		$querySteps = [];
@@ -224,6 +226,9 @@ public function addStep(Document $document, Session $session, array $steps, int
 			}
 		}
 		if (count($stepsToInsert) > 0) {
+			if ($this->isReadOnly($this->getFileForSession($session, $shareToken), $shareToken)) {
+				throw new NotPermittedException('Read-only client tries to push steps with changes');
+			}
 			$newVersion = $this->insertSteps($document, $session, $stepsToInsert);
 		}
 		// If there were any queries in the steps send the entire history