mirror of
https://github.com/discourse/discourse.git
synced 2024-11-25 09:42:07 +08:00
FEATURE: Allow site admin to mark a user's password as expired (#27314)
This commit adds the ability for site administrators to mark users' passwords as expired. Note that this commit does not add any client side interface to mark a user's password as expired. The following changes are introduced in this commit: 1. Adds a `user_passwords` table and `UserPassword` model. While the `user_passwords` table is currently used to only store expired passwords, it will be used in the future to store a user's current password as well. 2. Adds a `UserPasswordExpirer.expire_user_password` method which can be used from the Rails console to mark a user's password as expired. 3. Updates `SessionsController#create` to check that the user's current password has not been marked as expired after confirming the password. If the password is determined to be expired based on the existence of a `UserPassword` record with the `password_expired_at` column set, we will not log the user in and will display a password expired notice. A forgot password email is automatically send out to the user as well.
This commit is contained in:
parent
30f55cd64b
commit
e97ef7e9af
|
@ -15,6 +15,7 @@ class SessionController < ApplicationController
|
|||
allow_in_staff_writes_only_mode :email_login
|
||||
|
||||
ACTIVATE_USER_KEY = "activate_user"
|
||||
FORGOT_PASSWORD_EMAIL_LIMIT_PER_DAY = 6
|
||||
|
||||
def csrf
|
||||
render json: { csrf: form_authenticity_token }
|
||||
|
@ -332,8 +333,10 @@ class SessionController < ApplicationController
|
|||
rate_limit_second_factor!(user)
|
||||
|
||||
if user.present?
|
||||
# If their password is correct
|
||||
unless user.confirm_password?(params[:password])
|
||||
password = params[:password]
|
||||
|
||||
# If their password is incorrect
|
||||
if !user.confirm_password?(password)
|
||||
invalid_credentials
|
||||
return
|
||||
end
|
||||
|
@ -347,6 +350,18 @@ class SessionController < ApplicationController
|
|||
# User signed on with username and password, so let's prevent the invite link
|
||||
# from being used to log in (if one exists).
|
||||
Invite.invalidate_for_email(user.email)
|
||||
|
||||
# User's password has expired so they need to reset it
|
||||
if user.password_expired?(password)
|
||||
begin
|
||||
enqueue_password_reset_for_user(user)
|
||||
rescue RateLimiter::LimitExceeded
|
||||
# Just noop here as user would have already been sent the forgot password email more than once
|
||||
end
|
||||
|
||||
render json: { error: I18n.t("login.password_expired") }
|
||||
return
|
||||
end
|
||||
else
|
||||
invalid_credentials
|
||||
return
|
||||
|
@ -622,15 +637,7 @@ class SessionController < ApplicationController
|
|||
end
|
||||
|
||||
if user
|
||||
RateLimiter.new(nil, "forgot-password-login-day-#{user.username}", 6, 1.day).performed!
|
||||
email_token =
|
||||
user.email_tokens.create!(email: user.email, scope: EmailToken.scopes[:password_reset])
|
||||
Jobs.enqueue(
|
||||
:critical_user_email,
|
||||
type: "forgot_password",
|
||||
user_id: user.id,
|
||||
email_token: email_token.token,
|
||||
)
|
||||
enqueue_password_reset_for_user(user)
|
||||
else
|
||||
RateLimiter.new(
|
||||
nil,
|
||||
|
@ -897,4 +904,23 @@ class SessionController < ApplicationController
|
|||
|
||||
allowed_domains.split("|").include?(hostname)
|
||||
end
|
||||
|
||||
def enqueue_password_reset_for_user(user)
|
||||
RateLimiter.new(
|
||||
nil,
|
||||
"forgot-password-login-day-#{user.username}",
|
||||
FORGOT_PASSWORD_EMAIL_LIMIT_PER_DAY,
|
||||
1.day,
|
||||
).performed!
|
||||
|
||||
email_token =
|
||||
user.email_tokens.create!(email: user.email, scope: EmailToken.scopes[:password_reset])
|
||||
|
||||
Jobs.enqueue(
|
||||
:critical_user_email,
|
||||
type: "forgot_password",
|
||||
user_id: user.id,
|
||||
email_token: email_token.token,
|
||||
)
|
||||
end
|
||||
end
|
||||
|
|
|
@ -893,15 +893,18 @@ class UsersController < ApplicationController
|
|||
@user.password = params[:password]
|
||||
@user.password_required!
|
||||
@user.user_auth_tokens.destroy_all
|
||||
|
||||
if @user.save
|
||||
Invite.invalidate_for_email(@user.email) # invite link can't be used to log in anymore
|
||||
secure_session["password-#{token}"] = nil
|
||||
secure_session["second-factor-#{token}"] = nil
|
||||
|
||||
UserHistory.create!(
|
||||
target_user: @user,
|
||||
acting_user: @user,
|
||||
action: UserHistory.actions[:change_password],
|
||||
)
|
||||
|
||||
logon_after_password_reset
|
||||
end
|
||||
end
|
||||
|
|
|
@ -77,6 +77,7 @@ class User < ActiveRecord::Base
|
|||
dependent: :destroy
|
||||
has_one :invited_user, dependent: :destroy
|
||||
has_one :user_notification_schedule, dependent: :destroy
|
||||
has_many :passwords, class_name: "UserPassword", dependent: :destroy
|
||||
|
||||
# delete all is faster but bypasses callbacks
|
||||
has_many :bookmarks, dependent: :delete_all
|
||||
|
@ -926,6 +927,15 @@ class User < ActiveRecord::Base
|
|||
PasswordValidator.new(attributes: :password).validate_each(self, :password, @raw_password)
|
||||
end
|
||||
|
||||
def password_expired?(password)
|
||||
passwords
|
||||
.where("password_expired_at IS NOT NULL AND password_expired_at < ?", Time.zone.now)
|
||||
.any? do |user_password|
|
||||
user_password.password_hash ==
|
||||
hash_password(password, user_password.password_salt, user_password.password_algorithm)
|
||||
end
|
||||
end
|
||||
|
||||
def confirm_password?(password)
|
||||
return false unless password_hash && salt && password_algorithm
|
||||
confirmed = self.password_hash == hash_password(password, salt, password_algorithm)
|
||||
|
|
37
app/models/user_password.rb
Normal file
37
app/models/user_password.rb
Normal file
|
@ -0,0 +1,37 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
class UserPassword < ActiveRecord::Base
|
||||
validates :user_id, presence: true
|
||||
|
||||
validates :user_id,
|
||||
uniqueness: {
|
||||
scope: :password_expired_at,
|
||||
},
|
||||
if: -> { password_expired_at.nil? }
|
||||
|
||||
validates :password_hash, presence: true, length: { is: 64 }, uniqueness: { scope: :user_id }
|
||||
validates :password_salt, presence: true, length: { is: 32 }
|
||||
validates :password_algorithm, presence: true, length: { maximum: 64 }
|
||||
|
||||
belongs_to :user
|
||||
end
|
||||
|
||||
# == Schema Information
|
||||
#
|
||||
# Table name: user_passwords
|
||||
#
|
||||
# id :integer not null, primary key
|
||||
# user_id :integer not null
|
||||
# password_hash :string(64) not null
|
||||
# password_salt :string(32) not null
|
||||
# password_algorithm :string(64) not null
|
||||
# password_expired_at :datetime
|
||||
# created_at :datetime not null
|
||||
# updated_at :datetime not null
|
||||
#
|
||||
# Indexes
|
||||
#
|
||||
# idx_user_passwords_on_user_id_and_expired_at_and_hash (user_id,password_expired_at,password_hash)
|
||||
# index_user_passwords_on_user_id (user_id) UNIQUE WHERE (password_expired_at IS NULL)
|
||||
# index_user_passwords_on_user_id_and_password_hash (user_id,password_hash) UNIQUE
|
||||
#
|
13
app/services/user_password_expirer.rb
Normal file
13
app/services/user_password_expirer.rb
Normal file
|
@ -0,0 +1,13 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
class UserPasswordExpirer
|
||||
def self.expire_user_password(user)
|
||||
UserPassword.create!(
|
||||
user:,
|
||||
password_hash: user.password_hash,
|
||||
password_salt: user.salt,
|
||||
password_algorithm: user.password_algorithm,
|
||||
password_expired_at: Time.zone.now,
|
||||
)
|
||||
end
|
||||
end
|
|
@ -748,6 +748,7 @@ en:
|
|||
same_as_username: "is the same as your username. Please use a more secure password."
|
||||
same_as_email: "is the same as your email. Please use a more secure password."
|
||||
same_as_current: "is the same as your current password."
|
||||
same_as_previous: "is the same as your previous password."
|
||||
same_as_name: "is the same as your name."
|
||||
unique_characters: "has too many repeated characters. Please use a more secure password."
|
||||
username:
|
||||
|
@ -2759,10 +2760,10 @@ en:
|
|||
- "deactivated"
|
||||
- "inactive"
|
||||
- "unactivated"
|
||||
navigation_menu:
|
||||
- "sidebar"
|
||||
navigation_menu:
|
||||
- "sidebar"
|
||||
- "header dropdown"
|
||||
|
||||
|
||||
placeholder:
|
||||
discourse_connect_provider_secrets:
|
||||
key: "www.example.com"
|
||||
|
@ -2898,6 +2899,7 @@ en:
|
|||
not_approved: "Your account hasn't been approved yet. You will be notified by email when you are ready to log in."
|
||||
incorrect_username_email_or_password: "Incorrect username, email or password"
|
||||
incorrect_password: "Incorrect password"
|
||||
password_expired: "Password expired. Reset instructions sent to your email."
|
||||
incorrect_password_or_passkey: "Incorrect password or passkey"
|
||||
wait_approval: "Thanks for signing up. We will notify you when your account has been approved."
|
||||
active: "Your account is activated and ready to use."
|
||||
|
|
23
db/migrate/20240603234529_create_user_passwords.rb
Normal file
23
db/migrate/20240603234529_create_user_passwords.rb
Normal file
|
@ -0,0 +1,23 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
class CreateUserPasswords < ActiveRecord::Migration[7.0]
|
||||
def change
|
||||
create_table :user_passwords, id: :integer do |t|
|
||||
t.integer :user_id, null: false
|
||||
t.string :password_hash, limit: 64, null: false
|
||||
t.string :password_salt, limit: 32, null: false
|
||||
t.string :password_algorithm, limit: 64, null: false
|
||||
t.datetime :password_expired_at, null: true
|
||||
|
||||
t.timestamps
|
||||
end
|
||||
|
||||
add_index :user_passwords, %i[user_id], unique: true, where: "password_expired_at IS NULL"
|
||||
|
||||
add_index :user_passwords, %i[user_id password_hash], unique: true
|
||||
|
||||
add_index :user_passwords,
|
||||
%i[user_id password_expired_at password_hash],
|
||||
name: "idx_user_passwords_on_user_id_and_expired_at_and_hash"
|
||||
end
|
||||
end
|
|
@ -19,6 +19,8 @@ class PasswordValidator < ActiveModel::EachValidator
|
|||
record.errors.add(attribute, :same_as_email)
|
||||
elsif record.confirm_password?(value)
|
||||
record.errors.add(attribute, :same_as_current)
|
||||
elsif record.password_expired?(value)
|
||||
record.errors.add(attribute, :same_as_previous)
|
||||
elsif SiteSetting.block_common_passwords && CommonPasswords.common_password?(value)
|
||||
record.errors.add(attribute, :common)
|
||||
elsif value.chars.uniq.length < SiteSetting.password_unique_characters
|
||||
|
|
20
spec/fabricators/user_password_fabricator.rb
Normal file
20
spec/fabricators/user_password_fabricator.rb
Normal file
|
@ -0,0 +1,20 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
Fabricator(:user_password) do
|
||||
transient password: "myawesomefakepassword"
|
||||
|
||||
user { Fabricate(:user) }
|
||||
password_salt { SecureRandom.hex(User::PASSWORD_SALT_LENGTH) }
|
||||
password_algorithm { User::TARGET_PASSWORD_ALGORITHM }
|
||||
|
||||
after_build do |user_password, transients|
|
||||
user_password.password_hash =
|
||||
PasswordHasher.hash_password(
|
||||
password: transients[:password],
|
||||
salt: user_password.password_salt,
|
||||
algorithm: user_password.password_algorithm,
|
||||
)
|
||||
end
|
||||
end
|
||||
|
||||
Fabricator(:expired_user_password, from: :user_password) { password_expired_at { 1.day.ago } }
|
|
@ -141,6 +141,29 @@ RSpec.describe PasswordValidator do
|
|||
expect(record.errors[:password]).to include(password_error_message(:same_as_current))
|
||||
end
|
||||
|
||||
it "adds an error when new password is same as a previous password" do
|
||||
@password = "thisisaoldpassword"
|
||||
record.save!
|
||||
record.reload
|
||||
|
||||
new_password = "thisisanewpassword"
|
||||
|
||||
_expired_user_password =
|
||||
Fabricate(
|
||||
:expired_user_password,
|
||||
password: new_password,
|
||||
password_algorithm: record.password_algorithm,
|
||||
password_salt: record.salt,
|
||||
user: record,
|
||||
)
|
||||
|
||||
record.password = new_password
|
||||
@password = new_password
|
||||
validate
|
||||
|
||||
expect(record.errors[:password]).to include(password_error_message(:same_as_previous))
|
||||
end
|
||||
|
||||
it "validation required if password is required" do
|
||||
expect(record.password_validation_required?).to eq(true)
|
||||
end
|
||||
|
|
97
spec/models/user_password_spec.rb
Normal file
97
spec/models/user_password_spec.rb
Normal file
|
@ -0,0 +1,97 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
RSpec.describe UserPassword do
|
||||
context "for validations" do
|
||||
it "should validate presence of user_id" do
|
||||
user_password = Fabricate.build(:user_password, user_id: nil)
|
||||
|
||||
expect(user_password).not_to be_valid
|
||||
expect(user_password.errors[:user_id]).to include("can't be blank")
|
||||
end
|
||||
|
||||
it "should validate presence of password_hash" do
|
||||
user_password = Fabricate.build(:user_password)
|
||||
user_password.password_hash = nil
|
||||
|
||||
expect(user_password).not_to be_valid
|
||||
expect(user_password.errors[:password_hash]).to include("can't be blank")
|
||||
end
|
||||
|
||||
it "should validate that password_hash is 64 characters long" do
|
||||
user_password = Fabricate.build(:user_password)
|
||||
user_password.password_hash = "a" * 65
|
||||
|
||||
expect(user_password).not_to be_valid
|
||||
|
||||
expect(user_password.errors[:password_hash]).to include(
|
||||
"is the wrong length (should be 64 characters)",
|
||||
)
|
||||
end
|
||||
|
||||
it "should validate uniqueness of password_hash scoped to user_id" do
|
||||
password = "password"
|
||||
user_password_1 = Fabricate(:user_password, password:)
|
||||
user = user_password_1.user
|
||||
|
||||
user_password_2 =
|
||||
Fabricate.build(
|
||||
:user_password,
|
||||
user:,
|
||||
password:,
|
||||
password_salt: user_password_1.password_salt,
|
||||
password_algorithm: user_password_1.password_algorithm,
|
||||
)
|
||||
|
||||
expect(user_password_2).not_to be_valid
|
||||
expect(user_password_2.errors[:password_hash]).to include("has already been taken")
|
||||
end
|
||||
|
||||
it "should validate uniqueness of user_id scoped to password_expired_at" do
|
||||
user = Fabricate(:user)
|
||||
user_password_1 = Fabricate.create(:user_password, user:, password_expired_at: nil)
|
||||
|
||||
user_password_2 =
|
||||
Fabricate.build(:user_password, user: user_password_1.user, password_expired_at: nil)
|
||||
|
||||
expect(user_password_2).not_to be_valid
|
||||
expect(user_password_2.errors[:user_id]).to include("has already been taken")
|
||||
end
|
||||
|
||||
it "should validate presence of password_salt" do
|
||||
user_password = Fabricate.build(:user_password)
|
||||
user_password.password_salt = nil
|
||||
|
||||
expect(user_password).not_to be_valid
|
||||
expect(user_password.errors[:password_salt]).to include("can't be blank")
|
||||
end
|
||||
|
||||
it "should validate that password_salt is 32 characters long" do
|
||||
user_password = Fabricate.build(:user_password)
|
||||
user_password.password_salt = "a" * 33
|
||||
|
||||
expect(user_password).not_to be_valid
|
||||
|
||||
expect(user_password.errors[:password_salt]).to include(
|
||||
"is the wrong length (should be 32 characters)",
|
||||
)
|
||||
end
|
||||
|
||||
it "should validate presence of password_algorithm" do
|
||||
user_password = Fabricate.build(:user_password)
|
||||
user_password.password_algorithm = nil
|
||||
|
||||
expect(user_password).not_to be_valid
|
||||
expect(user_password.errors[:password_algorithm]).to include("can't be blank")
|
||||
end
|
||||
|
||||
it "should validate that password_algorithm is at most 64 characters long" do
|
||||
user_password = Fabricate.build(:user_password)
|
||||
user_password.password_algorithm = "a" * 65
|
||||
|
||||
expect(user_password).not_to be_valid
|
||||
expect(user_password.errors[:password_algorithm]).to include(
|
||||
"is too long (maximum is 64 characters)",
|
||||
)
|
||||
end
|
||||
end
|
||||
end
|
|
@ -1991,7 +1991,7 @@ RSpec.describe SessionController do
|
|||
end
|
||||
end
|
||||
|
||||
describe "success by username" do
|
||||
describe "success by username and password" do
|
||||
it "logs in correctly" do
|
||||
events =
|
||||
DiscourseEvent.track_events do
|
||||
|
@ -2030,6 +2030,70 @@ RSpec.describe SessionController do
|
|||
end
|
||||
end
|
||||
|
||||
describe "when user's password has been marked as expired" do
|
||||
let!(:expired_user_password) do
|
||||
Fabricate(
|
||||
:expired_user_password,
|
||||
user:,
|
||||
password: "myawesomepassword",
|
||||
password_salt: user.salt,
|
||||
password_algorithm: user.password_algorithm,
|
||||
)
|
||||
end
|
||||
|
||||
before { RateLimiter.enable }
|
||||
|
||||
use_redis_snapshotting
|
||||
|
||||
it "should return an error response code with the right error message and enqueues the password reset email" do
|
||||
expect_enqueued_with(
|
||||
job: :critical_user_email,
|
||||
args: {
|
||||
type: "forgot_password",
|
||||
user_id: user.id,
|
||||
},
|
||||
) do
|
||||
post "/session.json", params: { login: user.username, password: "myawesomepassword" }
|
||||
end
|
||||
|
||||
expect(response.status).to eq(200)
|
||||
expect(response.parsed_body["error"]).to eq(I18n.t("login.password_expired"))
|
||||
expect(session[:current_user_id]).to eq(nil)
|
||||
end
|
||||
|
||||
it "should limit the number of forgot password emails sent a day to the user when logging in with an expired password" do
|
||||
SiteSetting.max_logins_per_ip_per_minute =
|
||||
described_class::FORGOT_PASSWORD_EMAIL_LIMIT_PER_DAY + 1
|
||||
|
||||
SiteSetting.max_logins_per_ip_per_hour =
|
||||
described_class::FORGOT_PASSWORD_EMAIL_LIMIT_PER_DAY + 1
|
||||
|
||||
described_class::FORGOT_PASSWORD_EMAIL_LIMIT_PER_DAY.times do
|
||||
expect_enqueued_with(
|
||||
job: :critical_user_email,
|
||||
args: {
|
||||
type: "forgot_password",
|
||||
user_id: user.id,
|
||||
},
|
||||
) do
|
||||
post "/session.json", params: { login: user.username, password: "myawesomepassword" }
|
||||
expect(response.status).to eq(200)
|
||||
end
|
||||
end
|
||||
|
||||
expect_not_enqueued_with(
|
||||
job: :critical_user_email,
|
||||
args: {
|
||||
type: "forgot_password",
|
||||
user_id: user.id,
|
||||
},
|
||||
) do
|
||||
post "/session.json", params: { login: user.username, password: "myawesomepassword" }
|
||||
expect(response.status).to eq(200)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
context "when a user has security key-only 2FA login" do
|
||||
let!(:user_security_key) do
|
||||
Fabricate(
|
||||
|
|
23
spec/services/user_password_expirer_spec.rb
Normal file
23
spec/services/user_password_expirer_spec.rb
Normal file
|
@ -0,0 +1,23 @@
|
|||
# frozen_string_literal: true
|
||||
|
||||
RSpec.describe UserPasswordExpirer do
|
||||
fab!(:password) { "somerandompassword" }
|
||||
fab!(:user) { Fabricate(:user, password:) }
|
||||
|
||||
describe ".expire_user_password" do
|
||||
it "should create a new UserPassword record with the user's current password information" do
|
||||
freeze_time
|
||||
|
||||
described_class.expire_user_password(user)
|
||||
|
||||
expect(user.passwords.count).to eq(1)
|
||||
|
||||
user_password = user.passwords.first
|
||||
|
||||
expect(user_password.password_hash).to eq(user.password_hash)
|
||||
expect(user_password.password_salt).to eq(user.salt)
|
||||
expect(user_password.password_algorithm).to eq(user.password_algorithm)
|
||||
expect(user_password.password_expired_at).to eq_time(Time.zone.now)
|
||||
end
|
||||
end
|
||||
end
|
|
@ -38,6 +38,18 @@ shared_examples "login scenarios" do
|
|||
expect(page).to have_css(".header-dropdown-toggle.current-user")
|
||||
end
|
||||
|
||||
it "displays the right message when user's email has been marked as expired" do
|
||||
password = "myawesomepassword"
|
||||
user.update!(password:)
|
||||
expired_user_password = Fabricate(:expired_user_password, user:, password:)
|
||||
|
||||
login_modal.open
|
||||
login_modal.fill(username: user.username, password:)
|
||||
login_modal.click_login
|
||||
|
||||
expect(login_modal).to have_content(I18n.t("login.password_expired"))
|
||||
end
|
||||
|
||||
it "can reset password" do
|
||||
login_modal.open
|
||||
login_modal.fill_username("john")
|
||||
|
|
Loading…
Reference in New Issue
Block a user