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.