Skip to content

Commit

Permalink
Backport fixes to 3.2 (mastodon#15360)
Browse files Browse the repository at this point in the history
* Fix 2FA/sign-in token sessions being valid after password change (mastodon#14802)

If someone tries logging in to an account and is prompted for a 2FA
code or sign-in token, even if the account's password or e-mail is
updated in the meantime, the session will show the prompt and allow
the login process to complete with a valid 2FA code or sign-in token

* Fix Move handler not being triggered when failing to fetch target (mastodon#15107)

When failing to fetch the target account, the ProcessingWorker fails
as expected, but since it hasn't cleared the `move_in_progress` flag,
the next attempt at processing skips the `Move` activity altogether.

This commit changes it to clear the flag when encountering any
unexpected error on fetching the target account. This is likely to
occur because, of, e.g., a timeout, when many instances query the
same actor at the same time.

* Fix slow distinct queries where grouped queries are faster (mastodon#15287)

About 2x speed-up on inboxes query

* Fix possible inconsistencies in tag search (mastodon#14906)

Do not downcase the queried tag before passing it to postgres when searching:
- tags are not downcased on creation
- `arel_table[:name].lower.matches(pattern)` generates an ILIKE anyway
- if Postgres and Rails happen to use different case-folding rules,
  downcasing before query but not before insertion may mean that some
  tags with some casings are not searchable

* Fix updating account counters when account_stat is not yet created (mastodon#15108)

* Fix account processing failing because of large collections (mastodon#15027)

Fixes mastodon#15025

* Fix downloading remote media files when server returns empty filename (mastodon#14867)

Fixes mastodon#14817

* Fix webfinger redirect handling in ResolveAccountService (mastodon#15187)

* Fix webfinger redirect handling in ResolveAccountService

ResolveAccountService#process_webfinger! handled a one-step webfinger
redirection, but only accepting the result if it matched the exact URI passed
as input, defeating the point of a redirection check.

Instead, use the same logic as in `ActivityPub::FetchRemoteAccountService`,
updating the resulting `acct:` URI with the result of the first webfinger
query.

* Add tests

* Remove dependency on unused and unmaintained http_parser.rb gem (mastodon#14574)

It seems that years ago, the “http” gem dependend on the “http_parser.rb” gem
(it now depends on the “http-parser” gem), and, still years ago, we pulled
it from git in order to benefit from a bugfix that wasn't released yet (mastodon#7467).

* Add tootctl maintenance fix-duplicates (mastodon#14860, mastodon#15201, mastodon#15264, mastodon#15349, mastodon#15359)

* Fix old migration script not being able to run if it fails midway (mastodon#15361)

* Fix old migration script not being able to run if it fails midway

Improve the robustness of a migration script likely to fail because of database
corruption so it can run again once database corruptions are fixed.

* Display a specific error message in case of index corruption

Co-authored-by: Eugen Rochko <eugen@zeonfederated.com>
Co-authored-by: Claire <claire.github-309c@sitedethib.com>

Co-authored-by: Eugen Rochko <eugen@zeonfederated.com>
Co-authored-by: Claire <claire.github-309c@sitedethib.com>
  • Loading branch information
3 people authored and Mage committed Jan 14, 2022
1 parent 8dde739 commit 9a45ab5
Show file tree
Hide file tree
Showing 24 changed files with 824 additions and 90 deletions.
1 change: 0 additions & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ gem 'health_check', git: 'https://github.com/ianheggie/health_check', ref: '0b79
gem 'htmlentities', '~> 4.3'
gem 'http', '~> 4.4'
gem 'http_accept_language', '~> 2.1'
gem 'http_parser.rb', '~> 0.6', git: 'https://github.com/tmm1/http_parser.rb', ref: '54b17ba8c7d8d20a16dfc65d1775241833219cf2', submodules: true
gem 'httplog', '~> 1.4.3'
gem 'idn-ruby', require: 'idn'
gem 'kaminari', '~> 1.2'
Expand Down
9 changes: 0 additions & 9 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,6 @@ GIT
specs:
posix-spawn (0.3.13)

GIT
remote: https://github.com/tmm1/http_parser.rb
revision: 54b17ba8c7d8d20a16dfc65d1775241833219cf2
ref: 54b17ba8c7d8d20a16dfc65d1775241833219cf2
submodules: true
specs:
http_parser.rb (0.6.1)

GIT
remote: https://github.com/witgo/nilsimsa
revision: fd184883048b922b176939f851338d0a4971a532
Expand Down Expand Up @@ -709,7 +701,6 @@ DEPENDENCIES
htmlentities (~> 4.3)
http (~> 4.4)
http_accept_language (~> 2.1)
http_parser.rb (~> 0.6)!
httplog (~> 1.4.3)
i18n-tasks (~> 0.9)
idn-ruby
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/accounts_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ def only_media_scope
end

def account_media_status_ids
@account.media_attachments.attached.reorder(nil).select(:status_id).distinct
@account.media_attachments.attached.reorder(nil).select(:status_id).group(:status_id)
end

def no_replies_scope
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/admin/statuses_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def index
@statuses = @account.statuses.where(visibility: [:public, :unlisted])

if params[:media]
account_media_status_ids = @account.media_attachments.attached.reorder(nil).select(:status_id).distinct
account_media_status_ids = @account.media_attachments.attached.reorder(nil).select(:status_id).group(:status_id)
@statuses.merge!(Status.where(id: account_media_status_ids))
end

Expand Down
2 changes: 1 addition & 1 deletion app/controllers/api/base_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ def require_user!
elsif !current_user.approved?
render json: { error: 'Your login is currently pending approval' }, status: 403
else
set_user_activity
update_user_sign_in
end
end

Expand Down
22 changes: 21 additions & 1 deletion app/controllers/auth/sessions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ class Auth::SessionsController < Devise::SessionsController

skip_before_action :require_no_authentication, only: [:create]
skip_before_action :require_functional!
skip_before_action :update_user_sign_in

include TwoFactorAuthenticationConcern
include SignInTokenAuthenticationConcern
Expand All @@ -25,6 +26,7 @@ def new

def create
super do |resource|
resource.update_sign_in!(request, new_sign_in: true)
remember_me(resource)
flash.delete(:notice)
end
Expand All @@ -42,7 +44,7 @@ def destroy

def find_user
if session[:attempt_user_id]
User.find(session[:attempt_user_id])
User.find_by(id: session[:attempt_user_id])
else
user = User.authenticate_with_ldap(user_params) if Devise.ldap_authentication
user ||= User.authenticate_with_pam(user_params) if Devise.pam_authentication
Expand Down Expand Up @@ -75,6 +77,7 @@ def after_sign_out_path_for(_resource_or_scope)

def require_no_authentication
super

# Delete flash message that isn't entirely useful and may be confusing in
# most cases because /web doesn't display/clear flash messages.
flash.delete(:alert) if flash[:alert] == I18n.t('devise.failure.already_authenticated')
Expand All @@ -92,13 +95,30 @@ def set_body_classes

def home_paths(resource)
paths = [about_path]

if single_user_mode? && resource.is_a?(User)
paths << short_account_path(username: resource.account)
end

paths
end

def continue_after?
truthy_param?(:continue)
end

def restart_session
clear_attempt_from_session
redirect_to new_user_session_path, alert: I18n.t('devise.failure.timeout')
end

def set_attempt_session(user)
session[:attempt_user_id] = user.id
session[:attempt_user_updated_at] = user.updated_at.to_s
end

def clear_attempt_from_session
session.delete(:attempt_user_id)
session.delete(:attempt_user_updated_at)
end
end
16 changes: 9 additions & 7 deletions app/controllers/concerns/sign_in_token_authentication_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ def valid_sign_in_token_attempt?(user)
def authenticate_with_sign_in_token
user = self.resource = find_user

if user_params[:sign_in_token_attempt].present? && session[:attempt_user_id]
if user.present? && session[:attempt_user_id].present? && session[:attempt_user_updated_at] != user.updated_at.to_s
restart_session
elsif user_params.key?(:sign_in_token_attempt) && session[:attempt_user_id]
authenticate_with_sign_in_token_attempt(user)
elsif user.present? && user.external_or_valid_password?(user_params[:password])
prompt_for_sign_in_token(user)
Expand All @@ -27,7 +29,7 @@ def authenticate_with_sign_in_token

def authenticate_with_sign_in_token_attempt(user)
if valid_sign_in_token_attempt?(user)
session.delete(:attempt_user_id)
clear_attempt_from_session
remember_me(user)
sign_in(user)
else
Expand All @@ -42,10 +44,10 @@ def prompt_for_sign_in_token(user)
UserMailer.sign_in_token(user, request.remote_ip, request.user_agent, Time.now.utc.to_s).deliver_later!
end

set_locale do
session[:attempt_user_id] = user.id
@body_classes = 'lighter'
render :sign_in_token
end
set_attempt_session(user)

@body_classes = 'lighter'

set_locale { render :sign_in_token }
end
end
16 changes: 9 additions & 7 deletions app/controllers/concerns/two_factor_authentication_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ def valid_otp_attempt?(user)
def authenticate_with_two_factor
user = self.resource = find_user

if user_params[:otp_attempt].present? && session[:attempt_user_id]
if user.present? && session[:attempt_user_id].present? && session[:attempt_user_updated_at] != user.updated_at.to_s
restart_session
elsif user_params[:otp_attempt].present? && session[:attempt_user_id]
authenticate_with_two_factor_attempt(user)
elsif user.present? && user.external_or_valid_password?(user_params[:password])
prompt_for_two_factor(user)
Expand All @@ -30,7 +32,7 @@ def authenticate_with_two_factor

def authenticate_with_two_factor_attempt(user)
if valid_otp_attempt?(user)
session.delete(:attempt_user_id)
clear_attempt_from_session
remember_me(user)
sign_in(user)
else
Expand All @@ -40,10 +42,10 @@ def authenticate_with_two_factor_attempt(user)
end

def prompt_for_two_factor(user)
set_locale do
session[:attempt_user_id] = user.id
@body_classes = 'lighter'
render :two_factor
end
set_attempt_session(user)

@body_classes = 'lighter'

set_locale { render :two_factor }
end
end
7 changes: 3 additions & 4 deletions app/controllers/concerns/user_tracking_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,13 @@ module UserTrackingConcern
UPDATE_SIGN_IN_HOURS = 24

included do
before_action :set_user_activity
before_action :update_user_sign_in
end

private

def set_user_activity
return unless user_needs_sign_in_update?
current_user.update_tracked_fields!(request)
def update_user_sign_in
current_user.update_sign_in!(request) if user_needs_sign_in_update?
end

def user_needs_sign_in_update?
Expand Down
3 changes: 3 additions & 0 deletions app/lib/activitypub/activity/move.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ def perform

# Initiate a re-follow for each follower
MoveWorker.perform_async(origin_account.id, target_account.id)
rescue
unmark_as_processing!
raise
end

private
Expand Down
2 changes: 1 addition & 1 deletion app/models/account.rb
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,7 @@ def domains
end

def inboxes
urls = reorder(nil).where(protocol: :activitypub).pluck(Arel.sql("distinct coalesce(nullif(accounts.shared_inbox_url, ''), accounts.inbox_url)"))
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)
end

Expand Down
23 changes: 13 additions & 10 deletions app/models/account_stat.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,28 +21,26 @@ class AccountStat < ApplicationRecord

def increment_count!(key)
update(attributes_for_increment(key))
rescue ActiveRecord::StaleObjectError
rescue ActiveRecord::StaleObjectError, ActiveRecord::RecordNotUnique
begin
reload_with_id
rescue ActiveRecord::RecordNotFound
# Nothing to do
return
else
retry
end

retry
end

def decrement_count!(key)
update(key => [public_send(key) - 1, 0].max)
rescue ActiveRecord::StaleObjectError
update(attributes_for_decrement(key))
rescue ActiveRecord::StaleObjectError, ActiveRecord::RecordNotUnique
begin
reload_with_id
rescue ActiveRecord::RecordNotFound
# Nothing to do
return
else
retry
end

retry
end

private
Expand All @@ -53,8 +51,13 @@ def attributes_for_increment(key)
attrs
end

def attributes_for_decrement(key)
attrs = { key => [public_send(key) - 1, 0].max }
attrs
end

def reload_with_id
self.id = find_by!(account: account).id if new_record?
self.id = self.class.find_by!(account: account).id if new_record?
reload
end
end
2 changes: 1 addition & 1 deletion app/models/form/account_batch.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def block_domains!
end

def account_domains
accounts.pluck(Arel.sql('distinct domain')).compact
accounts.group(:domain).pluck(:domain).compact
end

def accounts
Expand Down
22 changes: 7 additions & 15 deletions app/models/tag.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,6 @@ class Tag < ApplicationRecord
has_and_belongs_to_many :accounts
has_and_belongs_to_many :sample_accounts, -> { local.discoverable.popular.limit(3) }, class_name: 'Account'

# Pawoo extension
has_one :suggestion_tag, dependent: :destroy

has_many :featured_tags, dependent: :destroy, inverse_of: :tag
has_one :account_tag_stat, dependent: :destroy

Expand All @@ -37,15 +34,13 @@ class Tag < ApplicationRecord

scope :reviewed, -> { where.not(reviewed_at: nil) }
scope :unreviewed, -> { where(reviewed_at: nil) }
# Pawoo extension: To prevent slow queries, extract frequently used tags within the last 1000 cases
scope :most_used, ->(account) { joins(:statuses).where(statuses: { id: account.statuses.select(:id).limit(1000) }).group(:id).order(Arel.sql('count(*) desc')) }
scope :pending_review, -> { unreviewed.where.not(requested_review_at: nil) }
scope :usable, -> { where(usable: [true, nil]) }
scope :listable, -> { where(listable: [true, nil]) }
scope :trendable, -> { Setting.trendable_by_default ? where(trendable: [true, nil]) : where(trendable: true) }
scope :discoverable, -> { listable.joins(:account_tag_stat).where(AccountTagStat.arel_table[:accounts_count].gt(0)).order(Arel.sql('account_tag_stats.accounts_count desc')) }
# Search with case-sensitive to use B-tree index.
scope :matches_name, ->(term) { where(arel_table[:name].lower.matches(sanitize_sql_like(normalize_and_downcase(term)) + '%', nil, true)) }
scope :most_used, ->(account) { joins(:statuses).where(statuses: { account: account }).group(:id).order(Arel.sql('count(*) desc')) }
scope :matches_name, ->(value) { where(arel_table[:name].matches("#{value}%")) }

delegate :accounts_count,
:accounts_count=,
Expand Down Expand Up @@ -131,9 +126,10 @@ def find_or_create_by_names(name_or_names)
end

def search_for(term, limit = 5, offset = 0, options = {})
striped_term = term.strip
query = Tag.listable.matches_name(striped_term)
query = query.merge(matching_name(striped_term).or(where.not(reviewed_at: nil))) if options[:exclude_unreviewed]
normalized_term = normalize(term.strip)
pattern = sanitize_sql_like(normalized_term) + '%'
query = Tag.listable.where(arel_table[:name].lower.matches(pattern))
query = query.where(arel_table[:name].lower.eq(normalized_term).or(arel_table[:reviewed_at].not_eq(nil))) if options[:exclude_unreviewed]

query.order(Arel.sql('length(name) ASC, name ASC'))
.limit(limit)
Expand All @@ -149,7 +145,7 @@ def find_normalized!(name)
end

def matching_name(name_or_names)
names = Array(name_or_names).map(&method(:normalize_and_downcase))
names = Array(name_or_names).map { |name| normalize(name).mb_chars.downcase.to_s }

if names.size == 1
where(arel_table[:name].lower.eq(names.first))
Expand All @@ -163,10 +159,6 @@ def matching_name(name_or_names)
def normalize(str)
str.gsub(/\A#/, '')
end

def normalize_and_downcase(str)
normalize(str).mb_chars.downcase.to_s
end
end

private
Expand Down
25 changes: 19 additions & 6 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ class User < ApplicationRecord
devise :two_factor_backupable,
otp_number_of_backup_codes: 10

devise :registerable, :recoverable, :rememberable, :trackable, :validatable,
devise :registerable, :recoverable, :rememberable, :validatable,
:confirmable

include Omniauthable
Expand Down Expand Up @@ -162,6 +162,24 @@ def confirm!
prepare_new_user! if new_user && approved?
end

def update_sign_in!(request, new_sign_in: false)
old_current, new_current = current_sign_in_at, Time.now.utc
self.last_sign_in_at = old_current || new_current
self.current_sign_in_at = new_current

old_current, new_current = current_sign_in_ip, request.remote_ip
self.last_sign_in_ip = old_current || new_current
self.current_sign_in_ip = new_current

if new_sign_in
self.sign_in_count ||= 0
self.sign_in_count += 1
end

save(validate: false) unless new_record?
prepare_returning_user!
end

def pending?
!approved?
end
Expand Down Expand Up @@ -193,11 +211,6 @@ def approve!
prepare_new_user!
end

def update_tracked_fields!(request)
super
prepare_returning_user!
end

def disable_two_factor!
self.otp_required_for_login = false
otp_backup_codes&.clear
Expand Down

0 comments on commit 9a45ab5

Please sign in to comment.