fix: attachment upload authorization in attachments controller (#4520)

* Enhance attachment upload authorization in attachments controller

* run request specs

Author: Paul-Bob

.github/workflows/feature-tests.yml

@@ -98,7 +98,9 @@ jobs:
 
     - name: Run tests
       id: run_tests
-      run: bundle exec rspec spec/features
+      run: |
+        bundle exec rspec spec/features
+        bundle exec rspec spec/requests
 
     - uses: actions/upload-artifact@v4
       with:

app/controllers/avo/attachments_controller.rb

@@ -7,14 +7,18 @@ class AttachmentsController < ApplicationController
     before_action :set_record, only: [:destroy, :create]
 
     def create
-      blob = ActiveStorage::Blob.create_and_upload! io: params[:file].to_io, filename: params[:filename]
       association_name = BaseResource.valid_attachment_name(@record, params[:attachment_key])
 
-      # If association name is present attach the blob to it
       if association_name
+        return render_upload_unauthorized unless authorized_to_upload(association_name)
+
+        blob = ActiveStorage::Blob.create_and_upload! io: params[:file].to_io, filename: params[:filename]
         @record.send(association_name).attach blob
-      # If key is present use the blob from the key else raise error
-      elsif params[:key].blank?
+      elsif params[:key].present?
+        return render_upload_unauthorized unless authorized_to_trix_upload?
+
+        blob = ActiveStorage::Blob.create_and_upload! io: params[:file].to_io, filename: params[:filename]
+      else
         raise ActionController::BadRequest.new("Could not find the attachment association for #{params[:attachment_key]} (check the `attachment_key` for this Trix field)")
       end
 
@@ -63,5 +67,17 @@ def destroy
     def authorized_to(action)
       @resource.authorization.authorize_action("#{action}_#{params[:attachment_name]}?", record: @record, raise_exception: false)
     end
+
+    def authorized_to_upload(attachment_name)
+      @resource.authorization.authorize_action("upload_#{attachment_name}?", record: @record, raise_exception: false)
+    end
+
+    def authorized_to_trix_upload?
+      @resource.authorization.authorize_action("update?", record: @record, raise_exception: false)
+    end
+
+    def render_upload_unauthorized
+      render json: {error: "Not authorized"}, status: :forbidden
+    end
   end
 end

spec/requests/avo/attachments_request_spec.rb

@@ -1,4 +1,99 @@
 require "rails_helper"
 
 RSpec.describe "Attachments", type: :request do
+  include ActionDispatch::TestProcess
+
+  let(:admin_user) do
+    User.create!(
+      first_name: "Admin",
+      last_name: "User",
+      email: "admin@example.com",
+      password: "password",
+      roles: {"admin" => true}
+    )
+  end
+
+  let(:post_record) do
+    Post.create!(name: "Hello", body: "World", user: admin_user)
+  end
+
+  def build_upload(content:, original_filename:, content_type:)
+    tmp = Tempfile.new(["upload", File.extname(original_filename)])
+    tmp.binmode
+    tmp.write(content)
+    tmp.rewind
+
+    Rack::Test::UploadedFile.new(tmp.path, content_type, original_filename: original_filename)
+  end
+
+  it "denies has_one_attached upload when upload_cover_photo? is false" do
+    sign_in admin_user
+
+    post_record.cover_photo.attach(
+      io: StringIO.new("old"),
+      filename: "old.txt",
+      content_type: "text/plain"
+    )
+
+    allow_any_instance_of(Avo::Services::AuthorizationService)
+      .to receive(:authorize_action)
+      .with("upload_cover_photo?", record: post_record, raise_exception: false)
+      .and_return(false)
+
+    upload = build_upload(content: "new", original_filename: "new.txt", content_type: "text/plain")
+    old_blob_id = post_record.reload.cover_photo.blob_id
+    old_checksum = post_record.cover_photo.blob.checksum
+
+    expect {
+      post "/admin/avo_api/resources/posts/#{post_record.to_param}/attachments",
+        params: {file: upload, filename: "new.txt", attachment_key: "cover_photo"},
+        headers: {"ACCEPT" => "application/json"}
+    }.not_to change { ActiveStorage::Blob.count }
+
+    post_record.reload
+    expect(post_record.cover_photo).to be_attached
+    expect(post_record.cover_photo.blob_id).to eq(old_blob_id)
+    expect(post_record.cover_photo.blob.checksum).to eq(old_checksum)
+
+    expect(response).to have_http_status(:forbidden)
+  end
+
+  it "denies has_many_attached upload when upload_attachments? is false" do
+    sign_in admin_user
+
+    allow_any_instance_of(Avo::Services::AuthorizationService)
+      .to receive(:authorize_action)
+      .with("upload_attachments?", record: post_record, raise_exception: false)
+      .and_return(false)
+
+    upload = build_upload(content: "one", original_filename: "one.txt", content_type: "text/plain")
+
+    expect {
+      post "/admin/avo_api/resources/posts/#{post_record.to_param}/attachments",
+        params: {file: upload, filename: "one.txt", attachment_key: "attachments"},
+        headers: {"ACCEPT" => "application/json"}
+    }.not_to change { ActiveStorage::Blob.count }
+
+    expect(post_record.reload.attachments.count).to eq(0)
+    expect(response).to have_http_status(:forbidden)
+  end
+
+  it "denies key/Trix-style upload when no attachment association is resolved" do
+    sign_in admin_user
+
+    allow_any_instance_of(Avo::Services::AuthorizationService)
+      .to receive(:authorize_action)
+      .with("update?", record: post_record, raise_exception: false)
+      .and_return(false)
+
+    upload = build_upload(content: "trix", original_filename: "trix.txt", content_type: "text/plain")
+
+    expect {
+      post "/admin/avo_api/resources/posts/#{post_record.to_param}/attachments",
+        params: {file: upload, filename: "trix.txt", attachment_key: "missing_field", key: "trixKey"},
+        headers: {"ACCEPT" => "application/json"}
+    }.not_to change { ActiveStorage::Blob.count }
+
+    expect(response).to have_http_status(:forbidden)
+  end
 end