Daily Refactor #19: Cleanup in AuthSourceLdap#authenticate

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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28

# app/models/auth_source_ldap.rb
class AuthSourceLdap  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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37

# 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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37

# 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

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34

# 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.

Reference commits