Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove invalid upload files #11851

Draft
wants to merge 31 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
43a5fc5
Remove orphaned blobs
alecslupu Oct 27, 2023
5d6ab94
Add Rake task to cleanup old unattached blobs
alecslupu Oct 27, 2023
bd7c937
Refactor task
alecslupu Oct 27, 2023
dcdba03
Add job work to remove unattached blobs after a period of time
alecslupu Nov 8, 2023
87ec9b4
Merge branch 'develop' of github.com:decidim/decidim into fix/attachm…
alecslupu Nov 8, 2023
f6323cc
Running linters
alecslupu Nov 8, 2023
1f0d755
Merge branch 'develop' of github.com:decidim/decidim into fix/attachm…
alecslupu Nov 10, 2023
d01bfce
Merge branch 'develop' of github.com:decidim/decidim into fix/attachm…
alecslupu Mar 8, 2024
84af5a6
Running linters
alecslupu Mar 8, 2024
b51842c
Fix spec
alecslupu Mar 8, 2024
e83826a
Merge branch 'develop' of github.com:decidim/decidim into fix/attachm…
alecslupu Mar 11, 2024
b9ace94
Fix spelling
alecslupu Mar 11, 2024
dd16d30
Merge branch 'develop' of github.com:decidim/decidim into fix/attachm…
alecslupu Apr 15, 2024
342751e
Remove the "decidim_core.clean_attachments" initializer and the Clean…
alecslupu Apr 15, 2024
3490ff8
Change the rake task so it prevents deleting blobs that are less than…
alecslupu Apr 15, 2024
63586fa
Change the Releases Notes so the task is run two times with one hour …
alecslupu Apr 15, 2024
2f38d9c
DirectUploadsController validations
alecslupu Apr 16, 2024
7bccc7e
Add specs
alecslupu Apr 16, 2024
2c961cd
Update RELEASE_NOTES.md
alecslupu Apr 16, 2024
8a9342b
Merge branch 'develop' into fix/attachment-validation-cleanup
alecslupu Apr 16, 2024
a544e43
Apply review recommendation
alecslupu Apr 16, 2024
31a384b
Fix specs
alecslupu Apr 17, 2024
dd28ea8
Merge branch 'develop' of github.com:decidim/decidim into fix/attachm…
alecslupu Jun 9, 2024
e03b5ba
Merge branch 'develop' of github.com:decidim/decidim into fix/attachm…
alecslupu Jun 12, 2024
74724a2
Convert Blob field to Blob type
alecslupu Jun 12, 2024
7ea1bbe
Remove CSV from allowed types
alecslupu Jun 12, 2024
ee2f2aa
A bit more security
alecslupu Jun 12, 2024
edc60c1
Fix specs
alecslupu Jun 13, 2024
b89acbd
More fixes
alecslupu Jun 13, 2024
5fd8c00
Fix Validator upload
alecslupu Jun 13, 2024
0aa5de1
lint
alecslupu Jun 13, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 7 additions & 5 deletions RELEASE_NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

alecslupu marked this conversation as resolved.
Show resolved Hide resolved
You can run the task with the following command:

Expand All @@ -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
Expand Down
55 changes: 55 additions & 0 deletions decidim-core/app/controllers/decidim/direct_uploads_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
# frozen_string_literal: true

module Decidim
class DirectUploadsController < ActiveStorage::DirectUploadsController
ahukkanen marked this conversation as resolved.
Show resolved Hide resolved
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
alecslupu marked this conversation as resolved.
Show resolved Hide resolved

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) }
ahukkanen marked this conversation as resolved.
Show resolved Hide resolved
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")
ahukkanen marked this conversation as resolved.
Show resolved Hide resolved
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")
ahukkanen marked this conversation as resolved.
Show resolved Hide resolved
current_organization.settings.upload_allowed_content_types_admin
else
current_organization.settings.upload_allowed_content_types
end
end
end
end
14 changes: 0 additions & 14 deletions decidim-core/app/jobs/decidim/clean_unattached_blob_job.rb

This file was deleted.

16 changes: 8 additions & 8 deletions decidim-core/lib/decidim/core/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
ahukkanen marked this conversation as resolved.
Show resolved Hide resolved

initializer "decidim_core.locales" do |app|
app.config.i18n.fallbacks = true
end
Expand Down Expand Up @@ -393,14 +401,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
Expand Down
2 changes: 1 addition & 1 deletion decidim-core/lib/tasks/decidim_attachments.rake
Original file line number Diff line number Diff line change
Expand Up @@ -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)
ahukkanen marked this conversation as resolved.
Show resolved Hide resolved
end
end
end
Original file line number Diff line number Diff line change
@@ -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
25 changes: 0 additions & 25 deletions decidim-core/spec/jobs/decidim/clean_unattached_blob_job_spec.rb

This file was deleted.

33 changes: 0 additions & 33 deletions decidim-core/spec/system/attachments_spec.rb

This file was deleted.