Merge pull request #14980 from opf/fix/ldap-create

LDAP user can't be created if optional attributes are not mapped

app/models/user.rb

@@ -244,20 +244,10 @@ class User < Principal
   def self.try_authentication_and_create_user(login, password)
     return nil if OpenProject::Configuration.disable_password_login?
 
-    attrs = LdapAuthSource.authenticate(login, password)
-    return unless attrs
+    user = LdapAuthSource.authenticate(login, password)
 
-    call = Users::CreateService
-      .new(user: User.system)
-      .call(attrs)
-
-    user = call.result
-
-    call.on_failure do |result|
-      Rails.logger.error "Failed to auto-create user from auth-source: #{result.message}"
-
-      # TODO We have no way to pass back the contract errors in this place
-      user.errors.merge! call.errors
+    if user&.new_record?
+      Rails.logger.error "Failed to auto-create user from auth-source, as data is missing."
     end
 
     user

app/services/ldap/base_service.rb

@@ -61,6 +61,8 @@ module Ldap
       if call.success?
         Rails.logger.info { "[LDAP user sync] User '#{call.result.login}' created." }
       else
+        # Ensure contract errors are merged into the user
+        call.result.errors.merge! call.errors
         Rails.logger.error { "[LDAP user sync] User '#{attrs[:login]}' could not be created: #{call.message}" }
       end
 

modules/ldap_groups/lib/tasks/ldap_groups.rake

@@ -58,36 +58,6 @@ namespace :ldap_groups do
       ldif = ENV.fetch('LDIF_FILE') { Rails.root.join('spec/fixtures/ldap/users.ldif') }
       ldap_server = Ladle::Server.new(quiet: false, port: '12389', domain: 'dc=example,dc=com', ldif:).start
 
-      puts <<~INFO
-        LDAP server ready at localhost:12389
-        Users Base dn: ou=people,dc=example,dc=com
-        Admin account: uid=admin,ou=system
-        Admin password: secret
-
-        --------------------------------------------------------
-
-        Attributes
-        Login: uid
-        First name: givenName
-        Last name: sn
-        Email: mail
-        Admin: isAdmin
-        memberOf: (Hard-coded, not virtual)
-
-        --------------------------------------------------------
-
-        Users:
-        uid=aa729,ou=people,dc=example,dc=com (Password: smada)
-        uid=bb459,ou=people,dc=example,dc=com (Password: niwdlab)
-        uid=cc414,ou=people,dc=example,dc=com (Password: retneprac)
-
-        --------------------------------------------------------
-
-        Groups:
-        cn=foo,ou=groups,dc=example,dc=com (Members: aa729)
-        cn=bar,ou=groups,dc=example,dc=com (Members: aa729, bb459, cc414)
-      INFO
-
       puts "Creating a connection called ladle"
       source = LdapAuthSource.find_or_initialize_by(name: 'ladle local development')
 
@@ -118,6 +88,36 @@ namespace :ldap_groups do
 
       LdapGroups::SynchronizationJob.perform_now
 
+      puts <<~INFO
+        LDAP server ready at localhost:12389
+        Users Base dn: ou=people,dc=example,dc=com
+        Admin account: uid=admin,ou=system
+        Admin password: secret
+
+        --------------------------------------------------------
+
+        Attributes
+        Login: uid
+        First name: givenName
+        Last name: sn
+        Email: mail
+        Admin: isAdmin
+        memberOf: (Hard-coded, not virtual)
+
+        --------------------------------------------------------
+
+        Users:
+        uid=aa729,ou=people,dc=example,dc=com (Password: smada)
+        uid=bb459,ou=people,dc=example,dc=com (Password: niwdlab)
+        uid=cc414,ou=people,dc=example,dc=com (Password: retneprac)
+
+        --------------------------------------------------------
+
+        Groups:
+        cn=foo,ou=groups,dc=example,dc=com (Members: aa729)
+        cn=bar,ou=groups,dc=example,dc=com (Members: aa729, bb459, cc414)
+      INFO
+
       puts "Send CTRL+D to stop the server"
       require 'irb'
       binding.irb

spec/controllers/account_controller_spec.rb

@@ -218,28 +218,6 @@ RSpec.describe AccountController, :skip_2fa_stage do
         expect(response).to redirect_to my_page_path
       end
 
-      context 'with an auth source',
-              with_settings: { self_registration: Setting::SelfRegistration.disabled } do
-        let(:auth_source) { create(:ldap_auth_source) }
-
-        it 'creates the user on the fly' do
-          allow(LdapAuthSource)
-            .to(receive(:authenticate))
-            .and_return(login: 'foo',
-                        firstname: 'Foo',
-                        lastname: 'Smith',
-                        mail: 'foo@bar.com',
-                        ldap_auth_source_id: auth_source.id)
-          post :login, params: { username: 'foo', password: 'bar' }
-
-          expect(response).to redirect_to home_url(first_time_user: true)
-          user = User.find_by(login: 'foo')
-          expect(user).to be_an_instance_of User
-          expect(user.ldap_auth_source_id).to eq(auth_source.id)
-          expect(user.current_password).to be_nil
-        end
-      end
-
       context 'with a relative url root' do
         around do |example|
           old_relative_url_root = OpenProject::Configuration['rails_relative_url_root']
@@ -434,48 +412,6 @@ RSpec.describe AccountController, :skip_2fa_stage do
         expect(response).to have_http_status :not_found
       end
     end
