Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Net::LDAP#encryption accepts string #239

Merged
merged 5 commits into from
Jan 4, 2016

Conversation

satoryu
Copy link
Collaborator

@satoryu satoryu commented Nov 25, 2015

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

@satoryu satoryu force-pushed the ldap_encryption_accepts_string branch from 1a1f700 to 6a2f702 Compare November 25, 2015 10:20
@jch
Copy link
Member

jch commented Nov 30, 2015

@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
Copy link

darix commented Nov 30, 2015

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
Copy link
Member

jch commented Nov 30, 2015

@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
Copy link

darix commented Nov 30, 2015

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
Copy link
Collaborator Author

satoryu commented Dec 1, 2015

@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
Copy link
Member

jch commented Dec 11, 2015

@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
Copy link
Collaborator Author

satoryu commented Dec 11, 2015

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

jch added a commit that referenced this pull request Jan 4, 2016
@jch jch merged commit 7907453 into ruby-ldap:master Jan 4, 2016
@jch
Copy link
Member

jch commented Jan 4, 2016

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

@jch jch mentioned this pull request Jan 11, 2016
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Jan 12, 2016
=== Net::LDAP 0.13.0

* Set a connect_timeout for the creation of a socket
  {#243}[ruby-ldap/ruby-net-ldap#243]
* Update bundler before installing gems with bundler
  {#245}[ruby-ldap/ruby-net-ldap#245]
* Net::LDAP#encryption accepts string
  {#239}[ruby-ldap/ruby-net-ldap#239]
* Adds correct UTF-8 encoding to Net::BER::BerIdentifiedString
  {#242}[ruby-ldap/ruby-net-ldap#242]
* Remove 2.3.0-preview since ruby-head already is included
  {#241}[ruby-ldap/ruby-net-ldap#241]
* Drop support for ruby 1.9.3
  {#240}[ruby-ldap/ruby-net-ldap#240]
* Fixed capitalization of StartTLSError
  {#234}[ruby-ldap/ruby-net-ldap#234]
@satoryu satoryu deleted the ldap_encryption_accepts_string branch August 24, 2016 22:01
This was referenced Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants