feat: move devise_ldap_authenticatable to net-ldap #331

brunto · 2025-12-17T14:19:01Z

This pull request replaces the use of the devise_ldap_authenticatable gem with a new custom LDAP authentication implementation using the net-ldap gem. It introduces a new LdapAdapter service for handling LDAP operations, updates configuration files, and removes references to the old gem and its settings. The changes improve maintainability and flexibility of LDAP integration.

LDAP Authentication Refactor:

Removed the devise_ldap_authenticatable gem and added the net-ldap gem to the Gemfile, shifting LDAP authentication to a custom implementation.
Replaced all usages of Devise::LDAP::Adapter in the codebase with the new LdapAdapter service, including user credential validation and attribute retrieval in user.rb. [1] [2]
Added a new app/services/ldap_adapter.rb file, which encapsulates LDAP connection handling, credential validation, and parameter retrieval, using configuration from ldap.yml and Rails credentials.

Configuration Updates:

Removed all devise_ldap_authenticatable-specific configuration from config/initializers/devise.rb, as it is no longer needed.
Refactored config/ldap.yml to simplify and clarify environment-specific LDAP settings, removing legacy authorization settings and using Rails credentials for sensitive values. Test environment now supports both mock and real LDAP servers via credentials. [1] [2]

Codebase Cleanup:

Removed exception handling specific to DeviseLdapAuthenticatable from application_controller.rb, as the new implementation handles errors internally.
Updated the Doorkeeper initializer to use the new LDAP authentication logic and clarified environment variable handling for enabling LDAP.

Copilot

Pull request overview

This pull request modernizes LDAP authentication by replacing the devise_ldap_authenticatable gem with a custom implementation using the net-ldap gem directly. The change provides better control over LDAP operations and simplifies the codebase by removing unused authorization features.

Introduces a new LdapAdapter service that encapsulates all LDAP operations (credential validation, parameter retrieval, and connection management)
Replaces all references to Devise::LDAP::Adapter with the new LdapAdapter throughout the codebase
Streamlines LDAP configuration by removing unused authorization settings and centralizing configuration in ldap.yml with Rails credentials

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
app/services/ldap_adapter.rb	New service implementing LDAP authentication with credential validation, parameter retrieval, and configurable SSL/TLS support
test/services/ldap_adapter_test.rb	Comprehensive test suite covering all LdapAdapter methods including edge cases and SSL configuration
config/ldap.yml	Simplified configuration with improved documentation and Rails credentials integration for all environments
config/initializers/doorkeeper.rb	Updated to use new LdapAdapter API and improved boolean casting for LDAP enablement check
config/initializers/devise.rb	Removed all devise_ldap_authenticatable-specific configuration
app/models/user.rb	Updated LDAP method calls to use new LdapAdapter
app/controllers/application_controller.rb	Removed DeviseLdapAuthenticatable exception handling no longer needed
Gemfile & Gemfile.lock	Replaced devise_ldap_authenticatable with net-ldap dependency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-17T14:35:00Z

app/services/ldap_adapter.rb

+    def config
+      @config ||= begin
+        config_file = Rails.root.join('config', 'ldap.yml')
+        if File.exist?(config_file)
+          # Process ERB template before parsing YAML
+          erb_content = ERB.new(File.read(config_file)).result
+          yaml_config = YAML.safe_load(erb_content, aliases: true)
+          env_config = yaml_config[Rails.env] || {}
+
+          # Convert string keys to symbols
+          env_config.transform_keys(&:to_sym)
+        else
+          Rails.logger.error "LDAP: Configuration file not found at #{config_file}"
+          {}
+        end
+      end
+    end


Concurrency concern: The @config class instance variable is not thread-safe. In a multi-threaded environment (like Puma in threaded mode), race conditions could occur when multiple threads call config() simultaneously before @config is initialized. Consider using a Mutex or Rails' built-in thread-safe memoization patterns like ActiveSupport::PerThreadRegistry or class_attribute with thread safety.

Copilot · 2025-12-17T14:35:00Z

app/services/ldap_adapter.rb

+        Rails.logger.info "LDAP: Found DN for user #{login}: #{dn}"
+        dn
+      else
+        Rails.logger.warn "LDAP: Could not find DN for user #{login}"


