From 7f2eeaf767873ef29f7c47c1cfecf7bc8de0743f Mon Sep 17 00:00:00 2001 From: Vinoth Kannan Date: Fri, 1 Dec 2017 09:49:24 +0530 Subject: [PATCH] FIX: Password required flag should be cleared whenever clearing the raw password (#5384) --- app/models/user.rb | 5 ++++ lib/validators/password_validator.rb | 3 ++- .../validators/password_validator_spec.rb | 24 +++++++++++++++++++ spec/controllers/session_controller_spec.rb | 8 +++---- spec/fabricators/user_fabricator.rb | 4 ++-- spec/models/user_spec.rb | 2 +- spec/requests/groups_controller_spec.rb | 4 ---- 7 files changed, 38 insertions(+), 12 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 7ba8b057f91..65e58f600f7 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -452,6 +452,10 @@ class User < ActiveRecord::Base !!@password_required end + def password_validation_required? + password_required? || @raw_password.present? + end + def has_password? password_hash.present? end @@ -1029,6 +1033,7 @@ class User < ActiveRecord::Base UserAuthToken.where(user_id: id).destroy_all # We should not carry this around after save @raw_password = nil + @password_required = false end end diff --git a/lib/validators/password_validator.rb b/lib/validators/password_validator.rb index ced6b2f9909..d5d24ada8ee 100644 --- a/lib/validators/password_validator.rb +++ b/lib/validators/password_validator.rb @@ -3,7 +3,8 @@ require_dependency "common_passwords/common_passwords" class PasswordValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) - return unless record.password_required? + return unless record.password_validation_required? + if value.nil? record.errors.add(attribute, :blank) elsif value.length < SiteSetting.min_admin_password_length && (record.admin? || is_developer?(record.email)) diff --git a/spec/components/validators/password_validator_spec.rb b/spec/components/validators/password_validator_spec.rb index 803bce9b921..1e6cc4cde05 100644 --- a/spec/components/validators/password_validator_spec.rb +++ b/spec/components/validators/password_validator_spec.rb @@ -134,6 +134,19 @@ describe PasswordValidator do validate expect(record.errors[:password]).to include(password_error_message(:same_as_current)) end + + it "validation required if password is required" do + expect(record.password_validation_required?).to eq(true) + end + + it "validation not required after save until a new password is set" do + @password = "myoldpassword" + record.save! + record.reload + expect(record.password_validation_required?).to eq(false) + record.password = "mynewpassword" + expect(record.password_validation_required?).to eq(true) + end end context "password not required" do @@ -144,6 +157,17 @@ describe PasswordValidator do validate expect(record.errors[:password]).not_to be_present end + + it "validation required if a password is set" do + @password = "mygameshow" + expect(record.password_validation_required?).to eq(true) + end + + it "adds an error even password not required" do + @password = "p" + validate + expect(record.errors[:password]).to be_present + end end end diff --git a/spec/controllers/session_controller_spec.rb b/spec/controllers/session_controller_spec.rb index f80e5b76438..3e47de45fb9 100644 --- a/spec/controllers/session_controller_spec.rb +++ b/spec/controllers/session_controller_spec.rb @@ -302,7 +302,7 @@ describe SessionController do @sso.sso_secret = SiteSetting.sso_secret @sso.return_sso_url = "http://somewhere.over.rainbow/sso" - @user = Fabricate(:user, password: "frogs", active: true, admin: true) + @user = Fabricate(:user, password: "myfrogs123ADMIN", active: true, admin: true) group = Fabricate(:group) group.add(@user) @user.reload @@ -314,7 +314,7 @@ describe SessionController do expect(response).to redirect_to("/login") post :create, - params: { login: @user.username, password: "frogs" }, + params: { login: @user.username, password: "myfrogs123ADMIN" }, format: :json, xhr: true @@ -422,7 +422,7 @@ describe SessionController do @sso.sso_secret = SiteSetting.sso_secret @sso.return_sso_url = "http://somewhere.over.rainbow/sso" - @user = Fabricate(:user, password: "frogs", active: true, admin: true) + @user = Fabricate(:user, password: "myfrogs123ADMIN", active: true, admin: true) EmailToken.update_all(confirmed: true) end @@ -431,7 +431,7 @@ describe SessionController do expect(response).to redirect_to("/login") post :create, - params: { login: @user.username, password: "frogs" }, + params: { login: @user.username, password: "myfrogs123ADMIN" }, format: :json, xhr: true diff --git a/spec/fabricators/user_fabricator.rb b/spec/fabricators/user_fabricator.rb index 3b63c195770..dc535a4e250 100644 --- a/spec/fabricators/user_fabricator.rb +++ b/spec/fabricators/user_fabricator.rb @@ -26,14 +26,14 @@ Fabricator(:evil_trout, from: :user) do name 'Evil Trout' username 'eviltrout' email 'eviltrout@somewhere.com' - password 'imafish' + password 'imafish123' end Fabricator(:walter_white, from: :user) do name 'Walter White' username 'heisenberg' email 'wwhite@bluemeth.com' - password 'letscook' + password 'letscook123' end Fabricator(:inactive_user, from: :user) do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index f599490e818..f9917959a73 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -644,7 +644,7 @@ describe User do UserAuthToken.generate!(user_id: @user.id) - @user.password = "passwordT" + @user.password = "passwordT0" @user.save! # must expire old token on password change diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index 003b348f53d..1d7c14abeb7 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -89,10 +89,6 @@ describe GroupsController do ) end - before do - sign_in(user) - end - context "when user is group owner" do before do group.add_owner(user)