From 5c524ea8a4d35a39c791699906a0ce26fd3d5efd Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Wed, 29 May 2019 14:26:06 +1000 Subject: [PATCH] FEATURE: introduce dedicated storage and DB constraints for anon users Previously we used custom fields to denote a user was anonymous, this was risky in that custom fields are prone to race conditions and are not properly dedicated, missing constraints and so on. The new table `anonymous_users` is properly protected. There is only one possible shadow account per user, which is enforced using a constraint. Every anonymous user will have a unique row in the new table. --- app/models/anonymous_user.rb | 23 ++++++++ app/models/user.rb | 17 +++--- app/services/anonymous_shadow_creator.rb | 50 +++++++++------- ...dd_unique_constraint_to_shadow_accounts.rb | 59 +++++++++++++++++++ spec/fabricators/user_fabricator.rb | 8 ++- .../services/anonymous_shadow_creator_spec.rb | 4 ++ 6 files changed, 129 insertions(+), 32 deletions(-) create mode 100644 app/models/anonymous_user.rb create mode 100644 db/migrate/20190529002752_add_unique_constraint_to_shadow_accounts.rb diff --git a/app/models/anonymous_user.rb b/app/models/anonymous_user.rb new file mode 100644 index 00000000000..57147625083 --- /dev/null +++ b/app/models/anonymous_user.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +class AnonymousUser < ActiveRecord::Base + belongs_to :user + belongs_to :master_user, class_name: 'User' +end + +# == Schema Information +# +# Table name: anonymous_users +# +# id :bigint not null, primary key +# user_id :integer not null +# master_user_id :integer not null +# active :boolean not null +# created_at :datetime not null +# updated_at :datetime not null +# +# Indexes +# +# index_anonymous_users_on_master_user_id (master_user_id) UNIQUE WHERE active +# index_anonymous_users_on_user_id (user_id) UNIQUE +# diff --git a/app/models/user.rb b/app/models/user.rb index 8576a6a2287..6f36dec65c0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -79,6 +79,12 @@ class User < ActiveRecord::Base where(method: UserSecondFactor.methods[:totp], enabled: true) }, class_name: "UserSecondFactor" + has_one :anonymous_user_master, class_name: 'AnonymousUser' + has_one :anonymous_user_shadow, ->(record) { where(active: true) }, foreign_key: :master_user_id, class_name: 'AnonymousUser' + + has_one :master_user, through: :anonymous_user_master + has_one :shadow_user, through: :anonymous_user_shadow, source: :user + has_one :user_stat, dependent: :destroy has_one :user_profile, dependent: :destroy, inverse_of: :user has_one :profile_background_upload, through: :user_profile @@ -177,12 +183,9 @@ class User < ActiveRecord::Base # excluding fake users like the system user or anonymous users scope :real, -> { human_users.where('NOT EXISTS( SELECT 1 - FROM user_custom_fields ucf - WHERE - ucf.user_id = users.id AND - ucf.name = ? AND - ucf.value::int > 0 - )', 'master_id') } + FROM anonymous_users a + WHERE a.user_id = users.id + )') } # TODO-PERF: There is no indexes on any of these # and NotifyMailingListSubscribers does a select-all-and-loop @@ -1149,7 +1152,7 @@ class User < ActiveRecord::Base def anonymous? SiteSetting.allow_anonymous_posting && trust_level >= 1 && - custom_fields["master_id"].to_i > 0 + !!anonymous_user_master end def is_singular_admin? diff --git a/app/services/anonymous_shadow_creator.rb b/app/services/anonymous_shadow_creator.rb index ae7569f359f..c5c4d873c12 100644 --- a/app/services/anonymous_shadow_creator.rb +++ b/app/services/anonymous_shadow_creator.rb @@ -1,37 +1,46 @@ # frozen_string_literal: true class AnonymousShadowCreator + attr_reader :user def self.get_master(user) - return unless user - return unless SiteSetting.allow_anonymous_posting - - if (master_id = user.custom_fields["master_id"].to_i) > 0 - User.find_by(id: master_id) - end + new(user).get_master end def self.get(user) + new(user).get + end + + def initialize(user) + @user = user + end + + def get_master + return unless user + return unless SiteSetting.allow_anonymous_posting + + user.master_user + end + + def get return unless user return unless SiteSetting.allow_anonymous_posting return if user.trust_level < SiteSetting.anonymous_posting_min_trust_level return if SiteSetting.must_approve_users? && !user.approved? - if (shadow_id = user.custom_fields["shadow_id"].to_i) > 0 - shadow = User.find_by(id: shadow_id) + shadow = user.shadow_user - if shadow && (shadow.post_count + shadow.topic_count) > 0 && - shadow.last_posted_at < SiteSetting.anonymous_account_duration_minutes.minutes.ago - shadow = nil - end - - shadow || create_shadow(user) - else - create_shadow(user) + if shadow && (shadow.post_count + shadow.topic_count) > 0 && + shadow.last_posted_at < SiteSetting.anonymous_account_duration_minutes.minutes.ago + shadow = nil end + + shadow || create_shadow! end - def self.create_shadow(user) + private + + def create_shadow! username = UserNameSuggester.suggest(I18n.t(:anonymous).downcase) User.transaction do @@ -57,11 +66,8 @@ class AnonymousShadowCreator shadow.email_tokens.update_all(confirmed: true) shadow.activate - # can not hold dupes - UserCustomField.where(user_id: user.id, name: "shadow_id").destroy_all - - UserCustomField.create!(user_id: user.id, name: "shadow_id", value: shadow.id) - UserCustomField.create!(user_id: shadow.id, name: "master_id", value: user.id) + AnonymousUser.where(master_user_id: user.id, active: true).update_all(active: false) + AnonymousUser.create!(user_id: shadow.id, master_user_id: user.id, active: true) shadow.reload user.reload diff --git a/db/migrate/20190529002752_add_unique_constraint_to_shadow_accounts.rb b/db/migrate/20190529002752_add_unique_constraint_to_shadow_accounts.rb new file mode 100644 index 00000000000..78a8a3f4f73 --- /dev/null +++ b/db/migrate/20190529002752_add_unique_constraint_to_shadow_accounts.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +class AddUniqueConstraintToShadowAccounts < ActiveRecord::Migration[5.2] + + def up + create_table :anonymous_users do |t| + t.integer :user_id, null: false + t.integer :master_user_id, null: false + t.boolean :active, null: false + t.timestamps + + t.index [:user_id], unique: true + t.index [:master_user_id], unique: true, where: 'active' + end + + rows = DB.exec <<~SQL + DELETE FROM user_custom_fields + WHERE name = 'shadow_id' AND value in ( + SELECT value + FROM user_custom_fields + WHERE name = 'shadow_id' + GROUP BY value + HAVING COUNT(*) > 1 + ) + SQL + + if rows > 0 + STDERR.puts "Removed #{rows} duplicate shadow users" + end + + rows = DB.exec <<~SQL + INSERT INTO anonymous_users(user_id, master_user_id, created_at, updated_at, active) + SELECT value::int, user_id, created_at, updated_at, 't' + FROM user_custom_fields + WHERE name = 'shadow_id' + SQL + + rows += DB.exec <<~SQL + INSERT INTO anonymous_users(user_id, master_user_id, created_at, updated_at, active) + SELECT f.user_id, value::int, f.created_at, f.updated_at, 'f' + FROM user_custom_fields f + LEFT JOIN anonymous_users a on a.user_id = f.user_id + WHERE name = 'master_id' AND a.user_id IS NULL + SQL + + if rows > 0 + STDERR.puts "Migrated #{rows} anon users to new structure" + end + + DB.exec <<~SQL + DELETE FROM user_custom_fields + WHERE name in ('shadow_id', 'master_id') + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/spec/fabricators/user_fabricator.rb b/spec/fabricators/user_fabricator.rb index 45ccb590a3b..6e6e4624f9c 100644 --- a/spec/fabricators/user_fabricator.rb +++ b/spec/fabricators/user_fabricator.rb @@ -101,9 +101,11 @@ Fabricator(:anonymous, from: :user) do trust_level TrustLevel[1] manual_locked_trust_level TrustLevel[1] - before_create do |user| - user.custom_fields["master_id"] = 1 - user.save! + after_create do + # this is not "the perfect" fabricator in that user id -1 is system + # but creating a proper account here is real slow and has a huge + # impact on the test suite run time + create_anonymous_user_master(master_user_id: -1, active: true) end end diff --git a/spec/services/anonymous_shadow_creator_spec.rb b/spec/services/anonymous_shadow_creator_spec.rb index 57e4d52d109..c47635f9552 100644 --- a/spec/services/anonymous_shadow_creator_spec.rb +++ b/spec/services/anonymous_shadow_creator_spec.rb @@ -34,6 +34,9 @@ describe AnonymousShadowCreator do expect(shadow.id).to eq(shadow2.id) create_post(user: shadow) + user.reload + shadow.reload + freeze_time 4.minutes.from_now shadow3 = AnonymousShadowCreator.get(user) @@ -56,6 +59,7 @@ describe AnonymousShadowCreator do expect(shadow.created_at).not_to eq(user.created_at) p = create_post + expect(Guardian.new(shadow).post_can_act?(p, :like)).to eq(false) expect(Guardian.new(user).post_can_act?(p, :like)).to eq(true)