From fb9320afc1fc88dd16132195af54c07c4ed57280 Mon Sep 17 00:00:00 2001 From: Jim Date: Mon, 27 Oct 2014 12:38:46 -0700 Subject: [PATCH] Limit Upload File Sizes to GridFS. This commit puts a limit on the size of files that course staff can upload to MongoDB. The limit is enforced on the frontend in javascript as well as backend via the /upload endpoint. The limit is hard-coded in cms/envs/common.py and may be changed according to the user's custom needs. If the user tries to upload a file that's too large, an error message will pop up, with a customizable url that pointing the user to an external page with an alternate upload procedure. This url is specified im cms/envs/common.py. If not set, this url will not be displayed. --- AUTHORS | 3 +- cms/djangoapps/contentstore/views/assets.py | 31 +++++++ .../contentstore/views/tests/test_assets.py | 33 ++++++- cms/envs/common.py | 10 +++ cms/static/coffee/spec/main.coffee | 10 ++- cms/static/coffee/spec/main_squire.coffee | 10 ++- .../coffee/spec/views/assets_spec.coffee | 83 +++++++++++------- cms/static/js/factories/asset_index.js | 12 ++- cms/static/js/spec/views/assets_spec.js | 53 +++++++++++- cms/static/js/views/assets.js | 85 +++++++++++++++---- cms/static/js_test.yml | 4 + cms/static/js_test_squire.yml | 4 + cms/static/require-config.js | 10 ++- cms/templates/asset_index.html | 8 +- 14 files changed, 295 insertions(+), 61 deletions(-) diff --git a/AUTHORS b/AUTHORS index 9ebfdbca68..33d5742e4d 100644 --- a/AUTHORS +++ b/AUTHORS @@ -180,4 +180,5 @@ Eugeny Kolpakov Omar Al-Ithawi Louis Pilfold Akiva Leffert -Mike Bifulco \ No newline at end of file +Mike Bifulco +Jim Zheng diff --git a/cms/djangoapps/contentstore/views/assets.py b/cms/djangoapps/contentstore/views/assets.py index 5618807996..9bd098b9ec 100644 --- a/cms/djangoapps/contentstore/views/assets.py +++ b/cms/djangoapps/contentstore/views/assets.py @@ -83,6 +83,9 @@ def _asset_index(request, course_key): return render_to_response('asset_index.html', { 'context_course': course_module, + 'max_file_size_in_mbs': settings.MAX_ASSET_UPLOAD_FILE_SIZE_IN_MB, + 'chunk_size_in_mbs': settings.UPLOAD_CHUNK_SIZE_IN_MB, + 'max_file_size_redirect_url': settings.MAX_ASSET_UPLOAD_FILE_SIZE_URL, 'asset_callback_url': reverse_course_url('assets_handler', course_key) }) @@ -152,6 +155,14 @@ def _get_assets_for_page(request, course_key, current_page, page_size, sort): ) +def get_file_size(upload_file): + """ + Helper method for getting file size of an upload file. + Can be used for mocking test file sizes. + """ + return upload_file.size + + @require_POST @ensure_csrf_cookie @login_required @@ -176,6 +187,26 @@ def _upload_asset(request, course_key): upload_file = request.FILES['file'] filename = upload_file.name mime_type = upload_file.content_type + size = get_file_size(upload_file) + + # If file is greater than a specified size, reject the upload + # request and send a message to the user. Note that since + # the front-end may batch large file uploads in smaller chunks, + # we validate the file-size on the front-end in addition to + # validating on the backend. (see cms/static/js/views/assets.js) + max_file_size_in_bytes = settings.MAX_ASSET_UPLOAD_FILE_SIZE_IN_MB * 1000 ** 2 + if size > max_file_size_in_bytes: + return JsonResponse({ + 'error': _( + 'File {filename} exceeds maximum size of ' + '{size_mb} MB. Please follow the instructions here ' + 'to upload a file elsewhere and link to it instead: ' + '{faq_url}').format( + filename=filename, + size_mb=settings.MAX_ASSET_UPLOAD_FILE_SIZE_IN_MB, + faq_url=settings.MAX_ASSET_UPLOAD_FILE_SIZE_URL, + ) + }, status=413) content_loc = StaticContent.compute_location(course_key, filename) diff --git a/cms/djangoapps/contentstore/views/tests/test_assets.py b/cms/djangoapps/contentstore/views/tests/test_assets.py index fa31e14888..bf891ecf9a 100644 --- a/cms/djangoapps/contentstore/views/tests/test_assets.py +++ b/cms/djangoapps/contentstore/views/tests/test_assets.py @@ -5,6 +5,7 @@ from datetime import datetime from io import BytesIO from pytz import UTC import json +from django.conf import settings from contentstore.tests.utils import CourseTestCase from contentstore.views import assets from contentstore.utils import reverse_course_url @@ -16,10 +17,14 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.xml_importer import import_from_xml from django.test.utils import override_settings from opaque_keys.edx.locations import SlashSeparatedCourseKey, AssetLocation -from django.conf import settings +import mock +from ddt import ddt +from ddt import data TEST_DATA_DIR = settings.COMMON_TEST_DATA_ROOT +MAX_FILE_SIZE = settings.MAX_ASSET_UPLOAD_FILE_SIZE_IN_MB * 1000 ** 2 + class AssetsTestCase(CourseTestCase): """ @@ -33,9 +38,14 @@ class AssetsTestCase(CourseTestCase): """ Post to the asset upload url """ + f = self.get_sample_asset(name) + return self.client.post(self.url, {"name": name, "file": f}) + + def get_sample_asset(self, name): + """Returns an in-memory file with the given name for testing""" f = BytesIO(name) f.name = name + ".txt" - return self.client.post(self.url, {"name": name, "file": f}) + return f class BasicAssetsTestCase(AssetsTestCase): @@ -132,6 +142,7 @@ class PaginationTestCase(AssetsTestCase): self.assertGreaterEqual(name2, name3) +@ddt class UploadTestCase(AssetsTestCase): """ Unit tests for uploading a file @@ -148,6 +159,24 @@ class UploadTestCase(AssetsTestCase): resp = self.client.post(self.url, {"name": "file.txt"}, "application/json") self.assertEquals(resp.status_code, 400) + @data( + (int(MAX_FILE_SIZE / 2.0), "small.file.test", 200), + (MAX_FILE_SIZE, "justequals.file.test", 200), + (MAX_FILE_SIZE + 90, "large.file.test", 413), + ) + @mock.patch('contentstore.views.assets.get_file_size') + def test_file_size(self, case, get_file_size): + max_file_size, name, status_code = case + + get_file_size.return_value = max_file_size + + f = self.get_sample_asset(name=name) + resp = self.client.post(self.url, { + "name": name, + "file": f + }) + self.assertEquals(resp.status_code, status_code) + class DownloadTestCase(AssetsTestCase): """ diff --git a/cms/envs/common.py b/cms/envs/common.py index 9243f3b514..6951a08a43 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -718,6 +718,16 @@ ADVANCED_SECURITY_CONFIG = {} SHIBBOLETH_DOMAIN_PREFIX = 'shib:' OPENID_DOMAIN_PREFIX = 'openid:' +### Size of chunks into which asset uploads will be divided +UPLOAD_CHUNK_SIZE_IN_MB = 10 + +### Max size of asset uploads to GridFS +MAX_ASSET_UPLOAD_FILE_SIZE_IN_MB = 10 + +# FAQ url to direct users to if they upload +# a file that exceeds the above size +MAX_ASSET_UPLOAD_FILE_SIZE_URL = "" + ################ ADVANCED_COMPONENT_TYPES ############### ADVANCED_COMPONENT_TYPES = [ diff --git a/cms/static/coffee/spec/main.coffee b/cms/static/coffee/spec/main.coffee index 7807bd39e2..7ab87a4461 100644 --- a/cms/static/coffee/spec/main.coffee +++ b/cms/static/coffee/spec/main.coffee @@ -15,6 +15,8 @@ requirejs.config({ "jquery.cookie": "xmodule_js/common_static/js/vendor/jquery.cookie", "jquery.qtip": "xmodule_js/common_static/js/vendor/jquery.qtip.min", "jquery.fileupload": "xmodule_js/common_static/js/vendor/jQuery-File-Upload/js/jquery.fileupload", + "jquery.fileupload-process": "xmodule_js/common_static/js/vendor/jQuery-File-Upload/js/jquery.fileupload-process", + "jquery.fileupload-validate": "xmodule_js/common_static/js/vendor/jQuery-File-Upload/js/jquery.fileupload-validate", "jquery.iframe-transport": "xmodule_js/common_static/js/vendor/jQuery-File-Upload/js/jquery.iframe-transport", "jquery.inputnumber": "xmodule_js/common_static/js/vendor/html5-input-polyfills/number-polyfill", "jquery.immediateDescendents": "xmodule_js/common_static/coffee/src/jquery.immediateDescendents", @@ -94,9 +96,15 @@ requirejs.config({ exports: "jQuery.fn.qtip" }, "jquery.fileupload": { - deps: ["jquery.iframe-transport"], + deps: ["jquery.ui", "jquery.iframe-transport"], exports: "jQuery.fn.fileupload" }, + "jquery.fileupload-process": { + deps: ["jquery.fileupload"] + }, + "jquery.fileupload-validate": { + deps: ["jquery.fileupload"] + }, "jquery.inputnumber": { deps: ["jquery"], exports: "jQuery.fn.inputNumber" diff --git a/cms/static/coffee/spec/main_squire.coffee b/cms/static/coffee/spec/main_squire.coffee index 6cbd7a1e26..66669b374f 100644 --- a/cms/static/coffee/spec/main_squire.coffee +++ b/cms/static/coffee/spec/main_squire.coffee @@ -14,6 +14,8 @@ requirejs.config({ "jquery.cookie": "xmodule_js/common_static/js/vendor/jquery.cookie", "jquery.qtip": "xmodule_js/common_static/js/vendor/jquery.qtip.min", "jquery.fileupload": "xmodule_js/common_static/js/vendor/jQuery-File-Upload/js/jquery.fileupload", + "jquery.fileupload-process": "xmodule_js/common_static/js/vendor/jQuery-File-Upload/js/jquery.fileupload-process", + "jquery.fileupload-validate": "xmodule_js/common_static/js/vendor/jQuery-File-Upload/js/jquery.fileupload-validate", "jquery.iframe-transport": "xmodule_js/common_static/js/vendor/jQuery-File-Upload/js/jquery.iframe-transport", "jquery.inputnumber": "xmodule_js/common_static/js/vendor/html5-input-polyfills/number-polyfill", "jquery.immediateDescendents": "xmodule_js/common_static/coffee/src/jquery.immediateDescendents", @@ -84,9 +86,15 @@ requirejs.config({ exports: "jQuery.fn.qtip" }, "jquery.fileupload": { - deps: ["jquery.iframe-transport"], + deps: ["jquery.ui", "jquery.iframe-transport"], exports: "jQuery.fn.fileupload" }, + "jquery.fileupload-process": { + deps: ["jquery.fileupload"] + }, + "jquery.fileupload-validate": { + deps: ["jquery.fileupload"] + }, "jquery.inputnumber": { deps: ["jquery"], exports: "jQuery.fn.inputNumber" diff --git a/cms/static/coffee/spec/views/assets_spec.coffee b/cms/static/coffee/spec/views/assets_spec.coffee index ac893c86a2..7709580a73 100644 --- a/cms/static/coffee/spec/views/assets_spec.coffee +++ b/cms/static/coffee/spec/views/assets_spec.coffee @@ -48,9 +48,12 @@ define ["jquery", "jasmine", "js/common_helpers/ajax_helpers", "squire"], @collection = new AssetCollection([@model]) @collection.url = "assets-url" - @view = new AssetView({model: @model}) + @createAssetView = (test) => + view = new AssetView({model: @model}) + requests = if test then AjaxHelpers["requests"](test) else null + return {view: view, requests: requests} - waitsFor (=> @view), "AssetView was not created", 1000 + waitsFor (=> @createAssetView), "AssetsView Creation function was not initialized", 1000 afterEach -> @injector.clean() @@ -58,10 +61,12 @@ define ["jquery", "jasmine", "js/common_helpers/ajax_helpers", "squire"], describe "Basic", -> it "should render properly", -> + {view: @view, requests: requests} = @createAssetView() @view.render() expect(@view.$el).toContainText("test asset") it "should pop a delete confirmation when the delete button is clicked", -> + {view: @view, requests: requests} = @createAssetView() @view.render().$(".remove-asset-button").click() expect(@promptSpies.constructor).toHaveBeenCalled() ctorOptions = @promptSpies.constructor.mostRecentCall.args[0] @@ -72,7 +77,7 @@ define ["jquery", "jasmine", "js/common_helpers/ajax_helpers", "squire"], describe "AJAX", -> it "should destroy itself on confirmation", -> - requests = AjaxHelpers["requests"](this) + {view: @view, requests: requests} = @createAssetView(this) @view.render().$(".remove-asset-button").click() ctorOptions = @promptSpies.constructor.mostRecentCall.args[0] @@ -92,7 +97,7 @@ define ["jquery", "jasmine", "js/common_helpers/ajax_helpers", "squire"], expect(@collection.contains(@model)).toBeFalsy() it "should not destroy itself if server errors", -> - requests = AjaxHelpers["requests"](this) + {view: @view, requests: requests} = @createAssetView(this) @view.render().$(".remove-asset-button").click() ctorOptions = @promptSpies.constructor.mostRecentCall.args[0] @@ -106,7 +111,7 @@ define ["jquery", "jasmine", "js/common_helpers/ajax_helpers", "squire"], expect(@collection.contains(@model)).toBeTruthy() it "should lock the asset on confirmation", -> - requests = AjaxHelpers["requests"](this) + {view: @view, requests: requests} = @createAssetView(this) @view.render().$(".lock-checkbox").click() # AJAX request has been sent, but not yet returned @@ -123,7 +128,7 @@ define ["jquery", "jasmine", "js/common_helpers/ajax_helpers", "squire"], expect(@model.get("locked")).toBeTruthy() it "should not lock the asset if server errors", -> - requests = AjaxHelpers["requests"](this) + {view: @view, requests: requests} = @createAssetView(this) @view.render().$(".lock-checkbox").click() # return an error response @@ -138,6 +143,7 @@ define ["jquery", "jasmine", "js/common_helpers/ajax_helpers", "squire"], appendSetFixtures($("