Split password reset functionality into PasswordsController

app/abilities/ability.rb

@@ -18,10 +18,11 @@ class Ability
       can :index, ChangesetComment
       can [:index, :rss, :show, :comments], DiaryEntry
       can [:index], Note
+      can [:lost_password, :reset_password], :password
       can [:index, :show], Redaction
       can [:new, :create, :destroy], :session
       can [:index, :show, :data, :georss, :picture, :icon], Trace
-      can [:terms, :new, :create, :save, :confirm, :confirm_resend, :confirm_email, :lost_password, :reset_password, :show, :auth_success, :auth_failure], User
+      can [:terms, :new, :create, :save, :confirm, :confirm_resend, :confirm_email, :show, :auth_success, :auth_failure], User
       can [:index, :show, :blocks_on, :blocks_by], UserBlock
       can [:index, :show], Node
       can [:index, :show, :full, :ways_for_node], Way

app/controllers/passwords_controller.rb

@@ -0,0 +1,67 @@
+class PasswordsController < ApplicationController
+  include SessionMethods
+
+  layout "site"
+
+  before_action :authorize_web
+  before_action :set_locale
+  before_action :check_database_readable
+
+  authorize_resource :class => false
+
+  before_action :check_database_writable, :only => [:lost_password, :reset_password]
+
+  def lost_password
+    @title = t "passwords.lost_password.title"
+
+    if request.post?
+      user = User.visible.find_by(:email => params[:email])
+
+      if user.nil?
+        users = User.visible.where("LOWER(email) = LOWER(?)", params[:email])
+
+        user = users.first if users.count == 1
+      end
+
+      if user
+        token = user.tokens.create
+        UserMailer.lost_password(user, token).deliver_later
+        flash[:notice] = t "passwords.lost_password.notice email on way"
+        redirect_to login_path
+      else
+        flash.now[:error] = t "passwords.lost_password.notice email cannot find"
+      end
+    end
+  end
+
+  def reset_password
+    @title = t "passwords.reset_password.title"
+
+    if params[:token]
+      token = UserToken.find_by(:token => params[:token])
+
+      if token
+        self.current_user = token.user
+
+        if params[:user]
+          current_user.pass_crypt = params[:user][:pass_crypt]
+          current_user.pass_crypt_confirmation = params[:user][:pass_crypt_confirmation]
+          current_user.status = "active" if current_user.status == "pending"
+          current_user.email_valid = true
+
+          if current_user.save
+            token.destroy
+            session[:fingerprint] = current_user.fingerprint
+            flash[:notice] = t "passwords.reset_password.flash changed"
+            successful_login(current_user)
+          end
+        end
+      else
+        flash[:error] = t "passwords.reset_password.flash token bad"
+        redirect_to :action => "lost_password"
+      end
+    else
+      head :bad_request
+    end
+  end
+end

app/controllers/users_controller.rb

@@ -12,7 +12,7 @@ class UsersController < ApplicationController
   authorize_resource
 
   before_action :require_self, :only => [:account]
-  before_action :check_database_writable, :only => [:new, :account, :confirm, :confirm_email, :lost_password, :reset_password, :go_public]
+  before_action :check_database_writable, :only => [:new, :account, :confirm, :confirm_email, :go_public]
   before_action :require_cookies, :only => [:new, :confirm]
   before_action :lookup_user_by_name, :only => [:set_status, :destroy]
   before_action :allow_thirdparty_images, :only => [:show, :account]
@@ -150,60 +150,6 @@ class UsersController < ApplicationController
     redirect_to :action => "account", :display_name => current_user.display_name
   end
 
