Improve support for aspects/circles (#8950)

* 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
This commit is contained in:
Eugen Rochko 2018-10-17 17:13:04 +02:00 committed by GitHub
parent 00387be289
commit ddd30f331c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
23 changed files with 142 additions and 35 deletions

View File

@ -96,7 +96,7 @@ class ActivityPub::Activity
end end
def notify_about_mentions(status) 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) next unless mention.account.local? && audience_includes?(mention.account)
NotifyService.new.call(mention.account, mention) NotifyService.new.call(mention.account, mention)
end end

View File

@ -28,6 +28,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity
process_status_params process_status_params
process_tags process_tags
process_audience
ApplicationRecord.transaction do ApplicationRecord.transaction do
@status = Status.create!(@params) @status = Status.create!(@params)
@ -66,6 +67,27 @@ class ActivityPub::Activity::Create < ActivityPub::Activity
end end
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) def attach_tags(status)
@tags.each do |tag| @tags.each do |tag|
status.tags << tag status.tags << tag
@ -113,7 +135,7 @@ class ActivityPub::Activity::Create < ActivityPub::Activity
return if account.nil? return if account.nil?
@mentions << Mention.new(account: account) @mentions << Mention.new(account: account, silent: false)
end end
def process_emoji(tag) def process_emoji(tag)

View File

@ -58,8 +58,8 @@ class ActivityPub::TagManager
[COLLECTIONS[:public]] [COLLECTIONS[:public]]
when 'unlisted', 'private' when 'unlisted', 'private'
[account_followers_url(status.account)] [account_followers_url(status.account)]
when 'direct' when 'direct', 'limited'
status.mentions.map { |mention| uri_for(mention.account) } status.active_mentions.map { |mention| uri_for(mention.account) }
end end
end end
@ -80,7 +80,7 @@ class ActivityPub::TagManager
cc << COLLECTIONS[:public] cc << COLLECTIONS[:public]
end 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 cc
end end

View File

@ -88,7 +88,7 @@ class FeedManager
end end
query.each do |status| 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) add_to_feed(:home, into_account.id, status)
end 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 status.reply? && (status.in_reply_to_id.nil? || status.in_reply_to_account_id.nil?)
return true if phrase_filtered?(status, receiver_id, :home) 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]) check_for_blocks.concat([status.account_id])
if status.reblog? if status.reblog?
check_for_blocks.concat([status.reblog.account_id]) 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 end
return true if blocks_or_mutes?(receiver_id, check_for_blocks, :home) 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 # This filter is called from NotifyService, but already after the sender of
# the notification has been checked for mute/block. Therefore, it's not # the notification has been checked for mute/block. Therefore, it's not
# necessary to check the author of the toot for mute/block again # 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? 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) 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)

View File

@ -27,7 +27,7 @@ class Formatter
return html.html_safe # rubocop:disable Rails/OutputSafety return html.html_safe # rubocop:disable Rails/OutputSafety
end end
linkable_accounts = status.mentions.map(&:account) linkable_accounts = status.active_mentions.map(&:account)
linkable_accounts << status.account linkable_accounts << status.account
html = raw_content html = raw_content

View 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, '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) 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)) append_element(entry, 'link', nil, rel: :mentioned, 'ostatus:object-type': OStatus::TagManager::TYPES[:person], href: OStatus::TagManager.instance.uri_for(mentioned.account))
end end

View File

@ -85,7 +85,7 @@ class AccountConversation < ApplicationRecord
private private
def participants_from_status(recipient, status) 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
end end

View File

@ -8,6 +8,7 @@
# created_at :datetime not null # created_at :datetime not null
# updated_at :datetime not null # updated_at :datetime not null
# account_id :bigint(8) # account_id :bigint(8)
# silent :boolean default(FALSE), not null
# #
class Mention < ApplicationRecord class Mention < ApplicationRecord
@ -18,10 +19,17 @@ class Mention < ApplicationRecord
validates :account, uniqueness: { scope: :status } validates :account, uniqueness: { scope: :status }
scope :active, -> { where(silent: false) }
scope :silent, -> { where(silent: true) }
delegate( delegate(
:username, :username,
:acct, :acct,
to: :account, to: :account,
prefix: true prefix: true
) )
def active?
!silent?
end
end end

