diff --git a/app/models/account.rb b/app/models/account.rb index 9d5c4a78e09..2203c96ca6a 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -54,7 +54,7 @@ # suspended_at :datetime # suspension_origin :integer # trendable :boolean -# uri :string default(""), not null +# uri :string # url :string # username :string default(""), not null # created_at :datetime not null @@ -122,7 +122,7 @@ class Account < ApplicationRecord validates :username, format: { with: USERNAME_ONLY_RE }, length: { maximum: USERNAME_LENGTH_HARD_LIMIT }, if: -> { (remote? || actor_type_application?) && will_save_change_to_username? } # Remote user validations - validates :uri, presence: true, unless: :local?, on: :create + validates :uri, presence: true, exclusion: { in: [''] }, uniqueness: true, unless: :local?, on: :create # Local user validations validates :username, format: { with: /\A[a-z0-9_]+\z/i }, length: { maximum: USERNAME_LENGTH_LIMIT }, if: -> { local? && will_save_change_to_username? && !actor_type_application? } @@ -136,7 +136,7 @@ class Account < ApplicationRecord validates :following_url, absence: true validates :inbox_url, absence: true validates :shared_inbox_url, absence: true - validates :uri, absence: true + validates :uri, absence: true, exclusion: { in: [''] } end validates :domain, exclusion: { in: [''] } diff --git a/db/migrate/20260720090737_change_account_uri_nullable.rb b/db/migrate/20260720090737_change_account_uri_nullable.rb new file mode 100644 index 00000000000..90fd8989275 --- /dev/null +++ b/db/migrate/20260720090737_change_account_uri_nullable.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class ChangeAccountUriNullable < ActiveRecord::Migration[8.1] + disable_ddl_transaction! + + class Account < ApplicationRecord; end + + def up + change_column_null :accounts, :uri, true + end + + def down + change_column_null :accounts, :uri, false + rescue ActiveRecord::NotNullViolation + Account.where(uri: nil).update_all(uri: '') + + retry + end +end diff --git a/db/migrate/20260720092724_change_account_uri_default.rb b/db/migrate/20260720092724_change_account_uri_default.rb new file mode 100644 index 00000000000..576a19478ec --- /dev/null +++ b/db/migrate/20260720092724_change_account_uri_default.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class ChangeAccountUriDefault < ActiveRecord::Migration[8.1] + def change + change_column_default :accounts, :uri, from: '', to: nil + end +end diff --git a/db/post_migrate/20260720100326_fix_blank_account_uri.rb b/db/post_migrate/20260720100326_fix_blank_account_uri.rb new file mode 100644 index 00000000000..451eede15ae --- /dev/null +++ b/db/post_migrate/20260720100326_fix_blank_account_uri.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class FixBlankAccountUri < ActiveRecord::Migration[8.1] + # Dummy classes to make migration possible across version changes + class Account < ApplicationRecord; end + + def up + Account.where(uri: '').in_batches.update_all(uri: nil) + end + + def down + Account.where(uri: nil).in_batches.update_all(uri: '') + end +end diff --git a/db/post_migrate/20260720103819_rename_index_accounts_on_uri_to_old.rb b/db/post_migrate/20260720103819_rename_index_accounts_on_uri_to_old.rb new file mode 100644 index 00000000000..2c34a7058f8 --- /dev/null +++ b/db/post_migrate/20260720103819_rename_index_accounts_on_uri_to_old.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class RenameIndexAccountsOnUriToOld < ActiveRecord::Migration[8.1] + def change + rename_index :accounts, :index_accounts_on_uri, :old_index_accounts_on_uri + end +end diff --git a/db/post_migrate/20260720104058_add_unique_index_on_accounts_uri.rb b/db/post_migrate/20260720104058_add_unique_index_on_accounts_uri.rb new file mode 100644 index 00000000000..155b7bc470f --- /dev/null +++ b/db/post_migrate/20260720104058_add_unique_index_on_accounts_uri.rb @@ -0,0 +1,115 @@ +# frozen_string_literal: true + +class AddUniqueIndexOnAccountsUri < ActiveRecord::Migration[8.1] + disable_ddl_transaction! + + # Dummy classes to make migration possible across version changes + class Account < ApplicationRecord; end + class AccountDeletionRequest < ApplicationRecord; end + class AccountModerationNote < ApplicationRecord; end + class AccountNote < ApplicationRecord; end + class AccountPin < ApplicationRecord; end + class AccountStat < ApplicationRecord; end + class Appeal < ApplicationRecord; end + class Block < ApplicationRecord; end + class CanonicalEmailBlock < ApplicationRecord; end + class Collection < ApplicationRecord; end + class CollectionItem < ApplicationRecord; end + class Favourite < ApplicationRecord; end + class Follow < ApplicationRecord; end + class FollowRecommendationSuppression < ApplicationRecord; end + class FollowRequest < ApplicationRecord; end + class ListAccount < ApplicationRecord; end + + class MediaAttachment < ApplicationRecord + self.inheritance_column = nil + end + + class Mention < ApplicationRecord; end + class Mute < ApplicationRecord; end + + class Notification < ApplicationRecord + self.inheritance_column = nil + end + + class NotificationPermission < ApplicationRecord; end + class NotificationRequest < ApplicationRecord; end + class Poll < ApplicationRecord; end + class PollVote < ApplicationRecord; end + class Quote < ApplicationRecord; end + class Report < ApplicationRecord; end + class SeveredRelationship < ApplicationRecord; end + class Status < ApplicationRecord; end + class StatusPin < ApplicationRecord; end + class TagFollow < ApplicationRecord; end + class Tombstone < ApplicationRecord; end + + def up + add_index :accounts, :uri, algorithm: :concurrently, unique: true + rescue ActiveRecord::RecordNotUnique + deduplicate_and_reindex! + rescue + remove_index :accounts, name: :index_accounts_on_uri + raise + end + + def down + remove_index :accounts, name: :index_accounts_on_uri + end + + private + + def deduplicate_and_reindex! + deduplicate_accounts! + + safety_assured { execute 'REINDEX INDEX CONCURRENTLY index_accounts_on_uri' } + rescue ActiveRecord::RecordNotUnique + retry + end + + def deduplicate_accounts! + duplicate_uris = select_all('SELECT uri FROM accounts WHERE uri IS NOT NULL GROUP BY uri HAVING count(*) > 1').rows + + duplicate_uris.each do |uri| + # Fetch the conflicting accounts and keep the most recently-discovered one as reference + duplicate_records = Account.where(uri: uri).reorder(Arel.sql("COALESCE(last_webfingered_at, 'epoch'::date)"), :created_at, :id).to_a + reference_account = duplicate_records.pop + + duplicate_records.each do |other_account| + merge_accounts!(reference_account, other_account) + other_account.destroy + end + end + end + + def merge_accounts!(account, other_account) + { + account_id: [ + Status, StatusPin, MediaAttachment, Poll, Report, Tombstone, Favourite, + Follow, FollowRequest, Block, Mute, + AccountModerationNote, AccountPin, AccountStat, ListAccount, + PollVote, Mention, AccountDeletionRequest, AccountNote, FollowRecommendationSuppression, + Appeal, TagFollow, Quote, Collection, CollectionItem + ], + from_account_id: [ + Notification, NotificationPermission, NotificationRequest + ], + target_account_id: [ + Follow, FollowRequest, Block, Mute, AccountModerationNote, AccountPin, AccountNote + ], + reference_account_id: [CanonicalEmailBlock], + account_warning_id: [Appeal], + local_account_id: [SeveredRelationship], + remote_account_id: [SeveredRelationship], + quoted_account_id: [Quote], + }.each do |attribute, classes| + classes.each do |klass| + klass.where({ attribute => other_account.id }).reorder(nil).find_each do |record| + record.update_attribute(attribute, account.id) + rescue ActiveRecord::RecordNotUnique + next + end + end + end + end +end diff --git a/db/post_migrate/20260720113713_remove_old_index_on_accounts_uri.rb b/db/post_migrate/20260720113713_remove_old_index_on_accounts_uri.rb new file mode 100644 index 00000000000..e91b9c067d0 --- /dev/null +++ b/db/post_migrate/20260720113713_remove_old_index_on_accounts_uri.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class RemoveOldIndexOnAccountsUri < ActiveRecord::Migration[8.1] + disable_ddl_transaction! + + def change + remove_index :accounts, :uri, name: :old_index_accounts_on_uri, algorithm: :concurrently + end +end diff --git a/db/post_migrate/20260720124731_clean_up_invalid_accounts.rb b/db/post_migrate/20260720124731_clean_up_invalid_accounts.rb new file mode 100644 index 00000000000..14af030d73f --- /dev/null +++ b/db/post_migrate/20260720124731_clean_up_invalid_accounts.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class CleanUpInvalidAccounts < ActiveRecord::Migration[8.1] + # Dummy classes to make migration possible across version changes + class Account < ApplicationRecord; end + + def up + # A very old bug could cause incompletely-processed remote accounts to be added + # to the database; those would not have a URI, which could be an issue in the + # future. Delete them. + Account.where.not(domain: nil).where(uri: nil).in_batches.delete_all + end + + def down; end +end diff --git a/db/schema.rb b/db/schema.rb index ecf113318dc..4062dba3531 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_07_06_143100) do +ActiveRecord::Schema[8.1].define(version: 2026_07_20_124731) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" @@ -204,14 +204,14 @@ ActiveRecord::Schema[8.1].define(version: 2026_07_06_143100) do t.integer "suspension_origin" t.boolean "trendable" t.datetime "updated_at", precision: nil, null: false - t.string "uri", default: "", null: false + t.string "uri" t.string "url" t.string "username", default: "", null: false t.index "(((setweight(to_tsvector('simple'::regconfig, (display_name)::text), 'A'::\"char\") || setweight(to_tsvector('simple'::regconfig, (username)::text), 'B'::\"char\")) || setweight(to_tsvector('simple'::regconfig, (COALESCE(domain, ''::character varying))::text), 'C'::\"char\")))", name: "search_index", using: :gin t.index "lower((username)::text), COALESCE(lower((domain)::text), ''::text)", name: "index_accounts_on_username_and_domain_lower", unique: true t.index ["domain", "id"], name: "index_accounts_on_domain_and_id" t.index ["moved_to_account_id"], name: "index_accounts_on_moved_to_account_id", where: "(moved_to_account_id IS NOT NULL)" - t.index ["uri"], name: "index_accounts_on_uri" + t.index ["uri"], name: "index_accounts_on_uri", unique: true t.index ["url"], name: "index_accounts_on_url", opclass: :text_pattern_ops, where: "(url IS NOT NULL)" end diff --git a/lib/mastodon/cli/accounts.rb b/lib/mastodon/cli/accounts.rb index 9f3fda27062..7f15a3686ed 100644 --- a/lib/mastodon/cli/accounts.rb +++ b/lib/mastodon/cli/accounts.rb @@ -253,20 +253,11 @@ module Mastodon::CLI desc 'fix-duplicates', 'Find duplicate remote accounts and merge them' option :dry_run, type: :boolean long_desc <<-LONG_DESC - Merge known remote accounts sharing an ActivityPub actor identifier. - - Such duplicates can occur when a remote server admin misconfigures their - domain configuration. + This command is deprecated as of Mastodon v4.7.0. LONG_DESC def fix_duplicates - Account.remote.duplicate_uris.pluck(:uri).each do |uri| - say("Duplicates found for #{uri}") - begin - ActivityPub::FetchRemoteAccountService.new.call(uri) unless dry_run? - rescue => e - say("Error processing #{uri}: #{e}", :red) - end - end + # TODO: remove this after 4.7.0 + say('This command is deprecated as Mastodon v4.7.0 migrations enforce ActivityPub actor identifier uniqueness', :yellow) end desc 'backup USERNAME', 'Request a backup for a user' diff --git a/lib/tasks/tests.rake b/lib/tasks/tests.rake index 48d2233c938..73fa2dfd228 100644 --- a/lib/tasks/tests.rake +++ b/lib/tasks/tests.rake @@ -164,11 +164,23 @@ namespace :tests do exit(1) end + unless Account.find_by(uri: 'https://social.example.com/alice').domain == 'example.com' + puts 'Duplicate accounts with same URI incorrectly deduplicated' + exit(1) + end + + if Account.exists?(domain: 'example.com', username: %w(bogus1 bogus2 bogus3)) + puts 'Bogus remote accounts not correctly purged' + exit(1) + end + puts 'No errors found. Database state is consistent with a successful migration process.' end desc 'Populate the database with test data for 3.3.0' task populate_v3_3_0: :environment do # rubocop:disable Naming/VariableNumber + user_public_key = ActiveRecord::Base.connection.quote(OpenSSL::PKey::RSA.new(2048).public_key.to_pem) + ActiveRecord::Base.connection.execute(<<~SQL.squish) INSERT INTO "webauthn_credentials" (user_id, nickname, external_id, public_key, created_at, updated_at) @@ -206,6 +218,19 @@ namespace :tests do (token, application_id, scopes, resource_owner_id, created_at) VALUES ('secret', 2, 'write:accounts read:me', 4, now()); + + /* Duplicate remote accounts + Before Mastodon v4.7.0, we had no uniqueness constraint on account `uri`, which lead to issues. + Furthermore, older tests caused remote accounts without protocol identifiers. + */ + INSERT INTO "accounts" + (id, username, domain, uri, private_key, public_key, created_at, updated_at, last_webfingered_at) + VALUES + (12, 'alice', 'social.example.com', 'https://social.example.com/alice', NULL, #{user_public_key}, '2021-01-01'::date, now(), '2021-01-09'::date), + (13, 'alice', 'example.com', 'https://social.example.com/alice', NULL, #{user_public_key}, '2021-01-10'::date, now(), '2021-01-11'::date), + (14, 'bogus1', 'example.com', '', NULL, '', now(), now(), now()), + (15, 'bogus2', 'example.com', '', NULL, '', now(), now(), now()), + (16, 'bogus3', 'example.com', '', NULL, '', now(), now(), now()); SQL end @@ -336,19 +361,19 @@ namespace :tests do (2, 'user', NULL, #{user_private_key}, #{user_public_key}, now(), now()); INSERT INTO "accounts" - (id, username, domain, private_key, public_key, created_at, updated_at, remote_url, salmon_url) + (id, username, domain, uri, private_key, public_key, created_at, updated_at, remote_url, salmon_url) VALUES - (3, 'remote', 'remote.com', NULL, #{remote_public_key}, now(), now(), + (3, 'remote', 'remote.com', 'https://remote.com/remote', NULL, #{remote_public_key}, now(), now(), 'https://remote.com/@remote', 'https://remote.com/salmon/1'), - (4, 'Remote', 'remote.com', NULL, #{remote_public_key}, now(), now(), + (4, 'Remote', 'remote.com', 'https://remote.com/Remote', NULL, #{remote_public_key}, now(), now(), 'https://remote.com/@Remote', 'https://remote.com/salmon/1'), - (5, 'REMOTE', 'Remote.com', NULL, #{remote_public_key2}, now() - interval '1 year', now() - interval '1 year', + (5, 'REMOTE', 'Remote.com', 'https://remote.com/stale/remote', NULL, #{remote_public_key2}, now() - interval '1 year', now() - interval '1 year', 'https://remote.com/stale/@REMOTE', 'https://remote.com/stale/salmon/1'); INSERT INTO "accounts" - (id, username, domain, private_key, public_key, created_at, updated_at, protocol, inbox_url, outbox_url, followers_url) + (id, username, domain, uri, private_key, public_key, created_at, updated_at, protocol, inbox_url, outbox_url, followers_url) VALUES - (6, 'bob', 'ActivityPub.com', NULL, #{remote_public_key_ap}, now(), now(), + (6, 'bob', 'ActivityPub.com', 'https://activitypub.com/users/bob', NULL, #{remote_public_key_ap}, now(), now(), 1, 'https://activitypub.com/users/bob/inbox', 'https://activitypub.com/users/bob/outbox', 'https://activitypub.com/users/bob/followers'); INSERT INTO "accounts" @@ -358,10 +383,11 @@ namespace :tests do (8, 'pt_user', NULL, #{user_private_key}, #{user_public_key}, now(), now()); INSERT INTO "accounts" - (id, username, domain, private_key, public_key, created_at, updated_at, protocol, inbox_url, outbox_url, followers_url, suspended) + (id, username, domain, uri, private_key, public_key, created_at, updated_at, protocol, inbox_url, outbox_url, followers_url, suspended) VALUES - (9, 'evil', 'activitypub.com', NULL, #{remote_public_key_ap}, now(), now(), - 1, 'https://activitypub.com/users/evil/inbox', 'https://activitypub.com/users/evil/outbox', + (9, 'evil', 'activitypub.com', 'https://activitypub.com/users/evil', NULL, + #{remote_public_key_ap}, now(), now(), 1, + 'https://activitypub.com/users/evil/inbox', 'https://activitypub.com/users/evil/outbox', 'https://activitypub.com/users/evil/followers', true); -- users diff --git a/spec/fabricators/account_fabricator.rb b/spec/fabricators/account_fabricator.rb index 9e10a0d64e2..4280b7f7d69 100644 --- a/spec/fabricators/account_fabricator.rb +++ b/spec/fabricators/account_fabricator.rb @@ -11,7 +11,7 @@ Fabricator(:account) do suspended_at { |attrs| attrs[:suspended] ? Time.now.utc : nil } silenced_at { |attrs| attrs[:silenced] ? Time.now.utc : nil } user { |attrs| attrs[:domain].nil? ? Fabricate.build(:user, account: nil) : nil } - uri { |attrs| attrs[:domain].nil? ? '' : "https://#{attrs[:domain]}/users/#{attrs[:username]}" } + uri { |attrs| attrs[:domain].nil? ? nil : "https://#{attrs[:domain]}/users/#{attrs[:username]}" } discoverable true indexable true diff --git a/spec/lib/mastodon/cli/accounts_spec.rb b/spec/lib/mastodon/cli/accounts_spec.rb index 73f5b83744c..33f5d802f40 100644 --- a/spec/lib/mastodon/cli/accounts_spec.rb +++ b/spec/lib/mastodon/cli/accounts_spec.rb @@ -648,25 +648,6 @@ RSpec.describe Mastodon::CLI::Accounts do end end - describe '#fix_duplicates' do - let(:action) { :fix_duplicates } - let(:service_double) { instance_double(ActivityPub::FetchRemoteAccountService, call: nil) } - let(:uri) { 'https://host.example/same/value' } - - context 'when there are duplicate URI accounts' do - before do - Fabricate.times(2, :account, domain: 'host.example', uri: uri, legacy_keypair: true) - allow(ActivityPub::FetchRemoteAccountService).to receive(:new).and_return(service_double) - end - - it 'finds the duplicates and calls fetch remote account service' do - expect { subject } - .to output_results('Duplicates found') - expect(service_double).to have_received(:call).with(uri) - end - end - end - describe '#backup' do let(:action) { :backup } diff --git a/spec/lib/mastodon/cli/maintenance_spec.rb b/spec/lib/mastodon/cli/maintenance_spec.rb index 6e1fb3cb6d1..38e15c91e1a 100644 --- a/spec/lib/mastodon/cli/maintenance_spec.rb +++ b/spec/lib/mastodon/cli/maintenance_spec.rb @@ -89,6 +89,11 @@ RSpec.describe Mastodon::CLI::Maintenance do def prepare_duplicate_data ActiveRecord::Base.connection.remove_index :accounts, name: :index_accounts_on_username_and_domain_lower + + # Re-create without uniqueness constraint for test purposes + ActiveRecord::Base.connection.remove_index :accounts, :uri + ActiveRecord::Base.connection.add_index :accounts, :uri + duplicate_record(:account, username: duplicate_account_username, domain: duplicate_account_domain, legacy_keypair: true) duplicate_record(:account, username: duplicate_account_username, domain: nil) end