Fix being able to import more than allowed number of follows (mastodo…

…n#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 <[email protected]>
hachyderm · Dec 26, 2020 · f1f96eb · f1f96eb
1 parent 4580129
commit f1f96eb
Show file tree

Hide file tree

Showing 11 changed files with 38 additions and 16 deletions.
diff --git a/app/models/concerns/account_interactions.rb b/app/models/concerns/account_interactions.rb
@@ -97,8 +97,8 @@ def follow_mapping(query, field)
     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 @@ def follow!(other_account, reblogs: nil, notify: nil, uri: nil, rate_limit: fals
     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
@@ -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
@@ -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
@@ -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
@@ -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 @@ def change_follow_request_options!
   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 @@ def request_follow!
   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
@@ -7,7 +7,7 @@ def perform(source_account_id, target_account_id)
     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
@@ -15,7 +15,11 @@ def perform(account_id, target_account_uri, relationship, options = {})
 
     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
@@ -19,7 +19,7 @@ def perform(target_account_id)
 
       # 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
@@ -14,7 +14,7 @@ def perform(follower_account_id, old_target_account_id, new_target_account_id, b
     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
@@ -384,7 +384,7 @@ def follow(username)
       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
@@ -23,8 +23,8 @@
       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