cschiewek / devise_ldap_authenticatable

Devise Module for LDAP
MIT License
593 stars 359 forks source link

Constant typo and authenticate fix #193

Closed drobny closed 9 years ago

drobny commented 9 years ago

Recent commit introduced typo in constant. Updated change also assumes find_for_ldap_authentication always returns a resource object but could return nil.

I tried to run the tests but got a few failures for old config variables I think.

Keep up the good work on this gem :)

acurley commented 9 years ago

@drobny Thanks for finding my oversight. However, I cannot see the circumstances when find_for_ldap_authentication would return nil? There is a guard clause

if resource.blank?
  resource = new
  resource[auth_key] = auth_key_value
  resource.password = attributes[:password]
end

that should ensure an object is returned by find_for_ldap_authentication.

Thanks for helping me understand!

acurley commented 9 years ago

@drobny Never mind. I see it now.

irb(main):001:0> User.find_for_ldap_authentication
=> nil

An unlikely circumstance, but nevertheless! Thanks for finding my error.

drobny commented 9 years ago

@acurley I agree it is very unlikely, the only reason I found it is because our company has written an internal gem that overrides the find_for_ldap_authentication method so it returns nil in some cases to reflect the current behaviour in this gem.

Thanks for sorting this all out, if there is anything I can do to help just let me know.