jaculabilis/danbooru
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

users: move emails to separate table.

Browse Source 
* Move emails from users table to email_addresses table.
* Validate that addresses are formatted correctly and are unique across
  users. Existing invalid emails are grandfathered in.
* Add is_verified flag (the address has been confirmed by the user).
* Add is_deliverable flag (an undeliverable address is an address that bounces).
* Normalize addresses to prevent registering multiple accounts with the
  same email address (using tricks like Gmail's plus addressing).
This commit is contained in:
evazion
2020-03-10 21:36:16 -05:00
parent 41304d6add
commit 258f4a8b95
22 changed files with 285 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
app/controllers/password_resets_controller.rb
View File

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

8
app/controllers/users_controller.rb
View File

@@ -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)

76
app/logical/email_normalizer.rb Normal file
View File

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

2
app/logical/spam_detector.rb
View File

@@ -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,

2
app/logical/user_deletion.rb
View File

@@ -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 = ''

7
app/logical/user_email_change.rb
View File

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

4
app/mailers/user_mailer.rb
View File

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

2
app/models/dmail.rb
View File

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

15
app/models/email_address.rb Normal file
View File

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

14
app/models/user.rb
View File

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

8
app/views/users/edit.html.erb
View File

@@ -25,7 +25,13 @@
<div class="input">
<label>Email</label>
<p>
<%= CurrentUser.user.email.presence || "<em>blank</em>".html_safe %> – <%= link_to "Change your email", new_maintenance_user_email_change_path %>
<% if @user.email_address.present? %>
<%= @user.email_address.address %>
<% else %>
<em>blank</em>
<% end %>
- <%= link_to "Change your email", new_maintenance_user_email_change_path %>
</p>
</div>

4
app/views/users/new.html.erb
View File

@@ -15,7 +15,9 @@
<div id="p3">
<%= 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 %>

20
db/migrate/20200309043653_create_email_addresses.rb Normal file
View File

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

88
db/structure.sql
View File

@@ -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');

9
script/fixes/063_migrate_emails.rb Executable file
View File

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

6
test/factories/email_address.rb Normal file
View File

@@ -0,0 +1,6 @@
FactoryBot.define do
factory(:email_address) do
address { FFaker::Internet.email }
is_verified { true }
end
end

1
test/factories/user.rb
View File

@@ -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}

8
test/functional/maintenance/user/email_changes_controller_test.rb
View File

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

24
test/functional/users_controller_test.rb
View File

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

6
test/unit/dmail_test.rb
View File

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

2
test/unit/spam_detector.rb
View File

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

4
test/unit/user_deletion_test.rb
View File

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