Merge pull request #3859 from AlchemyCMS/backport/8.2-stable/pr-3855

[8.2-stable] Fix Attachment#deletable? false positives

Author: tvdeyen

app/controllers/alchemy/admin/attachments_controller.rb

@@ -39,6 +39,15 @@ def index
           .page(params[:page] || 1)
           .per(items_per_page)
 
+        # Preload deletable ids for the current page in a single query so the
+        # view can decide which delete buttons to enable without calling
+        # +deletable?+ (a two-query check) per row. We pass the already-loaded
+        # ids (via +map(&:id)+) rather than the relation itself, because passing
+        # a paginated relation produces +IN (SELECT ... LIMIT n)+, which older
+        # MariaDB versions reject.
+        @deletable_attachment_ids =
+          Attachment.where(id: @attachments.map(&:id)).deletable.pluck(:id).to_set
+
         if in_overlay?
           archive_overlay
         end

app/controllers/alchemy/admin/pictures_controller.rb

@@ -8,6 +8,8 @@ class PicturesController < Alchemy::Admin::ResourcesController
       include CurrentLanguage
       include PictureDescriptionsFormHelper
 
+      before_action :load_pictures, only: :index
+
       before_action :load_resource,
         only: [:edit, :update, :url, :destroy]
 
@@ -19,6 +21,12 @@ class PicturesController < Alchemy::Admin::ResourcesController
         @picture = Picture.find(params[:id])
       end
 
+      # Preload deletable ids for the current page (index) or the current
+      # resource (update, which re-renders the +_picture+ partial via a
+      # turbo stream). One query replaces the per-row +deletable?+ check
+      # that would otherwise fire for every picture the view renders.
+      before_action :load_deletable_picture_ids, only: [:index, :update]
+
       add_alchemy_filter :by_file_format, type: :select, options: ->(query) do
         Alchemy::Picture.file_formats(query.result)
       end
@@ -30,8 +38,6 @@ class PicturesController < Alchemy::Admin::ResourcesController
       helper_method :picture_offset
 
       def index
-        @pictures = filtered_pictures(page: params[:page])
-
         if in_overlay?
           archive_overlay
         end
@@ -159,6 +165,21 @@ def items_per_page_options
 
       private
 
+      def load_pictures
+        @pictures = filtered_pictures(page: params[:page])
+      end
+
+      # Preload deletable ids in a single query so the view can decide which
+      # delete buttons to enable without calling +deletable?+ (a two-query
+      # check) per row. We pass already-loaded ids (via +map(&:id)+) rather
+      # than the relation itself, because passing a paginated relation
+      # produces +IN (SELECT ... LIMIT n)+, which older MariaDB versions
+      # reject.
+      def load_deletable_picture_ids
+        ids = @pictures ? @pictures.map(&:id) : [@picture.id]
+        @deletable_picture_ids = Picture.where(id: ids).deletable.pluck(:id).to_set
+      end
+
       def picture_offset
         ((params[:page] || 1).to_i - 1) * items_per_page
       end

app/models/alchemy/attachment.rb

@@ -42,6 +42,33 @@ class Attachment < BaseRecord
     scope :recent, -> { where("#{table_name}.created_at > ?", Time.current - 24.hours).order(:created_at) }
     scope :without_tag, -> { left_outer_joins(:taggings).where(gutentag_taggings: {id: nil}) }
 
+    # Override +Alchemy::RelatableResource#deletable+ to also exclude
+    # attachments referenced from +/attachment/:id/download+ URLs inside
+    # ingredient values (e.g. Richtext markup, Link ingredients, raw Html).
+    # Those URLs are written by the file tab of the link dialog and are
+    # not tracked via the polymorphic +related_object+ association, so the
+    # base scope cannot see them.
+    #
+    # Uses a correlated +NOT EXISTS+ subquery that builds the per-row LIKE
+    # pattern with +Arel::Nodes::Concat+, which compiles to +||+ on
+    # SQLite/PostgreSQL and +CONCAT()+ on MySQL.
+    scope :deletable, -> do
+      ingredients = Alchemy::Ingredient.arel_table
+      pattern = Arel::Nodes::Concat.new(
+        Arel::Nodes::Concat.new(
+          Arel::Nodes.build_quoted("%/attachment/"),
+          arel_table[:id]
+        ),
+        Arel::Nodes.build_quoted("/download%")
+      )
+      referenced = ingredients
+        .project(1)
+        .where(ingredients[:value].matches(pattern))
+
+      where("#{table_name}.id NOT IN (#{RelatableResource::RELATED_INGREDIENTS_SUBQUERY})", type: name)
+        .where.not(referenced.exists)
+    end
+
     # We need to define this method here to have it available in the validations below.
     class << self
       # The class used to generate URLs for attachments
