With the help of Nate Sutton I've done two small refactorings to AuthSourceLdap#authenticate and #authenticate_dn.
The Refactoring - One
This refactoring moved the check for an empty DN into the #authenticate_dn method. It makes sense to have all of the business logic and data validations for authenticating in one method.
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 = 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?
if authenticate_dn(dn, password)
logger.debug "Authentication successful for '#{login}'" if logger && logger.debug?
return attrs
else
return nil
end
rescue Net::LDAP::LdapError => text
raise "LdapError: " + text
end
# Check if a DN (user record) authenticates with the password
def authenticate_dn(dn, password)
ldap_con = initialize_ldap_con(dn, password)
return ldap_con.bind
end
end
After
# app/models/auth_source_ldap.rb
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?
logger.debug "DN found for #{login}: #{dn}" if logger && logger.debug?
end
if authenticate_dn(dn, password)
logger.debug "Authentication successful for '#{login}'" if logger && logger.debug?
return attrs
else
return nil
end
rescue Net::LDAP::LdapError => text
raise "LdapError: " + text
end
# Check if a DN (user record) authenticates with the password
def authenticate_dn(dn, password)
return nil if dn.empty?
ldap_con = initialize_ldap_con(dn, password)
return ldap_con.bind
end
end
The Refactoring - Two
This refactoring cleans up the internals of authenticate_dn by using Ruby's ability to implicitly return the last value in a method.
Before
# app/models/auth_source_ldap.rb
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?
logger.debug "DN found for #{login}: #{dn}" if logger && logger.debug?
end
if authenticate_dn(dn, password)
logger.debug "Authentication successful for '#{login}'" if logger && logger.debug?
return attrs
else
return nil
end
rescue Net::LDAP::LdapError => text
raise "LdapError: " + text
end
# Check if a DN (user record) authenticates with the password
def authenticate_dn(dn, password)
return nil if dn.empty?
ldap_con = initialize_ldap_con(dn, password)
return ldap_con.bind
end
end
After
# app/models/auth_source_ldap.rb
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?
logger.debug "DN found for #{login}: #{dn}" if logger && logger.debug?
end
if authenticate_dn(dn, password)
logger.debug "Authentication successful for '#{login}'" if logger && logger.debug?
return attrs
end
rescue Net::LDAP::LdapError => text
raise "LdapError: " + text
end
# Check if a DN (user record) authenticates with the password
def authenticate_dn(dn, password)
if dn.present? && password.present?
initialize_ldap_con(dn, password).bind
end
end
end
Review
With these two changes, what #authenticate is doing has become a lot clearer for each the different return paths (only 3 now). I think there is only one more refactoring I want to make to it before I move on; the ternary operation in the #search call. Once that's refactored, I'll be able to easily port over a new feature from one of my Redmine clients.
