From f1f96ebf02e96d21d84c52825cbac623b66488f8 Mon Sep 17 00:00:00 2001 From: ThibG Date: Sat, 26 Dec 2020 23:52:46 +0100 Subject: [PATCH] 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 --- app/models/concerns/account_interactions.rb | 8 ++++---- app/models/concerns/follow_limitable.rb | 17 +++++++++++++++++ app/models/follow.rb | 2 +- app/models/follow_request.rb | 2 +- app/services/follow_service.rb | 7 ++++--- app/workers/authorize_follow_worker.rb | 2 +- app/workers/import/relationship_worker.rb | 6 +++++- app/workers/refollow_worker.rb | 2 +- app/workers/unfollow_follow_worker.rb | 2 +- lib/mastodon/accounts_cli.rb | 2 +- spec/workers/refollow_worker_spec.rb | 4 ++-- 11 files changed, 38 insertions(+), 16 deletions(-) create mode 100644 app/models/concerns/follow_limitable.rb diff --git a/app/models/concerns/account_interactions.rb b/app/models/concerns/account_interactions.rb index e2c4b8acf59..974f57820d0 100644 --- a/app/models/concerns/account_interactions.rb +++ b/app/models/concerns/account_interactions.rb @@ -97,8 +97,8 @@ module AccountInteractions has_many :announcement_mutes, dependent: :destroy end - def follow!(other_account, reblogs: nil, notify: nil, uri: nil, rate_limit: false) - rel = active_relationships.create_with(show_reblogs: reblogs.nil? ? true : reblogs, notify: notify.nil? ? false : notify, uri: uri, rate_limit: rate_limit) + 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, bypass_follow_limit: bypass_limit) .find_or_create_by!(target_account: other_account) rel.show_reblogs = reblogs unless reblogs.nil? @@ -111,8 +111,8 @@ module AccountInteractions rel end - def request_follow!(other_account, reblogs: nil, notify: nil, uri: nil, rate_limit: false) - rel = follow_requests.create_with(show_reblogs: reblogs.nil? ? true : reblogs, notify: notify.nil? ? false : notify, uri: uri, rate_limit: rate_limit) + 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, bypass_follow_limit: bypass_limit) .find_or_create_by!(target_account: other_account) rel.show_reblogs = reblogs unless reblogs.nil? diff --git a/app/models/concerns/follow_limitable.rb b/app/models/concerns/follow_limitable.rb new file mode 100644 index 00000000000..c64060d6e56 --- /dev/null +++ b/app/models/concerns/follow_limitable.rb @@ -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 diff --git a/app/models/follow.rb b/app/models/follow.rb index 69a1722b303..a5e3fe80995 100644 --- a/app/models/follow.rb +++ b/app/models/follow.rb @@ -17,6 +17,7 @@ class Follow < ApplicationRecord include Paginable include RelationshipCacheable include RateLimitable + include FollowLimitable rate_limit by: :account, family: :follows @@ -26,7 +27,6 @@ class Follow < ApplicationRecord has_one :notification, as: :activity, dependent: :destroy validates :account_id, uniqueness: { scope: :target_account_id } - validates_with FollowLimitValidator, on: :create, if: :rate_limit? scope :recent, -> { reorder(id: :desc) } diff --git a/app/models/follow_request.rb b/app/models/follow_request.rb index 2d2a77b59c6..59fefcdf64e 100644 --- a/app/models/follow_request.rb +++ b/app/models/follow_request.rb @@ -17,6 +17,7 @@ class FollowRequest < ApplicationRecord include Paginable include RelationshipCacheable include RateLimitable + include FollowLimitable rate_limit by: :account, family: :follows @@ -26,7 +27,6 @@ class FollowRequest < ApplicationRecord has_one :notification, as: :activity, dependent: :destroy validates :account_id, uniqueness: { scope: :target_account_id } - validates_with FollowLimitValidator, on: :create, if: :rate_limit? def authorize! account.follow!(target_account, reblogs: show_reblogs, notify: notify, uri: uri) diff --git a/app/services/follow_service.rb b/app/services/follow_service.rb index 9625728510c..b98f7011d18 100644 --- a/app/services/follow_service.rb +++ b/app/services/follow_service.rb @@ -11,11 +11,12 @@ class FollowService < BaseService # @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] :bypass_locked + # @option [Boolean] :bypass_limit Allow following past the total follow number # @option [Boolean] :with_rate_limit def call(source_account, target_account, options = {}) @source_account = source_account @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 Mastodon::NotPermittedError if following_not_allowed? @@ -54,7 +55,7 @@ class FollowService < BaseService end 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? LocalNotificationWorker.perform_async(@target_account.id, follow_request.id, follow_request.class.name, :follow_request) @@ -66,7 +67,7 @@ class FollowService < BaseService end 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) MergeWorker.perform_async(@target_account.id, @source_account.id) diff --git a/app/workers/authorize_follow_worker.rb b/app/workers/authorize_follow_worker.rb index 0d501462463..f57900fa59a 100644 --- a/app/workers/authorize_follow_worker.rb +++ b/app/workers/authorize_follow_worker.rb @@ -7,7 +7,7 @@ class AuthorizeFollowWorker source_account = Account.find(source_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 true end diff --git a/app/workers/import/relationship_worker.rb b/app/workers/import/relationship_worker.rb index 4a455f3aebd..4a7100435f7 100644 --- a/app/workers/import/relationship_worker.rb +++ b/app/workers/import/relationship_worker.rb @@ -15,7 +15,11 @@ class Import::RelationshipWorker case relationship when 'follow' - FollowService.new.call(from_account, target_account, options) + begin + 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' UnfollowService.new.call(from_account, target_account) when 'block' diff --git a/app/workers/refollow_worker.rb b/app/workers/refollow_worker.rb index 98940680d07..319b001097b 100644 --- a/app/workers/refollow_worker.rb +++ b/app/workers/refollow_worker.rb @@ -19,7 +19,7 @@ class RefollowWorker # Schedule re-follow 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 next end diff --git a/app/workers/unfollow_follow_worker.rb b/app/workers/unfollow_follow_worker.rb index 71b5a0e3fa5..0bd5ff472e8 100644 --- a/app/workers/unfollow_follow_worker.rb +++ b/app/workers/unfollow_follow_worker.rb @@ -14,7 +14,7 @@ class UnfollowFollowWorker reblogs = follow&.show_reblogs? 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) rescue ActiveRecord::RecordNotFound, Mastodon::NotPermittedError true diff --git a/lib/mastodon/accounts_cli.rb b/lib/mastodon/accounts_cli.rb index cdd1db9954e..653bfca3010 100644 --- a/lib/mastodon/accounts_cli.rb +++ b/lib/mastodon/accounts_cli.rb @@ -384,7 +384,7 @@ module Mastodon end 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 say("OK, followed target from #{processed} accounts", :green) diff --git a/spec/workers/refollow_worker_spec.rb b/spec/workers/refollow_worker_spec.rb index 6b4c042912b..df6731b6400 100644 --- a/spec/workers/refollow_worker_spec.rb +++ b/spec/workers/refollow_worker_spec.rb @@ -23,8 +23,8 @@ describe RefollowWorker do result = subject.perform(account.id) 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(bob, account, reblogs: false, 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, bypass_limit: true) end end end