From 71ea85be6eaae486b1d5d0e8642a10c57d601e07 Mon Sep 17 00:00:00 2001 From: Michael Kohl Date: Thu, 21 Jan 2021 12:40:54 +0700 Subject: [PATCH] Verify current password on password change (#12174) * Verify current password on password change * Address PR comments * Allow password reset for logged in users * Make password change message more explicit * Reformat message * Fix specs --- app/controllers/passwords_controller.rb | 15 +++++ app/controllers/users_controller.rb | 30 ++++++++-- app/models/user.rb | 3 +- app/policies/user_policy.rb | 5 ++ .../users/_account_set_password.html.erb | 8 ++- config/routes.rb | 4 +- .../user/user_changes_password_spec.rb | 59 +++++++++++++++++++ 7 files changed, 117 insertions(+), 7 deletions(-) create mode 100644 app/controllers/passwords_controller.rb create mode 100644 spec/requests/user/user_changes_password_spec.rb diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb new file mode 100644 index 000000000..155893785 --- /dev/null +++ b/app/controllers/passwords_controller.rb @@ -0,0 +1,15 @@ +class PasswordsController < Devise::PasswordsController + # allow already signed in users to reset their password + skip_before_action :require_no_authentication + + def new + super + session[:referrer] = request.referer + end + + protected + + def after_sending_reset_password_instructions_path_for(_resource_name) + session[:referrer] + end +end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index a24b40e1e..abfdafd28 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1,7 +1,8 @@ class UsersController < ApplicationController before_action :set_no_cache_header - before_action :raise_suspended, only: %i[update] - before_action :set_user, only: %i[update request_destroy full_delete remove_identity] + before_action :raise_suspended, only: %i[update update_password] + before_action :set_user, + only: %i[update update_password request_destroy full_delete remove_identity] # rubocop:disable Layout/LineLength after_action :verify_authorized, except: %i[index signout_confirm add_org_admin remove_org_admin remove_from_org confirm_destroy] # rubocop:enable Layout/LineLength @@ -42,8 +43,7 @@ class UsersController < ApplicationController # preferred_languages is handled manually @user.language_settings["preferred_languages"] = Languages::LIST.keys & params[:user][:preferred_languages].to_a - - @user.attributes = permitted_attributes(@user) + @user.assign_attributes(permitted_attributes(@user)) if @user.save # NOTE: [@rhymes] this queues a job to fetch the feed each time the profile is updated, regardless if the user @@ -269,6 +269,24 @@ class UsersController < ApplicationController end end + def update_password + set_current_tab("account") + + if @user.update_with_password(password_params) + redirect_to user_settings_path(@tab) + else + Honeycomb.add_field("error", @user.errors.messages.reject { |_, v| v.empty? }) + Honeycomb.add_field("errored", true) + + if @tab + render :edit, status: :bad_request + else + flash[:error] = @user.errors_as_sentence + redirect_to user_settings_path + end + end + end + private def sanitize_user_params @@ -359,4 +377,8 @@ class UsersController < ApplicationController def profile_params params[:profile].permit(Profile.attributes) end + + def password_params + params.permit(:current_password, :password, :password_confirmation) + end end diff --git a/app/models/user.rb b/app/models/user.rb index 72c02dc06..518bffa99 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -90,7 +90,8 @@ class User < ApplicationRecord }x.freeze attr_accessor :scholar_email, :new_note, :note_for_current_role, :user_status, :pro, :merge_user_id, - :add_credits, :remove_credits, :add_org_credits, :remove_org_credits, :ip_address + :add_credits, :remove_credits, :add_org_credits, :remove_org_credits, :ip_address, + :current_password rolify after_add: :index_roles, after_remove: :index_roles diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index 4cd3d525d..564b85a5a 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -7,6 +7,7 @@ class UserPolicy < ApplicationPolicy config_font config_theme config_navbar + current_password currently_hacking_on currently_learning display_announcements @@ -116,6 +117,10 @@ class UserPolicy < ApplicationPolicy (user.has_role?(:trusted) || minimal_admin?) && !user.banned end + def update_password? + current_user? + end + def permitted_attributes PERMITTED_ATTRIBUTES end diff --git a/app/views/users/_account_set_password.html.erb b/app/views/users/_account_set_password.html.erb index 7ff08dcba..c3d360042 100644 --- a/app/views/users/_account_set_password.html.erb +++ b/app/views/users/_account_set_password.html.erb @@ -5,9 +5,15 @@

As an alternative to signing in via linked social accounts, you may create a password. + + If you signed up with a social media account, please reset the password for your primary email address (<%= current_user.email %>) first.

- <%= form_for @user do |f| %> + <%= form_with url: user_update_password_path do |f| %> +
+ <%= f.label :current_password, class: "crayons-field__label" %> + <%= f.password_field :current_password, class: "crayons-textfield", autocomplete: "off" %> +
<%= f.label :password, class: "crayons-field__label" %> <%= f.password_field :password, class: "crayons-textfield", autocomplete: "off" %> diff --git a/config/routes.rb b/config/routes.rb index 9784756dd..07ea51c2d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -10,7 +10,8 @@ Rails.application.routes.draw do devise_for :users, controllers: { omniauth_callbacks: "omniauth_callbacks", registrations: "registrations", - invitations: "invitations" + invitations: "invitations", + passwords: "passwords" } devise_scope :user do @@ -386,6 +387,7 @@ Rails.application.routes.draw do post "organizations/generate_new_secret" => "organizations#generate_new_secret" post "users/api_secrets" => "api_secrets#create", :as => :users_api_secrets delete "users/api_secrets/:id" => "api_secrets#destroy", :as => :users_api_secret + post "users/update_password", to: "users#update_password", as: :user_update_password # The priority is based upon order of creation: first created -> highest priority. # See how all your routes lay out with "rake routes". diff --git a/spec/requests/user/user_changes_password_spec.rb b/spec/requests/user/user_changes_password_spec.rb new file mode 100644 index 000000000..5df80c13a --- /dev/null +++ b/spec/requests/user/user_changes_password_spec.rb @@ -0,0 +1,59 @@ +require "rails_helper" + +RSpec.describe "User changes password", type: :request do + let(:password) { "password123" } + let(:user) { create(:user, password: password, password_confirmation: password) } + + describe "POST /users/update_password" do + it "does not update the password if the current password is wrong" do + sign_in user + post user_update_password_path, + params: { + current_password: "wrongwrong", + password: "testtest", + password_confirmation: "testtest" + } + + expect(response.body).to include("Current password is invalid") + end + + it "does not update the password if the new password is too short" do + sign_in user + post user_update_password_path, + params: { + current_password: password, + password: "test", + password_confirmation: "test" + } + + expect(response.body).to include("Password is too short (minimum is 8 characters)") + end + + it "does not update the password if the new passwords don't match" do + sign_in user + post user_update_password_path, + params: { + current_password: password, + password: "testtest", + password_confirmation: "testtesy" + } + + expect(response.body).to include("error") + expected_message = CGI.escapeHTML("Password doesn't match password confirmation") + expect(response.body).to include(expected_message) + end + + it "updates the password if all params are valid" do + sign_in user + post user_update_password_path, + params: { + current_password: password, + password: "testtest", + password_confirmation: "testtest" + } + + expect(response.body).not_to include("error") + expect(response).to redirect_to("/settings/account") + end + end +end