Merge pull request #1675 from sharelatex/spd-zip-file-limit-ui

Display more descriptive errors when uploading a project fails

GitOrigin-RevId: e206ca0d595f927ab141fb901644906f000160f8

services/web/app/coffee/Features/Project/ProjectController.coffee

@@ -239,6 +239,7 @@ module.exports = ProjectController =
 						hasSubscription: results.hasSubscription
 						isShowingV1Projects: results.v1Projects?
 						warnings: warnings
+						zipFileSizeLimit: Settings.maxUploadSize
 					}
 
 					if Settings?.algolia?.app_id? and Settings?.algolia?.read_only_api_key?

services/web/app/coffee/Features/Uploads/ArchiveManager.coffee

@@ -5,6 +5,7 @@ Path    = require "path"
 fse     = require "fs-extra"
 yauzl   = require "yauzl"
 Settings = require "settings-sharelatex"
+Errors = require "../Errors/Errors"
 _ = require("underscore")
 
 ONE_MEG = 1024 * 1024
@@ -16,7 +17,7 @@ module.exports = ArchiveManager =
 
 		totalSizeInBytes = null
 		yauzl.open source, {lazyEntries: true}, (err, zipfile) ->
-			return callback(err) if err?
+			return callback(new Errors.InvalidError("invalid_zip_file")) if err?
 
 			if Settings.maxEntitiesPerProject? and zipfile.entryCount > Settings.maxEntitiesPerProject
 				return callback(null, true) # too many files in zip file
@@ -113,7 +114,7 @@ module.exports = ArchiveManager =
 				return callback(err)
 
 			if isTooLarge
-				return callback(new Error("zip_too_large"))
+				return callback(new Errors.InvalidError("zip_contents_too_large"))
 
 			timer = new metrics.Timer("unzipDirectory")
 			logger.log source: source, destination: destination, "unzipping file"

services/web/app/coffee/Features/Uploads/ProjectUploadController.coffee

@@ -5,6 +5,22 @@ Path    = require "path"
 FileSystemImportManager = require "./FileSystemImportManager"
 ProjectUploadManager    = require "./ProjectUploadManager"
 AuthenticationController = require('../Authentication/AuthenticationController')
+Settings = require "settings-sharelatex"
+Errors = require "../Errors/Errors"
+multer = require('multer')
+
+upload = null
+
+try
+	upload = multer(
+		dest: Settings.path.uploadFolder
+		limits: fileSize: Settings.maxUploadSize
+	)
+catch err
+	if err.message == "EEXIST"
+		logger.log uploadFolder:Settings.path.uploadFolder, "dir already exists, continuing"
+	else
+		logger.err err:err, "caught error from multer in uploads router"
 
 module.exports = ProjectUploadController =
 	uploadProject: (req, res, next) ->
@@ -19,7 +35,10 @@ module.exports = ProjectUploadController =
 				logger.error
 					err: error, file_path: path, file_name: name,
 					"error uploading project"
-				res.send success: false
+				if error.name? && error.name == 'InvalidError'
+					res.status(422).json { success: false, error: req.i18n.translate(error.message) }
+				else
+					res.status(500).json { success: false, error: req.i18n.translate("upload_failed") }
 			else
 				logger.log
 					project: project._id, file_path: path, file_name: name,
@@ -52,3 +71,11 @@ module.exports = ProjectUploadController =
 					project_id: project_id, file_path: path, file_name: name, folder_id: folder_id
 					"uploaded file"
 				res.send success: true, entity_id: entity?._id, entity_type: entity?.type
+
+	multerMiddleware: (req, res, next) ->
+		return res.status(500).json {success: false, error: req.i18n.translate("upload_failed")} unless upload?
+		upload.single('qqfile') req, res, (err) ->
+			if err instanceof multer.MulterError && err.code == 'LIMIT_FILE_SIZE'
+				return res.status(422).json {success: false, error: req.i18n.translate("file_too_large")}
+
+			next(err)

services/web/app/coffee/Features/Uploads/UploadsRouter.coffee

@@ -3,19 +3,6 @@ AuthenticationController = require('../Authentication/AuthenticationController')
 ProjectUploadController = require "./ProjectUploadController"
 RateLimiterMiddleware = require('../Security/RateLimiterMiddleware')
 Settings = require('settings-sharelatex')
-multer = require('multer')
-
-try 
-	upload = multer(
-		dest: Settings.path.uploadFolder
-		limits: fileSize: Settings.maxUploadSize
-	)
-catch err
-	if err.message == "EEXIST"
-		logger.log uploadFolder:Settings.path.uploadFolder, "dir already exists, continuing"
-	else
-		logger.err err:err, "caught error from multer in uploads router"
-
 
 module.exports =
 	apply: (webRouter, apiRouter) ->
@@ -26,7 +13,7 @@ module.exports =
 				maxRequests: 20
 				timeInterval: 60
 			}),
-			upload.single('qqfile'),
+			ProjectUploadController.multerMiddleware,
 			ProjectUploadController.uploadProject
 
 		webRouter.post '/Project/:Project_id/upload',
@@ -38,5 +25,5 @@ module.exports =
 			}),
 			AuthenticationController.requireLogin(),
 			AuthorizationMiddleware.ensureUserCanWriteProjectContent,
-			upload.single('qqfile'),
+			ProjectUploadController.multerMiddleware,
 			ProjectUploadController.uploadFile

services/web/app/views/project/list/modals.pug

