SecurityNamespaceHandler: update schema version to 5.5

[Emkas]

Closes gh-8974

Pull request is on 5.5.x branch since it cannot be cherry-picked to any other branch without modifications.

(My first small change in this project, so I will be grateful for all suggestions)

[Emkas]

I can provide the same change on main, but since recent changes I'm having troubles with project build (missing plugin "org.asciidoctor.jvm.convert")

[jzheaux]

Hi, @Emkas, thanks for the contribution!

Since this has come up a couple of times now, I wonder if there is a better way to do this so that it's not so tricky to remember.

At the very least, I think we should add a test to SecurityNamespaceHandler that confirms this behavior against the contents of spring.schemas. Better might be to have SecurityNamespaceHandler read spring.schemas to find the most recent schema version; then the code would refer to that instead.

Are you able to add one or both of these to this PR?

Additionally, please ensure a complete and passing build for your next push. It appears that there are some failing tests related to this PR.

[jzheaux]

Sorry to hear you are having problems with the build. I just checked our CI, and things appear to be working. I also pulled the latest locally, and I was able to build fine.

Can you share more about the error you are experiencing, and I'm happy to see where I can help.

[Emkas]

Hello, @jzheaux! Thank you for a fast response.

Very sorry for tests. I though I run ./gradlew build as mentioned in README, but I must messed something up, when I was trying to import whole project to Eclipse. My everyday work is with Eclipse and Maven, so I'm not very good with Gradle... Long story short I was trying to import project into eclipse and ended up with broken repo... So I cloned it once again, installed Intellij and now I can see I cannot split my change in two separated changes. I made change in InMemoryXmlApplicationContext on my first try, then I split commit in 2 and uploaded just one of them for the try. I thought I run gradle correctly, but I looks I was wrong. Once again sorry for that.

Now about additional tests... Well that is why I created an issue as my first try to this problem. So I wouldn't try to write tests in the project I don't know (from a development site) and wouldn't bother with the stuff like parsing highest version number etc. :-) But OK, I could try. Let say I read spring.schemas and find out that spring-security-X.Y.Z.xsd is the lastest XSD. What you want me to test from SecurityNamespaceHandler? Method matchesVersionInternal is private and part of much bigger parse method.

In the nearest feture I'll at least try to provide correct PR (I gotta read how to correct a PR now :-)).

[jzheaux]

Thanks for digging into all of this, @Emkas! I'm glad to hear you are making progress in spite of your difficulties.

There have been problems with getting Eclipse to build Spring Security in the past. If you find a specific issue, please consider logging a ticket so we can take a look.

What you want me to test from SecurityNamespaceHandler?

Take a look at SecurityNamespaceHandlerTests for some examples. Essentially, an InMemoryXmlApplicationContext is constructed, which in turn uses SecurityNamespaceHandler to parse the XML. You might consider changing InMemoryXmlApplicationContext#SPRING_SECURITY_VERSION to be a computed version. Then the existing SecurityNamespaceHandlerTests would fail if SecurityNamespaceHandler is not updated.

If you run into trouble with this approach, let me know, and I can help further.

[Emkas]

Damn, I'd have a longer sleep if I'd just wait for you to answer :D

I wrote SecurityNamespaceHandlerTest class and was trying to write some tests of SecurityNamespaceHandler from scratch. After about half on hours I found out that there already is a SecurityNamespaceHandlerTest class without missing 's'. After that I thought that the first thing should be writing a test to test well... a test. Which was to test if InMemoryXmlApplicationContext#SPRING_SECURITY_VERSION is set correctly:

