search

ruby-ldap / ruby-net-ldap

Pure Ruby LDAP library
https://rubygems.org/gems/net-ldap
Other
399 stars 253 forks source link

Add in ability for users to specify LDAP controls when conducting searches #411

Closed gwillcox-r7 closed 1 year ago

gwillcox-r7 commented 1 year ago

This PR was made whilst working on fixes for https://github.com/rapid7/metasploit-framework/issues/17324 which lead to https://github.com/rapid7/metasploit-framework/pull/17342. What I discovered was that I wanted to send the LDAP_SERVER_SD_FLAGS_OID flag in a search request so that when retrieving the ntSecurityDescriptor field, one would not also retrieve the SACL, since this cannot be retrieved as a non-admin user.

However I found that implementation of search within the code presently doesn't allow one to specify controls when doing searches, and instead only uses a default control for paging responses.

This PR opens up the option for people to specify their own controls by specifying a controls key inside the args hash that is used by the search function so that users can specify additional controls when conducting search requests.

gwillcox-r7 commented 1 year ago

@HarlemSquirrel For clarification you would be expecting a test using the test-unit testing suite from https://test-unit.github.io/ to be added to this PR to test the new features being added? Just wanted to make sure I understood what was being asked for here; not familiar with unit testing as I haven't written tests before and most of the systems I have worked with have used RSpec for unit testing, so just want to make sure I'm not overlooking anything 👍

HarlemSquirrel commented 1 year ago

@HarlemSquirrel For clarification you would be expecting a test using the test-unit testing suite from https://test-unit.github.io/ to be added to this PR to test the new features being added? Just wanted to make sure I understood what was being asked for here; not familiar with unit testing as I haven't written tests before and most of the systems I have worked with have used RSpec for unit testing, so just want to make sure I'm not overlooking anything +1

@gwillcox-r7 Yeah that's it! Here's a test we have for testing the connection controls so something like that just below it should be good. https://github.com/ruby-ldap/ruby-net-ldap/blob/v0.18.0/test/test_ldap_connection.rb#L139-L144

gwillcox-r7 commented 1 year ago

@HarlemSquirrel For clarification you would be expecting a test using the test-unit testing suite from https://test-unit.github.io/ to be added to this PR to test the new features being added? Just wanted to make sure I understood what was being asked for here; not familiar with unit testing as I haven't written tests before and most of the systems I have worked with have used RSpec for unit testing, so just want to make sure I'm not overlooking anything +1

@gwillcox-r7 Yeah that's it! Here's a test we have for testing the connection controls so something like that just below it should be good. https://github.com/ruby-ldap/ruby-net-ldap/blob/v0.18.0/test/test_ldap_connection.rb#L139-L144

Thanks will take a look into this!

gwillcox-r7 commented 1 year ago

@HarlemSquirrel Do these tests look okay to you or do they need some more adjustments? I wasn't sure what needed to be tested or if I should also be mocking the data back or not.

gwillcox-r7 commented 1 year ago

Thanks @HarlemSquirrel much appreciated!