Simplify invalidated usernames and expose them as :id@handle.invalid to the REST API

app/models/account.rb

@@ -237,7 +237,17 @@ class Account < ApplicationRecord
     local? ? username : "#{username}@#{domain}"
   end
 
+  def pretty_username
+    # Return special username for user-facing invalid handle accounts
+    return id.to_s if invalidated_username?
+
+    username
+  end
+
   def pretty_acct
+    # Return special handle for user-facing invalid handle accounts
+    return "#{id}@handle.invalid" if invalidated_username?
+
     local? ? username : "#{username}@#{Addressable::IDNA.to_unicode(domain)}"
   end
 
@@ -254,17 +264,20 @@ class Account < ApplicationRecord
   end
 
   def invalidate_username!
+    raise ArgumentError if local?
     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.
+    # It is very unlikely that we will allow `!` in usernames in the future,
+    # and we will never allow ` ` in them either, so this ensure this will never
+    # match a valid username on a remote server.
+
     # Using the local ID ensures we won't have any conflict.
 
-    update_attribute(:username, "{invalid!#{id}}")
+    update_attribute(:username, "! #{id}")
   end
 
   def invalidated_username?
-    username.start_with?('{invalid!')
+    username.start_with?('! ')
   end
 
   def possibly_stale?

app/models/concerns/account/finder_concern.rb

@@ -25,6 +25,8 @@ module Account::FinderConcern
     end
 
     def find_remote(username, domain)
+      return if domain == 'handle.invalid'
+
       Account
         .with_username(username)
         .with_domain(domain)

app/serializers/rest/account_serializer.rb

@@ -154,6 +154,10 @@ class REST::AccountSerializer < ActiveModel::Serializer
     object.memorial?
   end
 
+  def username
+    object.pretty_username
+  end
+
   def invalid_handle
     object.invalidated_username?
   end

app/serializers/rest/announcement_serializer.rb

@@ -41,6 +41,10 @@ class REST::AnnouncementSerializer < ActiveModel::Serializer
       object.id.to_s
     end
 
+    def username
+      object.pretty_username
+    end
+
     def url
       ActivityPub::TagManager.instance.url_for(object)
     end

app/serializers/rest/status_serializer.rb

@@ -201,7 +201,7 @@ class REST::StatusSerializer < ActiveModel::Serializer
     end
 
     def username
-      object.account_username
+      object.account.pretty_username
     end
 
     def url

app/services/resolve_account_service.rb

@@ -25,7 +25,7 @@ class ResolveAccountService < BaseService
     # 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
 
-    return if domain_not_allowed?(@domain)
+    return if domain_not_allowed?(@domain) || @domain == 'handle.invalid'
 
     @account ||= Account.find_remote(@username, @domain)
 

spec/services/activitypub/process_account_service_spec.rb

@@ -501,7 +501,7 @@ RSpec.describe ActivityPub::ProcessAccountService do
           .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 change { conflicting_account.reload.username }.from('alice').to("{invalid!#{conflicting_account.id}}")
+          .and change { conflicting_account.reload.username }.from('alice').to("! #{conflicting_account.id}")
           .and(not_change { Account.count })
 
         expect(AccountRefreshWorker)
@@ -583,7 +583,7 @@ RSpec.describe ActivityPub::ProcessAccountService do
           .to change { account.reload.username }.from('bob').to('alice')
           .and not_change { account.reload.domain }
           .and not_change { account.reload.uri }
-          .and change { conflicting_account.reload.username }.from('alice').to("{invalid!#{conflicting_account.id}}")
+          .and change { conflicting_account.reload.username }.from('alice').to("! #{conflicting_account.id}")
           .and(not_change { Account.count })
 
         expect(AccountRefreshWorker)