@@ -112,6 +139,13 @@ def slug
       CGI.escape(file_name.gsub(/\.#{extension}$/, "").tr(".", " "))
     end
 
+    # Override +Alchemy::RelatableResource#deletable?+ to also consider
+    # +/attachment/:id/download+ links inside ingredient values (e.g.
+    # Richtext markup, Link ingredients, raw Html).
+    def deletable?
+      super && !referenced_in_ingredient_value?
+    end
+
     # Checks if the attachment is restricted, because it is attached on restricted pages only
     def restricted?
       related_pages.any? && related_pages.not_restricted.blank?
@@ -178,6 +212,12 @@ def file_type_allowed
       end
     end
 
+    def referenced_in_ingredient_value?
+      Alchemy::Ingredient
+        .where("value LIKE ?", "%/attachment/#{id}/download%")
+        .exists?
+    end
+
     def set_name
       self.name ||= Alchemy.storage_adapter.file_basename(self).humanize
     end

app/models/concerns/alchemy/relatable_resource.rb

@@ -2,6 +2,19 @@ module Alchemy
   module RelatableResource
     extend ActiveSupport::Concern
 
+    # SQL subquery selecting +related_object_id+ values for ingredients that
+    # reference a given polymorphic type. Intended to be composed into a
+    # +NOT IN (...)+ clause by +deletable+ and any overrides in including
+    # classes. Takes one named bind :type - the polymorphic type name,
+    # typically the class name of the including model
+    # (e.g. +"Alchemy::Attachment"+).
+    RELATED_INGREDIENTS_SUBQUERY = <<~SQL.squish
+      SELECT related_object_id
+      FROM alchemy_ingredients
+      WHERE related_object_id IS NOT NULL
+        AND related_object_type = :type
+    SQL
+
     class_methods do
       # Preload associations for element editor display
       #
@@ -18,10 +31,7 @@ def alchemy_element_preloads(records)
 
     included do
       scope :deletable, -> do
-        where(
-          "#{table_name}.id NOT IN (SELECT related_object_id FROM alchemy_ingredients WHERE related_object_id IS NOT NULL AND related_object_type = ?)",
-          name
-        )
+        where("#{table_name}.id NOT IN (#{RELATED_INGREDIENTS_SUBQUERY})", type: name)
       end
 
       has_many :related_ingredients,

app/views/alchemy/admin/attachments/_files_list.html.erb

@@ -61,7 +61,7 @@
         file_attribute: 'file' %>
     <% end %>
     <% table.with_action(:destroy) do |attachment| %>
-      <% if attachment.deletable? %>
+      <% if @deletable_attachment_ids.include?(attachment.id) %>
         <sl-tooltip content="<%= Alchemy.t(:delete_file) %>">
           <%= link_to_confirm_dialog render_icon(:minus),
             Alchemy.t(:confirm_to_delete_file),

app/views/alchemy/admin/pictures/_picture.html.erb

@@ -2,7 +2,7 @@
   <span class="picture_tool select">
     <%= check_box_tag "picture_ids[]", picture.id %>
   </span>
-  <% if picture.deletable? && can?(:destroy, picture) %>
+  <% if @deletable_picture_ids.include?(picture.id) && can?(:destroy, picture) %>
   <div class="picture_tool delete">
     <sl-tooltip content="<%= Alchemy.t('Delete image') %>">
       <%= link_to_confirm_dialog(

spec/controllers/alchemy/admin/attachments_controller_spec.rb

@@ -70,6 +70,32 @@ module Alchemy
         end
       end
 
+      describe "@deletable_attachment_ids" do
+        let!(:deletable_attachment) { create(:alchemy_attachment) }
+        let!(:referenced_attachment) { create(:alchemy_attachment) }
+
+        before do
+          create(
+            :alchemy_ingredient_richtext,
+            value: %(<a href="/attachment/#{referenced_attachment.id}/download">x</a>)
+          )
+        end
+
+        it "is a Set of ids of attachments on the current page that are deletable" do
+          get :index
+          expect(assigns(:deletable_attachment_ids)).to be_a(Set)
+          expect(assigns(:deletable_attachment_ids)).to include(deletable_attachment.id)
+          expect(assigns(:deletable_attachment_ids)).not_to include(referenced_attachment.id)
+        end
+
+        it "is scoped to the current page only" do
+          get :index, params: {per_page: 1}
+          # With per_page: 1 only one attachment is on the current page,
+          # so the set must have at most one element.
+          expect(assigns(:deletable_attachment_ids).size).to be <= 1
+        end
+      end
+
       describe "by_file_type filter" do
         let!(:png) { create(:alchemy_attachment) }
 

spec/controllers/alchemy/admin/pictures_controller_spec.rb

@@ -136,6 +136,27 @@ module Alchemy
           end
         end
       end
+
+      describe "@deletable_picture_ids" do
+        let!(:deletable_picture) { create(:alchemy_picture) }
+        let!(:referenced_picture) { create(:alchemy_picture) }
+
+        before do
+          create(:alchemy_ingredient_picture, related_object: referenced_picture)
+        end
+
+        it "is a Set of ids of pictures on the current page that are deletable" do
+          get :index
+          expect(assigns(:deletable_picture_ids)).to be_a(Set)
+          expect(assigns(:deletable_picture_ids)).to include(deletable_picture.id)
+          expect(assigns(:deletable_picture_ids)).not_to include(referenced_picture.id)
+        end
+
+        it "is scoped to the current page only" do
+          get :index, params: {per_page: 1}
+          expect(assigns(:deletable_picture_ids).size).to be <= 1
+        end
+      end
     end
 
     describe "#create" do

spec/models/alchemy/attachment_spec.rb

@@ -21,6 +21,72 @@ module Alchemy
       resource_name: :attachment,
       ingredient_type: :file
 
+    describe ".deletable" do
+      let!(:richtext_linked_attachment) { create(:alchemy_attachment) }
+      let!(:link_linked_attachment) { create(:alchemy_attachment) }
+      let!(:unlinked_attachment) { create(:alchemy_attachment) }
+
+      before do
+        create(
+          :alchemy_ingredient_richtext,
+          value: %(<p>See <a href="/attachment/#{richtext_linked_attachment.id}/download/foo.pdf">this</a></p>)
+        )
+        create(
+          :alchemy_ingredient_link,
+          value: "/attachment/#{link_linked_attachment.id}/download"
+        )
+      end
+
+      it "excludes attachments linked from any ingredient download URL" do
+        expect(described_class.deletable).to include(unlinked_attachment)
+        expect(described_class.deletable).not_to include(richtext_linked_attachment)
+        expect(described_class.deletable).not_to include(link_linked_attachment)
+      end
+    end
+
+    describe "#deletable?" do
+      let(:attachment) { create(:alchemy_attachment) }
+
+      subject { attachment.deletable? }
+
+      context "when referenced by a /attachment/:id/download URL in a Richtext ingredient" do
+        before do
+          create(
+            :alchemy_ingredient_richtext,
+            value: %(<a href="/attachment/#{attachment.id}/download">x</a>)
+          )
+        end
+
+        it { is_expected.to be(false) }
+      end
+
+      context "when referenced by a /attachment/:id/download URL in a Link ingredient" do
+        before do
+          create(
+            :alchemy_ingredient_link,
+            value: "/attachment/#{attachment.id}/download"
+          )
+        end
+
+        it { is_expected.to be(false) }
+      end
+
+      context "when another ingredient links to a different attachment whose id shares a digit prefix" do
+        let(:other_id) { attachment.id * 10 }
+
+        before do
+          create(
+            :alchemy_ingredient_richtext,
+            value: %(<a href="/attachment/#{other_id}/download">x</a>)
+          )
+        end
+
+        it "does not falsely report as referenced" do
+          is_expected.to be(true)
+        end
+      end
+    end
+
     it "has file mime type accessor" do
       expect(attachment.file_mime_type).to eq("image/png")
     end