Throw exception when specified ldif file does not exist #8434

dratler · 2020-04-25T07:20:00Z

Thanks for all the aid here is a PR I have opened with tests that I understand is missing and once they will throw exceptions this PR will be completed.

…nding of the task

eleftherias

Thanks for the PR @dratler!This is a great starting point.
I have left some comments inline.
I suggest adding the test to the UnboundIdContainerTests file, rather than creating a new one.

ldap/src/integration-test/java/org/springframework/security/ldap/SpringLdapFalseAuthTest.java

eleftherias · 2020-04-29T19:00:15Z

ldap/src/integration-test/java/org/springframework/security/ldap/SpringLdapFalseAuthTest.java

+		try {
+			new InMemoryDirectoryServer("dc=springframework,dc=org",
+					"classpath:missing-file.ldif");
+		} catch (LDAPException e) {


An IllegalArgumentException is more suitable in this case.
LDAPException should only be used when a problem occurs while performing LDAP-related processing.
In this case, we haven't started the LDAP-related processing yet, so we should use IllegalArgumentException.

The InMemoryDirectoryServer throw LDAPException I can wrap it but I think it's not a good practice.
public InMemoryDirectoryServer(final String... baseDNs) throws LDAPException

1. move the tests into UnboundIdContainerTests 2. Starting phase one of the commit

dratler · 2020-05-02T14:53:53Z

Hi @eleftherias ,
I have created a first draft of how to do this PR .
can you check if I'm on the right path ?

eleftherias

Thanks for the updates @dratler.
I have left feedback inline.

ldap/src/integration-test/java/org/springframework/security/ldap/SpringLdapFalseAuthTest.java

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

…curity into spring-projectsgh-7791

…ndIdContainerTests and to UnboundIdContainerLdifTests classes

…curity into spring-projectsgh-7791

eleftherias

Thanks for the updates @dratler!
I have requested some changes inline.

eleftherias · 2020-05-12T13:15:07Z

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

+		if (!StringUtils.hasText(ldif)) {
+			throw new IllegalArgumentException("Unable to load LDIF " + ldif);
+		}
+		if (!ldif.endsWith(".ldif")) {


Please remove this check. We do not require the file to have a ".ldif" extension to be processed.

eleftherias · 2020-05-12T13:16:02Z

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

@@ -67,7 +76,9 @@ public void setPort(int port) {

 	@Override
 	public void destroy() {
-		stop();
+		if (null != directoryServer) {


Let's keep this commit focused on throwing an exception with the LDIF file does not exist.
This doesn't seem like it is part of that.

eleftherias · 2020-05-12T13:58:07Z

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

+			if (resources.length > 0
+					&& resources[0].exists()
+					&& resources[0].isFile()
+					&& resources[0].isReadable()) {


With these changes, there is still no exception thrown when the file does not exist.
We need to check if the LDIF exists and throw an exception if it does not.
We can add the check for isFile() and isReadable() after we verify it exists.

Suggested change

if (resources.length > 0

&& resources[0].exists()

&& resources[0].isFile()

&& resources[0].isReadable()) {

if (resources.length > 0) {

if (!resources[0].exists()) {

throw new IllegalArgumentException("Could not find LDIF " + this.ldif);

}

.../integration-test/java/org/springframework/security/ldap/server/UnboundIdContainerTests.java

eleftherias · 2020-05-12T14:13:53Z

.../integration-test/java/org/springframework/security/ldap/server/UnboundIdContainerTests.java

+			server.destroy();
+		}
+	}
+


Please add a test for the original issue.
The test should create an UnboundIdContainer with an LDIF that does not exist, and ensure that an exception is thrown.

eleftherias

Thanks for the updates @dratler.
I have left some more comments inline.

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

.../integration-test/java/org/springframework/security/ldap/server/UnboundIdContainerTests.java

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

dratler · 2020-05-27T20:23:16Z

Hi @eleftherias ,
Sorry about the delay but I think this is the right one

eleftherias · 2020-05-28T18:59:05Z

@dratler It looks like the tests are still failing. Were you able to run the build on your local machine?

dratler · 2020-05-28T22:30:26Z

Hi @eleftherias ,
I have run it on my local machine twice before pushing it.
I didn't have any issues.
Let me check the error and I will work on them.
Sorry about thta

…curity into spring-projectsgh-7791

eleftherias · 2020-05-28T23:00:01Z

Thanks @dratler. You can also check out the Travis build that ran for this PR https://travis-ci.org/github/spring-projects/spring-security/builds/691902029

dratler · 2020-06-01T19:14:11Z

Hi @eleftherias ,
I have an issue I would like to advise with you.
First I would like to applogize about the previous commit after cleaning my machine I was able to reproduce it.
When performing ./gradlew build clean I'm getting execution regarding LdapServerBeanDefinitionParserTests. apacheDirectoryServerIsStartedByDefault related to Execution failed for task ':spring-security-itest-ldap-embedded-mode-unboundid:integrationTest'.
but the Stack trace indicates the following: org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'org.springframework.security.unboundidServerContainer': Invocation of init method failed; nested exception is java.lang.IllegalStateException: Unable to load LDIF classpath:users.ldif at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.initializeBean(AbstractAutowireCapableBeanFactory.java:1796) at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:595) at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:517) at org.springframework.beans.factory.support.AbstractBeanFactory.lambda$doGetBean$0(AbstractBeanFactory.java:323) at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.getSingleton(DefaultSingletonBeanRegistry.java:226) at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:321) at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:202) at org.springframework.beans.factory.support.DefaultListableBeanFactory.preInstantiateSingletons(DefaultListableBeanFactory.java:895) at org.springframework.context.support.AbstractApplicationContext.finishBeanFactoryInitialization(AbstractApplicationContext.java:878) at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:550) at org.springframework.context.support.ClassPathXmlApplicationContext.<init>(ClassPathXmlApplicationContext.java:144) at org.springframework.context.support.ClassPathXmlApplicationContext.<init>(ClassPathXmlApplicationContext.java:85) at org.springframework.security.LdapServerBeanDefinitionParserTests.setup(LdapServerBeanDefinitionParserTests.java:38) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:24) at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27) at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) at org.junit.runners.ParentRunner.run(ParentRunner.java:363) at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.runTestClass(JUnitTestClassExecutor.java:110) at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute(JUnitTestClassExecutor.java:58) at org.gradle.api.internal.tasks.testing.junit.JUnitTestClassExecutor.execute(JUnitTestClassExecutor.java:38) at org.gradle.api.internal.tasks.testing.junit.AbstractJUnitTestClassProcessor.processTestClass(AbstractJUnitTestClassProcessor.java:62) at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.processTestClass(SuiteTestClassProcessor.java:51) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36) at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24) at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33) at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:94) at com.sun.proxy.$Proxy2.processTestClass(Unknown Source) at org.gradle.api.internal.tasks.testing.worker.TestWorker.processTestClass(TestWorker.java:118) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36) at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24) at org.gradle.internal.remote.internal.hub.MessageHubBackedObjectConnection$DispatchWrapper.dispatch(MessageHubBackedObjectConnection.java:182) at org.gradle.internal.remote.internal.hub.MessageHubBackedObjectConnection$DispatchWrapper.dispatch(MessageHubBackedObjectConnection.java:164) at org.gradle.internal.remote.internal.hub.MessageHub$Handler.run(MessageHub.java:413) at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:64) at org.gradle.internal.concurrent.ManagedExecutorImpl$1.run(ManagedExecutorImpl.java:48) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) at org.gradle.internal.concurrent.ThreadFactoryImpl$ManagedThreadRunnable.run(ThreadFactoryImpl.java:56) at java.lang.Thread.run(Thread.java:748) Caused by: java.lang.IllegalStateException: Unable to load LDIF classpath:users.ldif at org.springframework.security.ldap.server.UnboundIdContainer.importLdif(UnboundIdContainer.java:133) at org.springframework.security.ldap.server.UnboundIdContainer.start(UnboundIdContainer.java:111) at org.springframework.security.ldap.server.UnboundIdContainer.afterPropertiesSet(UnboundIdContainer.java:82) at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.invokeInitMethods(AbstractAutowireCapableBeanFactory.java:1855) at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.initializeBean(AbstractAutowireCapableBeanFactory.java:1792) ... 60 more Caused by: java.lang.IllegalArgumentException: Unable to load LDIF :classpath:users.ldif at org.springframework.security.ldap.server.UnboundIdContainer.lambda$locateResource$1(UnboundIdContainer.java:150) at java.util.Optional.orElseThrow(Optional.java:290) at org.springframework.security.ldap.server.UnboundIdContainer.locateResource(UnboundIdContainer.java:150) at org.springframework.security.ldap.server.UnboundIdContainer.importLdif(UnboundIdContainer.java:128) ... 64 more
and there are 5 option can you help or advice how to debug that issue ?
I tried running these manually and they seem to be working and I want to resolve the issue

