]> cat aescling's git repositories - mastodon.git/commitdiff
Improve support for aspects/circles (#8950)
authorEugen Rochko <eugen@zeonfederated.com>
Wed, 17 Oct 2018 15:13:04 +0000 (17:13 +0200)
committerGitHub <noreply@github.com>
Wed, 17 Oct 2018 15:13:04 +0000 (17:13 +0200)
* Add silent column to mentions

* Save silent mentions in ActivityPub Create handler and optimize it

Move networking calls out of the database transaction

* Add "limited" visibility level masked as "private" in the API

Unlike DMs, limited statuses are pushed into home feeds. The access
control rules between direct and limited statuses is almost the same,
except for counter and conversation logic

* Ensure silent column is non-null, add spec

* Ensure filters don't check silent mentions for blocks/mutes

As those are "this person is also allowed to see" rather than "this
person is involved", therefore does not warrant filtering

* Clean up code

* Use Status#active_mentions to limit returned mentions

* Fix code style issues

* Use Status#active_mentions in Notification

And remove stream_entry eager-loading from Notification

23 files changed:
app/lib/activitypub/activity.rb
app/lib/activitypub/activity/create.rb
app/lib/activitypub/tag_manager.rb
app/lib/feed_manager.rb
app/lib/formatter.rb
app/lib/ostatus/atom_serializer.rb
app/models/account_conversation.rb
app/models/mention.rb
app/models/notification.rb
app/models/status.rb
app/models/stream_entry.rb
app/policies/status_policy.rb
app/serializers/activitypub/note_serializer.rb
app/serializers/rest/status_serializer.rb
app/services/batched_remove_status_service.rb
app/services/fan_out_on_write_service.rb
app/services/remove_status_service.rb
app/views/stream_entries/_detailed_status.html.haml
app/workers/activitypub/distribution_worker.rb
app/workers/activitypub/reply_distribution_worker.rb
db/migrate/20181010141500_add_silent_to_mentions.rb [new file with mode: 0644]
db/schema.rb
spec/lib/activitypub/activity/create_spec.rb

index 3a39b723ed13f70b3360bb73e475f52e5330cade..999954cb5bc9944e3b8eff348abf57e3906aa9b1 100644 (file)
@@ -96,7 +96,7 @@ class ActivityPub::Activity
   end
 
   def notify_about_mentions(status)
-    status.mentions.includes(:account).each do |mention|
+    status.active_mentions.includes(:account).each do |mention|
       next unless mention.account.local? && audience_includes?(mention.account)
       NotifyService.new.call(mention.account, mention)
     end
index 73475bf0295ff0b1945795c516bd9cc52ad7bcb5..7e6702a6344ae9123878653ac55677e6c1eddf90 100644 (file)
@@ -28,6 +28,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity
 
     process_status_params
     process_tags
+    process_audience
 
     ApplicationRecord.transaction do
       @status = Status.create!(@params)
@@ -66,6 +67,27 @@ class ActivityPub::Activity::Create < ActivityPub::Activity
     end
   end
 
+  def process_audience
+    (as_array(@object['to']) + as_array(@object['cc'])).uniq.each do |audience|
+      next if audience == ActivityPub::TagManager::COLLECTIONS[:public]
+
+      # Unlike with tags, there is no point in resolving accounts we don't already
+      # know here, because silent mentions would only be used for local access
+      # control anyway
+      account = account_from_uri(audience)
+
+      next if account.nil? || @mentions.any? { |mention| mention.account_id == account.id }
+
+      @mentions << Mention.new(account: account, silent: true)
+
+      # If there is at least one silent mention, then the status can be considered
+      # as a limited-audience status, and not strictly a direct message
+      next unless @params[:visibility] == :direct
+
+      @params[:visibility] = :limited
+    end
+  end
+
   def attach_tags(status)
     @tags.each do |tag|
       status.tags << tag
@@ -113,7 +135,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity
 
     return if account.nil?
 
-    @mentions << Mention.new(account: account)
+    @mentions << Mention.new(account: account, silent: false)
   end
 
   def process_emoji(tag)
index 95d1cf9f35326d7ea64016fcdce55b1a1e784d26..be3a562d00cdf46ec9b758f42389016673cddcc7 100644 (file)
@@ -58,8 +58,8 @@ class ActivityPub::TagManager
       [COLLECTIONS[:public]]
     when 'unlisted', 'private'
       [account_followers_url(status.account)]
-    when 'direct'
-      status.mentions.map { |mention| uri_for(mention.account) }
+    when 'direct', 'limited'
+      status.active_mentions.map { |mention| uri_for(mention.account) }
     end
   end
 
@@ -80,7 +80,7 @@ class ActivityPub::TagManager
       cc << COLLECTIONS[:public]
     end
 
-    cc.concat(status.mentions.map { |mention| uri_for(mention.account) }) unless status.direct_visibility?
+    cc.concat(status.active_mentions.map { |mention| uri_for(mention.account) }) unless status.direct_visibility? || status.limited_visibility?
 
     cc
   end
index b10e5dd244cbd4eee51cb9a2141752f672c47fba..3d7db27211a63dabeaf92b8029bfb53a7cda8943 100644 (file)
@@ -88,7 +88,7 @@ class FeedManager
     end
 
     query.each do |status|
-      next if status.direct_visibility? || filter?(:home, status, into_account)
+      next if status.direct_visibility? || status.limited_visibility? || filter?(:home, status, into_account)
       add_to_feed(:home, into_account.id, status)
     end
 
@@ -156,12 +156,12 @@ class FeedManager
     return true  if status.reply? && (status.in_reply_to_id.nil? || status.in_reply_to_account_id.nil?)
     return true  if phrase_filtered?(status, receiver_id, :home)
 
-    check_for_blocks = status.mentions.pluck(:account_id)
+    check_for_blocks = status.active_mentions.pluck(:account_id)
     check_for_blocks.concat([status.account_id])
 
     if status.reblog?
       check_for_blocks.concat([status.reblog.account_id])
-      check_for_blocks.concat(status.reblog.mentions.pluck(:account_id))
+      check_for_blocks.concat(status.reblog.active_mentions.pluck(:account_id))
     end
 
     return true if blocks_or_mutes?(receiver_id, check_for_blocks, :home)
@@ -188,7 +188,7 @@ class FeedManager
     # This filter is called from NotifyService, but already after the sender of
     # the notification has been checked for mute/block. Therefore, it's not
     # necessary to check the author of the toot for mute/block again
-    check_for_blocks = status.mentions.pluck(:account_id)
+    check_for_blocks = status.active_mentions.pluck(:account_id)
     check_for_blocks.concat([status.in_reply_to_account]) if status.reply? && !status.in_reply_to_account_id.nil?
 
     should_filter   = blocks_or_mutes?(receiver_id, check_for_blocks, :mentions)                                                         # Filter if it's from someone I blocked, in reply to someone I blocked, or mentioning someone I blocked (or muted)
index 35d5a09b766933269c74bb2a86e00b136dbea04f..d13884ec8186817673ed308e82516e996c286df7 100644 (file)
@@ -27,7 +27,7 @@ class Formatter
       return html.html_safe # rubocop:disable Rails/OutputSafety
     end
 
-    linkable_accounts = status.mentions.map(&:account)
+    linkable_accounts = status.active_mentions.map(&:account)
     linkable_accounts << status.account
 
     html = raw_content
index 1a0a635b3dceef172d1d334809047edceecce6c1..7a181fb404563e0fa845b3138611b253b2b3fde4 100644 (file)
@@ -354,7 +354,7 @@ class OStatus::AtomSerializer
     append_element(entry, 'summary', status.spoiler_text, 'xml:lang': status.language) if status.spoiler_text?
     append_element(entry, 'content', Formatter.instance.format(status).to_str || '.', type: 'html', 'xml:lang': status.language)
 
-    status.mentions.sort_by(&:id).each do |mentioned|
+    status.active_mentions.sort_by(&:id).each do |mentioned|
       append_element(entry, 'link', nil, rel: :mentioned, 'ostatus:object-type': OStatus::TagManager::TYPES[:person], href: OStatus::TagManager.instance.uri_for(mentioned.account))
     end
 
index a7205ec1a8b8e5438537a7460f29d2c97fe8a43b..c12c8d233fbc9113952c8c2d8bfc4c6d6d463163 100644 (file)
@@ -85,7 +85,7 @@ class AccountConversation < ApplicationRecord
     private
 
     def participants_from_status(recipient, status)
-      ((status.mentions.pluck(:account_id) + [status.account_id]).uniq - [recipient.id]).sort
+      ((status.active_mentions.pluck(:account_id) + [status.account_id]).uniq - [recipient.id]).sort
     end
   end
 
index 8ab886b18434028f17cbc40486c6a269e599f83e..d01a88e32eb976c16b885a9c8fb406c3ab873188 100644 (file)
@@ -8,6 +8,7 @@
 #  created_at :datetime         not null
 #  updated_at :datetime         not null
 #  account_id :bigint(8)
+#  silent     :boolean          default(FALSE), not null
 #
 
 class Mention < ApplicationRecord
@@ -18,10 +19,17 @@ class Mention < ApplicationRecord
 
   validates :account, uniqueness: { scope: :status }
 
+  scope :active, -> { where(silent: false) }
+  scope :silent, -> { where(silent: true) }
+
   delegate(
     :username,
     :acct,
     to: :account,
     prefix: true
   )
+
+  def active?
+    !silent?
+  end
 end
index b9bec08086c21828846b52fa7a749107ff33ed04..78b180301a5ba27dbf6cf5d42799b16c1d3e6292 100644 (file)
@@ -24,7 +24,7 @@ class Notification < ApplicationRecord
     favourite:      'Favourite',
   }.freeze
 
