From e400fd7e4cb057b62fca1fdf02417286165cdea5 Mon Sep 17 00:00:00 2001
From: Jan Sandbrink <j.sandbrink@openproject.com>
Date: Fri, 7 Nov 2025 15:49:14 +0100
Subject: [PATCH] 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 49f30176126..b4df8378471 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -369,8 +369,8 @@ class User < Principal
 
   # 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 2876829199f..5ac803a00b8 100644
--- a/spec/controllers/users_controller_spec.rb
+++ b/spec/controllers/users_controller_spec.rb
@@ -787,7 +787,7 @@ RSpec.describe UsersController do
                      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 @@ RSpec.describe UsersController do
       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 70f1405ddf0..594f04c6103 100644
--- a/spec/factories/user_factory.rb
+++ b/spec/factories/user_factory.rb
@@ -79,6 +79,11 @@ FactoryBot.define do
       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 a863d612c95..b3369c6bd5d 100644
--- a/spec/features/auth/lost_password_spec.rb
+++ b/spec/features/auth/lost_password_spec.rb
@@ -91,9 +91,9 @@ RSpec.describe "Lost password" do
     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 @@ RSpec.describe "Lost password" do
       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