Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

FIX: Pass upload type correctly to uploads#create (#29600) 

Prior to Uppy, the `uploads#create` endpoint used to receive a `type` param that indicated the purpose/target of the upload, such as `avatar`, `site_setting` and so on. With the introduction of Uppy, the `type` param became the MIME type of the file being uploaded, and the purpose/target of the upload became a new param called `upload_type`, however the backend could still use the `type` param (which now contains MIME type) as the purpose/target of the upload if `upload_type` is absent.

We technically don't need to send the MIME type over the network, but it seems like it's done by Uppy and we have no control over the `type` param that Uppy includes:

758de8167b/app/assets/javascripts/discourse/app/lib/uppy/uppy-upload.js (L146-L151)

This commit does a couple of things:

1. It amends the `uploads#create` endpoint so it always requires the `upload_type` param and doesn't fallback to `type` if `upload_type` is absent
2. It forces consumers of the `UppyUpload` class (and by extension `UppyImageUploader`) to specify `type` of the upload

Internal topic: t/140945.
This commit is contained in:
Osama Sayegh 2024-11-06 07:00:35 +03:00 committed by GitHub
parent ae721bd0f1
commit 6f8f6a7726
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 50 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/assets/javascripts/discourse/app/lib/uppy/uppy-upload.js
View File

