From 245064bb98ab11face7a04303fd62724820d9610 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Mon, 19 Feb 2024 06:09:43 -0500 Subject: [PATCH] Move "everyone" role and "instance actor" account magic number IDs to constants (#29260) --- app/models/account.rb | 5 +++-- app/models/concerns/account/finder_concern.rb | 4 ++-- app/models/user_role.rb | 10 ++++++---- db/migrate/20190715164535_add_instance_actor.rb | 6 ++++-- .../20220704024901_migrate_settings_to_user_roles.rb | 6 ++++-- db/seeds/02_instance_actor.rb | 2 +- lib/tasks/tests.rake | 2 +- spec/models/account_spec.rb | 4 ++-- spec/models/user_role_spec.rb | 6 +++--- 9 files changed, 26 insertions(+), 19 deletions(-) diff --git a/app/models/account.rb b/app/models/account.rb index 442d4a431d..d627fd6b64 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -64,6 +64,7 @@ class Account < ApplicationRecord ) BACKGROUND_REFRESH_INTERVAL = 1.week.freeze + INSTANCE_ACTOR_ID = -99 USERNAME_RE = /[a-z0-9_]+([a-z0-9_.-]+[a-z0-9_]+)?/i MENTION_RE = %r{(? { where.not(sensitized_at: nil) } scope :without_suspended, -> { where(suspended_at: nil) } scope :without_silenced, -> { where(silenced_at: nil) } - scope :without_instance_actor, -> { where.not(id: -99) } + scope :without_instance_actor, -> { where.not(id: INSTANCE_ACTOR_ID) } scope :recent, -> { reorder(id: :desc) } scope :bots, -> { where(actor_type: %w(Application Service)) } scope :groups, -> { where(actor_type: 'Group') } @@ -176,7 +177,7 @@ class Account < ApplicationRecord end def instance_actor? - id == -99 + id == INSTANCE_ACTOR_ID end alias bot bot? diff --git a/app/models/concerns/account/finder_concern.rb b/app/models/concerns/account/finder_concern.rb index 7faaddeb43..a7acff1cbb 100644 --- a/app/models/concerns/account/finder_concern.rb +++ b/app/models/concerns/account/finder_concern.rb @@ -13,11 +13,11 @@ module Account::FinderConcern end def representative - actor = Account.find(-99).tap(&:ensure_keys!) + actor = Account.find(Account::INSTANCE_ACTOR_ID).tap(&:ensure_keys!) actor.update!(username: 'mastodon.internal') if actor.username.include?(':') actor rescue ActiveRecord::RecordNotFound - Account.create!(id: -99, actor_type: 'Application', locked: true, username: 'mastodon.internal') + Account.create!(id: Account::INSTANCE_ACTOR_ID, actor_type: 'Application', locked: true, username: 'mastodon.internal') end def find_local(username) diff --git a/app/models/user_role.rb b/app/models/user_role.rb index 89354da542..ed64ca0538 100644 --- a/app/models/user_role.rb +++ b/app/models/user_role.rb @@ -38,6 +38,8 @@ class UserRole < ApplicationRecord delete_user_data: (1 << 19), }.freeze + EVERYONE_ROLE_ID = -99 + module Flags NONE = 0 ALL = FLAGS.values.reduce(&:|) @@ -94,7 +96,7 @@ class UserRole < ApplicationRecord before_validation :set_position - scope :assignable, -> { where.not(id: -99).order(position: :asc) } + scope :assignable, -> { where.not(id: EVERYONE_ROLE_ID).order(position: :asc) } has_many :users, inverse_of: :role, foreign_key: 'role_id', dependent: :nullify @@ -103,9 +105,9 @@ class UserRole < ApplicationRecord end def self.everyone - UserRole.find(-99) + UserRole.find(EVERYONE_ROLE_ID) rescue ActiveRecord::RecordNotFound - UserRole.create!(id: -99, permissions: Flags::DEFAULT) + UserRole.create!(id: EVERYONE_ROLE_ID, permissions: Flags::DEFAULT) end def self.that_can(*any_of_privileges) @@ -113,7 +115,7 @@ class UserRole < ApplicationRecord end def everyone? - id == -99 + id == EVERYONE_ROLE_ID end def nobody? diff --git a/db/migrate/20190715164535_add_instance_actor.rb b/db/migrate/20190715164535_add_instance_actor.rb index 3785dc2553..6871b37bdf 100644 --- a/db/migrate/20190715164535_add_instance_actor.rb +++ b/db/migrate/20190715164535_add_instance_actor.rb @@ -5,6 +5,8 @@ class AddInstanceActor < ActiveRecord::Migration[5.2] # Dummy class, to make migration possible across version changes validates :username, uniqueness: { scope: :domain, case_sensitive: false } + INSTANCE_ACTOR_ID = -99 + before_create :generate_keys def generate_keys @@ -15,10 +17,10 @@ class AddInstanceActor < ActiveRecord::Migration[5.2] end def up - Account.create!(id: -99, actor_type: 'Application', locked: true, username: Rails.configuration.x.local_domain) + Account.create!(id: Account::INSTANCE_ACTOR_ID, actor_type: 'Application', locked: true, username: Rails.configuration.x.local_domain) end def down - Account.find_by(id: -99, actor_type: 'Application').destroy! + Account.find_by(id: Account::INSTANCE_ACTOR_ID, actor_type: 'Application').destroy! end end diff --git a/db/post_migrate/20220704024901_migrate_settings_to_user_roles.rb b/db/post_migrate/20220704024901_migrate_settings_to_user_roles.rb index 00afee26d0..42dc37f08b 100644 --- a/db/post_migrate/20220704024901_migrate_settings_to_user_roles.rb +++ b/db/post_migrate/20220704024901_migrate_settings_to_user_roles.rb @@ -3,7 +3,9 @@ class MigrateSettingsToUserRoles < ActiveRecord::Migration[6.1] disable_ddl_transaction! - class UserRole < ApplicationRecord; end + class UserRole < ApplicationRecord + EVERYONE_ROLE_ID = -99 + end def up process_role_everyone @@ -17,7 +19,7 @@ class MigrateSettingsToUserRoles < ActiveRecord::Migration[6.1] private def process_role_everyone - everyone_role = UserRole.find_by(id: -99) + everyone_role = UserRole.find_by(id: UserRole::EVERYONE_ROLE_ID) return unless everyone_role everyone_role.permissions &= ~::UserRole::FLAGS[:invite_users] unless min_invite_role == 'user' diff --git a/db/seeds/02_instance_actor.rb b/db/seeds/02_instance_actor.rb index 55e83e8a08..2b6befec0d 100644 --- a/db/seeds/02_instance_actor.rb +++ b/db/seeds/02_instance_actor.rb @@ -1,3 +1,3 @@ # frozen_string_literal: true -Account.create_with(actor_type: 'Application', locked: true, username: 'mastodon.internal').find_or_create_by(id: -99) +Account.create_with(actor_type: 'Application', locked: true, username: 'mastodon.internal').find_or_create_by(id: Account::INSTANCE_ACTOR_ID) diff --git a/lib/tasks/tests.rake b/lib/tasks/tests.rake index 885be79f41..935f6d24a3 100644 --- a/lib/tasks/tests.rake +++ b/lib/tasks/tests.rake @@ -50,7 +50,7 @@ namespace :tests do exit(1) end - if Account.find(-99).private_key.blank? + if Account.find(Account::INSTANCE_ACTOR_ID).private_key.blank? puts 'Instance actor does not have a private key' exit(1) end diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index b1dca52dc5..f6376eb36e 100644 --- a/spec/models/account_spec.rb +++ b/spec/models/account_spec.rb @@ -746,13 +746,13 @@ RSpec.describe Account do end it 'is valid if we are creating an instance actor account with a period' do - account = Fabricate.build(:account, id: -99, actor_type: 'Application', locked: true, username: 'example.com') + account = Fabricate.build(:account, id: described_class::INSTANCE_ACTOR_ID, actor_type: 'Application', locked: true, username: 'example.com') expect(account.valid?).to be true end it 'is valid if we are creating a possibly-conflicting instance actor account' do _account = Fabricate(:account, username: 'examplecom') - instance_account = Fabricate.build(:account, id: -99, actor_type: 'Application', locked: true, username: 'example.com') + instance_account = Fabricate.build(:account, id: described_class::INSTANCE_ACTOR_ID, actor_type: 'Application', locked: true, username: 'example.com') expect(instance_account.valid?).to be true end diff --git a/spec/models/user_role_spec.rb b/spec/models/user_role_spec.rb index d5234ebe8d..9dd04a8172 100644 --- a/spec/models/user_role_spec.rb +++ b/spec/models/user_role_spec.rb @@ -164,12 +164,12 @@ RSpec.describe UserRole do end describe '#everyone?' do - it 'returns true when id is -99' do - subject.id = -99 + it 'returns true when id matches the everyone id' do + subject.id = described_class::EVERYONE_ROLE_ID expect(subject.everyone?).to be true end - it 'returns false when id is not -99' do + it 'returns false when id does not match the everyone id' do subject.id = 123 expect(subject.everyone?).to be false end