Merge pull request #17219 from opf/bug/59408-signing-in-after-two-fac…

…tor-methods-have-been-deleted-lead-to-a-500-error

[#59408] Signing in after two factor methods have been deleted lead to a 500 error

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
 

modules/two_factor_authentication/spec/features/account_activation_spec.rb

@@ -1,12 +1,14 @@
 require_relative "../spec_helper"
-require_relative "shared_2fa_examples"
+require_relative "shared_two_factor_examples"
 
 RSpec.describe "activating an invited account",
                :js,
                :with_cuprite,
                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

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 }

modules/two_factor_authentication/spec/features/backup_codes/login_with_backup_code_spec.rb

@@ -1,12 +1,14 @@
 require_relative "../../spec_helper"
-require_relative "../shared_2fa_examples"
+require_relative "../shared_two_factor_examples"
 
 RSpec.describe "Login with 2FA backup code",
                :js,
                :with_cuprite,
                with_settings: {
                  plugin_openproject_two_factor_authentication: { "active_strategies" => [:developer] }
                } do
+  include SharedTwoFactorExamples
+
   let(:user_password) { "bob!" * 4 }
   let(:user) do
     create(:user,

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,

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,

modules/two_factor_authentication/spec/features/login/login_with_deleted_2fa_spec.rb

@@ -0,0 +1,54 @@
+require_relative "../../spec_helper"
+require_relative "../shared_two_factor_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
+  include SharedTwoFactorExamples
+  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

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,

modules/two_factor_authentication/spec/features/login/switch_available_devices_spec.rb

@@ -1,12 +1,14 @@
 require_relative "../../spec_helper"
-require_relative "../shared_2fa_examples"
+require_relative "../shared_two_factor_examples"
 
 RSpec.describe "Login by switching 2FA device",
                :js,
                :with_cuprite,
                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,

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

modules/two_factor_authentication/spec/features/shared_2fa_examples.rb → modules/two_factor_authentication/spec/features/shared_two_factor_examples.rb

@@ -1,29 +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 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 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 expect_logged_in
-  visit my_account_path
-  wait_for_network_idle
-  expect(page).to have_css(".form--field-container", text: user.login)
-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_not_logged_in
-  visit my_account_path
-  expect(page).to have_no_css(".form--field-container", text: user.login)
+  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)
+  end
 end
 
 RSpec.shared_examples "login without 2FA" do