@ -73,7 +73,7 @@ function lazyMergeConfig(config) {
return mergedConfig;
}
const REQUIRED_CONFIG_KEYS = ["id", "uploadDone"];
const REQUIRED_CONFIG_KEYS = ["id", "uploadDone", "type"];
function validateConfig(config) {
for (const key of REQUIRED_CONFIG_KEYS) {
if (!config[key]) {

2
app/assets/javascripts/discourse/tests/integration/components/form-kit/controls/image-test.gjs
View File

@ -36,7 +36,7 @@ module(
await render(<template>
<Form @mutable={{true}} @data={{data}} as |form|>
<form.Field @name="image_url" @title="Foo" as |field|>
<field.Image />
<field.Image @type="site_setting" />
</form.Field>
</Form>
</template>);

8
app/assets/javascripts/discourse/tests/integration/components/uppy-image-uploader-test.js
View File

@ -9,7 +9,7 @@ module("Integration | Component | uppy-image-uploader", function (hooks) {
test("with image", async function (assert) {
await render(hbs`
<UppyImageUploader @id="test-uppy-image-uploader" @imageUrl="/images/avatar.png" @placeholderUrl="/not/used.png" />
<UppyImageUploader @type="avatar" @id="test-uppy-image-uploader" @imageUrl="/images/avatar.png" @placeholderUrl="/not/used.png" />
`);
assert.strictEqual(
@ -38,7 +38,9 @@ module("Integration | Component | uppy-image-uploader", function (hooks) {
});
test("without image", async function (assert) {
await render(hbs`<UppyImageUploader @id="test-uppy-image-uploader" />`);
await render(
hbs`<UppyImageUploader @type="site_setting" @id="test-uppy-image-uploader" />`
);
assert.strictEqual(
count(".d-icon-far-image"),
@ -55,7 +57,7 @@ module("Integration | Component | uppy-image-uploader", function (hooks) {
test("with placeholder", async function (assert) {
await render(
hbs`<UppyImageUploader @id="test-uppy-image-uploader" @placeholderUrl="/images/avatar.png" />`
hbs`<UppyImageUploader @type="composer" @id="test-uppy-image-uploader" @placeholderUrl="/images/avatar.png" />`
);
assert.strictEqual(

6
app/controllers/uploads_controller.rb
View File

@ -32,11 +32,9 @@ class UploadsController < ApplicationController
1.minute.to_i,
).performed!
params.permit(:type, :upload_type)
raise Discourse::InvalidParameters if params[:type].blank? && params[:upload_type].blank?
params.require(:upload_type)
# 50 characters ought to be enough for the upload type
type =
(params[:upload_type].presence || params[:type].presence).parameterize(separator: "_")[0..50]
type = params[:upload_type].parameterize(separator: "_")[0..50]
if type == "avatar" &&
(

67
spec/requests/uploads_controller_spec.rb
View File

@ -29,7 +29,7 @@ RSpec.describe UploadsController do
post "/uploads.json",
params: {
file: Rack::Test::UploadedFile.new(logo_file),
type: "avatar",
upload_type: "avatar",
}
expect(response.status).to eq(200)
@ -37,20 +37,20 @@ RSpec.describe UploadsController do
post "/uploads.json",
params: {
file: Rack::Test::UploadedFile.new(logo_file),
type: "avatar",
upload_type: "avatar",
}
expect(response.status).to eq(429)
end
end
it "expects a type or upload_type" do
it "expects upload_type" do
post "/uploads.json", params: { file: logo }
expect(response.status).to eq(400)
post "/uploads.json",
params: {
file: Rack::Test::UploadedFile.new(logo_file),
type: "avatar",
upload_type: "avatar",
}
expect(response.status).to eq 200
post "/uploads.json",
@ -62,7 +62,7 @@ RSpec.describe UploadsController do
end
it "is successful with an image" do
post "/uploads.json", params: { file: logo, type: "avatar" }
post "/uploads.json", params: { file: logo, upload_type: "avatar" }
expect(response.status).to eq 200
expect(response.parsed_body["id"]).to be_present
expect(Jobs::CreateAvatarThumbnails.jobs.size).to eq(1)
@ -74,14 +74,19 @@ RSpec.describe UploadsController do
upload = UploadCreator.new(logo_file, "logo.png").create_for(-1)
logo = Rack::Test::UploadedFile.new(file_from_fixtures("logo.png"))
post "/uploads.json", params: { file: logo, type: "site_setting", for_site_setting: "true" }
post "/uploads.json",
params: {
file: logo,
upload_type: "site_setting",
for_site_setting: "true",
}
expect(response.status).to eq 200
expect(response.parsed_body["url"]).to eq(upload.url)
end
it "returns cdn url" do
set_cdn_url "https://awesome.com"
post "/uploads.json", params: { file: logo, type: "composer" }
post "/uploads.json", params: { file: logo, upload_type: "composer" }
expect(response.status).to eq 200
expect(response.parsed_body["url"]).to start_with("https://awesome.com/uploads/default/")
end
@ -89,7 +94,7 @@ RSpec.describe UploadsController do
it "is successful with an attachment" do
SiteSetting.authorized_extensions = "*"
post "/uploads.json", params: { file: text_file, type: "composer" }
post "/uploads.json", params: { file: text_file, upload_type: "composer" }
expect(response.status).to eq 200
expect(Jobs::CreateAvatarThumbnails.jobs.size).to eq(0)
@ -109,7 +114,7 @@ RSpec.describe UploadsController do
post "/uploads.json",
params: {
url: url,
type: "avatar",
upload_type: "avatar",
},
headers: {
HTTP_API_KEY: api_key,
@ -127,7 +132,12 @@ RSpec.describe UploadsController do
it "correctly sets retain_hours for admins" do
sign_in(Fabricate(:admin))
post "/uploads.json", params: { file: logo, retain_hours: 100, type: "profile_background" }
post "/uploads.json",
params: {
file: logo,
retain_hours: 100,
upload_type: "profile_background",
}
id = response.parsed_body["id"]
expect(Jobs::CreateAvatarThumbnails.jobs.size).to eq(0)
@ -135,7 +145,7 @@ RSpec.describe UploadsController do
end
it "requires a file" do
post "/uploads.json", params: { type: "composer" }
post "/uploads.json", params: { upload_type: "composer" }
expect(Jobs::CreateAvatarThumbnails.jobs.size).to eq(0)
message = response.parsed_body
@ -147,7 +157,7 @@ RSpec.describe UploadsController do
SiteSetting.authorized_extensions = "*"
SiteSetting.max_attachment_size_kb = 1
post "/uploads.json", params: { file: text_file, type: "avatar" }
post "/uploads.json", params: { file: text_file, upload_type: "avatar" }
expect(response.status).to eq(422)
expect(Jobs::CreateAvatarThumbnails.jobs.size).to eq(0)
@ -159,18 +169,18 @@ RSpec.describe UploadsController do
it "ensures user belongs to uploaded_avatars_allowed_groups when uploading an avatar" do
SiteSetting.uploaded_avatars_allowed_groups = "13"
post "/uploads.json", params: { file: logo, type: "avatar" }
post "/uploads.json", params: { file: logo, upload_type: "avatar" }
expect(response.status).to eq(422)
user.change_trust_level!(TrustLevel[3])
post "/uploads.json", params: { file: logo, type: "avatar" }
post "/uploads.json", params: { file: logo, upload_type: "avatar" }
expect(response.status).to eq(200)
end
it "ensures discourse_connect_overrides_avatar is not enabled when uploading an avatar" do
SiteSetting.discourse_connect_overrides_avatar = true
post "/uploads.json", params: { file: logo, type: "avatar" }
post "/uploads.json", params: { file: logo, upload_type: "avatar" }
expect(response.status).to eq(422)
end
@ -182,7 +192,7 @@ RSpec.describe UploadsController do
post "/uploads.json",
params: {
file: text_file,
type: "composer",
upload_type: "composer",
for_private_message: "true",
}
@ -195,7 +205,12 @@ RSpec.describe UploadsController do
SiteSetting.authorized_extensions = ""
user.update!(admin: true)
post "/uploads.json", params: { file: logo, type: "site_setting", for_site_setting: "true" }
post "/uploads.json",
params: {
file: logo,
upload_type: "site_setting",
for_site_setting: "true",
}
expect(response.status).to eq(200)
id = response.parsed_body["id"]
@ -211,7 +226,7 @@ RSpec.describe UploadsController do
SiteSetting.authorized_extensions_for_staff = "*"
user.update_columns(moderator: true)
post "/uploads.json", params: { file: text_file, type: "composer" }
post "/uploads.json", params: { file: text_file, upload_type: "composer" }
expect(response.status).to eq(200)
data = response.parsed_body
@ -222,14 +237,14 @@ RSpec.describe UploadsController do
SiteSetting.authorized_extensions = ""
SiteSetting.authorized_extensions_for_staff = "*"
post "/uploads.json", params: { file: text_file, type: "composer" }
post "/uploads.json", params: { file: text_file, upload_type: "composer" }
data = response.parsed_body
expect(data["errors"].first).to eq(I18n.t("upload.unauthorized", authorized_extensions: ""))
end
it "returns an error when it could not determine the dimensions of an image" do
post "/uploads.json", params: { file: fake_jpg, type: "composer" }
post "/uploads.json", params: { file: fake_jpg, upload_type: "composer" }
expect(response.status).to eq(422)
expect(Jobs::CreateAvatarThumbnails.jobs.size).to eq(0)
@ -247,7 +262,7 @@ RSpec.describe UploadsController do
SiteSetting.authorized_extensions = "*"
SiteSetting.max_attachment_size_kb = 1
post "/uploads.json", params: { file: text_file, type: "composer" }
post "/uploads.json", params: { file: text_file, upload_type: "composer" }
expect(response.status).to eq(422)
errors = response.parsed_body["errors"]
@ -260,7 +275,7 @@ RSpec.describe UploadsController do
SiteSetting.authorized_extensions = "*"
SiteSetting.system_user_max_attachment_size_kb = 421_730
post "/uploads.json", params: { file: text_file, type: "composer" }
post "/uploads.json", params: { file: text_file, upload_type: "composer" }
expect(response.status).to eq(200)
end
@ -269,7 +284,7 @@ RSpec.describe UploadsController do
SiteSetting.max_attachment_size_kb = 1
SiteSetting.system_user_max_attachment_size_kb = 1
post "/uploads.json", params: { file: text_file, type: "composer" }
post "/uploads.json", params: { file: text_file, upload_type: "composer" }
expect(response.status).to eq(422)
errors = response.parsed_body["errors"]
@ -283,7 +298,7 @@ RSpec.describe UploadsController do
SiteSetting.max_attachment_size_kb = 10
SiteSetting.system_user_max_attachment_size_kb = 5
post "/uploads.json", params: { file: text_file, type: "composer" }
post "/uploads.json", params: { file: text_file, upload_type: "composer" }
expect(response.status).to eq(422)
errors = response.parsed_body["errors"]
@ -297,7 +312,7 @@ RSpec.describe UploadsController do
SiteSetting.max_attachment_size_kb = 1
SiteSetting.system_user_max_attachment_size_kb = 10
post "/uploads.json", params: { file: text_file, type: "composer" }
post "/uploads.json", params: { file: text_file, upload_type: "composer" }
expect(response.status).to eq(422)
errors = response.parsed_body["errors"]
@ -313,7 +328,7 @@ RSpec.describe UploadsController do
SiteSetting.authorized_extensions = "*"
sign_in(user)
post "/uploads.json", params: { file: fake_logo, type: "composer" }
post "/uploads.json", params: { file: fake_logo, upload_type: "composer" }
expect(response.status).to eq(200)