From: Andy Allan Date: Wed, 7 Apr 2021 13:39:12 +0000 (+0100) Subject: Split password reset functionality into PasswordsController X-Git-Tag: live~2179^2~1 X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/7a66c6d4eb65a5ad6438970375cf3ea6ac4e3cfc?ds=inline;hp=--cc Split password reset functionality into PasswordsController --- 7a66c6d4eb65a5ad6438970375cf3ea6ac4e3cfc diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb index b8aa82689..5db1b7eb5 100644 --- a/app/abilities/ability.rb +++ b/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 diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb new file mode 100644 index 000000000..331575964 --- /dev/null +++ b/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 diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index e389f6fbf..7c732d46e 100644 --- a/app/controllers/users_controller.rb +++ b/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] diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb index bb4412e8a..bb43bc962 100644 --- a/app/mailers/user_mailer.rb +++ b/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, diff --git a/app/views/users/lost_password.html.erb b/app/views/passwords/lost_password.html.erb similarity index 100% rename from app/views/users/lost_password.html.erb rename to app/views/passwords/lost_password.html.erb diff --git a/app/views/users/reset_password.html.erb b/app/views/passwords/reset_password.html.erb similarity index 100% rename from app/views/users/reset_password.html.erb rename to app/views/passwords/reset_password.html.erb diff --git a/app/views/sessions/new.html.erb b/app/views/sessions/new.html.erb index 2844f3389..69ea7d8fd 100644 --- a/app/views/sessions/new.html.erb +++ b/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 %> diff --git a/config/locales/en.yml b/config/locales/en.yml index e1907b936..38f977263 100644 --- a/config/locales/en.yml +++ b/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." diff --git a/config/routes.rb b/config/routes.rb index 1ba4eaa24..52e5a69e1 100644 --- a/config/routes.rb +++ b/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 => "/") diff --git a/test/controllers/passwords_controller_test.rb b/test/controllers/passwords_controller_test.rb new file mode 100644 index 000000000..8a6e0b901 --- /dev/null +++ b/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 diff --git a/test/controllers/users_controller_test.rb b/test/controllers/users_controller_test.rb index d5b915a85..5059ff698 100644 --- a/test/controllers/users_controller_test.rb +++ b/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