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

Add unbounid support in xml #7149

Merged
merged 5 commits into from
Aug 14, 2019
Merged

Add unbounid support in xml #7149

merged 5 commits into from
Aug 14, 2019

Conversation

eddumelendez
Copy link
Contributor

Currently, unboundid was added as a support for embbeded LDAP and it
is used on the Java Config. This commit introduces support from XML side.
Also, give the chance to users to move from apacheds to unboundid using
a new attribute mode.

Fixes gh-6011

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 26, 2019
@rwinch
Copy link
Member

rwinch commented Aug 1, 2019

Thanks for putting this together. I assume you are still working on it since it is marked WIP and the build is failing?

@eddumelendez eddumelendez marked this pull request as ready for review August 7, 2019 02:31
@eddumelendez
Copy link
Contributor Author

@rwinch this PR is ready to be reviewed

rwinch
rwinch requested changes Aug 9, 2019
View reviewed changes
Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

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

Thanks @eddumelendez! I have replied inline

...g/src/main/java/org/springframework/security/config/ldap/LdapServerBeanDefinitionParser.java Outdated Show resolved Hide resolved
config/src/main/resources/org/springframework/security/config/spring-security-5.2.xsd Outdated Show resolved Hide resolved
@eddumelendez
Copy link
Contributor Author

Thanks for the feedback @rwinch I will be working on these changes but I think tests will need to be out of the module and should be as a integration test or sample? WDYT?

@eddumelendez
Add unbounid support in xml
9b2af94 
Currently, spring-security provides apacheds integration by default. This
commit introduces a new `mode` in the `ldap-server` tag which allows to choose
beetween `apacheds` and `unboundid`. In order to keep backward compatibility
if `mode` is not set and apacheds jars are in the classpath apacheds is used
as a embedded ldap.

Fixes spring-projectsgh-6011
Currently, unboundid was added as a support for embbeded LDAP and it
is used on the Java Config. This commit introduces support from XML side.
Also, give the chance to users to move from apacheds to unboundid using
a new attribute `mode`.

Fixes spring-projectsgh-6011
@eddumelendez
Copy link
Contributor Author

@rwinch PR updated.

rwinch
rwinch requested changes Aug 12, 2019
View reviewed changes
Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

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

Thanks @eddumelendez! I have some additional feedback inline

...g/src/main/java/org/springframework/security/config/ldap/LdapServerBeanDefinitionParser.java Outdated

return (RootBeanDefinition) contextSource.getBeanDefinition();
}

private RootBeanDefinition getRootBeanDefinition(String mode) {
if (StringUtils.hasLength(mode)) {
Copy link
Member

Choose a reason for hiding this comment

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

It seems like there is a lot of duplication in the code. This block looks very similar to resolveEmbeddedLdapServer

...g/src/main/java/org/springframework/security/config/ldap/LdapServerBeanDefinitionParser.java Outdated
}
}
else {
for (Map.Entry<String, EmbeddedLdapServer> entry : this.embeddedServers.entrySet()) {
Copy link
Member

Choose a reason for hiding this comment

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

This iterates over the entries, but the order is not guaranteed in a HashMap. We want to be sure that ApacheDS is used first and then Unboundid (since this is more passive). Since there are only two entries, perhaps we could avoid using a Collection or Map.

...g/src/main/java/org/springframework/security/config/ldap/LdapServerBeanDefinitionParser.java Outdated

return (RootBeanDefinition) contextSource.getBeanDefinition();
}

private RootBeanDefinition getRootBeanDefinition(String mode) {
if (StringUtils.hasLength(mode)) {
if (isEmbeddedServerEnabled(mode)) {
Copy link
Member

Choose a reason for hiding this comment

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

Internally isEmbeddedServerEnabled resolves the embedded server and then it isn't used which is a bit strange.

...g/src/main/java/org/springframework/security/config/ldap/LdapServerBeanDefinitionParser.java Outdated
parserContext.getReaderContext().error(
"Only one embedded server bean is allowed per application context",
element);
}

parserContext.getRegistry().registerBeanDefinition(BeanIds.EMBEDDED_APACHE_DS,
apacheContainer);
EmbeddedLdapServer embeddedLdapServer = resolveEmbeddedLdapServer(mode);
Copy link
Member

Choose a reason for hiding this comment

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

Calling getRootBeanDefinition above (https://github.com/spring-projects/spring-security/pull/7149/files#diff-ddb1e7b48066bb067a15640ca4b9388cR170) calls isEmbeddedServerEnabled which then calls resolveEmbeddedLdapServer. Then resolveEmbeddedLdapServer is called again. I think we want to simplify this logic.

...g/src/main/java/org/springframework/security/config/ldap/LdapServerBeanDefinitionParser.java
}
}
}
return null;
Copy link
Member

Choose a reason for hiding this comment

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

By the time createEmbeddedServer is called, aren't we certain that an embedded server needs to be created? When is it ever expected it will be null? When would we expect isEmbeddedServerEnabled to be false?

@rwinch rwinch added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 12, 2019
@eddumelendez
Copy link
Contributor Author

@rwinch I have updated the PR taking into account your comments. complexity have been reduced in the implementation and test covering all the scenarios has been added.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 14, 2019
@rwinch rwinch self-assigned this Aug 14, 2019
@rwinch rwinch added in: config An issue in spring-security-config type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided labels Aug 14, 2019
@rwinch rwinch added this to the 5.2.0.RC1 milestone Aug 14, 2019
@rwinch rwinch merged commit abc9028 into spring-projects:master Aug 14, 2019
@rwinch
Copy link
Member

rwinch commented Aug 14, 2019

Thanks for the fast turnaround @eddumelendez! This is now merged into master

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

@rwinch rwinch rwinch approved these changes

Assignees

@rwinch rwinch

Labels
in: config An issue in spring-security-config type: enhancement A general enhancement
Projects
None yet
Milestone
5.2.0.RC1
Development

Successfully merging this pull request may close these issues.

ldap-server XML directive should support UnboundIdContainer
3 participants
@eddumelendez @rwinch @spring-projects-issues