FEATURE: add system_user_max_attachment_size_kb site setting (#28351)

* System user attachment size WIP

* spec check

* controller update

* add max to system_user_max_attachment_size_kb

* DEV: update to use static method for `max_attachment_size_for_user`

add test to use large image.
add check for failure.

* DEV: update `system_user_max_attachment_size_kb` default value to 0

remove unecessary test.
update tests to reflect the new default value of `system_user_max_attachment_size_kb`

* DEV: update maximum_file_size to check when is an attachment made by a system user

Add tests for when `system_user_max_attachment_size_kb` is over and under the limit
Add test for checking interaction with `max_attachment_size_kb`

* DEV: move `max_attachment_size_for_user` to private methods

* DEV: turn `max_attachment_size_for_user` into a static method

* DEV: typo in test case

* DEV: move max_attachment_size_for_user to private class method

* Revert "DEV: move max_attachment_size_for_user to private class method"

This reverts commit 5d5ae0b715.

---------

Co-authored-by: Gabriel Grubba <gabriel@discourse.org>
This commit is contained in:
Guhyoun Nam 2024-08-16 09:03:39 -05:00 committed by GitHub
parent a59c89211b
commit 9c1812e071
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 94 additions and 5 deletions

View File

@ -25,7 +25,6 @@ class UploadsController < ApplicationController
def create def create
# capture current user for block later on # capture current user for block later on
me = current_user me = current_user
RateLimiter.new( RateLimiter.new(
current_user, current_user,
"uploads-per-minute", "uploads-per-minute",
@ -228,7 +227,7 @@ class UploadsController < ApplicationController
"upload.attachments.too_large_humanized", "upload.attachments.too_large_humanized",
max_size: max_size:
ActiveSupport::NumberHelper.number_to_human_size( ActiveSupport::NumberHelper.number_to_human_size(
SiteSetting.max_attachment_size_kb.kilobytes, UploadsController.max_attachment_size_for_user(current_user).kilobytes,
), ),
), ),
) )
@ -277,7 +276,7 @@ class UploadsController < ApplicationController
if url.present? && is_api if url.present? && is_api
maximum_upload_size = [ maximum_upload_size = [
SiteSetting.max_image_size_kb, SiteSetting.max_image_size_kb,
SiteSetting.max_attachment_size_kb, UploadsController.max_attachment_size_for_user(current_user),
].max.kilobytes ].max.kilobytes
tempfile = tempfile =
begin begin
@ -319,12 +318,20 @@ class UploadsController < ApplicationController
private private
def self.max_attachment_size_for_user(user)
if user.id == Discourse::SYSTEM_USER_ID && !SiteSetting.system_user_max_attachment_size_kb.zero?
SiteSetting.system_user_max_attachment_size_kb
else
SiteSetting.max_attachment_size_kb
end
end
# We can preemptively check size for attachments, but not for (most) images # We can preemptively check size for attachments, but not for (most) images
# as they may be further reduced in size by UploadCreator (at this point # as they may be further reduced in size by UploadCreator (at this point
# they may have already been reduced in size by preprocessors) # they may have already been reduced in size by preprocessors)
def attachment_too_big?(file_name, file_size) def attachment_too_big?(file_name, file_size)
!FileHelper.is_supported_image?(file_name) && !FileHelper.is_supported_image?(file_name) &&
file_size >= SiteSetting.max_attachment_size_kb.kilobytes file_size >= UploadsController.max_attachment_size_for_user(current_user).kilobytes
end end
# Gifs are not resized on the client and not reduced in size by UploadCreator # Gifs are not resized on the client and not reduced in size by UploadCreator

View File

@ -1490,6 +1490,11 @@ files:
default: 4096 default: 4096
max: 1024000 max: 1024000
type: file_size_restriction type: file_size_restriction
system_user_max_attachment_size_kb:
default: 0
max: 4096000
type: file_size_restriction
hidden: true
max_image_megapixels: max_image_megapixels:
default: 40 default: 40
min: 5 min: 5

View File

@ -146,7 +146,14 @@ class UploadValidator < ActiveModel::Validator
if upload.for_export if upload.for_export
SiteSetting.max_export_file_size_kb SiteSetting.max_export_file_size_kb
else else
SiteSetting.get("max_#{type}_size_kb") if upload.user&.id == Discourse::SYSTEM_USER_ID && type == "attachment"
[
SiteSetting.get("system_user_max_attachment_size_kb"),
SiteSetting.get("max_attachment_size_kb"),
].max
else
SiteSetting.get("max_#{type}_size_kb")
end
end end
max_size_bytes = max_size_kb.kilobytes max_size_bytes = max_size_kb.kilobytes

View File

@ -2,6 +2,7 @@
RSpec.describe UploadsController do RSpec.describe UploadsController do
fab!(:user) { Fabricate(:user, refresh_auto_groups: true) } fab!(:user) { Fabricate(:user, refresh_auto_groups: true) }
fab!(:system_user) { Discourse.system_user }
describe "#create" do describe "#create" do
it "requires you to be logged in" do it "requires you to be logged in" do
@ -238,6 +239,75 @@ RSpec.describe UploadsController do
expect(message).to contain_exactly(I18n.t("upload.images.size_not_found")) expect(message).to contain_exactly(I18n.t("upload.images.size_not_found"))
end end
end end
context "when system user is logged in" do
before { sign_in(system_user) }
let(:text_file) { Rack::Test::UploadedFile.new(File.new("#{Rails.root}/LICENSE.txt")) }
it "properly returns errors if system_user_max_attachment_size_kb is not set" do
SiteSetting.authorized_extensions = "*"
SiteSetting.max_attachment_size_kb = 1
post "/uploads.json", params: { file: text_file, type: "composer" }
expect(response.status).to eq(422)
errors = response.parsed_body["errors"]
expect(errors.first).to eq(
I18n.t("upload.attachments.too_large_humanized", max_size: "1 KB"),
)
end
it "should accept large files if system user" do
SiteSetting.authorized_extensions = "*"
SiteSetting.system_user_max_attachment_size_kb = 421_730
post "/uploads.json", params: { file: text_file, type: "composer" }
expect(response.status).to eq(200)
end
it "should fail to accept large files if system user system_user_max_attachment_size_kb setting is low" do
SiteSetting.authorized_extensions = "*"
SiteSetting.max_attachment_size_kb = 1
SiteSetting.system_user_max_attachment_size_kb = 1
post "/uploads.json", params: { file: text_file, type: "composer" }
expect(response.status).to eq(422)
errors = response.parsed_body["errors"]
expect(errors.first).to eq(
I18n.t("upload.attachments.too_large_humanized", max_size: "1 KB"),
)
end
it "should fail to accept large files if system user system_user_max_attachment_size_kb setting is low and general setting is low" do
SiteSetting.authorized_extensions = "*"
SiteSetting.max_attachment_size_kb = 10
SiteSetting.system_user_max_attachment_size_kb = 5
post "/uploads.json", params: { file: text_file, type: "composer" }
expect(response.status).to eq(422)
errors = response.parsed_body["errors"]
expect(errors.first).to eq(
I18n.t("upload.attachments.too_large_humanized", max_size: "10 KB"),
)
end
it "should fail to accept large files if attachment_size settings are low" do
SiteSetting.authorized_extensions = "*"
SiteSetting.max_attachment_size_kb = 1
SiteSetting.system_user_max_attachment_size_kb = 10
post "/uploads.json", params: { file: text_file, type: "composer" }
expect(response.status).to eq(422)
errors = response.parsed_body["errors"]
expect(errors.first).to eq(
I18n.t("upload.attachments.too_large_humanized", max_size: "10 KB"),
)
end
end
end end
def upload_file(file, folder = "images") def upload_file(file, folder = "images")