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.

Sign up for GitHub

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

Normalize the encryption parameter passed to the LDAP constructor #264

Merged
merged 1 commit into from
Feb 3, 2016
Merged

Normalize the encryption parameter passed to the LDAP constructor #264

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions lib/net/ldap.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,7 +539,7 @@ def initialize(args = {})
@auth = args[:auth] || DefaultAuth
@base = args[:base] || DefaultTreebase
@force_no_page = args[:force_no_page] || DefaultForceNoPage
@encryption = args[:encryption] # may be nil
@encryption = normalize_encryption(args[:encryption]) # may be nil
@connect_timeout = args[:connect_timeout]

if pr = @auth[:password] and pr.respond_to?(:call)
Expand Down Expand Up @@ -609,13 +609,7 @@ def authenticate(username, password)
def encryption(args)
warn "Deprecation warning: please give :encryption option as a Hash to Net::LDAP.new"
return if args.nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You already moved this line to your normalize_encryption method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I debated whether to include that line in there or not. I'm trying to mimic the exact logic that currently exists, which is "if nil is passed to the encryption method, don't set the encryption". If I omit the line, It will set the encryption to nil.

I'm fine with changing it if you feel it is the desired behavior to have the method set the encryption to nil.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great point. 👍 to leaving this. Especially since it's a deprecated method.

return @encryption = args if args.is_a? Hash

case method = args.to_sym
when :simple_tls, :start_tls
args = { :method => method, :tls_options => {} }
end
@encryption = args
@encryption = normalize_encryption(args)
end

# #open takes the same parameters as #new. #open makes a network
Expand Down Expand Up @@ -1323,4 +1317,17 @@ def new_connection
}
raise e
end

# Normalize encryption parameter the constructor accepts, expands a few
# convenience symbols into recognizable hashes
def normalize_encryption(args)
return if args.nil?
return args if args.is_a? Hash

case method = args.to_sym
when :simple_tls, :start_tls
{ :method => method, :tls_options => {} }
end
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!
Could you make normalize_encryption do what #257 does?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, what exactly would you like me to do? The method should take 1 of 4 types of arguments (nil, string, symbol, or hash) and return either nil or a hash.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, sorry, I misunderstood. the purpose of this method is to normalize (I was thinking of validating a given encryption option when I saw your PR).
Anyway, please forget about my comment 🙇


end # class LDAP
20 changes: 20 additions & 0 deletions test/test_ldap.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,4 +91,24 @@ def test_encryption

assert_equal enc[:method], :start_tls
end

def test_normalize_encryption_symbol
enc = @subject.send(:normalize_encryption, :start_tls)
assert_equal enc, {:method => :start_tls, :tls_options => {}}
end

def test_normalize_encryption_nil
enc = @subject.send(:normalize_encryption, nil)
assert_equal enc, nil
end

def test_normalize_encryption_string
enc = @subject.send(:normalize_encryption, 'start_tls')
assert_equal enc, {:method => :start_tls, :tls_options => {}}
end

def test_normalize_encryption_hash
enc = @subject.send(:normalize_encryption, {:method => :start_tls, :tls_options => {:foo => :bar}})
assert_equal enc, {:method => :start_tls, :tls_options => {:foo => :bar}}
end
end