Introduced placeholder support for headers tag attributes

[rhamedy]

Fixes gh-6547

Two points

• Is it alright to update all the .rnc files starting from 4.2 upto 5.2? The reason I did that is because in one of the test's xml, I changed spring-security.xsd to spring-security-5.2.xsd (or 5.2) and it failed with error message complaining about schema not matching and changing to 4.2.xsd did not fail. This was before the change the schema.handlers.

• I am curious to whether I should include a check such as if (element.getAttribute(attributeName).indexOf("${") !== -1) in the resolveAttribute of HeadersBeanDefinitionParser. It might be expensive to run resolve even when it's not a property. I wonder if there are any edge cases with this if-check 🤔

In order to succeed in running the unit tests locally, I had to setup a local http server and make it point to the changed versions of .xsd files otherwise Eclipse pulls straight from the url which obviously has boolean type instead of token and xsd validation fails.

I was using a web server with a port (any port) that had : in it's URL and I found out that spring does not : in the URL of xml files and it converts it to = which means it fails to load the right NamespaceHandler.

[rhamedy]

I noticed that the previous build failed, and looking at the build logs the 6 new failing tests were run before Task :spring-security-config:rncToXsd 😕

Shouldn't the oder be other way round? If any changes to the rnc then run the rncToXsd task before running the tests?
@jzheaux ^

[jzheaux]

@rhamedy

Is it alright to update all the .rnc files starting from 4.2 up to 5.2?

Actually, let's just update 5.2. The reason is that we want clear alignment in what is supported in each released version as a whole. IOW, if we were to backport this to 5.1.x, etc., then we would update those accordingly. For this change, though, I believe we're just going to keep it to 5.2.x.

I am curious to whether I should include a check such as if (element.getAttribute(attributeName).indexOf("${") !== -1)

This will likely end up being more work than it's worth. We can perform that optimization later on down the road, if this causes performance issues.

Also to your specific concern, I believe a check like this is run very early by the placeholder resolver itself.

In order to succeed in running the unit tests locally, I had to setup a local http server

Hmm, that doesn't sound quite right. What should happen is Spring Framework's EntityResolver should look up the schema location on the classpath. Did you run rncToXsd before running the unit tests in your IDE? Is the spring-security-5.2.xsd file in your IDE correctly updated?

I noticed that the previous build failed, and looking at the build logs

For some reason, those logs aren't pulling up right now. If you wouldn't mind removing your changes to the older .rnc files, then when you force-push next time, I'll take a look at the build logs there and see if I can discover what is going on.

[rhamedy]

Alright! I reverted the changes to the old rnc files and only changed the 5.2 rnc file. I have also left the changes to the 5.2.xsd just to help the build do not rely on existing xsd. It should be alright as the build will overwrite it anyway. Let me know if you wish that to be dropped, I noticed that in previous build again the test were run before the rncToXsd task (you can see portion of the build below)

> Task :spring-security-config:checkstyleTest
> Task :spring-security-config:test
org.springframework.security.config.http.HttpHeadersConfigTests > requestWhenDefaultsDisabledWithPlaceholderFalseThenIncludeAllSecureHeaders FAILED
    org.springframework.beans.factory.xml.XmlBeanDefinitionStoreException at HttpHeadersConfigTests.java:199
        Caused by: org.xml.sax.SAXParseException at HttpHeadersConfigTests.java:199
org.springframework.security.config.http.HttpHeadersConfigTests > requestWhenHeadersEnabledViaPlaceholderThenResponseIncludesAllSecureHeaders FAILED
    org.springframework.beans.factory.xml.XmlBeanDefinitionStoreException at HttpHeadersConfigTests.java:102
        Caused by: org.xml.sax.SAXParseException at HttpHeadersConfigTests.java:102
org.springframework.security.config.http.HttpHeadersConfigTests > requestWhenHeadersDisabledViaPlaceholderThenResponseExcludesAllSecureHeaders FAILED
    org.springframework.beans.factory.xml.XmlBeanDefinitionStoreException at HttpHeadersConfigTests.java:89
        Caused by: org.xml.sax.SAXParseException at HttpHeadersConfigTests.java:89
org.springframework.security.config.http.HttpHeadersConfigTests > requestWhenHeadersDisabledRefMissingPlaceholderThenResponseIncludesAllSecureHeaders FAILED
    org.springframework.beans.factory.xml.XmlBeanDefinitionStoreException at HttpHeadersConfigTests.java:113
        Caused by: org.xml.sax.SAXParseException at HttpHeadersConfigTests.java:113
org.springframework.security.config.http.HttpHeadersConfigTests > requestWhenDefaultsDisabledWithPlaceholderTrueThenExcludesAllSecureHeaders FAILED
    org.springframework.beans.factory.xml.XmlBeanDefinitionStoreException at HttpHeadersConfigTests.java:186
        Caused by: org.xml.sax.SAXParseException at HttpHeadersConfigTests.java:186
org.springframework.security.config.http.HttpHeadersConfigTests > requestWhenDefaultsDisabledWithPlaceholderMissingThenIncludeAllSecureHeaders FAILED
    org.springframework.beans.factory.xml.XmlBeanDefinitionStoreException at HttpHeadersConfigTests.java:210
        Caused by: org.xml.sax.SAXParseException at HttpHeadersConfigTests.java:210
1211 tests completed, 6 failed
> Task :spring-security-config:test FAILED
> Task :spring-security-config:integrationTest
> Task :spring-security-config:jacocoTestReport
> Task :spring-security-config:rncToXsd

When I run the rncToXsd command locally, yes it does change all the target xsd files. I don't think there is an issue with that. Based on the build logs the issue seem to be the order in which the unit tests are run followed by the rncToXsd task. When the tests are run the xsd are not up-to-date.

Let's see how the build looks like this time 🤞
@jzheaux ^

[jzheaux]

Nice, @rhamedy! This looks great.

Just one more thing, I think. Can you explain the whitespace diff in the xsd file? Did your local build remove that whitespace, or maybe your editor?

I ask because, when I pull your branch onto my machine and run rncToXsd, it wants to change the whitespace back.

[rhamedy]

For some strange reason upon save in both eclipse and atom a whitespace in empty lines was removed. I rechecked out the xsd file from master, run the rncToXsd task and pushed it up and that did the trick.

I did come across a Git issue where it complained about trailing whitespace (not because of my changes but, how the xsds files are) and I have to use the --no-verify flag to force the commit to go through (locally). The git changes now look 👍

@jzheaux ^

[jzheaux]

Thanks, again, @rhamedy! This is now merged into master.