Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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/
  • Loading branch information
aswindevps committed Dec 16, 2021
1 parent e0c9687 commit 5ee209c
Show file tree
Hide file tree
Showing 6 changed files with 51 additions and 8 deletions.
6 changes: 6 additions & 0 deletions app/models/user.rb
Expand Up @@ -177,4 +177,10 @@ def webhook_data
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
4 changes: 2 additions & 2 deletions app/views/devise/mailer/confirmation_instructions.html.erb
@@ -1,13 +1,13 @@
<p>Welcome, <%= @resource.name %>!</p>

<% account_user = @resource&.account_users&.first %>
<% if account_user&.inviter.present? %>
<% if account_user&.inviter.present? && @resource.unconfirmed_email.blank? %>
<p><%= account_user.inviter.name %>, with <%= account_user.account.name %>, has invited you to try out Chatwoot! </p>
<% end %>

<p>You can confirm your account email through the link below:</p>

<% if account_user&.inviter.present? %>
<% if account_user&.inviter.present? && @resource.unconfirmed_email.blank? %>
<p><%= link_to 'Confirm my account', frontend_url('auth/password/edit', reset_password_token: @resource.send(:set_reset_password_token)) %></p>
<% else %>
<p><%= link_to 'Confirm my account', frontend_url('auth/confirmation', confirmation_token: @token) %></p>
Expand Down
2 changes: 1 addition & 1 deletion config/initializers/devise.rb
Expand Up @@ -133,7 +133,7 @@
# 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]
Expand Down
26 changes: 21 additions & 5 deletions spec/controllers/api/v1/profiles_controller_spec.rb
Expand Up @@ -42,19 +42,18 @@
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

expect(response).to have_http_status(:success)
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
Expand Down Expand Up @@ -100,6 +99,23 @@
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
Expand Down
14 changes: 14 additions & 0 deletions spec/mailers/confirmation_instructions_spec.rb
Expand Up @@ -47,5 +47,19 @@
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
7 changes: 7 additions & 0 deletions spec/models/user_spec.rb
Expand Up @@ -80,4 +80,11 @@
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

0 comments on commit 5ee209c

Please sign in to comment.