From 17ea22671de0705ec805bb157754d5ae5f24f9e3 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Thu, 25 Jan 2024 10:13:41 -0500 Subject: [PATCH 1/4] Fix `Style/GuardClause` cop in app/controllers (#28420) --- .rubocop_todo.yml | 4 ---- .../admin/confirmations_controller.rb | 14 +++++++------ .../auth/confirmations_controller.rb | 12 ++++++----- app/controllers/auth/passwords_controller.rb | 10 ++++------ .../webauthn_credentials_controller.rb | 20 ++++++++----------- 5 files changed, 27 insertions(+), 33 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index fd9dc18ac7..c8165c1edf 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -121,10 +121,6 @@ Style/GlobalStdStream: # Configuration parameters: MinBodyLength, AllowConsecutiveConditionals. Style/GuardClause: Exclude: - - 'app/controllers/admin/confirmations_controller.rb' - - 'app/controllers/auth/confirmations_controller.rb' - - 'app/controllers/auth/passwords_controller.rb' - - 'app/controllers/settings/two_factor_authentication/webauthn_credentials_controller.rb' - 'app/lib/activitypub/activity/block.rb' - 'app/lib/request.rb' - 'app/lib/request_pool.rb' diff --git a/app/controllers/admin/confirmations_controller.rb b/app/controllers/admin/confirmations_controller.rb index 7ccf5c9012..702550eecc 100644 --- a/app/controllers/admin/confirmations_controller.rb +++ b/app/controllers/admin/confirmations_controller.rb @@ -3,7 +3,7 @@ module Admin class ConfirmationsController < BaseController before_action :set_user - before_action :check_confirmation, only: [:resend] + before_action :redirect_confirmed_user, only: [:resend], if: :user_confirmed? def create authorize @user, :confirm? @@ -25,11 +25,13 @@ module Admin private - def check_confirmation - if @user.confirmed? - flash[:error] = I18n.t('admin.accounts.resend_confirmation.already_confirmed') - redirect_to admin_accounts_path - end + def redirect_confirmed_user + flash[:error] = I18n.t('admin.accounts.resend_confirmation.already_confirmed') + redirect_to admin_accounts_path + end + + def user_confirmed? + @user.confirmed? end end end diff --git a/app/controllers/auth/confirmations_controller.rb b/app/controllers/auth/confirmations_controller.rb index d9cd630905..7ca7be5f8e 100644 --- a/app/controllers/auth/confirmations_controller.rb +++ b/app/controllers/auth/confirmations_controller.rb @@ -7,7 +7,7 @@ class Auth::ConfirmationsController < Devise::ConfirmationsController before_action :set_body_classes before_action :set_confirmation_user!, only: [:show, :confirm_captcha] - before_action :require_unconfirmed! + before_action :redirect_confirmed_user, if: :signed_in_confirmed_user? before_action :extend_csp_for_captcha!, only: [:show, :confirm_captcha] before_action :require_captcha_if_needed!, only: [:show] @@ -65,10 +65,12 @@ class Auth::ConfirmationsController < Devise::ConfirmationsController @confirmation_user.nil? || @confirmation_user.confirmed? end - def require_unconfirmed! - if user_signed_in? && current_user.confirmed? && current_user.unconfirmed_email.blank? - redirect_to(current_user.approved? ? root_path : edit_user_registration_path) - end + def redirect_confirmed_user + redirect_to(current_user.approved? ? root_path : edit_user_registration_path) + end + + def signed_in_confirmed_user? + user_signed_in? && current_user.confirmed? && current_user.unconfirmed_email.blank? end def set_body_classes diff --git a/app/controllers/auth/passwords_controller.rb b/app/controllers/auth/passwords_controller.rb index a752194d5b..de001f062b 100644 --- a/app/controllers/auth/passwords_controller.rb +++ b/app/controllers/auth/passwords_controller.rb @@ -2,7 +2,7 @@ class Auth::PasswordsController < Devise::PasswordsController skip_before_action :check_self_destruct! - before_action :check_validity_of_reset_password_token, only: :edit + before_action :redirect_invalid_reset_token, only: :edit, unless: :reset_password_token_is_valid? before_action :set_body_classes layout 'auth' @@ -19,11 +19,9 @@ class Auth::PasswordsController < Devise::PasswordsController private - def check_validity_of_reset_password_token - unless reset_password_token_is_valid? - flash[:error] = I18n.t('auth.invalid_reset_password_token') - redirect_to new_password_path(resource_name) - end + def redirect_invalid_reset_token + flash[:error] = I18n.t('auth.invalid_reset_password_token') + redirect_to new_password_path(resource_name) end def set_body_classes diff --git a/app/controllers/settings/two_factor_authentication/webauthn_credentials_controller.rb b/app/controllers/settings/two_factor_authentication/webauthn_credentials_controller.rb index c86ede4f3a..9714d54f95 100644 --- a/app/controllers/settings/two_factor_authentication/webauthn_credentials_controller.rb +++ b/app/controllers/settings/two_factor_authentication/webauthn_credentials_controller.rb @@ -6,8 +6,8 @@ module Settings skip_before_action :check_self_destruct! skip_before_action :require_functional! - before_action :require_otp_enabled - before_action :require_webauthn_enabled, only: [:index, :destroy] + before_action :redirect_invalid_otp, unless: -> { current_user.otp_enabled? } + before_action :redirect_invalid_webauthn, only: [:index, :destroy], unless: -> { current_user.webauthn_enabled? } def index; end def new; end @@ -85,18 +85,14 @@ module Settings private - def require_otp_enabled - unless current_user.otp_enabled? - flash[:error] = t('webauthn_credentials.otp_required') - redirect_to settings_two_factor_authentication_methods_path - end + def redirect_invalid_otp + flash[:error] = t('webauthn_credentials.otp_required') + redirect_to settings_two_factor_authentication_methods_path end - def require_webauthn_enabled - unless current_user.webauthn_enabled? - flash[:error] = t('webauthn_credentials.not_enabled') - redirect_to settings_two_factor_authentication_methods_path - end + def redirect_invalid_webauthn + flash[:error] = t('webauthn_credentials.not_enabled') + redirect_to settings_two_factor_authentication_methods_path end end end From 0b38946c874f4e02295a303514aaf8895cf8a918 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Thu, 25 Jan 2024 10:18:15 -0500 Subject: [PATCH 2/4] Update paperclip and climate_control gems (#28379) --- Gemfile | 2 +- Gemfile.lock | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Gemfile b/Gemfile index 2d4e504ac7..4951304e39 100644 --- a/Gemfile +++ b/Gemfile @@ -123,7 +123,7 @@ group :test do gem 'database_cleaner-active_record' # Used to mock environment variables - gem 'climate_control', '~> 0.2' + gem 'climate_control' # Generating fake data for specs gem 'faker', '~> 3.2' diff --git a/Gemfile.lock b/Gemfile.lock index a31d0a929c..57b2580722 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -185,7 +185,7 @@ GEM elasticsearch (>= 7.12.0, < 7.14.0) elasticsearch-dsl chunky_png (1.4.0) - climate_control (0.2.0) + climate_control (1.2.0) cocoon (1.2.15) color_diff (0.1) concurrent-ruby (1.2.3) @@ -746,8 +746,8 @@ GEM temple (0.10.3) terminal-table (3.0.2) unicode-display_width (>= 1.1.1, < 3) - terrapin (0.6.0) - climate_control (>= 0.0.3, < 1.0) + terrapin (1.0.1) + climate_control test-prof (1.3.1) thor (1.3.0) tilt (2.3.0) @@ -836,7 +836,7 @@ DEPENDENCIES capybara (~> 3.39) charlock_holmes (~> 0.7.7) chewy (~> 7.3) - climate_control (~> 0.2) + climate_control cocoon (~> 1.2) color_diff (~> 0.1) concurrent-ruby From 4cdf62e576488e8c41f79c2a04a1630df9685592 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Thu, 25 Jan 2024 10:26:51 -0500 Subject: [PATCH 3/4] Extract `rebuild_index` method in maintenance CLI (#28911) --- lib/mastodon/cli/maintenance.rb | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/mastodon/cli/maintenance.rb b/lib/mastodon/cli/maintenance.rb index c644729b5e..a64206065d 100644 --- a/lib/mastodon/cli/maintenance.rb +++ b/lib/mastodon/cli/maintenance.rb @@ -244,10 +244,10 @@ module Mastodon::CLI end say 'Reindexing textual indexes on accounts…' - database_connection.execute('REINDEX INDEX search_index;') - database_connection.execute('REINDEX INDEX index_accounts_on_uri;') - database_connection.execute('REINDEX INDEX index_accounts_on_url;') - database_connection.execute('REINDEX INDEX index_accounts_on_domain_and_id;') if migrator_version >= 2023_05_24_190515 + rebuild_index(:search_index) + rebuild_index(:index_accounts_on_uri) + rebuild_index(:index_accounts_on_url) + rebuild_index(:index_accounts_on_domain_and_id) if migrator_version >= 2023_05_24_190515 end def deduplicate_users! @@ -274,7 +274,7 @@ module Mastodon::CLI database_connection.add_index :users, ['reset_password_token'], name: 'index_users_on_reset_password_token', unique: true, where: 'reset_password_token IS NOT NULL', opclass: :text_pattern_ops end - database_connection.execute('REINDEX INDEX index_users_on_unconfirmed_email;') if migrator_version >= 2023_07_02_151753 + rebuild_index(:index_users_on_unconfirmed_email) if migrator_version >= 2023_07_02_151753 end def deduplicate_users_process_email @@ -735,5 +735,9 @@ module Mastodon::CLI def db_table_exists?(table) database_connection.table_exists?(table) end + + def rebuild_index(name) + database_connection.execute("REINDEX INDEX #{name}") + end end end From 42ab855b2339c5cea3229c856ab539f883736b12 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Thu, 25 Jan 2024 10:28:27 -0500 Subject: [PATCH 4/4] Add specs for `Instance` model scopes and add `with_domain_follows` scope (#28767) --- .../admin/export_domain_blocks_controller.rb | 6 +- app/models/instance.rb | 14 +++ spec/models/instance_spec.rb | 104 ++++++++++++++++++ 3 files changed, 123 insertions(+), 1 deletion(-) create mode 100644 spec/models/instance_spec.rb diff --git a/app/controllers/admin/export_domain_blocks_controller.rb b/app/controllers/admin/export_domain_blocks_controller.rb index ffc4478172..9caafd9684 100644 --- a/app/controllers/admin/export_domain_blocks_controller.rb +++ b/app/controllers/admin/export_domain_blocks_controller.rb @@ -49,7 +49,7 @@ module Admin next end - @warning_domains = Instance.where(domain: @domain_blocks.map(&:domain)).where('EXISTS (SELECT 1 FROM follows JOIN accounts ON follows.account_id = accounts.id OR follows.target_account_id = accounts.id WHERE accounts.domain = instances.domain)').pluck(:domain) + @warning_domains = instances_from_imported_blocks.pluck(:domain) rescue ActionController::ParameterMissing flash.now[:alert] = I18n.t('admin.export_domain_blocks.no_file') set_dummy_import! @@ -58,6 +58,10 @@ module Admin private + def instances_from_imported_blocks + Instance.with_domain_follows(@domain_blocks.map(&:domain)) + end + def export_filename 'domain_blocks.csv' end diff --git a/app/models/instance.rb b/app/models/instance.rb index 2dec75d6fe..0fd31c8097 100644 --- a/app/models/instance.rb +++ b/app/models/instance.rb @@ -25,11 +25,25 @@ class Instance < ApplicationRecord scope :matches_domain, ->(value) { where(arel_table[:domain].matches("%#{value}%")) } scope :domain_starts_with, ->(value) { where(arel_table[:domain].matches("#{sanitize_sql_like(value)}%", false, true)) } scope :by_domain_and_subdomains, ->(domain) { where("reverse('.' || domain) LIKE reverse(?)", "%.#{domain}") } + scope :with_domain_follows, ->(domains) { where(domain: domains).where(domain_account_follows) } def self.refresh Scenic.database.refresh_materialized_view(table_name, concurrently: true, cascade: false) end + def self.domain_account_follows + Arel.sql( + <<~SQL.squish + EXISTS ( + SELECT 1 + FROM follows + JOIN accounts ON follows.account_id = accounts.id OR follows.target_account_id = accounts.id + WHERE accounts.domain = instances.domain + ) + SQL + ) + end + def readonly? true end diff --git a/spec/models/instance_spec.rb b/spec/models/instance_spec.rb new file mode 100644 index 0000000000..3e811d3325 --- /dev/null +++ b/spec/models/instance_spec.rb @@ -0,0 +1,104 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Instance do + describe 'Scopes' do + before { described_class.refresh } + + describe '#searchable' do + let(:expected_domain) { 'host.example' } + let(:blocked_domain) { 'other.example' } + + before do + Fabricate :account, domain: expected_domain + Fabricate :account, domain: blocked_domain + Fabricate :domain_block, domain: blocked_domain + end + + it 'returns records not domain blocked' do + results = described_class.searchable.pluck(:domain) + + expect(results) + .to include(expected_domain) + .and not_include(blocked_domain) + end + end + + describe '#matches_domain' do + let(:host_domain) { 'host.example.com' } + let(:host_under_domain) { 'host_under.example.com' } + let(:other_domain) { 'other.example' } + + before do + Fabricate :account, domain: host_domain + Fabricate :account, domain: host_under_domain + Fabricate :account, domain: other_domain + end + + it 'returns matching records' do + expect(described_class.matches_domain('host.exa').pluck(:domain)) + .to include(host_domain) + .and not_include(other_domain) + + expect(described_class.matches_domain('ple.com').pluck(:domain)) + .to include(host_domain) + .and not_include(other_domain) + + expect(described_class.matches_domain('example').pluck(:domain)) + .to include(host_domain) + .and include(other_domain) + + expect(described_class.matches_domain('host_').pluck(:domain)) # Preserve SQL wildcards + .to include(host_domain) + .and include(host_under_domain) + .and not_include(other_domain) + end + end + + describe '#by_domain_and_subdomains' do + let(:exact_match_domain) { 'example.com' } + let(:subdomain_domain) { 'foo.example.com' } + let(:partial_domain) { 'grexample.com' } + + before do + Fabricate(:account, domain: exact_match_domain) + Fabricate(:account, domain: subdomain_domain) + Fabricate(:account, domain: partial_domain) + end + + it 'returns matching instances' do + results = described_class.by_domain_and_subdomains('example.com').pluck(:domain) + + expect(results) + .to include(exact_match_domain) + .and include(subdomain_domain) + .and not_include(partial_domain) + end + end + + describe '#with_domain_follows' do + let(:example_domain) { 'example.host' } + let(:other_domain) { 'other.host' } + let(:none_domain) { 'none.host' } + + before do + example_account = Fabricate(:account, domain: example_domain) + other_account = Fabricate(:account, domain: other_domain) + Fabricate(:account, domain: none_domain) + + Fabricate :follow, account: example_account + Fabricate :follow, target_account: other_account + end + + it 'returns instances with domain accounts that have follows' do + results = described_class.with_domain_follows(['example.host', 'other.host', 'none.host']).pluck(:domain) + + expect(results) + .to include(example_domain) + .and include(other_domain) + .and not_include(none_domain) + end + end + end +end