Since I've been doing a lot of work on a Redmine plugin to add more LDAP features, I decided to use today to refactor some of Redmine's LDAP code.
The Refactoring
I just performed a simple extract method refactoring on the authenticate method in AuthSourceLdap to make it easier to understand what it does with the search results.
Before
# app/models/auth_source_ldap.rb
class AuthSourceLdap < AuthSource
# ...
def authenticate(login, password)
return nil if login.blank? || password.blank?
attrs = []
# get user's DN
ldap_con = initialize_ldap_con(self.account, self.account_password)
login_filter = Net::LDAP::Filter.eq( self.attr_login, login )
object_filter = Net::LDAP::Filter.eq( "objectClass", "*" )
dn = String.new
ldap_con.search( :base => self.base_dn,
:filter => object_filter & login_filter,
# only ask for the DN if on-the-fly registration is disabled
:attributes=> (onthefly_register? ? ['dn', self.attr_firstname, self.attr_lastname, self.attr_mail] : ['dn'])) do |entry|
dn = entry.dn
attrs = [:firstname => AuthSourceLdap.get_attr(entry, self.attr_firstname),
:lastname => AuthSourceLdap.get_attr(entry, self.attr_lastname),
:mail => AuthSourceLdap.get_attr(entry, self.attr_mail),
:auth_source_id => self.id ] if onthefly_register?
end
return nil if dn.empty?
logger.debug "DN found for #{login}: #{dn}" if logger && logger.debug?
# authenticate user
ldap_con = initialize_ldap_con(dn, password)
return nil unless ldap_con.bind
# return user's attributes
logger.debug "Authentication successful for '#{login}'" if logger && logger.debug?
attrs
rescue Net::LDAP::LdapError => text
raise "LdapError: " + text
end
# ...
end
After
# app/models/auth_source_ldap.rb
class AuthSourceLdap < AuthSource
# ...
def authenticate(login, password)
return nil if login.blank? || password.blank?
attrs = []
# get user's DN
ldap_con = initialize_ldap_con(self.account, self.account_password)
login_filter = Net::LDAP::Filter.eq( self.attr_login, login )
object_filter = Net::LDAP::Filter.eq( "objectClass", "*" )
dn = String.new
ldap_con.search( :base => self.base_dn,
:filter => object_filter & login_filter,
# only ask for the DN if on-the-fly registration is disabled
:attributes=> (onthefly_register? ? ['dn', self.attr_firstname, self.attr_lastname, self.attr_mail] : ['dn'])) do |entry|
dn = entry.dn
attrs = get_user_attributes_from_ldap_entry(entry) if onthefly_register?
end
return nil if dn.empty?
logger.debug "DN found for #{login}: #{dn}" if logger && logger.debug?
# authenticate user
ldap_con = initialize_ldap_con(dn, password)
return nil unless ldap_con.bind
# return user's attributes
logger.debug "Authentication successful for '#{login}'" if logger && logger.debug?
attrs
rescue Net::LDAP::LdapError => text
raise "LdapError: " + text
end
def get_user_attributes_from_ldap_entry(entry)
[
:firstname => AuthSourceLdap.get_attr(entry, self.attr_firstname),
:lastname => AuthSourceLdap.get_attr(entry, self.attr_lastname),
:mail => AuthSourceLdap.get_attr(entry, self.attr_mail),
:auth_source_id => self.id
]
end
# ...
end
Review
This refactoring served two purposes:
- It shortened
AuthSourceLdap#authenticate, which is difficult enough to understand - The utility method made it easy to get the user attributes from an LDAP entry; which I can use in two parts of my redmine_extra_ldap plugin
