From 91f4cb13ef2c31f7e10b0ec7e55f60754946bae4 Mon Sep 17 00:00:00 2001
From: Piotr Mrowczynski <mrow4a@yahoo.com>
Date: Mon, 31 Dec 2018 10:33:33 +0100
Subject: [PATCH] Improve files error handling

---
 apps/dav/lib/Connector/Sabre/FilesPlugin.php | 45 ++++++++++++++------
 apps/files/js/files.js                       |  8 ++--
 apps/files/tests/js/filesSpec.js             | 19 ++++++---
 3 files changed, 48 insertions(+), 24 deletions(-)

diff --git a/apps/dav/lib/Connector/Sabre/FilesPlugin.php b/apps/dav/lib/Connector/Sabre/FilesPlugin.php
index 71b706fb58f8..35c7b7a07f46 100644
--- a/apps/dav/lib/Connector/Sabre/FilesPlugin.php
+++ b/apps/dav/lib/Connector/Sabre/FilesPlugin.php
@@ -152,8 +152,9 @@ public function initialize(\Sabre\DAV\Server $server) {
 		$this->server->on('propPatch', [$this, 'handleUpdateProperties']);
 		$this->server->on('afterBind', [$this, 'sendFileIdHeader']);
 		$this->server->on('afterWriteContent', [$this, 'sendFileIdHeader']);
-		$this->server->on('afterMethod:GET', [$this,'httpGet']);
+		$this->server->on('afterMethod:GET', [$this, 'httpGet']);
 		$this->server->on('afterMethod:GET', [$this, 'handleDownloadToken']);
+		$this->server->on('exception', [$this, 'handleDownloadFailure']);
 		$this->server->on('afterResponse', function ($request, ResponseInterface $response) {
 			$body = $response->getBody();
 			if (\is_resource($body)) {
@@ -191,10 +192,21 @@ public function checkMove($source, $destination) {
 		}
 	}
 
+	/**
+	 * This sets a cookie to be able to recognize the failure of the download
+	 *
+	 * @param \Exception $ex
+	 */
+	public function handleDownloadFailure(\Exception $ex) {
+		$queryParams = $this->server->httpRequest->getQueryParameters();
+
+		if (isset($queryParams['downloadStartSecret'])) {
+			$this->setSecretCookie('ocDownloadStarted', '-1');
+		}
+	}
+
 	/**
 	 * This sets a cookie to be able to recognize the start of the download
-	 * the content must not be longer than 32 characters and must only contain
-	 * alphanumeric characters
 	 *
 	 * @param RequestInterface $request
 	 * @param ResponseInterface $response
@@ -202,18 +214,8 @@ public function checkMove($source, $destination) {
 	public function handleDownloadToken(RequestInterface $request, ResponseInterface $response) {
 		$queryParams = $request->getQueryParameters();
 
-		/**
-		 * this sets a cookie to be able to recognize the start of the download
-		 * the content must not be longer than 32 characters and must only contain
-		 * alphanumeric characters
-		 */
 		if (isset($queryParams['downloadStartSecret'])) {
-			$token = $queryParams['downloadStartSecret'];
-			if (!isset($token[32])
-				&& \preg_match('!^[a-zA-Z0-9]+$!', $token) === 1) {
-				// FIXME: use $response->setHeader() instead
-				\setcookie('ocDownloadStarted', $token, \time() + 20, '/');
-			}
+			$this->setSecretCookie('ocDownloadStarted', $queryParams['downloadStartSecret']);
 		}
 	}
 
@@ -433,4 +435,19 @@ public function sendFileIdHeader($filePath, \Sabre\DAV\INode $node = null) {
 			}
 		}
 	}
+
+	/**
+	 * This sets a cookie, which content must not be longer than 32 characters and must only contain
+	 * alphanumeric characters if request is successfuk, or -1 if request failed.
+	 *
+	 * @param $secretName
+	 * @param $secretToken
+	 */
+	private function setSecretCookie($secretName, $secretToken) {
+		if ($secretToken == '-1' || (!isset($secretToken[32])
+			&& \preg_match('!^[a-zA-Z0-9]+$!', $secretToken) === 1)) {
+			// FIXME: use $response->setHeader() instead
+			\setcookie($secretName, $secretToken, \time() + 20, '/');
+		}
+	}
 }
diff --git a/apps/files/js/files.js b/apps/files/js/files.js
index c6cca2199937..4a184d0eeaae 100644
--- a/apps/files/js/files.js
+++ b/apps/files/js/files.js
@@ -306,12 +306,14 @@
 		handleDownload: function(url, callback) {
 			var randomToken = Math.random().toString(36).substring(2),
 				checkForDownloadCookie = function() {
-					if (!OC.Util.isCookieSetToValue('ocDownloadStarted', randomToken)){
-						return false;
-					} else {
+					// Successfull request should return cookie with token, failed with -1
+					if (OC.Util.isCookieSetToValue('ocDownloadStarted', randomToken) ||
+						OC.Util.isCookieSetToValue('ocDownloadStarted', '-1')) {
 						callback();
 						return true;
 					}
+
+					return false
 				};
 
 			if (url.indexOf('?') >= 0) {
diff --git a/apps/files/tests/js/filesSpec.js b/apps/files/tests/js/filesSpec.js
index 5a7c75a73d88..e3934c458ab2 100644
--- a/apps/files/tests/js/filesSpec.js
+++ b/apps/files/tests/js/filesSpec.js
@@ -129,19 +129,24 @@ describe('OCA.Files.Files tests', function() {
 			token = OC.parseQueryString(redirectStub.getCall(0).args[0]).downloadStartSecret;
 			expect(token).toBeDefined();
 
-			expect(cookieStub.calledOnce).toEqual(true);
-			cookieStub.returns(false);
+			expect(cookieStub.callCount).toEqual(2);
+			cookieStub.onCall(0).returns(false);
+			cookieStub.onCall(1).returns(false);
 			clock.tick(600);
 
-			expect(cookieStub.calledTwice).toEqual(true);
-			expect(cookieStub.getCall(1).args[0]).toEqual('ocDownloadStarted');
-			expect(cookieStub.getCall(1).args[1]).toEqual(token);
+			expect(cookieStub.callCount).toEqual(4);
+			expect(cookieStub.getCall(2).args[0]).toEqual('ocDownloadStarted');
+			expect(cookieStub.getCall(2).args[1]).toEqual(token);
+			expect(cookieStub.getCall(3).args[0]).toEqual('ocDownloadStarted');
+			expect(cookieStub.getCall(3).args[1]).toEqual('-1');
 			expect(callbackStub.notCalled).toEqual(true);
 
+			// now 1 ocDownloadStarted expected, with token argument
+			// should be called and return true
 			cookieStub.returns(true);
-			clock.tick(2000);
 
-			expect(cookieStub.callCount).toEqual(3);
+			clock.tick(2000);
+			expect(cookieStub.callCount).toEqual(5);
 			expect(callbackStub.calledOnce).toEqual(true);
 		});
 	});