-  STATUS_INCLUDES = [:account, :application, :stream_entry, :media_attachments, :tags, mentions: :account, reblog: [:stream_entry, :account, :application, :media_attachments, :tags, mentions: :account]].freeze
+  STATUS_INCLUDES = [:account, :application, :media_attachments, :tags, active_mentions: :account, reblog: [:account, :application, :media_attachments, :tags, active_mentions: :account]].freeze
 
   belongs_to :account, optional: true
   belongs_to :from_account, class_name: 'Account', optional: true
index f61bd0fee41e0dea7b3a2619c01d7db69fdb5620..b18cb56b21d43fd8d250a6d4e3e21caf77e58bf3 100644 (file)
@@ -37,7 +37,7 @@ class Status < ApplicationRecord
 
   update_index('statuses#status', :proper) if Chewy.enabled?
 
-  enum visibility: [:public, :unlisted, :private, :direct], _suffix: :visibility
+  enum visibility: [:public, :unlisted, :private, :direct, :limited], _suffix: :visibility
 
   belongs_to :application, class_name: 'Doorkeeper::Application', optional: true
 
@@ -51,7 +51,8 @@ class Status < ApplicationRecord
   has_many :favourites, inverse_of: :status, dependent: :destroy
   has_many :reblogs, foreign_key: 'reblog_of_id', class_name: 'Status', inverse_of: :reblog, dependent: :destroy
   has_many :replies, foreign_key: 'in_reply_to_id', class_name: 'Status', inverse_of: :thread
