diff --git a/app/controllers/password_resets_controller.rb b/app/controllers/password_resets_controller.rb index ba22cf9b8..bf1fa2a5b 100644 --- a/app/controllers/password_resets_controller.rb +++ b/app/controllers/password_resets_controller.rb @@ -3,10 +3,15 @@ class PasswordResetsController < ApplicationController def create @user = User.find_by_name(params.dig(:user, :name)) - UserMailer.password_reset(@user).deliver_later - flash[:notice] = "Password reset email sent. Check your email" - respond_with(@user, location: new_session_path) + if @user.can_receive_email? + UserMailer.password_reset(@user).deliver_later + flash[:notice] = "Password reset email sent. Check your email" + respond_with(@user, location: new_session_path) + else + flash[:notice] = "Password not reset. This account does not have a valid, verified email address" + respond_with(@user) + end end def show diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 3c1bc20b0..58d8cb173 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -4,6 +4,7 @@ class UsersController < ApplicationController def new @user = User.new + @user.email_address = EmailAddress.new respond_with(@user) end @@ -110,7 +111,7 @@ class UsersController < ApplicationController def user_params(context) permitted_params = %i[ - password old_password password_confirmation email + password old_password password_confirmation comment_threshold default_image_size favorite_tags blacklisted_tags time_zone per_page custom_style theme @@ -123,7 +124,10 @@ class UsersController < ApplicationController enable_safe_mode enable_desktop_mode disable_post_tooltips ] - permitted_params << :name if context == :create + if context == :create + permitted_params += [:name, { email_address_attributes: [:address] }] + end + permitted_params << :level if CurrentUser.is_admin? params.require(:user).permit(permitted_params) diff --git a/app/logical/email_normalizer.rb b/app/logical/email_normalizer.rb new file mode 100644 index 000000000..19bb6c439 --- /dev/null +++ b/app/logical/email_normalizer.rb @@ -0,0 +1,76 @@ +module EmailNormalizer + module_function + + IGNORE_DOTS = %w[gmail.com] + IGNORE_PLUS_ADDRESSING = %w[gmail.com hotmail.com outlook.com live.com] + IGNORE_MINUS_ADDRESSING = %w[yahoo.com] + CANONICAL_DOMAINS = { + "googlemail.com" => "gmail.com", + "hotmail.co.uk" => "outlook.com", + "hotmail.co.jp" => "outlook.com", + "hotmail.co.th" => "outlook.com", + "hotmail.com" => "outlook.com", + "hotmail.ca" => "outlook.com", + "hotmail.de" => "outlook.com", + "hotmail.es" => "outlook.com", + "hotmail.fr" => "outlook.com", + "hotmail.it" => "outlook.com", + "live.com.au" => "outlook.com", + "live.com.ar" => "outlook.com", + "live.com.mx" => "outlook.com", + "live.co.uk" => "outlook.com", + "live.com" => "outlook.com", + "live.ca" => "outlook.com", + "live.cl" => "outlook.com", + "live.cn" => "outlook.com", + "live.de" => "outlook.com", + "live.fr" => "outlook.com", + "live.it" => "outlook.com", + "live.jp" => "outlook.com", + "live.nl" => "outlook.com", + "live.se" => "outlook.com", + "msn.com" => "outlook.com", + "yahoo.com.au" => "yahoo.com", + "yahoo.com.ar" => "yahoo.com", + "yahoo.com.br" => "yahoo.com", + "yahoo.com.cn" => "yahoo.com", + "yahoo.com.hk" => "yahoo.com", + "yahoo.com.mx" => "yahoo.com", + "yahoo.com.ph" => "yahoo.com", + "yahoo.com.sg" => "yahoo.com", + "yahoo.com.tw" => "yahoo.com", + "yahoo.com.vn" => "yahoo.com", + "yahoo.co.id" => "yahoo.com", + "yahoo.co.kr" => "yahoo.com", + "yahoo.co.jp" => "yahoo.com", + "yahoo.co.uk" => "yahoo.com", + "yahoo.ca" => "yahoo.com", + "yahoo.cn" => "yahoo.com", + "yahoo.de" => "yahoo.com", + "yahoo.es" => "yahoo.com", + "yahoo.fr" => "yahoo.com", + "yahoo.it" => "yahoo.com", + "ymail.com" => "yahoo.com", + "126.com" => "163.com", + "aim.com" => "aol.com", + "gmx.com" => "gmx.net", + "gmx.at" => "gmx.net", + "gmx.ch" => "gmx.net", + "gmx.de" => "gmx.net", + "gmx.fr" => "gmx.net", + "gmx.us" => "gmx.net", + } + + def normalize(address) + return nil unless address.count("@") == 1 + + name, domain = address.downcase.split("@") + + domain = CANONICAL_DOMAINS.fetch(domain, domain) + name = name.delete(".") if domain.in?(IGNORE_DOTS) + name = name.gsub(/\+.*\z/, "") if domain.in?(IGNORE_PLUS_ADDRESSING) + name = name.gsub(/-.*\z/, "") if domain.in?(IGNORE_MINUS_ADDRESSING) + + "#{name}@#{domain}" + end +end diff --git a/app/logical/spam_detector.rb b/app/logical/spam_detector.rb index 3f47623b1..a6ca4c2b0 100644 --- a/app/logical/spam_detector.rb +++ b/app/logical/spam_detector.rb @@ -12,7 +12,7 @@ class SpamDetector attr_accessor :record, :user, :user_ip, :content, :comment_type rakismet_attrs author: proc { user.name }, - author_email: proc { user.email }, + author_email: proc { user.email_address&.address }, blog_lang: "en", blog_charset: "UTF-8", comment_type: :comment_type, diff --git a/app/logical/user_deletion.rb b/app/logical/user_deletion.rb index aa0922e17..e06b2260e 100644 --- a/app/logical/user_deletion.rb +++ b/app/logical/user_deletion.rb @@ -29,7 +29,7 @@ class UserDeletion end def clear_user_settings - user.email = nil + user.email_address = nil user.last_logged_in_at = nil user.last_forum_read_at = nil user.favorite_tags = '' diff --git a/app/logical/user_email_change.rb b/app/logical/user_email_change.rb index d9a694e0b..2e97ac6b9 100644 --- a/app/logical/user_email_change.rb +++ b/app/logical/user_email_change.rb @@ -8,11 +8,10 @@ class UserEmailChange end def process - if User.authenticate(user.name, password).nil? - user.errors[:base] << "Password was incorrect" + if User.authenticate(user.name, password) + user.update(email_address_attributes: { address: new_email }) else - user.email = new_email - user.save + user.errors[:base] << "Password was incorrect" end end end diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index 00bdb1281..56a20b2a9 100644 --- a/app/mailers/user_mailer.rb +++ b/app/mailers/user_mailer.rb @@ -4,11 +4,11 @@ class UserMailer < ApplicationMailer def dmail_notice(dmail) @dmail = dmail - mail(:to => "#{dmail.to.name} <#{dmail.to.email}>", :subject => "#{Danbooru.config.app_name} - Message received from #{dmail.from.name}") + mail to: dmail.to.email_with_name, subject: "#{Danbooru.config.app_name} - Message received from #{dmail.from.name}" end def password_reset(user) @user = user - mail to: "#{@user.name} <#{@user.email}>", subject: "#{Danbooru.config.app_name} password reset request" + mail to: @user.email_with_name, subject: "#{Danbooru.config.app_name} password reset request" end end diff --git a/app/models/dmail.rb b/app/models/dmail.rb index d59e50ced..bf6533408 100644 --- a/app/models/dmail.rb +++ b/app/models/dmail.rb @@ -148,7 +148,7 @@ class Dmail < ApplicationRecord end def send_email - if is_recipient? && !is_deleted? && to.receive_email_notifications? && to.email =~ /@/ + if is_recipient? && !is_deleted? && to.receive_email_notifications? && to.can_receive_email? UserMailer.dmail_notice(self).deliver_now end end diff --git a/app/models/email_address.rb b/app/models/email_address.rb new file mode 100644 index 000000000..fde636524 --- /dev/null +++ b/app/models/email_address.rb @@ -0,0 +1,15 @@ +class EmailAddress < ApplicationRecord + # https://www.regular-expressions.info/email.html + EMAIL_REGEX = /\A[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}\z/ + + belongs_to :user + + validates :address, presence: true, confirmation: true, format: { with: EMAIL_REGEX } + validates :normalized_address, uniqueness: true + validates :user_id, uniqueness: true + + def address=(value) + self.normalized_address = EmailNormalizer.normalize(value) || address + super + end +end diff --git a/app/models/user.rb b/app/models/user.rb index 07cea2710..7896eb15f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -72,7 +72,6 @@ class User < ApplicationRecord after_initialize :initialize_attributes, if: :new_record? validates :name, user_name: true, on: :create - validates_uniqueness_of :email, :case_sensitive => false, :if => ->(rec) { rec.email.present? && rec.saved_change_to_email? } validates_length_of :password, :minimum => 5, :if => ->(rec) { rec.new_record? || rec.password.present?} validates_inclusion_of :default_image_size, :in => %w(large original) validates_inclusion_of :per_page, in: (1..PostSets::Post::MAX_PER_PAGE) @@ -82,7 +81,6 @@ class User < ApplicationRecord validate :validate_sock_puppets, :on => :create, :if => -> { Danbooru.config.enable_sock_puppet_validation? } before_validation :normalize_blacklisted_tags before_validation :set_per_page - before_validation :normalize_email before_create :encrypt_password_on_create before_update :encrypt_password_on_update before_create :promote_to_admin_if_first_user @@ -111,6 +109,7 @@ class User < ApplicationRecord has_one :api_key has_one :token_bucket + has_one :email_address, dependent: :destroy has_many :notes, foreign_key: :creator_id has_many :note_versions, :foreign_key => "updater_id" has_many :dmails, -> {order("dmails.id desc")}, :foreign_key => "owner_id" @@ -124,6 +123,7 @@ class User < ApplicationRecord has_many :tag_implications, foreign_key: :creator_id belongs_to :inviter, class_name: "User", optional: true + accepts_nested_attributes_for :email_address, reject_if: :all_blank, allow_destroy: true enum theme: { light: 0, dark: 100 }, _suffix: true # UserDeletion#rename renames deleted users to `user_<1234>~`. Tildes @@ -366,8 +366,12 @@ class User < ApplicationRecord end module EmailMethods - def normalize_email - self.email = nil if email.blank? + def email_with_name + "#{name} <#{email_address.address}>" + end + + def can_receive_email? + email_address.present? && email_address.is_verified? && email_address.is_deliverable? end end @@ -515,7 +519,7 @@ class User < ApplicationRecord if id == CurrentUser.user.id attributes += BOOLEAN_ATTRIBUTES attributes += %i[ - updated_at email last_logged_in_at last_forum_read_at + updated_at last_logged_in_at last_forum_read_at comment_threshold default_image_size favorite_tags blacklisted_tags time_zone per_page custom_style favorite_count api_regen_multiplier diff --git a/app/views/users/edit.html.erb b/app/views/users/edit.html.erb index ef02dd5a0..a9c68680e 100644 --- a/app/views/users/edit.html.erb +++ b/app/views/users/edit.html.erb @@ -25,7 +25,13 @@