@@ -256,6 +256,7 @@ script(type="text/ng-template", id="uploadProjectModalTemplate")
 		endpoint="/project/new/upload"
 		template-id="qq-project-uploader-template"
 		multiple="false"
+		size-limit=zipFileSizeLimit
 		allowed-extensions="['zip']"
 		on-complete-callback="onComplete"
 	)

services/web/public/src/directives/fineUpload.js

@@ -15,6 +15,7 @@ define(['base', 'fineuploader'], (App, qq) =>
       multiple: '=',
       endpoint: '@',
       templateId: '@',
+      sizeLimit: '@',
       allowedExtensions: '=',
       onCompleteCallback: '=',
       onUploadCallback: '=',
@@ -36,6 +37,9 @@ define(['base', 'fineuploader'], (App, qq) =>
       } else {
         validation = {}
       }
+      if (scope.sizeLimit) {
+        validation.sizeLimit = scope.sizeLimit
+      }
       const maxConnections = scope.maxConnections || 1
       const onComplete = scope.onCompleteCallback || function() {}
       const onUpload = scope.onUploadCallback || function() {}
@@ -72,7 +76,11 @@ define(['base', 'fineuploader'], (App, qq) =>
           onSubmit,
           onCancel
         },
-        template: templateId
+        template: templateId,
+        failedUploadTextDisplay: {
+          mode: 'custom',
+          responseProperty: 'error'
+        }
       })
       window.q = q
       if (scope.control != null) {

services/web/test/unit/coffee/Uploads/ArchiveManagerTests.coffee

@@ -3,6 +3,7 @@ expect = require("chai").expect
 chai = require('chai')
 should = chai.should()
 modulePath = "../../../../app/js/Features/Uploads/ArchiveManager.js"
+Errors = require("../../../../app/js/Features/Errors/Errors")
 SandboxedModule = require('sandboxed-module')
 events = require "events"
 
@@ -50,13 +51,13 @@ describe "ArchiveManager", ->
 
 		describe "with an error in the zip file header", ->
 			beforeEach (done) ->
-				@yauzl.open = sinon.stub().callsArgWith(2, new Error("Something went wrong"))
+				@yauzl.open = sinon.stub().callsArgWith(2, new Errors.InvalidError("invalid_zip_file"))
 				@ArchiveManager.extractZipArchive @source, @destination, (error) =>
 					@callback(error)
 					done()
 
 			it "should return the callback with an error", ->
-				@callback.calledWithExactly(new Error("Something went wrong")).should.equal true
+				sinon.assert.calledWithExactly(@callback, new Errors.InvalidError("invalid_zip_file"))
 
 			it "should log out the error", ->
 				@logger.error.called.should.equal true
@@ -69,7 +70,7 @@ describe "ArchiveManager", ->
 					done()
 
 			it "should return the callback with an error", ->
-				@callback.calledWithExactly(new Error("zip_too_large")).should.equal true
+				sinon.assert.calledWithExactly(@callback, new Errors.InvalidError("zip_contents_too_large"))
 
 			it "should not call yauzl.open", ->
 				@yauzl.open.called.should.equal false

services/web/test/unit/coffee/Uploads/ProjectUploadControllerTests.coffee

@@ -6,6 +6,7 @@ modulePath = "../../../../app/js/Features/Uploads/ProjectUploadController.js"
 SandboxedModule = require('sandboxed-module')
 MockRequest = require "../helpers/MockRequest"
 MockResponse = require "../helpers/MockResponse"
+Errors = require("../../../../app/js/Features/Errors/Errors")
 
 describe "ProjectUploadController", ->
 	beforeEach ->
@@ -88,8 +89,24 @@ describe "ProjectUploadController", ->
 				@ProjectUploadController.uploadProject @req, @res
 
 			it "should return a failed response to the FileUploader client", ->
-				expect(@res.body).to.deep.equal
-					success: false
+				expect(@res.body).to.deep.equal JSON.stringify({ success: false, error: "upload_failed" })
+
+			it "should output an error log line", ->
+				@logger.error
+					.calledWith(sinon.match.any, "error uploading project")
+					.should.equal true
+
+		describe "when ProjectUploadManager.createProjectFromZipArchive reports the file as invalid", ->
+			beforeEach ->
+				@ProjectUploadManager.createProjectFromZipArchive =
+					sinon.stub().callsArgWith(3, new Errors.InvalidError("zip_contents_too_large"), @project)
+				@ProjectUploadController.uploadProject @req, @res
+
+			it "should return the reported error to the FileUploader client", ->
+				expect(@res.body).to.deep.equal JSON.stringify({ success: false, error: "zip_contents_too_large" })
+
+			it "should return an 'unprocessable entity' status code", ->
+				expect(@res.statusCode).to.equal 422
 
 			it "should output an error log line", ->
 				@logger.error

services/web/test/unit/coffee/helpers/MockRequest.coffee

@@ -8,7 +8,7 @@ class MockRequest
 	body: {}
 	_parsedUrl:{}
 	i18n:
-		translate:->
+		translate: (str)-> str
 	route:
 		path: ''
 

services/web/test/unit/coffee/helpers/MockResponse.coffee

@@ -53,7 +53,7 @@ class MockResponse
 		if arguments.length < 2
 			if typeof status != "number"
 				body = status
-				status = 200
+				status = @statusCode || 200
 		@statusCode = status
 		@returned = true
 		@type = 'application/json'
@@ -64,7 +64,8 @@ class MockResponse
 		@body = JSON.stringify(body) if body
 		@callback() if @callback?
 
-	status: (@statusCode)->
+	status: (status)->
+		@statusCode = status
 		return @