From e400fd7e4cb057b62fca1fdf02417286165cdea5 Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Fri, 7 Nov 2025 15:49:14 +0100 Subject: [PATCH 1/2] Allow changing password if it exists Previously we'd be hiding the "change password" dialog on the basis of an external authentication method existing. However, that's not enough, because (at least with user remapping enabled) it's possible that a user that logged in via password once, gained the ability to login through SSO afterwards. Such a user then can use both mean to authenticate, thus they also need to be able to change a potentially compromised password. Much more work is needed here: Users need to be aware that their password still works, they need to be able to delete a password if they only want to use SSO and maybe there's also a use case for deleting an SSO association and going back to password-based logins. However, all of these things require more UI changes and some proper product development first. This change is a first step to improve the situation. --- app/models/user.rb | 4 ++-- spec/controllers/users_controller_spec.rb | 17 +++++++++++++++- spec/factories/user_factory.rb | 5 +++++ spec/features/auth/lost_password_spec.rb | 24 +++++++++++++++++++++-- 4 files changed, 45 insertions(+), 5 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 49f30176126b..b4df83784711 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -369,8 +369,8 @@ def check_password?(clear_password, update_legacy: true) # Does the backend storage allow this user to change their password? def change_password_allowed? - return false if uses_external_authentication? || - OpenProject::Configuration.disable_password_login? + return false if OpenProject::Configuration.disable_password_login? + return false if uses_external_authentication? && current_password.nil? ldap_auth_source_id.blank? end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 2876829199f7..5ac803a00b88 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -787,7 +787,7 @@ current_user: :user context "with external authentication" do - let(:some_user) { create(:user, identity_url: "some:identity") } + let(:some_user) { create(:user, :passwordless, identity_url: "some:identity") } before do as_logged_in_user(admin) do @@ -801,6 +801,21 @@ end end + context "with external authentication and an existing password" do + let(:some_user) { create(:user, identity_url: "some:identity") } + + before do + as_logged_in_user(admin) do + put :update, params: { id: some_user.id, user: { force_password_change: "true" } } + end + some_user.reload + end + + it "accepts setting force_password_change" do + expect(some_user.force_password_change).to be(true) + end + end + context "with ldap auth source" do let(:ldap_auth_source) { create(:ldap_auth_source) } diff --git a/spec/factories/user_factory.rb b/spec/factories/user_factory.rb index 70f1405ddf02..594f04c6103c 100644 --- a/spec/factories/user_factory.rb +++ b/spec/factories/user_factory.rb @@ -79,6 +79,11 @@ end end + trait :passwordless do + password { nil } + password_confirmation { nil } + end + factory :admin, parent: :user, class: "User" do firstname { "OpenProject" } sequence(:lastname) { |n| "Admin#{n}" } diff --git a/spec/features/auth/lost_password_spec.rb b/spec/features/auth/lost_password_spec.rb index a863d612c951..b3369c6bd5d9 100644 --- a/spec/features/auth/lost_password_spec.rb +++ b/spec/features/auth/lost_password_spec.rb @@ -91,9 +91,9 @@ end end - context "when user has identity_url" do + context "when user only authenticates via SSO" do let!(:provider) { create(:saml_provider, slug: "saml", display_name: "The SAML provider") } - let!(:user) { create(:user, authentication_provider: provider) } + let!(:user) { create(:user, :passwordless, authentication_provider: provider) } it "sends an email with external auth info" do visit account_lost_password_path @@ -111,4 +111,24 @@ expect(mail.body.parts.first.body.to_s).to include "(The SAML provider)" end end + + context "when authenticates via password & SSO" do + let!(:provider) { create(:saml_provider, slug: "saml", display_name: "The SAML provider") } + let!(:user) { create(:user, authentication_provider: provider) } + + it "sends an email with external auth info" do + visit account_lost_password_path + + # shows same flash for invalid and existing users + fill_in "mail", with: user.mail + click_on "Submit" + + expect_flash(message: I18n.t(:notice_account_lost_email_sent)) + + perform_enqueued_jobs + expect(ActionMailer::Base.deliveries.size).to be 1 + mail = ActionMailer::Base.deliveries.first + expect(mail.subject).to eq I18n.t("mail_subject_lost_password", value: Setting.app_title) + end + end end From ea71a616b98ee4394ba1386c9d6f08efb89a6d22 Mon Sep 17 00:00:00 2001 From: Jan Sandbrink Date: Mon, 10 Nov 2025 14:00:27 +0100 Subject: [PATCH 2/2] Test forgot password flow stricter Previously we tested nothing in the email, not even the subject. This means a broken implementation that always would send "can't change password" would not have been discovered. The new test checks the subject (comparable to the other test cases in the same file) and also does not simply assume that the mail will contain the correct link, but rather uses the link from the mail. --- spec/features/auth/lost_password_spec.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/spec/features/auth/lost_password_spec.rb b/spec/features/auth/lost_password_spec.rb index b3369c6bd5d9..91b8e354f328 100644 --- a/spec/features/auth/lost_password_spec.rb +++ b/spec/features/auth/lost_password_spec.rb @@ -52,10 +52,14 @@ perform_enqueued_jobs expect(ActionMailer::Base.deliveries.size).to be 1 + mail = ActionMailer::Base.deliveries.first + expect(mail.subject).to eq I18n.t("mail_subject_lost_password", value: Setting.app_title) # mimic the user clicking on the link in the mail token = Token::Recovery.first - visit account_lost_password_path(token: token.value) + mail_body = mail.body.parts.find { |p| p.mime_type == "text/html" }.body.to_s + mail_document = Capybara::Node::Simple.new(mail_body) + visit mail_document.find("a")["href"] fill_in "New password", with: new_password fill_in "Confirmation", with: new_password