Merge pull request #6585 from nextcloud/theming-fileupload-errorhandling

Theming: Handle errors on uploaded files properly

4 files changed, 222 insertions, 22 deletions

diff --git a/apps/theming/css/settings-admin.css b/apps/theming/css/settings-admin.css
index b32ed189ceb..7270ec59b83 100644
--- a/apps/theming/css/settings-admin.css
+++ b/apps/theming/css/settings-admin.css
@@ -78,6 +78,7 @@ form.uploadButton {
 
 #theming_settings_msg {
     vertical-align: middle;
+    border-radius: 3px;
 }
 
 #theming-preview-logo {
diff --git a/apps/theming/js/settings-admin.js b/apps/theming/js/settings-admin.js
index d9e66284d14..44a799a19b4 100644
--- a/apps/theming/js/settings-admin.js
+++ b/apps/theming/js/settings-admin.js
@@ -141,6 +141,7 @@ $(document).ready(function () {
 		fail: function (e, response){
 			OC.msg.finishedError('#theming_settings_msg', response._response.jqXHR.responseJSON.data.message);
 			$('label#uploadlogo').addClass('icon-upload').removeClass('icon-loading-small');
+			$('#theming_settings_loading').hide();
 		}
 	};
 	var uploadParamsLogin = {
@@ -159,6 +160,7 @@ $(document).ready(function () {
 		fail: function (e, response){
 			$('label#upload-login-background').removeClass('icon-loading-small').addClass('icon-upload');
 			OC.msg.finishedError('#theming_settings_msg', response._response.jqXHR.responseJSON.data.message);
+			$('#theming_settings_loading').hide();
 		}
 	};
 
diff --git a/apps/theming/lib/Controller/ThemingController.php b/apps/theming/lib/Controller/ThemingController.php
index 06c2c430b7f..ccc2634ec14 100644
--- a/apps/theming/lib/Controller/ThemingController.php
+++ b/apps/theming/lib/Controller/ThemingController.php
@@ -207,12 +207,34 @@ class ThemingController extends Controller {
 		}
 		$newLogo = $this->request->getUploadedFile('uploadlogo');
 		$newBackgroundLogo = $this->request->getUploadedFile('upload-login-background');
+		$error = null;
+		$phpFileUploadErrors = [
+			UPLOAD_ERR_OK => $this->l10n->t('There is no error, the file uploaded with success'),
+			UPLOAD_ERR_INI_SIZE => $this->l10n->t('The uploaded file exceeds the upload_max_filesize directive in php.ini'),
+			UPLOAD_ERR_FORM_SIZE => $this->l10n->t('The uploaded file exceeds the MAX_FILE_SIZE directive that was specified in the HTML form'),
+			UPLOAD_ERR_PARTIAL => $this->l10n->t('The uploaded file was only partially uploaded'),
+			UPLOAD_ERR_NO_FILE => $this->l10n->t('No file was uploaded'),
+			UPLOAD_ERR_NO_TMP_DIR => $this->l10n->t('Missing a temporary folder'),
+			UPLOAD_ERR_CANT_WRITE => $this->l10n->t('Failed to write file to disk.'),
+			UPLOAD_ERR_EXTENSION => $this->l10n->t('A PHP extension stopped the file upload.'),
+		];
 		if (empty($newLogo) && empty($newBackgroundLogo)) {
+			$error = $this->l10n->t('No file uploaded');
+		}
+		if (!empty($newLogo) && array_key_exists('error', $newLogo) && $newLogo['error'] !== UPLOAD_ERR_OK) {
+			$error = $phpFileUploadErrors[$newLogo['error']];
+		}
+		if (!empty($newBackgroundLogo) && array_key_exists('error', $newBackgroundLogo) && $newBackgroundLogo['error'] !== UPLOAD_ERR_OK) {
+			$error = $phpFileUploadErrors[$newBackgroundLogo['error']];
+		}
+
+		if ($error !== null) {
 			return new DataResponse(
 				[
 					'data' => [
-						'message' => $this->l10n->t('No file uploaded')
-					]
+						'message' => $error
+					],
+					'status' => 'failure',
 				],
 				Http::STATUS_UNPROCESSABLE_ENTITY
 			);
@@ -227,6 +249,18 @@ class ThemingController extends Controller {
 
 		if (!empty($newLogo)) {
 			$target = $folder->newFile('logo');
+			$supportedFormats = ['image/jpeg', 'image/png', 'image/gif', 'image/svg+xml', 'text/svg'];
+			if (!in_array($newLogo['type'], $supportedFormats)) {
+				return new DataResponse(
+					[
+						'data' => [
+							'message' => $this->l10n->t('Unsupported image type'),
+						],
+						'status' => 'failure',
+					],
+					Http::STATUS_UNPROCESSABLE_ENTITY
+				);
+			}
 			$target->putContent(file_get_contents($newLogo['tmp_name'], 'r'));
 			$this->themingDefaults->set('logoMime', $newLogo['type']);
 			$name = $newLogo['name'];
diff --git a/apps/theming/tests/Controller/ThemingControllerTest.php b/apps/theming/tests/Controller/ThemingControllerTest.php
index c03eccb6eef..e964e886e5c 100644
--- a/apps/theming/tests/Controller/ThemingControllerTest.php
+++ b/apps/theming/tests/Controller/ThemingControllerTest.php
@@ -131,8 +131,9 @@ class ThemingControllerTest extends TestCase {
 		$this->l10n
 			->expects($this->once())
 			->method('t')
-			->with($message)
-			->willReturn($message);
+			->will($this->returnCallback(function($str) {
+				return $str;
+			}));
 		$this->scssCacher
 			->expects($this->once())
 			->method('getCachedSCSS')
@@ -183,8 +184,9 @@ class ThemingControllerTest extends TestCase {
 		$this->l10n
 			->expects($this->once())
 			->method('t')
-			->with($message)
-			->willReturn($message);
+			->will($this->returnCallback(function($str) {
+				return $str;
+			}));
 
 		$expected = new DataResponse(
 			[
@@ -215,10 +217,11 @@ class ThemingControllerTest extends TestCase {
 			->with('upload-login-background')
 			->willReturn(null);
 		$this->l10n
-			->expects($this->once())
+			->expects($this->any())
 			->method('t')
-			->with('No file uploaded')
-			->willReturn('No file uploaded');
+			->will($this->returnCallback(function($str) {
+				return $str;
+			}));
 
 		$expected = new DataResponse(
 			[
@@ -226,6 +229,56 @@ class ThemingControllerTest extends TestCase {
 					[
 						'message' => 'No file uploaded',
 					],
+				'status' => 'failure',
+			],
+			Http::STATUS_UNPROCESSABLE_ENTITY
+		);
+
+		$this->assertEquals($expected, $this->themingController->updateLogo());
+	}
+
+	public function testUpdateLogoInvalidMimeType() {
+		$this->request
+			->expects($this->at(0))
+			->method('getParam')
+			->with('backgroundColor')
+			->willReturn(false);
+		$this->request
+			->expects($this->at(1))
+			->method('getUploadedFile')
+			->with('uploadlogo')
+			->willReturn([
+				'tmp_name' => 'logo.pdf',
+				'type' => 'application/pdf',
+				'name' => 'logo.pdf',
+				'error' => 0,
+			]);
+		$this->request
+			->expects($this->at(2))
+			->method('getUploadedFile')
+			->with('upload-login-background')
+			->willReturn(null);
+		$this->l10n
+			->expects($this->any())
+			->method('t')
+			->will($this->returnCallback(function($str) {
+				return $str;
+			}));
+
+		$folder = $this->createMock(ISimpleFolder::class);
+		$this->appData
+			->expects($this->once())
+			->method('getFolder')
+			->with('images')
+			->willReturn($folder);
+
+		$expected = new DataResponse(
+			[
+				'data' =>
+					[
+						'message' => 'Unsupported image type',
+					],
+				'status' => 'failure'
 			],
 			Http::STATUS_UNPROCESSABLE_ENTITY
 		);
@@ -258,13 +311,17 @@ class ThemingControllerTest extends TestCase {
 
 	public function dataUpdateImages() {
 		return [
-			[false],
-			[true]
+			['image/jpeg', false],
+			['image/jpeg', true],
+			['image/gif'],
+			['image/png'],
+			['image/svg+xml'],
+			['text/svg'],
 		];
 	}
 
 	/** @dataProvider dataUpdateImages */
-	public function testUpdateLogoNormalLogoUpload($folderExists) {
+	public function testUpdateLogoNormalLogoUpload($mimeType, $folderExists=true) {
 		$tmpLogo = \OC::$server->getTempManager()->getTemporaryFolder() . '/logo.svg';
 		$destination = \OC::$server->getTempManager()->getTemporaryFolder();
 
@@ -280,8 +337,9 @@ class ThemingControllerTest extends TestCase {
 			->with('uploadlogo')
 			->willReturn([
 				'tmp_name' => $tmpLogo,
-				'type' => 'text/svg',
+				'type' => $mimeType,
 				'name' => 'logo.svg',
+				'error' => 0,
 			]);
 		$this->request
 			->expects($this->at(2))
@@ -289,10 +347,11 @@ class ThemingControllerTest extends TestCase {
 			->with('upload-login-background')
 			->willReturn(null);
 		$this->l10n
-			->expects($this->once())
+			->expects($this->any())
 			->method('t')
-			->with('Saved')
-			->willReturn('Saved');
+			->will($this->returnCallback(function($str) {
+				return $str;
+			}));
 
 
 		$file = $this->createMock(ISimpleFile::class);
@@ -357,12 +416,14 @@ class ThemingControllerTest extends TestCase {
 				'tmp_name' => $tmpLogo,
 				'type' => 'text/svg',
 				'name' => 'logo.svg',
+				'error' => 0,
 			]);
 		$this->l10n
-			->expects($this->once())
+			->expects($this->any())
 			->method('t')
-			->with('Saved')
-			->willReturn('Saved');
+			->will($this->returnCallback(function($str) {
+				return $str;
+			}));
 
 		$file = $this->createMock(ISimpleFile::class);
 		$folder = $this->createMock(ISimpleFolder::class);
@@ -425,12 +486,14 @@ class ThemingControllerTest extends TestCase {
 				'tmp_name' => $tmpLogo,
 				'type' => 'text/svg',
 				'name' => 'logo.svg',
+				'error' => 0,
 			]);
 		$this->l10n
-			->expects($this->once())
+			->expects($this->any())
 			->method('t')
-			->with('Unsupported image type')
-			->willReturn('Unsupported image type');
+			->will($this->returnCallback(function($str) {
+				return $str;
+			}));
 
 		$folder = $this->createMock(ISimpleFolder::class);
 		$this->appData
@@ -452,6 +515,106 @@ class ThemingControllerTest extends TestCase {
 		$this->assertEquals($expected, $this->themingController->updateLogo());
 	}
 
+	public function dataPhpUploadErrors() {
+		return [
+			[UPLOAD_ERR_INI_SIZE, 'The uploaded file exceeds the upload_max_filesize directive in php.ini'],
+			[UPLOAD_ERR_FORM_SIZE, 'The uploaded file exceeds the MAX_FILE_SIZE directive that was specified in the HTML form'],
+			[UPLOAD_ERR_PARTIAL, 'The uploaded file was only partially uploaded'],
+			[UPLOAD_ERR_NO_FILE, 'No file was uploaded'],
+			[UPLOAD_ERR_NO_TMP_DIR, 'Missing a temporary folder'],
+			[UPLOAD_ERR_CANT_WRITE, 'Failed to write file to disk.'],
+			[UPLOAD_ERR_EXTENSION, 'A PHP extension stopped the file upload.'],
+		];
+	}
+
+	/**
+	 * @dataProvider dataPhpUploadErrors
+	 */
+	public function testUpdateLogoLoginScreenUploadWithInvalidImageUpload($error, $expectedErrorMessage) {
+		$this->request
+			->expects($this->at(0))
+			->method('getParam')
+			->with('backgroundColor')
+			->willReturn(false);
+		$this->request
+			->expects($this->at(1))
+			->method('getUploadedFile')
+			->with('uploadlogo')
+			->willReturn(null);
+		$this->request
+			->expects($this->at(2))
+			->method('getUploadedFile')
+			->with('upload-login-background')
+			->willReturn([
+				'tmp_name' => '',
+				'type' => 'text/svg',
+				'name' => 'logo.svg',
+				'error' => $error,
+			]);
+		$this->l10n
+			->expects($this->any())
+			->method('t')
+			->will($this->returnCallback(function($str) {
+				return $str;
+			}));
+
+		$expected = new DataResponse(
+			[
+				'data' =>
+					[
+						'message' => $expectedErrorMessage,
+					],
+				'status' => 'failure'
+			],
+			Http::STATUS_UNPROCESSABLE_ENTITY
+		);
+		$this->assertEquals($expected, $this->themingController->updateLogo());
+	}
+
+	/**
+	 * @dataProvider dataPhpUploadErrors
+	 */
+	public function testUpdateLogoUploadWithInvalidImageUpload($error, $expectedErrorMessage) {
+		$this->request
+			->expects($this->at(0))
+			->method('getParam')
+			->with('backgroundColor')
+			->willReturn(false);
+		$this->request
+			->expects($this->at(1))
+			->method('getUploadedFile')
+			->with('uploadlogo')
+			->willReturn([
+				'tmp_name' => '',
+				'type' => 'text/svg',
+				'name' => 'logo.svg',
+				'error' => $error,
+			]);
+		$this->request
+			->expects($this->at(2))
+			->method('getUploadedFile')
+			->with('upload-login-background')
+			->willReturn(null);
+		$this->l10n
+			->expects($this->any())
+			->method('t')
+			->will($this->returnCallback(function($str) {
+				return $str;
+			}));
+
+		$expected = new DataResponse(
+			[
+				'data' =>
+					[
+						'message' => $expectedErrorMessage
+					],
+				'status' => 'failure'
+			],
+			Http::STATUS_UNPROCESSABLE_ENTITY
+		);
+		$this->assertEquals($expected, $this->themingController->updateLogo());
+	}
+
 	public function testUndo() {
 		$this->l10n
 			->expects($this->once())