From dc1ebd45a30d806fcef2dc33679457285ba430b4 Mon Sep 17 00:00:00 2001 From: takayamaki Date: Thu, 30 Nov 2017 11:35:54 +0900 Subject: [PATCH 1/3] add index on stream_entries table (#5793) --- db/migrate/20171129172043_add_index_on_stream_entries.rb | 7 +++++++ db/schema.rb | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20171129172043_add_index_on_stream_entries.rb diff --git a/db/migrate/20171129172043_add_index_on_stream_entries.rb b/db/migrate/20171129172043_add_index_on_stream_entries.rb new file mode 100644 index 0000000000..478530c7fd --- /dev/null +++ b/db/migrate/20171129172043_add_index_on_stream_entries.rb @@ -0,0 +1,7 @@ +class AddIndexOnStreamEntries < ActiveRecord::Migration[5.1] + def change + commit_db_transaction + add_index :stream_entries, [:account_id, :activity_type, :id], algorithm: :concurrently + remove_index :stream_entries, name: :index_stream_entries_on_account_id + end +end diff --git a/db/schema.rb b/db/schema.rb index f2ef60375c..71674ece62 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20171125190735) do +ActiveRecord::Schema.define(version: 20171129172043) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -431,7 +431,7 @@ ActiveRecord::Schema.define(version: 20171125190735) do t.datetime "updated_at", null: false t.boolean "hidden", default: false, null: false t.bigint "account_id" - t.index ["account_id"], name: "index_stream_entries_on_account_id" + t.index ["account_id", "activity_type", "id"], name: "index_stream_entries_on_account_id_and_activity_type_and_id" t.index ["activity_id", "activity_type"], name: "index_stream_entries_on_activity_id_and_activity_type" end From 85e97ecab6db67faefb64557af9b2271d2b23735 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 30 Nov 2017 03:50:05 +0100 Subject: [PATCH 2/3] Fix too many forwards (#5854) * Avoid sending explicit Undo->Announce when original deleted * Do not forward a reply back to the server that sent it * Deduplicate inboxes of rebloggers' followers for delete forwarding * Adjust test * Fix wrong class, bad SQL, wrong variable, outdated comment --- app/lib/activitypub/activity/create.rb | 2 +- app/lib/activitypub/activity/delete.rb | 7 +++++-- app/models/account.rb | 4 ++++ app/services/remove_status_service.rb | 12 +++++++++--- app/workers/activitypub/raw_distribution_worker.rb | 4 ++-- spec/lib/activitypub/activity/delete_spec.rb | 13 ++++++------- 6 files changed, 27 insertions(+), 15 deletions(-) diff --git a/app/lib/activitypub/activity/create.rb b/app/lib/activitypub/activity/create.rb index 66e4f7c5ec..3e95cbd062 100644 --- a/app/lib/activitypub/activity/create.rb +++ b/app/lib/activitypub/activity/create.rb @@ -210,7 +210,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity def forward_for_reply return unless @json['signature'].present? && reply_to_local? - ActivityPub::RawDistributionWorker.perform_async(Oj.dump(@json), replied_to_status.account_id) + ActivityPub::RawDistributionWorker.perform_async(Oj.dump(@json), replied_to_status.account_id, [@account.preferred_inbox_url]) end def lock_options diff --git a/app/lib/activitypub/activity/delete.rb b/app/lib/activitypub/activity/delete.rb index 4c6afb0902..d0fb493425 100644 --- a/app/lib/activitypub/activity/delete.rb +++ b/app/lib/activitypub/activity/delete.rb @@ -30,8 +30,11 @@ class ActivityPub::Activity::Delete < ActivityPub::Activity def forward_for_reblogs(status) return if @json['signature'].blank? - ActivityPub::RawDistributionWorker.push_bulk(status.reblogs.includes(:account).references(:account).merge(Account.local).pluck(:account_id)) do |account_id| - [payload, account_id] + rebloggers_ids = status.reblogs.includes(:account).references(:account).merge(Account.local).pluck(:account_id) + inboxes = Account.where(id: ::Follow.where(target_account_id: rebloggers_ids).select(:account_id)).inboxes - [@account.preferred_inbox_url] + + ActivityPub::DeliveryWorker.push_bulk(inboxes) do |inbox_url| + [payload, rebloggers_ids.first, inbox_url] end end diff --git a/app/models/account.rb b/app/models/account.rb index f3f604eda0..b6d46b2cc7 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -214,6 +214,10 @@ class Account < ApplicationRecord Rails.cache.fetch("exclude_domains_for:#{id}") { domain_blocks.pluck(:domain) } end + def preferred_inbox_url + shared_inbox_url.presence || inbox_url + end + class << self def readonly_attributes super - %w(statuses_count following_count followers_count) diff --git a/app/services/remove_status_service.rb b/app/services/remove_status_service.rb index c75627205f..9f603bb36d 100644 --- a/app/services/remove_status_service.rb +++ b/app/services/remove_status_service.rb @@ -3,7 +3,7 @@ class RemoveStatusService < BaseService include StreamEntryRenderer - def call(status) + def call(status, options = {}) @payload = Oj.dump(event: :delete, payload: status.id.to_s) @status = status @account = status.account @@ -11,6 +11,7 @@ class RemoveStatusService < BaseService @mentions = status.mentions.includes(:account).to_a @reblogs = status.reblogs.to_a @stream_entry = status.stream_entry + @options = options remove_from_self if status.account.local? remove_from_followers @@ -22,7 +23,12 @@ class RemoveStatusService < BaseService @status.destroy! - return unless @account.local? + # There is no reason to send out Undo activities when the + # cause is that the original object has been removed, since + # original object being removed implicitly removes reblogs + # of it. The Delete activity of the original is forwarded + # separately. + return if !@account.local? || @options[:original_removed] remove_from_remote_followers remove_from_remote_affected @@ -104,7 +110,7 @@ class RemoveStatusService < BaseService # without us being able to do all the fancy stuff @reblogs.each do |reblog| - RemoveStatusService.new.call(reblog) + RemoveStatusService.new.call(reblog, original_removed: true) end end diff --git a/app/workers/activitypub/raw_distribution_worker.rb b/app/workers/activitypub/raw_distribution_worker.rb index d73466f6e5..41e61132fb 100644 --- a/app/workers/activitypub/raw_distribution_worker.rb +++ b/app/workers/activitypub/raw_distribution_worker.rb @@ -5,10 +5,10 @@ class ActivityPub::RawDistributionWorker sidekiq_options queue: 'push' - def perform(json, source_account_id) + def perform(json, source_account_id, exclude_inboxes = []) @account = Account.find(source_account_id) - ActivityPub::DeliveryWorker.push_bulk(inboxes) do |inbox_url| + ActivityPub::DeliveryWorker.push_bulk(inboxes - exclude_inboxes) do |inbox_url| [json, @account.id, inbox_url] end rescue ActiveRecord::RecordNotFound diff --git a/spec/lib/activitypub/activity/delete_spec.rb b/spec/lib/activitypub/activity/delete_spec.rb index 38254e31c3..37b93ecf73 100644 --- a/spec/lib/activitypub/activity/delete_spec.rb +++ b/spec/lib/activitypub/activity/delete_spec.rb @@ -1,8 +1,8 @@ require 'rails_helper' RSpec.describe ActivityPub::Activity::Delete do - let(:sender) { Fabricate(:account, domain: 'example.com') } - let(:status) { Fabricate(:status, account: sender, uri: 'foobar') } + let(:sender) { Fabricate(:account, domain: 'example.com') } + let(:status) { Fabricate(:status, account: sender, uri: 'foobar') } let(:json) do { @@ -30,13 +30,13 @@ RSpec.describe ActivityPub::Activity::Delete do context 'when the status has been reblogged' do describe '#perform' do subject { described_class.new(json, sender) } - let(:reblogger) { Fabricate(:account) } - let(:follower) { Fabricate(:account, username: 'follower', protocol: :activitypub, domain: 'example.com', inbox_url: 'http://example.com/inbox') } + let!(:reblogger) { Fabricate(:account) } + let!(:follower) { Fabricate(:account, username: 'follower', protocol: :activitypub, domain: 'example.com', inbox_url: 'http://example.com/inbox') } + let!(:reblog) { Fabricate(:status, account: reblogger, reblog: status) } before do stub_request(:post, 'http://example.com/inbox').to_return(status: 200) follower.follow!(reblogger) - Fabricate(:status, account: reblogger, reblog: status) subject.perform end @@ -45,8 +45,7 @@ RSpec.describe ActivityPub::Activity::Delete do end it 'sends delete activity to followers of rebloggers' do - # one for Delete original post, and one for Undo reblog (normal delivery) - expect(a_request(:post, 'http://example.com/inbox')).to have_been_made.twice + expect(a_request(:post, 'http://example.com/inbox')).to have_been_made.once end end end From 4c6b5dbe96b565d3db3cbf0912f3b9911bc3922a Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Thu, 30 Nov 2017 04:06:20 +0100 Subject: [PATCH 3/3] Add semi-support for Video/Image objects in ActivityPub (#5848) * Add semi-support for Video/Image objects in ActivityPub Video and Image objects will create corresponding status records with manually crafted text contents (title + URL) * Extract html-url-finding logic into JsonLdHelper * Fallback to id when url missing, extract supported object types --- app/helpers/jsonld_helper.rb | 18 +++++++ app/lib/activitypub/activity/create.rb | 51 ++++++++++++++----- app/lib/formatter.rb | 15 +++--- .../fetch_remote_status_service.rb | 2 +- .../activitypub/process_account_service.rb | 7 +-- .../fetch_remote_status_service_spec.rb | 36 +++++++++++++ 6 files changed, 103 insertions(+), 26 deletions(-) diff --git a/app/helpers/jsonld_helper.rb b/app/helpers/jsonld_helper.rb index a3441e6f93..6c7c38070a 100644 --- a/app/helpers/jsonld_helper.rb +++ b/app/helpers/jsonld_helper.rb @@ -9,6 +9,24 @@ module JsonLdHelper value.is_a?(Array) ? value.first : value 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) + single_value = if value.is_a?(Array) && !value.first.is_a?(String) + value.find { |link| preferred_type.nil? || ((link['mimeType'].presence || 'text/html') == preferred_type) } + elsif value.is_a?(Array) + value.first + else + value + end + + if single_value.nil? || single_value.is_a?(String) + single_value + else + single_value['href'] + end + end + def as_array(value) value.is_a?(Array) ? value : [value] end diff --git a/app/lib/activitypub/activity/create.rb b/app/lib/activitypub/activity/create.rb index 3e95cbd062..31e0abe39c 100644 --- a/app/lib/activitypub/activity/create.rb +++ b/app/lib/activitypub/activity/create.rb @@ -1,6 +1,9 @@ # frozen_string_literal: true class ActivityPub::Activity::Create < ActivityPub::Activity + SUPPORTED_TYPES = %w(Article Note).freeze + CONVERTED_TYPES = %w(Image Video).freeze + def perform return if delete_arrived_first?(object_uri) || unsupported_object_type? @@ -41,7 +44,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity url: object_url || @object['id'], account: @account, text: text_from_content || '', - language: language_from_content, + language: detected_language, spoiler_text: @object['summary'] || '', created_at: @options[:override_timestamps] ? nil : @object['published'], reply: @object['inReplyTo'].present?, @@ -165,40 +168,62 @@ class ActivityPub::Activity::Create < ActivityPub::Activity end def text_from_content + return Formatter.instance.linkify([text_from_name, object_url || @object['id']].join(' ')) if converted_object_type? + if @object['content'].present? @object['content'] - elsif language_map? + elsif content_language_map? @object['contentMap'].values.first end end - def language_from_content - return LanguageDetector.instance.detect(text_from_content, @account) unless language_map? - @object['contentMap'].keys.first + def text_from_name + if @object['name'].present? + @object['name'] + elsif name_language_map? + @object['nameMap'].values.first + end + end + + def detected_language + if content_language_map? + @object['contentMap'].keys.first + elsif name_language_map? + @object['nameMap'].keys.first + elsif supported_object_type? + LanguageDetector.instance.detect(text_from_content, @account) + end end def object_url return if @object['url'].blank? - - value = first_of_value(@object['url']) - - return value if value.is_a?(String) - - value['href'] + url_to_href(@object['url'], 'text/html') end - def language_map? + def content_language_map? @object['contentMap'].is_a?(Hash) && !@object['contentMap'].empty? end + def name_language_map? + @object['nameMap'].is_a?(Hash) && !@object['nameMap'].empty? + end + def unsupported_object_type? - @object.is_a?(String) || !%w(Article Note).include?(@object['type']) + @object.is_a?(String) || !(supported_object_type? || converted_object_type?) end def unsupported_media_type?(mime_type) mime_type.present? && !(MediaAttachment::IMAGE_MIME_TYPES + MediaAttachment::VIDEO_MIME_TYPES).include?(mime_type) end + def supported_object_type? + SUPPORTED_TYPES.include?(@object['type']) + end + + def converted_object_type? + CONVERTED_TYPES.include?(@object['type']) + end + def skip_download? return @skip_download if defined?(@skip_download) @skip_download ||= DomainBlock.find_by(domain: @account.domain)&.reject_media? diff --git a/app/lib/formatter.rb b/app/lib/formatter.rb index 733a1c4b71..9d8bc52db1 100644 --- a/app/lib/formatter.rb +++ b/app/lib/formatter.rb @@ -51,12 +51,7 @@ class Formatter def simplified_format(account) return reformat(account.note).html_safe unless account.local? # rubocop:disable Rails/OutputSafety - - html = encode_and_link_urls(account.note) - html = simple_format(html, {}, sanitize: false) - html = html.delete("\n") - - html.html_safe # rubocop:disable Rails/OutputSafety + linkify(account.note) end def sanitize(html, config) @@ -69,6 +64,14 @@ class Formatter html.html_safe # rubocop:disable Rails/OutputSafety end + def linkify(text) + html = encode_and_link_urls(text) + html = simple_format(html, {}, sanitize: false) + html = html.delete("\n") + + html.html_safe # rubocop:disable Rails/OutputSafety + end + private def encode(html) diff --git a/app/services/activitypub/fetch_remote_status_service.rb b/app/services/activitypub/fetch_remote_status_service.rb index 8de9283de5..7649bceca9 100644 --- a/app/services/activitypub/fetch_remote_status_service.rb +++ b/app/services/activitypub/fetch_remote_status_service.rb @@ -42,7 +42,7 @@ class ActivityPub::FetchRemoteStatusService < BaseService end def expected_type? - %w(Note Article).include? @json['type'] + (ActivityPub::Activity::Create::SUPPORTED_TYPES + ActivityPub::Activity::Create::CONVERTED_TYPES).include? @json['type'] end def needs_update(actor) diff --git a/app/services/activitypub/process_account_service.rb b/app/services/activitypub/process_account_service.rb index 5ee7d89ee9..06ca755636 100644 --- a/app/services/activitypub/process_account_service.rb +++ b/app/services/activitypub/process_account_service.rb @@ -107,12 +107,7 @@ class ActivityPub::ProcessAccountService < BaseService def url return if @json['url'].blank? - - value = first_of_value(@json['url']) - - return value if value.is_a?(String) - - value['href'] + url_to_href(@json['url'], 'text/html') end def outbox_total_items diff --git a/spec/services/activitypub/fetch_remote_status_service_spec.rb b/spec/services/activitypub/fetch_remote_status_service_spec.rb index 51f3fe3a18..ad26abc5b2 100644 --- a/spec/services/activitypub/fetch_remote_status_service_spec.rb +++ b/spec/services/activitypub/fetch_remote_status_service_spec.rb @@ -1,6 +1,8 @@ require 'rails_helper' RSpec.describe ActivityPub::FetchRemoteStatusService do + include ActionView::Helpers::TextHelper + let(:sender) { Fabricate(:account) } let(:recipient) { Fabricate(:account) } let(:valid_domain) { Rails.configuration.x.local_domain } @@ -19,6 +21,7 @@ RSpec.describe ActivityPub::FetchRemoteStatusService do describe '#call' do before do + stub_request(:head, 'https://example.com/watch?v=12345').to_return(status: 404, body: '') subject.call(object[:id], prefetched_body: Oj.dump(object)) end @@ -32,5 +35,38 @@ RSpec.describe ActivityPub::FetchRemoteStatusService do expect(status.text).to eq 'Lorem ipsum' end end + + context 'with Video object' do + let(:object) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: "https://#{valid_domain}/@foo/1234", + type: 'Video', + name: 'Nyan Cat 10 hours remix', + attributedTo: ActivityPub::TagManager.instance.uri_for(sender), + url: [ + { + type: 'Link', + mimeType: 'application/x-bittorrent', + href: 'https://example.com/12345.torrent', + }, + + { + type: 'Link', + mimeType: 'text/html', + href: 'https://example.com/watch?v=12345', + }, + ], + } + end + + it 'creates status' do + status = sender.statuses.first + + expect(status).to_not be_nil + expect(status.url).to eq 'https://example.com/watch?v=12345' + expect(strip_tags(status.text)).to eq 'Nyan Cat 10 hours remix https://example.com/watch?v=12345' + end + end end end