From 17ea22671de0705ec805bb157754d5ae5f24f9e3 Mon Sep 17 00:00:00 2001
From: Matt Jankowski <matt@jankowski.online>
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 <matt@jankowski.online>
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 <matt@jankowski.online>
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 <matt@jankowski.online>
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