diff --git a/app/controllers/admin/email_controller.rb b/app/controllers/admin/email_controller.rb index 455a22151e7..75e07bb22b6 100644 --- a/app/controllers/admin/email_controller.rb +++ b/app/controllers/admin/email_controller.rb @@ -73,7 +73,7 @@ class Admin::EmailController < Admin::AdminController params.require(:from) params.require(:to) # These strings aren't localized; they are sent to an anonymous SMTP user. - if !User.exists?(email: Email.downcase(params[:from])) && !SiteSetting.enable_staged_users + if !User.with_email(Email.downcase(params[:from])).exists? && !SiteSetting.enable_staged_users render json: { reject: true, reason: "Mail from your address is not accepted. Do you have an account here?" } elsif Email::Receiver.check_address(Email.downcase(params[:to])).nil? render json: { reject: true, reason: "Mail to this address is not accepted. Check the address and try to send again?" } diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index 90045704ff0..3c4182ac6da 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/app/controllers/admin/groups_controller.rb @@ -29,7 +29,11 @@ class Admin::GroupsController < Admin::AdminController users = (params[:users] || []).map {|u| u.downcase} valid_emails = {} valid_usernames = {} - valid_users = User.where("username_lower IN (:users) OR email IN (:users)", users: users).pluck(:id, :username_lower, :email) + + valid_users = User.joins(:user_emails) + .where("username_lower IN (:users) OR user_emails.email IN (:users)", users: users) + .pluck(:id, :username_lower, :"user_emails.email") + valid_users.map! do |id, username_lower, email| valid_emails[email] = valid_usernames[username_lower] = id id diff --git a/app/controllers/finish_installation_controller.rb b/app/controllers/finish_installation_controller.rb index c9a30724d48..4e73594d8a5 100644 --- a/app/controllers/finish_installation_controller.rb +++ b/app/controllers/finish_installation_controller.rb @@ -15,7 +15,7 @@ class FinishInstallationController < ApplicationController email = params[:email].strip raise Discourse::InvalidParameters.new unless @allowed_emails.include?(email) - return redirect_confirm(email) if User.where(email: email).exists? + return redirect_confirm(email) if UserEmail.exists?(email: email) @user.email = email @user.username = params[:username] @@ -37,7 +37,7 @@ class FinishInstallationController < ApplicationController def resend_email @email = session[:registered_email] - @user = User.where(email: @email).first + @user = User.find_by_email(@email) if @user.present? @email_token = @user.email_tokens.unconfirmed.active.first if @email_token.present? diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 74b7ad0f66d..4d447b0d3bb 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -153,7 +153,7 @@ class GroupsController < ApplicationController elsif params[:user_ids].present? User.find(params[:user_ids].split(",")) elsif params[:user_emails].present? - User.where(email: params[:user_emails].split(",")) + User.with_email(params[:user_emails].split(",")) else raise Discourse::InvalidParameters.new( 'user_ids or usernames or user_emails must be present' diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index fb510fa4eb0..69ad6520785 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -311,7 +311,7 @@ class UsersController < ApplicationController return fail_with("login.reserved_username") end - if user = User.find_by(staged: true, email: params[:email].strip.downcase) + if user = User.where(staged: true).with_email(params[:email].strip.downcase).first user_params.each { |k, v| user.send("#{k}=", v) } user.staged = false else @@ -490,7 +490,7 @@ class UsersController < ApplicationController RateLimiter.new(nil, "admin-login-hr-#{request.remote_ip}", 6, 1.hour).performed! RateLimiter.new(nil, "admin-login-min-#{request.remote_ip}", 3, 1.minute).performed! - user = User.where(email: params[:email], admin: true).human_users.first + user = User.with_email(params[:email]).where(admin: true).human_users.first if user email_token = user.email_tokens.create(email: user.email) Jobs.enqueue(:critical_user_email, type: :admin_login, user_id: user.id, email_token: email_token.token) @@ -606,6 +606,7 @@ class UsersController < ApplicationController User.transaction do @user.email = params[:email] + if @user.save @user.email_tokens.create(email: @user.email) enqueue_activation_email diff --git a/app/jobs/regular/automatic_group_membership.rb b/app/jobs/regular/automatic_group_membership.rb index 91a417927a9..b2f12d15022 100644 --- a/app/jobs/regular/automatic_group_membership.rb +++ b/app/jobs/regular/automatic_group_membership.rb @@ -13,7 +13,8 @@ module Jobs domains = group.automatic_membership_email_domains.gsub('.', '\.') - User.where("email ~* '@(#{domains})$'") + User.joins(:user_emails) + .where("user_emails.email ~* '@(#{domains})$'") .where("users.id NOT IN (SELECT user_id FROM group_users WHERE group_users.group_id = ?)", group_id) .activated .where(staged: false) diff --git a/app/models/email_token.rb b/app/models/email_token.rb index 225e083b54d..cff339a08eb 100644 --- a/app/models/email_token.rb +++ b/app/models/email_token.rb @@ -71,7 +71,7 @@ class EmailToken < ActiveRecord::Base end if user - return User.find_by(email: Email.downcase(user.email)) if Invite.redeem_from_email(user.email).present? + return User.find_by_email(user.email) if Invite.redeem_from_email(user.email).present? user end end diff --git a/app/models/group.rb b/app/models/group.rb index 5f8aa3fd8d4..5a9720c7f58 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -595,7 +595,7 @@ end # allow_membership_requests :boolean default(FALSE), not null # full_name :string # default_notification_level :integer default(3), not null -# visibility_level :integer default(0) +# visibility_level :integer default(0), not null # # Indexes # diff --git a/app/models/invite.rb b/app/models/invite.rb index 7f7455fa91f..01b6e970f1d 100644 --- a/app/models/invite.rb +++ b/app/models/invite.rb @@ -32,8 +32,9 @@ class Invite < ActiveRecord::Base def user_doesnt_already_exist @email_already_exists = false return if email.blank? - u = User.find_by("email = ?", Email.downcase(email)) - if u && u.id != self.user_id + user = User.find_by_email(email) + + if user && user.id != self.user_id @email_already_exists = true errors.add(:email) end @@ -98,11 +99,9 @@ class Invite < ActiveRecord::Base group_ids = opts[:group_ids] send_email = opts[:send_email].nil? ? true : opts[:send_email] custom_message = opts[:custom_message] - lower_email = Email.downcase(email) - user = User.find_by(email: lower_email) - if user + if user = User.find_by_email(lower_email) extend_permissions(topic, user, invited_by) if topic raise UserExists.new I18n.t("invite.user_exists", email: lower_email, username: user.username) end diff --git a/app/models/invite_redeemer.rb b/app/models/invite_redeemer.rb index 8205cb549e2..5de389af3b7 100644 --- a/app/models/invite_redeemer.rb +++ b/app/models/invite_redeemer.rb @@ -91,7 +91,7 @@ InviteRedeemer = Struct.new(:invite, :username, :name, :password, :user_custom_f end def get_existing_user - User.where(admin: false).find_by(email: invite.email) + User.where(admin: false).find_by_email(invite.email) end diff --git a/app/models/search_log.rb b/app/models/search_log.rb index ab873eb3c57..35a427d7185 100644 --- a/app/models/search_log.rb +++ b/app/models/search_log.rb @@ -55,3 +55,16 @@ class SearchLog < ActiveRecord::Base end end end + +# == Schema Information +# +# Table name: search_logs +# +# id :integer not null, primary key +# term :string not null +# user_id :integer +# ip_address :inet not null +# clicked_topic_id :integer +# search_type :integer not null +# created_at :datetime not null +# diff --git a/app/models/topic.rb b/app/models/topic.rb index 495459d4fd2..ac21e45b3eb 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -810,7 +810,7 @@ SQL end def grant_permission_to_user(lower_email) - user = User.find_by(email: lower_email) + user = User.find_by_email(lower_email) topic_allowed_users.create!(user_id: user.id) end diff --git a/app/models/user.rb b/app/models/user.rb index 38db6852c92..695f2f0b87d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -42,7 +42,11 @@ class User < ActiveRecord::Base has_many :email_change_requests, dependent: :destroy has_many :directory_items, dependent: :delete_all has_many :user_auth_tokens, dependent: :destroy + has_many :user_emails, dependent: :destroy + has_one :primary_email, -> { where(primary: true) }, + dependent: :destroy, + class_name: 'UserEmail' has_one :user_option, dependent: :destroy has_one :user_avatar, dependent: :destroy @@ -74,16 +78,13 @@ class User < ActiveRecord::Base delegate :last_sent_email_address, :to => :email_logs - before_validation :strip_downcase_email - validates_presence_of :username validate :username_validator, if: :username_changed? - validates :email, presence: true, uniqueness: true - validates :email, format: { with: EmailValidator.email_regex }, if: :email_changed? - validates :email, email: true, if: :should_validate_email? validate :password_validator validates :name, user_full_name: true, if: :name_changed?, length: { maximum: 255 } validates :ip_address, allowed_ip_address: {on: :create, message: :signup_not_allowed} + validates :primary_email, presence: true, if: :should_validate_primary_email? + validates_associated :primary_email, if: :should_validate_primary_email? after_initialize :add_trust_level @@ -123,6 +124,8 @@ class User < ActiveRecord::Base # set to true to optimize creation and save for imports attr_accessor :import_mode + scope :with_email, ->(email) { joins(:user_emails).where(user_emails: { email: email }) } + scope :human_users, -> { where('users.id > 0') } # excluding fake users like the system user or anonymous users @@ -232,7 +235,7 @@ class User < ActiveRecord::Base end def self.find_by_email(email) - find_by(email: Email.downcase(email)) + self.with_email(Email.downcase(email)).first end def self.find_by_username(username) @@ -267,8 +270,8 @@ class User < ActiveRecord::Base used_invite.try(:invited_by) end - def should_validate_email? - return !skip_email_validation && !staged? && email_changed? + def should_validate_primary_email? + !skip_email_validation && !staged? end # Approve this user @@ -935,6 +938,18 @@ class User < ActiveRecord::Base end end + def email + primary_email.email + end + + def email=(email) + if primary_email + self.new_record? ? primary_email.email = email : primary_email.update(email: email) + else + build_primary_email(email: email) + end + end + protected def badge_grant @@ -1011,13 +1026,6 @@ class User < ActiveRecord::Base self.username_lower = username.downcase end - def strip_downcase_email - if self.email - self.email = self.email.strip - self.email = self.email.downcase - end - end - def username_validator username_format_validator || begin lower = username.downcase @@ -1104,7 +1112,7 @@ end # name :string # seen_notification_id :integer default(0), not null # last_posted_at :datetime -# email :string(513) not null +# email :string(513) # password_hash :string(64) # salt :string(32) # active :boolean default(FALSE), not null diff --git a/app/models/user_email.rb b/app/models/user_email.rb new file mode 100644 index 00000000000..d97077c20a9 --- /dev/null +++ b/app/models/user_email.rb @@ -0,0 +1,45 @@ +require_dependency 'email_validator' + +class UserEmail < ActiveRecord::Base + belongs_to :user + + before_validation :strip_downcase_email + + validates :email, presence: true, uniqueness: true + + validates :email, email: true, format: { with: EmailValidator.email_regex }, + if: :skip_email_validation? + + validates :primary, uniqueness: { scope: [:user_id] } + + private + + def strip_downcase_email + if self.email + self.email = self.email.strip + self.email = self.email.downcase + end + end + + def skip_email_validation? + return true if user && user.skip_email_validation + email_changed? + end +end + +# == Schema Information +# +# Table name: user_emails +# +# id :integer not null, primary key +# user_id :integer not null +# email :string(513) not null +# primary :boolean default(FALSE), not null +# created_at :datetime +# updated_at :datetime +# +# Indexes +# +# index_user_emails_on_user_id (user_id) +# index_user_emails_on_user_id_and_primary (user_id,primary) UNIQUE +# diff --git a/db/fixtures/009_users.rb b/db/fixtures/009_users.rb index d725d78a414..8d84fad2dad 100644 --- a/db/fixtures/009_users.rb +++ b/db/fixtures/009_users.rb @@ -6,12 +6,18 @@ if user user.save end +UserEmail.seed do |ue| + ue.id = -1 + ue.email = "no_email" + ue.primary = true + ue.user_id = -1 +end + User.seed do |u| u.id = -1 u.name = "system" u.username = "system" u.username_lower = "system" - u.email = "no_email" u.password = SecureRandom.hex u.active = true u.admin = true @@ -56,12 +62,18 @@ ColumnDropper.drop( # User for the smoke tests if ENV["SMOKE"] == "1" + UserEmail.seed do |ue| + ue.id = 0 + ue.email = "smoke_user@discourse.org" + ue.primary = true + ue.user_id = 0 + end + smoke_user = User.seed do |u| u.id = 0 u.name = "smoke_user" u.username = "smoke_user" u.username_lower = "smoke_user" - u.email = "smoke_user@discourse.org" u.password = "P4ssw0rd" u.active = true u.approved = true @@ -77,4 +89,3 @@ if ENV["SMOKE"] == "1" EmailToken.where(user_id: smoke_user.id).update_all(confirmed: true) end - diff --git a/db/migrate/20170717084947_create_user_emails.rb b/db/migrate/20170717084947_create_user_emails.rb new file mode 100644 index 00000000000..eb6dc4980dc --- /dev/null +++ b/db/migrate/20170717084947_create_user_emails.rb @@ -0,0 +1,42 @@ +require_dependency 'column_dropper' + +class CreateUserEmails < ActiveRecord::Migration + def up + create_table :user_emails do |t| + t.integer :user_id, null: false + t.string :email, limit: 513, null: false + t.boolean :primary, default: false, null: false + t.timestamps + end + + add_index :user_emails, :user_id + add_index :user_emails, [:user_id, :primary], unique: true + + execute "CREATE UNIQUE INDEX index_user_emails_on_email ON user_emails (lower(email));" + + execute <<~SQL + INSERT INTO user_emails ( + id, + user_id, + email, + "primary", + created_at, + updated_at + ) SELECT + id, + id, + email, + 'TRUE', + created_at, + updated_at + FROM users + SQL + + change_column_null :users, :email, true + ColumnDropper.mark_readonly(:users, :email) + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/admin_user_index_query.rb b/lib/admin_user_index_query.rb index ffd1dd404d4..3a52f75661f 100644 --- a/lib/admin_user_index_query.rb +++ b/lib/admin_user_index_query.rb @@ -4,7 +4,7 @@ class AdminUserIndexQuery def initialize(params = {}, klass = User, trust_levels = TrustLevel.levels) @params = params - @query = initialize_query_with_order(klass) + @query = initialize_query_with_order(klass).joins(:user_emails) @trust_levels = trust_levels end @@ -52,7 +52,7 @@ class AdminUserIndexQuery if !custom_order.present? if params[:query] == "active" - order << "COALESCE(last_seen_at, to_date('1970-01-01', 'YYYY-MM-DD')) DESC" + order << "COALESCE(users.last_seen_at, to_date('1970-01-01', 'YYYY-MM-DD')) DESC" else order << "users.created_at DESC" end @@ -106,7 +106,7 @@ class AdminUserIndexQuery if ip = IPAddr.new(params[:filter]) rescue nil @query.where('ip_address <<= :ip OR registration_ip_address <<= :ip', ip: ip.to_cidr_s) else - @query.where('username_lower ILIKE :filter OR email ILIKE :filter', filter: "%#{params[:filter]}%") + @query.where('username_lower ILIKE :filter OR user_emails.email ILIKE :filter', filter: "%#{params[:filter]}%") end end end @@ -119,7 +119,7 @@ class AdminUserIndexQuery def filter_exclude if params[:exclude].present? - @query.where('id != ?', params[:exclude]) + @query.where('users.id != ?', params[:exclude]) end end diff --git a/lib/avatar_lookup.rb b/lib/avatar_lookup.rb index 4af184052a1..d0b5877272b 100644 --- a/lib/avatar_lookup.rb +++ b/lib/avatar_lookup.rb @@ -12,7 +12,7 @@ class AvatarLookup private def self.lookup_columns - @lookup_columns ||= %i{id email username uploaded_avatar_id} + @lookup_columns ||= %i{id user_emails.email username uploaded_avatar_id} end def users @@ -22,7 +22,8 @@ class AvatarLookup def user_lookup_hash # adding tap here is a personal taste thing hash = {} - User.where(:id => @user_ids) + User.joins(:user_emails) + .where(id: @user_ids) .select(AvatarLookup.lookup_columns) .each{ |user| hash[user.id] = user } hash diff --git a/lib/backup_restore/restorer.rb b/lib/backup_restore/restorer.rb index de6e133df13..2f4e7cdd46c 100644 --- a/lib/backup_restore/restorer.rb +++ b/lib/backup_restore/restorer.rb @@ -400,7 +400,7 @@ module BackupRestore end def notify_user - if user = User.find_by(email: @user_info[:email]) + if user = User.find_by_email(@user_info[:email]) log "Notifying '#{user.username}' of the end of the restore..." status = @success ? :restore_succeeded : :restore_failed diff --git a/lib/column_dropper.rb b/lib/column_dropper.rb index 766ddddddfb..2cfc81c3175 100644 --- a/lib/column_dropper.rb +++ b/lib/column_dropper.rb @@ -8,31 +8,65 @@ class ColumnDropper delay ||= Rails.env.production? ? 60 : 0 - sql = < 0 - on_drop&.call + on_drop&.call - columns.each do |column| - # safe cause it is protected on method entry, can not be passed in params - ActiveRecord::Base.exec_sql("ALTER TABLE #{table} DROP COLUMN IF EXISTS #{column}") - end + columns.each do |column| + # safe cause it is protected on method entry, can not be passed in params + ActiveRecord::Base.exec_sql("ALTER TABLE #{table} DROP COLUMN IF EXISTS #{column}") + + ActiveRecord::Base.exec_sql <<~SQL + DROP FUNCTION IF EXISTS #{readonly_function_name(table, column)}; + DROP TRIGGER IF EXISTS #{readonly_trigger_name(table, column)} ON #{table}; + SQL + end end end + + def self.mark_readonly(table_name, column_name) + ActiveRecord::Base.exec_sql <<-SQL + CREATE OR REPLACE FUNCTION #{readonly_function_name(table_name, column_name)} RETURNS trigger AS $rcr$ + BEGIN + RAISE EXCEPTION 'Discourse: #{column_name} in #{table_name} is readonly'; + END + $rcr$ LANGUAGE plpgsql; + SQL + + ActiveRecord::Base.exec_sql <<-SQL + CREATE TRIGGER #{readonly_trigger_name(table_name, column_name)} + BEFORE INSERT OR UPDATE OF #{column_name} + ON #{table_name} + FOR EACH ROW + WHEN (NEW.#{column_name} IS NOT NULL) + EXECUTE PROCEDURE #{readonly_function_name(table_name, column_name)}; + SQL + end + + private + + def self.readonly_function_name(table_name, column_name) + "raise_#{table_name}_#{column_name}_readonly()" + end + + def self.readonly_trigger_name(table_name, column_name) + "#{table_name}_#{column_name}_readonly" + end end diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index d4cdee0f7fc..6bb65bbded9 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -153,7 +153,7 @@ module Email if $redis.setnx(key, "1") $redis.expire(key, 25.hours) - if user = User.find_by(email: email) + if user = User.find_by_email(email) user.user_stat.bounce_score += score user.user_stat.reset_bounce_score_after = SiteSetting.reset_bounce_score_after_days.days.from_now user.user_stat.save diff --git a/lib/email_updater.rb b/lib/email_updater.rb index ab4c8246a2a..d7acb357813 100644 --- a/lib/email_updater.rb +++ b/lib/email_updater.rb @@ -81,7 +81,7 @@ class EmailUpdater confirm_result = :authorizing_new when EmailChangeRequest.states[:authorizing_new] change_req.update_column(:change_state, EmailChangeRequest.states[:complete]) - user.update_column(:email, token.email) + user.primary_email.update_column(:email, token.email) confirm_result = :complete end else diff --git a/lib/import_export/topic_importer.rb b/lib/import_export/topic_importer.rb index e7fc2d8f365..57496c5dab3 100644 --- a/lib/import_export/topic_importer.rb +++ b/lib/import_export/topic_importer.rb @@ -18,7 +18,7 @@ module ImportExport def import_users @export_data[:users].each do |u| - existing = User.where(email: u[:email]).first + existing = User.with_email(email: u[:email]).first if existing if existing.custom_fields["import_id"] != u[:id] existing.custom_fields["import_id"] = u[:id] diff --git a/plugins/discourse-narrative-bot/db/fixtures/001_discobot.rb b/plugins/discourse-narrative-bot/db/fixtures/001_discobot.rb index 8ba0d0322e3..e9ce7b2dd6a 100644 --- a/plugins/discourse-narrative-bot/db/fixtures/001_discobot.rb +++ b/plugins/discourse-narrative-bot/db/fixtures/001_discobot.rb @@ -4,12 +4,18 @@ user = User.find_by(id: -2) if !user suggested_username = UserNameSuggester.suggest(discobot_username) + UserEmail.seed do |ue| + ue.id = -2 + ue.email = "discobot_email" + ue.primary = true + ue.user_id = -2 + end + User.seed do |u| u.id = -2 u.name = discobot_username u.username = suggested_username u.username_lower = suggested_username.downcase - u.email = "discobot_email" u.password = SecureRandom.hex u.active = true u.approved = true diff --git a/script/bulk_import/base.rb b/script/bulk_import/base.rb index 7b25467838f..63d7041cb41 100644 --- a/script/bulk_import/base.rb +++ b/script/bulk_import/base.rb @@ -93,7 +93,7 @@ class BulkImport::Base puts "Loading users indexes..." @last_user_id = User.unscoped.maximum(:id) - @emails = User.unscoped.pluck(:email).to_set + @emails = User.unscoped.pluck(:"user_emails.email").to_set @usernames_lower = User.unscoped.pluck(:username_lower).to_set @mapped_usernames = UserCustomField.joins(:user).where(name: "import_username").pluck("user_custom_fields.value", "users.username").to_h diff --git a/script/import_scripts/base.rb b/script/import_scripts/base.rb index 616a534a5d8..c6255d85314 100644 --- a/script/import_scripts/base.rb +++ b/script/import_scripts/base.rb @@ -324,7 +324,7 @@ class ImportScripts::Base rescue => e # try based on email if e.try(:record).try(:errors).try(:messages).try(:[], :email).present? - if existing = User.find_by(email: opts[:email].downcase) + if existing = User.find_by_email(opts[:email].downcase) existing.custom_fields["import_id"] = import_id existing.save! u = existing diff --git a/spec/components/column_dropper_spec.rb b/spec/components/column_dropper_spec.rb index c1274ae6063..63ed5982da1 100644 --- a/spec/components/column_dropper_spec.rb +++ b/spec/components/column_dropper_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' require 'column_dropper' -describe ColumnDropper do +RSpec.describe ColumnDropper do def has_column?(table, column) Topic.exec_sql("SELECT 1 FROM INFORMATION_SCHEMA.COLUMNS @@ -48,5 +48,78 @@ describe ColumnDropper do expect(dropped_proc_called).to eq(true) end -end + describe '.mark_readonly' do + let(:table_name) { "table_with_readonly_column" } + + before do + ActiveRecord::Base.exec_sql <<~SQL + CREATE TABLE #{table_name} (topic_id INTEGER, email TEXT); + + INSERT INTO #{table_name} (topic_id, email) + VALUES (1, 'something@email.com'); + SQL + + described_class.mark_readonly(table_name, 'email') + end + + after do + ActiveRecord::Base.exec_sql <<~SQL + DROP TABLE IF EXISTS #{table_name}; + DROP TRIGGER IF EXISTS #{table_name}_email_readonly ON #{table_name}; + SQL + end + + it 'should prevent updates to the readonly column' do + expect do + ActiveRecord::Base.exec_sql <<~SQL + UPDATE #{table_name} + SET email = 'testing@email.com' + WHERE topic_id = 1; + SQL + end.to raise_error( + PG::RaiseException, + /Discourse: email in #{table_name} is readonly/ + ) + + ActiveRecord::Base.exec_sql("ROLLBACK") + end + + it 'should allow updates to the other columns' do + ActiveRecord::Base.exec_sql <<~SQL + UPDATE #{table_name} + SET topic_id = 2 + WHERE topic_id = 1 + SQL + + expect( + ActiveRecord::Base.exec_sql("SELECT * FROM #{table_name};").values + ).to include(["2", "something@email.com"]) + end + + it 'should prevent insertions to the readonly column' do + expect do + ActiveRecord::Base.exec_sql <<~SQL + INSERT INTO #{table_name} (topic_id, email) + VALUES (2, 'something@email.com'); + SQL + end.to raise_error( + PG::RaiseException, + /Discourse: email in table_with_readonly_column is readonly/ + ) + + ActiveRecord::Base.exec_sql("ROLLBACK") + end + + it 'should allow insertions to the other columns' do + ActiveRecord::Base.exec_sql <<~SQL + INSERT INTO #{table_name} (topic_id) + VALUES (2); + SQL + + expect( + ActiveRecord::Base.exec_sql("SELECT * FROM #{table_name} WHERE topic_id = 2;").values + ).to include(["2", nil]) + end + end +end diff --git a/spec/components/email/receiver_spec.rb b/spec/components/email/receiver_spec.rb index c49c98a81e4..bade562d33a 100644 --- a/spec/components/email/receiver_spec.rb +++ b/spec/components/email/receiver_spec.rb @@ -381,7 +381,7 @@ describe Email::Receiver do it "invites everyone in the chain but emails configured as 'incoming' (via reply, group or category)" do expect { process(:cc) }.to change(Topic, :count) - emails = Topic.last.allowed_users.pluck(:email) + emails = Topic.last.allowed_users.joins(:user_emails).pluck(:"user_emails.email") expect(emails.size).to eq(3) expect(emails).to include("someone@else.com", "discourse@bar.com", "wat@bar.com") end diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index 35e17ec0b4a..199f783f395 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/spec/controllers/admin/users_controller_spec.rb @@ -154,6 +154,10 @@ describe Admin::UsersController do @another_user = Fabricate(:coding_horror) end + after do + $redis.flushall + end + it "raises an error when the user doesn't have permission" do Guardian.any_instance.expects(:can_grant_admin?).with(@another_user).returns(false) xhr :put, :grant_admin, user_id: @another_user.id @@ -512,7 +516,7 @@ describe Admin::UsersController do xhr :post, :invite_admin, name: 'Bill', username: 'bill22', email: 'bill@bill.com' expect(response).to be_success - u = User.find_by(email: 'bill@bill.com') + u = User.find_by_email('bill@bill.com') expect(u.name).to eq("Bill") expect(u.username).to eq("bill22") expect(u.admin).to eq(true) @@ -587,7 +591,7 @@ describe Admin::UsersController do xhr :post, :sync_sso, Rack::Utils.parse_query(sso.payload) expect(response).to be_success - user = User.where(email: 'dr@claw.com').first + user = User.find_by_email('dr@claw.com') expect(user).to be_present expect(user.ip_address).to be_blank end @@ -599,7 +603,7 @@ describe Admin::UsersController do xhr :post, :sync_sso, Rack::Utils.parse_query(sso.payload) expect(response.status).to eq(403) - expect(JSON.parse(response.body)["message"]).to include("Email can't be blank") + expect(JSON.parse(response.body)["message"]).to include("Primary email is invalid") end end end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 44aac166d1c..bb9dabedbe9 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -822,7 +822,7 @@ describe UsersController do it "should succeed without the optional field" do xhr :post, :create, create_params expect(response).to be_success - inserted = User.where(email: @user.email).first + inserted = User.find_by_email(@user.email) expect(inserted).to be_present expect(inserted.custom_fields).to be_present expect(inserted.custom_fields["user_field_#{user_field.id}"]).to eq('value1') @@ -834,7 +834,7 @@ describe UsersController do create_params[:user_fields][optional_field.id.to_s] = 'value3' xhr :post, :create, create_params.merge(create_params) expect(response).to be_success - inserted = User.where(email: @user.email).first + inserted = User.find_by_email(@user.email) expect(inserted).to be_present expect(inserted.custom_fields).to be_present expect(inserted.custom_fields["user_field_#{user_field.id}"]).to eq('value1') @@ -846,7 +846,7 @@ describe UsersController do create_params[:user_fields][optional_field.id.to_s] = ('x' * 3000) xhr :post, :create, create_params.merge(create_params) expect(response).to be_success - inserted = User.where(email: @user.email).first + inserted = User.find_by_email(@user.email) val = inserted.custom_fields["user_field_#{optional_field.id}"] expect(val.length).to eq(UserField.max_length) @@ -868,7 +868,7 @@ describe UsersController do it "should succeed" do xhr :post, :create, create_params expect(response).to be_success - inserted = User.where(email: @user.email).first + inserted = User.find_by_email(@user.email) expect(inserted).to be_present expect(inserted.custom_fields).not_to be_present expect(inserted.custom_fields["user_field_#{user_field.id}"]).to be_blank @@ -883,7 +883,7 @@ describe UsersController do xhr :post, :create, email: staged.email, username: "zogstrip", password: "P4ssw0rd$$" result = ::JSON.parse(response.body) expect(result["success"]).to eq(true) - expect(User.find_by(email: staged.email).staged).to eq(false) + expect(User.find_by_email(staged.email).staged).to eq(false) end end @@ -2033,6 +2033,7 @@ describe UsersController do password: 'qwerqwer123', email: user.email } + expect(response).to_not be_success end diff --git a/spec/fabricators/user_email_fabricator.rb b/spec/fabricators/user_email_fabricator.rb new file mode 100644 index 00000000000..3d0d0cc8b0c --- /dev/null +++ b/spec/fabricators/user_email_fabricator.rb @@ -0,0 +1,4 @@ +Fabricator(:user_email) do + email { sequence(:email) { |i| "bruce#{i}@wayne.com" } } + primary true +end diff --git a/spec/integration/groups_spec.rb b/spec/integration/groups_spec.rb index ed5b5bb7c6b..c2755944528 100644 --- a/spec/integration/groups_spec.rb +++ b/spec/integration/groups_spec.rb @@ -315,6 +315,13 @@ describe "Groups" do expect(response).to be_success end + + it "adds by email" do + expect { xhr :put, "/groups/#{group.id}/members", user_emails: [user1.email, user2.email].join(",") } + .to change { group.users.count }.by(2) + + expect(response).to be_success + end end it "returns 422 if member already exists" do diff --git a/spec/models/discourse_single_sign_on_spec.rb b/spec/models/discourse_single_sign_on_spec.rb index ca3659605fd..63835953c0b 100644 --- a/spec/models/discourse_single_sign_on_spec.rb +++ b/spec/models/discourse_single_sign_on_spec.rb @@ -271,7 +271,7 @@ describe DiscourseSingleSignOn do user = sso.lookup_or_create_user(ip_address) expect(user.active).to eq(true) - user.update_columns(email: 'xXx@themovie.com') + user.primary_email.update_columns(email: 'xXx@themovie.com') user = sso.lookup_or_create_user(ip_address) expect(user.email).to eq(old_email) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 2db4346b0db..18512fc017c 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -9,11 +9,10 @@ describe User do describe 'emails' do let(:user) { Fabricate.build(:user) } - it { is_expected.to validate_presence_of :email } - describe 'when record has a valid email' do it "should be valid" do user.email = 'test@gmail.com' + expect(user).to be_valid end end @@ -21,7 +20,9 @@ describe User do describe 'when record has an invalid email' do it 'should not be valid' do user.email = 'test@gmailcom' + expect(user).to_not be_valid + expect(user.errors.messages).to include(:primary_email) end end end diff --git a/spec/services/badge_granter_spec.rb b/spec/services/badge_granter_spec.rb index cb906c282d5..ab4cda18ee4 100644 --- a/spec/services/badge_granter_spec.rb +++ b/spec/services/badge_granter_spec.rb @@ -38,8 +38,10 @@ describe BadgeGranter do describe 'preview' do it 'can correctly preview' do Fabricate(:user, email: 'sam@gmail.com') - result = BadgeGranter.preview('select id user_id, null post_id, created_at granted_at from users - where email like \'%gmail.com\'', explain: true) + result = BadgeGranter.preview('select u.id user_id, null post_id, u.created_at granted_at from users u + join user_emails ue on ue.user_id = u.id AND ue.primary + where ue.email like \'%gmail.com\'', explain: true) + expect(result[:grant_count]).to eq(1) expect(result[:query_plan]).to be_present end