Allow placeholder in headers disabled properties

[rptmat]

Summary

In the xml configuration for Spring Security, the xsd fail to validate attribute disabled of the headers element when using a placeholder

Actual Behavior

error thrown: cvc-datatype-valid.1.2.1: '${security.headers.disabled}' is not a valid value for 'boolean'

Expected Behavior

it should work and resolve the property

Configuration

<security:headers disabled="${security.headers.disabled}"/>

Version

5.1.3

[clevertension]

yes, the property is hard coded in org.springframework.security.config.http.HeadersBeanDefinitionParser,

boolean disabled = element != null
				&& "true".equals(element.getAttribute("disabled"));

it is easy to support, but now we should wait for the confirmation

[jzheaux]

Great idea, @rptmat57, would you be interested in submitting a PR?

[rhamedy]

I would like to work on this if @rptmat57 didn’t want to ☺️

[clevertension]

@jzheaux should the defaults-disabled property also be supported too?, just like

<security:headers disabled="${security.headers.disabled}" defaults-disabled="${security.headers.defaultDisabled}"/>

[jzheaux]

@clevertension yes, that sounds reasonable - I've updated the title accordingly

[jzheaux]

Okay, @rhamedy, let's see what @rptmat57 says first and then go from there.

[rptmat]

this all sounds great!
@rhamedy go ahead 👍

[clevertension]

@rhamedy good luck and fighting 👍

[rhamedy]

@jzheaux a quick update

If I understand correctly, for this issue we need to

• Update the spring-security-x-y.rnc file to replace boolean type with string and allow default to false
• Write a test that uses a sample xml config with headers and placeholder for disabled and defaults-disabled fields

Note: Currently none of the rnc files use a default prop as shown below.

I have updated managed to update the spring-security-4-0.rnc up to spring-security-5.2.rnc with the following

headers-options.attlist &=
	## Specifies if the default headers should be disabled. Default false.
	[ a:defaultValue = "false" ]
	attribute defaults-disabled {xsd:string}?
headers-options.attlist &=
	## Specifies if headers should be disabled. Default false.
	[ a:defaultValue = "false" ]
	attribute disabled {xsd:string}?

After running the necessary gradle command, the xsds are updated. The Eclipse IDE was still referencing the xsd from the web so I had to do a quick hack (using chrome web server) to point it to a the local updated xsd directory. How is the test/build going to pass when I submit a PR?

In order to write a test, I would also need to

• Add a properties file for placeholder in src/test/resources or somewhere accessible
• Update the configuration xml to include context xsd for the following
  <context:property-placeholder location="classpath:.properties" />

Does this make sense?

[jzheaux]

You are on the right path, though I would make just a few tweaks:

• Let's leave the default values out. For all other boolean values (as you noted), the default is enforced in the code.
• I'd probably use xsd:token since it is more specific and we'll get whitespace trimming
• Instead of a properties file, I might just do System.setProperty in the test, but yes, you'd likely need to add the PropertyPlaceholderConfigurer bean.
• You will likely need to edit HeadersBeanDefinitionParser to process the placeholder, e.g. parserContext.getReaderContext().getEnvironment().resolvePlaceholders()

If you wouldn't mind, please also update the spring.schemas file so that it points to spring-security-5.2.xsd.

Regarding your IDE, I think you may need to configure it to be aware of where on the file system the xsd is. I'm not certain that Eclipse XML validation knows about the spring.schemas file without some plugin. This won't be a problem on the build box because, at runtime, Spring will use a custom entity resolver to lookup the appropriate xsd on the classpath.

I'll also note here, in passing, that there is a difference between ${my.property} and #{systemProperties['my.property']} in Spring. The first is a simple property placeholder while the second is a SpEL expression. To get SpEL to work will likely be quite a bit more work, just because the decision about which headers to include actually is exercised there in the BeanDefinitionParser before the BeanFactory is available. I'd advise only adding ${} support for the time being.

[rhamedy]

@jzheaux thanks a lot for your reply. I have covered all the points but, what I was missing was the following piece

You will likely need to edit HeadersBeanDefinitionParser to process the placeholder, e.g. parserContext.getReaderContext().getEnvironment().resolvePlaceholders()

The Element kept returning the actual value without substituting. Thanks for response, will give it a try and make a PR 👍

[rhamedy]

@jzheaux I have mentioned a few points in the PR that might or might not need to be considered during code review. Thanks for the help 👍