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