From d4e6774a0c88931d907f275821eb001e6dd2cb2d Mon Sep 17 00:00:00 2001 From: Claire Date: Fri, 11 Feb 2022 14:52:07 +0100 Subject: [PATCH] Fix Undo Announce sometimes inlining the originally Announced status (#17516) * Change tests to have more specific expectations on sent ActivityPub payloads * Check that payload doesn't actually contain the contents of the boosted toot * Fix Undo Announce sometimes inlining the originally Announced status --- .../activitypub/activity_presenter.rb | 4 +- .../activitypub/undo_announce_serializer.rb | 2 +- spec/services/remove_status_service_spec.rb | 108 ++++++++++++++---- 3 files changed, 88 insertions(+), 26 deletions(-) diff --git a/app/presenters/activitypub/activity_presenter.rb b/app/presenters/activitypub/activity_presenter.rb index 5d174767f..38e8527e8 100644 --- a/app/presenters/activitypub/activity_presenter.rb +++ b/app/presenters/activitypub/activity_presenter.rb @@ -4,7 +4,7 @@ class ActivityPub::ActivityPresenter < ActiveModelSerializers::Model attributes :id, :type, :actor, :published, :to, :cc, :virtual_object class << self - def from_status(status) + def from_status(status, allow_inlining: true) new.tap do |presenter| presenter.id = ActivityPub::TagManager.instance.activity_uri_for(status) presenter.type = status.reblog? ? 'Announce' : 'Create' @@ -15,7 +15,7 @@ class ActivityPub::ActivityPresenter < ActiveModelSerializers::Model presenter.virtual_object = begin if status.reblog? - if status.account == status.proper.account && status.proper.private_visibility? && status.local? + if allow_inlining && status.account == status.proper.account && status.proper.private_visibility? && status.local? status.proper else ActivityPub::TagManager.instance.uri_for(status.proper) diff --git a/app/serializers/activitypub/undo_announce_serializer.rb b/app/serializers/activitypub/undo_announce_serializer.rb index a925efc18..a0f09e8ab 100644 --- a/app/serializers/activitypub/undo_announce_serializer.rb +++ b/app/serializers/activitypub/undo_announce_serializer.rb @@ -22,6 +22,6 @@ class ActivityPub::UndoAnnounceSerializer < ActivityPub::Serializer end def virtual_object - ActivityPub::ActivityPresenter.from_status(object) + ActivityPub::ActivityPresenter.from_status(object, allow_inlining: false) end end diff --git a/spec/services/remove_status_service_spec.rb b/spec/services/remove_status_service_spec.rb index fb7c6b462..482068d58 100644 --- a/spec/services/remove_status_service_spec.rb +++ b/spec/services/remove_status_service_spec.rb @@ -15,35 +15,97 @@ RSpec.describe RemoveStatusService, type: :service do jeff.follow!(alice) hank.follow!(alice) - - @status = PostStatusService.new.call(alice, text: 'Hello @bob@example.com') - FavouriteService.new.call(jeff, @status) - Fabricate(:status, account: bill, reblog: @status, uri: 'hoge') end - it 'removes status from author\'s home feed' do - subject.call(@status) - expect(HomeFeed.new(alice).get(10)).to_not include(@status.id) + context 'when removed status is not a reblog' do + before do + @status = PostStatusService.new.call(alice, text: 'Hello @bob@example.com ThisIsASecret') + FavouriteService.new.call(jeff, @status) + Fabricate(:status, account: bill, reblog: @status, uri: 'hoge') + end + + it 'removes status from author\'s home feed' do + subject.call(@status) + expect(HomeFeed.new(alice).get(10)).to_not include(@status.id) + end + + it 'removes status from local follower\'s home feed' do + subject.call(@status) + expect(HomeFeed.new(jeff).get(10)).to_not include(@status.id) + end + + it 'sends Delete activity to followers' do + subject.call(@status) + expect(a_request(:post, 'http://example.com/inbox').with( + body: hash_including({ + 'type' => 'Delete', + 'object' => { + 'type' => 'Tombstone', + 'id' => ActivityPub::TagManager.instance.uri_for(@status), + 'atomUri' => OStatus::TagManager.instance.uri_for(@status), + }, + }) + )).to have_been_made.once + end + + it 'sends Delete activity to rebloggers' do + subject.call(@status) + expect(a_request(:post, 'http://example2.com/inbox').with( + body: hash_including({ + 'type' => 'Delete', + 'object' => { + 'type' => 'Tombstone', + 'id' => ActivityPub::TagManager.instance.uri_for(@status), + 'atomUri' => OStatus::TagManager.instance.uri_for(@status), + }, + }) + )).to have_been_made.once + end + + it 'remove status from notifications' do + expect { subject.call(@status) }.to change { + Notification.where(activity_type: 'Favourite', from_account: jeff, account: alice).count + }.from(1).to(0) + end end - it 'removes status from local follower\'s home feed' do - subject.call(@status) - expect(HomeFeed.new(jeff).get(10)).to_not include(@status.id) + context 'when removed status is a private self-reblog' do + before do + @original_status = Fabricate(:status, account: alice, text: 'Hello ThisIsASecret', visibility: :private) + @status = ReblogService.new.call(alice, @original_status) + end + + it 'sends Undo activity to followers' do + subject.call(@status) + expect(a_request(:post, 'http://example.com/inbox').with( + body: hash_including({ + 'type' => 'Undo', + 'object' => hash_including({ + 'type' => 'Announce', + 'object' => ActivityPub::TagManager.instance.uri_for(@original_status), + }), + }) + )).to have_been_made.once + end end - it 'sends delete activity to followers' do - subject.call(@status) - expect(a_request(:post, 'http://example.com/inbox')).to have_been_made.twice - end + context 'when removed status is public self-reblog' do + before do + @original_status = Fabricate(:status, account: alice, text: 'Hello ThisIsASecret', visibility: :public) + @status = ReblogService.new.call(alice, @original_status) + end - it 'sends delete activity to rebloggers' do - subject.call(@status) - expect(a_request(:post, 'http://example2.com/inbox')).to have_been_made - end - - it 'remove status from notifications' do - expect { subject.call(@status) }.to change { - Notification.where(activity_type: 'Favourite', from_account: jeff, account: alice).count - }.from(1).to(0) + it 'sends Undo activity to followers' do + subject.call(@status) + expect(a_request(:post, 'http://example.com/inbox').with( + body: hash_including({ + 'type' => 'Undo', + 'object' => hash_including({ + 'type' => 'Announce', + 'object' => ActivityPub::TagManager.instance.uri_for(@original_status), + }), + }) + )).to have_been_made.once + end end end