From 216b85b053d091306e3311a21f5b050f70a75130 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Mon, 14 Dec 2020 09:06:34 +0100 Subject: Fix performance on instances list in admin UI (#15282) - Reduce duplicate queries - Remove n+1 queries - Add accounts count to detailed view - Add separate action log entry for updating existing domain blocks --- spec/models/account_spec.rb | 21 --------------------- 1 file changed, 21 deletions(-) (limited to 'spec/models') diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index 75f628076..1d000ed4d 100644 --- a/spec/models/account_spec.rb +++ b/spec/models/account_spec.rb @@ -440,13 +440,6 @@ RSpec.describe Account, type: :model do end end - describe '.domains' do - it 'returns domains' do - Fabricate(:account, domain: 'domain') - expect(Account.remote.domains).to match_array(['domain']) - end - end - describe '#statuses_count' do subject { Fabricate(:account) } @@ -737,20 +730,6 @@ RSpec.describe Account, type: :model do end end - describe 'by_domain_accounts' do - it 'returns accounts grouped by domain sorted by accounts' do - 2.times { Fabricate(:account, domain: 'example.com') } - Fabricate(:account, domain: 'example2.com') - - results = Account.where('id > 0').by_domain_accounts - expect(results.length).to eq 2 - expect(results.first.domain).to eq 'example.com' - expect(results.first.accounts_count).to eq 2 - expect(results.last.domain).to eq 'example2.com' - expect(results.last.accounts_count).to eq 1 - end - end - describe 'local' do it 'returns an array of accounts who do not have a domain' do account_1 = Fabricate(:account, domain: nil) -- cgit From 83579695593c941ab39ab545c10a7f711bf715fc Mon Sep 17 00:00:00 2001 From: ThibG Date: Tue, 15 Dec 2020 17:23:58 +0100 Subject: Fix admins being able to suspend their instance actor (#14567) * Fix admin being able to suspend their own instance account * Add text about the instance's own actor in admin view * Change instance actor notice from flash message to template * Do not list local instance actor in account moderation list --- app/models/account.rb | 3 ++- app/models/account_filter.rb | 2 +- app/policies/account_policy.rb | 4 ++-- app/views/admin/accounts/show.html.haml | 4 ++++ config/locales/en.yml | 1 + spec/models/account_filter_spec.rb | 2 +- 6 files changed, 11 insertions(+), 5 deletions(-) (limited to 'spec/models') diff --git a/app/models/account.rb b/app/models/account.rb index e21b353e9..80eb92a71 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -100,6 +100,7 @@ class Account < ApplicationRecord scope :sensitized, -> { where.not(sensitized_at: nil) } scope :without_suspended, -> { where(suspended_at: nil) } scope :without_silenced, -> { where(silenced_at: nil) } + scope :without_instance_actor, -> { where.not(id: -99) } scope :recent, -> { reorder(id: :desc) } scope :bots, -> { where(actor_type: %w(Application Service)) } scope :groups, -> { where(actor_type: 'Group') } @@ -222,7 +223,7 @@ class Account < ApplicationRecord end def suspended? - suspended_at.present? + suspended_at.present? && !instance_actor? end def suspended_permanently? diff --git a/app/models/account_filter.rb b/app/models/account_filter.rb index 7b6012e0f..2b001385f 100644 --- a/app/models/account_filter.rb +++ b/app/models/account_filter.rb @@ -45,7 +45,7 @@ class AccountFilter def scope_for(key, value) case key.to_s when 'local' - Account.local + Account.local.without_instance_actor when 'remote' Account.remote when 'by_domain' diff --git a/app/policies/account_policy.rb b/app/policies/account_policy.rb index 262ada42e..672e1786b 100644 --- a/app/policies/account_policy.rb +++ b/app/policies/account_policy.rb @@ -14,7 +14,7 @@ class AccountPolicy < ApplicationPolicy end def suspend? - staff? && !record.user&.staff? + staff? && !record.user&.staff? && !record.instance_actor? end def destroy? @@ -62,6 +62,6 @@ class AccountPolicy < ApplicationPolicy end def memorialize? - admin? && !record.user&.admin? + admin? && !record.user&.admin? && !record.instance_actor? end end diff --git a/app/views/admin/accounts/show.html.haml b/app/views/admin/accounts/show.html.haml index ae527cc23..27e1f80a7 100644 --- a/app/views/admin/accounts/show.html.haml +++ b/app/views/admin/accounts/show.html.haml @@ -1,6 +1,10 @@ - content_for :page_title do = @account.acct +- if @account.instance_actor? + .flash-message.notice + %strong= t('accounts.instance_actor_flash') + = render 'application/card', account: @account - account = @account diff --git a/config/locales/en.yml b/config/locales/en.yml index 1f5798fcc..83f75b28f 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -60,6 +60,7 @@ en: one: Follower other: Followers following: Following + instance_actor_flash: This account is a virtual actor used to represent the server itself and not any individual user. It is used for federation purposes and should not be suspended. joined: Joined %{date} last_active: last active link_verified_on: Ownership of this link was checked on %{date} diff --git a/spec/models/account_filter_spec.rb b/spec/models/account_filter_spec.rb index 176a0eeac..0cdb373f6 100644 --- a/spec/models/account_filter_spec.rb +++ b/spec/models/account_filter_spec.rb @@ -5,7 +5,7 @@ describe AccountFilter do it 'defaults to recent local not-suspended account list' do filter = described_class.new({}) - expect(filter.results).to eq Account.local.recent.without_suspended + expect(filter.results).to eq Account.local.without_instance_actor.recent.without_suspended end end -- cgit From eb35be0431b2cdd2bbf3339beb9c5a0839e1088b Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Fri, 18 Dec 2020 09:18:31 +0100 Subject: Fix follow limit preventing re-following of a moved account (#14207) --- app/models/follow.rb | 2 +- app/models/follow_request.rb | 2 +- app/models/import.rb | 1 + app/services/import_service.rb | 7 ++---- app/validators/import_validator.rb | 44 ++++++++++++++++++++++++++++++++++++++ config/locales/en.yml | 2 ++ spec/models/follow_spec.rb | 2 +- spec/models/import_spec.rb | 10 +++++++++ 8 files changed, 62 insertions(+), 8 deletions(-) create mode 100644 app/validators/import_validator.rb (limited to 'spec/models') diff --git a/app/models/follow.rb b/app/models/follow.rb index 55a9da792..69a1722b3 100644 --- a/app/models/follow.rb +++ b/app/models/follow.rb @@ -26,7 +26,7 @@ class Follow < ApplicationRecord has_one :notification, as: :activity, dependent: :destroy validates :account_id, uniqueness: { scope: :target_account_id } - validates_with FollowLimitValidator, on: :create + 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 c1f19149b..2d2a77b59 100644 --- a/app/models/follow_request.rb +++ b/app/models/follow_request.rb @@ -26,7 +26,7 @@ class FollowRequest < ApplicationRecord has_one :notification, as: :activity, dependent: :destroy validates :account_id, uniqueness: { scope: :target_account_id } - validates_with FollowLimitValidator, on: :create + 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/models/import.rb b/app/models/import.rb index 702453289..00a54892e 100644 --- a/app/models/import.rb +++ b/app/models/import.rb @@ -27,6 +27,7 @@ class Import < ApplicationRecord enum type: [:following, :blocking, :muting, :domain_blocking, :bookmarks] validates :type, presence: true + validates_with ImportValidator, on: :create has_attached_file :data validates_attachment_content_type :data, content_type: FILE_TYPES diff --git a/app/services/import_service.rb b/app/services/import_service.rb index 288e47f1e..0c6ef2238 100644 --- a/app/services/import_service.rb +++ b/app/services/import_service.rb @@ -27,7 +27,7 @@ class ImportService < BaseService def import_follows! parse_import_data!(['Account address']) - import_relationships!('follow', 'unfollow', @account.following, follow_limit, reblogs: { header: 'Show boosts', default: true }) + import_relationships!('follow', 'unfollow', @account.following, ROWS_PROCESSING_LIMIT, reblogs: { header: 'Show boosts', default: true }) end def import_blocks! @@ -85,6 +85,7 @@ class ImportService < BaseService head_items = items.uniq { |acct, _| acct.split('@')[1] } tail_items = items - head_items + Import::RelationshipWorker.push_bulk(head_items + tail_items) do |acct, extra| [@account.id, acct, action, extra] end @@ -133,10 +134,6 @@ class ImportService < BaseService Paperclip.io_adapters.for(@import.data).read end - def follow_limit - FollowLimitValidator.limit_for_account(@account) - end - def relations_map_for_account(account, account_ids) { blocking: {}, diff --git a/app/validators/import_validator.rb b/app/validators/import_validator.rb new file mode 100644 index 000000000..a182abfa5 --- /dev/null +++ b/app/validators/import_validator.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +class ImportValidator < ActiveModel::Validator + KNOWN_HEADERS = [ + 'Account address', + '#domain', + '#uri', + ].freeze + + def validate(import) + return if import.type.blank? || import.data.blank? + + # We parse because newlines could be part of individual rows. This + # runs on create so we should be reading the local file here before + # it is uploaded to object storage or moved anywhere... + csv_data = CSV.parse(import.data.queued_for_write[:original].read) + + row_count = csv_data.size + row_count -= 1 if KNOWN_HEADERS.include?(csv_data.first&.first) + + import.errors.add(:data, I18n.t('imports.errors.over_rows_processing_limit', count: ImportService::ROWS_PROCESSING_LIMIT)) if row_count > ImportService::ROWS_PROCESSING_LIMIT + + case import.type + when 'following' + validate_following_import(import, row_count) + end + end + + private + + def validate_following_import(import, row_count) + base_limit = FollowLimitValidator.limit_for_account(import.account) + + limit = begin + if import.overwrite? + base_limit + else + base_limit - import.account.following_count + end + end + + import.errors.add(:data, I18n.t('users.follow_limit_reached', limit: base_limit)) if row_count > limit + end +end diff --git a/config/locales/en.yml b/config/locales/en.yml index e91f4344f..18a207f5e 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -923,6 +923,8 @@ en: status: Verification status view_proof: View proof imports: + errors: + over_rows_processing_limit: contains more than %{count} rows modes: merge: Merge merge_long: Keep existing records and add new ones diff --git a/spec/models/follow_spec.rb b/spec/models/follow_spec.rb index 0c84e5e7b..e723a1ef2 100644 --- a/spec/models/follow_spec.rb +++ b/spec/models/follow_spec.rb @@ -5,7 +5,7 @@ RSpec.describe Follow, type: :model do let(:bob) { Fabricate(:account, username: 'bob') } describe 'validations' do - subject { Follow.new(account: alice, target_account: bob) } + subject { Follow.new(account: alice, target_account: bob, rate_limit: true) } it 'has a valid fabricator' do follow = Fabricate.build(:follow) diff --git a/spec/models/import_spec.rb b/spec/models/import_spec.rb index 321761166..a5eec1722 100644 --- a/spec/models/import_spec.rb +++ b/spec/models/import_spec.rb @@ -20,5 +20,15 @@ RSpec.describe Import, type: :model do import = Import.create(account: account, type: type) expect(import).to model_have_error_on_field(:data) end + + it 'is invalid with too many rows in data' do + import = Import.create(account: account, type: type, data: StringIO.new("foo@bar.com\n" * (ImportService::ROWS_PROCESSING_LIMIT + 10))) + expect(import).to model_have_error_on_field(:data) + end + + it 'is invalid when there are more rows when following limit' do + import = Import.create(account: account, type: type, data: StringIO.new("foo@bar.com\n" * (FollowLimitValidator.limit_for_account(account) + 10))) + expect(import).to model_have_error_on_field(:data) + end end end -- cgit