From 6b9338d15202c896f91eb2cc320ecb89e7e93fe1 Mon Sep 17 00:00:00 2001 From: Bert Goethals Date: Wed, 13 Dec 2023 15:24:33 +0100 Subject: [PATCH 1/3] Introduce failing test --- spec/two_factor_spec.rb | 85 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/spec/two_factor_spec.rb b/spec/two_factor_spec.rb index c22c056b..a9bba92a 100644 --- a/spec/two_factor_spec.rb +++ b/spec/two_factor_spec.rb @@ -2080,6 +2080,91 @@ def reset_otp_last_use page.find('#error_flash').text.must_equal 'You need to authenticate via an additional factor before continuing' end + it "should not accept pending sms codes when signing in" do + sms_phone = sms_message = nil + hmac_secret = '123' + hmac_old_secret = nil + old_secret_used = false + rodauth do + enable :login, :logout, :otp, :sms_codes + sms_codes_primary? true + hmac_secret do + hmac_secret + end + hmac_old_secret do + hmac_old_secret + end + otp_valid_code_for_old_secret do + raise if old_secret_used + old_secret_used = true + end + + sms_send do |phone, msg| + proc{super(phone, msg)}.must_raise NotImplementedError + sms_phone = phone + sms_message = msg + end + sms_remove_failures do + if super() == 1 + sms[sms_failures_column].must_equal 0 + sms.fetch(sms_code_column).must_be_nil + end + end + end + roda do |r| + r.rodauth + + r.redirect '/login' unless rodauth.logged_in? + + if rodauth.two_factor_authentication_setup? + r.redirect '/otp-auth' unless rodauth.authenticated? + view :content=>"With 2FA" + else + view :content=>"Without 2FA" + end + end + + login + + visit '/otp-setup' + page.title.must_equal 'Setup TOTP Authentication' + page.html.must_include ''0123456789' + fill_in 'Authentication Code', :with=>totp.now + click_button 'Setup TOTP Authentication' + page.find('#notice_flash').text.must_equal 'TOTP authentication is now setup' + + + + visit '/sms-setup' + page.title.must_equal 'Setup SMS Backup Number' + + fill_in 'Password', :with=>'0123456789' + fill_in 'Phone Number', :with=>'(123) 456-7890' + click_button 'Setup SMS Backup Number' + page.find('#notice_flash').text.must_equal 'SMS authentication needs confirmation' + sms_phone.must_equal '1234567890' + sms_message.must_match(/\ASMS confirmation code for www\.example\.com is \d{12}\z/) + + logout + login + # hmac_secret = '321' + # hmac_old_secret = nil + reset_otp_last_use + old_secret_used.must_equal false + fill_in 'Authentication Code', :with=>"#{totp.now[0..2]} #{totp.now[3..-1]}" + click_button 'Authenticate Using TOTP' + page.find('#notice_flash').text.must_equal 'You have been multifactor authenticated' + page.html.must_include 'With 2FA' + reset_otp_last_use + + visit '/sms-setup' + page.find('#error_flash').text.must_equal 'SMS authentication needs confirmation' + page.title.must_equal 'Confirm SMS Backup Number' + end + begin require 'webauthn/fake_client' rescue LoadError From 9bd21cfcf5ebff14dd76b00a6ad27121f554d6b0 Mon Sep 17 00:00:00 2001 From: Bert Goethals Date: Wed, 13 Dec 2023 15:26:59 +0100 Subject: [PATCH 2/3] Fix issue where we accidentally accept a non-confirmed SMS code during sign in --- lib/rodauth/features/sms_codes.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/rodauth/features/sms_codes.rb b/lib/rodauth/features/sms_codes.rb index 33a6c007..18b665f3 100644 --- a/lib/rodauth/features/sms_codes.rb +++ b/lib/rodauth/features/sms_codes.rb @@ -367,11 +367,12 @@ def sms_setup(phone_number) end def sms_remove_failures + return if sms_needs_confirmation? update_sms(sms_failures_column => 0, sms_code_column => nil) end def sms_confirm - sms_remove_failures + update_sms(sms_failures_column => 0, sms_code_column => nil) super if defined?(super) end From aac874c2fb3d59a4fab7f2b11d65f555484803f5 Mon Sep 17 00:00:00 2001 From: Bert Goethals Date: Wed, 13 Dec 2023 18:34:26 +0100 Subject: [PATCH 3/3] Improve queries used when dealing with sms codes Co-authored-by: Jeremy Evans --- lib/rodauth/features/sms_codes.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/rodauth/features/sms_codes.rb b/lib/rodauth/features/sms_codes.rb index 18b665f3..05e7f9f3 100644 --- a/lib/rodauth/features/sms_codes.rb +++ b/lib/rodauth/features/sms_codes.rb @@ -362,17 +362,17 @@ def sms_confirm_failure def sms_setup(phone_number) # Cannot handle uniqueness violation here, as the phone number given may not match the # one in the table. - sms_ds.insert(sms_id_column=>session_value, sms_phone_column=>phone_number) + sms_ds.insert(sms_id_column=>session_value, sms_phone_column=>phone_number, sms_failures_column=>nil) remove_instance_variable(:@sms) if instance_variable_defined?(:@sms) end def sms_remove_failures return if sms_needs_confirmation? - update_sms(sms_failures_column => 0, sms_code_column => nil) + update_hash_ds(sms, sms_ds.exclude(sms_failures_column=>nil), sms_failures_column=>0, sms_code_column=>nil) end def sms_confirm - update_sms(sms_failures_column => 0, sms_code_column => nil) + update_hash_ds(sms, sms_ds.where(sms_failures_column=>nil), sms_failures_column=>0, sms_code_column=>nil) super if defined?(super) end