-
Notifications
You must be signed in to change notification settings - Fork 282
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
Allow custom return attributes #2093
Allow custom return attributes #2093
Conversation
Signed-off-by: Martin Kemp <martin_leon.kemp@mercedes-benz.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution, allows happy to see a straight forward solution to a problem, good work!
To get this merged there are two outstanding items,
Test(s) for the new functionality
This functionality is not tested directly with unit test, I can add one if needed.
Could you please add this functionality?
CI failures
> Task :testClasses
Error: eckstyle] [ERROR] /home/runner/work/security/security/src/main/java/com/amazon/dlic/auth/ldap2/LDAPUserSearcher.java:20:8: Unused import - java.util.Arrays. [UnusedImports]
Error: eckstyle] [ERROR] /home/runner/work/security/security/src/main/java/com/amazon/dlic/auth/ldap2/LDAPUserSearcher.java:29:8: Unused import - org.ldaptive.ReturnAttributes. [UnusedImports]
> Task :checkstyleMain
> Task :checkstyleMain FAILED
See https://github.com/opensearch-project/security/actions/runs/3066577438/jobs/4956644926
Once these are resolved, it looks good to me.
Sure, will do next week. Thanks for reviewing. |
Signed-off-by: Martin Kemp <martin_leon.kemp@mercedes-benz.com>
Codecov Report
@@ Coverage Diff @@
## main #2093 +/- ##
============================================
+ Coverage 60.99% 61.02% +0.03%
- Complexity 3226 3229 +3
============================================
Files 256 256
Lines 18075 18077 +2
Branches 3225 3224 -1
============================================
+ Hits 11024 11031 +7
+ Misses 5472 5470 -2
+ Partials 1579 1576 -3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution @Martin-Kemp and for adding tests for this functionality!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
* Allow custom return attributes Signed-off-by: Martin Kemp <martin_leon.kemp@mercedes-benz.com> (cherry picked from commit b9b7e1f)
* Allow custom return attributes Signed-off-by: Martin Kemp <martin_leon.kemp@mercedes-benz.com>
* Allow custom return attributes Signed-off-by: Martin Kemp <martin_leon.kemp@mercedes-benz.com> Signed-off-by: Stephen Crawford <steecraw@amazon.com>
…project#2110) * Allow custom return attributes Signed-off-by: Martin Kemp <martin_leon.kemp@mercedes-benz.com> (cherry picked from commit b9b7e1f) Co-authored-by: Martin Kemp <martinkempsa@gmail.com>
Signed-off-by: Martin Kemp martin_leon.kemp@mercedes-benz.com
Description
Allows specifying which attributes to request from the ldap server.
It can be a security risk to request more attributes than needed from an ldap server.
All attributes are requested.
Issues Resolved
#2032
Is this a backport? If so, please add backport PR # and/or commits #
No
Testing
Manual testing with local ldap server. I made sure default behavior remains the same.
Updated unit tests.
This functionality is not tested directly with unit test, I can add one if needed.
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.