From d16161eab9029a509951146b80334c304c35fd34 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Sun, 9 Aug 2026 03:35:48 +0200 Subject: Refuse destroying an asset that is still attached --- app/helpers/node_actions_helper.rb | 17 ++----------- app/models/asset.rb | 45 ++++++++++------------------------ app/models/node_action.rb | 23 ++++++------------ config/locales/de.yml | 7 ++---- config/locales/en.yml | 7 ++---- test/models/asset_destroy_test.rb | 49 +++++++++++++------------------------- 6 files changed, 42 insertions(+), 106 deletions(-) diff --git a/app/helpers/node_actions_helper.rb b/app/helpers/node_actions_helper.rb index 960e28b3..09de4250 100644 --- a/app/helpers/node_actions_helper.rb +++ b/app/helpers/node_actions_helper.rb @@ -330,22 +330,9 @@ module NodeActionsHelper :asset => asset_ref(action)).html_safe end - def summarize_asset_attach action - m = action.metadata - key = m["headline"] ? "node_actions.asset_attach_headline" : "node_actions.asset_attach" - t(key, :actor => actor_ref(action), :subject => subject_ref(action), - :asset => asset_ref(action)).html_safe - end - def summarize_asset_destroy action - m = action.metadata - parts = [t("node_actions.asset_destroy", :actor => actor_ref(action), - :asset => asset_ref(action))] - parts << t("node_actions.asset_destroy_detached", - :paths => h(Array(m["detached_from"]).join(", "))) if m["detached_from"].present? - parts << t("node_actions.asset_destroy_headlines", - :paths => h(Array(m["headline_removed_from"]).join(", "))) if m["headline_removed_from"].present? - safe_join(parts, " ") + t("node_actions.asset_destroy", :actor => actor_ref(action), + :asset => asset_ref(action)).html_safe end def summarize_otp_enroll action diff --git a/app/models/asset.rb b/app/models/asset.rb index 4d43b18f..08ef5a78 100644 --- a/app/models/asset.rb +++ b/app/models/asset.rb @@ -41,44 +41,23 @@ class Asset < ApplicationRecord :ids => page_ids).distinct end - # An asset's reach is the reach of the pages carrying it: destroying one - # removes it from every live page at once, so a single restricted - # attachment makes the destruction a restricted act. - def restricted? - attached_nodes.any?(&:restricted?) - end - - # Witnessed destruction. Destroying an asset is a public-facing act - # even when unattached. The original and its variants are publicly - # reachable under /system/uploads, so an entry is always written, - # before the row and its files die. Every currently-attached node - # participates so its zoomed history shows the loss; the asset itself - # participates as the first non-Node subject (its participant row - # dangles after destroy, by design, the name lives on in metadata). + # Witnessed destruction, refused while the asset is attached to any + # current row. Detaching stays an in-editor act, so nothing + # destroyed here is carried by a page. The entry is still warranted: + # the original and its variants are reachable under /system/uploads + # until the row and its files die. def destroy_witnessed! user: - if user && !user.may_change_live?(self) - errors.add(:base, :not_permitted) + if attached_nodes.any? + errors.add(:base, :destroy_while_attached) raise ActiveRecord::RecordInvalid.new(self) end ActiveRecord::Base.transaction do - affected = attached_nodes.to_a - headline_losses = affected.select do |node| - [node.head, node.draft, node.autosave].compact.any? do |row| - row.related_assets.exists?(:asset_id => id, :headline => true) - end - end - - metadata = { - :asset_name => name, - :content_type => upload_content_type, - :path => upload.url.sub(/\?\d+$/, ""), - } - metadata[:detached_from] = affected.map(&:unique_name) if affected.any? - metadata[:headline_removed_from] = headline_losses.map(&:unique_name) if headline_losses.any? - - NodeAction.record!(:participants => [self] + affected, :user => user, - :action => "asset_destroy", **metadata) + NodeAction.record!(:participants => [self], :user => user, + :action => "asset_destroy", + :asset_name => name, + :content_type => upload_content_type, + :path => upload.url.sub(/\?\d+$/, "")) destroy! end end diff --git a/app/models/node_action.rb b/app/models/node_action.rb index 1e64861e..aab1d238 100644 --- a/app/models/node_action.rb +++ b/app/models/node_action.rb @@ -82,22 +82,13 @@ class NodeAction < ApplicationRecord # "asset_create" (witnessed upload; participants: the asset alone): # "asset_name", "content_type", "path" -- flat strings # - # "asset_attach" (out-of-band attach via Node#attach_asset!; written - # only when at least one new join was created, per the tandem rule -- - # in-editor curation stays draft-scoped and surfaces at publish. - # participants: the node (primary) and the asset): - # "asset_name", "path" -- flat strings - # "headline" -- boolean, only when set by this attach - # - # "asset_destroy" (witnessed asset deletion; always written, even for - # unattached assets -- the files were publicly reachable; node column - # nil, subjects via participants: the asset plus every then-attached - # node): - # "asset_name" -- flat string - # "content_type" -- flat string - # "path" -- public original path, flat string - # "detached_from" -- array of unique_names, only when any - # "headline_removed_from" -- array of unique_names, only when any + # "asset_destroy" (witnessed asset deletion; the files were publicly + # reachable, so an entry is always written. Destruction is refused + # while the asset is attached to any current row, so no node is ever + # affected: node column nil, the asset the sole participant): + # "asset_name" -- flat string + # "content_type" -- flat string + # "path" -- public original path, flat string # # "otp_enroll" / "otp_disable" / "otp_reset" (second-factor # lifecycle) and "user_create" / "user_deactivate" / diff --git a/config/locales/de.yml b/config/locales/de.yml index 58367aaf..92a46768 100644 --- a/config/locales/de.yml +++ b/config/locales/de.yml @@ -155,7 +155,7 @@ de: asset: attributes: base: - not_permitted: "Dieses Asset ist an geschützte Seiten angehängt; nur die Redaktion darf es löschen" + destroy_while_attached: "Ein Asset, das noch an Seiten angehängt ist, kann nicht gelöscht werden. Bitte entferne es zuerst im Editor" upload: unreadable_image: "konnte nicht als Bild gelesen werden. Möglicherweise ist sie beschädigt oder hat ein anderes Format, als der Name vermuten lässt." event: @@ -218,11 +218,7 @@ de: revision_restored: "wiederhergestellt am %{date} von %{actor}" asset_history: "Chronik" asset_create: "%{actor} hat das Asset „%{asset}“ hochgeladen" - asset_attach: "%{actor} hat „%{asset}“ an %{subject} angehängt" - asset_attach_headline: "%{actor} hat „%{asset}“ als Aufmacher an %{subject} angehängt" asset_destroy: "%{actor} hat das Asset „%{asset}“ gelöscht" - asset_destroy_detached: "— entfernt von %{paths}" - asset_destroy_headlines: "(war Aufmacher von %{paths})" detail_assets_added: "Anhänge hinzugefügt: %{names}" detail_assets_removed: "Anhänge entfernt: %{names}" assets_reordered: "Anhänge umsortiert" @@ -814,6 +810,7 @@ de: locked_prefix: "Die Seite ist von %{login} gesperrt —" locked_link: "dort entsperren" locked_suffix: ", dann von der Asset-Seite aus erneut anhängen." + resolve_link: "Seite öffnen, um den Bearbeitungskonflikt zu lösen" headline_prefix: "Der bestehende Aufmacher der Seite wurde behalten —" headline_link: "dort ändern" headline_suffix: ", falls gewünscht." diff --git a/config/locales/en.yml b/config/locales/en.yml index 4bdca31b..458a4069 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -106,7 +106,7 @@ en: asset: attributes: base: - not_permitted: "Only Redaktion members may change assets related to nodes in this section" + destroy_while_attached: "Cannot destroy an asset that is still attached to pages; remove it in each page's editor first" upload: unreadable_image: "could not be read as an image. It may be damaged, or not the format its name suggests." event: @@ -170,11 +170,7 @@ en: revision_restored: "restored %{date} by %{actor}" asset_history: "History" asset_destroy: "%{actor} destroyed asset “%{asset}”" - asset_destroy_detached: "— detached from %{paths}" - asset_destroy_headlines: "(was the headline of %{paths})" asset_create: "%{actor} uploaded asset “%{asset}”" - asset_attach: "%{actor} attached “%{asset}” to %{subject}" - asset_attach_headline: "%{actor} attached “%{asset}” to %{subject} as its headline" detail_assets_added: "attachments added: %{names}" detail_assets_removed: "attachments removed: %{names}" assets_reordered: "attachments reordered" @@ -766,6 +762,7 @@ en: locked_prefix: "The page is locked by %{login} —" locked_link: "unlock it there" locked_suffix: " first, then attach from this asset's page." + resolve_link: "Open the page to resolve this" headline_prefix: "The page's existing headline was kept —" headline_link: "change it there" headline_suffix: " if needed." diff --git a/test/models/asset_destroy_test.rb b/test/models/asset_destroy_test.rb index 2f38d692..54512b16 100644 --- a/test/models/asset_destroy_test.rb +++ b/test/models/asset_destroy_test.rb @@ -8,29 +8,6 @@ class AssetDestroyTest < ActiveSupport::TestCase :upload_content_type => "image/png") end - test "destroying an attached asset logs nodes and asset as participants" do - node = Node.root.children.create!(:slug => "asset_destroy_attached") - node.attach_asset!(@asset, :user => @user) - - @asset.destroy_witnessed!(:user => @user) - - action = NodeAction.where(:action => "asset_destroy").last - subjects = action.action_participants.map { |p| [p.subject_type, p.subject_id] } - assert_includes subjects, ["Asset", @asset.id] - assert_includes subjects, ["Node", node.id] - assert_equal [node.unique_name], action.metadata["detached_from"] - end - - test "records which nodes lost their headline" do - node = Node.root.children.create!(:slug => "asset_destroy_headline") - node.attach_asset!(@asset, :user => @user, :headline => true) - - @asset.destroy_witnessed!(:user => @user) - - action = NodeAction.where(:action => "asset_destroy").last - assert_equal [node.unique_name], action.metadata["headline_removed_from"] - end - test "an unattached asset is still witnessed" do @asset.destroy_witnessed!(:user => @user) @@ -49,16 +26,24 @@ class AssetDestroyTest < ActiveSupport::TestCase assert_nil action.action_participants.first.subject end - test "destroying an asset attached to a restricted node needs the redaktion role" do - editor = User.create!(:login => "asset_gate", :email => "ag@example.com", - :password => "secret", :password_confirmation => "secret") - updates = Node.root.children.create!(:slug => "updates") - node = updates.children.create!(:slug => "gated-attachment") - node.reload.attach_asset!(@asset, :user => nil) + test "destruction is refused while the asset is attached" do + node = Node.root.children.create!(:slug => "asset_destroy_attached") + node.attach_asset!(@asset, :user => @user) + + assert_raises(ActiveRecord::RecordInvalid) { @asset.destroy_witnessed!(:user => @user) } + assert Asset.exists?(@asset.id) + assert_equal 0, NodeAction.where(:action => "asset_destroy").count + end + + test "an attachment on a draft alone is enough to refuse" do + node = Node.root.children.create!(:slug => "asset_destroy_draft_only") + node.attach_asset!(@asset, :user => @user) + node.publish_draft!(@user) + node.lock_for_editing!(@user) + node.create_new_draft(@user) + node.head.related_assets.destroy_all - error = assert_raises(ActiveRecord::RecordInvalid) { @asset.destroy_witnessed!(:user => editor) } - assert_includes error.message, - I18n.t("activerecord.errors.models.asset.attributes.base.not_permitted") + assert_raises(ActiveRecord::RecordInvalid) { @asset.destroy_witnessed!(:user => @user) } assert Asset.exists?(@asset.id) end end -- cgit v1.3