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

Issue-1840: Improve password encoding support for LDAP #1909

Merged
merged 2 commits into from Dec 1, 2020
Merged

Issue-1840: Improve password encoding support for LDAP #1909

merged 2 commits into from Dec 1, 2020

Conversation

madhukarbharti
Copy link
Contributor

@madhukarbharti madhukarbharti commented Oct 7, 2020 •
edited by steve-todorov

Pull Request Description

This pull request closes #1840

Acceptance Test

  • Building the code with mvn clean install -Dintegration.tests still works.
  • Running mvn spring-boot:run in the strongbox-web-core still starts up the application correctly.
  • Building the code and running the strongbox-distribution from a zip or tar.gz still works.
  • The tests in the strongbox-web-integration-tests still run properly.

Questions

Code Review And Pre-Merge Checklist

  • My code follows the coding convention of this project.
  • I have performed a self-review of my own code.
  • I have commented my code in hard-to-understand areas.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

carlspring
carlspring requested changes Oct 7, 2020
View reviewed changes
Copy link
Member

@carlspring carlspring left a comment

Choose a reason for hiding this comment

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

@madhukarbharti ,

Please, re-format your changes (only) according to our coding convention. Could you please also check the "Code Review And Pre-Merge Checklist", as explained in your pull request?

Thank you! :)

@madhukarbharti madhukarbharti force-pushed the issue-1840 branch 3 times, most recently from 95eaf18 to 33f3c68 Compare October 8, 2020 18:29
@madhukarbharti madhukarbharti force-pushed the issue-1840 branch 2 times, most recently from 5f56f04 to fc5d0d2 Compare October 10, 2020 17:09
sbespalov
sbespalov reviewed Oct 13, 2020
View reviewed changes
Copy link
Member

@sbespalov sbespalov left a comment

Choose a reason for hiding this comment

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

@madhukarbharti thanks for the PR, it looks very good! Just few minor comments, please proceed with it so we could merge.

@steve-todorov steve-todorov changed the title Issue/1840 : Fix for Improve password encoding support for LDAP Issue-1840: Improve password encoding support for LDAP Oct 14, 2020
sbespalov
sbespalov suggested changes Oct 16, 2020
View reviewed changes
Copy link
Member

@sbespalov sbespalov left a comment

Choose a reason for hiding this comment

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

@madhukarbharti thanks for the changes. There is few more comments, could you please proceed with it as well?

...-api/src/main/java/org/carlspring/strongbox/configuration/Base64PasswordEncoderDelegate.java Outdated Show resolved Hide resolved
...-api/src/main/java/org/carlspring/strongbox/configuration/Base64PasswordEncoderDelegate.java Outdated Show resolved Hide resolved
.../src/test/java/org/carlspring/strongbox/configuration/Base64PasswordEncoderDelegateTest.java Outdated Show resolved Hide resolved
...box-security-api/src/main/java/org/carlspring/strongbox/configuration/WebSecurityConfig.java Outdated Show resolved Hide resolved
...va/org/carlspring/strongbox/authentication/api/impl/ldap/LdapAuthenticationProviderTest.java Outdated Show resolved Hide resolved
@sbespalov
Copy link
Member

@madhukarbharti thanks for your hard working on this task! there is some new clarifications of how we could proceed with LDAP password decode, could you please have a look?

@carlspring carlspring mentioned this pull request Oct 19, 2020
Merged
15 tasks
@carlspring
Copy link
Member

carlspring commented Oct 19, 2020 •
edited

Hi,

Please, be advised of the upcoming changes by @mknutsen79 in strongbox/strongbox-parent#113 and #1908 . You will have to rebase.

@steve-todorov steve-todorov added hacktoberfest Pre-selected issues for Hacktoberfest hacktoberfest-accepted labels Oct 26, 2020
@madhukarbharti
Copy link
Contributor Author

@steve-todorov Removed the @disabled annotations in the Nuget tests and updated PR.

@sbespalov sbespalov self-requested a review December 1, 2020 08:48
sbespalov
sbespalov approved these changes Dec 1, 2020
View reviewed changes
Copy link
Member

@sbespalov sbespalov left a comment

Choose a reason for hiding this comment

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

@madhukarbharti @steve-todorov I think we should merge. Thanks!

@sbespalov
Copy link
Member

@madhukarbharti could you please squash before merge.

steve-todorov
steve-todorov approved these changes Dec 1, 2020
View reviewed changes
Copy link
Member

@steve-todorov steve-todorov left a comment

Choose a reason for hiding this comment

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

@madhukarbharti thanks for your hard work on this!

// cc: @carlspring ICLA?

@madhukarbharti
Issue-1840 : Fix for Improve password encoding support for LDAP
9fc29ae 
- Addressed Review Comments
- Apply suggestions from code review
- Removed Disabled Nuget's Tests as remote link is working fine now.
@madhukarbharti
Copy link
Contributor Author

@steve-todorov Squashed and Rebased

@steve-todorov steve-todorov linked an issue Dec 1, 2020 that may be closed by this pull request
Improve test compatibility between UnboundID and OpenLDAP #1964
Closed
7 tasks
@steve-todorov steve-todorov mentioned this pull request Dec 1, 2020
Closed
7 tasks
@steve-todorov
issue-1964: Improve test compatibility between UnboundID and OpenLDAP
7f340d4 
- Added LdapServerTestConfig.
- Switched tests to use LdapServerTestConfig.
- Added some info in the README's.
- Separating base LDIF and additional LDIFs
- Improving strongbox-authentication-providers README.md.
- Improve test compatibility between UnboundID and OpenLDAP
- Renaming configuration field.
@steve-todorov steve-todorov merged commit 5e71385 into strongbox:master Dec 1, 2020
@steve-todorov
Copy link
Member

Congratulations @madhukarbharti! Thanks for your work on this again! :)

@carlspring
Copy link
Member

@madhukarbharti ,

Thank you very much for all your hard work and congratulations on your first pull request! :)

@madhukarbharti
Copy link
Contributor Author

Thank you so much @carlspring @sbespalov and @steve-todorov for all support and suggestions. It was really nice to work with you all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlspring carlspring carlspring requested changes

@steve-todorov steve-todorov steve-todorov approved these changes

@sbespalov sbespalov sbespalov approved these changes

Assignees
No one assigned
Labels
hacktoberfest Pre-selected issues for Hacktoberfest hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve test compatibility between UnboundID and OpenLDAP Improve password encoding support for LDAP
4 participants
@madhukarbharti @sbespalov @carlspring @steve-todorov