diff --git a/cypress/e2e/templates.spec.js b/cypress/e2e/templates.spec.js index 741195b7db..8d7c897a43 100644 --- a/cypress/e2e/templates.spec.js +++ b/cypress/e2e/templates.spec.js @@ -58,6 +58,62 @@ describe('Global templates', function() { }) }) + it('Surfaces the reason the server rejected the upload', function() { + cy.intercept('POST', '**/richdocuments/template', { + statusCode: 400, + body: { data: { message: 'File is too big' } }, + }).as('templateUploadRequest') + + cy.uploadSystemTemplate({ + fixturePath: 'templates/presentation.otp', + fileName: 'errortemplate.otp', + mimeType: 'application/vnd.oasis.opendocument.presentation-template', + }) + + cy.wait('@templateUploadRequest') + cy.contains('[role="alert"]', 'File is too big').should('be.visible') + cy.get('.template-btn[data-cy-template-btn-name="errortemplate"]').should('not.exist') + }) + + it('Reports a failure even when the error response carries no message', function() { + cy.intercept('POST', '**/richdocuments/template', { + statusCode: 500, + body: '', + }).as('templateUploadRequest') + + cy.uploadSystemTemplate({ + fixturePath: 'templates/presentation.otp', + fileName: 'errortemplate.otp', + mimeType: 'application/vnd.oasis.opendocument.presentation-template', + }) + + cy.wait('@templateUploadRequest') + cy.contains('[role="alert"]', 'Failed to upload the file').should('be.visible') + cy.get('.template-btn[data-cy-template-btn-name="errortemplate"]').should('not.exist') + }) + + it('Can be picked a second time after its upload failed', function() { + const template = { + fixturePath: 'templates/presentation.otp', + fileName: 'errortemplate.otp', + mimeType: 'application/vnd.oasis.opendocument.presentation-template', + } + + cy.intercept('POST', '**/richdocuments/template', { + statusCode: 400, + body: { data: { message: 'Failed to upload the file' } }, + }).as('templateUploadRequest') + + cy.uploadSystemTemplate(template) + cy.wait('@templateUploadRequest') + + // Picking the same file only fires a change event if the input was reset + cy.selectSystemTemplate(template) + cy.wait('@templateUploadRequest') + + cy.get('@templateUploadRequest.all').should('have.length', 2) + }) + it('Can be created by a user', () => { cy.uploadSystemTemplate({ fixturePath: 'templates/presentation.otp', diff --git a/cypress/support/commands.js b/cypress/support/commands.js index 6630aab64a..893d6ca7ed 100644 --- a/cypress/support/commands.js +++ b/cypress/support/commands.js @@ -368,6 +368,14 @@ Cypress.Commands.add('verifyOpen', (filename) => { }) }) +Cypress.Commands.add('selectSystemTemplate', ({ fixturePath, fileName, mimeType }) => { + cy.get('.settings-section input[type="file"]').selectFile({ + contents: `cypress/fixtures/${fixturePath}`, + fileName, + mimeType, + }, { force: true }) +}) + Cypress.Commands.add('uploadSystemTemplate', ({ fixturePath, fileName, mimeType }) => { cy.login(new User('admin', 'admin')) cy.visit('/settings/admin/richdocuments') @@ -376,11 +384,7 @@ Cypress.Commands.add('uploadSystemTemplate', ({ fixturePath, fileName, mimeType .contains('Global Templates') .scrollIntoView() - cy.get('.settings-section input[type="file"]').selectFile({ - contents: `cypress/fixtures/${fixturePath}`, - fileName, - mimeType, - }, { force: true }) + cy.selectSystemTemplate({ fixturePath, fileName, mimeType }) }) Cypress.Commands.add('submitTemplateFields', (fields) => { diff --git a/lib/Controller/TemplatesController.php b/lib/Controller/TemplatesController.php index 6d90f489f2..e19ca4c464 100644 --- a/lib/Controller/TemplatesController.php +++ b/lib/Controller/TemplatesController.php @@ -101,17 +101,23 @@ public function add() { $files = $this->request->getUploadedFile('files'); if (!is_null($files)) { - $mimeType = !empty($files['type'] ?? '') ? $files['type'] : $this->mimeTypeDetector->detect($files['tmp_name']); $error = $files['error'] ?? 0; if ($error !== 0) { $this->logger->error('Failed to get the uploaded file. PHP file upload error code: ' . $error); + $message = match ($error) { + UPLOAD_ERR_INI_SIZE, UPLOAD_ERR_FORM_SIZE => $this->l10n->t('File is too big'), + UPLOAD_ERR_NO_FILE => $this->l10n->t('No file was uploaded'), + default => $this->l10n->t('Failed to upload the file'), + }; return new JSONResponse( - ['data' => ['message' => $this->l10n->t('Failed to upload the file')]], + ['data' => ['message' => $message]], Http::STATUS_BAD_REQUEST ); } + $mimeType = !empty($files['type'] ?? '') ? $files['type'] : $this->mimeTypeDetector->detect($files['tmp_name']); + if (is_uploaded_file($files['tmp_name']) && !Filesystem::isFileBlacklisted($files['tmp_name'])) { if ($files['size'] > $this->maxSize) { return new JSONResponse( diff --git a/src/components/AdminSettings/GlobalTemplates.vue b/src/components/AdminSettings/GlobalTemplates.vue index 1efb85e68d..595605d9a2 100644 --- a/src/components/AdminSettings/GlobalTemplates.vue +++ b/src/components/AdminSettings/GlobalTemplates.vue @@ -112,17 +112,32 @@ export default { async selectFile() { const selectedFile = this.$refs.newTemplateInput?.files[0] - const templateAlreadyExists = this.existingTemplates.some((template) => { - return template.name === selectedFile.name - }) - if (!templateAlreadyExists) { + if (!selectedFile) { + return + } + + try { + const templateAlreadyExists = this.existingTemplates.some((template) => { + return template.name === selectedFile.name + }) + + if (templateAlreadyExists) { + showError(t('richdocuments', 'Template "{name}" already exists', { name: selectedFile.name })) + return + } + const template = await this.uploadTemplate(selectedFile) + if (template === null) { + return + } + this.existingTemplates.push(template) showSuccess(t('richdocuments', 'Uploaded template "{name}"', { name: template.name })) - } else { - showError(t('richdocuments', 'Template "{name}" already exists', { name: selectedFile.name })) + } finally { + // Allow selecting the same file again after a failed upload + this.$refs.newTemplateInput.value = '' } }, @@ -132,18 +147,20 @@ export default { formData.append('files', file) - let res = null try { - res = await axios.post(url, formData, { + const res = await axios.post(url, formData, { headers: { 'Content-Type': 'multipart/form-data', }, }) + + return res.data.data } catch (error) { - showError(error.response.data.data.message) - } + showError(error.response?.data?.data?.message + ?? t('richdocuments', 'Failed to upload the file')) - return res.data.data + return null + } }, async deleteTemplate(templateId) { diff --git a/tests/lib/Controller/TemplatesControllerTest.php b/tests/lib/Controller/TemplatesControllerTest.php new file mode 100644 index 0000000000..0f150fd604 --- /dev/null +++ b/tests/lib/Controller/TemplatesControllerTest.php @@ -0,0 +1,137 @@ +request = $this->createMock(IRequest::class); + $this->l10n = $this->createMock(IL10N::class); + $this->manager = $this->createMock(TemplateManager::class); + $this->mimeTypeDetector = $this->createMock(IMimeTypeDetector::class); + $this->logger = $this->createMock(LoggerInterface::class); + + $this->l10n->method('t')->willReturnArgument(0); + } + + private function makeController(): TemplatesController { + return new TemplatesController( + 'richdocuments', + $this->request, + $this->l10n, + $this->manager, + $this->createMock(IPreview::class), + $this->mimeTypeDetector, + $this->logger, + ); + } + + private function givenUploadedFile(?array $file): void { + $this->request->method('getUploadedFile')->with('files')->willReturn($file); + } + + private function getMessage(Http\Response $response): ?string { + return $response->getData()['data']['message'] ?? null; + } + + public static function uploadErrorProvider(): array { + return [ + 'exceeds upload_max_filesize' => [UPLOAD_ERR_INI_SIZE, 'File is too big'], + 'exceeds MAX_FILE_SIZE' => [UPLOAD_ERR_FORM_SIZE, 'File is too big'], + 'no file submitted' => [UPLOAD_ERR_NO_FILE, 'No file was uploaded'], + 'partial upload' => [UPLOAD_ERR_PARTIAL, 'Failed to upload the file'], + 'missing temp dir' => [UPLOAD_ERR_NO_TMP_DIR, 'Failed to upload the file'], + 'write failed' => [UPLOAD_ERR_CANT_WRITE, 'Failed to upload the file'], + 'stopped by extension' => [UPLOAD_ERR_EXTENSION, 'Failed to upload the file'], + ]; + } + + /** + * @dataProvider uploadErrorProvider + */ + public function testAddReportsUploadErrors(int $error, string $expectedMessage): void { + $this->givenUploadedFile([ + 'name' => 'template.otp', + 'type' => '', + 'tmp_name' => '', + 'error' => $error, + 'size' => 0, + ]); + $this->logger->expects($this->once())->method('error'); + + $response = $this->makeController()->add(); + + $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); + $this->assertSame($expectedMessage, $this->getMessage($response)); + } + + /** + * A failed upload leaves tmp_name empty, and detecting the mime type of an empty path + * raises a ValueError that surfaces as a 500 instead of the upload error. + * + * @dataProvider uploadErrorProvider + */ + public function testAddDoesNotDetectMimeTypeOfAFailedUpload(int $error): void { + $this->givenUploadedFile([ + 'name' => 'template.otp', + 'type' => '', + 'tmp_name' => '', + 'error' => $error, + 'size' => 0, + ]); + $this->mimeTypeDetector->expects($this->never())->method('detect'); + + $this->makeController()->add(); + } + + public function testAddRejectsAMissingUpload(): void { + $this->givenUploadedFile(null); + $this->mimeTypeDetector->expects($this->never())->method('detect'); + $this->manager->expects($this->never())->method('add'); + + $response = $this->makeController()->add(); + + $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); + $this->assertSame('Invalid file provided', $this->getMessage($response)); + } + + public function testAddRejectsAFileThatWasNotUploaded(): void { + $this->givenUploadedFile([ + 'name' => 'template.otp', + 'type' => 'application/vnd.oasis.opendocument.presentation-template', + 'tmp_name' => '/etc/passwd', + 'error' => UPLOAD_ERR_OK, + 'size' => 1024, + ]); + $this->manager->expects($this->never())->method('add'); + + $response = $this->makeController()->add(); + + $this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus()); + $this->assertSame('Invalid file provided', $this->getMessage($response)); + } +}