Security concern: The Distinguished Name (DN) and login identifier are being logged. The DN may contain sensitive organizational information. Consider sanitizing or redacting this information in log messages, especially in production environments, to comply with security and privacy best practices.

Copilot · 2025-12-17T14:35:00Z

config/ldap.yml

+#   - ldap_base: Base DN for user searches (e.g., 'ou=people,dc=example,dc=com')
+#   - ldap_admin_user: Admin bind DN (e.g., 'cn=admin,dc=example,dc=com')
+#   - ldap_admin_user_password: Admin bind password
+#   - ldap_ssl: Encryption method ('simple_tls', 'start_tls', or false for no encryption)


The configuration documentation states that ldap_ssl can be 'simple_tls', 'start_tls', or false for no encryption. However, the test environment defaults to 'simple_tls' as a string. Consider documenting that false should be passed as a boolean value, not the string 'false', to ensure proper encryption handling. The LdapAdapter code checks for presence with .present? which treats both empty strings and false differently.

Suggested change

# - ldap_ssl: Encryption method ('simple_tls', 'start_tls', or false for no encryption)

# - ldap_ssl: Encryption method ('simple_tls', 'start_tls', or boolean false for no encryption; use false, not the string 'false')

Copilot · 2025-12-17T14:35:00Z

app/services/ldap_adapter.rb

+      ldap = create_ldap_connection
+      return false unless ldap


Performance concern: A new LDAP connection is created for each authentication attempt and parameter retrieval. This could lead to performance issues under high load. Consider implementing connection pooling or reusing connections where appropriate, especially if multiple LDAP operations are performed in sequence.

Copilot · 2025-12-17T14:35:01Z

app/services/ldap_adapter.rb

+    rescue StandardError => e
+      Rails.logger.error "LDAP: Error validating credentials for #{login}: #{e.message}"
+      false
+    end


The rescue clause catches all StandardError exceptions but only logs them. Consider providing more specific error handling for different LDAP error scenarios (e.g., connection timeouts, invalid filters, server unavailable). This would allow for better error reporting and potentially different retry strategies based on the error type.

Copilot · 2025-12-17T14:35:01Z

app/services/ldap_adapter.rb

+        Rails.logger.info "LDAP: Successfully authenticated user: #{login}"
+      else
+        Rails.logger.warn "LDAP: Failed to authenticate user: #{login} - #{ldap.get_operation_result.message}"


Security concern: The login identifier is being logged in error and warning messages. This could expose sensitive user information in log files. Consider sanitizing or redacting the login identifier in log messages, especially in production environments, to comply with security and privacy best practices.

Copilot · 2025-12-17T14:35:02Z

app/services/ldap_adapter.rb

+        Rails.logger.info "LDAP: Retrieved #{param} for user #{login}"
+        value
+      else
+        Rails.logger.warn "LDAP: Could not find #{param} for user #{login}"


Security concern: The login identifier is being logged in log messages. This could expose sensitive user information in log files. Consider sanitizing or redacting the login identifier in log messages, especially in production environments, to comply with security and privacy best practices.

Copilot · 2025-12-17T14:35:02Z

app/services/ldap_adapter.rb

+        if File.exist?(config_file)
+          # Process ERB template before parsing YAML
+          erb_content = ERB.new(File.read(config_file)).result
+          yaml_config = YAML.safe_load(erb_content, aliases: true)


The YAML.safe_load method with the 'aliases: true' parameter may not be supported in older versions of Ruby. In Ruby < 3.1, the parameter name is 'aliases' not in keyword argument form. Consider using 'permitted_classes' and 'aliases' parameters in a way that's compatible with the Ruby version being used, or verify that the project requires Ruby 3.1+.

Suggested change

yaml_config = YAML.safe_load(erb_content, aliases: true)

yaml_config = YAML.safe_load(erb_content, [], [], true)

Copilot · 2025-12-17T14:35:02Z

test/services/ldap_adapter_test.rb