-
-    context 'with an auth source',
-            with_settings: { self_registration: Setting::SelfRegistration.disabled } do
-      let(:auth_source) { create(:ldap_auth_source) }
-
-      let(:user_attributes) do
-        {
-          login: 's.scallywag',
-          firstname: 'Scarlet',
-          lastname: 'Scallywag',
-          mail: 's.scallywag@openproject.com',
-          ldap_auth_source_id: auth_source.id
-        }
-      end
-
-      let(:authenticate) { true }
-
-      before do
-        allow(LdapAuthSource).to receive(:authenticate).and_return(authenticate ? user_attributes : nil)
-
-        # required so that the register view can be rendered
-        allow_any_instance_of(User).to receive(:change_password_allowed?).and_return(false) # rubocop:disable RSpec/AnyInstance
-      end
-
-      context 'with user limit reached' do
-        render_views
-
-        before do
-          allow(OpenProject::Enterprise).to receive(:user_limit_reached?).and_return(true)
-
-          post :login, params: { username: 'foo', password: 'bar' }
-        end
-
-        it 'shows the user limit error' do
-          expect(response.body).to have_text "User limit reached"
-        end
-
-        it 'renders the register form' do
-          expect(response.body).to include "/account/register"
-        end
-      end
-    end
   end
 
   describe '#login with omniauth_direct_login enabled',
@@ -893,36 +829,6 @@ RSpec.describe AccountController, :skip_2fa_stage do
             with_settings: { self_registration: Setting::SelfRegistration.disabled } do
       before do
         allow_any_instance_of(User).to receive(:change_password_allowed?).and_return(false) # rubocop:disable RSpec/AnyInstance
-        allow(LdapAuthSource).to receive(:authenticate).and_return(login: 'foo',
-                                                                   lastname: 'Smith',
-                                                                   ldap_auth_source_id: 66)
-      end
-
-      context 'with password login enabled' do
-        before do
-          post :login, params: { username: 'foo', password: 'bar' }
-        end
-
-        it 'registers the user on-the-fly' do
-          expect(subject).to respond_with :success
-          expect(response).to render_template :register
-
-          post :register,
-               params: {
-                 user: {
-                   firstname: 'Foo',
-                   lastname: 'Smith',
-                   mail: 'foo@bar.com'
-                 }
-               }
-          expect(response).to redirect_to home_path(first_time_user: true)
-
-          user = User.find_by_login('foo') # rubocop:disable Rails/DynamicFindBy
-
-          expect(user).to be_an_instance_of(User)
-          expect(user.ldap_auth_source_id).to be 66
-          expect(user.current_password).to be_nil
-        end
       end
 
       context 'with password login disabled' do
@@ -930,16 +836,6 @@ RSpec.describe AccountController, :skip_2fa_stage do
           allow(OpenProject::Configuration).to receive(:disable_password_login?).and_return(true)
         end
 
-        describe 'login' do
-          before do
-            post :login, params: { username: 'foo', password: 'bar' }
-          end
-
-          it 'is not found' do
-            expect(response).to have_http_status :not_found
-          end
-        end
-
         describe 'registration' do
           before do
             post :register,

spec/requests/auth/ldap_sso_spec.rb

@@ -0,0 +1,97 @@
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2024 the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See COPYRIGHT and LICENSE files for more details.
+#++
+
+require 'spec_helper'
+
+RSpec.describe 'LDAP authentication',
+               :skip_2fa_stage,
+               :skip_csrf,
+               type: :rails_request do
+
+  include_context 'with temporary LDAP'
+
+  let(:username) { 'aa729' }
+  let(:password) { 'smada' }
+  let(:params) { { username:, password: } }
+
+  subject do
+    post(signin_path, params:)
+    response
+  end
+
+  context 'when LDAP is onthefly_register' do
+    let(:onthefly_register) { true }
+
+    it 'creates the user on the fly' do
+      expect(User.find_by(login: 'aa729')).to be_nil
+
+      expect { subject }.to change(User.not_builtin.active, :count).by(1)
+
+      user = User.find_by(login: 'aa729')
+      expect(user).to be_present
+      expect(user).to be_active
+      expect(session[:user_id]).to eq user.id
+      expect(subject).to redirect_to '/?first_time_user=true'
+    end
+
+    context 'when not all attributes present' do
+      let(:attr_mail) { nil }
+
+      it 'does not save the user, but forwards to registration form' do
+        expect(User.find_by(login: 'aa729')).to be_nil
+
+        expect { subject }.not_to change(User.not_builtin.active, :count)
+        expect(subject).to render_template 'account/register'
+        expect(subject.body).to have_text "Email can't be blank"
+      end
+    end
+
+    context 'with user limit reached' do
+      before do
+        allow(OpenProject::Enterprise).to receive(:user_limit_reached?).and_return(true)
+      end
+
+      it 'shows the user limit error' do
+        expect(subject.body).to have_text "User limit reached"
+        expect(subject.body).to include "/account/register"
+      end
+    end
+
+    context 'with password login disabled' do
+      before do
+        allow(OpenProject::Configuration).to receive(:disable_password_login?).and_return(true)
+      end
+
+      describe 'login' do
+        it 'is not found' do
+          expect(subject).to have_http_status :not_found
+        end
+      end
+    end
+  end
+end

spec/support/shared/with_test_ldap.rb

@@ -54,12 +54,16 @@ RSpec.shared_context 'with temporary LDAP' do
            onthefly_register:,
            filter_string: ldap_filter,
            attr_login: 'uid',
-           attr_firstname: 'givenName',
-           attr_lastname: 'sn',
-           attr_mail: 'mail',
-           attr_admin: 'isAdmin')
+           attr_firstname:,
+           attr_lastname:,
+           attr_mail:,
+           attr_admin:)
   end
 
   let(:onthefly_register) { false }
   let(:ldap_filter) { nil }
+  let(:attr_firstname) { 'givenName' }
+  let(:attr_lastname) { 'sn' }
+  let(:attr_mail) { 'mail' }
+  let(:attr_admin) { 'isAdmin' }
 end