diff --git a/app/models/concerns/user_totp_methods.rb b/app/models/concerns/user_totp_methods.rb index 0aa94bdbb26..266ff61f178 100644 --- a/app/models/concerns/user_totp_methods.rb +++ b/app/models/concerns/user_totp_methods.rb @@ -46,8 +46,10 @@ def verify_totp(seed, otp) return false if seed.blank? totp = ROTP::TOTP.new(seed) - return false unless totp.verify(otp, drift_behind: 30, drift_ahead: 30) + totp_at = totp.verify(otp, drift_behind: 30, drift_ahead: 30, after: last_totp_at) + return false if totp_at.blank? + self.last_totp_at = Time.at(totp_at).utc save!(validate: false) end end diff --git a/db/migrate/20240720201622_add_last_totp_at_to_users.rb b/db/migrate/20240720201622_add_last_totp_at_to_users.rb new file mode 100644 index 00000000000..64982430961 --- /dev/null +++ b/db/migrate/20240720201622_add_last_totp_at_to_users.rb @@ -0,0 +1,5 @@ +class AddLastTotpAtToUsers < ActiveRecord::Migration[7.1] + def change + add_column :users, :last_totp_at, :timestamp, null: true + end +end diff --git a/db/schema.rb b/db/schema.rb index 8fbfefc8343..36bfa9037c0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -519,6 +519,7 @@ t.string "mfa_hashed_recovery_codes", default: [], array: true t.boolean "public_email", default: false, null: false t.datetime "deleted_at" + t.datetime "last_totp_at", precision: nil t.index "lower((email)::text) varchar_pattern_ops", name: "index_users_on_lower_email" t.index ["email"], name: "index_users_on_email" t.index ["handle"], name: "index_users_on_handle" diff --git a/test/functional/api/v1/deletions_controller_test.rb b/test/functional/api/v1/deletions_controller_test.rb index 9be60bb1bea..135cfb187f3 100644 --- a/test/functional/api/v1/deletions_controller_test.rb +++ b/test/functional/api/v1/deletions_controller_test.rb @@ -328,6 +328,8 @@ class Api::V1::DeletionsControllerTest < ActionController::TestCase should "not include mfa warnings" do @gems.each_value do |gem| + travel_to 30.seconds.from_now + @request.env["HTTP_OTP"] = ROTP::TOTP.new(@user.totp_seed).now delete :create, params: { gem_name: gem[:name], version: gem[:version] } diff --git a/test/functional/multifactor_auths_controller_test.rb b/test/functional/multifactor_auths_controller_test.rb index aab7a260730..9657c0f0e88 100644 --- a/test/functional/multifactor_auths_controller_test.rb +++ b/test/functional/multifactor_auths_controller_test.rb @@ -672,6 +672,8 @@ class MultifactorAuthsControllerTest < ActionController::TestCase should "redirect user back to mfa_redirect_uri after successful mfa setup" do @redirect_paths.each do |path| + travel_to 30.seconds.from_now + session[:mfa_redirect_uri] = path put :update, params: { level: "ui_and_api" } put :otp_update, params: { otp: ROTP::TOTP.new(@seed).now, level: "ui_and_api" } @@ -694,6 +696,8 @@ class MultifactorAuthsControllerTest < ActionController::TestCase should "redirect user back to mfa_redirect_uri after a failed setup + successful setup" do @redirect_paths.each do |path| + travel_to 30.seconds.from_now + session[:mfa_redirect_uri] = path put :update, params: { level: "ui_and_api" } put :otp_update, params: { otp: "12345", level: "ui_and_api" } diff --git a/test/models/concerns/user_multifactor_methods_test.rb b/test/models/concerns/user_multifactor_methods_test.rb index 52318bb0b35..e51651b49c4 100644 --- a/test/models/concerns/user_multifactor_methods_test.rb +++ b/test/models/concerns/user_multifactor_methods_test.rb @@ -410,6 +410,23 @@ class UserMultifactorMethodsTest < ActiveSupport::TestCase refute @user.ui_mfa_verified?(ROTP::TOTP.new(ROTP::Base32.random_base32).now) end + + should "return false when reused" do + code = ROTP::TOTP.new(@user.totp_seed).now + + assert @user.ui_mfa_verified?(code) + refute @user.ui_mfa_verified?(code) + end + + should "return true after waiting for the next interval" do + code = ROTP::TOTP.new(@user.totp_seed).now + + assert @user.ui_mfa_verified?(code) + travel_to 30.seconds.from_now do + refute @user.ui_mfa_verified?(code) + assert @user.ui_mfa_verified?(ROTP::TOTP.new(@user.totp_seed).now) + end + end end context "with webauthn otp" do @@ -453,6 +470,23 @@ class UserMultifactorMethodsTest < ActiveSupport::TestCase should "return false if otp is incorrect" do refute @user.api_mfa_verified?(ROTP::TOTP.new(ROTP::Base32.random_base32).now) end + + should "return false when reused" do + code = ROTP::TOTP.new(@user.totp_seed).now + + assert @user.api_mfa_verified?(code) + refute @user.api_mfa_verified?(code) + end + + should "return true after waiting for the next interval" do + code = ROTP::TOTP.new(@user.totp_seed).now + + assert @user.api_mfa_verified?(code) + travel_to 30.seconds.from_now do + refute @user.api_mfa_verified?(code) + assert @user.api_mfa_verified?(ROTP::TOTP.new(@user.totp_seed).now) + end + end end context "with webauthn otp" do