-  has_many :mentions, dependent: :destroy
+  has_many :mentions, dependent: :destroy, inverse_of: :status
+  has_many :active_mentions, -> { active }, class_name: 'Mention', inverse_of: :status
   has_many :media_attachments, dependent: :nullify
 
   has_and_belongs_to_many :tags
@@ -89,7 +90,7 @@ class Status < ApplicationRecord
                    :status_stat,
                    :tags,
                    :stream_entry,
-                   mentions: :account,
+                   active_mentions: :account,
                    reblog: [
                      :account,
                      :application,
@@ -98,7 +99,7 @@ class Status < ApplicationRecord
                      :media_attachments,
                      :conversation,
                      :status_stat,
-                     mentions: :account,
+                     active_mentions: :account,
                    ],
                    thread: :account
 
@@ -171,7 +172,11 @@ class Status < ApplicationRecord
   end
 
   def hidden?
-    private_visibility? || direct_visibility?
+    private_visibility? || direct_visibility? || limited_visibility?
+  end
+
+  def distributable?
+    public_visibility? || unlisted_visibility?
   end
 
   def with_media?
index a2f273281b6df07e7c5e2c43791114bd01babe86..1a9afc5c7b743264aeec96994b07afefa8ca6caf 100644 (file)
@@ -48,7 +48,7 @@ class StreamEntry < ApplicationRecord
   end
 
   def mentions
-    orphaned? ? [] : status.mentions.map(&:account)
+    orphaned? ? [] : status.active_mentions.map(&:account)
   end
 
   private
index 6addc8a8a8ebe770dca60cb56ab784101ec8dcdb..64a5111fc8f899bbb9e4de6b7da99ee3281f70eb 100644 (file)
@@ -12,7 +12,7 @@ class StatusPolicy < ApplicationPolicy
   end
 
   def show?
-    if direct?
+    if requires_mention?
       owned? || mention_exists?
     elsif private?
       owned? || following_author? || mention_exists?
@@ -22,7 +22,7 @@ class StatusPolicy < ApplicationPolicy
   end
 
   def reblog?
