diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index ac87a172dec..00e72a294b1 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -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 diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 79f13350a6d..bc4cdaa22ec 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -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 diff --git a/app/models/user.rb b/app/models/user.rb index 7fc0115b371..d3b2d8bd19e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -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) diff --git a/app/models/user_password.rb b/app/models/user_password.rb new file mode 100644 index 00000000000..80baa199e32 --- /dev/null +++ b/app/models/user_password.rb @@ -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 +# diff --git a/app/services/user_password_expirer.rb b/app/services/user_password_expirer.rb new file mode 100644 index 00000000000..2e294254e65 --- /dev/null +++ b/app/services/user_password_expirer.rb @@ -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 diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index adfb39f310c..924d1ff7123 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -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." diff --git a/db/migrate/20240603234529_create_user_passwords.rb b/db/migrate/20240603234529_create_user_passwords.rb new file mode 100644 index 00000000000..630ae093610 --- /dev/null +++ b/db/migrate/20240603234529_create_user_passwords.rb @@ -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 diff --git a/lib/validators/password_validator.rb b/lib/validators/password_validator.rb index dbbda201943..763ca656272 100644 --- a/lib/validators/password_validator.rb +++ b/lib/validators/password_validator.rb @@ -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 diff --git a/spec/fabricators/user_password_fabricator.rb b/spec/fabricators/user_password_fabricator.rb new file mode 100644 index 00000000000..017874f36a6 --- /dev/null +++ b/spec/fabricators/user_password_fabricator.rb @@ -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 } } diff --git a/spec/lib/validators/password_validator_spec.rb b/spec/lib/validators/password_validator_spec.rb index 2248efbe5a4..d220735166e 100644 --- a/spec/lib/validators/password_validator_spec.rb +++ b/spec/lib/validators/password_validator_spec.rb @@ -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 diff --git a/spec/models/user_password_spec.rb b/spec/models/user_password_spec.rb new file mode 100644 index 00000000000..aacebb0caec --- /dev/null +++ b/spec/models/user_password_spec.rb @@ -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 diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index d8f0e9d4f24..f441c51c3eb 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -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( diff --git a/spec/services/user_password_expirer_spec.rb b/spec/services/user_password_expirer_spec.rb new file mode 100644 index 00000000000..13f6854ad93 --- /dev/null +++ b/spec/services/user_password_expirer_spec.rb @@ -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 diff --git a/spec/system/login_spec.rb b/spec/system/login_spec.rb index a66cd82fbd1..c0337f184f8 100644 --- a/spec/system/login_spec.rb +++ b/spec/system/login_spec.rb @@ -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")