Fix being able to import more than allowed number of follows (#15384)

* Fix being able to import more than allowed number of follows

Without this commit, if someone tries importing a second list of accounts to
follow before the first one has been processed, this will queue imports for
the two whole lists, even if they exceed the account's allowed number of
outgoing follows.

This commit changes it so the individual queued imports aren't exempt from
the follow limit check (they remain exempt from the rate-limiting check
though).

* Catch validation errors to not re-queue failed follows

Co-authored-by: Claire <claire.github-309c@sitedethib.com>
This commit is contained in:
ThibG 2020-12-26 23:52:46 +01:00 committed by GitHub
parent 4580129c98
commit f1f96ebf02
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
11 changed files with 38 additions and 16 deletions

View file

@ -97,8 +97,8 @@ module AccountInteractions
has_many :announcement_mutes, dependent: :destroy has_many :announcement_mutes, dependent: :destroy
end end
def follow!(other_account, reblogs: nil, notify: nil, uri: nil, rate_limit: false) def follow!(other_account, reblogs: nil, notify: nil, uri: nil, rate_limit: false, bypass_limit: false)
rel = active_relationships.create_with(show_reblogs: reblogs.nil? ? true : reblogs, notify: notify.nil? ? false : notify, uri: uri, rate_limit: rate_limit) rel = active_relationships.create_with(show_reblogs: reblogs.nil? ? true : reblogs, notify: notify.nil? ? false : notify, uri: uri, rate_limit: rate_limit, bypass_follow_limit: bypass_limit)
.find_or_create_by!(target_account: other_account) .find_or_create_by!(target_account: other_account)
rel.show_reblogs = reblogs unless reblogs.nil? rel.show_reblogs = reblogs unless reblogs.nil?
@ -111,8 +111,8 @@ module AccountInteractions
rel rel
end end
def request_follow!(other_account, reblogs: nil, notify: nil, uri: nil, rate_limit: false) def request_follow!(other_account, reblogs: nil, notify: nil, uri: nil, rate_limit: false, bypass_limit: false)
rel = follow_requests.create_with(show_reblogs: reblogs.nil? ? true : reblogs, notify: notify.nil? ? false : notify, uri: uri, rate_limit: rate_limit) rel = follow_requests.create_with(show_reblogs: reblogs.nil? ? true : reblogs, notify: notify.nil? ? false : notify, uri: uri, rate_limit: rate_limit, bypass_follow_limit: bypass_limit)
.find_or_create_by!(target_account: other_account) .find_or_create_by!(target_account: other_account)
rel.show_reblogs = reblogs unless reblogs.nil? rel.show_reblogs = reblogs unless reblogs.nil?

View file

@ -0,0 +1,17 @@
# frozen_string_literal: true
module FollowLimitable
extend ActiveSupport::Concern
included do
validates_with FollowLimitValidator, on: :create, unless: :bypass_follow_limit?
end
def bypass_follow_limit=(value)
@bypass_follow_limit = value
end
def bypass_follow_limit?
@bypass_follow_limit
end
end

View file

@ -17,6 +17,7 @@ class Follow < ApplicationRecord
include Paginable include Paginable
include RelationshipCacheable include RelationshipCacheable
include RateLimitable include RateLimitable
include FollowLimitable
rate_limit by: :account, family: :follows rate_limit by: :account, family: :follows
@ -26,7 +27,6 @@ class Follow < ApplicationRecord
has_one :notification, as: :activity, dependent: :destroy has_one :notification, as: :activity, dependent: :destroy
validates :account_id, uniqueness: { scope: :target_account_id } validates :account_id, uniqueness: { scope: :target_account_id }
validates_with FollowLimitValidator, on: :create, if: :rate_limit?
scope :recent, -> { reorder(id: :desc) } scope :recent, -> { reorder(id: :desc) }

View file

@ -17,6 +17,7 @@ class FollowRequest < ApplicationRecord
include Paginable include Paginable
include RelationshipCacheable include RelationshipCacheable
include RateLimitable include RateLimitable
include FollowLimitable
rate_limit by: :account, family: :follows rate_limit by: :account, family: :follows
@ -26,7 +27,6 @@ class FollowRequest < ApplicationRecord
has_one :notification, as: :activity, dependent: :destroy has_one :notification, as: :activity, dependent: :destroy
validates :account_id, uniqueness: { scope: :target_account_id } validates :account_id, uniqueness: { scope: :target_account_id }
validates_with FollowLimitValidator, on: :create, if: :rate_limit?
def authorize! def authorize!
account.follow!(target_account, reblogs: show_reblogs, notify: notify, uri: uri) account.follow!(target_account, reblogs: show_reblogs, notify: notify, uri: uri)

View file

@ -11,11 +11,12 @@ class FollowService < BaseService
# @option [Boolean] :reblogs Whether or not to show reblogs, defaults to true # @option [Boolean] :reblogs Whether or not to show reblogs, defaults to true
# @option [Boolean] :notify Whether to create notifications about new posts, defaults to false # @option [Boolean] :notify Whether to create notifications about new posts, defaults to false
# @option [Boolean] :bypass_locked # @option [Boolean] :bypass_locked
# @option [Boolean] :bypass_limit Allow following past the total follow number
# @option [Boolean] :with_rate_limit # @option [Boolean] :with_rate_limit
def call(source_account, target_account, options = {}) def call(source_account, target_account, options = {})
@source_account = source_account @source_account = source_account
@target_account = ResolveAccountService.new.call(target_account, skip_webfinger: true) @target_account = ResolveAccountService.new.call(target_account, skip_webfinger: true)
@options = { bypass_locked: false, with_rate_limit: false }.merge(options) @options = { bypass_locked: false, bypass_limit: false, with_rate_limit: false }.merge(options)
raise ActiveRecord::RecordNotFound if following_not_possible? raise ActiveRecord::RecordNotFound if following_not_possible?
raise Mastodon::NotPermittedError if following_not_allowed? raise Mastodon::NotPermittedError if following_not_allowed?
@ -54,7 +55,7 @@ class FollowService < BaseService
end end
def request_follow! def request_follow!
follow_request = @source_account.request_follow!(@target_account, reblogs: @options[:reblogs], notify: @options[:notify], rate_limit: @options[:with_rate_limit]) follow_request = @source_account.request_follow!(@target_account, reblogs: @options[:reblogs], notify: @options[:notify], rate_limit: @options[:with_rate_limit], bypass_limit: @options[:bypass_limit])
if @target_account.local? if @target_account.local?
LocalNotificationWorker.perform_async(@target_account.id, follow_request.id, follow_request.class.name, :follow_request) LocalNotificationWorker.perform_async(@target_account.id, follow_request.id, follow_request.class.name, :follow_request)
@ -66,7 +67,7 @@ class FollowService < BaseService
end end
def direct_follow! def direct_follow!
follow = @source_account.follow!(@target_account, reblogs: @options[:reblogs], notify: @options[:notify], rate_limit: @options[:with_rate_limit]) follow = @source_account.follow!(@target_account, reblogs: @options[:reblogs], notify: @options[:notify], rate_limit: @options[:with_rate_limit], bypass_limit: @options[:bypass_limit])
LocalNotificationWorker.perform_async(@target_account.id, follow.id, follow.class.name, :follow) LocalNotificationWorker.perform_async(@target_account.id, follow.id, follow.class.name, :follow)
MergeWorker.perform_async(@target_account.id, @source_account.id) MergeWorker.perform_async(@target_account.id, @source_account.id)

View file

@ -7,7 +7,7 @@ class AuthorizeFollowWorker
source_account = Account.find(source_account_id) source_account = Account.find(source_account_id)
target_account = Account.find(target_account_id) target_account = Account.find(target_account_id)
AuthorizeFollowService.new.call(source_account, target_account) AuthorizeFollowService.new.call(source_account, target_account, bypass_limit: true)
rescue ActiveRecord::RecordNotFound rescue ActiveRecord::RecordNotFound
true true
end end

View file

@ -15,7 +15,11 @@ class Import::RelationshipWorker
case relationship case relationship
when 'follow' when 'follow'
begin
FollowService.new.call(from_account, target_account, options) FollowService.new.call(from_account, target_account, options)
rescue ActiveRecord::RecordInvalid
raise if FollowLimitValidator.limit_for_account(from_account) < from_account.following_count
end
when 'unfollow' when 'unfollow'
UnfollowService.new.call(from_account, target_account) UnfollowService.new.call(from_account, target_account)
when 'block' when 'block'

View file

@ -19,7 +19,7 @@ class RefollowWorker
# Schedule re-follow # Schedule re-follow
begin begin
FollowService.new.call(follower, target_account, reblogs: reblogs, notify: notify) FollowService.new.call(follower, target_account, reblogs: reblogs, notify: notify, bypass_limit: true)
rescue Mastodon::NotPermittedError, ActiveRecord::RecordNotFound, Mastodon::UnexpectedResponseError, HTTP::Error, OpenSSL::SSL::SSLError rescue Mastodon::NotPermittedError, ActiveRecord::RecordNotFound, Mastodon::UnexpectedResponseError, HTTP::Error, OpenSSL::SSL::SSLError
next next
end end

View file

@ -14,7 +14,7 @@ class UnfollowFollowWorker
reblogs = follow&.show_reblogs? reblogs = follow&.show_reblogs?
notify = follow&.notify? notify = follow&.notify?
FollowService.new.call(follower_account, new_target_account, reblogs: reblogs, notify: notify, bypass_locked: bypass_locked) FollowService.new.call(follower_account, new_target_account, reblogs: reblogs, notify: notify, bypass_locked: bypass_locked, bypass_limit: true)
UnfollowService.new.call(follower_account, old_target_account, skip_unmerge: true) UnfollowService.new.call(follower_account, old_target_account, skip_unmerge: true)
rescue ActiveRecord::RecordNotFound, Mastodon::NotPermittedError rescue ActiveRecord::RecordNotFound, Mastodon::NotPermittedError
true true

View file

@ -384,7 +384,7 @@ module Mastodon
end end
processed, = parallelize_with_progress(Account.local.without_suspended) do |account| processed, = parallelize_with_progress(Account.local.without_suspended) do |account|
FollowService.new.call(account, target_account) FollowService.new.call(account, target_account, bypass_limit: true)
end end
say("OK, followed target from #{processed} accounts", :green) say("OK, followed target from #{processed} accounts", :green)

View file

@ -23,8 +23,8 @@ describe RefollowWorker do
result = subject.perform(account.id) result = subject.perform(account.id)
expect(result).to be_nil expect(result).to be_nil
expect(service).to have_received(:call).with(alice, account, reblogs: true, notify: false) expect(service).to have_received(:call).with(alice, account, reblogs: true, notify: false, bypass_limit: true)
expect(service).to have_received(:call).with(bob, account, reblogs: false, notify: false) expect(service).to have_received(:call).with(bob, account, reblogs: false, notify: false, bypass_limit: true)
end end
end end
end end