- <%= CurrentUser.user.email.presence || "blank".html_safe %> – <%= link_to "Change your email", new_maintenance_user_email_change_path %> + <% if @user.email_address.present? %> + <%= @user.email_address.address %> + <% else %> + blank + <% end %> + + - <%= link_to "Change your email", new_maintenance_user_email_change_path %>

diff --git a/app/views/users/new.html.erb b/app/views/users/new.html.erb index 83b96e051..3081fdf0d 100644 --- a/app/views/users/new.html.erb +++ b/app/views/users/new.html.erb @@ -15,7 +15,9 @@
<%= edit_form_for(@user, html: { id: "signup-form" }) do |f| %> <%= f.input :name, as: :string %> - <%= f.input :email, required: false, as: :email, hint: "Optional" %> + <%= f.simple_fields_for :email_address do |fe| %> + <%= fe.input :address, label: "Email", required: false, as: :email, hint: "Optional" %> + <% end %> <%= f.input :password %> <%= f.input :password_confirmation %> diff --git a/db/migrate/20200309043653_create_email_addresses.rb b/db/migrate/20200309043653_create_email_addresses.rb new file mode 100644 index 000000000..aa5c5bf64 --- /dev/null +++ b/db/migrate/20200309043653_create_email_addresses.rb @@ -0,0 +1,20 @@ +class CreateEmailAddresses < ActiveRecord::Migration[6.0] + def change + create_table :email_addresses do |t| + t.timestamps + + t.references :user, index: false, null: false + t.string :address, null: false + t.string :normalized_address, null: false + t.boolean :is_verified, default: false, null: false + t.boolean :is_deliverable, default: true, null: false + + t.index :address + t.index :normalized_address + t.index :user_id, unique: true + + t.index :address, name: "index_email_addresses_on_address_trgm", using: :gin, opclass: :gin_trgm_ops + t.index :normalized_address, name: "index_email_addresses_on_normalized_address_trgm", using: :gin, opclass: :gin_trgm_ops + end + end +end diff --git a/db/structure.sql b/db/structure.sql index 9afdbd96b..2a025201a 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -880,6 +880,41 @@ CREATE SEQUENCE public.dtext_links_id_seq ALTER SEQUENCE public.dtext_links_id_seq OWNED BY public.dtext_links.id; +-- +-- Name: email_addresses; Type: TABLE; Schema: public; Owner: - +-- + +CREATE TABLE public.email_addresses ( + id bigint NOT NULL, + created_at timestamp(6) without time zone NOT NULL, + updated_at timestamp(6) without time zone NOT NULL, + user_id bigint NOT NULL, + address character varying NOT NULL, + normalized_address character varying NOT NULL, + is_verified boolean DEFAULT false NOT NULL, + is_deliverable boolean DEFAULT true NOT NULL +); + + +-- +-- Name: email_addresses_id_seq; Type: SEQUENCE; Schema: public; Owner: - +-- + +CREATE SEQUENCE public.email_addresses_id_seq + START WITH 1 + INCREMENT BY 1 + NO MINVALUE + NO MAXVALUE + CACHE 1; + + +-- +-- Name: email_addresses_id_seq; Type: SEQUENCE OWNED BY; Schema: public; Owner: - +-- + +ALTER SEQUENCE public.email_addresses_id_seq OWNED BY public.email_addresses.id; + + -- -- Name: favorite_groups; Type: TABLE; Schema: public; Owner: - -- @@ -3214,6 +3249,13 @@ ALTER TABLE ONLY public.dmails ALTER COLUMN id SET DEFAULT nextval('public.dmail ALTER TABLE ONLY public.dtext_links ALTER COLUMN id SET DEFAULT nextval('public.dtext_links_id_seq'::regclass); +-- +-- Name: email_addresses id; Type: DEFAULT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY public.email_addresses ALTER COLUMN id SET DEFAULT nextval('public.email_addresses_id_seq'::regclass); + + -- -- Name: favorite_groups id; Type: DEFAULT; Schema: public; Owner: - -- @@ -4243,6 +4285,14 @@ ALTER TABLE ONLY public.dtext_links ADD CONSTRAINT dtext_links_pkey PRIMARY KEY (id); +-- +-- Name: email_addresses email_addresses_pkey; Type: CONSTRAINT; Schema: public; Owner: - +-- + +ALTER TABLE ONLY public.email_addresses + ADD CONSTRAINT email_addresses_pkey PRIMARY KEY (id); + + -- -- Name: favorite_groups favorite_groups_pkey; Type: CONSTRAINT; Schema: public; Owner: - -- @@ -4826,6 +4876,41 @@ CREATE INDEX index_dtext_links_on_link_type ON public.dtext_links USING btree (l CREATE INDEX index_dtext_links_on_model_type_and_model_id ON public.dtext_links USING btree (model_type, model_id); +-- +-- Name: index_email_addresses_on_address; Type: INDEX; Schema: public; Owner: - +-- + +CREATE INDEX index_email_addresses_on_address ON public.email_addresses USING btree (address); + + +-- +-- Name: index_email_addresses_on_address_trgm; Type: INDEX; Schema: public; Owner: - +-- + +CREATE INDEX index_email_addresses_on_address_trgm ON public.email_addresses USING gin (address public.gin_trgm_ops); + + +-- +-- Name: index_email_addresses_on_normalized_address; Type: INDEX; Schema: public; Owner: - +-- + +CREATE INDEX index_email_addresses_on_normalized_address ON public.email_addresses USING btree (normalized_address); + + +-- +-- Name: index_email_addresses_on_normalized_address_trgm; Type: INDEX; Schema: public; Owner: - +-- + +CREATE INDEX index_email_addresses_on_normalized_address_trgm ON public.email_addresses USING gin (normalized_address public.gin_trgm_ops); + + +-- +-- Name: index_email_addresses_on_user_id; Type: INDEX; Schema: public; Owner: - +-- + +CREATE UNIQUE INDEX index_email_addresses_on_user_id ON public.email_addresses USING btree (user_id); + + -- -- Name: index_favorite_groups_on_creator_id; Type: INDEX; Schema: public; Owner: - -- @@ -7277,6 +7362,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20200223234015'), ('20200306202253'), ('20200307021204'), -('20200309035334'); +('20200309035334'), +('20200309043653'); diff --git a/script/fixes/063_migrate_emails.rb b/script/fixes/063_migrate_emails.rb new file mode 100755 index 000000000..22b2cc9e7 --- /dev/null +++ b/script/fixes/063_migrate_emails.rb @@ -0,0 +1,9 @@ +#!/usr/bin/env ruby + +require_relative "../../config/environment" + +User.where.not(email: nil).find_each.with_index do |user, n| + email = EmailAddress.new(user: user, address: user.email, is_verified: true) + email.save(validate: false) + puts "n=#{n} id=#{user.id} name=#{user.name} email=#{user.email} normalized_address=#{email.normalized_address}" +end diff --git a/test/factories/email_address.rb b/test/factories/email_address.rb new file mode 100644 index 000000000..e0b77da34 --- /dev/null +++ b/test/factories/email_address.rb @@ -0,0 +1,6 @@ +FactoryBot.define do + factory(:email_address) do + address { FFaker::Internet.email } + is_verified { true } + end +end diff --git a/test/factories/user.rb b/test/factories/user.rb index d364c5250..8dd25950b 100644 --- a/test/factories/user.rb +++ b/test/factories/user.rb @@ -4,7 +4,6 @@ FactoryBot.define do "user#{n}" end password {"password"} - email {FFaker::Internet.email} default_image_size {"large"} level {20} created_at {Time.now} diff --git a/test/functional/maintenance/user/email_changes_controller_test.rb b/test/functional/maintenance/user/email_changes_controller_test.rb index d26807ae7..e9fb82932 100644 --- a/test/functional/maintenance/user/email_changes_controller_test.rb +++ b/test/functional/maintenance/user/email_changes_controller_test.rb @@ -5,7 +5,7 @@ module Maintenance class EmailChangesControllerTest < ActionDispatch::IntegrationTest context "in all cases" do setup do - @user = create(:user, :email => "bob@ogres.net") + @user = create(:user, email_address: build(:email_address, { address: "bob@ogres.net" })) end context "#new" do @@ -20,16 +20,14 @@ module Maintenance should "work" do post_auth maintenance_user_email_change_path, @user, params: {:email_change => {:password => "password", :email => "abc@ogres.net"}} assert_redirected_to(edit_user_path(@user)) - @user.reload - assert_equal("abc@ogres.net", @user.email) + assert_equal("abc@ogres.net", @user.reload.email_address.address) end end context "with the incorrect password" do should "not work" do post_auth maintenance_user_email_change_path, @user, params: {:email_change => {:password => "passwordx", :email => "abc@ogres.net"}} - @user.reload - assert_equal("bob@ogres.net", @user.email) + assert_equal("bob@ogres.net", @user.reload.email_address.address) end end end diff --git a/test/functional/users_controller_test.rb b/test/functional/users_controller_test.rb index f669a1f51..cec2f97cc 100644 --- a/test/functional/users_controller_test.rb +++ b/test/functional/users_controller_test.rb @@ -114,8 +114,28 @@ class UsersControllerTest < ActionDispatch::IntegrationTest context "create action" do should "create a user" do - assert_difference("User.count", 1) do - post users_path, params: {:user => {:name => "xxx", :password => "xxxxx1", :password_confirmation => "xxxxx1"}} + user_params = { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1" } + post users_path, params: { user: user_params } + + assert_redirected_to User.last + assert_equal("xxx", User.last.name) + end + + should "create a user with a valid email" do + user_params = { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1", email_address_attributes: { address: "test@gmail.com" } } + post users_path, params: { user: user_params } + + assert_redirected_to User.last + assert_equal("xxx", User.last.name) + assert_equal("test@gmail.com", User.last.email_address.address) + end + + should "not create a user with an invalid email" do + user_params = { name: "xxx", password: "xxxxx1", password_confirmation: "xxxxx1", email_address_attributes: { address: "test" } } + + assert_no_difference("User.count") do + post users_path, params: { user: user_params } + assert_response :success end end diff --git a/test/unit/dmail_test.rb b/test/unit/dmail_test.rb index 0814c2181..fcb73a25e 100644 --- a/test/unit/dmail_test.rb +++ b/test/unit/dmail_test.rb @@ -18,7 +18,7 @@ class DmailTest < ActiveSupport::TestCase context "that is spam" do should "be automatically reported and deleted" do @recipient = create(:user) - @spammer = create(:user, created_at: 2.weeks.ago, email: "akismet-guaranteed-spam@example.com") + @spammer = create(:user, created_at: 2.weeks.ago, email_address: build(:email_address, address: "akismet-guaranteed-spam@example.com")) SpamDetector.stubs(:enabled?).returns(true) dmail = create(:dmail, owner: @recipient, from: @spammer, to: @recipient, creator_ip_addr: "127.0.0.1") @@ -87,14 +87,14 @@ class DmailTest < ActiveSupport::TestCase end should "send an email if the user wants it" do - user = create(:user, receive_email_notifications: true) + user = create(:user, receive_email_notifications: true, email_address: build(:email_address)) assert_difference("ActionMailer::Base.deliveries.size", 1) do create(:dmail, to: user, owner: user, body: "test [[tagme]]") end end should "create only one message for a split response" do - user = FactoryBot.create(:user, :receive_email_notifications => true) + user = create(:user, receive_email_notifications: true, email_address: build(:email_address)) assert_difference("ActionMailer::Base.deliveries.size", 1) do Dmail.create_split(from: CurrentUser.user, creator_ip_addr: "127.0.0.1", to_id: user.id, title: "foo", body: "foo") end diff --git a/test/unit/spam_detector.rb b/test/unit/spam_detector.rb index abaffde8e..af994cdb1 100644 --- a/test/unit/spam_detector.rb +++ b/test/unit/spam_detector.rb @@ -7,7 +7,7 @@ class SpamDetectorTest < ActiveSupport::TestCase SpamDetector.stubs(:enabled?).returns(true) @user = create(:gold_user, created_at: 1.month.ago) - @spammer = create(:user, created_at: 2.weeks.ago, email: "akismet-guaranteed-spam@example.com") + @spammer = create(:user, created_at: 2.weeks.ago, email_address: build(:email_address, address: "akismet-guaranteed-spam@example.com")) end context "for dmails" do diff --git a/test/unit/user_deletion_test.rb b/test/unit/user_deletion_test.rb index 292cffaf6..f63d987b1 100644 --- a/test/unit/user_deletion_test.rb +++ b/test/unit/user_deletion_test.rb @@ -27,13 +27,13 @@ class UserDeletionTest < ActiveSupport::TestCase context "a valid user deletion" do setup do - @user = create(:user, email: "ted@danbooru.com") + @user = create(:user, email_address: build(:email_address)) @deletion = UserDeletion.new(@user, "password") end should "blank out the email" do @deletion.delete! - assert_nil(@user.reload.email) + assert_nil(@user.reload.email_address) end should "rename the user" do