Commit 8e684809 authored by Timothy Andrew's avatar Timothy Andrew

Use a `ghost` boolean to track ghost users.

Rather than using a separate `ghost` state. This lets us have the benefits of
both ghost and blocked users (ghost: true, state: blocked) without having to
rewrite a number of queries to include cases for `state: ghost`.
parent 53c34c74
...@@ -124,6 +124,7 @@ class User < ActiveRecord::Base ...@@ -124,6 +124,7 @@ class User < ActiveRecord::Base
validate :unique_email, if: ->(user) { user.email_changed? } validate :unique_email, if: ->(user) { user.email_changed? }
validate :owns_notification_email, if: ->(user) { user.notification_email_changed? } validate :owns_notification_email, if: ->(user) { user.notification_email_changed? }
validate :owns_public_email, if: ->(user) { user.public_email_changed? } validate :owns_public_email, if: ->(user) { user.public_email_changed? }
validate :ghost_users_must_be_blocked
validates :avatar, file_size: { maximum: 200.kilobytes.to_i } validates :avatar, file_size: { maximum: 200.kilobytes.to_i }
before_validation :generate_password, on: :create before_validation :generate_password, on: :create
...@@ -185,8 +186,6 @@ class User < ActiveRecord::Base ...@@ -185,8 +186,6 @@ class User < ActiveRecord::Base
"administrator if you think this is an error." "administrator if you think this is an error."
end end
end end
state :ghost
end end
mount_uploader :avatar, AvatarUploader mount_uploader :avatar, AvatarUploader
...@@ -347,7 +346,7 @@ class User < ActiveRecord::Base ...@@ -347,7 +346,7 @@ class User < ActiveRecord::Base
# Return (create if necessary) the ghost user. The ghost user # Return (create if necessary) the ghost user. The ghost user
# owns records previously belonging to deleted users. # owns records previously belonging to deleted users.
def ghost def ghost
ghost_user = User.with_state(:ghost).first ghost_user = User.find_by_ghost(true)
ghost_user || ghost_user ||
begin begin
...@@ -359,7 +358,7 @@ class User < ActiveRecord::Base ...@@ -359,7 +358,7 @@ class User < ActiveRecord::Base
# Recheck if a ghost user is already present (one might have been) # Recheck if a ghost user is already present (one might have been)
# added between the time we last checked (first line of this method) # added between the time we last checked (first line of this method)
# and the time we acquired the lock. # and the time we acquired the lock.
ghost_user = User.with_state(:ghost).first ghost_user = User.find_by_ghost(true)
return ghost_user if ghost_user.present? return ghost_user if ghost_user.present?
uniquify = Uniquify.new uniquify = Uniquify.new
...@@ -373,7 +372,7 @@ class User < ActiveRecord::Base ...@@ -373,7 +372,7 @@ class User < ActiveRecord::Base
User.create( User.create(
username: username, password: Devise.friendly_token, username: username, password: Devise.friendly_token,
email: email, name: "Ghost User", state: :ghost email: email, name: "Ghost User", state: :blocked, ghost: true
) )
ensure ensure
advisory_lock.unlock advisory_lock.unlock
...@@ -477,6 +476,12 @@ class User < ActiveRecord::Base ...@@ -477,6 +476,12 @@ class User < ActiveRecord::Base
errors.add(:public_email, "is not an email you own") unless all_emails.include?(public_email) errors.add(:public_email, "is not an email you own") unless all_emails.include?(public_email)
end end
def ghost_users_must_be_blocked
if ghost? && !blocked?
errors.add(:ghost, 'cannot be enabled for a user who is not blocked.')
end
end
def update_emails_with_primary_email def update_emails_with_primary_email
primary_email_record = emails.find_by(email: email) primary_email_record = emails.find_by(email: email)
if primary_email_record if primary_email_record
......
...@@ -466,7 +466,6 @@ class NotificationService ...@@ -466,7 +466,6 @@ class NotificationService
users = users.to_a.compact.uniq users = users.to_a.compact.uniq
users = users.reject(&:blocked?) users = users.reject(&:blocked?)
users = users.reject(&:ghost?)
users.reject do |user| users.reject do |user|
global_notification_setting = user.global_notification_setting global_notification_setting = user.global_notification_setting
......
class AddColumnGhostToUsers < ActiveRecord::Migration
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
disable_ddl_transaction!
def up
add_column_with_default :users, :ghost, :boolean, default: false, allow_null: false
end
def down
remove_column :users, :ghost
end
end
...@@ -1282,6 +1282,7 @@ ActiveRecord::Schema.define(version: 20170216141440) do ...@@ -1282,6 +1282,7 @@ ActiveRecord::Schema.define(version: 20170216141440) do
t.string "incoming_email_token" t.string "incoming_email_token"
t.boolean "authorized_projects_populated" t.boolean "authorized_projects_populated"
t.boolean "notified_of_own_activity", default: false, null: false t.boolean "notified_of_own_activity", default: false, null: false
t.boolean "ghost", default: false, null: false
end end
add_index "users", ["admin"], name: "index_users_on_admin", using: :btree add_index "users", ["admin"], name: "index_users_on_admin", using: :btree
......
...@@ -26,6 +26,10 @@ FactoryGirl.define do ...@@ -26,6 +26,10 @@ FactoryGirl.define do
two_factor_via_otp two_factor_via_otp
end end
trait :ghost do
ghost true
end
trait :two_factor_via_otp do trait :two_factor_via_otp do
before(:create) do |user| before(:create) do |user|
user.otp_required_for_login = true user.otp_required_for_login = true
......
...@@ -208,6 +208,20 @@ describe User, models: true do ...@@ -208,6 +208,20 @@ describe User, models: true do
end end
end end
end end
describe 'ghost users' do
it 'does not allow a non-blocked ghost user' do
user = build(:user, :ghost, state: :active)
expect(user).to be_invalid
end
it 'allows a blocked ghost user' do
user = build(:user, :ghost, state: :blocked)
expect(user).to be_valid
end
end
end end
describe "scopes" do describe "scopes" do
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment