Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion app/mailers/user_mailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ class UserMailer < ActionMailer::Base
include Mailable
include LocaleHelper
before_action :attach_images
default from: email_with_name(Figaro.env.email_from, Figaro.env.email_from)
default from: email_with_name(Figaro.env.email_from, Figaro.env.email_from),
reply_to: email_with_name(Figaro.env.email_from, Figaro.env.email_from)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK the Reply-To header isn't useful unless you want replies to be sent somewhere other than the From header.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was actually added for the sake of consistency. As mentioned, our small group of generated emails follows one of two paths: either via our own Rails mailers, or Devise-generated messages. (Both ultimately call mail() in Rails.)

Devise messages include the header by default, and since we're no longer customizing those, it made sense to add it to our own mailers as well, with no discernible cost (and possibly a small boost) to deliverability. Unless we don't like the semantics of a non-reply Reply-To, in which case we would need to again customize the headers. (I'd advise not.)


def email_changed(old_email)
mail(to: old_email, subject: t('mailer.email_change_notice.subject'))
Expand Down
27 changes: 0 additions & 27 deletions config/initializers/devise_mailer_helpers.rb

This file was deleted.

36 changes: 36 additions & 0 deletions spec/config/initializers/active_job_logger_patch_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
require 'rails_helper'

# Covers config/initializers/active_job_logger_patch.rb, which overrides
# ActiveJob::Logging::LogSubscriber to standardize output and prevent sensitive
# user data from being logged.
describe ActiveJob::Logging::LogSubscriber do
it 'overrides the default job logger to output only specified parameters in JSON format' do
class FakeJob < ActiveJob::Base
def perform(sensitive_param:); end
end

# This list corresponds to the initializer's output
permitted_attributes = %w(
timestamp
event_type
job_class
job_queue
job_id
duration
)

# In this case, we need to assert before the action which logs, block-style to
# match the initializer
expect(Rails.logger).to receive(:info) do |&blk|
output = JSON.parse(blk.call)

# [Sidenote: The nested assertions don't seem to be reflected in the spec
# count--perhaps because of the uncommon block format?--but reversing them
# will show them failing as expected.]
output.keys.each { |k| expect(permitted_attributes).to include(k) }
expect(output.keys).to_not include('sensitive_param')
end

FakeJob.perform_later(sensitive_param: '111-22-3333')
end
end
12 changes: 12 additions & 0 deletions spec/features/users/user_edit_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
let(:user) { create(:user, :signed_up) }

context 'editing email' do
let(:new_email) { 'new_email@test.com' }

before do
sign_in_and_2fa_user(user)
visit manage_email_path
Expand All @@ -15,6 +17,16 @@

expect(page).to have_current_path manage_email_path
end

scenario 'user receives confirmation message at new address' do
fill_in 'Email', with: new_email
click_button 'Update'

open_last_email
click_email_link_matching(/confirmation_token/)

expect(page).to have_content(new_email)
end
end

context 'editing 2FA phone number' do
Expand Down
25 changes: 25 additions & 0 deletions spec/features/visitors/password_recovery_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -253,4 +253,29 @@
to(include('style-src \'self\' \'unsafe-inline\''))
end
end

context 'user resets password with unconfirmed email address edit' do
let(:original_email) { 'original_email@test.com' }
let(:new_email) { 'new_email@test.com' }
let(:user) { create(:user, :signed_up, email: original_email) }

before do
sign_in_and_2fa_user(user)
visit manage_email_path
end

it 'receives password reset message at original address' do
fill_in 'Email', with: new_email
click_button 'Update'

expect(page).to have_content t('devise.registrations.email_update_needs_confirmation')

visit sign_out_url
click_link t('links.passwords.forgot')
fill_in 'password_reset_email_form_email', with: original_email
click_button t('forms.buttons.continue')

expect(open_last_email).to be_delivered_to(original_email)
end
end
end