Merge branch 'improve-reset-tokens' into 'master'

Explain reset token expiration in emails

Update the new user emails to tell new users when their password reset token expires and provide a link to get a new one.  See #1921.  This MR adds new text to the emails:

```html
This link is valid for X days.  After it expires, you can <a href='new_user_password_url'>request a new one</a>
```

It will be more difficult to add the same link to the error message that's displayed when a user tries to reset his password with an expired token.  Currently, we don't know why the password change fails, we just show the Devise error messages on the form.

See merge request !1803

8 files changed, 119 insertions, 33 deletions

diff --git a/CHANGELOG b/CHANGELOG
index ade877feb9a..15bfe570f1a 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/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb
index 88459d4080a..145f27b67dd 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(user_email: user['email']))
+      end
+    end
+  end
 end
diff --git a/app/helpers/emails_helper.rb b/app/helpers/emails_helper.rb
index 0df3ecc90b7..128de18bc47 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(user_email: @user.email))
+    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/devise/passwords/new.html.haml b/app/views/devise/passwords/new.html.haml
index e8820daf58f..29ffe8a8be3 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 ebbe98dd472..4feacdaacff 100644
--- a/app/views/notify/new_user_email.html.haml
+++ b/app/views/notify/new_user_email.html.haml
@@ -11,4 +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
diff --git a/app/views/notify/new_user_email.text.erb b/app/views/notify/new_user_email.text.erb
index 96b26879a77..dd9b71e3b84 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 00000000000..7a3e38d7e63
--- /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 dbcf7286e45..37607b55eb1 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,39 +97,26 @@ describe Notify do
       )
     end
 
-    it 'includes a link to the site' do
-      is_expected.to have_body_text /#{example_site_path}/
+    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