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

ISPN-13470 Ldap UserPasswordCredentialLoaderBuilder shouldn't be added by default #9986

Closed
wants to merge 1 commit into from

Conversation

diegolovison
Copy link
Contributor

@@ -11,7 +11,7 @@
* @since 10.0
*/
public class LdapUserPasswordMapperConfiguration extends ConfigurationElement<LdapUserPasswordMapperConfiguration> {
static final AttributeDefinition<String> FROM = AttributeDefinition.builder(Attribute.FROM, "userPassword", String.class).immutable().build();
static final AttributeDefinition<String> FROM = AttributeDefinition.builder(Attribute.FROM, null, String.class).immutable().build();
Copy link
Member

Choose a reason for hiding this comment

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

Leave the default as-is. The only alternative that I know of is Active Directory, which uses sAMAccountName, but you cannot use that for evidence verification (only direct verification is possible).

builder.setUserPasswordAttribute(attributes.attribute(FROM).get());
if (!attributes.attribute(VERIFIABLE).get()) {
builder.disableVerification();
if (attributes.attribute(FROM).get() != null) {
Copy link
Member

Choose a reason for hiding this comment

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

I think the better option here is to add the usercredentialbuilder by default if direct-evidence-verification is false in LdapRealmConfiguration.

@diegolovison
Copy link
Contributor Author

Our credentials were removed from the LDAP server. I will work on this PR once they have been recreated.

@pruivo pruivo added the On Hold label Apr 14, 2022
@tristantarrant
Copy link
Member

tristantarrant commented May 24, 2022

I've made the changes myself in #9983
Once that is merged we can backport it to 13.0.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants