From 8bf4dc18ef4ae0694b7a57891829d40bf8db57a1 Mon Sep 17 00:00:00 2001 From: Claire Date: Thu, 16 Jul 2026 16:28:06 +0200 Subject: [PATCH] Add support for remote accounts changing handles (#39785) --- .../activitypub/process_account_service.rb | 64 +++-- spec/lib/activitypub/activity/update_spec.rb | 63 ++++- .../process_account_service_spec.rb | 218 ++++++++++++++++++ 3 files changed, 325 insertions(+), 20 deletions(-) diff --git a/app/services/activitypub/process_account_service.rb b/app/services/activitypub/process_account_service.rb index ec44d6391b3..4e5597f6b43 100644 --- a/app/services/activitypub/process_account_service.rb +++ b/app/services/activitypub/process_account_service.rb @@ -18,6 +18,7 @@ class ActivityPub::ProcessAccountService < BaseService class Error < StandardError; end + # It is the caller's responsibility to check that `json` is indeed from the origin matching `json['id']` def call(json, request_id: nil, only_key: false, signed_with_known_key: false, account: nil, suppress_errors: true) raise Error, "Actor #{json['id']} has unsupported URI scheme" if unsupported_uri_scheme?(json['id']) raise Error, "Actor #{json['id']} has no inbox" if json['inbox'].blank? @@ -30,13 +31,7 @@ class ActivityPub::ProcessAccountService < BaseService @only_key = only_key @webfinger_verified = false - if @account.present? - # TODO: handle renaming - @username = account.username - @domain = account.domain - else - extract_username_and_domain! - end + extract_username_and_domain! @domain = TagManager.instance.normalize_domain(@domain) return if @account.nil? && domain_not_allowed?(@domain) @@ -47,14 +42,17 @@ class ActivityPub::ProcessAccountService < BaseService @request_id = request_id || "#{Time.now.utc.to_i}-#{@username}@#{@domain}" with_redis_lock("process_account:#{@uri}") do - if @only_key - # `only_key` is used to update an existing account known by its `uri`. - # Lookup by handle and new account creation do not make sense in this case. - @account ||= Account.remote.find_by(uri: @uri) - return if @account.nil? - else - @account ||= Account.find_remote(@username, @domain) - end + # Now that Mastodon supports renaming accounts, assume URI is the most + # stable/trustworthy identifier. + @account ||= Account.remote.find_by(uri: @uri) # rubocop:disable Rails/FindByOrAssignmentMemoization + + # `only_key` is used to update an existing account known by its `uri`. + # Lookup by handle and new account creation do not make sense in this case. + return if @account.nil? && @only_key + + # Allow accounts to change URIs if they keep the same handle + # (typically, losing database or switching ActivityPub server implementation) + @account ||= Account.find_remote(@username, @domain) if @webfinger_verified @old_public_keys = @account.present? ? (@account.keypairs.pluck(:public_key) + [@account.public_key.presence].compact) : [] @old_protocol = @account&.protocol @@ -70,16 +68,24 @@ class ActivityPub::ProcessAccountService < BaseService end create_account + elsif @webfinger_verified + # The user has potentially changed handle, update it + rename_account! end update_account process_tags + # NOTE: while this case is unlikely due to the `rename_account!` above, + # we do not have a uniqueness constraint on URI, so this still needs to run process_duplicate_accounts! if @webfinger_verified end after_protocol_change! if protocol_changed? + + # TODO: extend this to an identity change, and do this on `uri` change as well after_key_change! if all_public_keys_changed? && !signed_with_known_key + # TODO: maybe tie tombstones to specific keys? i.e. we don't need to keep tombstones if all keys changed clear_tombstones! if all_public_keys_changed? after_suspension_change! if suspension_changed? @@ -100,6 +106,23 @@ class ActivityPub::ProcessAccountService < BaseService private + def rename_account! + raise 'Attempting to rename an account without having verified its webfinger handle' unless @webfinger_verified + + begin + # This will be a no-op if the username and domain haven't changed + @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 + + # TODO: better handle this scenario, by e.g. renaming the user to something unique + # and scheduling re-discovery + + @account.restore_attributes([:username, :domain]) + end + 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`. @@ -113,13 +136,21 @@ class ActivityPub::ProcessAccountService < BaseService @domain = Addressable::URI.parse(@uri).normalized_host end - check_webfinger! unless @only_key + if @account.present? && @username == @account.username && @domain == @account.domain + # This is an existing account whose handle has not changed, skip webfinger + @webfinger_verified = true + else + # This is either a new account, or an account whose handle has changed + check_webfinger! unless @only_key + end end def check_webfinger! webfinger = Webfinger.new("acct:#{@username}@#{@domain}").perform confirmed_username, confirmed_domain = split_acct(webfinger.subject) + raise Error, "Unsupported username format in webfinger response for #{@username}@#{@domain}" unless Account::USERNAME_ONLY_RE.match?(confirmed_username) + if @username.casecmp(confirmed_username).zero? && @domain.casecmp(confirmed_domain).zero? raise Error, "Webfinger response for #{@username}@#{@domain} does not loop back to #{@uri}" if webfinger.self_link_href != @uri @@ -133,6 +164,7 @@ class ActivityPub::ProcessAccountService < BaseService raise Webfinger::RedirectError, "Too many webfinger redirects for URI #{@uri} (stopped at #{@username}@#{@domain})" unless confirmed_username.casecmp(@username).zero? && confirmed_domain.casecmp(@domain).zero? raise Error, "Webfinger response for #{@username}@#{@domain} does not loop back to #{@uri}" if webfinger.self_link_href != @uri + raise Error, "Unsupported username format in webfinger response for #{@username}@#{@domain}" unless Account::USERNAME_ONLY_RE.match?(@username) @webfinger_verified = true rescue Webfinger::RedirectError => e diff --git a/spec/lib/activitypub/activity/update_spec.rb b/spec/lib/activitypub/activity/update_spec.rb index c281d03a94a..2a19f273721 100644 --- a/spec/lib/activitypub/activity/update_spec.rb +++ b/spec/lib/activitypub/activity/update_spec.rb @@ -46,7 +46,7 @@ RSpec.describe ActivityPub::Activity::Update do type: 'Update', actor: sender.uri, object: actor_json, - }.with_indifferent_access + }.deep_stringify_keys end before do @@ -55,12 +55,67 @@ RSpec.describe ActivityPub::Activity::Update do stub_request(:get, actor_json[:following]).to_return(status: 404) stub_request(:get, actor_json[:featured]).to_return(status: 404) stub_request(:get, actor_json[:featuredTags]).to_return(status: 404) - - subject.perform end it 'updates profile' do - expect(sender.reload.display_name).to eq 'Totally modified now' + expect { subject.perform } + .to change { sender.reload.display_name }.to('Totally modified now') + end + + context 'when the actor changes username through preferredUsername' do + let(:actor_json) do + { + '@context': [ + 'https://www.w3.org/ns/activitystreams', + 'https://w3id.org/security/v1', + { + manuallyApprovesFollowers: 'as:manuallyApprovesFollowers', + toot: 'http://joinmastodon.org/ns#', + featured: { '@id': 'toot:featured', '@type': '@id' }, + featuredTags: { '@id': 'toot:featuredTags', '@type': '@id' }, + }, + ], + id: sender.uri, + type: 'Person', + following: 'https://example.com/users/dfsdf/following', + followers: 'https://example.com/users/dfsdf/followers', + inbox: sender.inbox_url, + outbox: sender.outbox_url, + featured: 'https://example.com/users/dfsdf/featured', + featuredTags: 'https://example.com/users/dfsdf/tags', + preferredUsername: 'new_username', + name: 'Totally modified now', + publicKey: { + id: "#{sender.uri}#main-key", + owner: sender.uri, + publicKeyPem: sender.public_key, + }, + } + end + + let(:webfinger) do + { + subject: 'acct:new_username@example.com', + links: [ + { + rel: 'self', + href: actor_json[:id], + type: 'application/activity+json', + }, + ], + } + end + + before do + stub_request(:get, 'https://example.com/.well-known/webfinger?resource=acct:new_username@example.com') + .to_return_json(status: 200, body: webfinger, headers: { 'Content-Type': 'application/jrd+json' }) + end + + it 'updates the profile with the new username' do + expect { subject.perform } + .to change { sender.reload.display_name }.to('Totally modified now') + .and change { sender.reload.username }.to('new_username') + end end end diff --git a/spec/services/activitypub/process_account_service_spec.rb b/spec/services/activitypub/process_account_service_spec.rb index 6232f03759b..57c7e745eed 100644 --- a/spec/services/activitypub/process_account_service_spec.rb +++ b/spec/services/activitypub/process_account_service_spec.rb @@ -387,6 +387,224 @@ RSpec.describe ActivityPub::ProcessAccountService do end end + context 'with an account that has changed URI but not handle (typically, losing Mastodon database)' do + let!(:account) { Fabricate(:remote_account, username: 'alice', domain: 'example.com', uri: 'https://example.com/users/alice') } + + let(:payload) do + { + id: 'https://example.com/ap/users/1234', + type: 'Actor', + inbox: 'https://example.com/ap/users/1234/inbox', + webfinger: 'alice@example.com', + preferredUsername: 'alice', + attachment: [ + { type: 'PropertyValue', name: 'Pronouns', value: 'They/them' }, + { type: 'PropertyValue', name: 'Occupation', value: 'Unit test' }, + ], + }.deep_stringify_keys + end + + before do + stub_webfinger! + end + + it 'properly updates the existing account, without creating a new one or calling AccountMergingWorker' do + expect { subject.call(payload) } + .to change { account.reload.uri }.from('https://example.com/users/alice').to('https://example.com/ap/users/1234') + .and not_change { account.reload.acct } + .and(not_change { Account.count }) + + expect(AccountMergingWorker) + .to_not have_enqueued_sidekiq_job(AccountMergingWorker) + + expect(account.fields) + .to be_an(Array) + .and have_attributes(size: 2) + expect(account.fields.first) + .to be_an(Account::Field) + .and have_attributes( + name: eq('Pronouns'), + value: eq('They/them') + ) + expect(account.fields.last) + .to be_an(Account::Field) + .and have_attributes( + name: eq('Occupation'), + value: eq('Unit test') + ) + end + end + + context 'with an account that has changed names through `webfinger` property' do + let!(:account) { Fabricate(:remote_account, username: 'bob', domain: 'foo.test', uri: 'https://foo.test', inbox_url: 'https://foo.test/inbox') } + + let(:payload) do + { + id: 'https://foo.test', + type: 'Actor', + inbox: 'https://foo.test/inbox', + webfinger: 'alice@example.com', + attachment: [ + { type: 'PropertyValue', name: 'Pronouns', value: 'They/them' }, + { type: 'PropertyValue', name: 'Occupation', value: 'Unit test' }, + ], + }.deep_stringify_keys + end + + before do + stub_webfinger! + webfinger = { + subject: 'acct:alice@example.com', + links: [ + { + rel: 'self', + href: 'https://foo.test', + type: 'application/activity+json', + }, + ], + }.deep_stringify_keys + stub_request(:get, 'https://example.com/.well-known/webfinger?resource=acct:alice@example.com').to_return(body: webfinger.to_json, headers: { 'Content-Type': 'application/jrd+json' }) + end + + it 'parses property values, avatar and profile header as expected, updates account username without creating a new one or calling AccountMergingWorker' do + expect { subject.call(payload) } + .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 { Account.count }) + + expect(AccountMergingWorker) + .to_not have_enqueued_sidekiq_job(AccountMergingWorker) + + expect(account.fields) + .to be_an(Array) + .and have_attributes(size: 2) + expect(account.fields.first) + .to be_an(Account::Field) + .and have_attributes( + name: eq('Pronouns'), + value: eq('They/them') + ) + expect(account.fields.last) + .to be_an(Account::Field) + .and have_attributes( + name: eq('Occupation'), + value: eq('Unit test') + ) + end + + 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 + expect { subject.call(payload) } + .to not_change { account.reload.username } + .and not_change { account.reload.domain } + .and not_change { account.reload.uri } + .and not_change { conflicting_account.reload.acct } + .and(not_change { Account.count }) + + expect(AccountMergingWorker) + .to_not have_enqueued_sidekiq_job + + expect(account.fields) + .to be_an(Array) + .and have_attributes(size: 2) + expect(account.fields.first) + .to be_an(Account::Field) + .and have_attributes( + name: eq('Pronouns'), + value: eq('They/them') + ) + expect(account.fields.last) + .to be_an(Account::Field) + .and have_attributes( + name: eq('Occupation'), + value: eq('Unit test') + ) + end + end + end + + context 'with an account that has changed names and domain through `preferredUsername` property' do + let!(:account) { Fabricate(:remote_account, username: 'bob', domain: 'foo.test', uri: 'https://foo.test', inbox_url: 'https://foo.test/inbox') } + + let(:payload) do + { + id: 'https://foo.test', + type: 'Actor', + inbox: 'https://foo.test/inbox', + preferredUsername: 'alice', + attachment: [ + { type: 'PropertyValue', name: 'Pronouns', value: 'They/them' }, + { type: 'PropertyValue', name: 'Occupation', value: 'Unit test' }, + ], + }.deep_stringify_keys + end + + before do + stub_webfinger! + end + + it 'parses property values, avatar and profile header as expected, updates account username without creating a new one or calling AccountMergingWorker' do + expect { subject.call(payload) } + .to change { account.reload.username }.from('bob').to('alice') + .and not_change { account.reload.uri } + .and(not_change { Account.count }) + + expect(AccountMergingWorker) + .to_not have_enqueued_sidekiq_job + + expect(account.fields) + .to be_an(Array) + .and have_attributes(size: 2) + expect(account.fields.first) + .to be_an(Account::Field) + .and have_attributes( + name: eq('Pronouns'), + value: eq('They/them') + ) + expect(account.fields.last) + .to be_an(Account::Field) + .and have_attributes( + name: eq('Occupation'), + value: eq('Unit test') + ) + end + + 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 + expect { subject.call(payload) } + .to not_change { account.reload.username } + .and not_change { account.reload.domain } + .and not_change { account.reload.uri } + .and not_change { conflicting_account.reload.acct } + .and(not_change { Account.count }) + + expect(AccountMergingWorker) + .to_not have_enqueued_sidekiq_job + + expect(account.fields) + .to be_an(Array) + .and have_attributes(size: 2) + expect(account.fields.first) + .to be_an(Account::Field) + .and have_attributes( + name: eq('Pronouns'), + value: eq('They/them') + ) + expect(account.fields.last) + .to be_an(Account::Field) + .and have_attributes( + name: eq('Occupation'), + value: eq('Unit test') + ) + end + end + end + context 'with attribution domains' do let(:payload) do {