Net::LDAP#open does not cache bind result #334
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aligns implementation of open with other methods, so the result becomes accessible via get_operation_result
This reverts commit c8bd985
|
@mtodd this is ready for review 👍 |
…nto ldap-open-connection-bug
mtodd
approved these changes
Nov 18, 2019
There was a problem hiding this comment.
👍 thanks for tracking this down and making the fix.
Also 👍 to bumping the version to 0.16.2 as this isn't modifying any API interfaces; if anything, it will improve existing functionality by properly setting the @result as expected, which would classify this as a bug fix.
I believe this comment is indicating that the result isn't checked and acted on, not necessarily that the result should be dropped altogether. |
Merged
This was referenced Nov 19, 2019
netbsd-srcmastr
pushed a commit
to NetBSD/pkgsrc
that referenced
this pull request
Dec 3, 2019
Update ruby-net-ldap to 0.16.2.
=== Net::LDAP 0.16.2
* Net::LDAP#open does not cache bind result {#334}[ruby-ldap/ruby-net-ldap#334]
* Fix CI build {#333}[ruby-ldap/ruby-net-ldap#333]
* Fix to "undefined method 'result_code'" {#308}[ruby-ldap/ruby-net-ldap#308]
* Fixed Exception: incompatible character encodings: ASCII-8BIT and UTF-8 in filter.rb {#285}[ruby-ldap/ruby-net-ldap#285]
This was referenced Jan 24, 2023
Open
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi friends 👋 this is a follow up to #331, same fix, but including the fixed CI build 🎉
What
I identified that clients cannot safely rely on
Net::LDAP#get_operation_resultwhen usingNet::LDAP#openbecause@resultis not set. As a consequence, clients callingNet::LDAP#get_operation_resultwould get the previous cached@result. Yikes 🙀How
The solution is simple: to cache
@resultaccordingly, like other methods in the class do. You can see the 🐛 reproduced in ab18e5b. The test is 8ba3f95And then going 🍏 in 92be710
Assumptions
It's not clear to me if this was an intentional design choice. Maybe no one was meant to call
Net::LDAP#get_operation_resultafterNet::LDAP#open? If that's the case, I couldn't find any evidence.I see there are some comments explicitly calling out the result is deliberately ignored ¯_(ツ)_/¯