-
Notifications
You must be signed in to change notification settings - Fork 253
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
Correctly set BerIdentifiedString values to UTF-8 #212
Conversation
The change makes sense. Could you add a test for this? I think it might be more readable to split this into two statements. It threw me off a bit to see encoded_args = begin
args.respond_to?(:encode) ? args.encode('UTF-8') : args
rescue
args
end
super(encoded_args) Thoughts? |
@jch I would think the existing test handle the expectations on strings returned from the LDAP server. The problem is the original line
didn't really do anything. I think the intent of the code was to modify the string internally, so the correct call would have been As to the the change you propose, it doesn't really matter to me. If you prefer what you've proposed I can change it. Either one is valid. |
I'm not sure whether the intention is to modify the string in place. It's weird to me that this inherits from String rather than composing it, but looking at String::new, the doc does say it strings a new String object. I agree with you that the current code doesn't make sense though because that value isn't used or saved anywhere. Where are you running into an error with this? |
I ran into this error when dealing with Chinese names in Active Directory. |
@jch, is there something more you're looking for before you a decision is made on this request? |
@cmdrclueless I'm sorry for the slow response. Work has been busy, and the other maintainer is out of town. I think it's a good idea, I just want to test it more thoroughly myself before merging. |
@jch, no worries. I just wanted to make sure I had not dropped the ball on my side. |
Did an audit through the codebase and the only callsite that initializes ruby-net-ldap/lib/net/ber/ber_parser.rb Line 40 in 4abe573
|
Correctly set BerIdentifiedString values to UTF-8
=== Net::LDAP 0.12.1 * Whitespace formatting cleanup {#236}[ruby-ldap/ruby-net-ldap#236] * Set operation result if LDAP server is not accessible {#232}[ruby-ldap/ruby-net-ldap#232] === Net::LDAP 0.12.0 * DRY up connection handling logic {#224}[ruby-ldap/ruby-net-ldap#224] * Define auth adapters {#226}[ruby-ldap/ruby-net-ldap#226] * add slash to attribute value filter {#225}[ruby-ldap/ruby-net-ldap#225] * Add the ability to provide a list of hosts for a connection {#223}[ruby-ldap/ruby-net-ldap#223] * Specify the port of LDAP server by giving INTEGRATION_PORT {#221}[ruby-ldap/ruby-net-ldap#221] * Correctly set BerIdentifiedString values to UTF-8 {#212}[ruby-ldap/ruby-net-ldap#212] * Raise Net::LDAP::ConnectionRefusedError when new connection is refused. {#213}[ruby-ldap/ruby-net-ldap#213] * obscure auth password upon #inspect, added test, closes #216 {#217}[ruby-ldap/ruby-net-ldap#217] * Fixing incorrect error class name {#207}[ruby-ldap/ruby-net-ldap#207] * Travis update {#205}[ruby-ldap/ruby-net-ldap#205] * Remove obsolete rbx-19mode from Travis {#204}[ruby-ldap/ruby-net-ldap#204] * mv "sudo" from script/install-openldap to .travis.yml {#199}[ruby-ldap/ruby-net-ldap#199] * Remove meaningless shebang {#200}[ruby-ldap/ruby-net-ldap#200] * Fix Travis CI build {#202}[ruby-ldap/ruby-net-ldap#202] * README.rdoc: fix travis link {#195}[ruby-ldap/ruby-net-ldap#195]
Correctly set BerIdentifiedString values to UTF-8
The existing code did not set the actual internal value to UTF-8 when the encoding is correct. This patch attempts to resolve that problem by passing a UTF-8 encoded string to the super class when possible.