Allow users to update their email address #1745

Merged
Shpigford merged 14 commits from email-update into main 2025-02-01 01:29:49 +08:00
28 changed files with 225 additions and 15 deletions

View File

@@ -0,0 +1,18 @@
class EmailConfirmationsController < ApplicationController
Synxsystemtool commented 2025-01-31 13:14:36 +08:00 (Migrated from github.com)
Review
synxsystem96@gmail.com
zachgoll commented 2025-02-01 01:15:26 +08:00 (Migrated from github.com)
Review

Since we're skipping auth, this should never be true so we can remove it

Since we're skipping auth, this should never be true so we can remove it
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

View File

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

View File

@@ -4,10 +4,23 @@ class UsersController < ApplicationController
def update
zachgoll commented 2025-01-31 05:18:06 +08:00 (Migrated from github.com)
Review

There won't be any errors populated on the user model here for most cases (blank alert), so here and below we should probably just pass a string alert like "Something went wrong"

There won't be any errors populated on the user model here for most cases (blank alert), so here and below we should probably just pass a string alert like "Something went wrong"
@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

View File

@@ -0,0 +1,2 @@
module EmailConfirmationsHelper
end

View File

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

View File

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

View File

@@ -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)
zachgoll commented 2025-01-31 05:44:12 +08:00 (Migrated from github.com)
Review

I think we can remove this method and handle directly in the controller. Then the controller would just be:

def confirm
  # 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 root_path, notice: t(".success_login")
  else
    redirect_to root_path, alert: t(".invalid_token")
  end
end
I think we can remove this method and handle directly in the controller. Then the controller would just be: ```rb def confirm # 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 root_path, notice: t(".success_login") else redirect_to root_path, alert: t(".invalid_token") end end ```
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)

View File

@@ -0,0 +1,7 @@
<h1><%= t(".greeting") %></h1>
<p><%= t(".body") %></p>
<%= link_to @cta, @confirmation_url, class: "button" %>
<p class="footer"><%= t(".expiry_notice", hours: 24) %></p>

View File

@@ -0,0 +1,9 @@
EmailConfirmation#confirmation_email
<%= t(".greeting") %>
<%= t(".body") %>
<%= t(".cta") %>: <%= @confirmation_url %>
<%= t(".expiry_notice", hours: 24) %>

View File

@@ -13,6 +13,20 @@
<% end %>
</div>
<div class="flex items-center justify-between">
<div class="space-y-1">
<p class="text-sm"><%= t(".email_confirmation_title") %></p>
<p class="text-gray-500 text-sm"><%= t(".email_confirmation_description") %></p>
</div>
<%= 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| %>
<div class="relative inline-block select-none">
<%= 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, "&nbsp;".html_safe, class: "maybe-switch" %>
</div>
<% end %>
</div>
<% if Setting.require_invite_for_signup %>
<div class="flex items-center justify-between mb-4">
<div>

View File

@@ -5,10 +5,16 @@
<h1 class="text-gray-900 text-xl font-medium mb-4"><%= t(".page_title") %></h1>
<div class="space-y-4">
<%= 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 %>
<div>
<%= form.email_field :email, placeholder: t(".email"), label: t(".email") %>
<% if @user.unconfirmed_email.present? %>
zachgoll commented 2025-01-31 05:34:17 +08:00 (Migrated from github.com)
Review

From a UX standpoint, I think we need some sort of message underneath this when unconfirmed_email is present:

<p>You have requested to change your email to <%= @user.unconfirmed_email %>.  Please go to your email and confirm for the change to take effect.</p>
From a UX standpoint, I think we need some sort of message underneath this when `unconfirmed_email` is present: ```erb <p>You have requested to change your email to <%= @user.unconfirmed_email %>. Please go to your email and confirm for the change to take effect.</p> ```
<p class="mt-2 text-sm text-gray-600">
You have requested to change your email to <%= @user.unconfirmed_email %>. Please go to your email and confirm for the change to take effect.
</p>
<% end %>
<div class="grid grid-cols-2 gap-4 mt-4">
<%= 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") %>

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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

View File

@@ -9,6 +9,7 @@ Rails.application.routes.draw do
resources :sessions, only: %i[new create destroy]
zachgoll commented 2025-01-31 05:37:44 +08:00 (Migrated from github.com)
Review

Totally semantic and optional, but for consistency with the other routes I think this is a great use case for something like:

resource :email_confirmation, only: :new

Would still use email_confirmations_controller and the new route would be new_email_confirmation_url

Totally semantic and optional, but for consistency with the other routes I think this is a great use case for something like: ```rb resource :email_confirmation, only: :new ``` Would still use `email_confirmations_controller` and the new route would be `new_email_confirmation_url`
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]

View File

@@ -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
zachgoll commented 2025-02-01 01:16:11 +08:00 (Migrated from github.com)
Review

I totally missed this yesterday, but what is :email_confirmation_token for here? It doesn't seem to be used anywhere unless I'm missing something

I totally missed this yesterday, but what is `:email_confirmation_token` for here? It doesn't seem to be used anywhere unless I'm missing something
Shpigford commented 2025-02-01 01:18:23 +08:00 (Migrated from github.com)
Review

Ah, this is leftover from before switching to the rails generate_token_for stuff. Will remove.

Ah, this is leftover from before switching to the rails `generate_token_for` stuff. Will remove.
add_column :users, :email_confirmation_sent_at, :datetime
zachgoll commented 2025-01-31 05:45:26 +08:00 (Migrated from github.com)
Review

We should be able to delete :email_confirmation_sent_at and just let the generates_token_for expiry handle the expiration validation (see my comment above about removing the confirm_email_change method)

We should be able to delete `:email_confirmation_sent_at` and just let the `generates_token_for` expiry handle the expiration validation (see my comment above about removing the `confirm_email_change` method)
add_index :users, :email_confirmation_token, unique: true
end
end

View File

@@ -0,0 +1,5 @@
class RemoveEmailConfirmationSentAtFromUsers < ActiveRecord::Migration[7.2]
def change
remove_column :users, :email_confirmation_sent_at, :datetime
end
end

View File

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

3
db/schema.rb generated
View File

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

View File

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

View File

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

View File

@@ -30,4 +30,13 @@ family_member:
last_name: Dylan
email: jakobdylan@yahoo.com
password_digest: <%= BCrypt::Password.create('password') %>
onboarded_at: <%= 3.days.ago %>
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 %>

View File

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

View File

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

View File

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