Skip to content

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

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.

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

Closed
ybbabr19 opened this issue Jan 7, 2020 · 14 comments
Closed

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

ybbabr19 opened this issue Jan 7, 2020 · 14 comments
Assignees
Labels
in: ldap An issue in spring-security-ldap status: ideal-for-contribution An issue that we actively are looking for someone to help us with type: enhancement A general enhancement
Milestone

Comments

@ybbabr19
Copy link

ybbabr19 commented Jan 7, 2020

Summary

When configuring an embedded ldap server to import an ldif file (e.g., users.ldif), no log message (at not level) is written nor any exception is thrown if the provided file does not exist (e.g., uesrs.ldif):

private void importLdif(InMemoryDirectoryServer directoryServer) {
	if (StringUtils.hasText(this.ldif)) {
		Resource resource = this.context.getResource(this.ldif);
		try {
			if (resource.exists()) {
				try (InputStream inputStream = resource.getInputStream()) {
					directoryServer.importFromLDIF(false, new LDIFReader(inputStream));
				}
			}
		} catch (Exception ex) {
			throw new IllegalStateException("Unable to load LDIF " + this.ldif, ex);
		}
	}
}

While it is arguably legitimate not to throw an exception in this case and load the ldif file from a default location, logging a message out that warns the user that the configured ldif file was not found could spare some debugging time ;)

Actual Behavior

No log message is provided upon an unsuccessful import of a ldif file.

Expected Behavior

A log message that warns the users that the configured ldif file does not exist

Configuration

<security:ldap-server
		id="local-ldap-server"
		ldif="file:${configpath}/uesrs.ldif"
		port="8389"
		root="dc=springframework,dc=org"
	/>

On the file system however, the file is: ${configpath}/users.ldif

Version

5.2.1.RELEASE

Sample

https://github.com/spring-projects/spring-security/blob/master/ldap/src/main/java/org/springframework/security/ldap/server/UnboundIdContainer.java

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 7, 2020
@jzheaux jzheaux added in: ldap An issue in spring-security-ldap and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 14, 2020
@eleftherias
Copy link
Contributor

Thanks for the report @ybbabr19.

I agree that the application should not fail silently if the LDIF file is not found.
I think it is best to throw an exception.
Then it will behave the same as ApacheDSContainer.

Are you interested in submitting a PR to fix this?

@eleftherias eleftherias added the type: enhancement A general enhancement label Jan 16, 2020
@eleftherias eleftherias added the status: ideal-for-contribution An issue that we actively are looking for someone to help us with label Jan 28, 2020
@dratler
Copy link
Contributor

dratler commented Feb 20, 2020

If this still an option I would like to contribute to this issue.

@eleftherias
Copy link
Contributor

@dratler thank you, it's yours

@eleftherias
Copy link
Contributor

@dratler Have you had the chance to look into this issue?

@dratler
Copy link
Contributor

dratler commented Apr 5, 2020

Hi @eleftherias ,
I'm sorry I was over booked.
I'm trying yo build ldap server locally .
do you have something prepraed ?

@eleftherias
Copy link
Contributor

@dratler
Copy link
Contributor

dratler commented Apr 9, 2020

Hi @eleftherias ,
I have start wortking on it, I know there is an internal LDAP (LDIF) been used.
I would like to use that on is it fine ?
Also there are missing dependency within Gradel (@PreDestory annotation etc...).
It might take a while...

@eleftherias
Copy link
Contributor

@dratler
I hope the guidelines below can help you with the initial setup.

This issue can be demonstrated using the LDAP sample with a few modifications.
Firstly, add .contextSource().ldif("classpath:does-not-exist.ldif") to the configuration in SecurityConfig.java.
Then, to make the application use "unboundid" mode, replace compile apachedsDependencies with compile "com.unboundid:unboundid-ldapsdk" in the spring-security-samples-javaconfig-ldap.gradle.

If you run the application, you will see that no exception is thrown, even though the file does-not-exist.ldif is not on the classpath.
If you were to use "apacheds" mode, then you would see an exception when the file is not found.
This is the desired behaviour.
If you are curious to try "apacheds" mode, then you can update SecurityConfig.java as mentioned above, but leave the gradle file unchanged (still using the Apache DS dependencies).

The changes required to fix this will be in UnboundIdContainer, specifically the importLdif function.
We will need to throw an exception if the supplied resource (resources[0]) does not exist.

Could you please clarify what you mean in your comment above about the missing gradle dependency?

Please don't hesitate to reach out if you need any help or clarifications.
Thank you for taking the time to contribute!

@dratler
Copy link
Contributor

dratler commented Apr 11, 2020

Hi @eleftherias ,
I'm getting package javax.annotation does not exist
it's been thrown at the following classes :

  • src/integration-test/java/org/springframework/security/ldap/server/UnboundIdContainerLdifTests.java
    src/integration
  • test/java/org/springframework/security/ldap/userdetails/LdapUserDetailsManagerModifyPasswordTests.java
  • src/integration-test/java/org/springframework/security/ldap/ApacheDsContainerConfig.java

It affects @PreDestotry annotation.
I can add it into Gradle Compile or I'm missing something out?

For example the error

@PreDestroy
	 ^
  symbol:   class PreDestroy
  location: class ApacheDsContainerConfig

@eleftherias
Copy link
Contributor

@dratler You do not need to add any additional gradle dependencies.
Are you seeing this error in an IDE or command line?
Are you able to successfully run ./gradlew clean build?

@dratler
Copy link
Contributor

dratler commented Apr 14, 2020

Hi @eddumelendez ,
I'm able to run successfully ./gradlew clean build .
before I run Gradle clean and build via IDE , I'm using IntelliJ.
Now I'm trying to add .contextSource().ldif("classpath:does-not-exist.ldif") to SecurityConfig.java.
which one is it?
Is there a need to install also SprintFramework ?
the path for such file is src/main/resources/org/springframework/security/config/ I have found it at spring-security-config.gradle

thanks you for all the help

@eleftherias
Copy link
Contributor

Is there a need to install also Spring Framework ?

@dratler Spring Framework is already a dependency of Spring Security. There is no additional work needed to use Spring Framework.

I'm able to run successfully ./gradlew clean build .
before I run Gradle clean and build via IDE , I'm using IntelliJ.

If you are able to run ./gradlew clean build from the command line, and you cannot compile the project in the IDE, then it means the IDE is not configured correctly. Make sure you are importing it as a Gradle project.

Now I'm trying to add .contextSource().ldif("classpath:does-not-exist.ldif") to SecurityConfig.java.
which one is it?

You can add that to the LDAP sample which is found in samples/javaconfig/ldap.
This is only to demonstrate the issue.

If you have any further questions, you could open a draft pull request and we can discuss there.

@dratler
Copy link
Contributor

dratler commented Apr 25, 2020

Hi @eleftherias ,
here is the #8434 I have opened with the tests for this issue .
I would like to here your comments

@dratler
Copy link
Contributor

dratler commented May 6, 2020

Hi @eleftherias ,
I think I have resolved the issue.
I have also fixed some test classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: ldap An issue in spring-security-ldap status: ideal-for-contribution An issue that we actively are looking for someone to help us with type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants