From 6feb84e1ec62a7d89c09015db64a02e7bf82175c Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Mon, 18 Nov 2024 13:52:35 +0100 Subject: [PATCH 1/4] [#59408] Signing in after two factor methods have been deleted lead to a 500 error https://community.openproject.org/work_packages/59408 From de14ecc2963784265e7c2a7320601c18d2824d07 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Mon, 18 Nov 2024 14:52:45 +0100 Subject: [PATCH 2/4] add failing spec for 2fa behavior when deleting methods --- .../login/login_with_deleted_2fa_spec.rb | 53 +++++++++++++++++++ .../spec/features/shared_2fa_examples.rb | 7 +++ 2 files changed, 60 insertions(+) create mode 100644 modules/two_factor_authentication/spec/features/login/login_with_deleted_2fa_spec.rb diff --git a/modules/two_factor_authentication/spec/features/login/login_with_deleted_2fa_spec.rb b/modules/two_factor_authentication/spec/features/login/login_with_deleted_2fa_spec.rb new file mode 100644 index 000000000000..16ea18886bcf --- /dev/null +++ b/modules/two_factor_authentication/spec/features/login/login_with_deleted_2fa_spec.rb @@ -0,0 +1,53 @@ +require_relative "../../spec_helper" +require_relative "../shared_2fa_examples" + +RSpec.describe "Login after 2FA deleted 2FA was deleted (REGRESSION)", + :js, + :with_cuprite, + with_settings: { + plugin_openproject_two_factor_authentication: { + "active_strategies" => %i[developer totp] + } + } do + let(:user_password) { "bob!" * 4 } + let(:user) do + create(:user, + login: "bob", + password: user_password, + password_confirmation: user_password) + end + + let!(:device1) { create(:two_factor_authentication_device_sms, user:, active: true, default: false) } + let!(:device2) { create(:two_factor_authentication_device_totp, user:, active: true, default: true) } + + it "works correctly when not switching 2fa method" do + first_login_step + + # ensure that no 2fa device is stored in the session + session_data = Sessions::UserSession.last.data + expect(session_data["two_factor_authentication_device_id"]).to be_nil + + # destroy all 2fa devices + user.otp_devices.destroy_all + + # make sure we can sign in without 2fa + first_login_step + expect_logged_in + end + + it "works correctly when the 2fa method was switched before deleting" do + first_login_step + switch_two_factor_device(device1) + + # ensure that the selected 2fa device is stored in the session + session_data = Sessions::UserSession.last.data + expect(session_data["two_factor_authentication_device_id"]).to eq(device1.id) + + # destroy all 2fa devices + user.otp_devices.destroy_all + + # make sure we can sign in without 2fa + first_login_step + expect_logged_in + end +end diff --git a/modules/two_factor_authentication/spec/features/shared_2fa_examples.rb b/modules/two_factor_authentication/spec/features/shared_2fa_examples.rb index 2d0a94a24659..367cfbe75b19 100644 --- a/modules/two_factor_authentication/spec/features/shared_2fa_examples.rb +++ b/modules/two_factor_authentication/spec/features/shared_2fa_examples.rb @@ -8,6 +8,13 @@ def first_login_step wait_for_network_idle end +def switch_two_factor_device(device) + within("#login-form") do + click_link_or_button I18n.t(:text_otp_not_receive) + click_link_or_button device.redacted_identifier + end +end + def two_factor_step(token) expect(page).to have_css("input#otp") fill_in "otp", with: token From c1ed4756621ff94c932a6161cafdc16221edec29 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Mon, 18 Nov 2024 14:53:04 +0100 Subject: [PATCH 3/4] fix behavior that raises a 500 error when the stored device cannot be found --- .../two_factor_authentication/authentication_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/two_factor_authentication/app/controllers/two_factor_authentication/authentication_controller.rb b/modules/two_factor_authentication/app/controllers/two_factor_authentication/authentication_controller.rb index a2cb5a503807..d224bbbde888 100644 --- a/modules/two_factor_authentication/app/controllers/two_factor_authentication/authentication_controller.rb +++ b/modules/two_factor_authentication/app/controllers/two_factor_authentication/authentication_controller.rb @@ -106,7 +106,7 @@ def otp_service_for_verification(user) def remembered_device(user) if session[:two_factor_authentication_device_id] - user.otp_devices.find(session[:two_factor_authentication_device_id]) + user.otp_devices.find_by(id: session[:two_factor_authentication_device_id]) end end From c78e5474789ddae1fcfc18870661ee170d2a0ed2 Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Mon, 18 Nov 2024 15:04:24 +0100 Subject: [PATCH 4/4] style: do not pollute global namespace with test methods --- .../spec/features/account_activation_spec.rb | 4 +- .../generate_backup_codes_spec.rb | 2 +- .../login_with_backup_code_spec.rb | 4 +- .../features/login/login_enforced_2fa_spec.rb | 3 +- .../features/login/login_with_2fa_spec.rb | 3 +- .../login/login_with_deleted_2fa_spec.rb | 3 +- .../features/login/login_without_2fa_spec.rb | 3 +- .../login/switch_available_devices_spec.rb | 4 +- .../login_with_remember_cookie_spec.rb | 4 +- ...mples.rb => shared_two_factor_examples.rb} | 56 ++++++++++--------- 10 files changed, 50 insertions(+), 36 deletions(-) rename modules/two_factor_authentication/spec/features/{shared_2fa_examples.rb => shared_two_factor_examples.rb} (66%) diff --git a/modules/two_factor_authentication/spec/features/account_activation_spec.rb b/modules/two_factor_authentication/spec/features/account_activation_spec.rb index 3c5ac03cf980..a9a2a406c0c8 100644 --- a/modules/two_factor_authentication/spec/features/account_activation_spec.rb +++ b/modules/two_factor_authentication/spec/features/account_activation_spec.rb @@ -1,5 +1,5 @@ require_relative "../spec_helper" -require_relative "shared_2fa_examples" +require_relative "shared_two_factor_examples" RSpec.describe "activating an invited account", :js, @@ -7,6 +7,8 @@ with_settings: { plugin_openproject_two_factor_authentication: { "active_strategies" => [:developer] } } do + include SharedTwoFactorExamples + let(:user) do user = build(:user, first_login: true) UserInvitation.invite_user! user diff --git a/modules/two_factor_authentication/spec/features/backup_codes/generate_backup_codes_spec.rb b/modules/two_factor_authentication/spec/features/backup_codes/generate_backup_codes_spec.rb index 681720b9900e..674aebd2ee2a 100644 --- a/modules/two_factor_authentication/spec/features/backup_codes/generate_backup_codes_spec.rb +++ b/modules/two_factor_authentication/spec/features/backup_codes/generate_backup_codes_spec.rb @@ -1,5 +1,5 @@ require_relative "../../spec_helper" -require_relative "../shared_2fa_examples" +require_relative "../shared_two_factor_examples" RSpec.describe "Generate 2FA backup codes", :js, with_config: { "2fa": { active_strategies: [:developer] } } do let(:user_password) { "bob!" * 4 } diff --git a/modules/two_factor_authentication/spec/features/backup_codes/login_with_backup_code_spec.rb b/modules/two_factor_authentication/spec/features/backup_codes/login_with_backup_code_spec.rb index a8b1cd7fcefb..f858e7f7aae1 100644 --- a/modules/two_factor_authentication/spec/features/backup_codes/login_with_backup_code_spec.rb +++ b/modules/two_factor_authentication/spec/features/backup_codes/login_with_backup_code_spec.rb @@ -1,5 +1,5 @@ require_relative "../../spec_helper" -require_relative "../shared_2fa_examples" +require_relative "../shared_two_factor_examples" RSpec.describe "Login with 2FA backup code", :js, @@ -7,6 +7,8 @@ with_settings: { plugin_openproject_two_factor_authentication: { "active_strategies" => [:developer] } } do + include SharedTwoFactorExamples + let(:user_password) { "bob!" * 4 } let(:user) do create(:user, diff --git a/modules/two_factor_authentication/spec/features/login/login_enforced_2fa_spec.rb b/modules/two_factor_authentication/spec/features/login/login_enforced_2fa_spec.rb index 6701fd82b24c..1e2dc96b06c7 100644 --- a/modules/two_factor_authentication/spec/features/login/login_enforced_2fa_spec.rb +++ b/modules/two_factor_authentication/spec/features/login/login_enforced_2fa_spec.rb @@ -1,5 +1,5 @@ require_relative "../../spec_helper" -require_relative "../shared_2fa_examples" +require_relative "../shared_two_factor_examples" RSpec.describe "Login with enforced 2FA", :js, @@ -10,6 +10,7 @@ "enforced" => true } } do + include SharedTwoFactorExamples let(:user_password) { "bob!" * 4 } let(:user) do create(:user, diff --git a/modules/two_factor_authentication/spec/features/login/login_with_2fa_spec.rb b/modules/two_factor_authentication/spec/features/login/login_with_2fa_spec.rb index 8eff3e7d0be9..3b38562cad37 100644 --- a/modules/two_factor_authentication/spec/features/login/login_with_2fa_spec.rb +++ b/modules/two_factor_authentication/spec/features/login/login_with_2fa_spec.rb @@ -1,5 +1,5 @@ require_relative "../../spec_helper" -require_relative "../shared_2fa_examples" +require_relative "../shared_two_factor_examples" RSpec.describe "Login with 2FA device", :js, @@ -9,6 +9,7 @@ "active_strategies" => [:developer] } } do + include SharedTwoFactorExamples let(:user_password) { "bob!" * 4 } let(:user) do create(:user, diff --git a/modules/two_factor_authentication/spec/features/login/login_with_deleted_2fa_spec.rb b/modules/two_factor_authentication/spec/features/login/login_with_deleted_2fa_spec.rb index 16ea18886bcf..6eed2c36addc 100644 --- a/modules/two_factor_authentication/spec/features/login/login_with_deleted_2fa_spec.rb +++ b/modules/two_factor_authentication/spec/features/login/login_with_deleted_2fa_spec.rb @@ -1,5 +1,5 @@ require_relative "../../spec_helper" -require_relative "../shared_2fa_examples" +require_relative "../shared_two_factor_examples" RSpec.describe "Login after 2FA deleted 2FA was deleted (REGRESSION)", :js, @@ -9,6 +9,7 @@ "active_strategies" => %i[developer totp] } } do + include SharedTwoFactorExamples let(:user_password) { "bob!" * 4 } let(:user) do create(:user, diff --git a/modules/two_factor_authentication/spec/features/login/login_without_2fa_spec.rb b/modules/two_factor_authentication/spec/features/login/login_without_2fa_spec.rb index 34565fca2a2d..f68745efbd87 100644 --- a/modules/two_factor_authentication/spec/features/login/login_without_2fa_spec.rb +++ b/modules/two_factor_authentication/spec/features/login/login_without_2fa_spec.rb @@ -1,10 +1,11 @@ require_relative "../../spec_helper" -require_relative "../shared_2fa_examples" +require_relative "../shared_two_factor_examples" RSpec.describe "Login with no required OTP", :js, :with_cuprite, with_config: { "2fa": { active_strategies: [:developer] } } do + include SharedTwoFactorExamples let(:user_password) { "bob!" * 4 } let(:user) do create(:user, diff --git a/modules/two_factor_authentication/spec/features/login/switch_available_devices_spec.rb b/modules/two_factor_authentication/spec/features/login/switch_available_devices_spec.rb index 086daa515d84..a5ff551ac6ce 100644 --- a/modules/two_factor_authentication/spec/features/login/switch_available_devices_spec.rb +++ b/modules/two_factor_authentication/spec/features/login/switch_available_devices_spec.rb @@ -1,5 +1,5 @@ require_relative "../../spec_helper" -require_relative "../shared_2fa_examples" +require_relative "../shared_two_factor_examples" RSpec.describe "Login by switching 2FA device", :js, @@ -7,6 +7,8 @@ with_settings: { plugin_openproject_two_factor_authentication: { "active_strategies" => %i[developer totp] } } do + include SharedTwoFactorExamples + let(:user_password) { "bob!" * 4 } let(:user) do create(:user, diff --git a/modules/two_factor_authentication/spec/features/remember_cookie/login_with_remember_cookie_spec.rb b/modules/two_factor_authentication/spec/features/remember_cookie/login_with_remember_cookie_spec.rb index 5a02d1438094..c936c8d58c9c 100644 --- a/modules/two_factor_authentication/spec/features/remember_cookie/login_with_remember_cookie_spec.rb +++ b/modules/two_factor_authentication/spec/features/remember_cookie/login_with_remember_cookie_spec.rb @@ -1,5 +1,5 @@ require_relative "../../spec_helper" -require_relative "../shared_2fa_examples" +require_relative "../shared_two_factor_examples" RSpec.describe "Login with 2FA remember cookie", :js, @@ -10,6 +10,8 @@ allow_remember_for_days: 30 } } do + include SharedTwoFactorExamples + let(:user_password) do "user!user!" end diff --git a/modules/two_factor_authentication/spec/features/shared_2fa_examples.rb b/modules/two_factor_authentication/spec/features/shared_two_factor_examples.rb similarity index 66% rename from modules/two_factor_authentication/spec/features/shared_2fa_examples.rb rename to modules/two_factor_authentication/spec/features/shared_two_factor_examples.rb index 367cfbe75b19..3ef56d75a8af 100644 --- a/modules/two_factor_authentication/spec/features/shared_2fa_examples.rb +++ b/modules/two_factor_authentication/spec/features/shared_two_factor_examples.rb @@ -1,36 +1,38 @@ -def first_login_step - visit signin_path - within("#login-form") do - fill_in("username", with: user.login) - fill_in("password", with: user_password) - click_link_or_button I18n.t(:button_login) +module SharedTwoFactorExamples + def first_login_step + visit signin_path + within("#login-form") do + fill_in("username", with: user.login) + fill_in("password", with: user_password) + click_link_or_button I18n.t(:button_login) + end + wait_for_network_idle end - wait_for_network_idle -end -def switch_two_factor_device(device) - within("#login-form") do - click_link_or_button I18n.t(:text_otp_not_receive) - click_link_or_button device.redacted_identifier + def switch_two_factor_device(device) + within("#login-form") do + click_link_or_button I18n.t(:text_otp_not_receive) + click_link_or_button device.redacted_identifier + end end -end -def two_factor_step(token) - expect(page).to have_css("input#otp") - fill_in "otp", with: token - click_button I18n.t(:button_login) - wait_for_network_idle -end + def two_factor_step(token) + expect(page).to have_css("input#otp") + fill_in "otp", with: token + click_button I18n.t(:button_login) + wait_for_network_idle + end -def expect_logged_in - visit my_account_path - wait_for_network_idle - expect(page).to have_css(".form--field-container", text: user.login) -end + def expect_logged_in + visit my_account_path + wait_for_network_idle + expect(page).to have_css(".form--field-container", text: user.login) + end -def expect_not_logged_in - visit my_account_path - expect(page).to have_no_css(".form--field-container", text: user.login) + def expect_not_logged_in + visit my_account_path + expect(page).to have_no_css(".form--field-container", text: user.login) + end end RSpec.shared_examples "login without 2FA" do