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.

Sign up for GitHub

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

Jump to bottom

Log deprecation message on legacy ldap pool settings #2099

Merged
merged 7 commits into from
Oct 6, 2022
Merged

Log deprecation message on legacy ldap pool settings #2099

merged 7 commits into from
Oct 6, 2022

Conversation

peternied
Copy link
Member

@peternied peternied commented Sep 22, 2022 •
edited
Loading

Description

Log deprecation message on legacy ldap pool settings

Following up on comment #2091 (review) from a contribution by @Martin-Kemp

Issues

Testing

Manually tested - adding deprecation check involves migrating these tests to OpenSearchTestCase, far outside the scope of the existing security testing infrastructure, created #2098

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

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.

@peternied peternied requested a review from a team September 22, 2022 16:32
@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2022 •
edited
Loading

Codecov Report

Merging #2099 (3e90740) into main (aeb7ab5) will decrease coverage by 0.01%.
The diff coverage is 93.75%.

@@             Coverage Diff              @@
##               main    #2099      +/-   ##
============================================
- Coverage     61.06%   61.04%   -0.02%     
- Complexity     3231     3234       +3     
============================================
  Files           256      257       +1     
  Lines         18077    18110      +33     
  Branches       3224     3229       +5     
============================================
+ Hits          11039    11056      +17     
- Misses         5464     5472       +8     
- Partials       1574     1582       +8
Impacted Files Coverage Δ
...pensearch/security/setting/DeprecatedSettings.java 80.00% <80.00%> (ø)
.../main/java/com/amazon/dlic/auth/ldap/LdapUser.java 85.71% <100.00%> (ø)
...c/auth/ldap/backend/LDAPAuthenticationBackend.java 81.57% <100.00%> (+0.32%) ⬆️
.../dlic/auth/ldap2/LDAPConnectionFactoryFactory.java 57.46% <100.00%> (+0.64%) ⬆️
...ttp/netty/SecuritySSLNettyHttpServerTransport.java 78.94% <0.00%> (-16.06%) ⬇️
...urity/ssl/transport/SecuritySSLNettyTransport.java 62.36% <0.00%> (-4.31%) ⬇️
...earch/security/ssl/util/SSLConnectionTestUtil.java 93.18% <0.00%> (-2.28%) ⬇️
...ecurity/configuration/ConfigurationRepository.java 72.13% <0.00%> (-2.19%) ⬇️
...opensearch/security/ssl/util/SSLRequestHelper.java 63.80% <0.00%> (-0.90%) ⬇️
...ensearch/security/ssl/DefaultSecurityKeyStore.java 67.83% <0.00%> (+0.20%) ⬆️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

cwperks
cwperks previously approved these changes Sep 29, 2022
View reviewed changes
Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

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

Thanks @peternied! Can you link to the addition issue resolved by this PR (#2059) in the PR description? 🎸

DarshitChanpura
DarshitChanpura reviewed Sep 29, 2022
View reviewed changes
Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

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

Ty Peter. I will approve once the CI passes.

src/main/java/org/opensearch/security/setting/DeprecatedSettings.java Outdated
* Modifications Copyright OpenSearch Contributors. See
* GitHub history for details.
*/
package org.opensearch.security.setting;
Copy link
Member

Choose a reason for hiding this comment

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

👍

peternied
peternied commented Sep 30, 2022
View reviewed changes
Copy link
Member Author

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Sorry for the momentarily disappearing changes, everything has been restored, all is well

cwperks
cwperks previously approved these changes Oct 4, 2022
View reviewed changes
@cwperks cwperks added the backport 2.x backport to 2.x branch label Oct 4, 2022
@cwperks
Copy link
Member

cwperks commented Oct 4, 2022

@peternied Please resolve the unused import CI failure. This looks good to me. I will approve again once CI is passing.

@peternied
Fix checkstyle issues
b75a9ba 
Signed-off-by: Peter Nied <[email protected]>
@peternied
Fix license headers
252f25c 
Signed-off-by: Peter Nied <[email protected]>
cwperks
cwperks previously approved these changes Oct 4, 2022
View reviewed changes
@peternied
Copy link
Member Author

@opensearch-project/security Could I get another review?

DarshitChanpura
DarshitChanpura reviewed Oct 5, 2022
View reviewed changes
Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

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

Ty @peternied for this! I have added a couple comments requesting small changes. I will approve once those are addressed.

src/main/java/com/amazon/dlic/auth/ldap2/LDAPConnectionFactoryFactory.java Show resolved Hide resolved
src/main/java/com/amazon/dlic/auth/ldap2/LDAPConnectionFactoryFactory.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/security/setting/DeprecatedSettings.java Show resolved Hide resolved
src/main/java/org/opensearch/security/setting/DeprecatedSettings.java Show resolved Hide resolved
src/test/java/org/opensearch/security/setting/DeprecatedSettingsTest.java Show resolved Hide resolved
@peternied peternied merged commit e5ab646 into opensearch-project:main Oct 6, 2022
@peternied peternied deleted the log-deprecation branch October 6, 2022 20:52
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 6, 2022
@peternied @github-actions
Log deprecation message on legacy ldap pool settings (#2099)
f724d3e 
* Log deprecation message on legacy ldap pool settings

Signed-off-by: Peter Nied <[email protected]>
Signed-off-by: Peter Nied <[email protected]>
Co-authored-by: Martin Kemp <[email protected]>
(cherry picked from commit e5ab646)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Oct 6, 2022
Merged
peternied pushed a commit that referenced this pull request Oct 6, 2022
@opensearch-trigger-bot
Log deprecation message on legacy ldap pool settings (#2099) (#2147)
627c6b4 
* Log deprecation message on legacy ldap pool settings

Signed-off-by: Peter Nied <[email protected]>
Signed-off-by: Peter Nied <[email protected]>
Co-authored-by: Martin Kemp <[email protected]>
(cherry picked from commit e5ab646)
@peternied peternied mentioned this pull request Oct 7, 2022
Open
@peternied peternied mentioned this pull request Nov 3, 2022
Merged
3 tasks
stephen-crawford pushed a commit to stephen-crawford/security that referenced this pull request Nov 10, 2022
@peternied @stephen-crawford
Log deprecation message on legacy ldap pool settings (opensearch-proj…
7808726 
…ect#2099)

* Log deprecation message on legacy ldap pool settings

Signed-off-by: Peter Nied <[email protected]>
Signed-off-by: Peter Nied <[email protected]>
Co-authored-by: Martin Kemp <[email protected]>
Signed-off-by: Stephen Crawford <[email protected]>
wuychn pushed a commit to ochprince/security that referenced this pull request Mar 16, 2023
@opensearch-trigger-bot
Log deprecation message on legacy ldap pool settings (opensearch-proj…
388cdca 
…ect#2099) (opensearch-project#2147)

* Log deprecation message on legacy ldap pool settings

Signed-off-by: Peter Nied <[email protected]>
Signed-off-by: Peter Nied <[email protected]>
Co-authored-by: Martin Kemp <[email protected]>
(cherry picked from commit e5ab646)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DarshitChanpura DarshitChanpura DarshitChanpura approved these changes

@cwperks cwperks cwperks approved these changes

Assignees
No one assigned
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace non-inclusive term "whitelist" in LDAP configuration
4 participants
@peternied @codecov-commenter @cwperks @DarshitChanpura