Daily Refactor #17: Extract method in AuthSourceLdap

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

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

# 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 = [: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

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

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

  1. It shortened AuthSourceLdap#authenticate, which is difficult enough to understand
  2. 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

Reference commit