Skip to content

Bugfix/filter parser fails to parse blackets#157

Merged
jch merged 2 commits into
ruby-ldap:masterfrom
satoryu:bugfix/filter_parser_fails_to_parse_blackets
Nov 13, 2014
Merged

Bugfix/filter parser fails to parse blackets#157
jch merged 2 commits into
ruby-ldap:masterfrom
satoryu:bugfix/filter_parser_fails_to_parse_blackets

Conversation

@satoryu

@satoryu satoryu commented Nov 1, 2014

Copy link
Copy Markdown
Collaborator

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.

Tatsuya Sato added 2 commits April 18, 2014 09:55
@jch jch self-assigned this Nov 1, 2014
@jch

jch commented Nov 1, 2014

Copy link
Copy Markdown
Member

@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.

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

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

satoryu commented Nov 13, 2014

Copy link
Copy Markdown
Collaborator Author

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

jch commented Nov 13, 2014

Copy link
Copy Markdown
Member

@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

satoryu commented Nov 13, 2014

Copy link
Copy Markdown
Collaborator Author

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

@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