Add unbounid support in xml#7149
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
|
Thanks for putting this together. I assume you are still working on it since it is marked WIP and the build is failing? |
|
@rwinch this PR is ready to be reviewed |
rwinch
left a comment
rwinch
left a comment
There was a problem hiding this comment.
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
|
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? |
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
|
@rwinch PR updated. |
rwinch
left a comment
rwinch
left a comment
There was a problem hiding this comment.
Thanks @eddumelendez! I have some additional feedback inline
| } | ||
|
|
||
| private RootBeanDefinition getRootBeanDefinition(String mode) { | ||
| if (StringUtils.hasLength(mode)) { |
There was a problem hiding this comment.
It seems like there is a lot of duplication in the code. This block looks very similar to resolveEmbeddedLdapServer
| } | ||
| } | ||
| else { | ||
| for (Map.Entry<String, EmbeddedLdapServer> entry : this.embeddedServers.entrySet()) { |
There was a problem hiding this comment.
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.
|
|
||
| private RootBeanDefinition getRootBeanDefinition(String mode) { | ||
| if (StringUtils.hasLength(mode)) { | ||
| if (isEmbeddedServerEnabled(mode)) { |
There was a problem hiding this comment.
Internally isEmbeddedServerEnabled resolves the embedded server and then it isn't used which is a bit strange.
|
|
||
| parserContext.getRegistry().registerBeanDefinition(BeanIds.EMBEDDED_APACHE_DS, | ||
| apacheContainer); | ||
| EmbeddedLdapServer embeddedLdapServer = resolveEmbeddedLdapServer(mode); |
There was a problem hiding this comment.
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.
| } | ||
| } | ||
| } | ||
| return null; |
There was a problem hiding this comment.
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
added
status: waiting-for-feedback
|
@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
added
status: feedback-provided
rwinch
added
in: config
|
Thanks for the fast turnaround @eddumelendez! This is now merged into master |
3 participants
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