Add support for remote accounts changing handles (#39785)

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

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
 

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
       {