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 new DaoAuthenticationProvider constructor #12874

Closed
wants to merge 1 commit into from

Conversation

psvo
Copy link
Contributor

@psvo psvo commented Mar 16, 2023

Add a new constructor to the DaoAuthenticationProvider, which allows providing a custom PasswordEncoder to prevent instantiation of the default delegating PasswordEncoder in the default constructor.

This provides a way to instantiate the DaoAuthenticationProvider on JDKs where the default delegating PasswordEncoder cannot be instantiated due to limited JCE providers for compliance reasons (e.g., FIPS).

Closes gh-12873

@pivotal-cla This is an Obvious Fix

Add a new constructor to the DaoAuthenticationProvider, which allows
providing a custom PasswordEncoder to prevent instantiation of the
default delegating PasswordEncoder in the default constructor.

This provides a way to instantiate the DaoAuthenticationProvider on JDKs
where the default delegating PasswordEncoder cannot be instantiated due
to limited JCE providers for compliance reasons (e.g., FIPS).

Closes spring-projectsgh-12873
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 16, 2023
@marcusdacoregio marcusdacoregio self-assigned this Mar 31, 2023
@marcusdacoregio marcusdacoregio added status: duplicate A duplicate of another issue in: core An issue in spring-security-core type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 31, 2023
Copy link
Contributor

@marcusdacoregio marcusdacoregio left a comment

Choose a reason for hiding this comment

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

Hi @psvo, thank you for the PR.

This problem seems to be introduced in 5.8.x, therefore I think that the target branch should be 5.8.x. Additionally, the 5.6.x branch is not supported anymore.

this(PasswordEncoderFactories.createDelegatingPasswordEncoder());
}

public DaoAuthenticationProvider(PasswordEncoder passwordEncoder) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test where it makes sure that the provided PasswordEncoder is used?

setPasswordEncoder(PasswordEncoderFactories.createDelegatingPasswordEncoder());
this(PasswordEncoderFactories.createDelegatingPasswordEncoder());
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it would be great to add a little bit of javadoc here, with a little description and the @since 5.8.3 tag

@marcusdacoregio
Copy link
Contributor

@psvo I was discussing this with a team member and we don't think adding a new constructor to a patch release will improve things. Since that change broke existing applications, we should aim to keep the behavior as before (do not throw an exception for the new encoders). A new constructor could be added for 6.1, the next minor release, and we should provide a bug fix for 5.8.x and 6.0.x.

That said, please hold the changes until we are 100% sure what to do here.

@psvo
Copy link
Contributor Author

psvo commented Apr 4, 2023

That said, please hold the changes until we are 100% sure what to do here.

Ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core An issue in spring-security-core type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants