From f8aa304c7d8379c0566127d1ad169bccb39e1c7e Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Thu, 23 Jul 2020 14:49:29 -0400 Subject: [PATCH] REFACTOR: Remove `Discourse.SiteSettings` from uploads.js This involves passing the siteSettings around, which is somewhat error prone so I tried to be careful. --- .../app/components/composer-editor.js | 6 +- .../app/controllers/avatar-selector.js | 2 +- .../discourse/app/controllers/composer.js | 7 +- .../app/controllers/upload-selector.js | 25 ++-- .../javascripts/discourse/app/lib/uploads.js | 121 +++++++++--------- .../discourse/app/mixins/upload.js | 6 +- test/javascripts/lib/uploads-test.js | 108 +++++++++++----- 7 files changed, 172 insertions(+), 103 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/composer-editor.js b/app/assets/javascripts/discourse/app/components/composer-editor.js index eb2205e20e3..46dbaddbe81 100644 --- a/app/assets/javascripts/discourse/app/components/composer-editor.js +++ b/app/assets/javascripts/discourse/app/components/composer-editor.js @@ -81,7 +81,10 @@ export default Component.extend({ if (requiredCategoryMissing) { return "composer.reply_placeholder_choose_category"; } else { - const key = authorizesOneOrMoreImageExtensions(this.currentUser.staff) + const key = authorizesOneOrMoreImageExtensions( + this.currentUser.staff, + this.siteSettings + ) ? "reply_placeholder" : "reply_placeholder_no_images"; return `composer.${key}`; @@ -700,6 +703,7 @@ export default Component.extend({ const opts = { user: this.currentUser, + siteSettings: this.siteSettings, isPrivateMessage, allowStaffToUploadAnyFileInPm: this.siteSettings .allow_staff_to_upload_any_file_in_pm diff --git a/app/assets/javascripts/discourse/app/controllers/avatar-selector.js b/app/assets/javascripts/discourse/app/controllers/avatar-selector.js index 38d5da9a697..e3b995f1f26 100644 --- a/app/assets/javascripts/discourse/app/controllers/avatar-selector.js +++ b/app/assets/javascripts/discourse/app/controllers/avatar-selector.js @@ -49,7 +49,7 @@ export default Controller.extend(ModalFunctionality, { allowAvatarUpload() { return ( this.siteSettings.allow_uploaded_avatars && - allowsImages(this.currentUser.staff) + allowsImages(this.currentUser.staff, this.siteSettings) ); }, diff --git a/app/assets/javascripts/discourse/app/controllers/composer.js b/app/assets/javascripts/discourse/app/controllers/composer.js index 42242dc83cd..62b79702fc6 100644 --- a/app/assets/javascripts/discourse/app/controllers/composer.js +++ b/app/assets/javascripts/discourse/app/controllers/composer.js @@ -323,12 +323,15 @@ export default Controller.extend({ @discourseComputed allowUpload() { - return authorizesOneOrMoreExtensions(this.currentUser.staff); + return authorizesOneOrMoreExtensions( + this.currentUser.staff, + this.siteSettings + ); }, @discourseComputed() uploadIcon() { - return uploadIcon(this.currentUser.staff); + return uploadIcon(this.currentUser.staff, this.siteSettings); }, @action diff --git a/app/assets/javascripts/discourse/app/controllers/upload-selector.js b/app/assets/javascripts/discourse/app/controllers/upload-selector.js index 0c9b1d139d9..32e834aaa7e 100644 --- a/app/assets/javascripts/discourse/app/controllers/upload-selector.js +++ b/app/assets/javascripts/discourse/app/controllers/upload-selector.js @@ -10,37 +10,38 @@ import { uploadIcon } from "discourse/lib/uploads"; -function uploadTranslate(key, user) { - if (allowsAttachments(user.staff)) { - key += "_with_attachments"; - } - return `upload_selector.${key}`; -} - export default Controller.extend(ModalFunctionality, { imageUrl: null, local: equal("selection", "local"), remote: equal("selection", "remote"), selection: "local", + uploadTranslate(key) { + if (allowsAttachments(this.currentUser.staff, this.siteSettings)) { + key += "_with_attachments"; + } + return `upload_selector.${key}`; + }, + @discourseComputed() uploadIcon() { - return uploadIcon(this.currentUser.staff); + return uploadIcon(this.currentUser.staff, this.siteSettings); }, @discourseComputed() title() { - return uploadTranslate("title", this.currentUser); + return this.uploadTranslate("title"); }, @discourseComputed("selection") tip(selection) { const authorized_extensions = authorizesAllExtensions( - this.currentUser.staff + this.currentUser.staff, + this.siteSettings ) ? "" - : `(${authorizedExtensions(this.currentUser.staff)})`; - return I18n.t(uploadTranslate(`${selection}_tip`, this.currentUser), { + : `(${authorizedExtensions(this.currentUser.staff, this.siteSettings)})`; + return I18n.t(this.uploadTranslate(`${selection}_tip`), { authorized_extensions }); }, diff --git a/app/assets/javascripts/discourse/app/lib/uploads.js b/app/assets/javascripts/discourse/app/lib/uploads.js index 9ceafaf0a0f..84738928248 100644 --- a/app/assets/javascripts/discourse/app/lib/uploads.js +++ b/app/assets/javascripts/discourse/app/lib/uploads.js @@ -53,7 +53,7 @@ function validateUploadedFile(file, opts) { let user = opts.user; let staff = user && user.staff; - if (!authorizesOneOrMoreExtensions(staff)) return false; + if (!authorizesOneOrMoreExtensions(staff, opts.siteSettings)) return false; const name = file && file.name; @@ -69,10 +69,13 @@ function validateUploadedFile(file, opts) { } if (opts.imagesOnly) { - if (!isImage(name) && !isAuthorizedImage(name, staff)) { + if (!isImage(name) && !isAuthorizedImage(name, staff, opts.siteSettings)) { bootbox.alert( I18n.t("post.errors.upload_not_authorized", { - authorized_extensions: authorizedImagesExtensions(staff) + authorized_extensions: authorizedImagesExtensions( + staff, + opts.siteSettings + ) }) ); return false; @@ -83,10 +86,13 @@ function validateUploadedFile(file, opts) { return false; } } else { - if (!authorizesAllExtensions(staff) && !isAuthorizedFile(name, staff)) { + if ( + !authorizesAllExtensions(staff, opts.siteSettings) && + !isAuthorizedFile(name, staff, opts.siteSettings) + ) { bootbox.alert( I18n.t("post.errors.upload_not_authorized", { - authorized_extensions: authorizedExtensions(staff) + authorized_extensions: authorizedExtensions(staff, opts.siteSettings) }) ); return false; @@ -117,20 +123,20 @@ function extensionsToArray(exts) { .filter(ext => ext.indexOf("*") === -1); } -function extensions() { - return extensionsToArray(Discourse.SiteSettings.authorized_extensions); +function extensions(siteSettings) { + return extensionsToArray(siteSettings.authorized_extensions); } -function staffExtensions() { - return extensionsToArray( - Discourse.SiteSettings.authorized_extensions_for_staff +function staffExtensions(siteSettings) { + return extensionsToArray(siteSettings.authorized_extensions_for_staff); +} + +function imagesExtensions(staff, siteSettings) { + let exts = extensions(siteSettings).filter(ext => + IMAGES_EXTENSIONS_REGEX.test(ext) ); -} - -function imagesExtensions(staff) { - let exts = extensions().filter(ext => IMAGES_EXTENSIONS_REGEX.test(ext)); if (staff) { - const staffExts = staffExtensions().filter(ext => + const staffExts = staffExtensions(siteSettings).filter(ext => IMAGES_EXTENSIONS_REGEX.test(ext) ); exts = _.union(exts, staffExts); @@ -138,60 +144,61 @@ function imagesExtensions(staff) { return exts; } -function extensionsRegex() { - return new RegExp("\\.(" + extensions().join("|") + ")$", "i"); -} - -function imagesExtensionsRegex(staff) { - return new RegExp("\\.(" + imagesExtensions(staff).join("|") + ")$", "i"); -} - -function staffExtensionsRegex() { - return new RegExp("\\.(" + staffExtensions().join("|") + ")$", "i"); -} - -function isAuthorizedFile(fileName, staff) { - if (staff && staffExtensionsRegex().test(fileName)) { +function isAuthorizedFile(fileName, staff, siteSettings) { + if ( + staff && + new RegExp( + "\\.(" + staffExtensions(siteSettings).join("|") + ")$", + "i" + ).test(fileName) + ) { return true; } - return extensionsRegex().test(fileName); + + return new RegExp( + "\\.(" + extensions(siteSettings).join("|") + ")$", + "i" + ).test(fileName); } -function isAuthorizedImage(fileName, staff) { - return imagesExtensionsRegex(staff).test(fileName); +function isAuthorizedImage(fileName, staff, siteSettings) { + return new RegExp( + "\\.(" + imagesExtensions(staff, siteSettings).join("|") + ")$", + "i" + ); } -export function authorizedExtensions(staff) { - const exts = staff ? [...extensions(), ...staffExtensions()] : extensions(); +export function authorizedExtensions(staff, siteSettings) { + const exts = staff + ? [...extensions(siteSettings), ...staffExtensions(siteSettings)] + : extensions(siteSettings); return exts.filter(ext => ext.length > 0).join(", "); } -function authorizedImagesExtensions(staff) { - return authorizesAllExtensions(staff) +function authorizedImagesExtensions(staff, siteSettings) { + return authorizesAllExtensions(staff, siteSettings) ? "png, jpg, jpeg, gif, svg, ico" - : imagesExtensions(staff).join(", "); + : imagesExtensions(staff, siteSettings).join(", "); } -export function authorizesAllExtensions(staff) { +export function authorizesAllExtensions(staff, siteSettings) { return ( - Discourse.SiteSettings.authorized_extensions.indexOf("*") >= 0 || - (Discourse.SiteSettings.authorized_extensions_for_staff.indexOf("*") >= 0 && - staff) + siteSettings.authorized_extensions.indexOf("*") >= 0 || + (siteSettings.authorized_extensions_for_staff.indexOf("*") >= 0 && staff) ); } -export function authorizesOneOrMoreExtensions(staff) { - if (authorizesAllExtensions(staff)) return true; +export function authorizesOneOrMoreExtensions(staff, siteSettings) { + if (authorizesAllExtensions(staff, siteSettings)) return true; return ( - Discourse.SiteSettings.authorized_extensions.split("|").filter(ext => ext) - .length > 0 + siteSettings.authorized_extensions.split("|").filter(ext => ext).length > 0 ); } -export function authorizesOneOrMoreImageExtensions(staff) { - if (authorizesAllExtensions(staff)) return true; - return imagesExtensions(staff).length > 0; +export function authorizesOneOrMoreImageExtensions(staff, siteSettings) { + if (authorizesAllExtensions(staff, siteSettings)) return true; + return imagesExtensions(staff, siteSettings).length > 0; } export function isImage(path) { @@ -210,23 +217,23 @@ function uploadTypeFromFileName(fileName) { return isImage(fileName) ? "image" : "attachment"; } -export function allowsImages(staff) { +export function allowsImages(staff, siteSettings) { return ( - authorizesAllExtensions(staff) || - IMAGES_EXTENSIONS_REGEX.test(authorizedExtensions(staff)) + authorizesAllExtensions(staff, siteSettings) || + IMAGES_EXTENSIONS_REGEX.test(authorizedExtensions(staff, siteSettings)) ); } -export function allowsAttachments(staff) { +export function allowsAttachments(staff, siteSettings) { return ( - authorizesAllExtensions(staff) || - authorizedExtensions(staff).split(", ").length > - imagesExtensions(staff).length + authorizesAllExtensions(staff, siteSettings) || + authorizedExtensions(staff, siteSettings).split(", ").length > + imagesExtensions(staff, siteSettings).length ); } -export function uploadIcon(staff) { - return allowsAttachments(staff) ? "upload" : "far-image"; +export function uploadIcon(staff, siteSettings) { + return allowsAttachments(staff, siteSettings) ? "upload" : "far-image"; } function imageMarkdown(upload) { diff --git a/app/assets/javascripts/discourse/app/mixins/upload.js b/app/assets/javascripts/discourse/app/mixins/upload.js index b7394663a99..1c6b026ab70 100644 --- a/app/assets/javascripts/discourse/app/mixins/upload.js +++ b/app/assets/javascripts/discourse/app/mixins/upload.js @@ -82,7 +82,11 @@ export default Mixin.create({ $upload.on("fileuploadsubmit", (e, data) => { const opts = _.merge( - { bypassNewUserRestriction: true, user: this.currentUser }, + { + bypassNewUserRestriction: true, + user: this.currentUser, + siteSettings: this.siteSettings + }, this.validateUploadedFilesOptions() ); const isValid = validateUploadedFiles(data.files, opts); diff --git a/test/javascripts/lib/uploads-test.js b/test/javascripts/lib/uploads-test.js index 72a725d9562..16063e840c1 100644 --- a/test/javascripts/lib/uploads-test.js +++ b/test/javascripts/lib/uploads-test.js @@ -13,18 +13,27 @@ import { discourseModule } from "helpers/qunit-helpers"; discourseModule("lib:uploads"); -const validUpload = validateUploadedFiles; - -QUnit.test("validateUploadedFiles", assert => { - assert.not(validUpload(null), "no files are invalid"); - assert.not(validUpload(undefined), "undefined files are invalid"); - assert.not(validUpload([]), "empty array of files is invalid"); +QUnit.test("validateUploadedFiles", function(assert) { + assert.not( + validateUploadedFiles(null, { siteSettings: this.siteSettings }), + "no files are invalid" + ); + assert.not( + validateUploadedFiles(undefined, { siteSettings: this.siteSettings }), + "undefined files are invalid" + ); + assert.not( + validateUploadedFiles([], { siteSettings: this.siteSettings }), + "empty array of files is invalid" + ); }); -QUnit.test("uploading one file", assert => { +QUnit.test("uploading one file", function(assert) { sandbox.stub(bootbox, "alert"); - assert.not(validUpload([1, 2])); + assert.not( + validateUploadedFiles([1, 2], { siteSettings: this.siteSettings }) + ); assert.ok(bootbox.alert.calledWith(I18n.t("post.errors.too_many_uploads"))); }); @@ -33,7 +42,10 @@ QUnit.test("new user cannot upload images", function(assert) { sandbox.stub(bootbox, "alert"); assert.not( - validUpload([{ name: "image.png" }], { user: User.create() }), + validateUploadedFiles([{ name: "image.png" }], { + user: User.create(), + siteSettings: this.siteSettings + }), "the upload is not valid" ); assert.ok( @@ -48,7 +60,12 @@ QUnit.test("new user cannot upload attachments", function(assert) { this.siteSettings.newuser_max_attachments = 0; sandbox.stub(bootbox, "alert"); - assert.not(validUpload([{ name: "roman.txt" }], { user: User.create() })); + assert.not( + validateUploadedFiles([{ name: "roman.txt" }], { + user: User.create(), + siteSettings: this.siteSettings + }) + ); assert.ok( bootbox.alert.calledWith( I18n.t("post.errors.attachment_upload_not_allowed_for_new_user") @@ -56,24 +73,38 @@ QUnit.test("new user cannot upload attachments", function(assert) { ); }); -QUnit.test("ensures an authorized upload", assert => { +QUnit.test("ensures an authorized upload", function(assert) { sandbox.stub(bootbox, "alert"); - assert.not(validUpload([{ name: "unauthorized.html" }])); + assert.not( + validateUploadedFiles([{ name: "unauthorized.html" }], { + siteSettings: this.siteSettings + }) + ); assert.ok( bootbox.alert.calledWith( I18n.t("post.errors.upload_not_authorized", { - authorized_extensions: authorizedExtensions() + authorized_extensions: authorizedExtensions(false, this.siteSettings) }) ) ); }); -QUnit.test("skipping validation works", assert => { +QUnit.test("skipping validation works", function(assert) { const files = [{ name: "backup.tar.gz" }]; sandbox.stub(bootbox, "alert"); - assert.not(validUpload(files, { skipValidation: false })); - assert.ok(validUpload(files, { skipValidation: true })); + assert.not( + validateUploadedFiles(files, { + skipValidation: false, + siteSettings: this.siteSettings + }) + ); + assert.ok( + validateUploadedFiles(files, { + skipValidation: true, + siteSettings: this.siteSettings + }) + ); }); QUnit.test("staff can upload anything in PM", function(assert) { @@ -82,11 +113,14 @@ QUnit.test("staff can upload anything in PM", function(assert) { sandbox.stub(bootbox, "alert"); let user = User.create({ moderator: true }); - assert.not(validUpload(files, { user })); + assert.not( + validateUploadedFiles(files, { user, siteSettings: this.siteSettings }) + ); assert.ok( - validUpload(files, { + validateUploadedFiles(files, { isPrivateMessage: true, allowStaffToUploadAnyFileInPm: true, + siteSettings: this.siteSettings, user }) ); @@ -109,17 +143,24 @@ const dummyBlob = function() { } }; -QUnit.test("allows valid uploads to go through", assert => { +QUnit.test("allows valid uploads to go through", function(assert) { sandbox.stub(bootbox, "alert"); let user = User.create({ trust_level: 1 }); // image let image = { name: "image.png", size: imageSize }; - assert.ok(validUpload([image], { user })); + assert.ok( + validateUploadedFiles([image], { user, siteSettings: this.siteSettings }) + ); // pasted image let pastedImage = dummyBlob(); - assert.ok(validUpload([pastedImage], { user })); + assert.ok( + validateUploadedFiles([pastedImage], { + user, + siteSettings: this.siteSettings + }) + ); assert.not(bootbox.alert.calledOnce); }); @@ -140,42 +181,51 @@ QUnit.test("isImage", assert => { QUnit.test("allowsImages", function(assert) { this.siteSettings.authorized_extensions = "jpg|jpeg|gif"; - assert.ok(allowsImages(), "works"); + assert.ok(allowsImages(false, this.siteSettings), "works"); this.siteSettings.authorized_extensions = ".jpg|.jpeg|.gif"; - assert.ok(allowsImages(), "works with old extensions syntax"); + assert.ok( + allowsImages(false, this.siteSettings), + "works with old extensions syntax" + ); this.siteSettings.authorized_extensions = "txt|pdf|*"; assert.ok( - allowsImages(), + allowsImages(false, this.siteSettings), "images are allowed when all extensions are allowed" ); this.siteSettings.authorized_extensions = "json|jpg|pdf|txt"; assert.ok( - allowsImages(), + allowsImages(false, this.siteSettings), "images are allowed when at least one extension is an image extension" ); }); QUnit.test("allowsAttachments", function(assert) { this.siteSettings.authorized_extensions = "jpg|jpeg|gif"; - assert.not(allowsAttachments(), "no attachments allowed by default"); + assert.not( + allowsAttachments(false, this.siteSettings), + "no attachments allowed by default" + ); this.siteSettings.authorized_extensions = "jpg|jpeg|gif|*"; assert.ok( - allowsAttachments(), + allowsAttachments(false, this.siteSettings), "attachments are allowed when all extensions are allowed" ); this.siteSettings.authorized_extensions = "jpg|jpeg|gif|pdf"; assert.ok( - allowsAttachments(), + allowsAttachments(false, this.siteSettings), "attachments are allowed when at least one extension is not an image extension" ); this.siteSettings.authorized_extensions = ".jpg|.jpeg|.gif|.pdf"; - assert.ok(allowsAttachments(), "works with old extensions syntax"); + assert.ok( + allowsAttachments(false, this.siteSettings), + "works with old extensions syntax" + ); }); function testUploadMarkdown(filename, opts = {}) {