From e7adbf572a50f77590f889bb4d7cb9efb6fc7036 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 6 Sep 2017 19:01:28 +0200 Subject: [PATCH] Switch to static URIs, new URI format in both protocols for new statuses (#4815) * Decouple Status#local? from uri being nil * Replace on-the-fly URI generation with stored URIs - Generate URI in after_save hook for local statuses - Use static value in TagManager when available, fallback to tag format - Make TagManager use ActivityPub::TagManager to understand new format - Adjust tests * Use other heuristic for locality of old statuses, do not perform long query * Exclude tombstone stream entries from Atom feed * Prevent nil statuses from landing in Pubsubhubbub::DistributionWorker * Fix URI not being saved (#4818) * Add more specs for Status * Save generated uri immediately and also fix method order to minimize diff. * Fix alternate HTML URL in Atom * Fix tests * Remove not-null constraint from statuses migration to speed it up --- app/controllers/accounts_controller.rb | 2 +- app/lib/ostatus/atom_serializer.rb | 4 +-- app/lib/tag_manager.rb | 13 +++++--- app/models/status.rb | 14 ++++++++- .../pubsubhubbub/distribution_worker.rb | 2 +- .../20170905165803_add_local_to_statuses.rb | 5 +++ db/schema.rb | 3 +- spec/fabricators/status_fabricator.rb | 4 +++ spec/lib/activitypub/activity/delete_spec.rb | 2 +- spec/lib/activitypub/activity/undo_spec.rb | 2 +- spec/lib/formatter_spec.rb | 4 +-- spec/lib/ostatus/atom_serializer_spec.rb | 31 +++++++++---------- spec/lib/tag_manager_spec.rb | 15 ++------- spec/models/status_spec.rb | 27 ++++++++++++++-- spec/services/fetch_link_card_service_spec.rb | 2 +- 15 files changed, 84 insertions(+), 46 deletions(-) create mode 100644 db/migrate/20170905165803_add_local_to_statuses.rb diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index afa0417fa3..26ab6636b5 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -22,7 +22,7 @@ class AccountsController < ApplicationController format.atom do @entries = @account.stream_entries.where(hidden: false).with_includes.paginate_by_max_id(20, params[:max_id], params[:since_id]) - render xml: OStatus::AtomSerializer.render(OStatus::AtomSerializer.new.feed(@account, @entries.to_a)) + render xml: OStatus::AtomSerializer.render(OStatus::AtomSerializer.new.feed(@account, @entries.reject { |entry| entry.status.nil? })) end format.json do diff --git a/app/lib/ostatus/atom_serializer.rb b/app/lib/ostatus/atom_serializer.rb index 81fae4140e..b8e22a3813 100644 --- a/app/lib/ostatus/atom_serializer.rb +++ b/app/lib/ostatus/atom_serializer.rb @@ -65,7 +65,7 @@ class OStatus::AtomSerializer add_namespaces(entry) if root - append_element(entry, 'id', TagManager.instance.unique_tag(stream_entry.created_at, stream_entry.activity_id, stream_entry.activity_type)) + append_element(entry, 'id', TagManager.instance.uri_for(stream_entry.status)) append_element(entry, 'published', stream_entry.created_at.iso8601) append_element(entry, 'updated', stream_entry.updated_at.iso8601) append_element(entry, 'title', stream_entry&.status&.title || "#{stream_entry.account.acct} deleted status") @@ -86,7 +86,7 @@ class OStatus::AtomSerializer serialize_status_attributes(entry, stream_entry.status) end - append_element(entry, 'link', nil, rel: :alternate, type: 'text/html', href: account_stream_entry_url(stream_entry.account, stream_entry)) + append_element(entry, 'link', nil, rel: :alternate, type: 'text/html', href: TagManager.instance.url_for(stream_entry.status)) append_element(entry, 'link', nil, rel: :self, type: 'application/atom+xml', href: account_stream_entry_url(stream_entry.account, stream_entry, format: 'atom')) append_element(entry, 'thr:in-reply-to', nil, ref: TagManager.instance.uri_for(stream_entry.thread), href: TagManager.instance.url_for(stream_entry.thread)) if stream_entry.threaded? append_element(entry, 'ostatus:conversation', nil, ref: conversation_uri(stream_entry.status.conversation)) unless stream_entry&.status&.conversation_id.nil? diff --git a/app/lib/tag_manager.rb b/app/lib/tag_manager.rb index 5f87a2a48d..f33a20c6f7 100644 --- a/app/lib/tag_manager.rb +++ b/app/lib/tag_manager.rb @@ -49,12 +49,17 @@ class TagManager def unique_tag_to_local_id(tag, expected_type) return nil unless local_id?(tag) - matches = Regexp.new("objectId=([\\d]+):objectType=#{expected_type}").match(tag) - return matches[1] unless matches.nil? + + if ActivityPub::TagManager.instance.local_uri?(tag) + ActivityPub::TagManager.instance.uri_to_local_id(tag) + else + matches = Regexp.new("objectId=([\\d]+):objectType=#{expected_type}").match(tag) + return matches[1] unless matches.nil? + end end def local_id?(id) - id.start_with?("tag:#{Rails.configuration.x.local_domain}") + id.start_with?("tag:#{Rails.configuration.x.local_domain}") || ActivityPub::TagManager.instance.local_uri?(id) end def web_domain?(domain) @@ -92,7 +97,7 @@ class TagManager when :person account_url(target) when :note, :comment, :activity - unique_tag(target.created_at, target.id, 'Status') + target.uri || unique_tag(target.created_at, target.id, 'Status') end end diff --git a/app/models/status.rb b/app/models/status.rb index f44f79aafd..53eff0377e 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -22,6 +22,7 @@ # reblogs_count :integer default(0), not null # language :string # conversation_id :integer +# local :boolean default(FALSE) # class Status < ApplicationRecord @@ -84,7 +85,7 @@ class Status < ApplicationRecord end def local? - uri.nil? + attributes['local'] || uri.nil? end def reblog? @@ -131,11 +132,14 @@ class Status < ApplicationRecord !sensitive? && media_attachments.any? end + after_create :store_uri, if: :local? + before_validation :prepare_contents, if: :local? before_validation :set_reblog before_validation :set_visibility before_validation :set_conversation before_validation :set_sensitivity + before_validation :set_local class << self def not_in_filtered_languages(account) @@ -253,6 +257,10 @@ class Status < ApplicationRecord private + def store_uri + update_attribute(:uri, ActivityPub::TagManager.instance.uri_for(self)) if uri.nil? + end + def prepare_contents text&.strip! spoiler_text&.strip! @@ -292,4 +300,8 @@ class Status < ApplicationRecord thread.account_id end end + + def set_local + self.local = account.local? + end end diff --git a/app/workers/pubsubhubbub/distribution_worker.rb b/app/workers/pubsubhubbub/distribution_worker.rb index 2a5e60fa0e..524f6849fc 100644 --- a/app/workers/pubsubhubbub/distribution_worker.rb +++ b/app/workers/pubsubhubbub/distribution_worker.rb @@ -6,7 +6,7 @@ class Pubsubhubbub::DistributionWorker sidekiq_options queue: 'push' def perform(stream_entry_ids) - stream_entries = StreamEntry.where(id: stream_entry_ids).includes(:status).reject { |e| e.status&.direct_visibility? } + stream_entries = StreamEntry.where(id: stream_entry_ids).includes(:status).reject { |e| e.status.nil? || e.status.direct_visibility? } return if stream_entries.empty? diff --git a/db/migrate/20170905165803_add_local_to_statuses.rb b/db/migrate/20170905165803_add_local_to_statuses.rb new file mode 100644 index 0000000000..e89a0469de --- /dev/null +++ b/db/migrate/20170905165803_add_local_to_statuses.rb @@ -0,0 +1,5 @@ +class AddLocalToStatuses < ActiveRecord::Migration[5.1] + def change + add_column :statuses, :local, :boolean, null: true, default: false + end +end diff --git a/db/schema.rb b/db/schema.rb index ea7edc2f9c..21bde2086b 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: 20170905044538) do +ActiveRecord::Schema.define(version: 20170905165803) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -315,6 +315,7 @@ ActiveRecord::Schema.define(version: 20170905044538) do t.integer "reblogs_count", default: 0, null: false t.string "language" t.bigint "conversation_id" + t.boolean "local", default: false t.index ["account_id", "id"], name: "index_statuses_on_account_id_id" t.index ["conversation_id"], name: "index_statuses_on_conversation_id" t.index ["in_reply_to_id"], name: "index_statuses_on_in_reply_to_id" diff --git a/spec/fabricators/status_fabricator.rb b/spec/fabricators/status_fabricator.rb index 8ec5f4ba79..04bbbcf4b0 100644 --- a/spec/fabricators/status_fabricator.rb +++ b/spec/fabricators/status_fabricator.rb @@ -1,4 +1,8 @@ Fabricator(:status) do account text "Lorem ipsum dolor sit amet" + + after_build do |status| + status.uri = Faker::Internet.device_token if !status.account.local? && status.uri.nil? + end end diff --git a/spec/lib/activitypub/activity/delete_spec.rb b/spec/lib/activitypub/activity/delete_spec.rb index 65e743abb2..38254e31c3 100644 --- a/spec/lib/activitypub/activity/delete_spec.rb +++ b/spec/lib/activitypub/activity/delete_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' RSpec.describe ActivityPub::Activity::Delete do - let(:sender) { Fabricate(:account) } + let(:sender) { Fabricate(:account, domain: 'example.com') } let(:status) { Fabricate(:status, account: sender, uri: 'foobar') } let(:json) do diff --git a/spec/lib/activitypub/activity/undo_spec.rb b/spec/lib/activitypub/activity/undo_spec.rb index 4629a033f2..14c68efe5f 100644 --- a/spec/lib/activitypub/activity/undo_spec.rb +++ b/spec/lib/activitypub/activity/undo_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' RSpec.describe ActivityPub::Activity::Undo do - let(:sender) { Fabricate(:account) } + let(:sender) { Fabricate(:account, domain: 'example.com') } let(:json) do { diff --git a/spec/lib/formatter_spec.rb b/spec/lib/formatter_spec.rb index dfe1d8b8fe..ab04ccbabb 100644 --- a/spec/lib/formatter_spec.rb +++ b/spec/lib/formatter_spec.rb @@ -178,7 +178,7 @@ RSpec.describe Formatter do end context 'with remote status' do - let(:status) { Fabricate(:status, text: 'Beep boop', uri: 'beepboop') } + let(:status) { Fabricate(:status, account: remote_account, text: 'Beep boop') } it 'reformats' do is_expected.to eq 'Beep boop' @@ -226,7 +226,7 @@ RSpec.describe Formatter do end context 'with remote status' do - let(:status) { Fabricate(:status, text: '', uri: 'beep boop') } + let(:status) { Fabricate(:status, account: remote_account, text: '') } it 'returns tag-stripped text' do is_expected.to eq '' diff --git a/spec/lib/ostatus/atom_serializer_spec.rb b/spec/lib/ostatus/atom_serializer_spec.rb index 301a0ce30d..0451eceeb4 100644 --- a/spec/lib/ostatus/atom_serializer_spec.rb +++ b/spec/lib/ostatus/atom_serializer_spec.rb @@ -403,8 +403,7 @@ RSpec.describe OStatus::AtomSerializer do it 'returns element whose rendered view triggers creation when processed' do remote_account = Account.create!(username: 'username') - remote_status = Fabricate(:status, account: remote_account) - remote_status.stream_entry.update!(created_at: '2000-01-01T00:00:00Z') + remote_status = Fabricate(:status, account: remote_account, created_at: '2000-01-01T00:00:00Z') entry = OStatus::AtomSerializer.new.entry(remote_status.stream_entry, true) entry.nodes.delete_if { |node| node[:type] == 'application/activity+json' } # Remove ActivityPub link to simplify test @@ -421,7 +420,7 @@ RSpec.describe OStatus::AtomSerializer do ProcessFeedService.new.call(xml, account) - expect(Status.find_by(uri: "tag:remote,2000-01-01:objectId=#{remote_status.id}:objectType=Status")).to be_instance_of Status + expect(Status.find_by(uri: "https://remote/users/#{remote_status.account.to_param}/statuses/#{remote_status.id}")).to be_instance_of Status end end @@ -465,12 +464,11 @@ RSpec.describe OStatus::AtomSerializer do end it 'appends id element with unique tag' do - status = Fabricate(:status, reblog_of_id: nil) - status.stream_entry.update!(created_at: '2000-01-01T00:00:00Z') + status = Fabricate(:status, reblog_of_id: nil, created_at: '2000-01-01T00:00:00Z') entry = OStatus::AtomSerializer.new.entry(status.stream_entry) - expect(entry.id.text).to eq "tag:cb6e6126.ngrok.io,2000-01-01:objectId=#{status.id}:objectType=Status" + expect(entry.id.text).to eq "https://cb6e6126.ngrok.io/users/#{status.account.to_param}/statuses/#{status.id}" end it 'appends published element with created date' do @@ -515,7 +513,7 @@ RSpec.describe OStatus::AtomSerializer do entry = OStatus::AtomSerializer.new.entry(reblog.stream_entry) object = entry.nodes.find { |node| node.name == 'activity:object' } - expect(object.id.text).to eq "tag:cb6e6126.ngrok.io,2000-01-01:objectId=#{reblogged.id}:objectType=Status" + expect(object.id.text).to eq "https://cb6e6126.ngrok.io/users/#{reblogged.account.to_param}/statuses/#{reblogged.id}" end it 'does not append activity:object element if target is not present' do @@ -532,7 +530,7 @@ RSpec.describe OStatus::AtomSerializer do link = entry.nodes.find { |node| node.name == 'link' && node[:rel] == 'alternate' && node[:type] == 'text/html' } expect(link[:type]).to eq 'text/html' - expect(link[:href]).to eq "https://cb6e6126.ngrok.io/users/username/updates/#{status.stream_entry.id}" + expect(link[:href]).to eq "https://cb6e6126.ngrok.io/@username/#{status.id}" end it 'appends link element for itself' do @@ -553,7 +551,7 @@ RSpec.describe OStatus::AtomSerializer do entry = OStatus::AtomSerializer.new.entry(reply_status.stream_entry) in_reply_to = entry.nodes.find { |node| node.name == 'thr:in-reply-to' } - expect(in_reply_to[:ref]).to eq "tag:cb6e6126.ngrok.io,2000-01-01:objectId=#{in_reply_to_status.id}:objectType=Status" + expect(in_reply_to[:ref]).to eq "https://cb6e6126.ngrok.io/users/#{in_reply_to_status.account.to_param}/statuses/#{in_reply_to_status.id}" end it 'does not append thr:in-reply-to element if not threaded' do @@ -934,7 +932,7 @@ RSpec.describe OStatus::AtomSerializer do favourite_salmon = OStatus::AtomSerializer.new.favourite_salmon(favourite) object = favourite_salmon.nodes.find { |node| node.name == 'activity:object' } - expect(object.id.text).to eq "tag:cb6e6126.ngrok.io,2000-01-01:objectId=#{status.id}:objectType=Status" + expect(object.id.text).to eq "https://cb6e6126.ngrok.io/users/#{status.account.to_param}/statuses/#{status.id}" end it 'appends thr:in-reply-to element for status' do @@ -945,7 +943,7 @@ RSpec.describe OStatus::AtomSerializer do favourite_salmon = OStatus::AtomSerializer.new.favourite_salmon(favourite) in_reply_to = favourite_salmon.nodes.find { |node| node.name == 'thr:in-reply-to' } - expect(in_reply_to.ref).to eq "tag:cb6e6126.ngrok.io,2000-01-01:objectId=#{status.id}:objectType=Status" + expect(in_reply_to.ref).to eq "https://cb6e6126.ngrok.io/users/#{status.account.to_param}/statuses/#{status.id}" expect(in_reply_to.href).to eq "https://cb6e6126.ngrok.io/@username/#{status.id}" end @@ -1034,7 +1032,7 @@ RSpec.describe OStatus::AtomSerializer do unfavourite_salmon = OStatus::AtomSerializer.new.unfavourite_salmon(favourite) object = unfavourite_salmon.nodes.find { |node| node.name == 'activity:object' } - expect(object.id.text).to eq "tag:cb6e6126.ngrok.io,2000-01-01:objectId=#{status.id}:objectType=Status" + expect(object.id.text).to eq "https://cb6e6126.ngrok.io/users/#{status.account.to_param}/statuses/#{status.id}" end it 'appends thr:in-reply-to element for status' do @@ -1045,7 +1043,7 @@ RSpec.describe OStatus::AtomSerializer do unfavourite_salmon = OStatus::AtomSerializer.new.unfavourite_salmon(favourite) in_reply_to = unfavourite_salmon.nodes.find { |node| node.name == 'thr:in-reply-to' } - expect(in_reply_to.ref).to eq "tag:cb6e6126.ngrok.io,2000-01-01:objectId=#{status.id}:objectType=Status" + expect(in_reply_to.ref).to eq "https://cb6e6126.ngrok.io/users/#{status.account.to_param}/statuses/#{status.id}" expect(in_reply_to.href).to eq "https://cb6e6126.ngrok.io/@username/#{status.id}" end @@ -1453,7 +1451,7 @@ RSpec.describe OStatus::AtomSerializer do it 'appends id element with URL for status' do status = Fabricate(:status, created_at: '2000-01-01T00:00:00Z') object = OStatus::AtomSerializer.new.object(status) - expect(object.id.text).to eq "tag:cb6e6126.ngrok.io,2000-01-01:objectId=#{status.id}:objectType=Status" + expect(object.id.text).to eq "https://cb6e6126.ngrok.io/users/#{status.account.to_param}/statuses/#{status.id}" end it 'appends published element with created date' do @@ -1463,7 +1461,8 @@ RSpec.describe OStatus::AtomSerializer do end it 'appends updated element with updated date' do - status = Fabricate(:status, updated_at: '2000-01-01T00:00:00Z') + status = Fabricate(:status) + status.updated_at = '2000-01-01T00:00:00Z' object = OStatus::AtomSerializer.new.object(status) expect(object.updated.text).to eq '2000-01-01T00:00:00Z' end @@ -1523,7 +1522,7 @@ RSpec.describe OStatus::AtomSerializer do entry = OStatus::AtomSerializer.new.object(reply) in_reply_to = entry.nodes.find { |node| node.name == 'thr:in-reply-to' } - expect(in_reply_to.ref).to eq "tag:cb6e6126.ngrok.io,2000-01-01:objectId=#{thread.id}:objectType=Status" + expect(in_reply_to.ref).to eq "https://cb6e6126.ngrok.io/users/#{thread.account.to_param}/statuses/#{thread.id}" expect(in_reply_to.href).to eq "https://cb6e6126.ngrok.io/@username/#{thread.id}" end diff --git a/spec/lib/tag_manager_spec.rb b/spec/lib/tag_manager_spec.rb index 1fae6bec45..1cd6e0a6fe 100644 --- a/spec/lib/tag_manager_spec.rb +++ b/spec/lib/tag_manager_spec.rb @@ -157,23 +157,12 @@ RSpec.describe TagManager do describe '#uri_for' do subject { TagManager.instance.uri_for(target) } - context 'activity object' do - let(:target) { Fabricate(:status, reblog: Fabricate(:status)).stream_entry } - - before { target.update!(created_at: '2000-01-01T00:00:00Z') } - - it 'returns the unique tag for status' do - expect(target.object_type).to eq :activity - is_expected.to eq "tag:cb6e6126.ngrok.io,2000-01-01:objectId=#{target.id}:objectType=Status" - end - end - context 'comment object' do let(:target) { Fabricate(:status, created_at: '2000-01-01T00:00:00Z', reply: true) } it 'returns the unique tag for status' do expect(target.object_type).to eq :comment - is_expected.to eq "tag:cb6e6126.ngrok.io,2000-01-01:objectId=#{target.id}:objectType=Status" + is_expected.to eq target.uri end end @@ -182,7 +171,7 @@ RSpec.describe TagManager do it 'returns the unique tag for status' do expect(target.object_type).to eq :note - is_expected.to eq "tag:cb6e6126.ngrok.io,2000-01-01:objectId=#{target.id}:objectType=Status" + is_expected.to eq target.uri end end diff --git a/spec/models/status_spec.rb b/spec/models/status_spec.rb index 626fc3f985..484effd5e1 100644 --- a/spec/models/status_spec.rb +++ b/spec/models/status_spec.rb @@ -13,9 +13,15 @@ RSpec.describe Status, type: :model do end it 'returns false if a remote URI is set' do - subject.uri = 'a' + alice.update(domain: 'example.com') + subject.save expect(subject.local?).to be false end + + it 'returns true if a URI is set and `local` is true' do + subject.update(uri: 'example.com', local: true) + expect(subject.local?).to be true + end end describe '#reblog?' do @@ -495,7 +501,7 @@ RSpec.describe Status, type: :model do end end - describe 'before_create' do + describe 'before_validation' do it 'sets account being replied to correctly over intermediary nodes' do first_status = Fabricate(:status, account: bob) intermediary = Fabricate(:status, thread: first_status, account: alice) @@ -512,5 +518,22 @@ RSpec.describe Status, type: :model do parent = Fabricate(:status, text: 'First') expect(Status.create(account: alice, thread: parent, text: 'Response').conversation_id).to eq parent.conversation_id end + + it 'sets `local` to true for status by local account' do + expect(Status.create(account: alice, text: 'foo').local).to be true + end + + it 'sets `local` to false for status by remote account' do + alice.update(domain: 'example.com') + expect(Status.create(account: alice, text: 'foo').local).to be false + end + end + + describe 'after_create' do + it 'saves ActivityPub uri as uri for local status' do + status = Status.create(account: alice, text: 'foo') + status.reload + expect(status.uri).to start_with('https://') + end end end diff --git a/spec/services/fetch_link_card_service_spec.rb b/spec/services/fetch_link_card_service_spec.rb index 3a0786d03c..b0aa740ac9 100644 --- a/spec/services/fetch_link_card_service_spec.rb +++ b/spec/services/fetch_link_card_service_spec.rb @@ -55,7 +55,7 @@ RSpec.describe FetchLinkCardService do end context 'in a remote status' do - let(:status) { Fabricate(:status, uri: 'abc', text: 'Habt ihr ein paar gute Links zu #Wannacry herumfliegen? Ich will mal unter
https://github.com/qbi/WannaCry was sammeln. !security ') } + let(:status) { Fabricate(:status, account: Fabricate(:account, domain: 'example.com'), text: 'Habt ihr ein paar gute Links zu #Wannacry herumfliegen? Ich will mal unter
https://github.com/qbi/WannaCry was sammeln. !security ') } it 'parses out URLs' do expect(a_request(:head, 'https://github.com/qbi/WannaCry')).to have_been_made.at_least_once