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/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

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)
     );
   },
 

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

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)})`;
+      : `(${authorizedExtensions(this.currentUser.staff, this.siteSettings)})`;
-    return I18n.t(uploadTranslate(`${selection}_tip`, this.currentUser), {
+    return I18n.t(this.uploadTranslate(`${selection}_tip`), {
       authorized_extensions
     });
   },

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() {
+function extensions(siteSettings) {
-  return extensionsToArray(Discourse.SiteSettings.authorized_extensions);
+  return extensionsToArray(siteSettings.authorized_extensions);
 }
 
-function staffExtensions() {
+function staffExtensions(siteSettings) {
-  return extensionsToArray(
+  return extensionsToArray(siteSettings.authorized_extensions_for_staff);
-    Discourse.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() {
+function isAuthorizedFile(fileName, staff, siteSettings) {
-  return new RegExp("\\.(" + extensions().join("|") + ")$", "i");
+  if (
-}
+    staff &&
-
+    new RegExp(
-function imagesExtensionsRegex(staff) {
+      "\\.(" + staffExtensions(siteSettings).join("|") + ")$",
-  return new RegExp("\\.(" + imagesExtensions(staff).join("|") + ")$", "i");
+      "i"
-}
+    ).test(fileName)
-
+  ) {
-function staffExtensionsRegex() {
-  return new RegExp("\\.(" + staffExtensions().join("|") + ")$", "i");
-}
-
-function isAuthorizedFile(fileName, staff) {
-  if (staff && staffExtensionsRegex().test(fileName)) {
     return true;
   }
-  return extensionsRegex().test(fileName);
+
+  return new RegExp(
+    "\\.(" + extensions(siteSettings).join("|") + ")$",
+    "i"
+  ).test(fileName);
 }
 
-function isAuthorizedImage(fileName, staff) {
+function isAuthorizedImage(fileName, staff, siteSettings) {
-  return imagesExtensionsRegex(staff).test(fileName);
+  return new RegExp(
+    "\\.(" + imagesExtensions(staff, siteSettings).join("|") + ")$",
+    "i"
+  );
 }
 
-export function authorizedExtensions(staff) {
+export function authorizedExtensions(staff, siteSettings) {
-  const exts = staff ? [...extensions(), ...staffExtensions()] : extensions();
+  const exts = staff
+    ? [...extensions(siteSettings), ...staffExtensions(siteSettings)]
+    : extensions(siteSettings);
   return exts.filter(ext => ext.length > 0).join(", ");
 }
 
-function authorizedImagesExtensions(staff) {
+function authorizedImagesExtensions(staff, siteSettings) {
-  return authorizesAllExtensions(staff)
+  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 ||
+    siteSettings.authorized_extensions.indexOf("*") >= 0 ||
-    (Discourse.SiteSettings.authorized_extensions_for_staff.indexOf("*") >= 0 &&
+    (siteSettings.authorized_extensions_for_staff.indexOf("*") >= 0 && staff)
-      staff)
   );
 }
 
-export function authorizesOneOrMoreExtensions(staff) {
+export function authorizesOneOrMoreExtensions(staff, siteSettings) {
-  if (authorizesAllExtensions(staff)) return true;
+  if (authorizesAllExtensions(staff, siteSettings)) return true;
 
   return (
-    Discourse.SiteSettings.authorized_extensions.split("|").filter(ext => ext)
+    siteSettings.authorized_extensions.split("|").filter(ext => ext).length > 0
-      .length > 0
   );
 }
 
-export function authorizesOneOrMoreImageExtensions(staff) {
+export function authorizesOneOrMoreImageExtensions(staff, siteSettings) {
-  if (authorizesAllExtensions(staff)) return true;
+  if (authorizesAllExtensions(staff, siteSettings)) return true;
-  return imagesExtensions(staff).length > 0;
+  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) ||
+    authorizesAllExtensions(staff, siteSettings) ||
-    IMAGES_EXTENSIONS_REGEX.test(authorizedExtensions(staff))
+    IMAGES_EXTENSIONS_REGEX.test(authorizedExtensions(staff, siteSettings))
   );
 }
 
-export function allowsAttachments(staff) {
+export function allowsAttachments(staff, siteSettings) {
   return (
-    authorizesAllExtensions(staff) ||
+    authorizesAllExtensions(staff, siteSettings) ||
-    authorizedExtensions(staff).split(", ").length >
+    authorizedExtensions(staff, siteSettings).split(", ").length >
-      imagesExtensions(staff).length
+      imagesExtensions(staff, siteSettings).length
   );
 }
 
-export function uploadIcon(staff) {
+export function uploadIcon(staff, siteSettings) {
-  return allowsAttachments(staff) ? "upload" : "far-image";
+  return allowsAttachments(staff, siteSettings) ? "upload" : "far-image";
 }
 
 function imageMarkdown(upload) {

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);

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", function(assert) {
-
+  assert.not(
-QUnit.test("validateUploadedFiles", assert => {
+    validateUploadedFiles(null, { siteSettings: this.siteSettings }),
-  assert.not(validUpload(null), "no files are invalid");
+    "no files are invalid"
-  assert.not(validUpload(undefined), "undefined files are invalid");
+  );
-  assert.not(validUpload([]), "empty array of files is 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.not(
-  assert.ok(validUpload(files, { skipValidation: true }));
+    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 = {}) {