View File

@ -24,7 +24,7 @@ class Notification < ApplicationRecord
favourite: 'Favourite', favourite: 'Favourite',
}.freeze }.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 :account, optional: true
belongs_to :from_account, class_name: 'Account', optional: true belongs_to :from_account, class_name: 'Account', optional: true

View File

@ -37,7 +37,7 @@ class Status < ApplicationRecord
update_index('statuses#status', :proper) if Chewy.enabled? 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 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 :favourites, inverse_of: :status, dependent: :destroy
has_many :reblogs, foreign_key: 'reblog_of_id', class_name: 'Status', inverse_of: :reblog, 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 :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_many :media_attachments, dependent: :nullify
has_and_belongs_to_many :tags has_and_belongs_to_many :tags
@ -89,7 +90,7 @@ class Status < ApplicationRecord
:status_stat, :status_stat,
:tags, :tags,
:stream_entry, :stream_entry,
mentions: :account, active_mentions: :account,
reblog: [ reblog: [
:account, :account,
:application, :application,
@ -98,7 +99,7 @@ class Status < ApplicationRecord
:media_attachments, :media_attachments,
:conversation, :conversation,
:status_stat, :status_stat,
mentions: :account, active_mentions: :account,
], ],
thread: :account thread: :account
@ -171,7 +172,11 @@ class Status < ApplicationRecord
end end
def hidden? def hidden?
private_visibility? || direct_visibility? private_visibility? || direct_visibility? || limited_visibility?
end
def distributable?
public_visibility? || unlisted_visibility?
end end
def with_media? def with_media?

View File

@ -48,7 +48,7 @@ class StreamEntry < ApplicationRecord
end end
def mentions def mentions
orphaned? ? [] : status.mentions.map(&:account) orphaned? ? [] : status.active_mentions.map(&:account)
end end
private private

View File

@ -12,7 +12,7 @@ class StatusPolicy < ApplicationPolicy
end end
def show? def show?
if direct? if requires_mention?
owned? || mention_exists? owned? || mention_exists?
elsif private? elsif private?
owned? || following_author? || mention_exists? owned? || following_author? || mention_exists?
@ -22,7 +22,7 @@ class StatusPolicy < ApplicationPolicy
end end
def reblog? def reblog?
!direct? && (!private? || owned?) && show? && !blocking_author? !requires_mention? && (!private? || owned?) && show? && !blocking_author?
end end
def favourite? def favourite?
@ -41,8 +41,8 @@ class StatusPolicy < ApplicationPolicy
private private
def direct? def requires_mention?
record.direct_visibility? record.direct_visibility? || record.limited_visibility?
end end
def owned? def owned?

View File

@ -68,7 +68,7 @@ class ActivityPub::NoteSerializer < ActiveModel::Serializer
end end
def virtual_tags 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 end
def atom_uri def atom_uri

View File

@ -36,6 +36,17 @@ class REST::StatusSerializer < ActiveModel::Serializer
!current_user.nil? !current_user.nil?
end 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 def uri
OStatus::TagManager.instance.uri_for(object) OStatus::TagManager.instance.uri_for(object)
end end
@ -88,7 +99,7 @@ class REST::StatusSerializer < ActiveModel::Serializer
end end
def ordered_mentions def ordered_mentions
object.mentions.to_a.sort_by(&:id) object.active_mentions.to_a.sort_by(&:id)
end end
class ApplicationSerializer < ActiveModel::Serializer class ApplicationSerializer < ActiveModel::Serializer

View File

@ -12,7 +12,7 @@ class BatchedRemoveStatusService < BaseService
def call(statuses) 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 } 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 @tags = statuses.map { |s| [s.id, s.tags.pluck(:name)] }.to_h
@stream_entry_batches = [] @stream_entry_batches = []

