From 9edab463682f5d5beb0a7f4e4574d708c95bbc52 Mon Sep 17 00:00:00 2001 From: ThibG Date: Wed, 27 Feb 2019 14:57:14 +0100 Subject: [PATCH 1/8] Fix mention processing for unknwon accounts on incoming ActivityPub Notes (#10125) `::FetchRemoteAccountService` is not `ActivityPub::FetchRemoteAccountService`, its second argument is the pre-fetched body. Passing `id: false` actually passed a `Hash` as the prefetched body, instead of properly resolving unknown remote accounts. --- app/lib/activitypub/activity/create.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/lib/activitypub/activity/create.rb b/app/lib/activitypub/activity/create.rb index d7bd65c80..6d58aba70 100644 --- a/app/lib/activitypub/activity/create.rb +++ b/app/lib/activitypub/activity/create.rb @@ -159,7 +159,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity return if tag['href'].blank? account = account_from_uri(tag['href']) - account = ::FetchRemoteAccountService.new.call(tag['href'], id: false) if account.nil? + account = ::FetchRemoteAccountService.new.call(tag['href']) if account.nil? return if account.nil? From 74258ef77a24211a1fe59f6a9cb9244a4ad3caa3 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" Date: Wed, 27 Feb 2019 14:58:08 +0100 Subject: [PATCH 2/8] Bump hamlit-rails from 0.2.1 to 0.2.2 (#10124) Bumps [hamlit-rails](https://github.com/mfung/hamlit-rails) from 0.2.1 to 0.2.2. - [Release notes](https://github.com/mfung/hamlit-rails/releases) - [Commits](https://github.com/mfung/hamlit-rails/compare/v0.2.1...v0.2.2) Signed-off-by: dependabot[bot] --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index a92fadb8a..d23f89440 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -243,7 +243,7 @@ GEM temple (>= 0.8.0) thor tilt - hamlit-rails (0.2.1) + hamlit-rails (0.2.2) actionpack (>= 4.0.1) activesupport (>= 4.0.1) hamlit (>= 1.2.0) From 34315fcda53a3ce29a50a253bd63f7e70ef8e4b3 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" Date: Wed, 27 Feb 2019 15:02:57 +0100 Subject: [PATCH 3/8] Bump capybara from 3.13.2 to 3.14.0 (#10119) Bumps [capybara](https://github.com/teamcapybara/capybara) from 3.13.2 to 3.14.0. - [Release notes](https://github.com/teamcapybara/capybara/releases) - [Changelog](https://github.com/teamcapybara/capybara/blob/master/History.md) - [Commits](https://github.com/teamcapybara/capybara/compare/3.13.2...3.14.0) Signed-off-by: dependabot[bot] --- Gemfile | 2 +- Gemfile.lock | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index e6d7bd4d0..46d77f662 100644 --- a/Gemfile +++ b/Gemfile @@ -107,7 +107,7 @@ group :production, :test do end group :test do - gem 'capybara', '~> 3.13' + gem 'capybara', '~> 3.14' gem 'climate_control', '~> 0.2' gem 'faker', '~> 1.9' gem 'microformats', '~> 4.1' diff --git a/Gemfile.lock b/Gemfile.lock index d23f89440..447444740 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -126,7 +126,7 @@ GEM sshkit (~> 1.3) capistrano-yarn (2.0.2) capistrano (~> 3.0) - capybara (3.13.2) + capybara (3.14.0) addressable mini_mime (>= 0.1.3) nokogiri (~> 1.8) @@ -671,7 +671,7 @@ DEPENDENCIES capistrano-rails (~> 1.4) capistrano-rbenv (~> 2.1) capistrano-yarn (~> 2.0) - capybara (~> 3.13) + capybara (~> 3.14) charlock_holmes (~> 0.7.6) chewy (~> 5.0) cld3 (~> 3.2.3) From df0120670398246739144466bf327193383ba530 Mon Sep 17 00:00:00 2001 From: trwnh Date: Wed, 27 Feb 2019 08:03:28 -0600 Subject: [PATCH 4/8] Allow getting-started to scroll on short screens (#10075) At 480px height, there is not enough space to fully display the footer. --- app/javascript/styles/mastodon/components.scss | 1 + 1 file changed, 1 insertion(+) diff --git a/app/javascript/styles/mastodon/components.scss b/app/javascript/styles/mastodon/components.scss index d88557559..0163e90b9 100644 --- a/app/javascript/styles/mastodon/components.scss +++ b/app/javascript/styles/mastodon/components.scss @@ -2336,6 +2336,7 @@ a.account__display-name { .getting-started { color: $dark-text-color; + overflow: auto; &__footer { flex: 0 0 auto; From 6e8743d17a92438bd2332e97f824bc212fa4b96e Mon Sep 17 00:00:00 2001 From: ThibG Date: Wed, 27 Feb 2019 19:13:16 +0100 Subject: [PATCH 5/8] Fix direct timeline pagination in the WebUI (#10126) The `hasMore` property of timelines in redux store was set whenever an API request returned only one page of results, *even* if the query only requested newer conversations (using `since_id`), causing `hasMore` to be incorrectly set to false whenever fetching new toots in the direct timeline, which happens each time the direct message column is opened. (Basically #9516 for direct messages) --- app/javascript/mastodon/actions/conversations.js | 7 +++++-- app/javascript/mastodon/reducers/conversations.js | 6 +++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/app/javascript/mastodon/actions/conversations.js b/app/javascript/mastodon/actions/conversations.js index 3c2ea9680..c6e062ef7 100644 --- a/app/javascript/mastodon/actions/conversations.js +++ b/app/javascript/mastodon/actions/conversations.js @@ -41,13 +41,15 @@ export const expandConversations = ({ maxId } = {}) => (dispatch, getState) => { params.since_id = getState().getIn(['conversations', 'items', 0, 'last_status']); } + const isLoadingRecent = !!params.since_id; + api(getState).get('/api/v1/conversations', { params }) .then(response => { const next = getLinks(response).refs.find(link => link.rel === 'next'); dispatch(importFetchedAccounts(response.data.reduce((aggr, item) => aggr.concat(item.accounts), []))); dispatch(importFetchedStatuses(response.data.map(item => item.last_status).filter(x => !!x))); - dispatch(expandConversationsSuccess(response.data, next ? next.uri : null)); + dispatch(expandConversationsSuccess(response.data, next ? next.uri : null, isLoadingRecent)); }) .catch(err => dispatch(expandConversationsFail(err))); }; @@ -56,10 +58,11 @@ export const expandConversationsRequest = () => ({ type: CONVERSATIONS_FETCH_REQUEST, }); -export const expandConversationsSuccess = (conversations, next) => ({ +export const expandConversationsSuccess = (conversations, next, isLoadingRecent) => ({ type: CONVERSATIONS_FETCH_SUCCESS, conversations, next, + isLoadingRecent, }); export const expandConversationsFail = error => ({ diff --git a/app/javascript/mastodon/reducers/conversations.js b/app/javascript/mastodon/reducers/conversations.js index 955a07754..9564bffcd 100644 --- a/app/javascript/mastodon/reducers/conversations.js +++ b/app/javascript/mastodon/reducers/conversations.js @@ -35,7 +35,7 @@ const updateConversation = (state, item) => state.update('items', list => { } }); -const expandNormalizedConversations = (state, conversations, next) => { +const expandNormalizedConversations = (state, conversations, next, isLoadingRecent) => { let items = ImmutableList(conversations.map(conversationToMap)); return state.withMutations(mutable => { @@ -66,7 +66,7 @@ const expandNormalizedConversations = (state, conversations, next) => { }); } - if (!next) { + if (!next && !isLoadingRecent) { mutable.set('hasMore', false); } @@ -81,7 +81,7 @@ export default function conversations(state = initialState, action) { case CONVERSATIONS_FETCH_FAIL: return state.set('isLoading', false); case CONVERSATIONS_FETCH_SUCCESS: - return expandNormalizedConversations(state, action.conversations, action.next); + return expandNormalizedConversations(state, action.conversations, action.next, action.isLoadingRecent); case CONVERSATIONS_UPDATE: return updateConversation(state, action.conversation); case CONVERSATIONS_MOUNT: From 9d3c6f1849120e732a9230959cb302575765ea8c Mon Sep 17 00:00:00 2001 From: ThibG Date: Thu, 28 Feb 2019 15:22:21 +0100 Subject: [PATCH 6/8] Improved remote thread fetching (#10106) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fetch up to 5 replies when discovering a new remote status This is used for resolving threads downwards. The originating server must add a “replies” attributes with such replies for it to be useful. * Add some tests for ActivityPub::FetchRepliesWorker * Add specs for ActivityPub::FetchRepliesService * Serialize up to 5 public self-replies for ActivityPub notes * Add specs for ActivityPub::NoteSerializer * Move exponential backoff logic to a worker concern * Fetch first page of paginated collections when fetching thread replies * Add specs for paginated collections in replies * Move Note replies serialization to a first CollectionPage The collection isn't actually paginable yet as it has no id nor a `next` field. This may come in another PR. * Use pluck(:uri) instead of map(&:uri) to improve performances * Fix fetching replies when they are in a CollectionPage --- app/lib/activitypub/activity/create.rb | 10 ++ .../concerns/status_threading_concern.rb | 4 + .../activitypub/collection_presenter.rb | 2 +- .../activitypub/collection_serializer.rb | 5 +- .../activitypub/note_serializer.rb | 13 ++ .../activitypub/fetch_replies_service.rb | 60 +++++++++ .../activitypub/fetch_replies_worker.rb | 12 ++ app/workers/concerns/exponential_backoff.rb | 11 ++ app/workers/fetch_reply_worker.rb | 12 ++ app/workers/thread_resolve_worker.rb | 5 +- spec/serializers/activitypub/note_spec.rb | 44 +++++++ .../activitypub/fetch_replies_service_spec.rb | 122 ++++++++++++++++++ .../activitypub/fetch_replies_worker_spec.rb | 40 ++++++ 13 files changed, 333 insertions(+), 7 deletions(-) create mode 100644 app/services/activitypub/fetch_replies_service.rb create mode 100644 app/workers/activitypub/fetch_replies_worker.rb create mode 100644 app/workers/concerns/exponential_backoff.rb create mode 100644 app/workers/fetch_reply_worker.rb create mode 100644 spec/serializers/activitypub/note_spec.rb create mode 100644 spec/services/activitypub/fetch_replies_service_spec.rb create mode 100644 spec/workers/activitypub/fetch_replies_worker_spec.rb diff --git a/app/lib/activitypub/activity/create.rb b/app/lib/activitypub/activity/create.rb index 6d58aba70..0980f94ba 100644 --- a/app/lib/activitypub/activity/create.rb +++ b/app/lib/activitypub/activity/create.rb @@ -40,6 +40,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity end resolve_thread(@status) + fetch_replies(@status) distribute(@status) forward_for_reply if @status.public_visibility? || @status.unlisted_visibility? end @@ -213,6 +214,15 @@ class ActivityPub::Activity::Create < ActivityPub::Activity ThreadResolveWorker.perform_async(status.id, in_reply_to_uri) end + def fetch_replies(status) + collection = @object['replies'] + return if collection.nil? + replies = ActivityPub::FetchRepliesService.new.call(status, collection, false) + return if replies.present? + uri = value_or_id(collection) + ActivityPub::FetchRepliesWorker.perform_async(status.id, uri) unless uri.nil? + end + def conversation_from_uri(uri) return nil if uri.nil? return Conversation.find_by(id: OStatus::TagManager.instance.unique_tag_to_local_id(uri, 'Conversation')) if OStatus::TagManager.instance.local_id?(uri) diff --git a/app/models/concerns/status_threading_concern.rb b/app/models/concerns/status_threading_concern.rb index b9c800c2a..15eb695cd 100644 --- a/app/models/concerns/status_threading_concern.rb +++ b/app/models/concerns/status_threading_concern.rb @@ -11,6 +11,10 @@ module StatusThreadingConcern find_statuses_from_tree_path(descendant_ids(limit, max_child_id, since_child_id, depth), account, promote: true) end + def self_replies(limit) + account.statuses.where(in_reply_to_id: id, visibility: [:public, :unlisted]).reorder(id: :asc).limit(limit) + end + private def ancestor_ids(limit) diff --git a/app/presenters/activitypub/collection_presenter.rb b/app/presenters/activitypub/collection_presenter.rb index ec84ab1a3..28331f0c4 100644 --- a/app/presenters/activitypub/collection_presenter.rb +++ b/app/presenters/activitypub/collection_presenter.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true class ActivityPub::CollectionPresenter < ActiveModelSerializers::Model - attributes :id, :type, :size, :items, :part_of, :first, :last, :next, :prev + attributes :id, :type, :size, :items, :page, :part_of, :first, :last, :next, :prev end diff --git a/app/serializers/activitypub/collection_serializer.rb b/app/serializers/activitypub/collection_serializer.rb index e8960131b..b03609957 100644 --- a/app/serializers/activitypub/collection_serializer.rb +++ b/app/serializers/activitypub/collection_serializer.rb @@ -7,7 +7,8 @@ class ActivityPub::CollectionSerializer < ActiveModel::Serializer super end - attributes :id, :type + attribute :id, if: -> { object.id.present? } + attribute :type attribute :total_items, if: -> { object.size.present? } attribute :next, if: -> { object.next.present? } attribute :prev, if: -> { object.prev.present? } @@ -37,6 +38,6 @@ class ActivityPub::CollectionSerializer < ActiveModel::Serializer end def page? - object.part_of.present? + object.part_of.present? || object.page.present? end end diff --git a/app/serializers/activitypub/note_serializer.rb b/app/serializers/activitypub/note_serializer.rb index c9d23e25f..6b0978ad3 100644 --- a/app/serializers/activitypub/note_serializer.rb +++ b/app/serializers/activitypub/note_serializer.rb @@ -13,6 +13,8 @@ class ActivityPub::NoteSerializer < ActiveModel::Serializer has_many :media_attachments, key: :attachment has_many :virtual_tags, key: :tag + has_one :replies, serializer: ActivityPub::CollectionSerializer + def id ActivityPub::TagManager.instance.uri_for(object) end @@ -33,6 +35,17 @@ class ActivityPub::NoteSerializer < ActiveModel::Serializer { object.language => Formatter.instance.format(object) } end + def replies + ActivityPub::CollectionPresenter.new( + type: :unordered, + first: ActivityPub::CollectionPresenter.new( + type: :unordered, + page: true, + items: object.self_replies(5).pluck(:uri) + ) + ) + end + def language? object.language.present? end diff --git a/app/services/activitypub/fetch_replies_service.rb b/app/services/activitypub/fetch_replies_service.rb new file mode 100644 index 000000000..95c486a43 --- /dev/null +++ b/app/services/activitypub/fetch_replies_service.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +class ActivityPub::FetchRepliesService < BaseService + include JsonLdHelper + + def call(parent_status, collection_or_uri, allow_synchronous_requests = true) + @account = parent_status.account + @allow_synchronous_requests = allow_synchronous_requests + + @items = collection_items(collection_or_uri) + return if @items.nil? + + FetchReplyWorker.push_bulk(filtered_replies) + + @items + end + + private + + def collection_items(collection_or_uri) + collection = fetch_collection(collection_or_uri) + return unless collection.is_a?(Hash) + + collection = fetch_collection(collection['first']) if collection['first'].present? + return unless collection.is_a?(Hash) + + case collection['type'] + when 'Collection', 'CollectionPage' + collection['items'] + when 'OrderedCollection', 'OrderedCollectionPage' + collection['orderedItems'] + end + end + + def fetch_collection(collection_or_uri) + return collection_or_uri if collection_or_uri.is_a?(Hash) + return unless @allow_synchronous_requests + return if invalid_origin?(collection_or_uri) + collection = fetch_resource_without_id_validation(collection_or_uri) + raise Mastodon::UnexpectedResponseError if collection.nil? + collection + end + + def filtered_replies + # Only fetch replies to the same server as the original status to avoid + # amplification attacks. + + # Also limit to 5 fetched replies to limit potential for DoS. + @items.map { |item| value_or_id(item) }.reject { |uri| invalid_origin?(uri) }.take(5) + end + + def invalid_origin?(url) + return true if unsupported_uri_scheme?(url) + + needle = Addressable::URI.parse(url).host + haystack = Addressable::URI.parse(@account.uri).host + + !haystack.casecmp(needle).zero? + end +end diff --git a/app/workers/activitypub/fetch_replies_worker.rb b/app/workers/activitypub/fetch_replies_worker.rb new file mode 100644 index 000000000..bf466db54 --- /dev/null +++ b/app/workers/activitypub/fetch_replies_worker.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +class ActivityPub::FetchRepliesWorker + include Sidekiq::Worker + include ExponentialBackoff + + sidekiq_options queue: 'pull', retry: 3 + + def perform(parent_status_id, replies_uri) + ActivityPub::FetchRepliesService.new.call(Status.find(parent_status_id), replies_uri) + end +end diff --git a/app/workers/concerns/exponential_backoff.rb b/app/workers/concerns/exponential_backoff.rb new file mode 100644 index 000000000..f2b931e33 --- /dev/null +++ b/app/workers/concerns/exponential_backoff.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module ExponentialBackoff + extend ActiveSupport::Concern + + included do + sidekiq_retry_in do |count| + 15 + 10 * (count**4) + rand(10 * (count**4)) + end + end +end diff --git a/app/workers/fetch_reply_worker.rb b/app/workers/fetch_reply_worker.rb new file mode 100644 index 000000000..f7aa25e81 --- /dev/null +++ b/app/workers/fetch_reply_worker.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +class FetchReplyWorker + include Sidekiq::Worker + include ExponentialBackoff + + sidekiq_options queue: 'pull', retry: 3 + + def perform(child_url) + FetchRemoteStatusService.new.call(child_url) + end +end diff --git a/app/workers/thread_resolve_worker.rb b/app/workers/thread_resolve_worker.rb index c18a778d5..8bba9ca75 100644 --- a/app/workers/thread_resolve_worker.rb +++ b/app/workers/thread_resolve_worker.rb @@ -2,13 +2,10 @@ class ThreadResolveWorker include Sidekiq::Worker + include ExponentialBackoff sidekiq_options queue: 'pull', retry: 3 - sidekiq_retry_in do |count| - 15 + 10 * (count**4) + rand(10 * (count**4)) - end - def perform(child_status_id, parent_url) child_status = Status.find(child_status_id) parent_status = FetchRemoteStatusService.new.call(parent_url) diff --git a/spec/serializers/activitypub/note_spec.rb b/spec/serializers/activitypub/note_spec.rb new file mode 100644 index 000000000..55bfbc16b --- /dev/null +++ b/spec/serializers/activitypub/note_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe ActivityPub::NoteSerializer do + let!(:account) { Fabricate(:account) } + let!(:other) { Fabricate(:account) } + let!(:parent) { Fabricate(:status, account: account, visibility: :public) } + let!(:reply1) { Fabricate(:status, account: account, thread: parent, visibility: :public) } + let!(:reply2) { Fabricate(:status, account: account, thread: parent, visibility: :public) } + let!(:reply3) { Fabricate(:status, account: other, thread: parent, visibility: :public) } + let!(:reply4) { Fabricate(:status, account: account, thread: parent, visibility: :public) } + let!(:reply5) { Fabricate(:status, account: account, thread: parent, visibility: :direct) } + + before(:each) do + @serialization = ActiveModelSerializers::SerializableResource.new(parent, serializer: ActivityPub::NoteSerializer, adapter: ActivityPub::Adapter) + end + + subject { JSON.parse(@serialization.to_json) } + + it 'has a Note type' do + expect(subject['type']).to eql('Note') + end + + it 'has a replies collection' do + expect(subject['replies']['type']).to eql('Collection') + end + + it 'has a replies collection with a first Page' do + expect(subject['replies']['first']['type']).to eql('CollectionPage') + end + + it 'includes public self-replies in its replies collection' do + expect(subject['replies']['first']['items']).to include(reply1.uri, reply2.uri, reply4.uri) + end + + it 'does not include replies from others in its replies collection' do + expect(subject['replies']['first']['items']).to_not include(reply3.uri) + end + + it 'does not include replies with direct visibility in its replies collection' do + expect(subject['replies']['first']['items']).to_not include(reply5.uri) + end +end diff --git a/spec/services/activitypub/fetch_replies_service_spec.rb b/spec/services/activitypub/fetch_replies_service_spec.rb new file mode 100644 index 000000000..65c453341 --- /dev/null +++ b/spec/services/activitypub/fetch_replies_service_spec.rb @@ -0,0 +1,122 @@ +require 'rails_helper' + +RSpec.describe ActivityPub::FetchRepliesService, type: :service do + let(:actor) { Fabricate(:account, domain: 'example.com', uri: 'http://example.com/account') } + let(:status) { Fabricate(:status, account: actor) } + let(:collection_uri) { 'http://example.com/replies/1' } + + let(:items) do + [ + 'http://example.com/self-reply-1', + 'http://example.com/self-reply-2', + 'http://example.com/self-reply-3', + 'http://other.com/other-reply-1', + 'http://other.com/other-reply-2', + 'http://other.com/other-reply-3', + 'http://example.com/self-reply-4', + 'http://example.com/self-reply-5', + 'http://example.com/self-reply-6', + ] + end + + let(:payload) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + type: 'Collection', + id: collection_uri, + items: items, + }.with_indifferent_access + end + + subject { described_class.new } + + describe '#call' do + context 'when the payload is a Collection with inlined replies' do + context 'when passing the collection itself' do + it 'spawns workers for up to 5 replies on the same server' do + allow(FetchReplyWorker).to receive(:push_bulk) + subject.call(status, payload) + expect(FetchReplyWorker).to have_received(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5']) + end + end + + context 'when passing the URL to the collection' do + before do + stub_request(:get, collection_uri).to_return(status: 200, body: Oj.dump(payload)) + end + + it 'spawns workers for up to 5 replies on the same server' do + allow(FetchReplyWorker).to receive(:push_bulk) + subject.call(status, collection_uri) + expect(FetchReplyWorker).to have_received(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5']) + end + end + end + + context 'when the payload is an OrderedCollection with inlined replies' do + let(:payload) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + type: 'OrderedCollection', + id: collection_uri, + orderedItems: items, + }.with_indifferent_access + end + + context 'when passing the collection itself' do + it 'spawns workers for up to 5 replies on the same server' do + allow(FetchReplyWorker).to receive(:push_bulk) + subject.call(status, payload) + expect(FetchReplyWorker).to have_received(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5']) + end + end + + context 'when passing the URL to the collection' do + before do + stub_request(:get, collection_uri).to_return(status: 200, body: Oj.dump(payload)) + end + + it 'spawns workers for up to 5 replies on the same server' do + allow(FetchReplyWorker).to receive(:push_bulk) + subject.call(status, collection_uri) + expect(FetchReplyWorker).to have_received(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5']) + end + end + end + + context 'when the payload is a paginated Collection with inlined replies' do + let(:payload) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + type: 'Collection', + id: collection_uri, + first: { + type: 'CollectionPage', + partOf: collection_uri, + items: items, + } + }.with_indifferent_access + end + + context 'when passing the collection itself' do + it 'spawns workers for up to 5 replies on the same server' do + allow(FetchReplyWorker).to receive(:push_bulk) + subject.call(status, payload) + expect(FetchReplyWorker).to have_received(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5']) + end + end + + context 'when passing the URL to the collection' do + before do + stub_request(:get, collection_uri).to_return(status: 200, body: Oj.dump(payload)) + end + + it 'spawns workers for up to 5 replies on the same server' do + allow(FetchReplyWorker).to receive(:push_bulk) + subject.call(status, collection_uri) + expect(FetchReplyWorker).to have_received(:push_bulk).with(['http://example.com/self-reply-1', 'http://example.com/self-reply-2', 'http://example.com/self-reply-3', 'http://example.com/self-reply-4', 'http://example.com/self-reply-5']) + end + end + end + end +end diff --git a/spec/workers/activitypub/fetch_replies_worker_spec.rb b/spec/workers/activitypub/fetch_replies_worker_spec.rb new file mode 100644 index 000000000..91ef3c4b9 --- /dev/null +++ b/spec/workers/activitypub/fetch_replies_worker_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe ActivityPub::FetchRepliesWorker do + subject { described_class.new } + + let(:account) { Fabricate(:account, uri: 'https://example.com/user/1') } + let(:status) { Fabricate(:status, account: account) } + + let(:payload) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'https://example.com/statuses_replies/1', + type: 'Collection', + items: [], + } + end + + let(:json) { Oj.dump(payload) } + + describe 'perform' do + it 'performs a request if the collection URI is from the same host' do + stub_request(:get, 'https://example.com/statuses_replies/1').to_return(status: 200, body: json) + subject.perform(status.id, 'https://example.com/statuses_replies/1') + expect(a_request(:get, 'https://example.com/statuses_replies/1')).to have_been_made.once + end + + it 'does not perform a request if the collection URI is from a different host' do + stub_request(:get, 'https://other.com/statuses_replies/1').to_return(status: 200) + subject.perform(status.id, 'https://other.com/statuses_replies/1') + expect(a_request(:get, 'https://other.com/statuses_replies/1')).to_not have_been_made + end + + it 'raises when request fails' do + stub_request(:get, 'https://example.com/statuses_replies/1').to_return(status: 500) + expect { subject.perform(status.id, 'https://example.com/statuses_replies/1') }.to raise_error Mastodon::UnexpectedResponseError + end + end +end From d8498b39838eb352a9b268e0f22b1b025320dc73 Mon Sep 17 00:00:00 2001 From: ThibG Date: Thu, 28 Feb 2019 18:16:34 +0100 Subject: [PATCH 7/8] Give the `replies` collection an identifier and enable pagination (#10128) --- app/controllers/statuses_controller.rb | 53 +++++++++++++++++++ app/lib/activitypub/tag_manager.rb | 6 +++ .../activitypub/note_serializer.rb | 10 ++-- config/routes.rb | 1 + 4 files changed, 67 insertions(+), 3 deletions(-) diff --git a/app/controllers/statuses_controller.rb b/app/controllers/statuses_controller.rb index 15d59fd89..3686bd9fd 100644 --- a/app/controllers/statuses_controller.rb +++ b/app/controllers/statuses_controller.rb @@ -18,6 +18,7 @@ class StatusesController < ApplicationController before_action :redirect_to_original, only: [:show] before_action :set_referrer_policy_header, only: [:show] before_action :set_cache_headers + before_action :set_replies, only: [:replies] content_security_policy only: :embed do |p| p.frame_ancestors(false) @@ -63,8 +64,37 @@ class StatusesController < ApplicationController render 'stream_entries/embed', layout: 'embedded' end + def replies + skip_session! + + render json: replies_collection_presenter, + serializer: ActivityPub::CollectionSerializer, + adapter: ActivityPub::Adapter, + content_type: 'application/activity+json', + skip_activities: true + end + private + def replies_collection_presenter + page = ActivityPub::CollectionPresenter.new( + id: replies_account_status_url(@account, @status, page_params), + type: :unordered, + part_of: replies_account_status_url(@account, @status), + next: next_page, + items: @replies.map { |status| status.local ? status : status.id } + ) + if page_requested? + page + else + ActivityPub::CollectionPresenter.new( + id: replies_account_status_url(@account, @status), + type: :unordered, + first: page + ) + end + end + def create_descendant_thread(starting_depth, statuses) depth = starting_depth + statuses.size if depth < DESCENDANTS_DEPTH_LIMIT @@ -174,4 +204,27 @@ class StatusesController < ApplicationController return if @status.public_visibility? || @status.unlisted_visibility? response.headers['Referrer-Policy'] = 'origin' end + + def page_requested? + params[:page] == 'true' + end + + def set_replies + @replies = page_params[:other_accounts] ? Status.where.not(account_id: @account.id) : @account.statuses + @replies = @replies.where(in_reply_to_id: @status.id, visibility: [:public, :unlisted]) + @replies = @replies.paginate_by_min_id(DESCENDANTS_LIMIT, params[:min_id]) + end + + def next_page + last_reply = @replies.last + return if last_reply.nil? + same_account = last_reply.account_id == @account.id + return unless same_account || @replies.size == DESCENDANTS_LIMIT + same_account = false unless @replies.size == DESCENDANTS_LIMIT + replies_account_status_url(@account, @status, page: true, min_id: last_reply.id, other_accounts: !same_account) + end + + def page_params + { page: true, other_accounts: params[:other_accounts], min_id: params[:min_id] }.compact + end end diff --git a/app/lib/activitypub/tag_manager.rb b/app/lib/activitypub/tag_manager.rb index be3a562d0..892bb9974 100644 --- a/app/lib/activitypub/tag_manager.rb +++ b/app/lib/activitypub/tag_manager.rb @@ -48,6 +48,12 @@ class ActivityPub::TagManager activity_account_status_url(target.account, target) end + def replies_uri_for(target, page_params = nil) + raise ArgumentError, 'target must be a local activity' unless %i(note comment activity).include?(target.object_type) && target.local? + + replies_account_status_url(target.account, target, page_params) + end + # Primary audience of a status # Public statuses go out to primarily the public collection # Unlisted and private statuses go out primarily to the followers collection diff --git a/app/serializers/activitypub/note_serializer.rb b/app/serializers/activitypub/note_serializer.rb index 6b0978ad3..4aab993a9 100644 --- a/app/serializers/activitypub/note_serializer.rb +++ b/app/serializers/activitypub/note_serializer.rb @@ -13,7 +13,7 @@ class ActivityPub::NoteSerializer < ActiveModel::Serializer has_many :media_attachments, key: :attachment has_many :virtual_tags, key: :tag - has_one :replies, serializer: ActivityPub::CollectionSerializer + has_one :replies, serializer: ActivityPub::CollectionSerializer, if: :local? def id ActivityPub::TagManager.instance.uri_for(object) @@ -36,12 +36,16 @@ class ActivityPub::NoteSerializer < ActiveModel::Serializer end def replies + replies = object.self_replies(5).pluck(:id, :uri) + last_id = replies.last&.first ActivityPub::CollectionPresenter.new( type: :unordered, + id: ActivityPub::TagManager.instance.replies_uri_for(object), first: ActivityPub::CollectionPresenter.new( type: :unordered, - page: true, - items: object.self_replies(5).pluck(:uri) + part_of: ActivityPub::TagManager.instance.replies_uri_for(object), + items: replies.map(&:second), + next: last_id ? ActivityPub::TagManager.instance.replies_uri_for(object, page: true, min_id: last_id) : nil ) ) end diff --git a/config/routes.rb b/config/routes.rb index ded62981d..9a83d0f88 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -56,6 +56,7 @@ Rails.application.routes.draw do member do get :activity get :embed + get :replies end end From ee7d79c0ce411ff7754e3ac31dbf27cb9e298f73 Mon Sep 17 00:00:00 2001 From: ThibG Date: Thu, 28 Feb 2019 21:35:16 +0100 Subject: [PATCH 8/8] Fix serialization of boosts (#10129) The condition introduced by #9998 was wrong, serializing boosts that weren't self-boosts, and not serializing self-boosts. --- app/serializers/activitypub/activity_serializer.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/serializers/activitypub/activity_serializer.rb b/app/serializers/activitypub/activity_serializer.rb index b51e8c544..c001e28aa 100644 --- a/app/serializers/activitypub/activity_serializer.rb +++ b/app/serializers/activitypub/activity_serializer.rb @@ -3,8 +3,8 @@ class ActivityPub::ActivitySerializer < ActiveModel::Serializer attributes :id, :type, :actor, :published, :to, :cc - has_one :proper, key: :object, serializer: ActivityPub::NoteSerializer, unless: :owned_announce? - attribute :proper_uri, key: :object, if: :owned_announce? + has_one :proper, key: :object, serializer: ActivityPub::NoteSerializer, if: :serialize_object? + attribute :proper_uri, key: :object, unless: :serialize_object? attribute :atom_uri, if: :announce? def id @@ -43,7 +43,9 @@ class ActivityPub::ActivitySerializer < ActiveModel::Serializer object.reblog? end - def owned_announce? - announce? && object.account == object.proper.account && object.proper.private_visibility? + def serialize_object? + return true unless announce? + # Serialize private self-boosts of local toots + object.account == object.proper.account && object.proper.private_visibility? && object.local? end end