Fix edits with no actual changes being allowed (#17843)

* Fix edits with no actual changes being allowed locally

* Fix edits with no actual changes being allowed through ActivityPub

* Fix false positive changes caused by description processing in model

* Fix not recording poll expiration update

* Fix test

* Revert changes to ProcessStatusUpdateService

* Various fixes and improvements

* Fix code style issues

* Various changes and improvements

* Add guard clause
This commit is contained in:
Eugen Rochko 2022-03-26 00:38:44 +01:00 committed by GitHub
parent e3a2203061
commit 71f2b95106
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 121 additions and 52 deletions

View File

@ -27,7 +27,9 @@ class ActivityPub::Parser::MediaAttachmentParser
end end
def description def description
@json['summary'].presence || @json['name'].presence str = @json['summary'].presence || @json['name'].presence
str = str.strip[0...MediaAttachment::MAX_DESCRIPTION_LENGTH] if str.present?
str
end end
def focus def focus

View File

@ -0,0 +1,35 @@
# frozen_string_literal: true
module StatusSnapshotConcern
extend ActiveSupport::Concern
included do
has_many :edits, class_name: 'StatusEdit', inverse_of: :status, dependent: :destroy
end
def edited?
edited_at.present?
end
def build_snapshot(account_id: nil, at_time: nil, rate_limit: true)
# We don't use `edits#new` here to avoid it having saved when the
# status is saved, since we want to control that manually
StatusEdit.new(
status_id: id,
text: text,
spoiler_text: spoiler_text,
sensitive: sensitive,
ordered_media_attachment_ids: ordered_media_attachment_ids&.dup || media_attachments.pluck(:id),
media_descriptions: ordered_media_attachments.map(&:description),
poll_options: preloadable_poll&.options&.dup,
account_id: account_id || self.account_id,
created_at: at_time || edited_at,
rate_limit: rate_limit
)
end
def snapshot!(**options)
build_snapshot(**options).save!
end
end

View File

@ -185,7 +185,7 @@ class MediaAttachment < ApplicationRecord
remotable_attachment :thumbnail, IMAGE_LIMIT, suppress_errors: true, download_on_assign: false remotable_attachment :thumbnail, IMAGE_LIMIT, suppress_errors: true, download_on_assign: false
validates :account, presence: true validates :account, presence: true
validates :description, length: { maximum: MAX_DESCRIPTION_LENGTH }, if: :local? validates :description, length: { maximum: MAX_DESCRIPTION_LENGTH }
validates :file, presence: true, if: :local? validates :file, presence: true, if: :local?
validates :thumbnail, absence: true, if: -> { local? && !audio_or_video? } validates :thumbnail, absence: true, if: -> { local? && !audio_or_video? }
@ -258,7 +258,6 @@ class MediaAttachment < ApplicationRecord
after_commit :enqueue_processing, on: :create after_commit :enqueue_processing, on: :create
after_commit :reset_parent_cache, on: :update after_commit :reset_parent_cache, on: :update
before_create :prepare_description, unless: :local?
before_create :set_unknown_type before_create :set_unknown_type
before_create :set_processing before_create :set_processing
@ -306,10 +305,6 @@ class MediaAttachment < ApplicationRecord
self.type = :unknown if file.blank? && !type_changed? self.type = :unknown if file.blank? && !type_changed?
end end
def prepare_description
self.description = description.strip[0...MAX_DESCRIPTION_LENGTH] unless description.nil?
end
def set_type_and_extension def set_type_and_extension
self.type = begin self.type = begin
if VIDEO_MIME_TYPES.include?(file_content_type) if VIDEO_MIME_TYPES.include?(file_content_type)

View File

@ -35,6 +35,7 @@ class Status < ApplicationRecord
include Paginable include Paginable
include Cacheable include Cacheable
include StatusThreadingConcern include StatusThreadingConcern
include StatusSnapshotConcern
include RateLimitable include RateLimitable
rate_limit by: :account, family: :statuses rate_limit by: :account, family: :statuses
@ -59,8 +60,6 @@ class Status < ApplicationRecord
belongs_to :thread, foreign_key: 'in_reply_to_id', class_name: 'Status', inverse_of: :replies, optional: true belongs_to :thread, foreign_key: 'in_reply_to_id', class_name: 'Status', inverse_of: :replies, optional: true
belongs_to :reblog, foreign_key: 'reblog_of_id', class_name: 'Status', inverse_of: :reblogs, optional: true belongs_to :reblog, foreign_key: 'reblog_of_id', class_name: 'Status', inverse_of: :reblogs, optional: true
has_many :edits, class_name: 'StatusEdit', inverse_of: :status, dependent: :destroy
has_many :favourites, inverse_of: :status, dependent: :destroy has_many :favourites, inverse_of: :status, dependent: :destroy
has_many :bookmarks, inverse_of: :status, dependent: :destroy has_many :bookmarks, inverse_of: :status, dependent: :destroy
has_many :reblogs, foreign_key: 'reblog_of_id', class_name: 'Status', inverse_of: :reblog, dependent: :destroy has_many :reblogs, foreign_key: 'reblog_of_id', class_name: 'Status', inverse_of: :reblog, dependent: :destroy
@ -212,24 +211,6 @@ class Status < ApplicationRecord
public_visibility? || unlisted_visibility? public_visibility? || unlisted_visibility?
end end
def snapshot!(account_id: nil, at_time: nil, rate_limit: true)
edits.create!(
text: text,
spoiler_text: spoiler_text,
sensitive: sensitive,
ordered_media_attachment_ids: ordered_media_attachment_ids || media_attachments.pluck(:id),
media_descriptions: ordered_media_attachments.map(&:description),
poll_options: preloadable_poll&.options,
account_id: account_id || self.account_id,
created_at: at_time || edited_at,
rate_limit: rate_limit
)
end
def edited?
edited_at.present?
end
alias sign? distributable? alias sign? distributable?
def with_media? def with_media?

View File

@ -4,6 +4,8 @@ class ActivityPub::ProcessStatusUpdateService < BaseService
include JsonLdHelper include JsonLdHelper
def call(status, json) def call(status, json)
raise ArgumentError, 'Status has unsaved changes' if status.changed?
@json = json @json = json
@status_parser = ActivityPub::Parser::StatusParser.new(@json) @status_parser = ActivityPub::Parser::StatusParser.new(@json)
@uri = @status_parser.uri @uri = @status_parser.uri
@ -17,16 +19,19 @@ class ActivityPub::ProcessStatusUpdateService < BaseService
last_edit_date = status.edited_at.presence || status.created_at last_edit_date = status.edited_at.presence || status.created_at
# Since we rely on tracking of previous changes, ensure clean slate
status.clear_changes_information
# Only allow processing one create/update per status at a time # Only allow processing one create/update per status at a time
RedisLock.acquire(lock_options) do |lock| RedisLock.acquire(lock_options) do |lock|
if lock.acquired? if lock.acquired?
Status.transaction do Status.transaction do
create_previous_edit! record_previous_edit!
update_media_attachments! update_media_attachments!
update_poll! update_poll!
update_immediate_attributes! update_immediate_attributes!
update_metadata! update_metadata!
create_edit! create_edits!
end end
queue_poll_notifications! queue_poll_notifications!
@ -216,19 +221,14 @@ class ActivityPub::ProcessStatusUpdateService < BaseService
{ redis: Redis.current, key: "create:#{@uri}", autorelease: 15.minutes.seconds } { redis: Redis.current, key: "create:#{@uri}", autorelease: 15.minutes.seconds }
end end
def create_previous_edit! def record_previous_edit!
# We only need to create a previous edit when no previous edits exist, e.g. @previous_edit = @status.build_snapshot(at_time: @status.created_at, rate_limit: false) if @status.edits.empty?
# when the status has never been edited. For other cases, we always create
# an edit, so the step can be skipped
return if @status.edits.any?
@status.snapshot!(at_time: @status.created_at, rate_limit: false)
end end
def create_edit! def create_edits!
return unless significant_changes? return unless significant_changes?
@previous_edit&.save!
@status.snapshot!(account_id: @account.id, rate_limit: false) @status.snapshot!(account_id: @account.id, rate_limit: false)
end end

View File

@ -4,6 +4,8 @@ class UpdateStatusService < BaseService
include Redisable include Redisable
include LanguagesHelper include LanguagesHelper
class NoChangesSubmittedError < StandardError; end
# @param [Status] status # @param [Status] status
# @param [Integer] account_id # @param [Integer] account_id
# @param [Hash] options # @param [Hash] options
@ -17,6 +19,8 @@ class UpdateStatusService < BaseService
@status = status @status = status
@options = options @options = options
@account_id = account_id @account_id = account_id
@media_attachments_changed = false
@poll_changed = false
Status.transaction do Status.transaction do
create_previous_edit! create_previous_edit!
@ -32,18 +36,24 @@ class UpdateStatusService < BaseService
broadcast_updates! broadcast_updates!
@status @status
rescue NoChangesSubmittedError
# For calls that result in no changes, swallow the error
# but get back to the original state
@status.reload
end end
private private
def update_media_attachments! def update_media_attachments!
previous_media_attachments = @status.media_attachments.to_a previous_media_attachments = @status.ordered_media_attachments.to_a
next_media_attachments = validate_media! next_media_attachments = validate_media!
added_media_attachments = next_media_attachments - previous_media_attachments added_media_attachments = next_media_attachments - previous_media_attachments
MediaAttachment.where(id: added_media_attachments.map(&:id)).update_all(status_id: @status.id) MediaAttachment.where(id: added_media_attachments.map(&:id)).update_all(status_id: @status.id)
@status.ordered_media_attachment_ids = (@options[:media_ids] || []).map(&:to_i) & next_media_attachments.map(&:id) @status.ordered_media_attachment_ids = (@options[:media_ids] || []).map(&:to_i) & next_media_attachments.map(&:id)
@media_attachments_changed = previous_media_attachments.map(&:id) != @status.ordered_media_attachment_ids
@status.media_attachments.reload @status.media_attachments.reload
end end
@ -69,20 +79,23 @@ class UpdateStatusService < BaseService
# If for some reasons the options were changed, it invalidates all previous # If for some reasons the options were changed, it invalidates all previous
# votes, so we need to remove them # votes, so we need to remove them
poll_changed = true if @options[:poll][:options] != poll.options || ActiveModel::Type::Boolean.new.cast(@options[:poll][:multiple]) != poll.multiple @poll_changed = true if @options[:poll][:options] != poll.options || ActiveModel::Type::Boolean.new.cast(@options[:poll][:multiple]) != poll.multiple
poll.options = @options[:poll][:options] poll.options = @options[:poll][:options]
poll.hide_totals = @options[:poll][:hide_totals] || false poll.hide_totals = @options[:poll][:hide_totals] || false
poll.multiple = @options[:poll][:multiple] || false poll.multiple = @options[:poll][:multiple] || false
poll.expires_in = @options[:poll][:expires_in] poll.expires_in = @options[:poll][:expires_in]
poll.reset_votes! if poll_changed poll.reset_votes! if @poll_changed
poll.save! poll.save!
@status.poll_id = poll.id @status.poll_id = poll.id
elsif previous_poll.present? elsif previous_poll.present?
previous_poll.destroy previous_poll.destroy
@poll_changed = true
@status.poll_id = nil @status.poll_id = nil
end end
@poll_changed = true if @previous_expires_at != @status.preloadable_poll&.expires_at
end end
def update_immediate_attributes! def update_immediate_attributes!
@ -90,8 +103,11 @@ class UpdateStatusService < BaseService
@status.spoiler_text = @options[:spoiler_text] || '' if @options.key?(:spoiler_text) @status.spoiler_text = @options[:spoiler_text] || '' if @options.key?(:spoiler_text)
@status.sensitive = @options[:sensitive] || @options[:spoiler_text].present? if @options.key?(:sensitive) || @options.key?(:spoiler_text) @status.sensitive = @options[:sensitive] || @options[:spoiler_text].present? if @options.key?(:sensitive) || @options.key?(:spoiler_text)
@status.language = valid_locale_cascade(@options[:language], @status.language, @status.account.user&.preferred_posting_language, I18n.default_locale) @status.language = valid_locale_cascade(@options[:language], @status.language, @status.account.user&.preferred_posting_language, I18n.default_locale)
@status.edited_at = Time.now.utc
# We raise here to rollback the entire transaction
raise NoChangesSubmittedError unless significant_changes?
@status.edited_at = Time.now.utc
@status.save! @status.save!
end end
@ -137,4 +153,8 @@ class UpdateStatusService < BaseService
def create_edit! def create_edit!
@status.snapshot!(account_id: @account_id) @status.snapshot!(account_id: @account_id)
end end
def significant_changes?
@status.changed? || @poll_changed || @media_attachments_changed
end
end end

View File

@ -186,14 +186,6 @@ RSpec.describe MediaAttachment, type: :model do
expect(media.valid?).to be false expect(media.valid?).to be false
end end
describe 'descriptions for remote attachments' do
it 'are cut off at 1500 characters' do
media = Fabricate(:media_attachment, description: 'foo' * 1000, remote_url: 'http://example.com/blah.jpg')
expect(media.description.size).to be <= 1_500
end
end
describe 'size limit validation' do describe 'size limit validation' do
it 'rejects video files that are too large' do it 'rejects video files that are too large' do
stub_const 'MediaAttachment::IMAGE_LIMIT', 100.megabytes stub_const 'MediaAttachment::IMAGE_LIMIT', 100.megabytes

View File

@ -46,6 +46,29 @@ RSpec.describe ActivityPub::ProcessStatusUpdateService, type: :service do
expect(status.reload.spoiler_text).to eq 'Show more' expect(status.reload.spoiler_text).to eq 'Show more'
end end
context 'with no changes at all' do
let(:payload) do
{
'@context': 'https://www.w3.org/ns/activitystreams',
id: 'foo',
type: 'Note',
content: 'Hello world',
}
end
before do
subject.call(status, json)
end
it 'does not create any edits' do
expect(status.reload.edits).to be_empty
end
it 'does not mark status as edited' do
expect(status.edited?).to be false
end
end
context 'with no changes and originally with no ordered_media_attachment_ids' do context 'with no changes and originally with no ordered_media_attachment_ids' do
let(:payload) do let(:payload) do
{ {
@ -61,8 +84,12 @@ RSpec.describe ActivityPub::ProcessStatusUpdateService, type: :service do
subject.call(status, json) subject.call(status, json)
end end
it 'does not record an update' do it 'does not create any edits' do
expect(status.reload.edited?).to be false expect(status.reload.edits).to be_empty
end
it 'does not mark status as edited' do
expect(status.edited?).to be false
end end
end end

View File

@ -3,6 +3,23 @@ require 'rails_helper'
RSpec.describe UpdateStatusService, type: :service do RSpec.describe UpdateStatusService, type: :service do
subject { described_class.new } subject { described_class.new }
context 'when nothing changes' do
let!(:status) { Fabricate(:status, text: 'Foo', language: 'en') }
before do
allow(ActivityPub::DistributionWorker).to receive(:perform_async)
subject.call(status, status.account_id, text: 'Foo')
end
it 'does not create an edit' do
expect(status.reload.edits).to be_empty
end
it 'does not notify anyone' do
expect(ActivityPub::DistributionWorker).to_not have_received(:perform_async)
end
end
context 'when text changes' do context 'when text changes' do
let!(:status) { Fabricate(:status, text: 'Foo') } let!(:status) { Fabricate(:status, text: 'Foo') }
let(:preview_card) { Fabricate(:preview_card) } let(:preview_card) { Fabricate(:preview_card) }