View File

@ -10,6 +10,8 @@ class FanOutOnWriteService < BaseService
if status.direct_visibility? if status.direct_visibility?
deliver_to_own_conversation(status) deliver_to_own_conversation(status)
elsif status.limited_visibility?
deliver_to_mentioned_followers(status)
else else
deliver_to_self(status) if status.account.local? deliver_to_self(status) if status.account.local?
deliver_to_followers(status) deliver_to_followers(status)
@ -53,6 +55,16 @@ class FanOutOnWriteService < BaseService
end end
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) def render_anonymous_payload(status)
@payload = InlineRenderer.render(status, nil, :status) @payload = InlineRenderer.render(status, nil, :status)
@payload = Oj.dump(event: :update, payload: @payload) @payload = Oj.dump(event: :update, payload: @payload)

View File

@ -8,7 +8,7 @@ class RemoveStatusService < BaseService
@status = status @status = status
@account = status.account @account = status.account
@tags = status.tags.pluck(:name).to_a @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 @reblogs = status.reblogs.to_a
@stream_entry = status.stream_entry @stream_entry = status.stream_entry
@options = options @options = options

View File

@ -51,7 +51,7 @@
- if status.direct_visibility? - if status.direct_visibility?
%span.detailed-status__link< %span.detailed-status__link<
= fa_icon('envelope') = fa_icon('envelope')
- elsif status.private_visibility? - elsif status.private_visibility? || status.limited_visibility?
%span.detailed-status__link< %span.detailed-status__link<
= fa_icon('lock') = fa_icon('lock')
- else - else

View File

@ -23,7 +23,7 @@ class ActivityPub::DistributionWorker
private private
def skip_distribution? def skip_distribution?
@status.direct_visibility? @status.direct_visibility? || @status.limited_visibility?
end end
def relayable? def relayable?

View File

@ -9,7 +9,7 @@ class ActivityPub::ReplyDistributionWorker
@status = Status.find(status_id) @status = Status.find(status_id)
@account = @status.thread&.account @account = @status.thread&.account
return if @account.nil? || skip_distribution? return unless @account.present? && @status.distributable?
ActivityPub::DeliveryWorker.push_bulk(inboxes) do |inbox_url| ActivityPub::DeliveryWorker.push_bulk(inboxes) do |inbox_url|
[signed_payload, @status.account_id, inbox_url] [signed_payload, @status.account_id, inbox_url]
@ -20,10 +20,6 @@ class ActivityPub::ReplyDistributionWorker
private private
def skip_distribution?
@status.private_visibility? || @status.direct_visibility?
end
def inboxes def inboxes
@inboxes ||= @account.followers.inboxes @inboxes ||= @account.followers.inboxes
end end

View File

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

View File

@ -10,7 +10,7 @@
# #
# It's strongly recommended that you check this file into your version control system. # 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 # These are extensions that must be enabled in order to support this database
enable_extension "plpgsql" enable_extension "plpgsql"
@ -301,6 +301,7 @@ ActiveRecord::Schema.define(version: 2018_10_07_025445) do
t.datetime "created_at", null: false t.datetime "created_at", null: false
t.datetime "updated_at", null: false t.datetime "updated_at", null: false
t.bigint "account_id" 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 ["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" t.index ["status_id"], name: "index_mentions_on_status_id"
end end

View File

@ -105,6 +105,31 @@ RSpec.describe ActivityPub::Activity::Create do
end end
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 context 'direct' do
let(:recipient) { Fabricate(:account) } let(:recipient) { Fabricate(:account) }
@ -114,6 +139,10 @@ RSpec.describe ActivityPub::Activity::Create do
type: 'Note', type: 'Note',
content: 'Lorem ipsum', content: 'Lorem ipsum',
to: ActivityPub::TagManager.instance.uri_for(recipient), to: ActivityPub::TagManager.instance.uri_for(recipient),
tag: {
type: 'Mention',
href: ActivityPub::TagManager.instance.uri_for(recipient),
},
} }
end end