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

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

Merged

Conversation

gwillcox-r7
Copy link
Contributor

This PR was made whilst working on fixes for rapid7/metasploit-framework#17324 which lead to rapid7/metasploit-framework#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.

Copy link
Member

@HarlemSquirrel HarlemSquirrel left a comment

Choose a reason for hiding this comment

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

Hey thanks for the work here! This sounds like a great idea! Could you add a test for this to verify functionality?

Thanks!

@gwillcox-r7
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

Copy link
Member

@HarlemSquirrel HarlemSquirrel left a comment

Choose a reason for hiding this comment

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

Looks like just a few Rubocop complaints here now. I can fix those after the merge. Thank you for the contribution!

@HarlemSquirrel HarlemSquirrel merged commit 03ab921 into ruby-ldap:master Jun 6, 2023
0 of 8 checks passed
@gwillcox-r7
Copy link
Contributor Author

Thanks @HarlemSquirrel much appreciated!

@gwillcox-r7 gwillcox-r7 deleted the add-in-ldap-control-flexibility branch June 6, 2023 15:09
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.

None yet

2 participants