-    !direct? && (!private? || owned?) && show? && !blocking_author?
+    !requires_mention? && (!private? || owned?) && show? && !blocking_author?
   end
 
   def favourite?
@@ -41,8 +41,8 @@ class StatusPolicy < ApplicationPolicy
 
   private
 
-  def direct?
-    record.direct_visibility?
+  def requires_mention?
+    record.direct_visibility? || record.limited_visibility?
   end
 
   def owned?
index 82b7ffe95cb87ea97f0ea15da139d787fc0aec24..c9d23e25fa8543e6d594fdc3bd39f57bbac8dc6a 100644 (file)
@@ -68,7 +68,7 @@ class ActivityPub::NoteSerializer < ActiveModel::Serializer
   end
 
   def virtual_tags
-    object.mentions.to_a.sort_by(&:id) + object.tags + object.emojis
+    object.active_mentions.to_a.sort_by(&:id) + object.tags + object.emojis
   end
 
   def atom_uri
index 61423f9615193d3193be6d7acd640b00cd02b99b..1f2f46b7e68b3b9c18a631162e04b73449e6ba6d 100644 (file)
@@ -36,6 +36,17 @@ class REST::StatusSerializer < ActiveModel::Serializer
     !current_user.nil?
   end
 
+  def visibility
+    # This visibility is masked behind "private"
+    # to avoid API changes because there are no
+    # UX differences
+    if object.limited_visibility?
+      'private'
+    else
+      object.visibility
+    end
+  end
+
   def uri
     OStatus::TagManager.instance.uri_for(object)
   end
@@ -88,7 +99,7 @@ class REST::StatusSerializer < ActiveModel::Serializer
   end
 
   def ordered_mentions
-    object.mentions.to_a.sort_by(&:id)
+    object.active_mentions.to_a.sort_by(&:id)
   end
 
   class ApplicationSerializer < ActiveModel::Serializer
index 2fcb3cc66bb0aa99864c1d9d14711bbf6c79801c..b8ab58938de8ab2fcc57c5da9cad3e983dbb4514 100644 (file)
@@ -12,7 +12,7 @@ class BatchedRemoveStatusService < BaseService
   def call(statuses)
     statuses = Status.where(id: statuses.map(&:id)).includes(:account, :stream_entry).flat_map { |status| [status] + status.reblogs.includes(:account, :stream_entry).to_a }
 
-    @mentions = statuses.map { |s| [s.id, s.mentions.includes(:account).to_a] }.to_h
+    @mentions = statuses.map { |s| [s.id, s.active_mentions.includes(:account).to_a] }.to_h
     @tags     = statuses.map { |s| [s.id, s.tags.pluck(:name)] }.to_h
 
     @stream_entry_batches  = []
index 5ddddf3a904359e2cfdb00cca2a167114aec8f3d..7f2a9177545b30bc199c9c1ed2e194bc0c8df5ee 100644 (file)
@@ -10,6 +10,8 @@ class FanOutOnWriteService < BaseService
 
     if status.direct_visibility?
       deliver_to_own_conversation(status)
+    elsif status.limited_visibility?
+      deliver_to_mentioned_followers(status)
     else
       deliver_to_self(status) if status.account.local?
       deliver_to_followers(status)
@@ -53,6 +55,16 @@ class FanOutOnWriteService < BaseService
     end
   end
 
+  def deliver_to_mentioned_followers(status)
+    Rails.logger.debug "Delivering status #{status.id} to limited followers"
+
+    status.mentions.includes(:account).each do |mention|
+      mentioned_account = mention.account
+      next if !mentioned_account.local? || !mentioned_account.following?(status.account) || FeedManager.instance.filter?(:home, status, mention.account_id)
+      FeedManager.instance.push_to_home(mentioned_account, status)
+    end
+  end
+
   def render_anonymous_payload(status)
     @payload = InlineRenderer.render(status, nil, :status)
     @payload = Oj.dump(event: :update, payload: @payload)
index 1ee645e6d8b266db1618528c215282155842e159..11d28e783d04c86d5c48ccca49370badc3311e34 100644 (file)
@@ -8,7 +8,7 @@ class RemoveStatusService < BaseService
     @status       = status
     @account      = status.account
     @tags         = status.tags.pluck(:name).to_a
-    @mentions     = status.mentions.includes(:account).to_a
+    @mentions     = status.active_mentions.includes(:account).to_a
     @reblogs      = status.reblogs.to_a
     @stream_entry = status.stream_entry
     @options      = options
