Merge pull request #14069 from opf/chore/ldap-sync-login

Update users using their LDAP attributes when logging in

app/controllers/account_controller.rb

@@ -272,7 +272,7 @@ def change_password
   def auth_source_sso_failed
     failure = session.delete :auth_source_sso_failure
     login = failure[:login]
-    user = find_or_create_sso_user(login, save: false)
+    user = find_user_from_auth_source(login) || build_user_from_auth_source(login)
 
     if user.try(:new_record?)
       return onthefly_creation_failed user, login: user.login, ldap_auth_source_id: user.ldap_auth_source_id

app/controllers/concerns/auth_source_sso.rb

@@ -37,7 +37,8 @@ def perform_header_sso(login, user)
 
     Rails.logger.debug { "Starting header-based auth source SSO for #{header_name}='#{op_auth_header_value}'" }
 
-    user = find_or_create_sso_user(login, save: true)
+    # Try to find an existing, or autocreate a new user for onthefly ldap connections
+    user = LdapAuthSource.find_user(login)
     handle_sso_for! user, login
   end
 
@@ -110,43 +111,23 @@ def extract_from_header(value)
     end
   end
 
-  def find_or_create_sso_user(login, save: false)
-    find_user_from_auth_source(login) || create_user_from_auth_source(login, save:)
-  end
-
   def find_user_from_auth_source(login)
     User
       .by_login(login)
       .where.not(ldap_auth_source_id: nil)
       .first
   end
 
-  def create_user_from_auth_source(login, save:)
-    attrs = LdapAuthSource.find_user(login)
+  def build_user_from_auth_source(login)
+    attrs = LdapAuthSource.get_user_attributes(login)
     return unless attrs
 
-    attrs[:login] = login
-
-    call =
-      if save
-        Users::CreateService
-          .new(user: User.system)
-          .call(attrs)
-      else
-        Users::SetAttributesService
-          .new(model: User.new, user: User.system, contract_class: Users::CreateContract)
-          .call(attrs)
-      end
+    call = Users::SetAttributesService
+      .new(model: User.new, user: User.system, contract_class: Users::CreateContract)
+      .call(attrs.merge(login:))
 
     user = call.result
 
-    call.on_success do
-      logger.info(
-        "User '#{user.login}' created from external auth source: " +
-          "#{user.ldap_auth_source.type} - #{user.ldap_auth_source.name}"
-      )
-    end
-
     call.on_failure do
       logger.error "Tried to create user '#{login}' from external auth source but failed: #{call.message}"
     end

app/models/ldap_auth_source.rb

@@ -43,6 +43,7 @@ class Error < ::StandardError; end
   def self.unique_attribute
     :name
   end
