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

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.

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

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

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"
 

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

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 %>

spec/helpers/emails_helper_spec.rb

+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

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