index 0b204d4374cd9eecd0651aedc3e23a1f2995196d..6e6d0eda858f2c4c8df50db56ed0d7c029620c39 100644 (file)
@@ -51,7 +51,7 @@
     - if status.direct_visibility?
       %span.detailed-status__link<
         = fa_icon('envelope')
-    - elsif status.private_visibility?
+    - elsif status.private_visibility? || status.limited_visibility?
       %span.detailed-status__link<
         = fa_icon('lock')
     - else
index c2bfd4f2f13b43630a87ffd7673f8f90801bcb31..17c1ef7fffb54ef89b5fdcd3d2c577f7a6dd1857 100644 (file)
@@ -23,7 +23,7 @@ class ActivityPub::DistributionWorker
   private
 
   def skip_distribution?
-    @status.direct_visibility?
+    @status.direct_visibility? || @status.limited_visibility?
   end
 
   def relayable?
index fe99fc05f295691d412417fd555c056f1fcf5384..c0ed3a1f40ee58f848793e426db1fa1c892ee2e5 100644 (file)
@@ -9,7 +9,7 @@ class ActivityPub::ReplyDistributionWorker
     @status  = Status.find(status_id)
     @account = @status.thread&.account
 
-    return if @account.nil? || skip_distribution?
+    return unless @account.present? && @status.distributable?
 
     ActivityPub::DeliveryWorker.push_bulk(inboxes) do |inbox_url|
       [signed_payload, @status.account_id, inbox_url]
@@ -20,10 +20,6 @@ class ActivityPub::ReplyDistributionWorker
 
   private
 
-  def skip_distribution?
-    @status.private_visibility? || @status.direct_visibility?
-  end
-
   def inboxes
     @inboxes ||= @account.followers.inboxes
   end
diff --git a/db/migrate/20181010141500_add_silent_to_mentions.rb b/db/migrate/20181010141500_add_silent_to_mentions.rb
new file mode 100644 (file)
index 0000000..dbb4fba
--- /dev/null
@@ -0,0 +1,23 @@
+require Rails.root.join('lib', 'mastodon', 'migration_helpers')
+
+class AddSilentToMentions < ActiveRecord::Migration[5.2]
+  include Mastodon::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  def up
+    safety_assured do
+      add_column_with_default(
+        :mentions,
+        :silent,
+        :boolean,
+        allow_null: false,
+        default: false
+      )
+    end
+  end
+
+  def down
+    remove_column :mentions, :silent
+  end
+end
index bf6ab4e68cff1d136113de1093af3e008b30a4db..f79f26f16e2cbc45c66b0144a605575fe5a9e6cf 100644 (file)
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2018_10_07_025445) do
+ActiveRecord::Schema.define(version: 2018_10_10_141500) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -301,6 +301,7 @@ ActiveRecord::Schema.define(version: 2018_10_07_025445) do
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
     t.bigint "account_id"
+    t.boolean "silent", default: false, null: false
     t.index ["account_id", "status_id"], name: "index_mentions_on_account_id_and_status_id", unique: true
     t.index ["status_id"], name: "index_mentions_on_status_id"
   end
index 62b9db8c24d251526902f1a9d7c948e52218c1f0..cd20b7c7cb006982ee1247dd099938c436a3c5eb 100644 (file)
@@ -105,6 +105,31 @@ RSpec.describe ActivityPub::Activity::Create do
       end
     end
 
+    context 'limited' do
+      let(:recipient) { Fabricate(:account) }
+
+      let(:object_json) do
+        {
+          id: [ActivityPub::TagManager.instance.uri_for(sender), '#bar'].join,
+          type: 'Note',
+          content: 'Lorem ipsum',
+          to: ActivityPub::TagManager.instance.uri_for(recipient),
+        }
+      end
+
+      it 'creates status' do
+        status = sender.statuses.first
+
+        expect(status).to_not be_nil
+        expect(status.visibility).to eq 'limited'
+      end
+
+      it 'creates silent mention' do
+        status = sender.statuses.first
+        expect(status.mentions.first).to be_silent
+      end
+    end
+
     context 'direct' do
       let(:recipient) { Fabricate(:account) }
 
@@ -114,6 +139,10 @@ RSpec.describe ActivityPub::Activity::Create do
           type: 'Note',
           content: 'Lorem ipsum',
           to: ActivityPub::TagManager.instance.uri_for(recipient),
+          tag: {
+            type: 'Mention',
+            href: ActivityPub::TagManager.instance.uri_for(recipient),
+          },
         }
       end