From 97c257dddf123bb1f595896390621c0402d71e4a Mon Sep 17 00:00:00 2001 From: Chris Rose Date: Sun, 30 Jan 2022 17:28:33 -0800 Subject: [PATCH] bugfix: canonicalize emails for users This allows us to reliably search out user emails for login, and should reduce some of the oddities of logins. Fixes #133 --- Gemfile | 1 + Gemfile.lock | 8 ++++++ app/models/user.rb | 20 ++++++++------- changelog.d/133.bugfix | 1 + config/initializers/email_address.rb | 3 +++ .../20220130232402_canonicalize_email.rb | 17 +++++++++++++ db/structure.sql | 6 +++-- spec/controllers/transfers_controller_spec.rb | 25 ++++++++++++++++--- spec/models/user_spec.rb | 8 ++++++ 9 files changed, 74 insertions(+), 15 deletions(-) create mode 100644 changelog.d/133.bugfix create mode 100644 config/initializers/email_address.rb create mode 100644 db/migrate/20220130232402_canonicalize_email.rb diff --git a/Gemfile b/Gemfile index b904bac0..4db7a23b 100644 --- a/Gemfile +++ b/Gemfile @@ -25,6 +25,7 @@ gem "aws-sdk-s3" # hugo packet is big, let s3 handle the downloads gem "bootsnap" # boot large ruby/rails apps faster gem "bundler-audit" # checks for insecure gems gem "devise" # authentication solution for Rails with Warden +gem "email_address" # canonicalize email addresses gem "gem-licenses" # print libraries depended on by this project, grouped by licence gem "httparty" # high level abstraction for rest integrations gem "jbuilder" # Build JSON APIs with ease. Read more: https://github.com/rails/jbuilder diff --git a/Gemfile.lock b/Gemfile.lock index cf53deaf..379c1969 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -135,6 +135,8 @@ GEM diff-lcs (1.4.4) docile (1.4.0) e2mmap (0.1.0) + email_address (0.2.2) + simpleidn erubi (1.10.0) et-orbi (1.2.6) tzinfo @@ -374,6 +376,8 @@ GEM simplecov_json_formatter (~> 0.1) simplecov-html (0.12.3) simplecov_json_formatter (0.1.3) + simpleidn (0.2.1) + unf (~> 0.1.4) solargraph (0.44.1) backport (~> 1.2) benchmark @@ -418,6 +422,9 @@ GEM tiny_tds (2.1.5) tzinfo (2.0.4) concurrent-ruby (~> 1.0) + unf (0.1.4) + unf_ext + unf_ext (0.0.8) unicode-display_width (2.1.0) warden (1.2.9) rack (>= 2.0.9) @@ -454,6 +461,7 @@ DEPENDENCIES database_cleaner-active_record database_plumber! devise + email_address factory_bot_rails faker gem-licenses diff --git a/app/models/user.rb b/app/models/user.rb index ac6efda8..af357af4 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -33,11 +33,17 @@ class User < ApplicationRecord has_many :reservations, through: :active_claims has_many :carts - # See Cart's validations, one active, pending cart and one active, processing cart at a time. - has_one :active_pending_cart, -> () { active_pending }, class_name: "Cart" - has_one :active_processing_cart, -> () { active_processing }, class_name: "Cart" + # See Cart's validations, one active, pending cart and one active, processing cart at a time. + has_one :active_pending_cart, -> { active_pending }, class_name: "Cart" + has_one :active_processing_cart, -> { active_processing }, class_name: "Cart" + attribute :email, :canonical_email_address + attribute :user_provided_email, :string + def email=(email_address) + self[:user_provided_email] = email_address + self[:email] = email_address + end validates :email, presence: true, uniqueness: true validates :hugo_download_counter, presence: true @@ -55,12 +61,8 @@ def in_stripe? def email_address_format_valid return if email.nil? # covered by presence: true - if !email.match(Devise.email_regexp) - errors.add(:email, "is an unsupported format") - end + errors.add(:email, "is an unsupported format") unless email.match(Devise.email_regexp) - if email.include?("/") - errors.add(:email, "slashes are unsupported") - end + errors.add(:email, "slashes are unsupported") if email.include?("/") end end diff --git a/changelog.d/133.bugfix b/changelog.d/133.bugfix new file mode 100644 index 00000000..aad9c4f0 --- /dev/null +++ b/changelog.d/133.bugfix @@ -0,0 +1 @@ +Canonicalize user emails for login diff --git a/config/initializers/email_address.rb b/config/initializers/email_address.rb new file mode 100644 index 00000000..0b9830b2 --- /dev/null +++ b/config/initializers/email_address.rb @@ -0,0 +1,3 @@ +ActiveRecord::Type.register(:email_address, EmailAddress::EmailAddressType) +ActiveRecord::Type.register(:canonical_email_address, + EmailAddress::CanonicalEmailAddressType) diff --git a/db/migrate/20220130232402_canonicalize_email.rb b/db/migrate/20220130232402_canonicalize_email.rb new file mode 100644 index 00000000..b05e129c --- /dev/null +++ b/db/migrate/20220130232402_canonicalize_email.rb @@ -0,0 +1,17 @@ +class CanonicalizeEmail < ActiveRecord::Migration[6.1] + def up + add_column :users, :user_provided_email, :string + execute "UPDATE users SET user_provided_email = email" + + User.all.each do |user| + curr_email = user.user_provided_email + user.email = curr_email + user.save + end + end + + def down + execute "UPDATE users SET email = user_provided_email" + remove_column :users, :user_provided_email + end +end diff --git a/db/structure.sql b/db/structure.sql index 7b6cb09b..73b83ad9 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -791,7 +791,8 @@ CREATE TABLE public.users ( current_sign_in_ip inet, last_sign_in_ip inet, stripe_id character varying, - hugo_download_counter integer DEFAULT 0 NOT NULL + hugo_download_counter integer DEFAULT 0 NOT NULL, + user_provided_email character varying ); @@ -1547,6 +1548,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20210916230829'), ('20211110022812'), ('20211120232041'), -('20220116010218'); +('20220116010218'), +('20220130232402'); diff --git a/spec/controllers/transfers_controller_spec.rb b/spec/controllers/transfers_controller_spec.rb index dbe6bc37..0d5c28e8 100644 --- a/spec/controllers/transfers_controller_spec.rb +++ b/spec/controllers/transfers_controller_spec.rb @@ -32,7 +32,7 @@ let(:show_update_params) do { id: new_user.email, - reservation_id: reservation.id, + reservation_id: reservation.id } end @@ -73,7 +73,7 @@ describe "#create" do subject(:post_create) do post :create, params: { email: new_user.email, - reservation_id: reservation.id } + reservation_id: reservation.id } end it "bounces you if you're not logged in" do @@ -114,6 +114,23 @@ .to(new_user) end + context "the user email case is different" do + let(:show_update_params) do + { + id: new_user.email.upcase, + reservation_id: reservation.id + } + end + it "transfers between users" do + puts "After setting up the user, before setting up the transfer to #{new_user.email} using #{show_update_params[:id]}" + + expect { update_reservation_transfer } + .to change { reservation.reload.user } + .from(old_user) + .to(new_user) + end + end + it "doesn't copy contact" do expect { update_reservation_transfer }.to_not change { old_user.reload.claims.last.conzealand_contact } expect(new_user.reload.claims.last.conzealand_contact).to be_nil @@ -125,7 +142,7 @@ id: new_user.email, reservation_id: reservation.id, plan_transfer: { - copy_contact: "1", + copy_contact: "1" } } end @@ -142,7 +159,7 @@ expect(MembershipMailer).to_not receive(:transfer) patch :update, params: { id: "invalid email", - reservation_id: reservation.id, + reservation_id: reservation.id } end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 45abd6bd..5e86f6a2 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -25,6 +25,14 @@ expect(build(:user, email: "harry/potter@hogwarts.net")).to_not be_valid end + it "should canonicalize the provided email" do + expect(build(:user, email: "Manual@Mail.com").email).to eq("manual@mail.com") + end + + it "should retain the user provided email" do + expect(build(:user, email: "Manual@Mail.com").user_provided_email).to eq("Manual@Mail.com") + end + # I felt the need to do this because factory code gets quite hairy, especially with factories calling factories from # the :with_reservation trait. describe "user factory links" do