-  def lost_password
-    @title = t "users.lost_password.title"
-
-    if request.post?
-      user = User.visible.find_by(:email => params[:email])
-
-      if user.nil?
-        users = User.visible.where("LOWER(email) = LOWER(?)", params[:email])
-
-        user = users.first if users.count == 1
-      end
-
-      if user
-        token = user.tokens.create
-        UserMailer.lost_password(user, token).deliver_later
-        flash[:notice] = t "users.lost_password.notice email on way"
-        redirect_to login_path
-      else
-        flash.now[:error] = t "users.lost_password.notice email cannot find"
-      end
-    end
-  end
-
-  def reset_password
-    @title = t "users.reset_password.title"
-
-    if params[:token]
-      token = UserToken.find_by(:token => params[:token])
-
-      if token
-        self.current_user = token.user
-
-        if params[:user]
-          current_user.pass_crypt = params[:user][:pass_crypt]
-          current_user.pass_crypt_confirmation = params[:user][:pass_crypt_confirmation]
-          current_user.status = "active" if current_user.status == "pending"
-          current_user.email_valid = true
-
-          if current_user.save
-            token.destroy
-            session[:fingerprint] = current_user.fingerprint
-            flash[:notice] = t "users.reset_password.flash changed"
-            successful_login(current_user)
-          end
-        end
-      else
-        flash[:error] = t "users.reset_password.flash token bad"
-        redirect_to :action => "lost_password"
-      end
-    else
-      head :bad_request
-    end
-  end
-
   def new
     @title = t "users.new.title"
     @referer = if params[:referer]

app/mailers/user_mailer.rb

@@ -34,7 +34,7 @@ class UserMailer < ApplicationMailer
 
   def lost_password(user, token)
     with_recipient_locale user do
