From 41873de11d1e05e52bf5b03851b5494a30004ff5 Mon Sep 17 00:00:00 2001 From: Josh Pigford Date: Fri, 31 Jan 2025 11:29:49 -0600 Subject: [PATCH] Allow users to update their email address (#1745) * Change email address * Email confirmation * Email change test * Lint * Schema reset * Set test email sender * Select specific user fixture * Refactor/cleanup * Remove unused email_confirmation_token * Current user would never be true * Fix translation test failures --- .../email_confirmations_controller.rb | 18 +++++++++++++ .../settings/hostings_controller.rb | 6 ++++- app/controllers/users_controller.rb | 25 ++++++++++++++--- app/helpers/email_confirmations_helper.rb | 2 ++ app/mailers/email_confirmation_mailer.rb | 15 +++++++++++ app/models/setting.rb | 2 ++ app/models/user.rb | 27 ++++++++++++++++++- .../confirmation_email.html.erb | 7 +++++ .../confirmation_email.text.erb | 9 +++++++ .../hostings/_invite_code_settings.html.erb | 14 ++++++++++ app/views/settings/profiles/show.html.erb | 8 +++++- config/environments/test.rb | 6 +++++ config/locales/views/accounts/en.yml | 4 +++ .../views/email_confirmation_mailer/en.yml | 9 +++++++ config/locales/views/settings/en.yml | 4 +++ config/locales/views/settings/hostings/en.yml | 4 ++- config/locales/views/users/en.yml | 4 ++- config/routes.rb | 1 + ...0191533_add_email_confirmation_to_users.rb | 9 +++++++ ...e_email_confirmation_sent_at_from_users.rb | 5 ++++ ...ove_email_confirmation_token_from_users.rb | 6 +++++ db/schema.rb | 3 ++- .../email_confirmations_controller_test.rb | 12 +++++++++ .../transactions_controller_test.rb | 4 +-- test/fixtures/users.yml | 11 +++++++- .../mailers/email_confirmation_mailer_test.rb | 14 ++++++++++ .../email_confirmation_mailer_preview.rb | 7 +++++ test/models/user_test.rb | 4 +-- 28 files changed, 225 insertions(+), 15 deletions(-) create mode 100644 app/controllers/email_confirmations_controller.rb create mode 100644 app/helpers/email_confirmations_helper.rb create mode 100644 app/mailers/email_confirmation_mailer.rb create mode 100644 app/views/email_confirmation_mailer/confirmation_email.html.erb create mode 100644 app/views/email_confirmation_mailer/confirmation_email.text.erb create mode 100644 config/locales/views/email_confirmation_mailer/en.yml create mode 100644 db/migrate/20250130191533_add_email_confirmation_to_users.rb create mode 100644 db/migrate/20250130214500_remove_email_confirmation_sent_at_from_users.rb create mode 100644 db/migrate/20250131171943_remove_email_confirmation_token_from_users.rb create mode 100644 test/controllers/email_confirmations_controller_test.rb create mode 100644 test/mailers/email_confirmation_mailer_test.rb create mode 100644 test/mailers/previews/email_confirmation_mailer_preview.rb diff --git a/app/controllers/email_confirmations_controller.rb b/app/controllers/email_confirmations_controller.rb new file mode 100644 index 00000000..eb5c3755 --- /dev/null +++ b/app/controllers/email_confirmations_controller.rb @@ -0,0 +1,18 @@ +class EmailConfirmationsController < ApplicationController + skip_before_action :set_request_details, only: :new + skip_authentication only: :new + + def new + # Returns nil if the token is invalid OR expired + @user = User.find_by_token_for(:email_confirmation, params[:token]) + + if @user&.unconfirmed_email && @user&.update( + email: @user.unconfirmed_email, + unconfirmed_email: nil + ) + redirect_to new_session_path, notice: t(".success_login") + else + redirect_to root_path, alert: t(".invalid_token") + end + end +end diff --git a/app/controllers/settings/hostings_controller.rb b/app/controllers/settings/hostings_controller.rb index 222ae018..aae6513c 100644 --- a/app/controllers/settings/hostings_controller.rb +++ b/app/controllers/settings/hostings_controller.rb @@ -22,6 +22,10 @@ class Settings::HostingsController < SettingsController Setting.require_invite_for_signup = hosting_params[:require_invite_for_signup] end + if hosting_params.key?(:require_email_confirmation) + Setting.require_email_confirmation = hosting_params[:require_email_confirmation] + end + if hosting_params.key?(:synth_api_key) Setting.synth_api_key = hosting_params[:synth_api_key] end @@ -34,7 +38,7 @@ class Settings::HostingsController < SettingsController private def hosting_params - params.require(:setting).permit(:render_deploy_hook, :upgrades_setting, :require_invite_for_signup, :synth_api_key) + params.require(:setting).permit(:render_deploy_hook, :upgrades_setting, :require_invite_for_signup, :require_email_confirmation, :synth_api_key) end def raise_if_not_self_hosted diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 55b75581..5bb4f45f 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -4,10 +4,23 @@ class UsersController < ApplicationController def update @user = Current.user - @user.update!(user_params.except(:redirect_to, :delete_profile_image)) - @user.profile_image.purge if should_purge_profile_image? + if email_changed? + if @user.initiate_email_change(user_params[:email]) + if Rails.application.config.app_mode.self_hosted? && !Setting.require_email_confirmation + handle_redirect(t(".success")) + else + redirect_to settings_profile_path, notice: t(".email_change_initiated") + end + else + error_message = @user.errors.any? ? @user.errors.full_messages.to_sentence : t(".email_change_failed") + redirect_to settings_profile_path, alert: error_message + end + else + @user.update!(user_params.except(:redirect_to, :delete_profile_image)) + @user.profile_image.purge if should_purge_profile_image? - handle_redirect(t(".success")) + handle_redirect(t(".success")) + end end def destroy @@ -38,9 +51,13 @@ class UsersController < ApplicationController user_params[:profile_image].blank? end + def email_changed? + user_params[:email].present? && user_params[:email] != @user.email + end + def user_params params.require(:user).permit( - :first_name, :last_name, :profile_image, :redirect_to, :delete_profile_image, :onboarded_at, + :first_name, :last_name, :email, :profile_image, :redirect_to, :delete_profile_image, :onboarded_at, family_attributes: [ :name, :currency, :country, :locale, :date_format, :timezone, :id, :data_enrichment_enabled ] ) end diff --git a/app/helpers/email_confirmations_helper.rb b/app/helpers/email_confirmations_helper.rb new file mode 100644 index 00000000..c5f58449 --- /dev/null +++ b/app/helpers/email_confirmations_helper.rb @@ -0,0 +1,2 @@ +module EmailConfirmationsHelper +end diff --git a/app/mailers/email_confirmation_mailer.rb b/app/mailers/email_confirmation_mailer.rb new file mode 100644 index 00000000..3ad99b96 --- /dev/null +++ b/app/mailers/email_confirmation_mailer.rb @@ -0,0 +1,15 @@ +class EmailConfirmationMailer < ApplicationMailer + # Subject can be set in your I18n file at config/locales/en.yml + # with the following lookup: + # + # en.email_confirmation_mailer.confirmation_email.subject + # + def confirmation_email + @user = params[:user] + @subject = t(".subject") + @cta = t(".cta") + @confirmation_url = new_email_confirmation_url(token: @user.generate_token_for(:email_confirmation)) + + mail to: @user.unconfirmed_email, subject: @subject + end +end diff --git a/app/models/setting.rb b/app/models/setting.rb index d576fbea..41355fee 100644 --- a/app/models/setting.rb +++ b/app/models/setting.rb @@ -20,4 +20,6 @@ class Setting < RailsSettings::Base field :synth_api_key, type: :string, default: ENV["SYNTH_API_KEY"] field :require_invite_for_signup, type: :boolean, default: false + + field :require_email_confirmation, type: :boolean, default: ENV.fetch("REQUIRE_EMAIL_CONFIRMATION", "true") == "true" end diff --git a/app/models/user.rb b/app/models/user.rb index 0d50ba87..92171040 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -7,9 +7,10 @@ class User < ApplicationRecord has_many :impersonated_support_sessions, class_name: "ImpersonationSession", foreign_key: :impersonated_id, dependent: :destroy accepts_nested_attributes_for :family, update_only: true - validates :email, presence: true, uniqueness: true + validates :email, presence: true, uniqueness: true, format: { with: URI::MailTo::EMAIL_REGEXP } validate :ensure_valid_profile_image normalizes :email, with: ->(email) { email.strip.downcase } + normalizes :unconfirmed_email, with: ->(email) { email&.strip&.downcase } normalizes :first_name, :last_name, with: ->(value) { value.strip.presence } @@ -25,6 +26,30 @@ class User < ApplicationRecord password_salt&.last(10) end + generates_token_for :email_confirmation, expires_in: 1.day do + unconfirmed_email + end + + def pending_email_change? + unconfirmed_email.present? + end + + def initiate_email_change(new_email) + return false if new_email == email + return false if new_email == unconfirmed_email + + if Rails.application.config.app_mode.self_hosted? && !Setting.require_email_confirmation + update(email: new_email) + else + if update(unconfirmed_email: new_email) + EmailConfirmationMailer.with(user: self).confirmation_email.deliver_later + true + else + false + end + end + end + def request_impersonation_for(user_id) impersonated = User.find(user_id) impersonator_support_sessions.create!(impersonated: impersonated) diff --git a/app/views/email_confirmation_mailer/confirmation_email.html.erb b/app/views/email_confirmation_mailer/confirmation_email.html.erb new file mode 100644 index 00000000..be87ccd0 --- /dev/null +++ b/app/views/email_confirmation_mailer/confirmation_email.html.erb @@ -0,0 +1,7 @@ +

<%= t(".greeting") %>

+ +

<%= t(".body") %>

+ +<%= link_to @cta, @confirmation_url, class: "button" %> + +

<%= t(".expiry_notice", hours: 24) %>

\ No newline at end of file diff --git a/app/views/email_confirmation_mailer/confirmation_email.text.erb b/app/views/email_confirmation_mailer/confirmation_email.text.erb new file mode 100644 index 00000000..8f5fa14b --- /dev/null +++ b/app/views/email_confirmation_mailer/confirmation_email.text.erb @@ -0,0 +1,9 @@ +EmailConfirmation#confirmation_email + +<%= t(".greeting") %> + +<%= t(".body") %> + +<%= t(".cta") %>: <%= @confirmation_url %> + +<%= t(".expiry_notice", hours: 24) %> \ No newline at end of file diff --git a/app/views/settings/hostings/_invite_code_settings.html.erb b/app/views/settings/hostings/_invite_code_settings.html.erb index e9889d75..de3b1e22 100644 --- a/app/views/settings/hostings/_invite_code_settings.html.erb +++ b/app/views/settings/hostings/_invite_code_settings.html.erb @@ -13,6 +13,20 @@ <% end %> +
+
+

<%= t(".email_confirmation_title") %>

+

<%= t(".email_confirmation_description") %>

+
+ + <%= styled_form_with model: Setting.new, url: settings_hosting_path, method: :patch, data: { controller: "auto-submit-form", "auto-submit-form-trigger-event-value" => "blur" } do |form| %> +
+ <%= form.check_box :require_email_confirmation, class: "sr-only peer", "data-auto-submit-form-target": "auto", "data-autosubmit-trigger-event": "input", disabled: !Current.user.admin? %> + <%= form.label :require_email_confirmation, " ".html_safe, class: "maybe-switch" %> +
+ <% end %> +
+ <% if Setting.require_invite_for_signup %>
diff --git a/app/views/settings/profiles/show.html.erb b/app/views/settings/profiles/show.html.erb index 5f0d5e10..d0ffb6da 100644 --- a/app/views/settings/profiles/show.html.erb +++ b/app/views/settings/profiles/show.html.erb @@ -5,10 +5,16 @@

<%= t(".page_title") %>

<%= settings_section title: t(".profile_title"), subtitle: t(".profile_subtitle") do %> - <%= styled_form_with model: @user, class: "space-y-4" do |form| %> + <%= styled_form_with model: @user, url: user_path(@user), class: "space-y-4" do |form| %> <%= render "settings/user_avatar_field", form: form, user: @user %>
+ <%= form.email_field :email, placeholder: t(".email"), label: t(".email") %> + <% if @user.unconfirmed_email.present? %> +

+ You have requested to change your email to <%= @user.unconfirmed_email %>. Please go to your email and confirm for the change to take effect. +

+ <% end %>
<%= form.text_field :first_name, placeholder: t(".first_name"), label: t(".first_name") %> <%= form.text_field :last_name, placeholder: t(".last_name"), label: t(".last_name") %> diff --git a/config/environments/test.rb b/config/environments/test.rb index 37637b90..23bd03f3 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -18,10 +18,14 @@ Rails.application.configure do config.eager_load = ENV["CI"].present? # Configure public file server for tests with Cache-Control for performance. + config.public_file_server.enabled = true config.public_file_server.headers = { "Cache-Control" => "public, max-age=#{1.hour.to_i}" } + # Set default sender email for tests + ENV["EMAIL_SENDER"] = "hello@maybefinance.com" + # Show full error reports and disable caching. config.consider_all_requests_local = true config.action_controller.perform_caching = false @@ -69,4 +73,6 @@ Rails.application.configure do config.active_record.encryption.encrypt_fixtures = true config.autoload_paths += %w[test/support] + + config.action_mailer.default_url_options = { host: "example.com" } end diff --git a/config/locales/views/accounts/en.yml b/config/locales/views/accounts/en.yml index 94aa32b2..9162acdd 100644 --- a/config/locales/views/accounts/en.yml +++ b/config/locales/views/accounts/en.yml @@ -73,3 +73,7 @@ en: or entering manually. update: success: "%{type} account updated" + email_confirmations: + new: + success_login: "Your email has been confirmed. Please log in with your new email address." + invalid_token: "Invalid or expired confirmation link." diff --git a/config/locales/views/email_confirmation_mailer/en.yml b/config/locales/views/email_confirmation_mailer/en.yml new file mode 100644 index 00000000..110746a8 --- /dev/null +++ b/config/locales/views/email_confirmation_mailer/en.yml @@ -0,0 +1,9 @@ +--- +en: + email_confirmation_mailer: + confirmation_email: + subject: "Maybe: Confirm your email change" + greeting: "Hello!" + body: "You recently requested to change your email address. Click the button below to confirm this change." + cta: "Confirm email change" + expiry_notice: "This link will expire in %{hours} hours." \ No newline at end of file diff --git a/config/locales/views/settings/en.yml b/config/locales/views/settings/en.yml index 5bfa97d4..c082139c 100644 --- a/config/locales/views/settings/en.yml +++ b/config/locales/views/settings/en.yml @@ -79,6 +79,7 @@ en: invitation_link: Invitation link invite_member: Add member last_name: Last Name + email: Email page_title: Account pending: Pending profile_subtitle: Customize how you appear on Maybe @@ -87,3 +88,6 @@ en: user_avatar_field: accepted_formats: JPG or PNG. 5MB max. choose: Choose + users: + update: + success: Profile updated successfully diff --git a/config/locales/views/settings/hostings/en.yml b/config/locales/views/settings/hostings/en.yml index 90a89fd7..06334a31 100644 --- a/config/locales/views/settings/hostings/en.yml +++ b/config/locales/views/settings/hostings/en.yml @@ -7,7 +7,9 @@ en: so via an invite code generate_tokens: Generate new code generated_tokens: Generated codes - title: Require invite code for new sign ups + title: Require invite code for signup + email_confirmation_title: Require email confirmation + email_confirmation_description: When enabled, users must confirm their email address when changing it. provider_settings: description: Configure settings for your hosting provider render_deploy_hook_label: Render Deploy Hook URL diff --git a/config/locales/views/users/en.yml b/config/locales/views/users/en.yml index 6b488278..5eaf6f63 100644 --- a/config/locales/views/users/en.yml +++ b/config/locales/views/users/en.yml @@ -4,4 +4,6 @@ en: destroy: success: Your account has been deleted. update: - success: Your profile has been updated. + success: "Your profile has been updated." + email_change_initiated: "Please check your new email address for confirmation instructions." + email_change_failed: "Failed to change email address." diff --git a/config/routes.rb b/config/routes.rb index d83ac11b..33d534a2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -9,6 +9,7 @@ Rails.application.routes.draw do resources :sessions, only: %i[new create destroy] resource :password_reset, only: %i[new create edit update] resource :password, only: %i[edit update] + resource :email_confirmation, only: :new resources :users, only: %i[update destroy] diff --git a/db/migrate/20250130191533_add_email_confirmation_to_users.rb b/db/migrate/20250130191533_add_email_confirmation_to_users.rb new file mode 100644 index 00000000..ac929b48 --- /dev/null +++ b/db/migrate/20250130191533_add_email_confirmation_to_users.rb @@ -0,0 +1,9 @@ +class AddEmailConfirmationToUsers < ActiveRecord::Migration[7.2] + def change + add_column :users, :unconfirmed_email, :string + add_column :users, :email_confirmation_token, :string + add_column :users, :email_confirmation_sent_at, :datetime + + add_index :users, :email_confirmation_token, unique: true + end +end diff --git a/db/migrate/20250130214500_remove_email_confirmation_sent_at_from_users.rb b/db/migrate/20250130214500_remove_email_confirmation_sent_at_from_users.rb new file mode 100644 index 00000000..89533213 --- /dev/null +++ b/db/migrate/20250130214500_remove_email_confirmation_sent_at_from_users.rb @@ -0,0 +1,5 @@ +class RemoveEmailConfirmationSentAtFromUsers < ActiveRecord::Migration[7.2] + def change + remove_column :users, :email_confirmation_sent_at, :datetime + end +end diff --git a/db/migrate/20250131171943_remove_email_confirmation_token_from_users.rb b/db/migrate/20250131171943_remove_email_confirmation_token_from_users.rb new file mode 100644 index 00000000..db1db8b8 --- /dev/null +++ b/db/migrate/20250131171943_remove_email_confirmation_token_from_users.rb @@ -0,0 +1,6 @@ +class RemoveEmailConfirmationTokenFromUsers < ActiveRecord::Migration[7.2] + def change + remove_index :users, :email_confirmation_token + remove_column :users, :email_confirmation_token, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 54a65580..8efce7d1 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2025_01_28_203303) do +ActiveRecord::Schema[7.2].define(version: 2025_01_31_171943) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -661,6 +661,7 @@ ActiveRecord::Schema[7.2].define(version: 2025_01_28_203303) do t.string "role", default: "member", null: false t.boolean "active", default: true, null: false t.datetime "onboarded_at" + t.string "unconfirmed_email" t.index ["email"], name: "index_users_on_email", unique: true t.index ["family_id"], name: "index_users_on_family_id" end diff --git a/test/controllers/email_confirmations_controller_test.rb b/test/controllers/email_confirmations_controller_test.rb new file mode 100644 index 00000000..beee4f73 --- /dev/null +++ b/test/controllers/email_confirmations_controller_test.rb @@ -0,0 +1,12 @@ +require "test_helper" + +class EmailConfirmationsControllerTest < ActionDispatch::IntegrationTest + test "should get confirm" do + user = users(:new_email) + user.update!(unconfirmed_email: "new@example.com") + token = user.generate_token_for(:email_confirmation) + + get new_email_confirmation_path(token: token) + assert_redirected_to new_session_path + end +end diff --git a/test/controllers/transactions_controller_test.rb b/test/controllers/transactions_controller_test.rb index e4920819..73492e6e 100644 --- a/test/controllers/transactions_controller_test.rb +++ b/test/controllers/transactions_controller_test.rb @@ -10,7 +10,7 @@ class TransactionsControllerTest < ActionDispatch::IntegrationTest test "transaction count represents filtered total" do family = families(:empty) - sign_in family.users.first + sign_in users(:empty) account = family.accounts.create! name: "Test", balance: 0, currency: "USD", accountable: Depository.new 3.times do @@ -32,7 +32,7 @@ class TransactionsControllerTest < ActionDispatch::IntegrationTest test "can paginate" do family = families(:empty) - sign_in family.users.first + sign_in users(:empty) account = family.accounts.create! name: "Test", balance: 0, currency: "USD", accountable: Depository.new 11.times do diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index d7e7444d..246fdf56 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -30,4 +30,13 @@ family_member: last_name: Dylan email: jakobdylan@yahoo.com password_digest: <%= BCrypt::Password.create('password') %> - onboarded_at: <%= 3.days.ago %> \ No newline at end of file + onboarded_at: <%= 3.days.ago %> + +new_email: + family: empty + first_name: Test + last_name: User + email: user@example.com + unconfirmed_email: new@example.com + password_digest: <%= BCrypt::Password.create('password123') %> + onboarded_at: <%= Time.current %> \ No newline at end of file diff --git a/test/mailers/email_confirmation_mailer_test.rb b/test/mailers/email_confirmation_mailer_test.rb new file mode 100644 index 00000000..2727cfb9 --- /dev/null +++ b/test/mailers/email_confirmation_mailer_test.rb @@ -0,0 +1,14 @@ +require "test_helper" + +class EmailConfirmationMailerTest < ActionMailer::TestCase + test "confirmation_email" do + user = users(:new_email) + user.unconfirmed_email = "new@example.com" + + mail = EmailConfirmationMailer.with(user: user).confirmation_email + assert_equal I18n.t("email_confirmation_mailer.confirmation_email.subject"), mail.subject + assert_equal [ user.unconfirmed_email ], mail.to + assert_equal [ "hello@maybefinance.com" ], mail.from + assert_match "confirm", mail.body.encoded + end +end diff --git a/test/mailers/previews/email_confirmation_mailer_preview.rb b/test/mailers/previews/email_confirmation_mailer_preview.rb new file mode 100644 index 00000000..36aa4648 --- /dev/null +++ b/test/mailers/previews/email_confirmation_mailer_preview.rb @@ -0,0 +1,7 @@ +# Preview all emails at http://localhost:3000/rails/mailers/email_confirmation_mailer +class EmailConfirmationMailerPreview < ActionMailer::Preview + # Preview this email at http://localhost:3000/rails/mailers/email_confirmation_mailer/confirmation_email + def confirmation_email + EmailConfirmationMailer.confirmation_email + end +end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 582fc2e8..dde2c44e 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -38,8 +38,8 @@ class UserTest < ActiveSupport::TestCase end test "email address is normalized" do - @user.update!(email: " User@ExAMPle.CoM ") - assert_equal "user@example.com", @user.reload.email + @user.update!(email: " UNIQUE-User@ExAMPle.CoM ") + assert_equal "unique-user@example.com", @user.reload.email end test "display name" do