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

Bugfix/filter parser fails to parse blackets #157

Merged

Conversation

satoryu
Copy link
Collaborator

@satoryu satoryu commented Nov 1, 2014

I forgot I sent a pull-request #72 to the upstream repository. the upstream has been evolving since I sent the pull request.
So I've merged the head of the branch master and rewrite a test code.

@jch Thank you for informing me.

@jch jch self-assigned this Nov 1, 2014
@jch
Copy link
Member

jch commented Nov 1, 2014

@satoryu 👍

I'm not familiar with the bracket syntax in your code. What does it mean, and how is it used? Could you point me to where it's declared in the spec? I found http://tools.ietf.org/html/rfc4511#section-4.5.1.7, but did not dig further into X.511 Clause 7.8.

@@ -10,6 +10,10 @@ def test_multibyte_characters
assert_kind_of Net::LDAP::Filter, Net::LDAP::Filter::FilterParser.parse("(cn=名前)")
end

def test_brackets
assert_kind_of Net::LDAP::Filter, Net::LDAP::Filter::FilterParser.parse("(cn=[{something}])")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jch

What does it mean, and how is it used?

Do you point this cn=[{something}] ? if so, this filter does not make sense as a filter.
As I read the article, I came to think FilterParser should parse a given string including []{} as a filter.

Copy link
Member

Choose a reason for hiding this comment

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

I see, this just allows other valid characters to be constructed into a filter. @satoryu could you write an integration test against OpenLDAP to ensure this is valid? There are instructions here. Also, have you tested this to be valid against ActiveDirectory?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I got it 🍣 I'll try to write an integration test.
now I'm using this patch against ActiveDirectory. it works well in my environment :)

@Scorcher
Copy link

I am not familiar with ruby, but I have a problem to set filter in Redmine, while this bug is present in this library. Developer sent me to this library... http://www.redmine.org/issues/11289
"Brackets" are valid symbols.
Example:
This is native ldap client and this command looking for Active users:

rugionpro:releases n$ ldapsearch -LLL -h localhost -p 2389 -z 1 -x -b "dc=rugion,dc=ru" -s sub '(&(objectClass=posixAccount)(sambaAcctFlags=[U          ]))' "dn" 
dn: *****************,dc=rugion,dc=ru

I fixed it temporarily like so:

root@samui:# diff -up ruby/1.9.1/gems/net-ldap-0.3.1/lib/net/ldap/filter.rb.bak ruby/1.9.1/gems/net-ldap-0.3.1/lib/net/ldap/filter.rb
--- ruby/1.9.1/gems/net-ldap-0.3.1/lib/net/ldap/filter.rb.bak 2014-11-11 16:25:00.619443359 +0500
+++ ruby/1.9.1/gems/net-ldap-0.3.1/lib/net/ldap/filter.rb     2014-11-13 13:34:51.391417719 +0500
@@ -733,7 +733,7 @@ class Net::LDAP::Filter
         scanner.scan(/\s*/)
         if op = scanner.scan(/<=|>=|!=|:=|=/)
           scanner.scan(/\s*/)
-          if value = scanner.scan(/(?:[-\w*.+@=,#\$%&!'\s]|\\[a-fA-F\d]{2})+/)
+          if value = scanner.scan(/(?:[-\w*.+@=,\[\]#\$%&!'\s]|\\[a-fA-F\d]{2})+/)
             # 20100313 AZ: Assumes that "(uid=george*)" is the same as
             # "(uid=george* )". The standard doesn't specify, but I can find
             # no examples that suggest otherwise.

@satoryu
Copy link
Collaborator Author

satoryu commented Nov 13, 2014

Sorry, guys. just I've confirmed the updates works well in Integration Test.

time INTEGRATION=openldap INTEGRATION_HOST=$ip bundle exec rake                                                   (git|bugfix/filter_parser_fails_to_parse_blackets)
/usr/local/var/rbenv/versions/2.1.0/bin/ruby -I"lib:test" -I"/usr/local/var/rbenv/versions/2.1.0/lib/ruby/gems/2.1.0/gems/rake-10.3.1/lib" "/usr/local/var/rbenv/versions/2.1.0/lib/ruby/gems/2.1.0/gems/rake-10.3.1/lib/rake/rake_test_loader.rb" "test/ber/core_ext/test_array.rb" "test/ber/core_ext/test_string.rb" "test/ber/test_ber.rb" "test/integration/test_add.rb" "test/integration/test_ber.rb" "test/integration/test_bind.rb" "test/integration/test_delete.rb" "test/integration/test_open.rb" "test/integration/test_return_codes.rb" "test/integration/test_search.rb" "test/test_dn.rb" "test/test_entry.rb" "test/test_filter.rb" "test/test_filter_parser.rb" "test/test_helper.rb" "test/test_ldap.rb" "test/test_ldap_connection.rb" "test/test_ldif.rb" "test/test_password.rb" "test/test_rename.rb" "test/test_search.rb" "test/test_snmp.rb" "test/test_ssl_ber.rb"
Run options:

# Running tests:

Finished tests in 2.000158s, 81.4936 tests/s, 365.4711 assertions/s.
163 tests, 731 assertions, 0 failures, 0 errors, 0 skips

ruby -v: ruby 2.1.0p0 (2013-12-25 revision 44422) [x86_64-darwin14.0]
INTEGRATION=openldap INTEGRATION_HOST=$ip bundle exec rake  1.03s user 0.31s system 39% cpu 3.407 total

@jch
Copy link
Member

jch commented Nov 13, 2014

@Scorcher thanks for the reproduction. @satoryu 🍻 This will be merged in the next release 0.10.0.

jch added a commit that referenced this pull request Nov 13, 2014
…rse_blackets

Bugfix/filter parser fails to parse blackets
@jch jch merged commit 18fcec2 into ruby-ldap:master Nov 13, 2014
@satoryu
Copy link
Collaborator Author

satoryu commented Nov 13, 2014

@jch @Scorcher thanks a lot!

@satoryu satoryu deleted the bugfix/filter_parser_fails_to_parse_blackets branch November 14, 2014 00:55
@Scorcher
Copy link

@jch @satoryu Thank you!

astratto pushed a commit to astratto/ruby-net-ldap that referenced this pull request Dec 18, 2015
…ils_to_parse_blackets

Bugfix/filter parser fails to parse blackets
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