-      @url = url_for(:controller => "users", :action => "reset_password",
+      @url = url_for(:controller => "passwords", :action => "reset_password",
                      :token => token.token)
 
       mail :to => user.email,

app/views/sessions/new.html.erb

@@ -13,7 +13,7 @@
     <%= hidden_field_tag("referer", h(params[:referer])) %>
 
     <%= f.text_field :username, :label => t(".email or username"), :tabindex => 1, :value => params[:username] %>
-    <%= f.password_field :password, :label => t(".password"), :tabindex => 2, :value => "", :help => link_to(t(".lost password link"), :controller => "users", :action => "lost_password") %>
+    <%= f.password_field :password, :label => t(".password"), :tabindex => 2, :value => "", :help => link_to(t(".lost password link"), :controller => "passwords", :action => "lost_password") %>
     <%= f.form_group do %>
       <%= f.check_box :remember_me, { :label => t(".remember"), :tabindex => 3, :checked => (params[:remember_me] == "yes") }, "yes" %>
     <% end %>

config/locales/en.yml

@@ -1586,6 +1586,21 @@ en:
       as_unread: "Message marked as unread"
     destroy:
       destroyed: "Message deleted"
+  passwords:
+    lost_password:
+      title: "Lost password"
+      heading: "Forgotten Password?"
+      email address: "Email Address:"
+      new password button: "Reset password"
+      help_text: "Enter the email address you used to sign up, we will send a link to it that you can use to reset your password."
+      notice email on way: "Sorry you lost it :-( but an email is on its way so you can reset it soon."
+      notice email cannot find: "Could not find that email address, sorry."
+    reset_password:
+      title: "Reset password"
+      heading: "Reset Password for %{user}"
+      reset: "Reset Password"
+      flash changed: "Your password has been changed."
+      flash token bad: "Did not find that token, check the URL maybe?"
   sessions:
     new:
       title: "Login"
@@ -2274,20 +2289,6 @@ en:
     destroy:
       flash: "Destroyed the client application registration"
   users:
-    lost_password:
-      title: "Lost password"
-      heading: "Forgotten Password?"
-      email address: "Email Address:"
-      new password button: "Reset password"
-      help_text: "Enter the email address you used to sign up, we will send a link to it that you can use to reset your password."
-      notice email on way: "Sorry you lost it :-( but an email is on its way so you can reset it soon."
-      notice email cannot find: "Could not find that email address, sorry."
-    reset_password:
-      title: "Reset password"
-      heading: "Reset Password for %{user}"
-      reset: "Reset Password"
-      flash changed: "Your password has been changed."
-      flash token bad: "Did not find that token, check the URL maybe?"
     new:
       title: "Sign Up"
       no_auto_account_create: "Unfortunately we are not currently able to create an account for you automatically."

config/routes.rb

@@ -161,8 +161,8 @@ OpenStreetMap::Application.routes.draw do
   match "/user/confirm" => "users#confirm", :via => [:get, :post]
   match "/user/confirm-email" => "users#confirm_email", :via => [:get, :post]
   post "/user/go_public" => "users#go_public"
-  match "/user/reset-password" => "users#reset_password", :via => [:get, :post]
-  match "/user/forgot-password" => "users#lost_password", :via => [:get, :post]
+  match "/user/reset-password" => "passwords#reset_password", :via => [:get, :post], :as => :user_reset_password
+  match "/user/forgot-password" => "passwords#lost_password", :via => [:get, :post], :as => :user_forgot_password
   get "/user/suspended" => "users#suspended"
 
   get "/index.html", :to => redirect(:path => "/")

test/controllers/passwords_controller_test.rb

@@ -0,0 +1,153 @@
+require "test_helper"
+
+class PasswordsControllerTest < ActionDispatch::IntegrationTest
+  ##
+  # test all routes which lead to this controller
+  def test_routes
+    assert_routing(
+      { :path => "/user/forgot-password", :method => :get },
+      { :controller => "passwords", :action => "lost_password" }
+    )
+    assert_routing(
+      { :path => "/user/forgot-password", :method => :post },
+      { :controller => "passwords", :action => "lost_password" }
+    )
+    assert_routing(
+      { :path => "/user/reset-password", :method => :get },
+      { :controller => "passwords", :action => "reset_password" }
+    )
+    assert_routing(
+      { :path => "/user/reset-password", :method => :post },
+      { :controller => "passwords", :action => "reset_password" }
+    )
+  end
+
+  def test_lost_password
+    # Test fetching the lost password page
+    get user_forgot_password_path
+    assert_response :success
+    assert_template :lost_password
+    assert_select "div#notice", false
+
+    # Test resetting using the address as recorded for a user that has an
+    # address which is duplicated in a different case by another user
+    user = create(:user)
+    uppercase_user = build(:user, :email => user.email.upcase).tap { |u| u.save(:validate => false) }
+
+    # Resetting with GET should fail
+    assert_no_difference "ActionMailer::Base.deliveries.size" do
+      perform_enqueued_jobs do
+        get user_forgot_password_path, :params => { :email => user.email }
+      end
+    end
+    assert_response :success
+    assert_template :lost_password
+
+    # Resetting with POST should work
+    assert_difference "ActionMailer::Base.deliveries.size", 1 do
+      perform_enqueued_jobs do
+        post user_forgot_password_path, :params => { :email => user.email }
+      end
+    end
+    assert_response :redirect
+    assert_redirected_to login_path
+    assert_match(/^Sorry you lost it/, flash[:notice])
+    email = ActionMailer::Base.deliveries.first
+    assert_equal 1, email.to.count
+    assert_equal user.email, email.to.first
+    ActionMailer::Base.deliveries.clear
+
+    # Test resetting using an address that matches a different user
+    # that has the same address in a different case
+    assert_difference "ActionMailer::Base.deliveries.size", 1 do
+      perform_enqueued_jobs do
+        post user_forgot_password_path, :params => { :email => user.email.upcase }
+      end
+    end
+    assert_response :redirect
+    assert_redirected_to login_path
+    assert_match(/^Sorry you lost it/, flash[:notice])
+    email = ActionMailer::Base.deliveries.first
+    assert_equal 1, email.to.count
+    assert_equal uppercase_user.email, email.to.first
+    ActionMailer::Base.deliveries.clear
+
+    # Test resetting using an address that is a case insensitive match
+    # for more than one user but not an exact match for either
+    assert_no_difference "ActionMailer::Base.deliveries.size" do
+      perform_enqueued_jobs do
+        post user_forgot_password_path, :params => { :email => user.email.titlecase }
+      end
+    end
+    assert_response :success
+    assert_template :lost_password
+    assert_select ".error", /^Could not find that email address/
+
+    # Test resetting using the address as recorded for a user that has an
+    # address which is case insensitively unique
+    third_user = create(:user)
+    assert_difference "ActionMailer::Base.deliveries.size", 1 do
+      perform_enqueued_jobs do
+        post user_forgot_password_path, :params => { :email => third_user.email }
+      end
+    end
+    assert_response :redirect
+    assert_redirected_to login_path
+    assert_match(/^Sorry you lost it/, flash[:notice])
+    email = ActionMailer::Base.deliveries.first
+    assert_equal 1, email.to.count
+    assert_equal third_user.email, email.to.first
+    ActionMailer::Base.deliveries.clear
+
+    # Test resetting using an address that matches a user that has the
+    # same (case insensitively unique) address in a different case
+    assert_difference "ActionMailer::Base.deliveries.size", 1 do
+      perform_enqueued_jobs do
+        post user_forgot_password_path, :params => { :email => third_user.email.upcase }
+      end
+    end
+    assert_response :redirect
+    assert_redirected_to login_path
+    assert_match(/^Sorry you lost it/, flash[:notice])
+    email = ActionMailer::Base.deliveries.first
+    assert_equal 1, email.to.count
+    assert_equal third_user.email, email.to.first
+    ActionMailer::Base.deliveries.clear
+  end
+
+  def test_reset_password
+    user = create(:user, :pending)
+    # Test a request with no token
+    get user_reset_password_path
+    assert_response :bad_request
+
+    # Test a request with a bogus token
+    get user_reset_password_path, :params => { :token => "made_up_token" }
+    assert_response :redirect
+    assert_redirected_to :action => :lost_password
+
+    # Create a valid token for a user
+    token = user.tokens.create
+
+    # Test a request with a valid token
+    get user_reset_password_path, :params => { :token => token.token }
+    assert_response :success
+    assert_template :reset_password
+
+    # Test that errors are reported for erroneous submissions
+    post user_reset_password_path, :params => { :token => token.token, :user => { :pass_crypt => "new_password", :pass_crypt_confirmation => "different_password" } }
+    assert_response :success
+    assert_template :reset_password
+    assert_select "div.invalid-feedback"
+
+    # Test setting a new password
+    post user_reset_password_path, :params => { :token => token.token, :user => { :pass_crypt => "new_password", :pass_crypt_confirmation => "new_password" } }
+    assert_response :redirect
+    assert_redirected_to root_path
+    assert_equal user.id, session[:user]
+    user.reload
+    assert_equal "active", user.status
+    assert user.email_valid
+    assert_equal user, User.authenticate(:username => user.email, :password => "new_password")
+  end
+end

test/controllers/users_controller_test.rb

@@ -59,23 +59,6 @@ class UsersControllerTest < ActionDispatch::IntegrationTest
       { :controller => "users", :action => "go_public" }
     )
 
