Add schema validation options for embedded LDAP #8195

mouellet · 2017-02-04T16:45:18Z

Unboundid uses a standard schema to validate LDIF files so importing one with a custom attribute type or object class fails.

This spring.ldap.embedded.validation.enabled property provides a way to deactivate the schema validation.

Alternatively, a custom schema file can be specified with the spring.ldap.embedded.validation.schema property. Such schema typically contains new or customized attribute types or object classes, so it's merged with the standard schema.

Fixes gh-8171

pivotal-issuemaster · 2017-02-04T16:45:19Z

@mouellet Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

pivotal-issuemaster · 2017-02-04T16:46:55Z

@mouellet Thank you for signing the Contributor License Agreement!

snicoll

Thanks for the PR! I've added a few comments there.

snicoll · 2017-02-05T08:03:05Z

...java/org/springframework/boot/autoconfigure/ldap/embedded/EmbeddedLdapAutoConfiguration.java

+		if (this.embeddedProperties.getValidation().isEnabled()) {
+			schema = Schema.getDefaultStandardSchema();
+			String schemaLocation = this.embeddedProperties.getValidation().getSchema();
+			if (StringUtils.hasText(schemaLocation)) {


Rather than using a String there you could use a Resource. That way the binder will do the work of making sure that resource is valid (if set).

Good point! Should the spring.ldap.embedded.ldif property also use a Resource? I'm not sure about having two ways to load a resource in this same configuration.

Good point. I'll take care of that once I've merged this PR.

snicoll · 2017-02-05T08:04:05Z

...java/org/springframework/boot/autoconfigure/ldap/embedded/EmbeddedLdapAutoConfiguration.java

+				}
+			}
+		}
+		config.setSchema(schema);


Not sure why you need to keep a handle to the schema there outside of the if block. I'd rather keep all that private (unless set it to null has some kind of purpose?)

Yes, setting the schema to null is what disables the validation. Should it be more explicit with something like https://github.com/jgribonvald/embedded-ldap/blob/master/src/test/java/org/esco/test/ldap/LdapServerRule.java#L69-L76?

I think that just relying on the fact that the property is intialized to null is not very clear. I'd rather have an explicit call to null. Can you update your PR with what we've discussed here please?

snicoll · 2017-02-05T08:04:48Z

...c/main/java/org/springframework/boot/autoconfigure/ldap/embedded/EmbeddedLdapProperties.java

+	/**
+	 * Schema validation
+	 */
+	private Validation validation = new Validation();


Nested configuration properties can be (and should be) final. You don't need a setter since the object instance for this properties object is always there.

snicoll · 2017-02-05T08:05:43Z

...c/main/java/org/springframework/boot/autoconfigure/ldap/embedded/EmbeddedLdapProperties.java

+		/**
+		 * Enable LDAP schema validation
+		 */
+		private boolean enabled = true;


What is the semantic of that flag? Does it make sense to set a schema and disable validation? Why do you need to set both a schema and the validation flag. Perhaps we should check for the presence of the schema only?

Those really are two features. Most of the time you'll only need to disable the validation altogether and thats basically what the flag is for.

But if you don't have custom attribute types or object classes you can still have validation against the default standard schema provided by unboundid as it is initialized in InMemoryDirectoryServerConfig with Schema.getDefaultStandardSchema(). So not providing a custom schema here doesn't mean you don't want validation.

Of course it doesn't make sense to set a schema and disable validation...

snicoll · 2017-02-06T13:10:52Z

@mouellet it looks like you've done a weird rebase of your branch. Can you squash your commits in a single one and push force in your branch? There are a lot of unrelated changes in your branch now. Thanks.

snicoll · 2017-02-08T12:39:37Z

ping @mouellet

snicoll · 2017-02-08T12:40:08Z

Ah sorry I just pinged you but you've fixed it. Awesome, thanks!

snicoll · 2017-02-08T13:56:55Z

...java/org/springframework/boot/autoconfigure/ldap/embedded/EmbeddedLdapAutoConfiguration.java