+
   prepend ::Mixins::UniqueFinder
 
   enum tls_mode: {
@@ -69,27 +70,42 @@ def self.authenticate(login, password)
     where(onthefly_register: true).find_each do |source|
       begin
         Rails.logger.debug { "Authenticating '#{login}' against '#{source.name}'" }
-        attrs = source.authenticate(login, password)
+        user = source.authenticate(login, password)
       rescue StandardError => e
         Rails.logger.error "Error during authentication: #{e.message}"
-        attrs = nil
+        user = nil
       end
-      return attrs if attrs
+      return user if user
     end
     nil
   end
 
+  ##
+  # Find a user by login in any of the available sources.
+  # If it's an onthefly_register ldap connection, this might implictly create the user.
   def self.find_user(login)
+    find_each do |source|
+      Rails.logger.debug { "Looking up '#{login}' in '#{source.name}'" }
+      user = source.find_user login
+      return user if user
+    rescue StandardError => e
+      Rails.logger.error "Error during authentication: #{e.message}"
+    end
+
+    nil
+  end
+
+  def self.get_user_attributes(login)
     where(onthefly_register: true).find_each do |source|
       begin
         Rails.logger.debug { "Looking up '#{login}' in '#{source.name}'" }
-        attrs = source.find_user login
+        attrs = source.get_user_attributes login
       rescue StandardError => e
         Rails.logger.error "Error during authentication: #{e.message}"
         attrs = nil
       end
 
-      return attrs if attrs
+      return attrs.except(:dn) if attrs
     end
     nil
   end
@@ -109,11 +125,11 @@ def account_password=(arg)
   def authenticate(login, password)
     return nil if login.blank? || password.blank?
 
-    attrs = get_user_dn(login)
+    attrs = get_user_attributes(login)
 
     if attrs && attrs[:dn] && authenticate_dn(attrs[:dn], password)
       Rails.logger.debug { "Authentication successful for '#{login}'" }
-      attrs.except(:dn)
+      synchronize_user(login, attrs)
     end
   rescue Net::LDAP::Error => e
     raise LdapAuthSource::Error, "LdapError: #{e.message}"
@@ -122,16 +138,39 @@ def authenticate(login, password)
   def find_user(login)
     return nil if login.blank?
 
-    attrs = get_user_dn(login)
+    attrs = get_user_attributes(login)
 
     if attrs && attrs[:dn]
       Rails.logger.debug { "Lookup successful for '#{login}'" }
-      attrs.except(:dn)
+      synchronize_user(login, attrs)
     end
   rescue Net::LDAP::Error => e
     raise LdapAuthSource::Error, "LdapError: #{e.message}"
   end
 
+  # Get the user's dn and any attributes for them, given their login
+  def get_user_attributes(login)
+    ldap_con = initialize_ldap_con(account, account_password)
+
+    attrs = {}
+
+    filter = login_filter(login)
+    Rails.logger.debug do
+      "LDAP initializing search (BASE=#{base_dn}), (FILTER=#{filter})"
+    end
+
+    ldap_con.search(base: base_dn,
+                    filter:,
+                    attributes: search_attributes) do |entry|
+      attrs = get_user_attributes_from_ldap_entry(entry)
+      Rails.logger.debug { "DN found for #{login}: #{attrs[:dn]}" }
+    end
+
+    attrs
+  rescue Net::LDAP::Error => e
+    raise LdapAuthSource::Error, "LdapError: #{e.message}"
+  end
+
   # Open and return a system connection
   def with_connection
     yield initialize_ldap_con(account, account_password)
@@ -170,15 +209,8 @@ def mapped_attributes(entry)
 
   # Return the attributes needed for the LDAP search.
   #
-  # @param all_attributes [Boolean] Whether to return all user attributes
-  #
-  # By default, it will only include the user attributes if on-the-fly registration is enabled
-  def search_attributes(all_attributes = onthefly_register?)
-    if all_attributes
-      ['dn', attr_login, attr_firstname, attr_lastname, attr_mail, attr_admin].compact
-    else
-      ['dn', attr_login]
-    end
+  def search_attributes
+    ['dn', attr_login, attr_firstname, attr_lastname, attr_mail, attr_admin].compact
   end
 
   ##
@@ -217,6 +249,29 @@ def read_ldap_certificates
 
   private
 
+  def synchronize_user(login, attrs)
+    user = mapped_user(login)
+
+    # If onthefly_register is false, and the user is not found, do nothing
+    return if user.nil?
+
+    ::Ldap::PostLoginSyncService
+      .new(self, user:, attributes: attrs.except(:dn))
+      .call
+      .result
+  end
+
+  def mapped_user(login)
+    User.find_by(login:, ldap_auth_source_id: id) || onthefly_user(login)
+  end
+
+  def onthefly_user(login)
+    return unless onthefly_register?
+    return if User.by_login(login).exists?
+
+    User.new(login:, ldap_auth_source_id: id)
+  end
+
   def strip_ldap_attributes
     %i[attr_login attr_firstname attr_lastname attr_mail attr_admin].each do |attr|
       self[attr] = self[attr].strip unless self[attr].nil?
@@ -230,8 +285,7 @@ def initialize_ldap_con(ldap_user, ldap_password)
 
     options = ldap_connection_options
     unless ldap_user.blank? && ldap_password.blank?
-      options.merge!(auth: { method: :simple, username: ldap_user,
-                             password: ldap_password })
+      options[:auth] = { method: :simple, username: ldap_user, password: ldap_password }
     end
     Net::LDAP.new options
   end
@@ -275,33 +329,6 @@ def authenticate_dn(dn, password)
     end
   end
 
-  # Get the user's dn and any attributes for them, given their login
-  def get_user_dn(login)
-    ldap_con = initialize_ldap_con(account, account_password)
-
-    attrs = {}
-
-    filter = login_filter(login)
-    Rails.logger.debug do
-      "LDAP initializing search (BASE=#{base_dn}), (FILTER=#{filter})"
-    end
-
-    ldap_con.search(base: base_dn,
-                    filter:,
-                    attributes: search_attributes) do |entry|
-      attrs =
-        if onthefly_register?
-          get_user_attributes_from_ldap_entry(entry)
-        else
-          { dn: entry.dn }
-        end
-
-      Rails.logger.debug { "DN found for #{login}: #{attrs[:dn]}" }
-    end
-
-    attrs
-  end
-
   def self.get_attr(entry, attr_name)
     if attr_name.present?
       entry[attr_name].is_a?(Array) ? entry[attr_name].first : entry[attr_name]

app/services/ldap/base_service.rb

@@ -24,40 +24,47 @@ def synchronize_user(user, ldap_con)
       Rails.logger.debug { "[LDAP user sync] Synchronizing user #{user.login}." }
 
       update_attributes = user_attributes(user.login, ldap_con)
-      lock_user!(user) if update_attributes.nil? && user.persisted?
-      return unless update_attributes
+      synchronize_user_attributes(user, update_attributes)
+    end
 
-      if user.new_record?
-        try_to_create(update_attributes)
+    def synchronize_user_attributes(user, attributes)
+      if attributes.blank?
+        lock_user!(user)
+      elsif user.new_record?
+        try_to_create(attributes)
       else
-        try_to_update(user, update_attributes)
+        try_to_update(user, attributes)
       end
     end
 
     # Try to create the user from attributes
     def try_to_update(user, attrs)
       call = Users::UpdateService
         .new(model: user, user: User.system)
-        .call(attrs)
+        .call(attrs.merge(ldap_auth_source_id: ldap.id))
 
       if call.success?
         activate_user!(user)
         Rails.logger.info { "[LDAP user sync] User '#{call.result.login}' updated." }
       else
         Rails.logger.error { "[LDAP user sync] User '#{user.login}' could not be updated: #{call.message}" }
       end
+
+      call
     end
 
     def try_to_create(attrs)
       call = Users::CreateService
         .new(user: User.system)
-        .call(attrs)
+        .call(attrs.merge(ldap_auth_source_id: ldap.id))
 
       if call.success?
         Rails.logger.info { "[LDAP user sync] User '#{call.result.login}' created." }
       else
         Rails.logger.error { "[LDAP user sync] User '#{attrs[:login]}' could not be created: #{call.message}" }
       end
+
+      call
     end
 
     ##
@@ -113,7 +120,7 @@ def find_entries_by(login:, ldap_con: new_ldap_connection)
         .search(
           base: ldap.base_dn,
           filter: ldap.login_filter(login),
-          attributes: ldap.search_attributes(true)
+          attributes: ldap.search_attributes
         )
         .map { |entry| ldap.get_user_attributes_from_ldap_entry(entry).except(:dn) }
     end

app/services/ldap/import_users_from_filter_service.rb

@@ -22,7 +22,7 @@ def get_entries_from_filter(&)
       ldap_con.search(
         base: ldap.base_dn,
         filter: filter & ldap.default_filter,
-        attributes: ldap.search_attributes(true),
+        attributes: ldap.search_attributes,
         &
       )
     end

app/services/ldap/post_login_sync_service.rb

@@ -0,0 +1,21 @@
+module Ldap
+  class PostLoginSyncService < BaseService
+    attr_reader :user, :update_attributes
+
+    def initialize(ldap, user:, attributes:)
+      super(ldap)
+
+      @user = user
+      @update_attributes = attributes
+    end
+
+    private
+
+    def perform
+      synchronize_user_attributes(user, update_attributes)
+    rescue StandardError => e
+      Rails.logger.error { "Failed to synchronize user after login #{ldap.name}: #{e.message}" }
+      ServiceResult.failure(message: "Failed to synchronize user #{user.login}: #{e.message}")
+    end
+  end
+end

app/services/users/login_service.rb

@@ -115,14 +115,21 @@ def session_identification
     end
 
     def retained_session_values
+      controller.session.to_h.slice *(default_retained_keys + omniauth_provider_keys)
+    end
+
+    def omniauth_provider_keys
       provider_name = session[:omniauth_provider]
-      return unless provider_name
+      return [] unless provider_name
 
       provider = ::OpenProject::Plugins::AuthPlugin.find_provider_by_name(provider_name)
-      return unless provider && provider[:retain_from_session]
+      return [] unless provider && provider[:retain_from_session]
+
+      provider[:retain_from_session]
+    end
 
-      retained_keys = provider[:retain_from_session] + ['omniauth_provider']
-      controller.session.to_h.slice(*retained_keys)
+    def default_retained_keys
+      %w[omniauth_provider user_from_auth_header]
     end
 
     ##