-    assert_routing(
-      { :path => "/user/forgot-password", :method => :get },
-      { :controller => "users", :action => "lost_password" }
-    )
-    assert_routing(
-      { :path => "/user/forgot-password", :method => :post },
-      { :controller => "users", :action => "lost_password" }
-    )
-    assert_routing(
-      { :path => "/user/reset-password", :method => :get },
-      { :controller => "users", :action => "reset_password" }
-    )
-    assert_routing(
-      { :path => "/user/reset-password", :method => :post },
-      { :controller => "users", :action => "reset_password" }
-    )
-
     assert_routing(
       { :path => "/user/suspended", :method => :get },
       { :controller => "users", :action => "suspended" }
@@ -774,135 +757,6 @@ class UsersControllerTest < ActionDispatch::IntegrationTest
     assert User.find(user.id).data_public
   end
 
-  def test_lost_password
-    # Test fetching the lost password page
-    get user_forgot_password_path
-    assert_response :success
-    assert_template :lost_password
-    assert_select "div#notice", false
-
-    # Test resetting using the address as recorded for a user that has an
-    # address which is duplicated in a different case by another user
-    user = create(:user)
-    uppercase_user = build(:user, :email => user.email.upcase).tap { |u| u.save(:validate => false) }
-
-    # Resetting with GET should fail
-    assert_no_difference "ActionMailer::Base.deliveries.size" do
-      perform_enqueued_jobs do
-        get user_forgot_password_path, :params => { :email => user.email }
-      end
-    end
-    assert_response :success
-    assert_template :lost_password
-
-    # Resetting with POST should work
-    assert_difference "ActionMailer::Base.deliveries.size", 1 do
-      perform_enqueued_jobs do
-        post user_forgot_password_path, :params => { :email => user.email }
-      end
-    end
-    assert_response :redirect
-    assert_redirected_to login_path
-    assert_match(/^Sorry you lost it/, flash[:notice])
-    email = ActionMailer::Base.deliveries.first
-    assert_equal 1, email.to.count
-    assert_equal user.email, email.to.first
-    ActionMailer::Base.deliveries.clear
-
-    # Test resetting using an address that matches a different user
-    # that has the same address in a different case
-    assert_difference "ActionMailer::Base.deliveries.size", 1 do
-      perform_enqueued_jobs do
-        post user_forgot_password_path, :params => { :email => user.email.upcase }
-      end
-    end
-    assert_response :redirect
-    assert_redirected_to login_path
-    assert_match(/^Sorry you lost it/, flash[:notice])
-    email = ActionMailer::Base.deliveries.first
-    assert_equal 1, email.to.count
-    assert_equal uppercase_user.email, email.to.first
-    ActionMailer::Base.deliveries.clear
-
-    # Test resetting using an address that is a case insensitive match
-    # for more than one user but not an exact match for either
-    assert_no_difference "ActionMailer::Base.deliveries.size" do
-      perform_enqueued_jobs do
-        post user_forgot_password_path, :params => { :email => user.email.titlecase }
-      end
-    end
-    assert_response :success
-    assert_template :lost_password
-    assert_select ".error", /^Could not find that email address/
-
-    # Test resetting using the address as recorded for a user that has an
-    # address which is case insensitively unique
-    third_user = create(:user)
-    assert_difference "ActionMailer::Base.deliveries.size", 1 do
-      perform_enqueued_jobs do
-        post user_forgot_password_path, :params => { :email => third_user.email }
-      end
-    end
-    assert_response :redirect
-    assert_redirected_to login_path
-    assert_match(/^Sorry you lost it/, flash[:notice])
-    email = ActionMailer::Base.deliveries.first
-    assert_equal 1, email.to.count
-    assert_equal third_user.email, email.to.first
-    ActionMailer::Base.deliveries.clear
-
-    # Test resetting using an address that matches a user that has the
-    # same (case insensitively unique) address in a different case
-    assert_difference "ActionMailer::Base.deliveries.size", 1 do
-      perform_enqueued_jobs do
-        post user_forgot_password_path, :params => { :email => third_user.email.upcase }
-      end
-    end
-    assert_response :redirect
-    assert_redirected_to login_path
-    assert_match(/^Sorry you lost it/, flash[:notice])
-    email = ActionMailer::Base.deliveries.first
-    assert_equal 1, email.to.count
-    assert_equal third_user.email, email.to.first
-    ActionMailer::Base.deliveries.clear
-  end
-
-  def test_reset_password
-    user = create(:user, :pending)
-    # Test a request with no token
-    get user_reset_password_path
-    assert_response :bad_request
-
-    # Test a request with a bogus token
-    get user_reset_password_path, :params => { :token => "made_up_token" }
-    assert_response :redirect
-    assert_redirected_to :action => :lost_password
-
-    # Create a valid token for a user
-    token = user.tokens.create
-
-    # Test a request with a valid token
-    get user_reset_password_path, :params => { :token => token.token }
-    assert_response :success
-    assert_template :reset_password
-
-    # Test that errors are reported for erroneous submissions
-    post user_reset_password_path, :params => { :token => token.token, :user => { :pass_crypt => "new_password", :pass_crypt_confirmation => "different_password" } }
-    assert_response :success
-    assert_template :reset_password
-    assert_select "div.invalid-feedback"
-
-    # Test setting a new password
-    post user_reset_password_path, :params => { :token => token.token, :user => { :pass_crypt => "new_password", :pass_crypt_confirmation => "new_password" } }
-    assert_response :redirect
-    assert_redirected_to root_path
-    assert_equal user.id, session[:user]
-    user.reload
-    assert_equal "active", user.status
-    assert user.email_valid
-    assert_equal user, User.authenticate(:username => user.email, :password => "new_password")
-  end
-
   def test_account
     # Get a user to work with - note that this user deliberately
     # conflicts with uppercase_user in the email and display name