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 @@ -85,10 +85,11 @@ def signup_with_your_email
end
end

def reset_password_instructions(token:)
def reset_password_instructions(token:, request_id:)
with_user_locale(user) do
@locale = locale_url_param
@token = token
@request_id = request_id
@pending_profile_requires_verification = user.decorate.pending_profile_requires_verification?
@hide_title = @pending_profile_requires_verification
mail(to: email_address.email, subject: t('user_mailer.reset_password_instructions.subject'))
Expand Down
1 change: 1 addition & 0 deletions app/services/request_password_reset.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ def send_reset_password_instructions
token = user.set_reset_password_token
UserMailer.with(user: user, email_address: email_address_record).reset_password_instructions(
token: token,
request_id: request_id,
).deliver_now_or_later

event = PushNotification::RecoveryActivatedEvent.new(user: user)
Expand Down
3 changes: 3 additions & 0 deletions app/views/devise/passwords/edit.html.erb
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
<% title t('titles.passwords.change') %>

<% request_id = params[:request_id] || sp_session[:request_id] %>

<%= render PageHeadingComponent.new.with_content(t('headings.passwords.change')) %>

<p><%= t('instructions.password.password_key') %></p>
Expand All @@ -19,6 +21,7 @@
required: true,
},
) %>
<%= hidden_field_tag('request_id', request_id) %>
<%= render 'devise/shared/password_strength', forbidden_passwords: @forbidden_passwords %>
<%= f.submit t('forms.passwords.edit.buttons.submit'), class: 'display-block margin-y-5' %>
<% end %>
Expand Down
6 changes: 3 additions & 3 deletions app/views/user_mailer/reset_password_instructions.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
<td>
<center>
<%= link_to t('user_mailer.reset_password_instructions.link_text'),
edit_user_password_url(reset_password_token: @token, locale: @locale),
edit_user_password_url(reset_password_token: @token, locale: @locale, request_id: @request_id),
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.

can you provide some background on what linking the request_id does here? does it keep some session around that we need for attempts API?

If so, can we add some sort of acceptance spec that maybe clicks one of these emails and sees the event linked correctly? or something that helps explain why?

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.

Yes! I added this (some other emails also pass request_id in URL and they are logging events) to keep some SP sessions.
Sure let me check I will try to add some tests.
image

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.

currently, events get logged when the user clicks the reset password link from the email within the same browser session, but if a user closes the browser and then clicks the reset password link from the email our system does not log the event for that reason I added request_id in the links.

target: '_blank', class: 'float-center', align: 'center', rel: 'noopener' %>
</center>
</td>
Expand All @@ -48,8 +48,8 @@
</table>

<p>
<%= link_to edit_user_password_url(reset_password_token: @token, locale: @locale),
edit_user_password_url(reset_password_token: @token, locale: @locale),
<%= link_to edit_user_password_url(reset_password_token: @token, locale: @locale, request_id: @request_id),
edit_user_password_url(reset_password_token: @token, locale: @locale, request_id: @request_id),
target: '_blank', rel: 'noopener' %>
</p>

Expand Down
49 changes: 49 additions & 0 deletions spec/features/irs_attempts_api/event_tracking_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -126,4 +126,53 @@
expect(events.count).to eq(0)
end
end

scenario 'reset password from an IRS with new browser session and request_id tracks events' do
freeze_time do
user = create(:user, :signed_up)
visit_idp_from_ial1_oidc_sp(
client_id: service_provider.issuer,
irs_attempts_api_session_id: 'test-session-id',
)

visit root_path
fill_forgot_password_form(user)
set_new_browser_session
click_reset_password_link_from_email
fill_reset_password_form

events = irs_attempts_api_tracked_events(timestamp: Time.zone.now)
expected_event_types = %w[forgot-password-email-sent forgot-password-email-confirmed
forgot-password-new-password-submitted]
received_event_types = events.map(&:event_type)

expect(events.count).to eq received_event_types.count
expect(received_event_types).to match_array(expected_event_types)
end
end

# rubocop:disable Layout/LineLength
scenario 'reset password from an IRS with new browser session and without request_id does not track event' do
freeze_time do
user = create(:user, :signed_up)
visit_idp_from_ial1_oidc_sp(
client_id: service_provider.issuer,
irs_attempts_api_session_id: 'test-session-id',
)

visit root_path
fill_forgot_password_form(user)
set_new_browser_session
click_reset_password_link_from_email
fill_reset_password_form(without_request_id: true)

events = irs_attempts_api_tracked_events(timestamp: Time.zone.now)
expected_event_types = %w[forgot-password-email-sent forgot-password-email-confirmed]
received_event_types = events.map(&:event_type)

expect(events.count).to eq received_event_types.count
expect(received_event_types).to match_array(expected_event_types)
end
end
# rubocop:enable Layout/LineLength
end
2 changes: 1 addition & 1 deletion spec/mailers/previews/user_mailer_preview.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ def signup_with_your_email

def reset_password_instructions
UserMailer.with(user: user, email_address: email_address_record).reset_password_instructions(
token: SecureRandom.hex,
token: SecureRandom.hex, request_id: SecureRandom.hex,
)
end

Expand Down
32 changes: 32 additions & 0 deletions spec/support/features/session_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -657,5 +657,37 @@ def set_new_browser_session
# For when we want to login from a new browser to avoid the default 'remember device' behavior
Capybara.reset_session!
end

def fill_forgot_password_form(user)
click_link t('links.passwords.forgot')
fill_in t('account.index.email'), with: user.email
click_button t('forms.buttons.continue')

expect(current_path).to eq forgot_password_path
end

def click_reset_password_link_from_email
expect(last_email.subject).to eq t('user_mailer.reset_password_instructions.subject')
expect(last_email.html_part.body).to include MarketingSite.help_url
expect(last_email.html_part.body).to have_content(
t(
'user_mailer.reset_password_instructions.footer',
expires: (Devise.reset_password_within / 3600),
),
)
open_last_email
click_email_link_matching(/reset_password_token/)

expect(page.html).not_to include(t('notices.dap_participation'))
expect(current_path).to eq edit_user_password_path
end

def fill_reset_password_form(without_request_id: nil)
fill_in t('forms.passwords.edit.labels.password'), with: 'newVal!dPassw0rd'
find_field('request_id', type: :hidden).set(nil) if without_request_id
click_button t('forms.passwords.edit.buttons.submit')

expect(current_path).to eq new_user_session_path
end
end
end