Net::LDAP#encryption accepts string

[satoryu]

As #238 reported, if Net::LDAP.new is given encryption type as String instead of Symbol, it breaks in establishing a connection.

This pull request provides a solution: allowing to give encryption type as String.

CC: @darix

[jch]

@satoryu thanks for starting this. I think your code is correct, but I'd rather we address underlying problem: we should have only one way of specifying encryption settings. I always thought it strange to have a method instead of a setter / constructor argument for setting encryption args. initialize already calls encryption, but I'd prefer it didn't have any special logic and always expected a hash.

This is where I'd like to get to:

class Net::LDAP
  # encryption: Hash
  #   method: - :simple_tls (let's standardize on one key, not multiple aliases)
  #   options: - Hash of options for that method
  def initialize(args)
    @encryption = args[:encryption]
  end
end

I imagine we'd need to:

• deprecate #encryption
• deprecate multiple method names start_tls, simple_tls in favor of one
• update docs
• update tests

What do you think?

[darix]

imho the only weird part about this method is that it doesn't use the assigning signature

def encryption=(args)
...
end

was the first method i was looking for. but other than that it is a nice way to set the setting. and it matches the function <=> option mapping you see with the other params for the initializer.

[jch]

@darix following the setter convention would be less surprising, but the problem is setting @encryption only makes sense before a connection is opened. If a caller sets encryption after open, all the behavior is unexpected/undefined. Should it do nothing? That'd be bad because the caller expects the connection to be encrypted. Should we close and reopen the existing connection? That'd be hard to debug too.

Thanks for that initial bug report to spark this discussion 👍

[darix]

On 2015-11-30 09:56:06 -0800, Jerry Cheung wrote:

@darix following the setter convention would be less surprising, but
the problem is setting @encryption only makes sense before a
connection is opened. If a caller sets encryption after open, all the
behavior is unexpected/undefined. Should it do nothing? That'd be bad
because the caller expects the connection to be encrypted. Should we
close and reopen the existing connection? That'd be hard to debug too.

I do not see how that is much different from calling

ldap_object.encryption(somearg)

once the connection is established.

We would have 2 options in that case.

a) reject the setting with an error Net::LDAP::OpenConnectionError
b) close the current connection and reopen it with the new settings.

the second option might have unwanted consequences like incomplete
operations, especially relevant for write operations. so the first
option might be better.

[satoryu]

@jch I agree with your idea about deprecating encryption and two options start_tls and simple_tls and only supporting Hash option for specifying encryption type. As this change would break some user codes which expects these, the minor version should be bumped up.

@darix I prefer option a) and showing deprecation massage when calling encryption method.

[jch]

@satoryu any interest in PR-ing the ideas listed above? If you're busy, I'll try to carve out some time during the holidays.

[satoryu]

@jch sorry for late response. I'd like to keep doing PRing in this week end.

[jch]

@satoryu happy new years, and thanks for following up!