Daily Refactor #18: Extract method in AuthSourceLdap

The Refactoring

AuthSourceLdap#authenticate still can use some more refactoring. I performed another extract method, this time to create a method to authenticate an DN (Distinguished Name).

Before

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22

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

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

# 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

Review

Although it ended up adding more code than it took away, this refactoring made it more clear how a user (DN) is authenticated. Before, the method used guard clauses to control the logic which made it difficult to understand when they would be triggered. Now it’s easier to see the two different code paths because they are controlled by the if authenticate_dn statement.

Also, like yesterday’s refactoring, this adds a new private method that can be used by other methods in the class instead of duplicating the code.

Reference commit