From 0bfab084a811d7dad1f1929ee7b5c2bc59015173 Mon Sep 17 00:00:00 2001 From: Vinnie Okada Date: Sun, 26 Apr 2015 22:04:33 -0600 Subject: [PATCH 1/4] Explain reset token expiration in emails Tell new users when their password reset token expires and provide a link to get a new one. --- CHANGELOG | 1 + app/helpers/emails_helper.rb | 19 ++++++++++ app/views/notify/new_user_email.html.haml | 2 + app/views/notify/new_user_email.text.erb | 2 + spec/helpers/emails_helper_spec.rb | 46 +++++++++++++++++++++++ spec/mailers/notify_spec.rb | 5 +++ 6 files changed, 75 insertions(+) create mode 100644 spec/helpers/emails_helper_spec.rb diff --git a/CHANGELOG b/CHANGELOG index ade877feb9..15bfe570f1 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -32,6 +32,7 @@ v 7.11.0 (unreleased) - Show Atom feed buttons everywhere where applicable. - Add project activity atom feed. - Don't crash when an MR from a fork has a cross-reference comment from the target project on one of its commits. + - Explain how to get a new password reset token in welcome emails - Include commit comments in MR from a forked project. - Fix adding new group members from admin area - Group milestones by title in the dashboard and all other issue views. diff --git a/app/helpers/emails_helper.rb b/app/helpers/emails_helper.rb index 0df3ecc90b..12aa561a14 100644 --- a/app/helpers/emails_helper.rb +++ b/app/helpers/emails_helper.rb @@ -35,4 +35,23 @@ module EmailsHelper lexer = Rugments::Lexers::Diff.new raw formatter.format(lexer.lex(diffcontent)) end + + def password_reset_token_valid_time + valid_hours = Devise.reset_password_within / 60 / 60 + if valid_hours >= 24 + unit = 'day' + valid_length = (valid_hours / 24).floor + else + unit = 'hour' + valid_length = valid_hours.floor + end + + pluralize(valid_length, unit) + end + + def reset_token_expire_message + link_tag = link_to('request a new one', new_user_password_url) + msg = "This link is valid for #{password_reset_token_valid_time}. " + msg << "After it expires, you can #{link_tag}." + end end diff --git a/app/views/notify/new_user_email.html.haml b/app/views/notify/new_user_email.html.haml index ebbe98dd47..39cb01d4d2 100644 --- a/app/views/notify/new_user_email.html.haml +++ b/app/views/notify/new_user_email.html.haml @@ -12,3 +12,5 @@ - if @user.created_by_id %p = link_to "Click here to set your password", edit_password_url(@user, :reset_password_token => @token) + %p + = reset_token_expire_message diff --git a/app/views/notify/new_user_email.text.erb b/app/views/notify/new_user_email.text.erb index 96b26879a7..dd9b71e3b8 100644 --- a/app/views/notify/new_user_email.text.erb +++ b/app/views/notify/new_user_email.text.erb @@ -5,4 +5,6 @@ The Administrator created an account for you. Now you are a member of the compan login.................. <%= @user.email %> <% if @user.created_by_id %> <%= link_to "Click here to set your password", edit_password_url(@user, :reset_password_token => @token) %> + + <%= reset_token_expire_message %> <% end %> diff --git a/spec/helpers/emails_helper_spec.rb b/spec/helpers/emails_helper_spec.rb new file mode 100644 index 0000000000..7a3e38d7e6 --- /dev/null +++ b/spec/helpers/emails_helper_spec.rb @@ -0,0 +1,46 @@ +require 'spec_helper' + +describe EmailsHelper do + describe 'password_reset_token_valid_time' do + def validate_time_string(time_limit, expected_string) + Devise.reset_password_within = time_limit + expect(password_reset_token_valid_time).to eq(expected_string) + end + + context 'when time limit is less than 2 hours' do + it 'should display the time in hours using a singular unit' do + validate_time_string(1.hour, '1 hour') + end + end + + context 'when time limit is 2 or more hours' do + it 'should display the time in hours using a plural unit' do + validate_time_string(2.hours, '2 hours') + end + end + + context 'when time limit contains fractions of an hour' do + it 'should round down to the nearest hour' do + validate_time_string(96.minutes, '1 hour') + end + end + + context 'when time limit is 24 or more hours' do + it 'should display the time in days using a singular unit' do + validate_time_string(24.hours, '1 day') + end + end + + context 'when time limit is 2 or more days' do + it 'should display the time in days using a plural unit' do + validate_time_string(2.days, '2 days') + end + end + + context 'when time limit contains fractions of a day' do + it 'should round down to the nearest day' do + validate_time_string(57.hours, '2 days') + end + end + end +end diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index dbcf7286e4..4da91eea98 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -91,6 +91,11 @@ describe Notify do it 'includes a link to the site' do is_expected.to have_body_text /#{example_site_path}/ end + + it 'explains the reset link expiration' do + is_expected.to have_body_text(/This link is valid for \d+ (hours?|days?)/) + is_expected.to have_body_text(new_user_password_url) + end end From c68c23210bdf9f0d7212fa55e7bef71ac0f87bcf Mon Sep 17 00:00:00 2001 From: Vinnie Okada Date: Wed, 13 May 2015 20:29:15 -0600 Subject: [PATCH 2/4] Redirect if password reset token is expired Don't display the password editing form if the user's token is expired; redirect to the form that allows users to request a new password reset token. --- app/controllers/passwords_controller.rb | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb index 88459d4080..fbb9d371a7 100644 --- a/app/controllers/passwords_controller.rb +++ b/app/controllers/passwords_controller.rb @@ -36,4 +36,24 @@ class PasswordsController < Devise::PasswordsController end end end + + def edit + super + reset_password_token = Devise.token_generator.digest( + User, + :reset_password_token, + resource.reset_password_token + ) + + unless reset_password_token.nil? + user = User.where( + reset_password_token: reset_password_token + ).first_or_initialize + + unless user.reset_password_period_valid? + flash[:alert] = 'Your password reset token has expired.' + redirect_to(new_user_password_url) + end + end + end end From af428b12598f06073327bc63d75d9c358c95067a Mon Sep 17 00:00:00 2001 From: Vinnie Okada Date: Wed, 13 May 2015 21:57:16 -0600 Subject: [PATCH 3/4] Fill in email on the new password form --- app/controllers/passwords_controller.rb | 2 +- app/helpers/emails_helper.rb | 2 +- app/views/devise/passwords/new.html.haml | 2 +- app/views/notify/new_user_email.html.haml | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb index fbb9d371a7..145f27b67d 100644 --- a/app/controllers/passwords_controller.rb +++ b/app/controllers/passwords_controller.rb @@ -52,7 +52,7 @@ class PasswordsController < Devise::PasswordsController unless user.reset_password_period_valid? flash[:alert] = 'Your password reset token has expired.' - redirect_to(new_user_password_url) + redirect_to(new_user_password_url(user_email: user['email'])) end end end diff --git a/app/helpers/emails_helper.rb b/app/helpers/emails_helper.rb index 12aa561a14..128de18bc4 100644 --- a/app/helpers/emails_helper.rb +++ b/app/helpers/emails_helper.rb @@ -50,7 +50,7 @@ module EmailsHelper end def reset_token_expire_message - link_tag = link_to('request a new one', new_user_password_url) + link_tag = link_to('request a new one', new_user_password_url(user_email: @user.email)) msg = "This link is valid for #{password_reset_token_valid_time}. " msg << "After it expires, you can #{link_tag}." end diff --git a/app/views/devise/passwords/new.html.haml b/app/views/devise/passwords/new.html.haml index e8820daf58..29ffe8a8be 100644 --- a/app/views/devise/passwords/new.html.haml +++ b/app/views/devise/passwords/new.html.haml @@ -6,7 +6,7 @@ .devise-errors = devise_error_messages! .clearfix.append-bottom-20 - = f.email_field :email, placeholder: "Email", class: "form-control", required: true + = f.email_field :email, placeholder: "Email", class: "form-control", required: true, value: params[:user_email] .clearfix = f.submit "Reset password", class: "btn-primary btn" diff --git a/app/views/notify/new_user_email.html.haml b/app/views/notify/new_user_email.html.haml index 39cb01d4d2..4feacdaacf 100644 --- a/app/views/notify/new_user_email.html.haml +++ b/app/views/notify/new_user_email.html.haml @@ -11,6 +11,6 @@ - if @user.created_by_id %p - = link_to "Click here to set your password", edit_password_url(@user, :reset_password_token => @token) + = link_to "Click here to set your password", edit_password_url(@user, reset_password_token: @token) %p = reset_token_expire_message From 56e06b03bfb76a6fea263940cd4512276c3225c8 Mon Sep 17 00:00:00 2001 From: Vinnie Okada Date: Wed, 13 May 2015 22:21:34 -0600 Subject: [PATCH 4/4] Add tests for password reset token changes --- spec/mailers/notify_spec.rb | 57 ++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 33 deletions(-) diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 4da91eea98..37607b55eb 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -5,6 +5,8 @@ describe Notify do include EmailSpec::Matchers include RepoHelpers + new_user_address = 'newguy@example.com' + let(:gitlab_sender_display_name) { Gitlab.config.gitlab.email_display_name } let(:gitlab_sender) { Gitlab.config.gitlab.email_from } let(:gitlab_sender_reply_to) { Gitlab.config.gitlab.email_reply_to } @@ -55,18 +57,9 @@ describe Notify do end end - describe 'for new users, the email' do - let(:example_site_path) { root_path } - let(:new_user) { create(:user, email: 'newguy@example.com', created_by_id: 1) } - - token = 'kETLwRaayvigPq_x3SNM' - - subject { Notify.new_user_email(new_user.id, token) } - - it_behaves_like 'an email sent from GitLab' - + shared_examples 'a new user email' do |user_email, site_path| it 'is sent to the new user' do - is_expected.to deliver_to new_user.email + is_expected.to deliver_to user_email end it 'has the correct subject' do @@ -74,9 +67,25 @@ describe Notify do end it 'contains the new user\'s login name' do - is_expected.to have_body_text /#{new_user.email}/ + is_expected.to have_body_text /#{user_email}/ end + it 'includes a link to the site' do + is_expected.to have_body_text /#{site_path}/ + end + end + + describe 'for new users, the email' do + let(:example_site_path) { root_path } + let(:new_user) { create(:user, email: new_user_address, created_by_id: 1) } + + token = 'kETLwRaayvigPq_x3SNM' + + subject { Notify.new_user_email(new_user.id, token) } + + it_behaves_like 'an email sent from GitLab' + it_behaves_like 'a new user email', new_user_address + it 'contains the password text' do is_expected.to have_body_text /Click here to set your password/ end @@ -88,44 +97,26 @@ describe Notify do ) end - it 'includes a link to the site' do - is_expected.to have_body_text /#{example_site_path}/ - end - it 'explains the reset link expiration' do is_expected.to have_body_text(/This link is valid for \d+ (hours?|days?)/) is_expected.to have_body_text(new_user_password_url) + is_expected.to have_body_text(/\?user_email=.*%40.*/) end end describe 'for users that signed up, the email' do let(:example_site_path) { root_path } - let(:new_user) { create(:user, email: 'newguy@example.com', password: "securePassword") } + let(:new_user) { create(:user, email: new_user_address, password: "securePassword") } subject { Notify.new_user_email(new_user.id) } it_behaves_like 'an email sent from GitLab' - - it 'is sent to the new user' do - is_expected.to deliver_to new_user.email - end - - it 'has the correct subject' do - is_expected.to have_subject /^Account was created for you$/i - end - - it 'contains the new user\'s login name' do - is_expected.to have_body_text /#{new_user.email}/ - end + it_behaves_like 'a new user email', new_user_address it 'should not contain the new user\'s password' do is_expected.not_to have_body_text /password/ end - - it 'includes a link to the site' do - is_expected.to have_body_text /#{example_site_path}/ - end end describe 'user added ssh key' do