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

@@ -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

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?

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)
+
+    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
 

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

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?
 

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

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"

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

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

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
     {

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: '<script>alert("Hello")</script>', uri: 'beep boop') }
+      let(:status)  { Fabricate(:status, account: remote_account, text: '<script>alert("Hello")</script>') }
 
       it 'returns tag-stripped text' do
         is_expected.to eq ''

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
 

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
 

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

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 #<span class="tag"><a href="https://quitter.se/tag/wannacry" target="_blank" rel="tag noopener" title="https://quitter.se/tag/wannacry">Wannacry</a></span> herumfliegen?   Ich will mal unter <br> <a href="https://github.com/qbi/WannaCry" target="_blank" rel="noopener" title="https://github.com/qbi/WannaCry">https://github.com/qbi/WannaCry</a> was sammeln. !<a href="http://sn.jonkman.ca/group/416/id" target="_blank" rel="noopener" title="http://sn.jonkman.ca/group/416/id">security</a>&nbsp;') }
+    let(:status) { Fabricate(:status, account: Fabricate(:account, domain: 'example.com'), text: 'Habt ihr ein paar gute Links zu #<span class="tag"><a href="https://quitter.se/tag/wannacry" target="_blank" rel="tag noopener" title="https://quitter.se/tag/wannacry">Wannacry</a></span> herumfliegen?   Ich will mal unter <br> <a href="https://github.com/qbi/WannaCry" target="_blank" rel="noopener" title="https://github.com/qbi/WannaCry">https://github.com/qbi/WannaCry</a> was sammeln. !<a href="http://sn.jonkman.ca/group/416/id" target="_blank" rel="noopener" title="http://sn.jonkman.ca/group/416/id">security</a>&nbsp;') }
 
     it 'parses out URLs' do
       expect(a_request(:head, 'https://github.com/qbi/WannaCry')).to have_been_made.at_least_once