From 6f88aede9ea2353cfe3a06f3c97d4f2318b906dc Mon Sep 17 00:00:00 2001 From: Claire Date: Thu, 16 Jul 2026 11:51:18 +0200 Subject: [PATCH] Change handling of handle conflicts to invalidate known-outdated account handle --- app/models/account.rb | 18 +++++++++++++++-- .../activitypub/process_account_service.rb | 16 +++++++++++---- app/services/resolve_account_service.rb | 3 +++ app/workers/account_refresh_worker.rb | 4 ++-- .../process_account_service_spec.rb | 20 ++++++++++++------- 5 files changed, 46 insertions(+), 15 deletions(-) diff --git a/app/models/account.rb b/app/models/account.rb index 9d5c4a78e09..a86778f5971 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -253,14 +253,28 @@ class Account < ApplicationRecord "acct:#{local_username_and_domain}" end + def invalidate_username! + return if invalidated_username? + + # This uses some characters (`{`, `}`, `!`) that we are extremely unlikely to allow + # i nthe future, in order to ensure this will never match a webfinger handle. + # Using the local ID ensures we won't have any conflict. + + update_attribute(:username, "{invalid!#{id}}") + end + + def invalidated_username? + username.start_with?('{invalid!') + end + def possibly_stale? - last_webfingered_at.nil? || last_webfingered_at <= STALE_THRESHOLD.ago + last_webfingered_at.nil? || last_webfingered_at <= STALE_THRESHOLD.ago || invalidated_username? end def needs_background_refresh? return false if local? - return true if last_webfingered_at.blank? || last_webfingered_at <= BACKGROUND_REFRESH_INTERVAL.ago + return true if last_webfingered_at.blank? || last_webfingered_at <= BACKGROUND_REFRESH_INTERVAL.ago || invalidated_username? # TODO: Remove some time after 4.6 # This is temporary workaround to speed up account refreshs after diff --git a/app/services/activitypub/process_account_service.rb b/app/services/activitypub/process_account_service.rb index 4e5597f6b43..2b28f4692d2 100644 --- a/app/services/activitypub/process_account_service.rb +++ b/app/services/activitypub/process_account_service.rb @@ -114,15 +114,23 @@ class ActivityPub::ProcessAccountService < BaseService @account.update!(username: @username, domain: @domain) rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotUnique # This account (identified by ActivityPub `id`) is being renamed to a handle that was - # previously known by this Mastodon server as a different account… ignore the renaming for now + # previously known by this Mastodon server as a different account… - # TODO: better handle this scenario, by e.g. renaming the user to something unique - # and scheduling re-discovery + rename_conflicting_account! - @account.restore_attributes([:username, :domain]) + retry end end + def rename_conflicting_account! + conflicting_account = Account.find_remote(@username, @domain) + return if conflicting_account.nil? || conflicting_account.local? || conflicting_account.uri == @account.uri + + conflicting_account.invalidate_username! + + AccountRefreshWorker.perform_async(conflicting_account.id, { 'request_id' => @request_id }) + end + def extract_username_and_domain! # FEP-2c59 defines a `webfinger` attribute that makes things more explicit and spares an extra request in some cases. # It supersedes `preferredUsername`. diff --git a/app/services/resolve_account_service.rb b/app/services/resolve_account_service.rb index cd96b55c740..3da9a8ab772 100644 --- a/app/services/resolve_account_service.rb +++ b/app/services/resolve_account_service.rb @@ -13,12 +13,15 @@ class ResolveAccountService < BaseService # @option options [Boolean] :skip_webfinger Do not attempt any webfinger query or refreshing account data # @option options [Boolean] :skip_cache Get the latest data from origin even if cache is not due to update yet # @option options [Boolean] :suppress_errors When failing, return nil instead of raising an error + # @option options [String] :request_id Used to limit the number of HTTP requests issued from a single outside request # @return [Account] def call(uri, options = {}) return if uri.blank? process_options!(uri, options) + return ActivityPub::FetchRemoteAccountService.new.call(@account.uri, suppress_errors: @options[:suppress_errors], request_id: options[:request_id]) if @account&.remote? && @account.invalidated_username? + # First of all we want to check if we've got the account # record with the URI already, and if so, we can exit early diff --git a/app/workers/account_refresh_worker.rb b/app/workers/account_refresh_worker.rb index b129a14226f..15b9ba14730 100644 --- a/app/workers/account_refresh_worker.rb +++ b/app/workers/account_refresh_worker.rb @@ -5,10 +5,10 @@ class AccountRefreshWorker sidekiq_options queue: 'pull', retry: 3, dead: false, lock: :until_executed, lock_ttl: 1.day.to_i - def perform(account_id) + def perform(account_id, options = {}) account = Account.find_by(id: account_id) return unless account&.needs_background_refresh? - ResolveAccountService.new.call(account) + ResolveAccountService.new.call(account, { request_id: options['request_id'] }) end end diff --git a/spec/services/activitypub/process_account_service_spec.rb b/spec/services/activitypub/process_account_service_spec.rb index 57c7e745eed..ebfbbcd0168 100644 --- a/spec/services/activitypub/process_account_service_spec.rb +++ b/spec/services/activitypub/process_account_service_spec.rb @@ -496,14 +496,17 @@ RSpec.describe ActivityPub::ProcessAccountService do context 'when the destination handle is already occupied' do let!(:conflicting_account) { Fabricate(:remote_account, username: 'alice', domain: 'example.com', uri: 'https://foo.test/original_alice', inbox_url: 'https://foo.test/original_alice/inbox') } - it 'updates the profile but does not touch the usernames or call AccountMergingWorker' do + it 'updates the profile without creating a new one or calling AccountMergingWorker, renames conflicting and schedules refresh' do expect { subject.call(payload) } - .to not_change { account.reload.username } - .and not_change { account.reload.domain } + .to change { account.reload.username }.from('bob').to('alice') + .and change { account.reload.domain }.from('foo.test').to('example.com') .and not_change { account.reload.uri } - .and not_change { conflicting_account.reload.acct } + .and change { conflicting_account.reload.username }.from('alice').to("{invalid!#{conflicting_account.id}}") .and(not_change { Account.count }) + expect(AccountRefreshWorker) + .to have_enqueued_sidekiq_job(conflicting_account.id, { 'request_id' => /.*-.*@.*/ }) + expect(AccountMergingWorker) .to_not have_enqueued_sidekiq_job @@ -575,14 +578,17 @@ RSpec.describe ActivityPub::ProcessAccountService do context 'when the destination handle is already occupied' do let!(:conflicting_account) { Fabricate(:remote_account, username: 'alice', domain: 'foo.test', uri: 'https://foo.test/original_alice', inbox_url: 'https://foo.test/original_alice/inbox') } - it 'updates the profile but does not touch the usernames or call AccountMergingWorker' do + it 'updates the profile without creating a new one or calling AccountMergingWorker, renames conflicting and schedules refresh' do expect { subject.call(payload) } - .to not_change { account.reload.username } + .to change { account.reload.username }.from('bob').to('alice') .and not_change { account.reload.domain } .and not_change { account.reload.uri } - .and not_change { conflicting_account.reload.acct } + .and change { conflicting_account.reload.username }.from('alice').to("{invalid!#{conflicting_account.id}}") .and(not_change { Account.count }) + expect(AccountRefreshWorker) + .to have_enqueued_sidekiq_job(conflicting_account.id, { 'request_id' => /.*-.*@.*/ }) + expect(AccountMergingWorker) .to_not have_enqueued_sidekiq_job