From ddbe906c25fc370c92fa557d3637d50b7af97754 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sat, 12 Mar 2022 09:11:36 +0100 Subject: [PATCH] Fix not updating a status when newer version is fetched manually (#17745) --- app/helpers/jsonld_helper.rb | 8 ++++ app/lib/activitypub/activity.rb | 10 +--- app/lib/activitypub/activity/update.rb | 2 +- .../fetch_remote_status_service.rb | 30 +++++++----- spec/rails_helper.rb | 4 +- .../fetch_remote_status_service_spec.rb | 48 ++++++++++++++++--- 6 files changed, 72 insertions(+), 30 deletions(-) diff --git a/app/helpers/jsonld_helper.rb b/app/helpers/jsonld_helper.rb index ef16eef08..102e4b132 100644 --- a/app/helpers/jsonld_helper.rb +++ b/app/helpers/jsonld_helper.rb @@ -15,6 +15,14 @@ module JsonLdHelper value.is_a?(Array) ? value.first : value end + def uri_from_bearcap(str) + if str&.start_with?('bear:') + Addressable::URI.parse(str).query_values['u'] + else + str + end + end + # The url attribute can be a string, an array of strings, or an array of objects. # The objects could include a mimeType. Not-included mimeType means it's text/html. def url_to_href(value, preferred_type = nil) diff --git a/app/lib/activitypub/activity.rb b/app/lib/activitypub/activity.rb index 706960f92..f599e1b58 100644 --- a/app/lib/activitypub/activity.rb +++ b/app/lib/activitypub/activity.rb @@ -71,15 +71,7 @@ class ActivityPub::Activity end def object_uri - @object_uri ||= begin - str = value_or_id(@object) - - if str&.start_with?('bear:') - Addressable::URI.parse(str).query_values['u'] - else - str - end - end + @object_uri ||= uri_from_bearcap(value_or_id(@object)) end def unsupported_object_type? diff --git a/app/lib/activitypub/activity/update.rb b/app/lib/activitypub/activity/update.rb index 36ad5f446..5802ead5d 100644 --- a/app/lib/activitypub/activity/update.rb +++ b/app/lib/activitypub/activity/update.rb @@ -20,7 +20,7 @@ class ActivityPub::Activity::Update < ActivityPub::Activity end def update_status - return reject_payload! if invalid_origin?(@object['id']) + return reject_payload! if invalid_origin?(object_uri) @status = Status.find_by(uri: object_uri, account_id: @account.id) diff --git a/app/services/activitypub/fetch_remote_status_service.rb b/app/services/activitypub/fetch_remote_status_service.rb index 4f789d50b..9672b3d2b 100644 --- a/app/services/activitypub/fetch_remote_status_service.rb +++ b/app/services/activitypub/fetch_remote_status_service.rb @@ -15,24 +15,30 @@ class ActivityPub::FetchRemoteStatusService < BaseService return unless supported_context? - actor_id = nil + actor_uri = nil activity_json = nil + object_uri = nil if expected_object_type? - actor_id = value_or_id(first_of_value(@json['attributedTo'])) - activity_json = { 'type' => 'Create', 'actor' => actor_id, 'object' => @json } + actor_uri = value_or_id(first_of_value(@json['attributedTo'])) + activity_json = { 'type' => 'Create', 'actor' => actor_uri, 'object' => @json } + object_uri = uri_from_bearcap(@json['id']) elsif expected_activity_type? - actor_id = value_or_id(first_of_value(@json['actor'])) + actor_uri = value_or_id(first_of_value(@json['actor'])) activity_json = @json + object_uri = uri_from_bearcap(value_or_id(@json['object'])) end - return if activity_json.nil? || !trustworthy_attribution?(@json['id'], actor_id) + return if activity_json.nil? || object_uri.nil? || !trustworthy_attribution?(@json['id'], actor_uri) - actor = ActivityPub::TagManager.instance.uri_to_resource(actor_id, Account) - actor = ActivityPub::FetchRemoteAccountService.new.call(actor_id, id: true) if actor.nil? || needs_update?(actor) + actor = account_from_uri(actor_uri) return if actor.nil? || actor.suspended? + # If we fetched a status that already exists, then we need to treat the + # activity as an update rather than create + activity_json['type'] = 'Update' if equals_or_includes_any?(activity_json['type'], %w(Create)) && Status.where(uri: object_uri, account_id: actor.id).exists? + ActivityPub::Activity.factory(activity_json, actor).perform end @@ -43,6 +49,12 @@ class ActivityPub::FetchRemoteStatusService < BaseService Addressable::URI.parse(uri).normalized_host.casecmp(Addressable::URI.parse(attributed_to).normalized_host).zero? end + def account_from_uri(uri) + actor = ActivityPub::TagManager.instance.uri_to_resource(uri, Account) + actor = ActivityPub::FetchRemoteAccountService.new.call(uri, id: true) if actor.nil? || actor.possibly_stale? + actor + end + def supported_context? super(@json) end @@ -54,8 +66,4 @@ class ActivityPub::FetchRemoteStatusService < BaseService def expected_object_type? equals_or_includes_any?(@json['type'], ActivityPub::Activity::Create::SUPPORTED_TYPES + ActivityPub::Activity::Create::CONVERTED_TYPES) end - - def needs_update?(actor) - actor.possibly_stale? - end end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 86c2a9c52..217a8f2f8 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -60,9 +60,7 @@ RSpec.configure do |config| config.after :each do Rails.cache.clear - - keys = Redis.current.keys - Redis.current.del(keys) if keys.any? + Redis.current.del(Redis.current.keys) end end diff --git a/spec/services/activitypub/fetch_remote_status_service_spec.rb b/spec/services/activitypub/fetch_remote_status_service_spec.rb index 94574aa7f..68816e554 100644 --- a/spec/services/activitypub/fetch_remote_status_service_spec.rb +++ b/spec/services/activitypub/fetch_remote_status_service_spec.rb @@ -3,9 +3,11 @@ require 'rails_helper' RSpec.describe ActivityPub::FetchRemoteStatusService, type: :service do include ActionView::Helpers::TextHelper - let(:sender) { Fabricate(:account) } - let(:recipient) { Fabricate(:account) } - let(:valid_domain) { Rails.configuration.x.local_domain } + let!(:sender) { Fabricate(:account).tap { |account| account.update(uri: ActivityPub::TagManager.instance.uri_for(account)) } } + let!(:recipient) { Fabricate(:account) } + let!(:valid_domain) { Rails.configuration.x.local_domain } + + let(:existing_status) { nil } let(:note) do { @@ -19,11 +21,13 @@ RSpec.describe ActivityPub::FetchRemoteStatusService, type: :service do subject { described_class.new } + before do + stub_request(:head, 'https://example.com/watch?v=12345').to_return(status: 404, body: '') + end + describe '#call' do before do - sender.update(uri: ActivityPub::TagManager.instance.uri_for(sender)) - - stub_request(:head, 'https://example.com/watch?v=12345').to_return(status: 404, body: '') + existing_status subject.call(object[:id], prefetched_body: Oj.dump(object)) end @@ -186,5 +190,37 @@ RSpec.describe ActivityPub::FetchRemoteStatusService, type: :service do expect(sender.statuses.first).to be_nil end end + + context 'when status already exists' do + let(:existing_status) { Fabricate(:status, account: sender, text: 'Foo', uri: note[:id]) } + + context 'with a Note object' do + let(:object) { note } + + it 'updates status' do + existing_status.reload + expect(existing_status.text).to eq 'Lorem ipsum' + expect(existing_status.edits).to_not be_empty + end + end + + context 'with a Create activity' do + let(:object) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: "https://#{valid_domain}/@foo/1234/create", + type: 'Create', + actor: ActivityPub::TagManager.instance.uri_for(sender), + object: note, + } + end + + it 'updates status' do + existing_status.reload + expect(existing_status.text).to eq 'Lorem ipsum' + expect(existing_status.edits).to_not be_empty + end + end + end end end