Daily Refactor #20.1: Bonus Extract Method in AuthSourceLdap

Right after publishing the final refactoring for AuthSourceLdap, another really good one jumped out at me and I couldn’t wait until next week to fix it.

The Refactoring

Before

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

# app/models/auth_source_ldap.rb
class AuthSourceLdap  self.base_dn, 
                     :filter => object_filter & login_filter, 
                     :attributes=> search_attributes) 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
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

# app/models/auth_source_ldap.rb
class AuthSourceLdap  text
    raise "LdapError: " + text
  end
 
  # Get the user's dn and any attributes for them, given their login
  def get_user_dn(login)
    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", "*" ) 
    attrs = []
 
    ldap_con.search( :base => self.base_dn, 
                     :filter => object_filter & login_filter, 
                     :attributes=> search_attributes) do |entry|
 
      if onthefly_register?
        attrs = get_user_attributes_from_ldap_entry(entry)
      else
        attrs = [:dn => entry.dn]
      end
 
      logger.debug "DN found for #{login}: #{attrs.first[:dn]}" if logger && logger.debug?
    end
 
    attrs
  end
end

Review

This refactor moves a massive block of code out of AuthSourceLdap#authenticate. Now #authenticate works at a higher level of abstraction and relies on #get_user_dn and #authenticate_dn to handle the actual LDAP queries.

The only reason I noticed this reafactoring was with the single inline comment remaining in #authenticate. Whenever I see an inline comment, I try to stop and see if that code can be isolated and moved to another method.

Reference commit