-
Notifications
You must be signed in to change notification settings - Fork 72
feat: move devise_ldap_authenticatable to net-ldap #331
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,176 @@ | ||||||||||||||||
| # frozen_string_literal: true | ||||||||||||||||
|
|
||||||||||||||||
| require 'net/ldap' | ||||||||||||||||
|
|
||||||||||||||||
| class LdapAdapter | ||||||||||||||||
| class << self | ||||||||||||||||
| # Validates user credentials against LDAP server | ||||||||||||||||
| # @param login [String] the user's login identifier | ||||||||||||||||
| # @param password [String] the user's password | ||||||||||||||||
| # @return [Boolean] true if credentials are valid | ||||||||||||||||
| def valid_credentials?(login, password) | ||||||||||||||||
| return false if login.blank? || password.blank? | ||||||||||||||||
|
|
||||||||||||||||
| ldap = create_ldap_connection | ||||||||||||||||
| return false unless ldap | ||||||||||||||||
|
|
||||||||||||||||
| # Bind with admin credentials first (if configured) | ||||||||||||||||
| unless bind_as_admin(ldap) | ||||||||||||||||
| Rails.logger.error "LDAP: Failed to bind as admin" | ||||||||||||||||
| return false | ||||||||||||||||
| end | ||||||||||||||||
|
|
||||||||||||||||
| # Search for user | ||||||||||||||||
| user_dn = find_user_dn(ldap, login) | ||||||||||||||||
| return false unless user_dn | ||||||||||||||||
|
|
||||||||||||||||
| # Now attempt to bind as the user to validate their password | ||||||||||||||||
| ldap.auth(user_dn, password) | ||||||||||||||||
| result = ldap.bind | ||||||||||||||||
|
|
||||||||||||||||
| if result | ||||||||||||||||
| Rails.logger.info "LDAP: Successfully authenticated user: #{login}" | ||||||||||||||||
| else | ||||||||||||||||
| Rails.logger.warn "LDAP: Failed to authenticate user: #{login} - #{ldap.get_operation_result.message}" | ||||||||||||||||
|
Comment on lines
+32
to
+34
|
||||||||||||||||
| end | ||||||||||||||||
|
|
||||||||||||||||
| result | ||||||||||||||||
| rescue StandardError => e | ||||||||||||||||
| Rails.logger.error "LDAP: Error validating credentials for #{login}: #{e.message}" | ||||||||||||||||
| false | ||||||||||||||||
| end | ||||||||||||||||
|
Comment on lines
+38
to
+41
|
||||||||||||||||
|
|
||||||||||||||||
| # Retrieves a specific LDAP parameter for a user | ||||||||||||||||
| # @param login [String] the user's login identifier | ||||||||||||||||
| # @param param [String] the LDAP attribute to retrieve (e.g., 'mail') | ||||||||||||||||
| # @return [Array, nil] array of values for the attribute, or nil if not found | ||||||||||||||||
| def get_ldap_param(login, param) | ||||||||||||||||
| return nil if login.blank? || param.blank? | ||||||||||||||||
|
|
||||||||||||||||
| ldap = create_ldap_connection | ||||||||||||||||
| return nil unless ldap | ||||||||||||||||
|
|
||||||||||||||||
| # Bind with admin credentials | ||||||||||||||||
| unless bind_as_admin(ldap) | ||||||||||||||||
| Rails.logger.error "LDAP: Failed to bind as admin" | ||||||||||||||||
| return nil | ||||||||||||||||
| end | ||||||||||||||||
|
|
||||||||||||||||
| # Search for user and retrieve the specified parameter | ||||||||||||||||
| filter = Net::LDAP::Filter.eq(config[:attribute], login) | ||||||||||||||||
| result = ldap.search( | ||||||||||||||||
| base: config[:base], | ||||||||||||||||
| filter: filter, | ||||||||||||||||
| attributes: [param] | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| if result && result.first | ||||||||||||||||
| value = result.first[param] | ||||||||||||||||
| Rails.logger.info "LDAP: Retrieved #{param} for user #{login}" | ||||||||||||||||
| value | ||||||||||||||||
| else | ||||||||||||||||
| Rails.logger.warn "LDAP: Could not find #{param} for user #{login}" | ||||||||||||||||
|
Comment on lines
+69
to
+72
|
||||||||||||||||
| nil | ||||||||||||||||
| end | ||||||||||||||||
| rescue StandardError => e | ||||||||||||||||
| Rails.logger.error "LDAP: Error retrieving #{param} for #{login}: #{e.message}" | ||||||||||||||||
| nil | ||||||||||||||||
| end | ||||||||||||||||
|
|
||||||||||||||||
| # Loads LDAP configuration from ldap.yml | ||||||||||||||||
| # @return [Hash] LDAP configuration hash | ||||||||||||||||
| 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) | ||||||||||||||||
|
Comment on lines
+86
to
+88
|
||||||||||||||||
| # Process ERB template before parsing YAML | |
| erb_content = ERB.new(File.read(config_file)).result | |
| yaml_config = YAML.safe_load(erb_content, aliases: true) | |
| yaml_content = File.read(config_file) | |
| yaml_config = YAML.safe_load(yaml_content, aliases: true) |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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+.
| yaml_config = YAML.safe_load(erb_content, aliases: true) | |
| yaml_config = YAML.safe_load(erb_content, [], [], true) |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method only checks if config[:host] is present but doesn't validate other required configuration values like :base or :attribute. If these are missing, operations will fail with unclear errors later. Consider validating all required configuration parameters upfront and providing clear error messages about which configuration values are missing.
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -371,11 +371,11 @@ | |||||||||||||||||||||
| user = nil | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| # 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) | ||||||||||||||||||||||
|
||||||||||||||||||||||
| 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) |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,31 +1,15 @@ | ||||||
| ## Authorizations | ||||||
| # Uncomment out the merging for each environment that you'd like to include. | ||||||
| # You can also just copy and paste the tree (do not include the "authorizations") to each | ||||||
| # environment if you need something different per environment. | ||||||
| authorizations: &AUTHORIZATIONS | ||||||
| allow_unauthenticated_bind: false | ||||||
| group_base: ou=groups,dc=test,dc=com | ||||||
| ## Requires config.ldap_check_group_membership in devise.rb be true | ||||||
| # Can have multiple values, must match all to be authorized | ||||||
| required_groups: | ||||||
| # If only a group name is given, membership will be checked against "uniqueMember" | ||||||
| - cn=admins,ou=groups,dc=test,dc=com | ||||||
| - cn=users,ou=groups,dc=test,dc=com | ||||||
| # If an array is given, the first element will be the attribute to check against, the second the group name | ||||||
| - ["moreMembers", "cn=users,ou=groups,dc=test,dc=com"] | ||||||
| ## Requires config.ldap_check_attributes in devise.rb to be true | ||||||
| ## Can have multiple attributes and values, must match all to be authorized | ||||||
| require_attribute: | ||||||
| objectClass: inetOrgPerson | ||||||
| authorizationRole: postsAdmin | ||||||
| ## Requires config.ldap_check_attributes_presence in devise.rb to be true | ||||||
| ## Can have multiple attributes set to true or false to check presence, all must match all to be authorized | ||||||
| require_attribute_presence: | ||||||
| mail: true | ||||||
| telephoneNumber: true | ||||||
| serviceAccount: false | ||||||
|
|
||||||
| ## Environment | ||||||
| # LDAP Configuration | ||||||
| # This file configures LDAP authentication settings for each environment. | ||||||
| # All sensitive values should be stored in Rails encrypted credentials. | ||||||
| # | ||||||
| # Required credentials for each environment: | ||||||
| # - ldap_host: LDAP server hostname | ||||||
| # - ldap_port: LDAP server port (typically 389 for non-SSL, 636 for SSL) | ||||||
| # - ldap_attribute: Search attribute (e.g., 'cn', 'uid', 'sAMAccountName') | ||||||
| # - 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) | ||||||
|
||||||
| # - 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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.