From 216b85b053d091306e3311a21f5b050f70a75130 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Mon, 14 Dec 2020 09:06:34 +0100 Subject: [PATCH] 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 --- Gemfile | 1 + Gemfile.lock | 4 + .../admin/domain_blocks_controller.rb | 5 +- app/controllers/admin/instances_controller.rb | 44 +------- .../api/v1/instances/peers_controller.rb | 2 +- app/models/account.rb | 6 +- app/models/concerns/domain_materializable.rb | 13 +++ app/models/domain_allow.rb | 1 + app/models/domain_block.rb | 1 + app/models/instance.rb | 65 ++++++++--- app/models/instance_filter.rb | 31 ++++-- app/models/unavailable_domain.rb | 2 + app/policies/domain_block_policy.rb | 4 + app/presenters/instance_presenter.rb | 2 +- app/views/admin/instances/_instance.html.haml | 25 +++++ app/views/admin/instances/index.html.haml | 34 ++---- app/views/admin/instances/show.html.haml | 60 ++++++----- .../scheduler/instance_refresh_scheduler.rb | 11 ++ config/brakeman.ignore | 101 +++++++++++++++--- config/locales/en.yml | 3 + config/sidekiq.yml | 3 + db/migrate/20201206004238_create_instances.rb | 9 ++ db/schema.rb | 27 ++++- db/views/instances_v01.sql | 17 +++ lib/mastodon/domains_cli.rb | 4 +- .../admin/instances_controller_spec.rb | 6 +- spec/models/account_spec.rb | 21 ---- 27 files changed, 331 insertions(+), 171 deletions(-) create mode 100644 app/models/concerns/domain_materializable.rb create mode 100644 app/views/admin/instances/_instance.html.haml create mode 100644 app/workers/scheduler/instance_refresh_scheduler.rb create mode 100644 db/migrate/20201206004238_create_instances.rb create mode 100644 db/views/instances_v01.sql diff --git a/Gemfile b/Gemfile index 63becb7cca..3aee5d7ccf 100644 --- a/Gemfile +++ b/Gemfile @@ -82,6 +82,7 @@ gem 'mario-redis-lock', '~> 1.2', require: 'redis_lock' gem 'rqrcode', '~> 1.1' gem 'ruby-progressbar', '~> 1.10' gem 'sanitize', '~> 5.2' +gem 'scenic', '~> 1.5' gem 'sidekiq', '~> 6.1' gem 'sidekiq-scheduler', '~> 3.0' gem 'sidekiq-unique-jobs', '~> 6.0' diff --git a/Gemfile.lock b/Gemfile.lock index f7192d0846..c4c8d99041 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -561,6 +561,9 @@ GEM crass (~> 1.0.2) nokogiri (>= 1.8.0) nokogumbo (~> 2.0) + scenic (1.5.4) + activerecord (>= 4.0.0) + railties (>= 4.0.0) securecompare (1.0.0) semantic_range (2.3.0) sidekiq (6.1.2) @@ -782,6 +785,7 @@ DEPENDENCIES rubocop-rails (~> 2.8) ruby-progressbar (~> 1.10) sanitize (~> 5.2) + scenic (~> 1.5) sidekiq (~> 6.1) sidekiq-bulk (~> 0.2.0) sidekiq-scheduler (~> 3.0) diff --git a/app/controllers/admin/domain_blocks_controller.rb b/app/controllers/admin/domain_blocks_controller.rb index 74a36b79ca..6a5b41a747 100644 --- a/app/controllers/admin/domain_blocks_controller.rb +++ b/app/controllers/admin/domain_blocks_controller.rb @@ -29,6 +29,7 @@ module Admin @domain_block = existing_domain_block @domain_block.update(resource_params) end + if @domain_block.save DomainBlockWorker.perform_async(@domain_block.id) log_action :create, @domain_block @@ -40,7 +41,7 @@ module Admin end def update - authorize :domain_block, :create? + authorize :domain_block, :update? @domain_block.update(update_params) @@ -48,7 +49,7 @@ module Admin if @domain_block.save DomainBlockWorker.perform_async(@domain_block.id, severity_changed) - log_action :create, @domain_block + log_action :update, @domain_block redirect_to admin_instances_path(limited: '1'), notice: I18n.t('admin.domain_blocks.created_msg') else render :edit diff --git a/app/controllers/admin/instances_controller.rb b/app/controllers/admin/instances_controller.rb index 1790becbf2..b5918d231c 100644 --- a/app/controllers/admin/instances_controller.rb +++ b/app/controllers/admin/instances_controller.rb @@ -2,65 +2,31 @@ module Admin class InstancesController < BaseController - before_action :set_domain_block, only: :show - before_action :set_domain_allow, only: :show + before_action :set_instances, only: :index before_action :set_instance, only: :show def index authorize :instance, :index? - - @instances = ordered_instances end def show authorize :instance, :show? - - @following_count = Follow.where(account: Account.where(domain: params[:id])).count - @followers_count = Follow.where(target_account: Account.where(domain: params[:id])).count - @reports_count = Report.where(target_account: Account.where(domain: params[:id])).count - @blocks_count = Block.where(target_account: Account.where(domain: params[:id])).count - @available = DeliveryFailureTracker.available?(params[:id]) - @media_storage = MediaAttachment.where(account: Account.where(domain: params[:id])).sum(:file_file_size) - @private_comment = @domain_block&.private_comment - @public_comment = @domain_block&.public_comment end private - def set_domain_block - @domain_block = DomainBlock.rule_for(params[:id]) - end - - def set_domain_allow - @domain_allow = DomainAllow.rule_for(params[:id]) - end - def set_instance - resource = Account.by_domain_accounts.find_by(domain: params[:id]) - resource ||= @domain_block - resource ||= @domain_allow + @instance = Instance.find(params[:id]) + end - if resource - @instance = Instance.new(resource) - else - not_found - end + def set_instances + @instances = filtered_instances.page(params[:page]) end def filtered_instances InstanceFilter.new(whitelist_mode? ? { allowed: true } : filter_params).results end - def paginated_instances - filtered_instances.page(params[:page]) - end - - helper_method :paginated_instances - - def ordered_instances - paginated_instances.map { |resource| Instance.new(resource) } - end - def filter_params params.slice(*InstanceFilter::KEYS).permit(*InstanceFilter::KEYS) end diff --git a/app/controllers/api/v1/instances/peers_controller.rb b/app/controllers/api/v1/instances/peers_controller.rb index 9fa4409357..2877fec52d 100644 --- a/app/controllers/api/v1/instances/peers_controller.rb +++ b/app/controllers/api/v1/instances/peers_controller.rb @@ -8,7 +8,7 @@ class Api::V1::Instances::PeersController < Api::BaseController def index expires_in 1.day, public: true - render_with_cache(expires_in: 1.day) { Account.remote.domains } + render_with_cache(expires_in: 1.day) { Instance.where.not(domain: DomainBlock.select(:domain)).pluck(:domain) } end private diff --git a/app/models/account.rb b/app/models/account.rb index ed11a514d5..e21b353e9a 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -67,6 +67,7 @@ class Account < ApplicationRecord include Paginable include AccountCounters include DomainNormalizable + include DomainMaterializable include AccountMerging TRUST_LEVELS = { @@ -103,7 +104,6 @@ class Account < ApplicationRecord scope :bots, -> { where(actor_type: %w(Application Service)) } scope :groups, -> { where(actor_type: 'Group') } scope :alphabetic, -> { order(domain: :asc, username: :asc) } - scope :by_domain_accounts, -> { group(:domain).select(:domain, 'COUNT(*) AS accounts_count').order('accounts_count desc') } scope :matches_username, ->(value) { where(arel_table[:username].matches("#{value}%")) } scope :matches_display_name, ->(value) { where(arel_table[:display_name].matches("#{value}%")) } scope :matches_domain, ->(value) { where(arel_table[:domain].matches("%#{value}%")) } @@ -438,10 +438,6 @@ class Account < ApplicationRecord super - %w(statuses_count following_count followers_count) end - def domains - reorder(nil).pluck(Arel.sql('distinct accounts.domain')) - end - def inboxes urls = reorder(nil).where(protocol: :activitypub).group(:preferred_inbox_url).pluck(Arel.sql("coalesce(nullif(accounts.shared_inbox_url, ''), accounts.inbox_url) AS preferred_inbox_url")) DeliveryFailureTracker.without_unavailable(urls) diff --git a/app/models/concerns/domain_materializable.rb b/app/models/concerns/domain_materializable.rb new file mode 100644 index 0000000000..88337f8c00 --- /dev/null +++ b/app/models/concerns/domain_materializable.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module DomainMaterializable + extend ActiveSupport::Concern + + included do + after_create_commit :refresh_instances_view + end + + def refresh_instances_view + Instance.refresh unless domain.nil? || Instance.where(domain: domain).exists? + end +end diff --git a/app/models/domain_allow.rb b/app/models/domain_allow.rb index 5fe0e3a29c..4b0a89c184 100644 --- a/app/models/domain_allow.rb +++ b/app/models/domain_allow.rb @@ -12,6 +12,7 @@ class DomainAllow < ApplicationRecord include DomainNormalizable + include DomainMaterializable validates :domain, presence: true, uniqueness: true, domain: true diff --git a/app/models/domain_block.rb b/app/models/domain_block.rb index 2b18e01fad..829d7583ba 100644 --- a/app/models/domain_block.rb +++ b/app/models/domain_block.rb @@ -16,6 +16,7 @@ class DomainBlock < ApplicationRecord include DomainNormalizable + include DomainMaterializable enum severity: [:silence, :suspend, :noop] diff --git a/app/models/instance.rb b/app/models/instance.rb index 3c740f8a2b..29be036626 100644 --- a/app/models/instance.rb +++ b/app/models/instance.rb @@ -1,26 +1,63 @@ # frozen_string_literal: true +# == Schema Information +# +# Table name: instances +# +# domain :string primary key +# accounts_count :bigint(8) +# -class Instance - include ActiveModel::Model +class Instance < ApplicationRecord + self.primary_key = :domain - attr_accessor :domain, :accounts_count, :domain_block + has_many :accounts, foreign_key: :domain, primary_key: :domain - def initialize(resource) - @domain = resource.domain - @accounts_count = resource.respond_to?(:accounts_count) ? resource.accounts_count : nil - @domain_block = resource.is_a?(DomainBlock) ? resource : DomainBlock.rule_for(domain) - @domain_allow = resource.is_a?(DomainAllow) ? resource : DomainAllow.rule_for(domain) + belongs_to :domain_block, foreign_key: :domain, primary_key: :domain + belongs_to :domain_allow, foreign_key: :domain, primary_key: :domain + + scope :matches_domain, ->(value) { where(arel_table[:domain].matches("%#{value}%")) } + + def self.refresh + Scenic.database.refresh_materialized_view(table_name, concurrently: true, cascade: false) end - def countable? - @accounts_count.present? + def readonly? + true + end + + def delivery_failure_tracker + @delivery_failure_tracker ||= DeliveryFailureTracker.new(domain) + end + + def following_count + @following_count ||= Follow.where(account: accounts).count + end + + def followers_count + @followers_count ||= Follow.where(target_account: accounts).count + end + + def reports_count + @reports_count ||= Report.where(target_account: accounts).count + end + + def blocks_count + @blocks_count ||= Block.where(target_account: accounts).count + end + + def public_comment + domain_block&.public_comment + end + + def private_comment + domain_block&.private_comment + end + + def media_storage + @media_storage ||= MediaAttachment.where(account: accounts).sum(:file_file_size) end def to_param domain end - - def cache_key - domain - end end diff --git a/app/models/instance_filter.rb b/app/models/instance_filter.rb index 9c467bc276..0598d8fead 100644 --- a/app/models/instance_filter.rb +++ b/app/models/instance_filter.rb @@ -13,18 +13,27 @@ class InstanceFilter end def results - if params[:limited].present? - scope = DomainBlock - scope = scope.matches_domain(params[:by_domain]) if params[:by_domain].present? - scope.order(id: :desc) - elsif params[:allowed].present? - scope = DomainAllow - scope = scope.matches_domain(params[:by_domain]) if params[:by_domain].present? - scope.order(id: :desc) + scope = Instance.includes(:domain_block, :domain_allow).order(accounts_count: :desc) + + params.each do |key, value| + scope.merge!(scope_for(key, value.to_s.strip)) if value.present? + end + + scope + end + + private + + def scope_for(key, value) + case key.to_s + when 'limited' + Instance.joins(:domain_block).reorder(Arel.sql('domain_blocks.id desc')) + when 'allowed' + Instance.joins(:domain_allow).reorder(Arel.sql('domain_allows.id desc')) + when 'by_domain' + Instance.matches_domain(value) else - scope = Account.remote - scope = scope.matches_domain(params[:by_domain]) if params[:by_domain].present? - scope.by_domain_accounts + raise "Unknown filter: #{key}" end end end diff --git a/app/models/unavailable_domain.rb b/app/models/unavailable_domain.rb index e2918b5860..5e8870bde0 100644 --- a/app/models/unavailable_domain.rb +++ b/app/models/unavailable_domain.rb @@ -12,6 +12,8 @@ class UnavailableDomain < ApplicationRecord include DomainNormalizable + validates :domain, presence: true, uniqueness: true + after_commit :reset_cache! private diff --git a/app/policies/domain_block_policy.rb b/app/policies/domain_block_policy.rb index 47c0a81af4..543259ccef 100644 --- a/app/policies/domain_block_policy.rb +++ b/app/policies/domain_block_policy.rb @@ -13,6 +13,10 @@ class DomainBlockPolicy < ApplicationPolicy admin? end + def update? + admin? + end + def destroy? admin? end diff --git a/app/presenters/instance_presenter.rb b/app/presenters/instance_presenter.rb index c150bf742b..1bfdd40ac9 100644 --- a/app/presenters/instance_presenter.rb +++ b/app/presenters/instance_presenter.rb @@ -29,7 +29,7 @@ class InstancePresenter end def domain_count - Rails.cache.fetch('distinct_domain_count') { Account.distinct.count(:domain) } + Rails.cache.fetch('distinct_domain_count') { Instance.count } end def sample_accounts diff --git a/app/views/admin/instances/_instance.html.haml b/app/views/admin/instances/_instance.html.haml new file mode 100644 index 0000000000..188d0d9841 --- /dev/null +++ b/app/views/admin/instances/_instance.html.haml @@ -0,0 +1,25 @@ +.directory__tag + = link_to admin_instance_path(instance) do + %h4 + = instance.domain + %small + - if instance.domain_block + - first_item = true + - if !instance.domain_block.noop? + = t("admin.domain_blocks.severity.#{instance.domain_block.severity}") + - first_item = false + - unless instance.domain_block.suspend? + - if instance.domain_block.reject_media? + - unless first_item + • + = t('admin.domain_blocks.rejecting_media') + - first_item = false + - if instance.domain_block.reject_reports? + - unless first_item + • + = t('admin.domain_blocks.rejecting_reports') + - elsif whitelist_mode? + = t('admin.accounts.whitelisted') + - else + = t('admin.accounts.no_limits_imposed') + .trends__item__current{ title: t('admin.instances.known_accounts', count: instance.accounts_count) }= number_to_human instance.accounts_count, strip_insignificant_zeros: true diff --git a/app/views/admin/instances/index.html.haml b/app/views/admin/instances/index.html.haml index 696ba3c7fe..5f20e7ec04 100644 --- a/app/views/admin/instances/index.html.haml +++ b/app/views/admin/instances/index.html.haml @@ -32,32 +32,10 @@ %hr.spacer/ -- @instances.each do |instance| - .directory__tag - = link_to admin_instance_path(instance) do - %h4 - = instance.domain - %small - - if instance.domain_block - - first_item = true - - if !instance.domain_block.noop? - = t("admin.domain_blocks.severity.#{instance.domain_block.severity}") - - first_item = false - - unless instance.domain_block.suspend? - - if instance.domain_block.reject_media? - - unless first_item - • - = t('admin.domain_blocks.rejecting_media') - - first_item = false - - if instance.domain_block.reject_reports? - - unless first_item - • - = t('admin.domain_blocks.rejecting_reports') - - elsif whitelist_mode? - = t('admin.accounts.whitelisted') - - else - = t('admin.accounts.no_limits_imposed') - - if instance.countable? - .trends__item__current{ title: t('admin.instances.known_accounts', count: instance.accounts_count) }= number_to_human instance.accounts_count, strip_insignificant_zeros: true +- if @instances.empty? + %div.muted-hint.center-text + = t 'admin.instances.empty' +- else + = render @instances -= paginate paginated_instances += paginate @instances diff --git a/app/views/admin/instances/show.html.haml b/app/views/admin/instances/show.html.haml index 92e14c0df0..0b9382771b 100644 --- a/app/views/admin/instances/show.html.haml +++ b/app/views/admin/instances/show.html.haml @@ -3,57 +3,59 @@ .dashboard__counters %div - %div - .dashboard__counters__num= number_with_delimiter @following_count - .dashboard__counters__label= t 'admin.instances.total_followed_by_them' - %div - %div - .dashboard__counters__num= number_with_delimiter @followers_count - .dashboard__counters__label= t 'admin.instances.total_followed_by_us' - %div - %div - .dashboard__counters__num= number_to_human_size @media_storage - .dashboard__counters__label= t 'admin.instances.total_storage' - %div - %div - .dashboard__counters__num= number_with_delimiter @blocks_count - .dashboard__counters__label= t 'admin.instances.total_blocked_by_us' + = link_to admin_accounts_path(remote: '1', by_domain: @instance.domain) do + .dashboard__counters__num= number_with_delimiter @instance.accounts_count + .dashboard__counters__label= t 'admin.accounts.title' %div = link_to admin_reports_path(by_target_domain: @instance.domain) do - .dashboard__counters__num= number_with_delimiter @reports_count + .dashboard__counters__num= number_with_delimiter @instance.reports_count .dashboard__counters__label= t 'admin.instances.total_reported' + %div + %div + .dashboard__counters__num= number_to_human_size @instance.media_storage + .dashboard__counters__label= t 'admin.instances.total_storage' + %div + %div + .dashboard__counters__num= number_with_delimiter @instance.following_count + .dashboard__counters__label= t 'admin.instances.total_followed_by_them' + %div + %div + .dashboard__counters__num= number_with_delimiter @instance.followers_count + .dashboard__counters__label= t 'admin.instances.total_followed_by_us' + %div + %div + .dashboard__counters__num= number_with_delimiter @instance.blocks_count + .dashboard__counters__label= t 'admin.instances.total_blocked_by_us' + %div %div .dashboard__counters__num - - if @available + - if @instance.delivery_failure_tracker.available? = fa_icon 'check' - else = fa_icon 'times' .dashboard__counters__label= t 'admin.instances.delivery_available' -- if @private_comment.present? +- if @instance.private_comment.present? .speech-bubble .speech-bubble__bubble - = simple_format(h(@private_comment)) + = simple_format(h(@instance.private_comment)) .speech-bubble__owner= t 'admin.instances.private_comment' -- if @public_comment.present? +- if @instance.public_comment.present? .speech-bubble .speech-bubble__bubble - = simple_format(h(@public_comment)) + = simple_format(h(@instance.public_comment)) .speech-bubble__owner= t 'admin.instances.public_comment' %hr.spacer/ %div.action-buttons %div - = link_to t('admin.accounts.title'), admin_accounts_path(remote: '1', by_domain: @instance.domain), class: 'button' - - %div - - if @domain_allow - = link_to t('admin.domain_allows.undo'), admin_domain_allow_path(@domain_allow), class: 'button button--destructive', data: { confirm: t('admin.accounts.are_you_sure'), method: :delete } - - elsif @domain_block - = link_to t('admin.domain_blocks.edit'), edit_admin_domain_block_path(@domain_block), class: 'button' - = link_to t('admin.domain_blocks.undo'), admin_domain_block_path(@domain_block), class: 'button' + - if @instance.domain_allow + = link_to t('admin.domain_allows.undo'), admin_domain_allow_path(@instance.domain_allow), class: 'button button--destructive', data: { confirm: t('admin.accounts.are_you_sure'), method: :delete } + - elsif @instance.domain_block + = link_to t('admin.domain_blocks.edit'), edit_admin_domain_block_path(@instance.domain_block), class: 'button' + = link_to t('admin.domain_blocks.undo'), admin_domain_block_path(@instance.domain_block), class: 'button' - else = link_to t('admin.domain_blocks.add_new'), new_admin_domain_block_path(_domain: @instance.domain), class: 'button' diff --git a/app/workers/scheduler/instance_refresh_scheduler.rb b/app/workers/scheduler/instance_refresh_scheduler.rb new file mode 100644 index 0000000000..917404becc --- /dev/null +++ b/app/workers/scheduler/instance_refresh_scheduler.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class Scheduler::InstanceRefreshScheduler + include Sidekiq::Worker + + sidekiq_options lock: :until_executed, retry: 0 + + def perform + Instance.refresh + end +end diff --git a/config/brakeman.ignore b/config/brakeman.ignore index baa993c78a..dcbfd02b4e 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -102,6 +102,37 @@ "confidence": "Weak", "note": "" }, + { + "warning_type": "Dynamic Render Path", + "warning_code": 15, + "fingerprint": "4704e8093e3e0561bf705f892e8fc6780419f8255f4440b1c0afd09339bd6446", + "check_name": "Render", + "message": "Render path contains parameter value", + "file": "app/views/admin/instances/index.html.haml", + "line": 39, + "link": "https://brakemanscanner.org/docs/warning_types/dynamic_render_path/", + "code": "render(action => filtered_instances.page(params[:page]), {})", + "render_path": [ + { + "type": "controller", + "class": "Admin::InstancesController", + "method": "index", + "line": 10, + "file": "app/controllers/admin/instances_controller.rb", + "rendered": { + "name": "admin/instances/index", + "file": "app/views/admin/instances/index.html.haml" + } + } + ], + "location": { + "type": "template", + "template": "admin/instances/index" + }, + "user_input": "params[:page]", + "confidence": "Weak", + "note": "" + }, { "warning_type": "Redirect", "warning_code": 18, @@ -122,6 +153,26 @@ "confidence": "High", "note": "" }, + { + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "6e4051854bb62e2ddbc671f82d6c2328892e1134b8b28105ecba9b0122540714", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "app/models/account.rb", + "line": 491, + "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "find_by_sql([\" WITH first_degree AS (\\n SELECT target_account_id\\n FROM follows\\n WHERE account_id = ?\\n UNION ALL\\n SELECT ?\\n )\\n SELECT\\n accounts.*,\\n (count(f.id) + 1) * ts_rank_cd(#{textsearch}, #{query}, 32) AS rank\\n FROM accounts\\n LEFT OUTER JOIN follows AS f ON (accounts.id = f.account_id AND f.target_account_id = ?)\\n WHERE accounts.id IN (SELECT * FROM first_degree)\\n AND #{query} @@ #{textsearch}\\n AND accounts.suspended_at IS NULL\\n AND accounts.moved_to_account_id IS NULL\\n GROUP BY accounts.id\\n ORDER BY rank DESC\\n LIMIT ? OFFSET ?\\n\".squish, account.id, account.id, account.id, limit, offset])", + "render_path": null, + "location": { + "type": "method", + "class": "Account", + "method": "advanced_search_for" + }, + "user_input": "textsearch", + "confidence": "Medium", + "note": "" + }, { "warning_type": "SQL Injection", "warning_code": 0, @@ -163,23 +214,23 @@ "note": "" }, { - "warning_type": "Mass Assignment", - "warning_code": 105, - "fingerprint": "8f63dec68951d9bcf7eddb15af9392b2e1333003089c41fb76688dfd3579f394", - "check_name": "PermitAttributes", - "message": "Potentially dangerous key allowed for mass assignment", - "file": "app/controllers/api/v1/crypto/deliveries_controller.rb", - "line": 23, - "link": "https://brakemanscanner.org/docs/warning_types/mass_assignment/", - "code": "params.require(:device).permit(:account_id, :device_id, :type, :body, :hmac)", + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "9251d682c4e2840e1b2fea91e7d758efe2097ecb7f6255c065e3750d25eb178c", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "app/models/account.rb", + "line": 460, + "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "find_by_sql([\" SELECT\\n accounts.*,\\n ts_rank_cd(#{textsearch}, #{query}, 32) AS rank\\n FROM accounts\\n WHERE #{query} @@ #{textsearch}\\n AND accounts.suspended_at IS NULL\\n AND accounts.moved_to_account_id IS NULL\\n ORDER BY rank DESC\\n LIMIT ? OFFSET ?\\n\".squish, limit, offset])", "render_path": null, "location": { "type": "method", - "class": "Api::V1::Crypto::DeliveriesController", - "method": "resource_params" + "class": "Account", + "method": "search_for" }, - "user_input": ":account_id", - "confidence": "High", + "user_input": "textsearch", + "confidence": "Medium", "note": "" }, { @@ -273,6 +324,26 @@ "confidence": "High", "note": "" }, + { + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "e21d8fee7a5805761679877ca35ed1029c64c45ef3b4012a30262623e1ba8bb9", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "app/models/account.rb", + "line": 507, + "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "find_by_sql([\" SELECT\\n accounts.*,\\n (count(f.id) + 1) * ts_rank_cd(#{textsearch}, #{query}, 32) AS rank\\n FROM accounts\\n LEFT OUTER JOIN follows AS f ON (accounts.id = f.account_id AND f.target_account_id = ?) OR (accounts.id = f.target_account_id AND f.account_id = ?)\\n WHERE #{query} @@ #{textsearch}\\n AND accounts.suspended_at IS NULL\\n AND accounts.moved_to_account_id IS NULL\\n GROUP BY accounts.id\\n ORDER BY rank DESC\\n LIMIT ? OFFSET ?\\n\".squish, account.id, account.id, limit, offset])", + "render_path": null, + "location": { + "type": "method", + "class": "Account", + "method": "advanced_search_for" + }, + "user_input": "textsearch", + "confidence": "Medium", + "note": "" + }, { "warning_type": "Mass Assignment", "warning_code": 105, @@ -294,6 +365,6 @@ "note": "" } ], - "updated": "2020-06-01 18:18:02 +0200", - "brakeman_version": "4.8.0" + "updated": "2020-12-07 01:17:13 +0100", + "brakeman_version": "4.10.0" } diff --git a/config/locales/en.yml b/config/locales/en.yml index 59f561aa33..f89f50e4dd 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -255,6 +255,7 @@ en: unsuspend_account: Unsuspend Account update_announcement: Update Announcement update_custom_emoji: Update Custom Emoji + update_domain_block: Update Domain Block update_status: Update Status actions: assigned_to_self_report: "%{name} assigned report %{target} to themselves" @@ -295,6 +296,7 @@ en: unsuspend_account: "%{name} unsuspended %{target}'s account" update_announcement: "%{name} updated announcement %{target}" update_custom_emoji: "%{name} updated emoji %{target}" + update_domain_block: "%{name} updated domain block for %{target}" update_status: "%{name} updated status by %{target}" deleted_status: "(deleted status)" empty: No logs found. @@ -437,6 +439,7 @@ en: instances: by_domain: Domain delivery_available: Delivery is available + empty: No domains found. known_accounts: one: "%{count} known account" other: "%{count} known accounts" diff --git a/config/sidekiq.yml b/config/sidekiq.yml index 5de25de234..a71c1098ec 100644 --- a/config/sidekiq.yml +++ b/config/sidekiq.yml @@ -36,3 +36,6 @@ pghero_scheduler: cron: '0 0 * * *' class: Scheduler::PgheroScheduler + instance_refresh_scheduler: + cron: '0 * * * *' + class: Scheduler::InstanceRefreshScheduler diff --git a/db/migrate/20201206004238_create_instances.rb b/db/migrate/20201206004238_create_instances.rb new file mode 100644 index 0000000000..a4b866894a --- /dev/null +++ b/db/migrate/20201206004238_create_instances.rb @@ -0,0 +1,9 @@ +class CreateInstances < ActiveRecord::Migration[5.2] + def change + create_view :instances, materialized: true + + # To be able to refresh the view concurrently, + # at least one unique index is required + safety_assured { add_index :instances, :domain, unique: true } + end +end diff --git a/db/schema.rb b/db/schema.rb index 873c37f67c..2f9d369be2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2020_10_17_234926) do +ActiveRecord::Schema.define(version: 2020_12_06_004238) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -1045,4 +1045,29 @@ ActiveRecord::Schema.define(version: 2020_10_17_234926) do add_foreign_key "web_push_subscriptions", "users", on_delete: :cascade add_foreign_key "web_settings", "users", name: "fk_11910667b2", on_delete: :cascade add_foreign_key "webauthn_credentials", "users" + + create_view "instances", materialized: true, sql_definition: <<-SQL + WITH domain_counts(domain, accounts_count) AS ( + SELECT accounts.domain, + count(*) AS accounts_count + FROM accounts + WHERE (accounts.domain IS NOT NULL) + GROUP BY accounts.domain + ) + SELECT domain_counts.domain, + domain_counts.accounts_count + FROM domain_counts + UNION + SELECT domain_blocks.domain, + COALESCE(domain_counts.accounts_count, (0)::bigint) AS accounts_count + FROM (domain_blocks + LEFT JOIN domain_counts ON (((domain_counts.domain)::text = (domain_blocks.domain)::text))) + UNION + SELECT domain_allows.domain, + COALESCE(domain_counts.accounts_count, (0)::bigint) AS accounts_count + FROM (domain_allows + LEFT JOIN domain_counts ON (((domain_counts.domain)::text = (domain_allows.domain)::text))); + SQL + add_index "instances", ["domain"], name: "index_instances_on_domain", unique: true + end diff --git a/db/views/instances_v01.sql b/db/views/instances_v01.sql new file mode 100644 index 0000000000..94acd61a13 --- /dev/null +++ b/db/views/instances_v01.sql @@ -0,0 +1,17 @@ +WITH domain_counts(domain, accounts_count) +AS ( + SELECT domain, COUNT(*) as accounts_count + FROM accounts + WHERE domain IS NOT NULL + GROUP BY domain +) +SELECT domain, accounts_count +FROM domain_counts +UNION +SELECT domain_blocks.domain, COALESCE(domain_counts.accounts_count, 0) +FROM domain_blocks +LEFT OUTER JOIN domain_counts ON domain_counts.domain = domain_blocks.domain +UNION +SELECT domain_allows.domain, COALESCE(domain_counts.accounts_count, 0) +FROM domain_allows +LEFT OUTER JOIN domain_counts ON domain_counts.domain = domain_allows.domain diff --git a/lib/mastodon/domains_cli.rb b/lib/mastodon/domains_cli.rb index 5433ddd9d7..3c2dfd4ec2 100644 --- a/lib/mastodon/domains_cli.rb +++ b/lib/mastodon/domains_cli.rb @@ -53,6 +53,8 @@ module Mastodon custom_emojis_count = custom_emojis.count custom_emojis.destroy_all unless options[:dry_run] + Instance.refresh unless options[:dry_run] + say("Removed #{custom_emojis_count} custom emojis", :green) end @@ -83,7 +85,7 @@ module Mastodon processed = Concurrent::AtomicFixnum.new(0) failed = Concurrent::AtomicFixnum.new(0) start_at = Time.now.to_f - seed = start ? [start] : Account.remote.domains + seed = start ? [start] : Instance.pluck(:domain) blocked_domains = Regexp.new('\\.?' + DomainBlock.where(severity: 1).pluck(:domain).join('|') + '$') progress = create_progress_bar diff --git a/spec/controllers/admin/instances_controller_spec.rb b/spec/controllers/admin/instances_controller_spec.rb index 412b814439..8c0b309f2a 100644 --- a/spec/controllers/admin/instances_controller_spec.rb +++ b/spec/controllers/admin/instances_controller_spec.rb @@ -9,10 +9,10 @@ RSpec.describe Admin::InstancesController, type: :controller do describe 'GET #index' do around do |example| - default_per_page = Account.default_per_page - Account.paginates_per 1 + default_per_page = Instance.default_per_page + Instance.paginates_per 1 example.run - Account.paginates_per default_per_page + Instance.paginates_per default_per_page end it 'renders instances' do diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index 75f628076d..1d000ed4d2 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)