+			Resource schemaLocation = this.embeddedProperties.getValidation().getSchema();
+			try {
+				config.setSchema(Schema.mergeSchemas(Schema.getDefaultStandardSchema(),
+						Schema.getSchema(schemaLocation.getFile())));


You can't really do that I am afraid. Resource is an abstraction and it may not return a file. The only reason why the tests currently pass is because you are running them with target/test-classes so resources effectively point to a file. If you use the same inside a repackaged jar, getFile will return null.

I am not super keen to introduce a feature that requires to point to java.io.File. Looking at the code for Schema, there isn't an easy way to read a Schema from a InputStream. IMO that feature should be available before we proceed with this PR

I took a closer look at the Schema class and I think there's a simpler solution, delegate the whole file loading to that class... It does mean reverting the property to a String. I'll push a sample, let me know what you think!

snicoll · 2017-02-08T13:57:14Z

...c/main/java/org/springframework/boot/autoconfigure/ldap/embedded/EmbeddedLdapProperties.java

@@ -109,4 +120,34 @@ public void setPassword(String password) {

 	}

+	static class Validation {


Must be public

snicoll

Let's keep those changes on hold for the moment. I'll revisit your PR once we have a new version.

snicoll · 2017-02-10T08:51:34Z

...java/org/springframework/boot/autoconfigure/ldap/embedded/EmbeddedLdapAutoConfiguration.java

 			try {
 				config.setSchema(Schema.mergeSchemas(Schema.getDefaultStandardSchema(),
-						Schema.getSchema(schemaLocation.getFile())));
+						Schema.getSchema(


That's not going to work. That code expects the String to point to a File. I've opened an issue to get an InputStream variant so I'd rather wait for that.

snicoll · 2017-02-20T09:01:26Z

Awesome, 3.2.1 contains the method I've requested

See gh-8195

* pr/8195: Polish contribution Add schema validation options for embedded LDAP

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 4, 2017

snicoll requested changes Feb 5, 2017

View reviewed changes

snicoll mentioned this pull request Feb 5, 2017

Embedded LDAP server schema validation options #8171

Closed

mouellet added a commit to mouellet/spring-boot that referenced this pull request Feb 6, 2017

Provide requested changes to spring-projects#8195

c0b6fa5

mouellet added a commit to mouellet/spring-boot that referenced this pull request Feb 6, 2017

Provide requested changes to spring-projects#8195

69e5aeb

snicoll added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Feb 6, 2017

snicoll added this to the 1.5.2 milestone Feb 6, 2017

Add schema validation options for embedded LDAP

c230c92

mouellet force-pushed the gh-8171 branch from 09c2958 to c230c92 Compare February 7, 2017 11:08

snicoll self-assigned this Feb 7, 2017

snicoll requested changes Feb 8, 2017

View reviewed changes

snicoll added the status: on-hold We can't start working on this issue yet label Feb 8, 2017

snicoll removed this from the 1.5.2 milestone Feb 8, 2017

snicoll mentioned this pull request Feb 9, 2017

Create a Schema from an InputStream pingidentity/ldapsdk#23

Closed

mouellet added 2 commits February 9, 2017 05:53

Making nested class public

ffa2258

Delegating schema file loading to external library

562f5eb

snicoll reviewed Feb 10, 2017

View reviewed changes

snicoll removed the status: on-hold We can't start working on this issue yet label Feb 20, 2017

snicoll added this to the 1.5.2 milestone Feb 20, 2017

snicoll mentioned this pull request Feb 20, 2017

Upgrade to Unboundid LDAP SDK 3.2.1 #8333

Closed

snicoll pushed a commit that referenced this pull request Feb 20, 2017

Add schema validation options for embedded LDAP

fa8f0a6

See gh-8195

snicoll added a commit that referenced this pull request Feb 20, 2017

Merge pull request #8195 from mouellet:gh-8171

3b88609

* pr/8195: Polish contribution Add schema validation options for embedded LDAP

snicoll closed this in 43a534f Feb 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add schema validation options for embedded LDAP #8195

Add schema validation options for embedded LDAP #8195

mouellet commented Feb 4, 2017

pivotal-issuemaster commented Feb 4, 2017

pivotal-issuemaster commented Feb 4, 2017

snicoll left a comment

snicoll Feb 5, 2017

mouellet Feb 5, 2017

snicoll Feb 6, 2017

snicoll Feb 5, 2017

mouellet Feb 5, 2017

snicoll Feb 6, 2017

snicoll Feb 5, 2017

snicoll Feb 5, 2017

mouellet Feb 5, 2017

snicoll commented Feb 6, 2017

snicoll commented Feb 8, 2017

snicoll commented Feb 8, 2017

snicoll Feb 8, 2017

mouellet Feb 9, 2017

snicoll Feb 8, 2017

snicoll left a comment

snicoll Feb 10, 2017

snicoll commented Feb 20, 2017

		@@ -109,4 +120,34 @@ public void setPassword(String password) {

		}

		static class Validation {

Add schema validation options for embedded LDAP #8195

Add schema validation options for embedded LDAP #8195

Conversation

mouellet commented Feb 4, 2017

pivotal-issuemaster commented Feb 4, 2017

pivotal-issuemaster commented Feb 4, 2017

snicoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snicoll commented Feb 6, 2017

snicoll commented Feb 8, 2017

snicoll commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snicoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snicoll commented Feb 20, 2017