Spring-security: ReactorContextTestExecutionListener should use named hooks

[bsideup]

NOTE: This is marked for first time contributors only. If you want the issue, please comment to claim it. If you need any help with this issue, just ask on the ticket and we will be glad to help.

Summary

Currently, ReactorContextTestExecutionListener resets all previously set onLastOperator hooks:
https://github.com/spring-projects/spring-security/blob/dca3645850e082d845e17a6572af84aa44b97f12/test/src/main/java/org/springframework/security/test/context/support/ReactorContextTestExecutionListener.java#L66-L68
This might affect users' code, and it is recommended to use named hooks instead.

Actual Behavior

ReactorContextTestExecutionListener resets all pre-configured onLastOperator hooks after test method.

Expected Behavior

ReactorContextTestExecutionListener resets only the hooks it sets.

Version

5.1.1.RELEASE

[rwinch]

This would be fixed by registering it via

private static final String CONTEXT_OPERATOR_KEY = SecurityContext.class.getName();
...
Hooks.onLastOperator(CONTEXT_OPERATOR_KEY, Operators.lift((s, sub) -> new SecuritySubContext<>(sub, securityContext)));

Then it would get removed via

Hooks.resetOnLastOperator(CONTEXT_OPERATOR_KEY); 

We then want to ensure we add a test that validates if a different Hook was registered, then .afterTestMethod will not remove that different Hook.

[ankitthakur]

I think, it is working correctly as is expected, unless we would like to create some singleton api for test environment. Otherwise this approach might be useless on different environments with different db configurations, Like Linux with Oracle test cases could be different than With SqlServer. Whereas it is common across both platform for business logics.

[aotitoola]

Mine 😁🥳

[rwinch]

@ankitthakur I'm not sure I understand your comment at https://github.com/spring-projects/spring-security/issues/6075#issuecomment-439149451 This is an issue. The problem is that someone else might have registered a hook and Spring Security is deleting a hook it did not create.

[rwinch]

@daejii Thanks for volunteering! The issue is all yours. If you have any questions or need any help, please don't hesitate to ask! Thanks again!

[aotitoola]

Hello, @rwinch. I understand that we are trying to prevent Spring Security from deleting a hook it did not create. I have made the changes as regards registering and removing.

However, I would like to get some clarity on the test. Should I add a new test method in the ReactorContextTestExecutionListenerTests class, the test that validates if a different Hook was registered?

This is my first time contributing. As such, I would like to get the right context before making any changes. Thanks a lot for the opportunity to volunteer. I look forward to your response.

[rwinch]

@daejii Thanks again for volunteering to work on the issue.

Should I add a new test method in the ReactorContextTestExecutionListenerTests class, the test that validates if a different Hook was registered?

Yes. I'd add a new test method here

This is my first time contributing. As such, I would like to get the right context before making any changes. Thanks a lot for the opportunity to volunteer. I look forward to your response.

Not a problem at all. Thanks for volunteering your time to help Spring Security!

Please let me know if you have any additional questions.

PS: Even if things are as we want up front, if you submit a PR I'm happy to help you resolve anything that comes up. This means if you prefer you can submit a PR and then point to where you are having issues and we can work together until it resolves. We really want to be flexible in helping you because we understand you are volunteering your time to help the community. Thanks again!

[aotitoola]

@rwinch Thanks for your response. I'm happy to give up my time to do this.

I tried run this test, just to see how it behaves:

    @Test
    public void afterTestMethodWhenDifferentHookIsRegistered() throws Exception {

        this.listener.afterTestMethod(this.testContext);

    }

I got this error:

Error:(32, 41) java: package org.springframework.security.ldap does not exist
Error:(33, 56) java: package org.springframework.security.ldap.authentication does not exist
Error:(41, 48) java: package org.springframework.security.ldap.server does not exist
Error:(42, 53) java: package org.springframework.security.ldap.userdetails does not exist

Is there something I'm doing wrong? I will appreciate if you can point me in the right direction.
Also, I would like to know if a "beforeTestMethod" is necessary (I noticed some of the afterTest methods use a beforeTest method) ?

Thank you.

[rwinch]

@daejii Hmm...how are you running it? It appears that for some reason the classpath is not getting setup correctly. I'm guessing this is happening in your IDE? If so, what IDE and version are you using? Did you try updating your IDE? Does it happen if you run from the command line with ./gradlew test?

Also, I would like to know if a "beforeTestMethod" is necessary (I noticed some of the afterTest methods use a beforeTest method) ?

No. For this test it is not necessary. Many of the tests ran before just to ensure it was setup so we could verify after cleared things properly. Since this test is more about ensuring that we don't clear other customizations, we don't need to run the beforeTest method.

[aotitoola]

@rwinch I created a PR. #6122. Kindly review. Your feedback is important to me. Thanks for all the help.