Fix not updating a status when newer version is fetched manually (#17745)

6 files changed, 72 insertions, 30 deletions

diff --git a/app/helpers/jsonld_helper.rb b/app/helpers/jsonld_helper.rb
index ef16eef0864..102e4b13281 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 706960f929a..f599e1b58fd 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 36ad5f4465b..5802ead5d98 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 4f789d50b99..9672b3d2b6a 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 86c2a9c52fd..217a8f2f85b 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 94574aa7f43..68816e55476 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