From 322d74fc2aa9db2d05e5bad944661c6edf1660e1 Mon Sep 17 00:00:00 2001 From: ThibG Date: Fri, 17 Jul 2020 07:07:54 +0200 Subject: Fix boosted toots from blocked account not being retroactively removed from TL (#14339) * Fix boosted toots from blocked account not being retroactively removed from TL Fixes #14301 * Add test for clear_from_timeline --- spec/lib/feed_manager_spec.rb | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) (limited to 'spec') diff --git a/spec/lib/feed_manager_spec.rb b/spec/lib/feed_manager_spec.rb index b996997b1..22eddd2ab 100644 --- a/spec/lib/feed_manager_spec.rb +++ b/spec/lib/feed_manager_spec.rb @@ -429,4 +429,29 @@ RSpec.describe FeedManager do expect(Redis.current).to have_received(:publish).with("timeline:#{receiver.id}", deletion) end end + + describe '#clear_from_timeline' do + let(:account) { Fabricate(:account) } + let(:followed_account) { Fabricate(:account) } + let(:target_account) { Fabricate(:account) } + let(:status_1) { Fabricate(:status, account: followed_account) } + let(:status_2) { Fabricate(:status, account: target_account) } + let(:status_3) { Fabricate(:status, account: followed_account, mentions: [Fabricate(:mention, account: target_account)]) } + let(:status_4) { Fabricate(:status, mentions: [Fabricate(:mention, account: target_account)]) } + let(:status_5) { Fabricate(:status, account: followed_account, reblog: status_4) } + let(:status_6) { Fabricate(:status, account: followed_account, reblog: status_2) } + let(:status_7) { Fabricate(:status, account: followed_account) } + + before do + [status_1, status_3, status_5, status_6, status_7].each do |status| + Redis.current.zadd("feed:home:#{account.id}", status.id, status.id) + end + end + + it 'correctly cleans the timeline' do + FeedManager.instance.clear_from_timeline(account, target_account) + + expect(Redis.current.zrange("feed:home:#{account.id}", 0, -1)).to eq [status_1.id.to_s, status_7.id.to_s] + end + end end -- cgit From f55dd193f9a62623054dba1537d01bd7f5cd32f3 Mon Sep 17 00:00:00 2001 From: ThibG Date: Wed, 22 Jul 2020 11:44:02 +0200 Subject: Fix RSS feeds not being cachable (#14368) * Add tests for some cachable responses This only covers responses that we should have managed to make cachable so far. It's not the case of all responses that should be cachable in the end. * Fix RSS feeds not being cachable --- app/controllers/application_controller.rb | 2 +- spec/controllers/accounts_controller_spec.rb | 31 +++++++++++++--------- .../activitypub/collections_controller_spec.rb | 23 +++++++++++----- .../activitypub/outboxes_controller_spec.rb | 23 +++++++++++----- .../activitypub/replies_controller_spec.rb | 23 +++++++++++----- spec/controllers/statuses_controller_spec.rb | 23 +++++++++++----- 6 files changed, 88 insertions(+), 37 deletions(-) (limited to 'spec') diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 973db6aca..2201e463e 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -55,7 +55,7 @@ class ApplicationController < ActionController::Base end def store_current_location - store_location_for(:user, request.url) unless request.format == :json + store_location_for(:user, request.url) unless [:json, :rss].include?(request.format&.to_sym) end def require_admin! diff --git a/spec/controllers/accounts_controller_spec.rb b/spec/controllers/accounts_controller_spec.rb index bd36f5494..93bf2c83f 100644 --- a/spec/controllers/accounts_controller_spec.rb +++ b/spec/controllers/accounts_controller_spec.rb @@ -5,6 +5,21 @@ RSpec.describe AccountsController, type: :controller do let(:account) { Fabricate(:user).account } + shared_examples 'cachable response' do + it 'does not set cookies' do + expect(response.cookies).to be_empty + expect(response.headers['Set-Cookies']).to be nil + end + + it 'does not set sessions' do + expect(session).to be_empty + end + + it 'returns public Cache-Control header' do + expect(response.headers['Cache-Control']).to include 'public' + end + end + describe 'GET #show' do let(:format) { 'html' } @@ -323,9 +338,7 @@ RSpec.describe AccountsController, type: :controller do expect(response.content_type).to eq 'application/activity+json' end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' it 'renders account' do json = body_as_json @@ -343,9 +356,7 @@ RSpec.describe AccountsController, type: :controller do expect(response.content_type).to eq 'application/activity+json' end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' it 'returns Vary header with Signature' do expect(response.headers['Vary']).to include 'Signature' @@ -401,9 +412,7 @@ RSpec.describe AccountsController, type: :controller do expect(response.content_type).to eq 'application/activity+json' end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' it 'renders account' do json = body_as_json @@ -447,9 +456,7 @@ RSpec.describe AccountsController, type: :controller do expect(response).to have_http_status(200) end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' end context do diff --git a/spec/controllers/activitypub/collections_controller_spec.rb b/spec/controllers/activitypub/collections_controller_spec.rb index 56be49be3..89939d1d2 100644 --- a/spec/controllers/activitypub/collections_controller_spec.rb +++ b/spec/controllers/activitypub/collections_controller_spec.rb @@ -6,6 +6,21 @@ RSpec.describe ActivityPub::CollectionsController, type: :controller do let!(:account) { Fabricate(:account) } let(:remote_account) { nil } + shared_examples 'cachable response' do + it 'does not set cookies' do + expect(response.cookies).to be_empty + expect(response.headers['Set-Cookies']).to be nil + end + + it 'does not set sessions' do + expect(session).to be_empty + end + + it 'returns public Cache-Control header' do + expect(response.headers['Cache-Control']).to include 'public' + end + end + before do allow(controller).to receive(:signed_request_account).and_return(remote_account) @@ -31,9 +46,7 @@ RSpec.describe ActivityPub::CollectionsController, type: :controller do expect(response.content_type).to eq 'application/activity+json' end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' it 'returns orderedItems with pinned statuses' do json = body_as_json @@ -58,9 +71,7 @@ RSpec.describe ActivityPub::CollectionsController, type: :controller do expect(response.content_type).to eq 'application/activity+json' end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' it 'returns orderedItems with pinned statuses' do json = body_as_json diff --git a/spec/controllers/activitypub/outboxes_controller_spec.rb b/spec/controllers/activitypub/outboxes_controller_spec.rb index 03490533d..1baf5a623 100644 --- a/spec/controllers/activitypub/outboxes_controller_spec.rb +++ b/spec/controllers/activitypub/outboxes_controller_spec.rb @@ -3,6 +3,21 @@ require 'rails_helper' RSpec.describe ActivityPub::OutboxesController, type: :controller do let!(:account) { Fabricate(:account) } + shared_examples 'cachable response' do + it 'does not set cookies' do + expect(response.cookies).to be_empty + expect(response.headers['Set-Cookies']).to be nil + end + + it 'does not set sessions' do + expect(session).to be_empty + end + + it 'returns public Cache-Control header' do + expect(response.headers['Cache-Control']).to include 'public' + end + end + before do Fabricate(:status, account: account, visibility: :public) Fabricate(:status, account: account, visibility: :unlisted) @@ -39,9 +54,7 @@ RSpec.describe ActivityPub::OutboxesController, type: :controller do expect(json[:totalItems]).to eq 4 end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' end context 'with page requested' do @@ -62,9 +75,7 @@ RSpec.describe ActivityPub::OutboxesController, type: :controller do expect(json[:orderedItems].all? { |item| item[:to].include?(ActivityPub::TagManager::COLLECTIONS[:public]) || item[:cc].include?(ActivityPub::TagManager::COLLECTIONS[:public]) }).to be true end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' end end diff --git a/spec/controllers/activitypub/replies_controller_spec.rb b/spec/controllers/activitypub/replies_controller_spec.rb index d956e1b35..ed383864d 100644 --- a/spec/controllers/activitypub/replies_controller_spec.rb +++ b/spec/controllers/activitypub/replies_controller_spec.rb @@ -7,6 +7,21 @@ RSpec.describe ActivityPub::RepliesController, type: :controller do let(:remote_reply_id) { nil } let(:remote_account) { nil } + shared_examples 'cachable response' do + it 'does not set cookies' do + expect(response.cookies).to be_empty + expect(response.headers['Set-Cookies']).to be nil + end + + it 'does not set sessions' do + expect(session).to be_empty + end + + it 'returns public Cache-Control header' do + expect(response.headers['Cache-Control']).to include 'public' + end + end + before do allow(controller).to receive(:signed_request_account).and_return(remote_account) @@ -36,9 +51,7 @@ RSpec.describe ActivityPub::RepliesController, type: :controller do expect(response.content_type).to eq 'application/activity+json' end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' it 'returns items with account\'s own replies' do json = body_as_json @@ -87,9 +100,7 @@ RSpec.describe ActivityPub::RepliesController, type: :controller do expect(response.content_type).to eq 'application/activity+json' end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' context 'without only_other_accounts' do it 'returns items with account\'s own replies' do diff --git a/spec/controllers/statuses_controller_spec.rb b/spec/controllers/statuses_controller_spec.rb index ba1f1370a..cd6e1e607 100644 --- a/spec/controllers/statuses_controller_spec.rb +++ b/spec/controllers/statuses_controller_spec.rb @@ -5,6 +5,21 @@ require 'rails_helper' describe StatusesController do render_views + shared_examples 'cachable response' do + it 'does not set cookies' do + expect(response.cookies).to be_empty + expect(response.headers['Set-Cookies']).to be nil + end + + it 'does not set sessions' do + expect(session).to be_empty + end + + it 'returns public Cache-Control header' do + expect(response.headers['Cache-Control']).to include 'public' + end + end + describe 'GET #show' do let(:account) { Fabricate(:account) } let(:status) { Fabricate(:status, account: account) } @@ -80,9 +95,7 @@ describe StatusesController do expect(response.headers['Vary']).to eq 'Accept' end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' it 'returns Content-Type header' do expect(response.headers['Content-Type']).to include 'application/activity+json' @@ -470,9 +483,7 @@ describe StatusesController do expect(response.headers['Vary']).to eq 'Accept' end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' - end + it_behaves_like 'cachable response' it 'returns Content-Type header' do expect(response.headers['Content-Type']).to include 'application/activity+json' -- cgit From 5d9acc0ce41aa0274fef2dd321a8fad1fb0665ea Mon Sep 17 00:00:00 2001 From: ThibG Date: Wed, 22 Jul 2020 11:45:35 +0200 Subject: Fix not handling Undo on some activity types when they aren't inlined (#14346) * Fix not handling Undo on some activity types when they aren't inlined When receiving an Undo for a non-inlined activity, try looking it up in database using the URI. The queries are ad-hoc because we don't have a global index of object URIs, and not all activity types are stored in database with an index on their URI. Announces are just statuses, and have an index on URIs, so this check can be done efficiently. Accepts cannot be handled at all because we don't record their URI at any point. Follows don't have an index on URI, but they have an index on the issuing account, which should make such queries largely manageable. Likes don't have an index on URI, they have an index on the issuing account, but the number of favs per account may be very high, so I decided not to handle that. Blocks don't have an index on URI, but they have an index on the issuing account, which should make such queries largely manageable. In all cases, if an Undo could not be handled properly, we call `delete_later!` because that does not require us to know more than the URI of the undone property. * Add tests * Make newer blocks overwrite older ones Allows re-synchronizing block info by re-blocking and un-blocking again when the original Undo Block has been lost. --- app/lib/activitypub/activity/block.rb | 7 +++- app/lib/activitypub/activity/undo.rb | 51 +++++++++++++++++++++++++++++ spec/lib/activitypub/activity/block_spec.rb | 22 +++++++++++++ spec/lib/activitypub/activity/undo_spec.rb | 35 ++++++++++++++++++-- 4 files changed, 112 insertions(+), 3 deletions(-) (limited to 'spec') diff --git a/app/lib/activitypub/activity/block.rb b/app/lib/activitypub/activity/block.rb index a17a2d50a..90477bf33 100644 --- a/app/lib/activitypub/activity/block.rb +++ b/app/lib/activitypub/activity/block.rb @@ -4,7 +4,12 @@ class ActivityPub::Activity::Block < ActivityPub::Activity def perform target_account = account_from_uri(object_uri) - return if target_account.nil? || !target_account.local? || @account.blocking?(target_account) + return if target_account.nil? || !target_account.local? + + if @account.blocking?(target_account) + @account.block_relationships.find_by(target_account: target_account).update(uri: @json['id']) if @json['id'].present? + return + end UnfollowService.new.call(target_account, @account) if target_account.following?(@account) diff --git a/app/lib/activitypub/activity/undo.rb b/app/lib/activitypub/activity/undo.rb index 599823c6e..9eff1b71c 100644 --- a/app/lib/activitypub/activity/undo.rb +++ b/app/lib/activitypub/activity/undo.rb @@ -13,11 +13,62 @@ class ActivityPub::Activity::Undo < ActivityPub::Activity undo_like when 'Block' undo_block + when nil + handle_reference end end private + def handle_reference + # Some implementations do not inline the object, and as we don't have a + # global index, we have to guess what object it is. + return if object_uri.nil? + + try_undo_announce || try_undo_accept || try_undo_follow || try_undo_like || try_undo_block || delete_later!(object_uri) + end + + def try_undo_announce + status = Status.where.not(reblog_of_id: nil).find_by(uri: object_uri, account: @account) + if status.present? + RemoveStatusService.new.call(status) + true + else + false + end + end + + def try_undo_accept + # We can't currently handle `Undo Accept` as we don't record `Accept`'s uri + false + end + + def try_undo_follow + follow = @account.follow_requests.find_by(uri: object_uri) || @account.active_relationships.find_by(uri: object_uri) + + if follow.present? + follow.destroy + true + else + false + end + end + + def try_undo_like + # There is an index on accounts, but an account may have *many* favs, so this may be too costly + false + end + + def try_undo_block + block = @account.block_relationships.find_by(uri: object_uri) + if block.present? + UnblockService.new.call(@account, block.target_account) + true + else + false + end + end + def undo_announce return if object_uri.nil? diff --git a/spec/lib/activitypub/activity/block_spec.rb b/spec/lib/activitypub/activity/block_spec.rb index 94d37356d..42bdfdc81 100644 --- a/spec/lib/activitypub/activity/block_spec.rb +++ b/spec/lib/activitypub/activity/block_spec.rb @@ -28,6 +28,28 @@ RSpec.describe ActivityPub::Activity::Block do end end + context 'when the recipient is already blocked' do + before do + sender.block!(recipient, uri: 'old') + end + + describe '#perform' do + subject { described_class.new(json, sender) } + + before do + subject.perform + end + + it 'creates a block from sender to recipient' do + expect(sender.blocking?(recipient)).to be true + end + + it 'sets the uri to that of last received block activity' do + expect(sender.block_relationships.find_by(target_account: recipient).uri).to eq 'foo' + end + end + end + context 'when the recipient follows the sender' do before do recipient.follow!(sender) diff --git a/spec/lib/activitypub/activity/undo_spec.rb b/spec/lib/activitypub/activity/undo_spec.rb index 9545e1f46..c0309e49d 100644 --- a/spec/lib/activitypub/activity/undo_spec.rb +++ b/spec/lib/activitypub/activity/undo_spec.rb @@ -50,6 +50,19 @@ RSpec.describe ActivityPub::Activity::Undo do expect(sender.reblogged?(status)).to be false end end + + context 'with only object uri' do + let(:object_json) { 'bar' } + + before do + Fabricate(:status, reblog: status, account: sender, uri: 'bar') + end + + it 'deletes the reblog by uri' do + subject.perform + expect(sender.reblogged?(status)).to be false + end + end end context 'with Accept' do @@ -91,13 +104,22 @@ RSpec.describe ActivityPub::Activity::Undo do end before do - sender.block!(recipient) + sender.block!(recipient, uri: 'bar') end it 'deletes block from sender to recipient' do subject.perform expect(sender.blocking?(recipient)).to be false end + + context 'with only object uri' do + let(:object_json) { 'bar' } + + it 'deletes block from sender to recipient' do + subject.perform + expect(sender.blocking?(recipient)).to be false + end + end end context 'with Follow' do @@ -113,13 +135,22 @@ RSpec.describe ActivityPub::Activity::Undo do end before do - sender.follow!(recipient) + sender.follow!(recipient, uri: 'bar') end it 'deletes follow from sender to recipient' do subject.perform expect(sender.following?(recipient)).to be false end + + context 'with only object uri' do + let(:object_json) { 'bar' } + + it 'deletes follow from sender to recipient' do + subject.perform + expect(sender.following?(recipient)).to be false + end + end end context 'with Like' do -- cgit