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

@@ -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
+#

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
+                     FROM anonymous_users a
-                     WHERE
+                     WHERE a.user_id = users.id
-                       ucf.user_id = users.id AND
+                  )') }
-                       ucf.name = ? AND
-                       ucf.value::int > 0
-                  )', 'master_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?

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
+    new(user).get_master
-    return unless SiteSetting.allow_anonymous_posting
-
-    if (master_id = user.custom_fields["master_id"].to_i) > 0
-      User.find_by(id: master_id)
-    end
   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.shadow_user
-      shadow = User.find_by(id: shadow_id)
 
     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)
+    shadow || create_shadow!
-    else
-      create_shadow(user)
-    end
   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
+      AnonymousUser.where(master_user_id: user.id, active: true).update_all(active: false)
-      UserCustomField.where(user_id: user.id, name: "shadow_id").destroy_all
+      AnonymousUser.create!(user_id: shadow.id, master_user_id: user.id, active: true)
-
-      UserCustomField.create!(user_id: user.id, name: "shadow_id", value: shadow.id)
-      UserCustomField.create!(user_id: shadow.id, name: "master_id", value: user.id)
 
       shadow.reload
       user.reload

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

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|
+  after_create do
-    user.custom_fields["master_id"] = 1
+    # this is not "the perfect" fabricator in that user id -1 is system
-    user.save!
+    # 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
 

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)