eleftherias · 2020-06-01T20:03:09Z

Thanks for reaching out @dratler.
Here is why the test apacheDirectoryServerIsStartedByDefault is throwing an exception:

The updated code this.context.getResources(CLASSPATH_ALL_URL_PREFIX) is getting multiple resources from the classpath, not just LDIFs.
Then Objects.requireNonNull(resource.getFilename()).equalsIgnoreCase(this.ldif) is trying to match the resources to the provided LDIF pattern which is "classpath:users.ldif", but no file is named that, the file in this example is simply "users.ldif".

To mitigate this, we do not need to get all resources, just the LDIFs that the user has specified with their pattern.

Resource[] resources = this.context.getResources(this.ldif);

Then, if the resource they are asking for does not exist, we should throw an exception.

if (resources.length > 0) {
	if (!resources[0].exists()) {
			throw new IllegalArgumentException("Could not find LDIF " + this.ldif);
	}
	// ...

Let me know if this is unclear or if you have any questions.

dratler · 2020-06-08T18:54:41Z

Hi @eleftherias ,
I'm checking to see if a fix I have to build is working please ignore this commit.
thank you

…curity into spring-projectsgh-7791

dratler · 2020-06-21T19:33:40Z

Hi @eleftherias ,
I think I have completed this assignment I'm sorry it took a while, please review it and I will comply according to you comment.
thank you for the time and all the help.

eleftherias

Thanks for the updates @dratler.
I have left some more feedback inline.

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

ldap/src/integration-test/resources/applicationContext-security.xml

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

…curity into spring-projectsgh-7791

dratler · 2020-07-18T18:53:20Z

Hi @eleftherias ,
I think I'm done.
please review this PR thank you for all the inputs.

eleftherias · 2020-07-22T17:01:19Z

Thanks @dratler! The changes have been merged to master via 7c4a706.

Added file Test that I understand is needed - this is is for understa…

753a927

…nding of the task

dratler mentioned this pull request Apr 25, 2020

No log message or exception if expected ldif file does not exist #7791

Closed

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

eleftherias self-assigned this Apr 27, 2020

eleftherias suggested changes Apr 29, 2020

View reviewed changes

eleftherias changed the title ~~PR 7791~~ Throw exception when specified ldif file does not exist Apr 30, 2020

Following inputs I have done the followings:

4a6b497

1. move the tests into UnboundIdContainerTests 2. Starting phase one of the commit

adding formating

ab67520

eleftherias suggested changes May 5, 2020

View reviewed changes

sdratler1 added 4 commits May 5, 2020 23:53

Merge branch 'master' of https://github.com/spring-projects/spring-se…

075458b

…curity into spring-projectsgh-7791

Adding ldap fix to UnboundIdContainer class and adding tests to Unbou…

921fbab

…ndIdContainerTests and to UnboundIdContainerLdifTests classes

adding some test and fix missing context on destory

10d3492

Merge branch 'master' of https://github.com/spring-projects/spring-se…

8db1207

…curity into spring-projectsgh-7791

eleftherias suggested changes May 12, 2020

View reviewed changes

sdratler1 added 2 commits May 17, 2020 23:10

Fixing the tests

2a8c5dd

Fix test

c99b252

eleftherias suggested changes May 21, 2020

View reviewed changes

eleftherias 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 27, 2020

fixing ldif loading issue

9fac54f

Merge branch 'master' of https://github.com/spring-projects/spring-se…

7429f9e

…curity into spring-projectsgh-7791

Adding more tests and better validation

bc8e099

attempt to fix the issue + add to git ignore /lib/ and .hprof files

23ac382

sdratler1 added 3 commits June 16, 2020 23:31

attempt to resolve issue

179a466

Merge branch 'master' of https://github.com/spring-projects/spring-se…

e4c9d81

…curity into spring-projectsgh-7791

attempt fix 1

0efb886

eleftherias suggested changes Jun 23, 2020

View reviewed changes

sdratler1 added 2 commits July 14, 2020 23:54

handle requested rejects

1fa8a4b

Merge branch 'master' of https://github.com/spring-projects/spring-se…

479d2bc

…curity into spring-projectsgh-7791

dratler requested a review from eleftherias July 18, 2020 18:53

eleftherias closed this Jul 22, 2020

eleftherias added the status: duplicate A duplicate of another issue label Jul 22, 2020

eleftherias added this to the 5.4.0-RC1 milestone Jul 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw exception when specified ldif file does not exist #8434

Throw exception when specified ldif file does not exist #8434

dratler commented Apr 25, 2020

eleftherias left a comment

eleftherias Apr 29, 2020

dratler May 2, 2020

dratler commented May 2, 2020

eleftherias left a comment

eleftherias left a comment

eleftherias May 12, 2020

eleftherias May 12, 2020

eleftherias May 12, 2020

dratler May 15, 2020

eleftherias May 12, 2020

eleftherias left a comment

dratler commented May 27, 2020

eleftherias commented May 28, 2020

dratler commented May 28, 2020

eleftherias commented May 28, 2020

dratler commented Jun 1, 2020

eleftherias commented Jun 1, 2020

dratler commented Jun 8, 2020

dratler commented Jun 21, 2020

eleftherias left a comment

dratler commented Jul 18, 2020

eleftherias commented Jul 22, 2020

Throw exception when specified ldif file does not exist #8434

Throw exception when specified ldif file does not exist #8434

Conversation

dratler commented Apr 25, 2020

eleftherias left a comment

Choose a reason for hiding this comment

eleftherias Apr 29, 2020

Choose a reason for hiding this comment

dratler May 2, 2020

Choose a reason for hiding this comment

dratler commented May 2, 2020

eleftherias left a comment

Choose a reason for hiding this comment

eleftherias left a comment

Choose a reason for hiding this comment

eleftherias May 12, 2020

Choose a reason for hiding this comment

eleftherias May 12, 2020

Choose a reason for hiding this comment

eleftherias May 12, 2020

Choose a reason for hiding this comment

dratler May 15, 2020

Choose a reason for hiding this comment

eleftherias May 12, 2020

Choose a reason for hiding this comment

eleftherias left a comment

Choose a reason for hiding this comment

dratler commented May 27, 2020

eleftherias commented May 28, 2020

dratler commented May 28, 2020

eleftherias commented May 28, 2020

dratler commented Jun 1, 2020

eleftherias commented Jun 1, 2020

dratler commented Jun 8, 2020

dratler commented Jun 21, 2020

eleftherias left a comment

Choose a reason for hiding this comment

dratler commented Jul 18, 2020

eleftherias commented Jul 22, 2020