@Test
public void InMemoryXmlWebApplicationContextRunsWithNewestXSDAsDefault() throws IOException {
	InMemoryXmlApplicationContext defaultContext = new InMemoryXmlApplicationContext("");
	String computedXSDVersion = getSpringVersionFromSpringSchemas();
	InMemoryXmlApplicationContext versionedContext = new InMemoryXmlApplicationContext("", computedXSDVersion,
			null);

	Resource defaultXML = Whitebox.getInternalState(defaultContext, "inMemoryXml");
	Resource versionedXML = Whitebox.getInternalState(versionedContext, "inMemoryXml");

	assertThat(defaultXML).isEqualTo(versionedXML)
			.withFailMessage("InMemoryXmlApplicationContext is not using a current XSD schema as a default");
}

Than I was trying to find out how to test it without WhiteBox and without getting direct access to the InMemoryXmlApplicationContext#SPRING_SECURITY_VERSION. Then I thought it's really time to sleep, so I turn off my PC and... read the message from you on my phone :-)

---

If you say that InMemoryXmlApplicationContext#SPRING_SECURITY_VERSION should be computed, I'll do that.

If I'd ammend a commit on forked repo, push it (with force), will this force a test retrigger here on this PR?

[Emkas]

Sorry to hear you are having problems with the build. I just checked our CI, and things appear to be working. I also pulled the latest locally, and I was able to build fine.

Can you share more about the error you are experiencing, and I'm happy to see where I can help.

On master/main I got errors from checkstyle from .js files... from doc. This is a fragment of the result:

Task :checkstyleNohttp
Execution optimizations have been disabled for task ':checkstyleNohttp' to ensure correctness due to the following reasons:
  - Gradle detected a problem with the following location: '/home/emkas/spring-security'. Reason: Task ':checkstyleNohttp' uses this output of task ':spring-security-config:rncToXsd' without declaring an explicit or implicit dependency. This can lead to incorrect results being produced, depending on what order the tasks are executed. Please refer to https://docs.gradle.org/7.2/userguide/validation_problems.html#implicit_dependency for more details about this problem.
[ant:checkstyle] [ERROR] /home/emkas/spring-security/docs/manual/build/api/jquery/external/jquery/jquery.js:595:13: http:// URLs are not allowed but got 'http://www.w3.org/TR/css3-selectors/'. Use https:// instead. [NoHttp]

(...)

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':checkstyleNohttp'.
> Checkstyle rule violations were found. See the report at: file:///home/emkas/spring-security/build/reports/checkstyleNohttp/nohttp.html
  Checkstyle files with violations: 14
  Checkstyle violations by severity: [error:84]

Maybe I'm doing something wrong. I'm starting with ./gradlew clean, then checkout on main, then publishToMavenLocal (this fails one time, second time checkstyle is not complaining), then build.

[jzheaux]

Then I thought it's really time to sleep, so I turn off my PC and... read the message from you on my phone :-)

Hey, thanks for sticking with it! I'm glad you found my message.

nohttp

I've seen that behavior before, too, that nohttp sometimes reports errors against files in the build folders. It's typically when I'm switching between branches where the file hierarchy differs, creating mismatched files in build. This doesn't quite sound like your case, so if you can find a deterministic way to reproduce it, please feel free to file a ticket.

The command that I usually run before updating a pull request is ./gradle format check.

[jzheaux]

If I'd ammend a commit on forked repo, push it (with force), will this force a test retrigger here on this PR?

Yes

[Emkas]

This time all tests are 👍

Looks like now I'll just wait for a code review. Tell me if this is what you wanted.

[Emkas]

@jzheaux Do you want the same change on main (with 5.6 instead of 5.5) or would you do it on your own inside your team?

And tell me please, is it OK to do some small organizational PR like fix broken javadoc links or typos in some small files (like in plugin configs).

[Emkas]

@jzheaux Do you want the same change on main (with 5.6 instead of 5.5) or would you do it on your own inside your team?

OK, I see that you've done some changes (split & getLast -> Pattern.match) so I'll assume that you'll merge this into main on you own :-)

[jzheaux]

Thanks, @Emkas, for the PR!

Yes, I did some polishing and I added one more test. Also, I've ported this over to 5.6.