Add support for LDAPS testing #48315
Conversation
Add to EmbeddedLdapProperties: -boolean ldaps -String sslBundleName Create setLdapsListener method to create and set the LDAPS listener for the server. Add test for new embedded LDAP setup. Issue#48060 Signed-off-by: CatiaCorreia <catia.correia97@gmail.com>
spring-projects-issues
added
the
status: waiting-for-triage
snicoll
left a comment
snicoll
left a comment
There was a problem hiding this comment.
Can you please review the proposal. I've added a couple of comments for a start.
| } | ||
| } | ||
|
|
||
| @ConditionalOnBean(SslBundles.class) |
There was a problem hiding this comment.
I don't know what this is but you can't add a condition on a private method. What's that supposed to do?
| } | ||
|
|
||
| @Test | ||
| void testLdapsVersion() { |
There was a problem hiding this comment.
That doesn't test the SSL bundles, nor the binding. It doesn't work atm anyway (see previous comment).
snicoll
added
the
status: waiting-for-feedback
wilkinsona
left a comment
wilkinsona
left a comment
There was a problem hiding this comment.
Thanks very much for the PR, @CatiaCorreia. I've left a few comments for your consideration.
| /** | ||
| * Listener type. | ||
| */ | ||
| private boolean ldaps; | ||
|
|
||
| /** | ||
| * Embedded LDAPS client SSL bundle name. | ||
| */ | ||
| @Nullable private String sslBundleName; |
There was a problem hiding this comment.
For consistency with other SSL configuration in Spring Boot, the properties should be grouped together in an SSL inner-class. That class should have an enabled property, a bundle property as well as properties for configuring things without using an SSL bundle. org.springframework.boot.amqp.autoconfigure.RabbitProperties.Ssl is a client-side example of that sort of arrangement.
| } | ||
|
|
||
| @Bean | ||
| InMemoryDirectoryServer directoryServer(ApplicationContext applicationContext) throws LDAPException { |
There was a problem hiding this comment.
SslBundles should be injected here using ObjectProvider<SslBundles> sslBundles. You can then use getIfAvailable() to get the bean if it exists or null if it does not.
| } | ||
| } | ||
|
|
||
| @ConditionalOnBean(SslBundles.class) |
There was a problem hiding this comment.
This won't have any effect here as conditions only work on classes and @Bean methods. It can be removed if you inject SslBundles as suggested above.
|
Thank you for all the feedback. I will start working on the changes right away. |
spring-projects-issues
added
status: feedback-provided
wilkinsona
added
status: waiting-for-feedback
|
I'm writing this message to give a status report. I have implemented the changes discussed above and have tried testing them. I believe I have successfully implemented the creation of the server with SSL without SslBundles. However when it comes to the SslBundles I haven't been able to get a successful test even thought I can't find a problem. There seems to be a problem when the client tries to get a connection to the server (LDAPException code 91). I have committed the changes to a separate branch as I was unsure if I should commit them to this one directly. The branch is found here: https://github.com/CatiaCorreia/spring-boot/tree/gh%2348060-Troubleshooting |
spring-projects-issues
added
status: feedback-provided
4 participants
Add to EmbeddedLdapProperties:
-boolean ldaps
-String sslBundleName
Create setLdapsListener method to create and set the LDAPS listener for the server. Add test for new embedded LDAP setup.
Issue#48060
Signed-off-by: CatiaCorreia catia.correia97@gmail.com