From 43a5fc5d1a030ec0a50cda863547fd8abb1d6e49 Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Fri, 27 Oct 2023 15:16:06 +0300 Subject: [PATCH 01/23] Remove orphaned blobs --- .../app/commands/decidim/validate_upload.rb | 12 +++++++++++- .../spec/commands/decidim/validate_upload_spec.rb | 13 +++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/decidim-core/app/commands/decidim/validate_upload.rb b/decidim-core/app/commands/decidim/validate_upload.rb index 6d590f0fb822..cdea7c6dac7a 100644 --- a/decidim-core/app/commands/decidim/validate_upload.rb +++ b/decidim-core/app/commands/decidim/validate_upload.rb @@ -7,9 +7,19 @@ def initialize(form) end def call - return broadcast(:invalid, @form.errors) if @form.invalid? + if @form.invalid? + remove_invalid_file + return broadcast(:invalid, @form.errors) + end broadcast(:ok) end + + private + + def remove_invalid_file + blob = ActiveStorage::Blob.find_signed(@form.blob) + blob.purge if blob.present? + end end end diff --git a/decidim-core/spec/commands/decidim/validate_upload_spec.rb b/decidim-core/spec/commands/decidim/validate_upload_spec.rb index 2e61b7a21b38..450d1be1ccf5 100644 --- a/decidim-core/spec/commands/decidim/validate_upload_spec.rb +++ b/decidim-core/spec/commands/decidim/validate_upload_spec.rb @@ -9,16 +9,23 @@ module Decidim let(:form) do double( invalid?: invalid, + blob:, errors: ) end let(:invalid) { false } let(:errors) { [] } + let(:blob) { "foobar" } describe "when form is valid" do it "broadcasts ok" do expect { command.call }.to broadcast(:ok) end + + it "ignores file removal" do + expect(ActiveStorage::Blob).not_to receive(:find_signed).with(blob) + command.call + end end describe "when the form is not valid" do @@ -26,8 +33,14 @@ module Decidim let(:errors) { ["File too dummy"] } it "broadcasts invalid" do + allow(ActiveStorage::Blob).to receive(:find_signed).with(blob).and_return(double(purge: true)) expect { command.call }.to broadcast(:invalid, errors) end + + it "removes the invalid file" do + expect(ActiveStorage::Blob).to receive(:find_signed) + command.call + end end end end From 5d6ab94337609fdecb964151c45d54fb83eec52a Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Fri, 27 Oct 2023 18:39:05 +0300 Subject: [PATCH 02/23] Add Rake task to cleanup old unattached blobs --- RELEASE_NOTES.md | 12 ++++++++++++ decidim-core/lib/tasks/decidim_attachments.rake | 14 ++++++++++++++ 2 files changed, 26 insertions(+) create mode 100644 decidim-core/lib/tasks/decidim_attachments.rake diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 4dc78cd27963..7cbf01c61b2f 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -285,6 +285,18 @@ bundle exec rails decidim:robots:replace You can see more details about this change on PR [\#11693](https://github.com/decidim/decidim/pull/11693) +### 3.12 Clean up orphaned attachment blobs + +We have added a new task that helps you clean the orphaned attachment blobs. This task will remove all the attachment blobs that are not referenced by any attachment record. This helps cleaning your filesystem of unused files. + +You can run the task with the following command: + +```bash +bundle exec decidim:attachmens:cleanup +``` + +You can see more details about this change on PR [\#11851](https://github.com/decidim/decidim/pull/11851) + ## 4. Scheduled tasks Implementers need to configure these changes it in your scheduler task system in the production server. We give the examples diff --git a/decidim-core/lib/tasks/decidim_attachments.rake b/decidim-core/lib/tasks/decidim_attachments.rake new file mode 100644 index 000000000000..ccc6db0e2dc9 --- /dev/null +++ b/decidim-core/lib/tasks/decidim_attachments.rake @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +namespace :decidim do + namespace :attachmens do + desc "Cleanup the orphaned blobs attachments" + task cleanup: :environment do + ActiveStorage::Blob.includes(:attachments).find_each do |blob| + next if blob.attachments.any? + + blob.purge + end + end + end +end From bd7c937df68a03aaf8eba6187a53d3797cb43051 Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Fri, 27 Oct 2023 21:51:12 +0300 Subject: [PATCH 03/23] Refactor task --- decidim-core/lib/tasks/decidim_attachments.rake | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/decidim-core/lib/tasks/decidim_attachments.rake b/decidim-core/lib/tasks/decidim_attachments.rake index ccc6db0e2dc9..b5b523fbc281 100644 --- a/decidim-core/lib/tasks/decidim_attachments.rake +++ b/decidim-core/lib/tasks/decidim_attachments.rake @@ -4,11 +4,7 @@ namespace :decidim do namespace :attachmens do desc "Cleanup the orphaned blobs attachments" task cleanup: :environment do - ActiveStorage::Blob.includes(:attachments).find_each do |blob| - next if blob.attachments.any? - - blob.purge - end + ActiveStorage::Blob.unattached.find_each(&:purge_later) end end end From dcdba03b126017635f47b3b62029648b6f0662cf Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Wed, 8 Nov 2023 12:11:27 +0200 Subject: [PATCH 04/23] Add job work to remove unattached blobs after a period of time --- .../jobs/decidim/clean_unattached_blob_job.rb | 14 ++++++++ decidim-core/lib/decidim/core/engine.rb | 8 +++++ .../decidim/clean_unattached_blob_job_spec.rb | 25 +++++++++++++++ decidim-core/spec/system/attachments_spec.rb | 32 +++++++++++++++++++ 4 files changed, 79 insertions(+) create mode 100644 decidim-core/app/jobs/decidim/clean_unattached_blob_job.rb create mode 100644 decidim-core/spec/jobs/decidim/clean_unattached_blob_job_spec.rb create mode 100644 decidim-core/spec/system/attachments_spec.rb diff --git a/decidim-core/app/jobs/decidim/clean_unattached_blob_job.rb b/decidim-core/app/jobs/decidim/clean_unattached_blob_job.rb new file mode 100644 index 000000000000..5523a6c0979e --- /dev/null +++ b/decidim-core/app/jobs/decidim/clean_unattached_blob_job.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +module Decidim + class CleanUnattachedBlobJob < ApplicationJob + def perform(key) + blob = ActiveStorage::Blob.find_by(key:) + + return if blob.blank? + return if blob.attachments.any? + + blob.purge + end + end +end diff --git a/decidim-core/lib/decidim/core/engine.rb b/decidim-core/lib/decidim/core/engine.rb index 3035b2eddba5..72f4009a7982 100644 --- a/decidim-core/lib/decidim/core/engine.rb +++ b/decidim-core/lib/decidim/core/engine.rb @@ -280,6 +280,14 @@ class Engine < ::Rails::Engine end end + initializer "decidim_core.clean_attachments" do + config.to_prepare do + ActiveSupport::Notifications.subscribe("service_upload.active_storage") do |_, _, _, _, payload| + Decidim::CleanUnattachedBlobJob.set(wait: 10.minutes).perform_later(payload[:key]) + end + end + end + initializer "decidim_core.validators" do config.to_prepare do # Decidim overrides to the file content type validator diff --git a/decidim-core/spec/jobs/decidim/clean_unattached_blob_job_spec.rb b/decidim-core/spec/jobs/decidim/clean_unattached_blob_job_spec.rb new file mode 100644 index 000000000000..fd9192ce878c --- /dev/null +++ b/decidim-core/spec/jobs/decidim/clean_unattached_blob_job_spec.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +require "spec_helper" + +module Decidim + describe CleanUnattachedBlobJob do + it "skips attached files" do + user = create(:user) + + expect { described_class.perform_now(user.avatar.blob.key) }.not_to change(ActiveStorage::Blob, :count) + expect(user.reload.avatar).to be_attached + end + + it "ignores missing blobs" do + expect { described_class.perform_now("missing") }.not_to raise_error + end + + it "deletes unattached files" do + blob = ActiveStorage::Blob.create_and_upload!(io: StringIO.new(Decidim::Dev.asset("import_proposals.csv")), filename: "funky.bin") + + expect(blob.attachments.size).to eq(0) + expect { described_class.perform_now(blob.key) }.to change(ActiveStorage::Blob, :count).by(-1) + end + end +end diff --git a/decidim-core/spec/system/attachments_spec.rb b/decidim-core/spec/system/attachments_spec.rb new file mode 100644 index 000000000000..16cdfc70356f --- /dev/null +++ b/decidim-core/spec/system/attachments_spec.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +require "spec_helper" + +describe "Attachment spec", type: :system do + let(:user) { create(:user, :confirmed) } + let(:organization) { user.organization } + let(:file_location) { Decidim::Dev.asset("import_proposals.csv") } + let(:filename) { file_location.to_s.split("/").last } + + before do + switch_to_host(organization.host) + login_as user, scope: :user + visit decidim.account_path + ActiveJob::Base.queue_adapter.enqueued_jobs.clear + end + + it "Enqueues the cleanup job" do + find("#user_avatar_button").click + + within ".upload-modal" do + click_remove(true) + input_element = find("input[type='file']", visible: :all) + input_element.attach_file(file_location) + within "[data-filename='#{filename}']" do + expect(page).to have_css(filled_selector(true), wait: 5) + expect(page).to have_content(filename.first(12)) + end + end + expect(Decidim::CleanUnattachedBlobJob).to have_been_enqueued + end +end From f6323ccb3f90d44460babe500c2ae842793ce1ac Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Wed, 8 Nov 2023 13:32:29 +0200 Subject: [PATCH 05/23] Running linters --- decidim-core/spec/system/attachments_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/decidim-core/spec/system/attachments_spec.rb b/decidim-core/spec/system/attachments_spec.rb index 16cdfc70356f..84890b2a434f 100644 --- a/decidim-core/spec/system/attachments_spec.rb +++ b/decidim-core/spec/system/attachments_spec.rb @@ -2,7 +2,7 @@ require "spec_helper" -describe "Attachment spec", type: :system do +describe "Attachment spec" do let(:user) { create(:user, :confirmed) } let(:organization) { user.organization } let(:file_location) { Decidim::Dev.asset("import_proposals.csv") } From 84af5a620578aae69a3da78bfc0f5f9dc8c08ff1 Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Fri, 8 Mar 2024 22:46:15 +0200 Subject: [PATCH 06/23] Running linters --- decidim-core/spec/system/attachments_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/decidim-core/spec/system/attachments_spec.rb b/decidim-core/spec/system/attachments_spec.rb index 84890b2a434f..57e568dea8de 100644 --- a/decidim-core/spec/system/attachments_spec.rb +++ b/decidim-core/spec/system/attachments_spec.rb @@ -16,7 +16,7 @@ end it "Enqueues the cleanup job" do - find("#user_avatar_button").click + find_by_id("user_avatar_button").click within ".upload-modal" do click_remove(true) From b51842c5071a8be6ee5dadbb87b3aad875d3551e Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Fri, 8 Mar 2024 22:59:56 +0200 Subject: [PATCH 07/23] Fix spec --- decidim-core/lib/decidim/core/engine.rb | 2 +- decidim-core/spec/system/attachments_spec.rb | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/decidim-core/lib/decidim/core/engine.rb b/decidim-core/lib/decidim/core/engine.rb index 51fff23b443b..8c6170c15ec5 100644 --- a/decidim-core/lib/decidim/core/engine.rb +++ b/decidim-core/lib/decidim/core/engine.rb @@ -394,7 +394,7 @@ class Engine < ::Rails::Engine end initializer "decidim_core.clean_attachments" do - config.to_prepare do + config.after_initialize do ActiveSupport::Notifications.subscribe("service_upload.active_storage") do |_, _, _, _, payload| Decidim::CleanUnattachedBlobJob.set(wait: 10.minutes).perform_later(payload[:key]) end diff --git a/decidim-core/spec/system/attachments_spec.rb b/decidim-core/spec/system/attachments_spec.rb index 57e568dea8de..d2c30519c29c 100644 --- a/decidim-core/spec/system/attachments_spec.rb +++ b/decidim-core/spec/system/attachments_spec.rb @@ -12,7 +12,6 @@ switch_to_host(organization.host) login_as user, scope: :user visit decidim.account_path - ActiveJob::Base.queue_adapter.enqueued_jobs.clear end it "Enqueues the cleanup job" do @@ -20,6 +19,8 @@ within ".upload-modal" do click_remove(true) + + ActiveJob::Base.queue_adapter.enqueued_jobs.clear input_element = find("input[type='file']", visible: :all) input_element.attach_file(file_location) within "[data-filename='#{filename}']" do From b9ace94034993a15487b33ec4eef156e71f47e4a Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Mon, 11 Mar 2024 14:28:43 +0200 Subject: [PATCH 08/23] Fix spelling --- RELEASE_NOTES.md | 2 +- decidim-core/lib/tasks/decidim_attachments.rake | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 08df4d4e390b..7a92e60b5783 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -127,7 +127,7 @@ We have added a new task that helps you clean the orphaned attachment blobs. Thi You can run the task with the following command: ```bash -bundle exec decidim:attachmens:cleanup +bundle exec decidim:attachments:cleanup ``` You can see more details about this change on PR [\#11851](https://github.com/decidim/decidim/pull/11851) diff --git a/decidim-core/lib/tasks/decidim_attachments.rake b/decidim-core/lib/tasks/decidim_attachments.rake index b5b523fbc281..5568d73a50f0 100644 --- a/decidim-core/lib/tasks/decidim_attachments.rake +++ b/decidim-core/lib/tasks/decidim_attachments.rake @@ -1,7 +1,7 @@ # frozen_string_literal: true namespace :decidim do - namespace :attachmens do + namespace :attachments do desc "Cleanup the orphaned blobs attachments" task cleanup: :environment do ActiveStorage::Blob.unattached.find_each(&:purge_later) From 342751eb2fa7b059d63c83d8fe0130c6b886ef2a Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Tue, 16 Apr 2024 01:12:32 +0300 Subject: [PATCH 09/23] Remove the "decidim_core.clean_attachments" initializer and the CleanUnattachedBlobJob --- .../jobs/decidim/clean_unattached_blob_job.rb | 14 -------- decidim-core/lib/decidim/core/engine.rb | 8 ----- .../decidim/clean_unattached_blob_job_spec.rb | 25 -------------- decidim-core/spec/system/attachments_spec.rb | 33 ------------------- 4 files changed, 80 deletions(-) delete mode 100644 decidim-core/app/jobs/decidim/clean_unattached_blob_job.rb delete mode 100644 decidim-core/spec/jobs/decidim/clean_unattached_blob_job_spec.rb delete mode 100644 decidim-core/spec/system/attachments_spec.rb diff --git a/decidim-core/app/jobs/decidim/clean_unattached_blob_job.rb b/decidim-core/app/jobs/decidim/clean_unattached_blob_job.rb deleted file mode 100644 index 5523a6c0979e..000000000000 --- a/decidim-core/app/jobs/decidim/clean_unattached_blob_job.rb +++ /dev/null @@ -1,14 +0,0 @@ -# frozen_string_literal: true - -module Decidim - class CleanUnattachedBlobJob < ApplicationJob - def perform(key) - blob = ActiveStorage::Blob.find_by(key:) - - return if blob.blank? - return if blob.attachments.any? - - blob.purge - end - end -end diff --git a/decidim-core/lib/decidim/core/engine.rb b/decidim-core/lib/decidim/core/engine.rb index 8c6170c15ec5..a2999df38149 100644 --- a/decidim-core/lib/decidim/core/engine.rb +++ b/decidim-core/lib/decidim/core/engine.rb @@ -393,14 +393,6 @@ class Engine < ::Rails::Engine end end - initializer "decidim_core.clean_attachments" do - config.after_initialize do - ActiveSupport::Notifications.subscribe("service_upload.active_storage") do |_, _, _, _, payload| - Decidim::CleanUnattachedBlobJob.set(wait: 10.minutes).perform_later(payload[:key]) - end - end - end - initializer "decidim_core.validators" do config.to_prepare do # Decidim overrides to the file content type validator diff --git a/decidim-core/spec/jobs/decidim/clean_unattached_blob_job_spec.rb b/decidim-core/spec/jobs/decidim/clean_unattached_blob_job_spec.rb deleted file mode 100644 index fd9192ce878c..000000000000 --- a/decidim-core/spec/jobs/decidim/clean_unattached_blob_job_spec.rb +++ /dev/null @@ -1,25 +0,0 @@ -# frozen_string_literal: true - -require "spec_helper" - -module Decidim - describe CleanUnattachedBlobJob do - it "skips attached files" do - user = create(:user) - - expect { described_class.perform_now(user.avatar.blob.key) }.not_to change(ActiveStorage::Blob, :count) - expect(user.reload.avatar).to be_attached - end - - it "ignores missing blobs" do - expect { described_class.perform_now("missing") }.not_to raise_error - end - - it "deletes unattached files" do - blob = ActiveStorage::Blob.create_and_upload!(io: StringIO.new(Decidim::Dev.asset("import_proposals.csv")), filename: "funky.bin") - - expect(blob.attachments.size).to eq(0) - expect { described_class.perform_now(blob.key) }.to change(ActiveStorage::Blob, :count).by(-1) - end - end -end diff --git a/decidim-core/spec/system/attachments_spec.rb b/decidim-core/spec/system/attachments_spec.rb deleted file mode 100644 index d2c30519c29c..000000000000 --- a/decidim-core/spec/system/attachments_spec.rb +++ /dev/null @@ -1,33 +0,0 @@ -# frozen_string_literal: true - -require "spec_helper" - -describe "Attachment spec" do - let(:user) { create(:user, :confirmed) } - let(:organization) { user.organization } - let(:file_location) { Decidim::Dev.asset("import_proposals.csv") } - let(:filename) { file_location.to_s.split("/").last } - - before do - switch_to_host(organization.host) - login_as user, scope: :user - visit decidim.account_path - end - - it "Enqueues the cleanup job" do - find_by_id("user_avatar_button").click - - within ".upload-modal" do - click_remove(true) - - ActiveJob::Base.queue_adapter.enqueued_jobs.clear - input_element = find("input[type='file']", visible: :all) - input_element.attach_file(file_location) - within "[data-filename='#{filename}']" do - expect(page).to have_css(filled_selector(true), wait: 5) - expect(page).to have_content(filename.first(12)) - end - end - expect(Decidim::CleanUnattachedBlobJob).to have_been_enqueued - end -end From 3490ff85dff92e5f4deb740f662d819e8f54435f Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Tue, 16 Apr 2024 01:20:54 +0300 Subject: [PATCH 10/23] Change the rake task so it prevents deleting blobs that are less than 60 minutes --- decidim-core/lib/tasks/decidim_attachments.rake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/decidim-core/lib/tasks/decidim_attachments.rake b/decidim-core/lib/tasks/decidim_attachments.rake index 5568d73a50f0..d504b3df3f77 100644 --- a/decidim-core/lib/tasks/decidim_attachments.rake +++ b/decidim-core/lib/tasks/decidim_attachments.rake @@ -4,7 +4,7 @@ namespace :decidim do namespace :attachments do desc "Cleanup the orphaned blobs attachments" task cleanup: :environment do - ActiveStorage::Blob.unattached.find_each(&:purge_later) + ActiveStorage::Blob.unattached.where(ActiveStorage::Blob.arel_table[:created_at].lteq(1.hour.ago)).find_each(&:purge_later) end end end From 63586fa1add270cbc6c64d4a135d8a5fb36355aa Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Tue, 16 Apr 2024 01:25:27 +0300 Subject: [PATCH 11/23] Change the Releases Notes so the task is run two times with one hour of difference just to be sure --- RELEASE_NOTES.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 7a92e60b5783..639285d9180f 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -116,13 +116,11 @@ bin/rails decidim:upgrade:fix_orphan_categorizations You can read more about this change on PR [\#12143](https://github.com/decidim/decidim/pull/12143). -### 3.5. [[TITLE OF THE ACTION]] +### 3.5. Clean up orphaned attachment blobs -You can read more about this change on PR [\#XXXX](https://github.com/decidim/decidim/pull/XXXX). - -### 3.13 Clean up orphaned attachment blobs +We have added a new task that helps you clean the orphaned attachment blobs. This task will remove all the attachment blobs that have been created for more than 1 hour and are not yet referenced by any attachment record. This helps cleaning your filesystem of unused files. -We have added a new task that helps you clean the orphaned attachment blobs. This task will remove all the attachment blobs that are not referenced by any attachment record. This helps cleaning your filesystem of unused files. +For a consistent behavior we are recommending to run this task 2 times at least 1 hour apart. You can run the task with the following command: @@ -132,6 +130,10 @@ bundle exec decidim:attachments:cleanup You can see more details about this change on PR [\#11851](https://github.com/decidim/decidim/pull/11851) +### 3.6. [[TITLE OF THE ACTION]] + +You can read more about this change on PR [\#XXXX](https://github.com/decidim/decidim/pull/XXXX). + ## 4. Scheduled tasks Implementers need to configure these changes it in your scheduler task system in the production server. We give the examples From 2f38d9c3af58ce1e9c00def9d2245dc3806526f3 Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Tue, 16 Apr 2024 03:50:13 +0300 Subject: [PATCH 12/23] DirectUploadsController validations it prevents access by unauthenticated users it validates by the allowed mime types in system defined for participants in case the current_user is a normal participant it validates by the allowed mime types in system defined for admins in case the current_user is an admin --- .../decidim/direct_uploads_controller.rb | 51 +++++++++++++++++++ decidim-core/lib/decidim/core/engine.rb | 8 +++ 2 files changed, 59 insertions(+) create mode 100644 decidim-core/app/controllers/decidim/direct_uploads_controller.rb diff --git a/decidim-core/app/controllers/decidim/direct_uploads_controller.rb b/decidim-core/app/controllers/decidim/direct_uploads_controller.rb new file mode 100644 index 000000000000..7275c474a0bf --- /dev/null +++ b/decidim-core/app/controllers/decidim/direct_uploads_controller.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +module Decidim + class DirectUploadsController < ActiveStorage::DirectUploadsController + include Decidim::NeedsOrganization + skip_before_action :verify_organization + + before_action :check_organization! + before_action :check_authenticated! + before_action :validate_direct_upload + + protected + + def validate_direct_upload + maximum_allowed_size = current_organization.settings.upload_maximum_file_size + + extension = File.extname(blob_args[:filename]).delete(".") + + head :unprocessable_entity unless maximum_allowed_size.try(:to_i) >= blob_args[:byte_size] + head :unprocessable_entity unless content_types.any? { |pattern| pattern.match?(blob_args[:content_type]) } + head :unprocessable_entity unless content_types.any? { |pattern| pattern.match?(MiniMime.lookup_by_extension(extension).content_type) } + head :unprocessable_entity unless allowed_extensions.any? { |pattern| pattern.match?(extension) } + rescue NoMethodError + head :unprocessable_entity + end + + def check_organization! + head :unauthorized if current_organization.blank? + end + + def check_authenticated! + head :unauthorized if current_user.blank? + end + + def allowed_extensions + if URI.parse(request.referer).path.starts_with?("/admin") + current_organization.settings.upload_allowed_file_extensions_admin + else + current_organization.settings.upload_allowed_file_extensions + end + end + + def content_types + if URI.parse(request.referer).path.starts_with?("/admin") + current_organization.settings.upload_allowed_content_types_admin + else + current_organization.settings.upload_allowed_content_types + end + end + end +end diff --git a/decidim-core/lib/decidim/core/engine.rb b/decidim-core/lib/decidim/core/engine.rb index a2999df38149..074696231b96 100644 --- a/decidim-core/lib/decidim/core/engine.rb +++ b/decidim-core/lib/decidim/core/engine.rb @@ -266,6 +266,14 @@ class Engine < ::Rails::Engine app.config.exceptions_app = Decidim::Core::Engine.routes end + initializer "decidim_core.direct_uploader_paths", after: "decidim_core.exceptions_app" do |_app| + Rails.application.routes.prepend do + scope ActiveStorage.routes_prefix do + post "/direct_uploads" => "decidim/direct_uploads#create", :as => :decidim_direct_uploads + end + end + end + initializer "decidim_core.locales" do |app| app.config.i18n.fallbacks = true end From 7bccc7ee7ebe9529e06bf10cec5dc1baebc34024 Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Tue, 16 Apr 2024 08:35:09 +0300 Subject: [PATCH 13/23] Add specs --- .../decidim/direct_uploads_controller.rb | 6 +- .../decidim/direct_uploads_controller_spec.rb | 145 ++++++++++++++++++ 2 files changed, 150 insertions(+), 1 deletion(-) create mode 100644 decidim-core/spec/controllers/decidim/direct_uploads_controller_spec.rb diff --git a/decidim-core/app/controllers/decidim/direct_uploads_controller.rb b/decidim-core/app/controllers/decidim/direct_uploads_controller.rb index 7275c474a0bf..4200d43ce3ff 100644 --- a/decidim-core/app/controllers/decidim/direct_uploads_controller.rb +++ b/decidim-core/app/controllers/decidim/direct_uploads_controller.rb @@ -11,12 +11,16 @@ class DirectUploadsController < ActiveStorage::DirectUploadsController protected + def verify_organization; end + def validate_direct_upload maximum_allowed_size = current_organization.settings.upload_maximum_file_size extension = File.extname(blob_args[:filename]).delete(".") - head :unprocessable_entity unless maximum_allowed_size.try(:to_i) >= blob_args[:byte_size] + Rails.logger.debug content_types.inspect + + head :unprocessable_entity unless maximum_allowed_size.try(:to_i) >= blob_args[:byte_size].try(:to_i) head :unprocessable_entity unless content_types.any? { |pattern| pattern.match?(blob_args[:content_type]) } head :unprocessable_entity unless content_types.any? { |pattern| pattern.match?(MiniMime.lookup_by_extension(extension).content_type) } head :unprocessable_entity unless allowed_extensions.any? { |pattern| pattern.match?(extension) } diff --git a/decidim-core/spec/controllers/decidim/direct_uploads_controller_spec.rb b/decidim-core/spec/controllers/decidim/direct_uploads_controller_spec.rb new file mode 100644 index 000000000000..c2bae13984d2 --- /dev/null +++ b/decidim-core/spec/controllers/decidim/direct_uploads_controller_spec.rb @@ -0,0 +1,145 @@ +# frozen_string_literal: true + +require "spec_helper" + +module Decidim + describe DirectUploadsController do + describe "POST #create" do + let(:checksum) { OpenSSL::Digest.base64digest("MD5", "Hello") } + + let(:blob) do + { + filename: "hello.txt", + byte_size: 6, + checksum:, + content_type: "text/plain" + } + end + + let(:extensions) { %w(txt) } + let(:content_types) { %w(text/plain) } + let(:file_size) { { "default" => 5, "avatar" => 2 } } + + let(:file_upload_settings) do + { + "allowed_file_extensions" => { + "default" => extensions, + "admin" => extensions, + "image" => extensions + }, + "allowed_content_types" => { + "default" => content_types, + "admin" => content_types + }, + "maximum_file_size" => file_size + } + end + + let(:organization) { create(:organization, file_upload_settings:, favicon: nil, official_img_footer: nil) } + let!(:user) { create(:user, :confirmed, organization:, avatar: nil) } + + let(:params) do + { + blob:, + direct_upload: blob + } + end + + context "when the user is not logged in" do + before do + request.env["decidim.current_organization"] = organization + end + + it "returns unauthorized" do + post(:create, params:) + + expect(response).to have_http_status(:unauthorized) + end + end + + context "when the organization does not exists" do + before do + request.env["decidim.current_organization"] = nil + request.headers["HTTP_REFERER"] = "http://#{organization.host}" + sign_in user + end + + it "returns unauthorized" do + post(:create, params:) + + expect(response).to have_http_status(:unauthorized) + end + end + + context "when the upload is successful" do + before do + request.env["decidim.current_organization"] = organization + request.headers["HTTP_REFERER"] = "http://#{organization.host}" + sign_in user + end + + it "returns success" do + post(:create, params:) + + expect(response).to have_http_status(:ok) + expect(response.body).to include("content_type") + end + end + + context "when the attachment is not allowed" do + before do + request.env["decidim.current_organization"] = organization + request.headers["HTTP_REFERER"] = "http://#{organization.host}" + sign_in user + end + + context "when content_type is not allowed" do + let(:content_types) { %w(image/jpeg) } + + it "returns renders unprocessable entity" do + post(:create, params:) + + expect(response).to have_http_status(:unprocessable_entity) + end + end + + context "when extension is not allowed" do + let(:extensions) { %w(jpeg) } + + it "returns renders unprocessable entity" do + post(:create, params:) + + expect(response).to have_http_status(:unprocessable_entity) + end + end + + context "when size is not allowed" do + let(:file_size) { { "default" => 0, "avatar" => 0 } } + + it "returns renders unprocessable entity" do + post(:create, params:) + + expect(response).to have_http_status(:unprocessable_entity) + end + end + + context "when extension not matching content_type" do + let(:blob) do + { + filename: "hello.pdf", + byte_size: 6, + checksum:, + content_type: "text/plain" + } + end + + it "returns renders unprocessable entity" do + post(:create, params:) + + expect(response).to have_http_status(:unprocessable_entity) + end + end + end + end + end +end From 2c961cd30a7eababe16496b60984185ea72f3bc4 Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Tue, 16 Apr 2024 10:53:58 +0300 Subject: [PATCH 14/23] Update RELEASE_NOTES.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Andrés Pereira de Lucena --- RELEASE_NOTES.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 639285d9180f..7ec349165f1c 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -120,8 +120,6 @@ You can read more about this change on PR [\#12143](https://github.com/decidim/d We have added a new task that helps you clean the orphaned attachment blobs. This task will remove all the attachment blobs that have been created for more than 1 hour and are not yet referenced by any attachment record. This helps cleaning your filesystem of unused files. -For a consistent behavior we are recommending to run this task 2 times at least 1 hour apart. - You can run the task with the following command: ```bash From a544e43e3439fcdfb253873760a26e3d64c59b47 Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Tue, 16 Apr 2024 19:15:57 +0300 Subject: [PATCH 15/23] Apply review recommendation --- .../concerns/decidim/direct_upload.rb | 76 +++++++++++++++++++ .../decidim/direct_uploads_controller.rb | 55 -------------- decidim-core/lib/decidim/core.rb | 6 ++ decidim-core/lib/decidim/core/engine.rb | 6 +- .../lib/decidim/organization_settings.rb | 7 +- .../lib/tasks/decidim_attachments.rake | 2 +- .../direct_uploads_controller_spec.rb | 2 +- .../generators/app_templates/initializer.rb | 4 + 8 files changed, 95 insertions(+), 63 deletions(-) create mode 100644 decidim-core/app/controllers/concerns/decidim/direct_upload.rb delete mode 100644 decidim-core/app/controllers/decidim/direct_uploads_controller.rb rename decidim-core/spec/controllers/{decidim => active_storage}/direct_uploads_controller_spec.rb (99%) diff --git a/decidim-core/app/controllers/concerns/decidim/direct_upload.rb b/decidim-core/app/controllers/concerns/decidim/direct_upload.rb new file mode 100644 index 000000000000..6be4849e89a3 --- /dev/null +++ b/decidim-core/app/controllers/concerns/decidim/direct_upload.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +module Decidim + module DirectUpload + extend ActiveSupport::Concern + + included do + include Decidim::NeedsOrganization + skip_before_action :verify_organization + + before_action :check_organization! + before_action :check_authenticated! + before_action :validate_direct_upload + end + + protected + + def verify_organization; end + + def validate_direct_upload + return if current_admin.present? + + head :unprocessable_entity unless [ + maximum_allowed_size.try(:to_i) >= blob_args[:byte_size].try(:to_i), + content_types.any? { |pattern| pattern.match?(blob_args[:content_type]) }, + content_types.any? { |pattern| pattern.match?(MiniMime.lookup_by_extension(extension)&.content_type) }, + allowed_extensions.any? { |pattern| pattern.match?(extension) } + ].all? + rescue NoMethodError + head :unprocessable_entity + end + + def extension + File.extname(blob_args[:filename]).delete(".") + end + + def maximum_allowed_size + current_organization.settings.upload_maximum_file_size + end + + def check_organization! + head :unauthorized if current_organization.blank? && current_admin.blank? + end + + def check_authenticated! + head :unauthorized if current_user.blank? && current_admin.blank? + end + + def allowed_extensions + if user_has_elevated_role? + current_organization.settings.upload_allowed_file_extensions_admin + else + current_organization.settings.upload_allowed_file_extensions + end + end + + def content_types + if user_has_elevated_role? + current_organization.settings.upload_allowed_content_types_admin + else + current_organization.settings.upload_allowed_content_types + end + end + + private + + def user_has_elevated_role? + [ + current_user&.admin?, + defined?(Decidim::Assemblies::AssembliesWithUserRole) && Decidim::Assemblies::AssembliesWithUserRole.for(current_user).any?, + defined?(Decidim::Conferences::ConferencesWithUserRole) && Decidim::Conferences::ConferencesWithUserRole.for(current_user).any?, + defined?(Decidim::ParticipatoryProcessesWithUserRole) && Decidim::ParticipatoryProcessesWithUserRole.for(current_user).any? + ].any? + end + end +end diff --git a/decidim-core/app/controllers/decidim/direct_uploads_controller.rb b/decidim-core/app/controllers/decidim/direct_uploads_controller.rb deleted file mode 100644 index 4200d43ce3ff..000000000000 --- a/decidim-core/app/controllers/decidim/direct_uploads_controller.rb +++ /dev/null @@ -1,55 +0,0 @@ -# frozen_string_literal: true - -module Decidim - class DirectUploadsController < ActiveStorage::DirectUploadsController - include Decidim::NeedsOrganization - skip_before_action :verify_organization - - before_action :check_organization! - before_action :check_authenticated! - before_action :validate_direct_upload - - protected - - def verify_organization; end - - def validate_direct_upload - maximum_allowed_size = current_organization.settings.upload_maximum_file_size - - extension = File.extname(blob_args[:filename]).delete(".") - - Rails.logger.debug content_types.inspect - - head :unprocessable_entity unless maximum_allowed_size.try(:to_i) >= blob_args[:byte_size].try(:to_i) - head :unprocessable_entity unless content_types.any? { |pattern| pattern.match?(blob_args[:content_type]) } - head :unprocessable_entity unless content_types.any? { |pattern| pattern.match?(MiniMime.lookup_by_extension(extension).content_type) } - head :unprocessable_entity unless allowed_extensions.any? { |pattern| pattern.match?(extension) } - rescue NoMethodError - head :unprocessable_entity - end - - def check_organization! - head :unauthorized if current_organization.blank? - end - - def check_authenticated! - head :unauthorized if current_user.blank? - end - - def allowed_extensions - if URI.parse(request.referer).path.starts_with?("/admin") - current_organization.settings.upload_allowed_file_extensions_admin - else - current_organization.settings.upload_allowed_file_extensions - end - end - - def content_types - if URI.parse(request.referer).path.starts_with?("/admin") - current_organization.settings.upload_allowed_content_types_admin - else - current_organization.settings.upload_allowed_content_types - end - end - end -end diff --git a/decidim-core/lib/decidim/core.rb b/decidim-core/lib/decidim/core.rb index 823563c5f280..f315d6092d9e 100644 --- a/decidim-core/lib/decidim/core.rb +++ b/decidim-core/lib/decidim/core.rb @@ -561,6 +561,12 @@ def self.reset_all_column_information {} end + # This config parameter is used to set the default amount of time after which the unattached blobs + # are cleaned up. The default value is 60 minutes. + config_accessor :clean_up_unattached_blobs_after do + 60.minutes + end + # Public: Registers a global engine. This method is intended to be used # by component engines that also offer unscoped functionality # diff --git a/decidim-core/lib/decidim/core/engine.rb b/decidim-core/lib/decidim/core/engine.rb index 074696231b96..b8968b59901a 100644 --- a/decidim-core/lib/decidim/core/engine.rb +++ b/decidim-core/lib/decidim/core/engine.rb @@ -267,10 +267,8 @@ class Engine < ::Rails::Engine end initializer "decidim_core.direct_uploader_paths", after: "decidim_core.exceptions_app" do |_app| - Rails.application.routes.prepend do - scope ActiveStorage.routes_prefix do - post "/direct_uploads" => "decidim/direct_uploads#create", :as => :decidim_direct_uploads - end + config.to_prepare do + ActiveStorage::DirectUploadsController.include Decidim::DirectUpload end end diff --git a/decidim-core/lib/decidim/organization_settings.rb b/decidim-core/lib/decidim/organization_settings.rb index 07aeb2e05d6a..5894a291141a 100644 --- a/decidim-core/lib/decidim/organization_settings.rb +++ b/decidim-core/lib/decidim/organization_settings.rb @@ -104,8 +104,8 @@ def defaults_hash { "upload" => { "allowed_file_extensions" => { - "default" => %w(jpg jpeg png webp pdf rtf txt), - "admin" => %w(jpg jpeg png webp pdf doc docx xls xlsx ppt pptx ppx rtf txt odt ott odf otg ods ots), + "default" => %w(jpg jpeg png webp pdf rtf txt csv), + "admin" => %w(jpg jpeg png webp pdf doc docx xls xlsx ppt pptx ppx rtf txt odt ott odf otg ods ots csv json), "image" => %w(jpg jpeg png webp) }, "allowed_content_types" => { @@ -114,6 +114,7 @@ def defaults_hash application/pdf application/rtf text/plain + text/csv ), "admin" => %w( image/* @@ -125,7 +126,9 @@ def defaults_hash application/vnd.oasis.opendocument application/pdf application/rtf + application/json text/plain + text/csv ) }, "maximum_file_size" => { diff --git a/decidim-core/lib/tasks/decidim_attachments.rake b/decidim-core/lib/tasks/decidim_attachments.rake index d504b3df3f77..eaf3551c4c47 100644 --- a/decidim-core/lib/tasks/decidim_attachments.rake +++ b/decidim-core/lib/tasks/decidim_attachments.rake @@ -4,7 +4,7 @@ namespace :decidim do namespace :attachments do desc "Cleanup the orphaned blobs attachments" task cleanup: :environment do - ActiveStorage::Blob.unattached.where(ActiveStorage::Blob.arel_table[:created_at].lteq(1.hour.ago)).find_each(&:purge_later) + ActiveStorage::Blob.unattached.where(ActiveStorage::Blob.arel_table[:created_at].lteq(Decidim.clean_up_unattached_blobs_after.ago)).find_each(&:purge_later) end end end diff --git a/decidim-core/spec/controllers/decidim/direct_uploads_controller_spec.rb b/decidim-core/spec/controllers/active_storage/direct_uploads_controller_spec.rb similarity index 99% rename from decidim-core/spec/controllers/decidim/direct_uploads_controller_spec.rb rename to decidim-core/spec/controllers/active_storage/direct_uploads_controller_spec.rb index c2bae13984d2..7bf6cd118542 100644 --- a/decidim-core/spec/controllers/decidim/direct_uploads_controller_spec.rb +++ b/decidim-core/spec/controllers/active_storage/direct_uploads_controller_spec.rb @@ -2,7 +2,7 @@ require "spec_helper" -module Decidim +module ActiveStorage describe DirectUploadsController do describe "POST #create" do let(:checksum) { OpenSSL::Digest.base64digest("MD5", "Hello") } diff --git a/decidim-generators/lib/decidim/generators/app_templates/initializer.rb b/decidim-generators/lib/decidim/generators/app_templates/initializer.rb index 3058c63c4487..f596a852f135 100644 --- a/decidim-generators/lib/decidim/generators/app_templates/initializer.rb +++ b/decidim-generators/lib/decidim/generators/app_templates/initializer.rb @@ -382,6 +382,10 @@ # Read more: https://docs.decidim.org/en/develop/configure/initializer#_content_security_policy config.content_security_policies_extra = {} + # This config parameter is used to set the default amount of time after which the unattached blobs + # are cleaned up. The default value is 60 minutes. + # config.clean_up_unattached_blobs_after = 60.minutes + # Admin admin password configurations Rails.application.secrets.dig(:decidim, :admin_password, :strong).tap do |strong_pw| # When the strong password is not configured, default to true From 31a384b667c50ecceced6a1c25464b6cf92beb03 Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Wed, 17 Apr 2024 08:58:15 +0300 Subject: [PATCH 16/23] Fix specs --- decidim-core/lib/decidim/organization_settings.rb | 6 ++++-- decidim-core/spec/lib/organization_settings_spec.rb | 7 +++++-- decidim-dev/lib/decidim/dev/assets/invalid_extension.log | 1 + .../commands/decidim/forms/answer_questionnaire_spec.rb | 2 +- .../commands/decidim/initiatives/update_initiative_spec.rb | 2 +- 5 files changed, 12 insertions(+), 6 deletions(-) create mode 100644 decidim-dev/lib/decidim/dev/assets/invalid_extension.log diff --git a/decidim-core/lib/decidim/organization_settings.rb b/decidim-core/lib/decidim/organization_settings.rb index 5894a291141a..9cfe64632036 100644 --- a/decidim-core/lib/decidim/organization_settings.rb +++ b/decidim-core/lib/decidim/organization_settings.rb @@ -104,8 +104,8 @@ def defaults_hash { "upload" => { "allowed_file_extensions" => { - "default" => %w(jpg jpeg png webp pdf rtf txt csv), - "admin" => %w(jpg jpeg png webp pdf doc docx xls xlsx ppt pptx ppx rtf txt odt ott odf otg ods ots csv json), + "default" => %w(jpg jpeg png webp pdf rtf txt csv md), + "admin" => %w(jpg jpeg png webp pdf doc docx xls xlsx ppt pptx ppx rtf txt odt ott odf otg ods ots csv json md), "image" => %w(jpg jpeg png webp) }, "allowed_content_types" => { @@ -113,6 +113,7 @@ def defaults_hash image/* application/pdf application/rtf + text/markdown text/plain text/csv ), @@ -127,6 +128,7 @@ def defaults_hash application/pdf application/rtf application/json + text/markdown text/plain text/csv ) diff --git a/decidim-core/spec/lib/organization_settings_spec.rb b/decidim-core/spec/lib/organization_settings_spec.rb index 94b193a981c7..18e53c557e26 100644 --- a/decidim-core/spec/lib/organization_settings_spec.rb +++ b/decidim-core/spec/lib/organization_settings_spec.rb @@ -11,8 +11,8 @@ module Decidim let(:default_settings) do { "allowed_file_extensions" => { - "default" => %w(jpg jpeg png webp pdf rtf txt), - "admin" => %w(jpg jpeg png webp pdf doc docx xls xlsx ppt pptx ppx rtf txt odt ott odf otg ods ots), + "default" => %w(jpg jpeg png webp pdf rtf txt csv), + "admin" => %w(jpg jpeg png webp pdf doc docx xls xlsx ppt pptx ppx rtf txt odt ott odf otg ods ots csv json), "image" => %w(jpg jpeg png webp) }, "allowed_content_types" => { @@ -21,6 +21,7 @@ module Decidim application/pdf application/rtf text/plain + text/csv ), "admin" => %w( image/* @@ -32,7 +33,9 @@ module Decidim application/vnd.oasis.opendocument application/pdf application/rtf + application/json text/plain + text/csv ) }, "maximum_file_size" => { diff --git a/decidim-dev/lib/decidim/dev/assets/invalid_extension.log b/decidim-dev/lib/decidim/dev/assets/invalid_extension.log new file mode 100644 index 000000000000..87b10bd8e638 --- /dev/null +++ b/decidim-dev/lib/decidim/dev/assets/invalid_extension.log @@ -0,0 +1 @@ +FooBar diff --git a/decidim-forms/spec/commands/decidim/forms/answer_questionnaire_spec.rb b/decidim-forms/spec/commands/decidim/forms/answer_questionnaire_spec.rb index a07a5c51a43c..5b82380919f2 100644 --- a/decidim-forms/spec/commands/decidim/forms/answer_questionnaire_spec.rb +++ b/decidim-forms/spec/commands/decidim/forms/answer_questionnaire_spec.rb @@ -168,7 +168,7 @@ def tokenize(id) }, { title: "CSV document", - file: upload_test_file(Decidim::Dev.asset("verify_user_groups.csv"), content_type: "text/csv") + file: upload_test_file(Decidim::Dev.asset("invalid_extension.log"), content_type: "text/plain") } ] end diff --git a/decidim-initiatives/spec/commands/decidim/initiatives/update_initiative_spec.rb b/decidim-initiatives/spec/commands/decidim/initiatives/update_initiative_spec.rb index ff8417c50479..555ccea97689 100644 --- a/decidim-initiatives/spec/commands/decidim/initiatives/update_initiative_spec.rb +++ b/decidim-initiatives/spec/commands/decidim/initiatives/update_initiative_spec.rb @@ -165,7 +165,7 @@ module Initiatives let(:uploaded_files) do [ upload_test_file(Decidim::Dev.test_file("city.jpeg", "image/jpeg")), - upload_test_file(Decidim::Dev.test_file("verify_user_groups.csv", "text/csv")) + upload_test_file(Decidim::Dev.test_file("invalid_extension.log", "text/plain")) ] end From 74724a202745ea6551bbb5b9a2fbd6af8a0dfd47 Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Wed, 12 Jun 2024 23:35:15 +0300 Subject: [PATCH 17/23] Convert Blob field to Blob type --- decidim-core/app/commands/decidim/validate_upload.rb | 3 +-- decidim-core/app/forms/decidim/upload_validation_form.rb | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/decidim-core/app/commands/decidim/validate_upload.rb b/decidim-core/app/commands/decidim/validate_upload.rb index cdea7c6dac7a..1abbde93bbbd 100644 --- a/decidim-core/app/commands/decidim/validate_upload.rb +++ b/decidim-core/app/commands/decidim/validate_upload.rb @@ -18,8 +18,7 @@ def call private def remove_invalid_file - blob = ActiveStorage::Blob.find_signed(@form.blob) - blob.purge if blob.present? + @form.blob.purge if @form.blob.present? end end end diff --git a/decidim-core/app/forms/decidim/upload_validation_form.rb b/decidim-core/app/forms/decidim/upload_validation_form.rb index e4741c58191c..1bc2ae09b736 100644 --- a/decidim-core/app/forms/decidim/upload_validation_form.rb +++ b/decidim-core/app/forms/decidim/upload_validation_form.rb @@ -10,7 +10,7 @@ class UploadValidationForm < Decidim::Form # Property is named as attribute in upload modal and passthru validator, but # it cannot be named as attribute here. attribute :property, String - attribute :blob, String + attribute :blob, Decidim::Attributes::Blob attribute :form_class, String validates :resource_class, presence: true From 7ea1bbe099c9a9a96d8aef1468325c70ce1c58a4 Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Thu, 13 Jun 2024 00:04:54 +0300 Subject: [PATCH 18/23] Remove CSV from allowed types --- decidim-core/lib/decidim/organization_settings.rb | 3 +-- decidim-core/spec/lib/organization_settings_spec.rb | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/decidim-core/lib/decidim/organization_settings.rb b/decidim-core/lib/decidim/organization_settings.rb index 8986afc08452..fdcf575a51f3 100644 --- a/decidim-core/lib/decidim/organization_settings.rb +++ b/decidim-core/lib/decidim/organization_settings.rb @@ -104,7 +104,7 @@ def defaults_hash { "upload" => { "allowed_file_extensions" => { - "default" => %w(jpg jpeg png webp pdf rtf txt csv md), + "default" => %w(jpg jpeg png webp pdf rtf txt md), "admin" => %w(jpg jpeg png webp pdf doc docx xls xlsx ppt pptx ppx rtf txt odt ott odf otg ods ots csv json md), "image" => %w(jpg jpeg png webp) }, @@ -115,7 +115,6 @@ def defaults_hash application/rtf text/markdown text/plain - text/csv ), "admin" => %w( image/* diff --git a/decidim-core/spec/lib/organization_settings_spec.rb b/decidim-core/spec/lib/organization_settings_spec.rb index ab84ec97ab23..b66d66d1e69e 100644 --- a/decidim-core/spec/lib/organization_settings_spec.rb +++ b/decidim-core/spec/lib/organization_settings_spec.rb @@ -11,7 +11,7 @@ module Decidim let(:default_settings) do { "allowed_file_extensions" => { - "default" => %w(jpg jpeg png webp pdf rtf txt csv), + "default" => %w(jpg jpeg png webp pdf rtf txt), "admin" => %w(jpg jpeg png webp pdf doc docx xls xlsx ppt pptx ppx rtf txt odt ott odf otg ods ots csv json), "image" => %w(jpg jpeg png webp) }, @@ -21,7 +21,6 @@ module Decidim application/pdf application/rtf text/plain - text/csv ), "admin" => %w( image/* From ee2f2aaa12b4c78909e538596e926718899e9130 Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Thu, 13 Jun 2024 00:16:41 +0300 Subject: [PATCH 19/23] A bit more security --- decidim-core/app/commands/decidim/validate_upload.rb | 2 +- .../app/controllers/concerns/decidim/direct_upload.rb | 11 ++++++++--- decidim-core/lib/tasks/decidim_attachments.rake | 4 +++- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/decidim-core/app/commands/decidim/validate_upload.rb b/decidim-core/app/commands/decidim/validate_upload.rb index 1abbde93bbbd..149ce190e31d 100644 --- a/decidim-core/app/commands/decidim/validate_upload.rb +++ b/decidim-core/app/commands/decidim/validate_upload.rb @@ -18,7 +18,7 @@ def call private def remove_invalid_file - @form.blob.purge if @form.blob.present? + @form.blob.purge_later if @form.blob.present? end end end diff --git a/decidim-core/app/controllers/concerns/decidim/direct_upload.rb b/decidim-core/app/controllers/concerns/decidim/direct_upload.rb index 6be4849e89a3..7b08c2749337 100644 --- a/decidim-core/app/controllers/concerns/decidim/direct_upload.rb +++ b/decidim-core/app/controllers/concerns/decidim/direct_upload.rb @@ -8,9 +8,10 @@ module DirectUpload include Decidim::NeedsOrganization skip_before_action :verify_organization - before_action :check_organization! - before_action :check_authenticated! - before_action :validate_direct_upload + before_action :check_organization!, + :check_authenticated!, + :check_user_belongs_to_organization, + :validate_direct_upload end protected @@ -46,6 +47,10 @@ def check_authenticated! head :unauthorized if current_user.blank? && current_admin.blank? end + def check_user_belongs_to_organization + head :unauthorized unless current_organization == current_user.organization + end + def allowed_extensions if user_has_elevated_role? current_organization.settings.upload_allowed_file_extensions_admin diff --git a/decidim-core/lib/tasks/decidim_attachments.rake b/decidim-core/lib/tasks/decidim_attachments.rake index eaf3551c4c47..920d6e518cec 100644 --- a/decidim-core/lib/tasks/decidim_attachments.rake +++ b/decidim-core/lib/tasks/decidim_attachments.rake @@ -4,7 +4,9 @@ namespace :decidim do namespace :attachments do desc "Cleanup the orphaned blobs attachments" task cleanup: :environment do - ActiveStorage::Blob.unattached.where(ActiveStorage::Blob.arel_table[:created_at].lteq(Decidim.clean_up_unattached_blobs_after.ago)).find_each(&:purge_later) + return if Decidim.clean_up_unattached_blobs_after.to_i.zero? + + ActiveStorage::Blob.unattached.where(created_at: ..Decidim.clean_up_unattached_blobs_after.ago).find_each(batch_size: 100, &:purge_later) end end end From edc60c1fee5a0a1ff87e2348bd872962b42bb58a Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Thu, 13 Jun 2024 14:50:12 +0300 Subject: [PATCH 20/23] Fix specs --- RELEASE_NOTES.md | 1 + .../app/controllers/concerns/decidim/direct_upload.rb | 6 ++++-- .../app/packs/src/decidim/direct_uploads/upload_modal.js | 3 +++ decidim-core/spec/lib/organization_settings_spec.rb | 6 ++++-- decidim-proposals/spec/system/edit_proposal_spec.rb | 4 ++-- 5 files changed, 14 insertions(+), 6 deletions(-) diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 21ef51370636..d832ecae2db3 100644 --- a/RELEASE_NOTES.md +++ b/RELEASE_NOTES.md @@ -181,6 +181,7 @@ bundle exec decidim:attachments:cleanup You can see more details about this change on PR [\#11851](https://github.com/decidim/decidim/pull/11851) ### 3.8. [[TITLE OF THE ACTION]] + You can read more about this change on PR [\#XXXX](https://github.com/decidim/decidim/pull/XXXX). ## 4. Scheduled tasks diff --git a/decidim-core/app/controllers/concerns/decidim/direct_upload.rb b/decidim-core/app/controllers/concerns/decidim/direct_upload.rb index 7b08c2749337..bc057462b375 100644 --- a/decidim-core/app/controllers/concerns/decidim/direct_upload.rb +++ b/decidim-core/app/controllers/concerns/decidim/direct_upload.rb @@ -21,14 +21,14 @@ def verify_organization; end def validate_direct_upload return if current_admin.present? - head :unprocessable_entity unless [ + render json: [errors: ["Foo bar"] ], status: :unprocessable_entity unless [ maximum_allowed_size.try(:to_i) >= blob_args[:byte_size].try(:to_i), content_types.any? { |pattern| pattern.match?(blob_args[:content_type]) }, content_types.any? { |pattern| pattern.match?(MiniMime.lookup_by_extension(extension)&.content_type) }, allowed_extensions.any? { |pattern| pattern.match?(extension) } ].all? rescue NoMethodError - head :unprocessable_entity + render json: [errors: ["Foo bar3 "] ], status: :unprocessable_entity end def extension @@ -48,6 +48,8 @@ def check_authenticated! end def check_user_belongs_to_organization + return if current_admin.present? + head :unauthorized unless current_organization == current_user.organization end diff --git a/decidim-core/app/packs/src/decidim/direct_uploads/upload_modal.js b/decidim-core/app/packs/src/decidim/direct_uploads/upload_modal.js index ff1b752bf451..1940f6e26b3c 100644 --- a/decidim-core/app/packs/src/decidim/direct_uploads/upload_modal.js +++ b/decidim-core/app/packs/src/decidim/direct_uploads/upload_modal.js @@ -77,6 +77,9 @@ export default class UploadModal { uploader.upload.create((error, blob) => { if (error) { uploader.errors = [error] + this.uploadItems.replaceChild(this.createUploadItem(file, [error], { value: 100 }), item); + this.updateDropZone(); + } else { // attach the file hash to submit the form, when the file has been uploaded file.hiddenField = blob.signed_id diff --git a/decidim-core/spec/lib/organization_settings_spec.rb b/decidim-core/spec/lib/organization_settings_spec.rb index b66d66d1e69e..cb282d1af7d1 100644 --- a/decidim-core/spec/lib/organization_settings_spec.rb +++ b/decidim-core/spec/lib/organization_settings_spec.rb @@ -11,8 +11,8 @@ module Decidim let(:default_settings) do { "allowed_file_extensions" => { - "default" => %w(jpg jpeg png webp pdf rtf txt), - "admin" => %w(jpg jpeg png webp pdf doc docx xls xlsx ppt pptx ppx rtf txt odt ott odf otg ods ots csv json), + "default" => %w(jpg jpeg png webp pdf rtf txt md), + "admin" => %w(jpg jpeg png webp pdf doc docx xls xlsx ppt pptx ppx rtf txt odt ott odf otg ods ots csv json md), "image" => %w(jpg jpeg png webp) }, "allowed_content_types" => { @@ -20,6 +20,7 @@ module Decidim image/* application/pdf application/rtf + text/markdown text/plain ), "admin" => %w( @@ -33,6 +34,7 @@ module Decidim application/pdf application/rtf application/json + text/markdown text/plain text/csv ) diff --git a/decidim-proposals/spec/system/edit_proposal_spec.rb b/decidim-proposals/spec/system/edit_proposal_spec.rb index 0b6a570499c7..40a2234f5e40 100644 --- a/decidim-proposals/spec/system/edit_proposal_spec.rb +++ b/decidim-proposals/spec/system/edit_proposal_spec.rb @@ -52,10 +52,10 @@ it "shows validation error when format is not accepted" do click_on "Edit proposal" - dynamically_attach_file(:proposal_documents, Decidim::Dev.asset("participatory_text.md"), keep_modal_open: true) do + dynamically_attach_file(:proposal_documents, Decidim::Dev.asset("dummy-dummies-example.xlsx"), keep_modal_open: true) do expect(page).to have_content("Accepted formats: #{Decidim::OrganizationSettings.for(organization).upload_allowed_file_extensions_image.join(", ")}") end - expect(page).to have_content("only files with the following extensions are allowed: jpeg, jpg, pdf, png, rtf, txt") + expect(page).to have_content("Validation error!") end context "with a file and photo" do From b89acbdea6740dd54e1e0b62fe82a0191dab8f3f Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Thu, 13 Jun 2024 15:00:06 +0300 Subject: [PATCH 21/23] More fixes --- .../app/controllers/concerns/decidim/direct_upload.rb | 6 +++--- .../spec/system/meeting_registrations_spec.rb | 8 ++------ 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/decidim-core/app/controllers/concerns/decidim/direct_upload.rb b/decidim-core/app/controllers/concerns/decidim/direct_upload.rb index bc057462b375..a951b06f1b22 100644 --- a/decidim-core/app/controllers/concerns/decidim/direct_upload.rb +++ b/decidim-core/app/controllers/concerns/decidim/direct_upload.rb @@ -21,14 +21,14 @@ def verify_organization; end def validate_direct_upload return if current_admin.present? - render json: [errors: ["Foo bar"] ], status: :unprocessable_entity unless [ + head :unprocessable_entity unless [ maximum_allowed_size.try(:to_i) >= blob_args[:byte_size].try(:to_i), content_types.any? { |pattern| pattern.match?(blob_args[:content_type]) }, content_types.any? { |pattern| pattern.match?(MiniMime.lookup_by_extension(extension)&.content_type) }, allowed_extensions.any? { |pattern| pattern.match?(extension) } ].all? rescue NoMethodError - render json: [errors: ["Foo bar3 "] ], status: :unprocessable_entity + head :unprocessable_entity end def extension @@ -49,7 +49,7 @@ def check_authenticated! def check_user_belongs_to_organization return if current_admin.present? - + head :unauthorized unless current_organization == current_user.organization end diff --git a/decidim-meetings/spec/system/meeting_registrations_spec.rb b/decidim-meetings/spec/system/meeting_registrations_spec.rb index 90cd57f1f94a..e44ebc13a239 100644 --- a/decidim-meetings/spec/system/meeting_registrations_spec.rb +++ b/decidim-meetings/spec/system/meeting_registrations_spec.rb @@ -301,13 +301,9 @@ def questionnaire_public_path it "shows errors for invalid file" do visit questionnaire_public_path - dynamically_attach_file("questionnaire_responses_0_add_documents", Decidim::Dev.asset("verify_user_groups.csv")) + dynamically_attach_file("questionnaire_responses_0_add_documents", Decidim::Dev.asset("verify_user_groups.csv"), keep_modal_open: true) - expect(page).to have_field("public_participation", checked: false) - find_by_id("questionnaire_tos_agreement").set(true) - accept_confirm { click_on "Submit" } - - expect(page).to have_content("Needs to be reattached") + expect(page).to have_content("Validation error!") end context "and the announcement for the meeting is configured" do From 5fd8c0098135c7b33dabcc9f01759c58a8fe2cc8 Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Thu, 13 Jun 2024 16:16:15 +0300 Subject: [PATCH 22/23] Fix Validator upload --- decidim-core/spec/commands/decidim/validate_upload_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/decidim-core/spec/commands/decidim/validate_upload_spec.rb b/decidim-core/spec/commands/decidim/validate_upload_spec.rb index 450d1be1ccf5..8ffd3f168a58 100644 --- a/decidim-core/spec/commands/decidim/validate_upload_spec.rb +++ b/decidim-core/spec/commands/decidim/validate_upload_spec.rb @@ -15,7 +15,8 @@ module Decidim end let(:invalid) { false } let(:errors) { [] } - let(:blob) { "foobar" } + let(:io) { Decidim::Dev.test_file("city.jpeg", "image/jpeg") } + let(:blob) { ActiveStorage::Blob.create_and_upload!(io: , filename: "city.jpeg") } describe "when form is valid" do it "broadcasts ok" do @@ -33,12 +34,11 @@ module Decidim let(:errors) { ["File too dummy"] } it "broadcasts invalid" do - allow(ActiveStorage::Blob).to receive(:find_signed).with(blob).and_return(double(purge: true)) expect { command.call }.to broadcast(:invalid, errors) end it "removes the invalid file" do - expect(ActiveStorage::Blob).to receive(:find_signed) + expect(blob).to receive(:purge_later) command.call end end From 0aa5de1587112cf660ac5db6b9feb3a0c9e5f0c7 Mon Sep 17 00:00:00 2001 From: Alexandru Emil Lupu Date: Thu, 13 Jun 2024 16:21:21 +0300 Subject: [PATCH 23/23] lint --- decidim-core/spec/commands/decidim/validate_upload_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/decidim-core/spec/commands/decidim/validate_upload_spec.rb b/decidim-core/spec/commands/decidim/validate_upload_spec.rb index 8ffd3f168a58..06edda7fd298 100644 --- a/decidim-core/spec/commands/decidim/validate_upload_spec.rb +++ b/decidim-core/spec/commands/decidim/validate_upload_spec.rb @@ -16,7 +16,7 @@ module Decidim let(:invalid) { false } let(:errors) { [] } let(:io) { Decidim::Dev.test_file("city.jpeg", "image/jpeg") } - let(:blob) { ActiveStorage::Blob.create_and_upload!(io: , filename: "city.jpeg") } + let(:blob) { ActiveStorage::Blob.create_and_upload!(io:, filename: "city.jpeg") } describe "when form is valid" do it "broadcasts ok" do