From 5ee209c079fedf8f76e2054dac568c58257b0d9a Mon Sep 17 00:00:00 2001 From: "Aswin Dev P.S" Date: Thu, 16 Dec 2021 06:02:49 -0800 Subject: [PATCH] chore: Fix user email re-confirmation flow (#3581) Users can change their email from profile settings. They will be logged out immediately. Users can log in again with the updated email without verifying the same. This is a security problem. So this change enforce the user to reconfirm the email after changing it. Users can log in with the updated email only after the confirmation. Fixes: https://huntr.dev/bounties/7afd04b4-232e-4907-8a3c-acf8bd4b5b22/ --- app/models/user.rb | 6 +++++ .../mailer/confirmation_instructions.html.erb | 4 +-- config/initializers/devise.rb | 2 +- .../api/v1/profiles_controller_spec.rb | 26 +++++++++++++++---- .../mailers/confirmation_instructions_spec.rb | 14 ++++++++++ spec/models/user_spec.rb | 7 +++++ 6 files changed, 51 insertions(+), 8 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 81a4f6cd0..16798904c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -177,4 +177,10 @@ class User < ApplicationRecord type: 'user' } end + + # https://github.com/lynndylanhurley/devise_token_auth/blob/6d7780ee0b9750687e7e2871b9a1c6368f2085a9/app/models/devise_token_auth/concerns/user.rb#L45 + # Since this method is overriden in devise_token_auth it breaks the email reconfirmation flow. + def will_save_change_to_email? + mutations_from_database.changed?('email') + end end diff --git a/app/views/devise/mailer/confirmation_instructions.html.erb b/app/views/devise/mailer/confirmation_instructions.html.erb index cf956463e..f5297932a 100644 --- a/app/views/devise/mailer/confirmation_instructions.html.erb +++ b/app/views/devise/mailer/confirmation_instructions.html.erb @@ -1,13 +1,13 @@

Welcome, <%= @resource.name %>!

<% account_user = @resource&.account_users&.first %> -<% if account_user&.inviter.present? %> +<% if account_user&.inviter.present? && @resource.unconfirmed_email.blank? %>

<%= account_user.inviter.name %>, with <%= account_user.account.name %>, has invited you to try out Chatwoot!

<% end %>

You can confirm your account email through the link below:

-<% if account_user&.inviter.present? %> +<% if account_user&.inviter.present? && @resource.unconfirmed_email.blank? %>

<%= link_to 'Confirm my account', frontend_url('auth/password/edit', reset_password_token: @resource.send(:set_reset_password_token)) %>

<% else %>

<%= link_to 'Confirm my account', frontend_url('auth/confirmation', confirmation_token: @token) %>

diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 134005945..d9d1802c3 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -133,7 +133,7 @@ Devise.setup do |config| # initial account confirmation) to be applied. Requires additional unconfirmed_email # db field (see migrations). Until confirmed, new email is stored in # unconfirmed_email column, and copied to email column on successful confirmation. - config.reconfirmable = false + config.reconfirmable = true # Defines which key will be used when confirming an account # config.confirmation_keys = [:email] diff --git a/spec/controllers/api/v1/profiles_controller_spec.rb b/spec/controllers/api/v1/profiles_controller_spec.rb index 961d5c091..39a936a90 100644 --- a/spec/controllers/api/v1/profiles_controller_spec.rb +++ b/spec/controllers/api/v1/profiles_controller_spec.rb @@ -42,10 +42,9 @@ RSpec.describe 'Profile API', type: :request do context 'when it is an authenticated user' do let(:agent) { create(:user, password: 'Test123!', account: account, role: :agent) } - it 'updates the name & email' do - new_email = Faker::Internet.email + it 'updates the name' do put '/api/v1/profile', - params: { profile: { name: 'test', email: new_email } }, + params: { profile: { name: 'test' } }, headers: agent.create_new_auth_token, as: :json @@ -53,8 +52,8 @@ RSpec.describe 'Profile API', type: :request do json_response = JSON.parse(response.body) agent.reload expect(json_response['id']).to eq(agent.id) - expect(json_response['email']).to eq(agent.email) - expect(agent.email).to eq(new_email) + expect(json_response['name']).to eq(agent.name) + expect(agent.name).to eq('test') end it 'updates the password when current password is provided' do @@ -100,6 +99,23 @@ RSpec.describe 'Profile API', type: :request do expect(json_response['ui_settings']['is_contact_sidebar_open']).to eq(false) end end + + context 'when an authenticated user updates email' do + let(:agent) { create(:user, password: 'Test123!', account: account, role: :agent) } + + it 'populates the unconfirmed email' do + new_email = Faker::Internet.email + put '/api/v1/profile', + params: { profile: { email: new_email } }, + headers: agent.create_new_auth_token, + as: :json + + expect(response).to have_http_status(:success) + agent.reload + + expect(agent.unconfirmed_email).to eq(new_email) + end + end end describe 'DELETE /api/v1/profile/avatar' do diff --git a/spec/mailers/confirmation_instructions_spec.rb b/spec/mailers/confirmation_instructions_spec.rb index e322d9351..aa75627bb 100644 --- a/spec/mailers/confirmation_instructions_spec.rb +++ b/spec/mailers/confirmation_instructions_spec.rb @@ -47,5 +47,19 @@ RSpec.describe 'Confirmation Instructions', type: :mailer do expect(mail.body).not_to include('app/auth/confirmation') end end + + context 'when user updates the email' do + before do + confirmable_user.update!(email: 'user@example.com') + end + + it 'sends a confirmation link' do + confirmation_mail = Devise::Mailer.confirmation_instructions(confirmable_user.reload, nil, {}) + + expect(confirmation_mail.body).to include('app/auth/confirmation?confirmation_token') + expect(confirmation_mail.body).not_to include('app/auth/password/edit') + expect(confirmable_user.unconfirmed_email.blank?).to be false + end + end end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 674525a6f..e9923cd38 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -80,4 +80,11 @@ RSpec.describe User do expect(token_count).to eq(1) end end + + context 'when user changes the email' do + it 'mutates the value' do + user.email = 'user@example.com' + expect(user.will_save_change_to_email?).to be true + end + end end