+  test "creates LDAP connection without encryption when ssl is false" do
+    mock_config = {
+      host: 'ldap.test.com',
+      port: 389,
+      base: 'ou=people,dc=test,dc=com',
+      attribute: 'cn',
+      ssl: false
+    }
+
+    connection_created = false
+    Net::LDAP.stub :new, ->(options) {
+      connection_created = true
+      assert_nil options[:encryption], "Expected no encryption when ssl is false"
+      Minitest::Mock.new
+    } do
+      LdapAdapter.stub :config, mock_config do
+        LdapAdapter.get_ldap_param('testuser', 'mail')
+      end
+    end
+
+    assert connection_created, "Expected LDAP connection to be created"
+  end


The test description says "when ssl is false" but this only tests the default case. Consider adding explicit test cases for when ssl is explicitly set to false as a boolean (not just checking nil encryption). Additionally, the assertion message could be clearer: "Expected encryption to be nil when ssl is false" would be more precise.

Copilot · 2025-12-17T14:35:02Z

config/initializers/doorkeeper.rb


    # LDAP IS ACTIVE
-    if ENV['DEVISE_LDAP_LOGGER'].present? && User.check_ldap_credentials(login, password) # ldap valide
+    if ActiveModel::Type::Boolean.new.cast(ENV['DEVISE_LDAP_LOGGER']) && User.check_ldap_credentials(login, password)


The environment variable name 'DEVISE_LDAP_LOGGER' is misleading and inconsistent with its actual purpose. This variable is being used to enable/disable LDAP authentication, not configure logging. Consider renaming it to something more appropriate like 'ENABLE_LDAP_AUTH' or 'LDAP_ENABLED' to better reflect its function and improve code maintainability.

Suggested change

if ActiveModel::Type::Boolean.new.cast(ENV['DEVISE_LDAP_LOGGER']) && User.check_ldap_credentials(login, password)

ldap_enabled =

if ENV.key?('ENABLE_LDAP_AUTH')

ActiveModel::Type::Boolean.new.cast(ENV['ENABLE_LDAP_AUTH'])

else

# Backwards compatibility: fall back to legacy DEVISE_LDAP_LOGGER flag

ActiveModel::Type::Boolean.new.cast(ENV['DEVISE_LDAP_LOGGER'])

end

if ldap_enabled && User.check_ldap_credentials(login, password)

brunto requested review from Copilot and kevin-atnos December 17, 2025 14:19

Copilot started reviewing on behalf of brunto December 17, 2025 14:19 View session

brunto force-pushed the ldap_migration branch from c56ec02 to ab8cbfe Compare December 17, 2025 14:20

Copilot AI reviewed Dec 17, 2025

View reviewed changes

feat: move devise_ldap_authenticatable to net-ldap

9472075

brunto force-pushed the ldap_migration branch from ab8cbfe to 9472075 Compare January 6, 2026 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: move devise_ldap_authenticatable to net-ldap #331

feat: move devise_ldap_authenticatable to net-ldap #331

Uh oh!

brunto commented Dec 17, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

kevin-atnos Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Copilot AI Dec 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	# - ldap_ssl: Encryption method ('simple_tls', 'start_tls', or false for no encryption)
	# - ldap_ssl: Encryption method ('simple_tls', 'start_tls', or boolean false for no encryption; use false, not the string 'false')

	yaml_config = YAML.safe_load(erb_content, aliases: true)
	yaml_config = YAML.safe_load(erb_content, [], [], true)

-    if ActiveModel::Type::Boolean.new.cast(ENV['DEVISE_LDAP_LOGGER']) && User.check_ldap_credentials(login, password)
+    ldap_enabled =
+      if ENV.key?('ENABLE_LDAP_AUTH')
+        ActiveModel::Type::Boolean.new.cast(ENV['ENABLE_LDAP_AUTH'])
+      else
+        # Backwards compatibility: fall back to legacy DEVISE_LDAP_LOGGER flag
+        ActiveModel::Type::Boolean.new.cast(ENV['DEVISE_LDAP_LOGGER'])
+      end
+    if ldap_enabled && User.check_ldap_credentials(login, password)

feat: move devise_ldap_authenticatable to net-ldap #331

Are you sure you want to change the base?

feat: move devise_ldap_authenticatable to net-ldap #331

Uh oh!

Conversation

brunto commented Dec 17, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

kevin-atnos Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants