Allow port=0 for ApacheDSContainer #8416

evgeniycheban · 2020-04-18T23:04:49Z

rwinch

Thanks for the PR. I think there might be some confusion as to what we are looking for. Please see my comment inline.

rwinch · 2020-04-20T19:26:34Z

ldap/src/main/java/org/springframework/security/ldap/server/ApacheDSContainer.java

@@ -143,6 +148,9 @@ public void afterPropertiesSet() throws Exception {
 		server.setDirectoryService(service);
 		// AbstractLdapIntegrationTests assume IPv4, so we specify the same here

+		if (this.port == 0) {
+			this.port = getRandomPort();
+		}


Sorry for the confusion. We are already finding a random port in this way within our configuration support.

The problem is sometimes the port gets resolved to an open port and assigned, the port is then taken up by another process or thread, and then this code tries to use the now taken port.

To fix this we want to be able to pass in 0 to TcpTransport to avoid a race condition.

Thanks for the review. Do I understand correctly that we should also add an accessor to ApacheDSContainer for configured port value i.e 0?

It's not for the configured value, but for the actual port that is resolved by TcpTransport.

I think it should be getLocalPort for the actual port.

Yes we want the name of the method to be getLocalPort

rwinch

Thanks for the quick turnaround. I have provided some feedback inline.

rwinch · 2020-04-24T14:51:48Z

ldap/src/main/java/org/springframework/security/ldap/server/ApacheDSContainer.java

+
+		SocketAcceptor socketAcceptor = server.getSocketAcceptor(transport);
+		InetSocketAddress localAddress = socketAcceptor.getLocalAddress();
+		this.localPort = localAddress.getPort();


I think this logic should probably be moved to the start method since a user could invoke start directly.

Should we also move the server initialization to the start method?

Let's try and make as little changes in this PR as possible. If we see additional room for improvement, let's create separate tickets.

rwinch

Thanks. This looks good. The last changes are to have the code take advantage of these changes. For example, LdapAuthenticationProviderConfigurer can now be updated to use the fact that 0 works.

I'm also wondering if it is actually better to use getPort() to return the used port if 0 is passed in since if the DLS or XML config passes 0, then the port returns the randomly selected port.

evgeniycheban · 2020-05-03T02:00:31Z

@rwinch I made changes to LdapAuthenticationProviderConfigurer.

rwinch

Thanks for the updates. We still need changes in LdapServerBeanDefinitionParser. We should also make sure our integration tests are always using port 0 and not use new ServerSocket(0).getLocalPort() anymore.

evgeniycheban · 2020-05-06T20:37:36Z

Thanks for the updates. We still need changes in LdapServerBeanDefinitionParser. We should also make sure our integration tests are always using port 0 and not use new ServerSocket(0).getLocalPort() anymore.

@rwinch I made changes to LdapServerBeanDefinitionParser as well.
What should we do with tests that use ServerSocket(0).getLocalPort() to get a list of available ports? For example ApacheDSContainerTests#multipleInstancesSimultanciously.

evgeniycheban · 2020-05-13T18:44:47Z

@rwinch Could you please take a look at the latest changes when you have a moment?

rwinch

Thanks for the updates. I've provided feedback inline

rwinch · 2020-05-18T15:16:19Z

ldap/src/main/java/org/springframework/security/ldap/DefaultSpringSecurityContextSource.java

+	 * @param container the embedded DS container
+	 * @return an instance which will connect to the embedded LDAP server
+	 */
+	public static DefaultSpringSecurityContextSource createEmbeddedContextSource(EmbeddedDsContainer container) {


I don't think we should need to add this method. I'd prefer to keep as many things private scope as possible.

I added this factory method for XML config because we need to create and post process a bean of the embedded container first and then use the actual port value to build a url.

This can likely be done with a package private method within the config itself.

Could you please provide some example of how this can be done in BeanDefinitionParser?

Got it. Added EmbeddedLdapServerConfigBean with factory method.

rwinch · 2020-05-18T15:17:22Z

ldap/src/main/java/org/springframework/security/ldap/server/EmbeddedDsContainer.java

+ * @see ApacheDSContainer
+ * @see UnboundIdContainer
+ */
+public interface EmbeddedDsContainer {


I don't think we should need to add a new public interface.

Fixes spring-projectsgh-8144

rwinch · 2020-05-21T22:10:27Z

Thanks for the Pull Request and your patience as we worked on finding the best solution! This is now merged into master via 0fa339f 😄

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 18, 2020

rwinch requested changes Apr 20, 2020

View reviewed changes

evgeniycheban force-pushed the gh-8144 branch from 5922b16 to 1b7daae Compare April 22, 2020 18:06

evgeniycheban requested a review from rwinch April 22, 2020 18:07

evgeniycheban force-pushed the gh-8144 branch from 1b7daae to 3e7f8b6 Compare April 22, 2020 18:23

rwinch requested changes Apr 24, 2020

View reviewed changes

evgeniycheban force-pushed the gh-8144 branch from 3e7f8b6 to 491138d Compare April 28, 2020 15:28

evgeniycheban requested a review from rwinch April 28, 2020 17:22

rwinch requested changes Apr 29, 2020

View reviewed changes

evgeniycheban force-pushed the gh-8144 branch from 491138d to 9308ad5 Compare May 3, 2020 01:48

evgeniycheban requested a review from rwinch May 3, 2020 02:04

evgeniycheban force-pushed the gh-8144 branch 4 times, most recently from 5c060c6 to ec8cf95 Compare May 3, 2020 14:28

rwinch requested changes May 5, 2020

View reviewed changes

evgeniycheban force-pushed the gh-8144 branch 3 times, most recently from a79cbd8 to 56d154f Compare May 6, 2020 18:09

evgeniycheban force-pushed the gh-8144 branch 4 times, most recently from 62586fe to 358251b Compare May 8, 2020 23:54

evgeniycheban requested a review from rwinch May 11, 2020 18:59

rwinch requested changes May 18, 2020

View reviewed changes

evgeniycheban force-pushed the gh-8144 branch from 358251b to e0f8d4d Compare May 18, 2020 23:55

Allow port=0 for ApacheDSContainer

446c560

Fixes spring-projectsgh-8144

evgeniycheban force-pushed the gh-8144 branch from e0f8d4d to 446c560 Compare May 19, 2020 02:22

evgeniycheban requested a review from rwinch May 19, 2020 09:47

rwinch added in: ldap An issue in spring-security-ldap type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels May 21, 2020

rwinch self-assigned this May 21, 2020

rwinch added this to the 5.4.0-M2 milestone May 21, 2020

rwinch closed this May 21, 2020

Allow port=0 for ApacheDSContainer #8416

Allow port=0 for ApacheDSContainer #8416

Uh oh!

Conversation

evgeniycheban commented Apr 18, 2020

Uh oh!

rwinch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwinch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwinch left a comment

Choose a reason for hiding this comment

Uh oh!

evgeniycheban commented May 3, 2020

Uh oh!

rwinch left a comment

Choose a reason for hiding this comment

Uh oh!

evgeniycheban commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evgeniycheban commented May 13, 2020

Uh oh!

rwinch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evgeniycheban May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwinch commented May 21, 2020

Uh oh!

Uh oh!

evgeniycheban commented May 6, 2020 •

edited

Loading

evgeniycheban May 18, 2020 •

edited

Loading