Devise::LdapAdapter.get_ldap_param return nil when array passed #69

Closed
jsirex opened this Issue Jan 18, 2012 · 4 comments

Comments

Projects
None yet
4 participants
@jsirex

jsirex commented Jan 18, 2012

I have in ldap user with two 'mail' attribute

When I try to search it using get_ldap_param I get nil. When user has only one attribute all ok.

Here is a little test i wrote in irb:

irb(main):006:0> a = [1,2]
=> [1, 2]
irb(main):007:0> a = a.first if false
=> nil
irb(main):008:0> a
=> [1, 2]

So 'a' got a right result but return result is 'nil'.
File: ldap_adapter.rb, line: 103
@stevenyxu

This comment has been minimized.

Show comment
Hide comment
@stevenyxu

stevenyxu Jan 18, 2012

Collaborator

I think you're right. Do you mind preparing a test case and a fix for this? Let me know. If not, I'll get to it when I get the chance. In the meantime, adding this to an initializer should fix it:

module Devise
  module LdapAdapter
    class LdapConnect
      def ldap_param_value(param)
        filter = Net::LDAP::Filter.eq(@attribute.to_s, @login.to_s)
        ldap_entry = nil
        @ldap.search(:filter => filter) {|entry| ldap_entry = entry}

        if ldap_entry 
          if ldap_entry[param]
            DeviseLdapAuthenticatable::Logger.send("Requested param #{param} has value #{ldap_entry.send(param)}")
            value = ldap_entry.send(param)
            value.is_a?(Array) && value.count == 1 ? value.first : value # fixed here
          else
            DeviseLdapAuthenticatable::Logger.send("Requested param #{param} does not exist")
            value = nil
          end
        else
          DeviseLdapAuthenticatable::Logger.send("Requested ldap entry does not exist")
          value = nil
        end
      end
    end
  end
end

If this doesn't address your issue, let me know and I'll poke around further.

Collaborator

stevenyxu commented Jan 18, 2012

I think you're right. Do you mind preparing a test case and a fix for this? Let me know. If not, I'll get to it when I get the chance. In the meantime, adding this to an initializer should fix it:

module Devise
  module LdapAdapter
    class LdapConnect
      def ldap_param_value(param)
        filter = Net::LDAP::Filter.eq(@attribute.to_s, @login.to_s)
        ldap_entry = nil
        @ldap.search(:filter => filter) {|entry| ldap_entry = entry}

        if ldap_entry 
          if ldap_entry[param]
            DeviseLdapAuthenticatable::Logger.send("Requested param #{param} has value #{ldap_entry.send(param)}")
            value = ldap_entry.send(param)
            value.is_a?(Array) && value.count == 1 ? value.first : value # fixed here
          else
            DeviseLdapAuthenticatable::Logger.send("Requested param #{param} does not exist")
            value = nil
          end
        else
          DeviseLdapAuthenticatable::Logger.send("Requested ldap entry does not exist")
          value = nil
        end
      end
    end
  end
end

If this doesn't address your issue, let me know and I'll poke around further.

@jsirex

This comment has been minimized.

Show comment
Hide comment
@jsirex

jsirex Jan 19, 2012

Thank you for reply.
I'm not going to prepare any tests because I'm very beginner on rails (also beginner in programming).
I confirm that this change works.

I also propose to change the behavior of ldap_param_value to always return array. It related to ldap nature that you can have several same params with different values and NET::LDAP::Entry always return array. This helps developers to keep in mind that they suddenly can get array of values.
In real application we got an double check of "value.is_a?(Array)". Each time you use Devise::LdapAdapter.get_ldap_param
you must check weather is a return value array:

# first check is_a?(Array) and transform array->string in get_ldap_param
mail =Devise::LdapAdapter.get_ldap_param(self.username, 'mail')
# now i don't know is mail array or string and i have check it again
mail.is_a?(Array) ? mail.fisrt : mail

If the get_ldap_param will always return array I can safely write:

mail = Devise::LdapAdapter.get_ldap_param(self.username, 'mail').first # zero checks

I looked at this topic: http://stackoverflow.com/a/5684335/1111202

jsirex commented Jan 19, 2012

Thank you for reply.
I'm not going to prepare any tests because I'm very beginner on rails (also beginner in programming).
I confirm that this change works.

I also propose to change the behavior of ldap_param_value to always return array. It related to ldap nature that you can have several same params with different values and NET::LDAP::Entry always return array. This helps developers to keep in mind that they suddenly can get array of values.
In real application we got an double check of "value.is_a?(Array)". Each time you use Devise::LdapAdapter.get_ldap_param
you must check weather is a return value array:

# first check is_a?(Array) and transform array->string in get_ldap_param
mail =Devise::LdapAdapter.get_ldap_param(self.username, 'mail')
# now i don't know is mail array or string and i have check it again
mail.is_a?(Array) ? mail.fisrt : mail

If the get_ldap_param will always return array I can safely write:

mail = Devise::LdapAdapter.get_ldap_param(self.username, 'mail').first # zero checks

I looked at this topic: http://stackoverflow.com/a/5684335/1111202

@rohitn

This comment has been minimized.

Show comment
Hide comment
@rohitn

rohitn Jan 26, 2012

+1 on always returning an Array

jsirex instead of a test you can always coerce to an array: Array(mail).first

rohitn commented Jan 26, 2012

+1 on always returning an Array

jsirex instead of a test you can always coerce to an array: Array(mail).first

@cschiewek

This comment has been minimized.

Show comment
Hide comment
@cschiewek

cschiewek Jun 20, 2012

Owner

I think I agree. If the LDAP entry exists, we should always return an array for consistencies sake.

Owner

cschiewek commented Jun 20, 2012

I think I agree. If the LDAP entry exists, we should always return an array for consistencies sake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment