Skip to content

Mark methodSecurityMetadataSource as infrastructure bean #9860

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 9, 2021

Conversation

dodgex
Copy link
Contributor

@dodgex dodgex commented Jun 3, 2021

This PR adds the infrastructure role to the methodSecurityMetadataSource bean.

As there is another bean that is logged as non eligible for post processing this is only a partial fix for #9845. Unfortunately I have not yet found a way to fix this. I will continue the discussion about the missing bean in the issue.

@pivotal-cla
Copy link

@dodgex 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-cla
Copy link

@dodgex Thank you for signing the Contributor License Agreement!

@eleftherias eleftherias added in: config An issue in spring-security-config type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 3, 2021
Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @dodgex! I've left some feedback inline. Additionally, once you squash, please ensure that the commit message follows the formatting guidelines.

@@ -174,6 +174,8 @@ public void afterSingletonsInstantiated() {
if (grantedAuthorityDefaults != null) {
this.defaultMethodExpressionHandler.setDefaultRolePrefix(grantedAuthorityDefaults.getRolePrefix());
}

this.defaultMethodExpressionHandler = objectPostProcessor.postProcess(this.defaultMethodExpressionHandler);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, there would be a test to show that a BeanPostProcessor can modify the instance. Can you add one in GlobalMethodSecurityConfigurationTests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to do that, but I have two problems here. First I can't even run the current test.

spring-security>gradlew :spring-security-config:test --tests "org.springframework.security.config.annotation.method.configuration.GlobalMethodSecurityConfigurationTests"
Could not write standard input to Gradle Test Executor 6.
java.io.IOException: Die Pipe wird gerade geschlossen
        at java.base/java.io.FileOutputStream.writeBytes(Native Method)
        at java.base/java.io.FileOutputStream.write(FileOutputStream.java:347)
        at java.base/java.io.BufferedOutputStream.flushBuffer(BufferedOutputStream.java:81)
        at java.base/java.io.BufferedOutputStream.flush(BufferedOutputStream.java:142)
        at org.gradle.process.internal.streams.ExecOutputHandleRunner.forwardContent(ExecOutputHandleRunner.java:68)
        at org.gradle.process.internal.streams.ExecOutputHandleRunner.run(ExecOutputHandleRunner.java:53)
        at org.gradle.internal.concurrent.ExecutorPolicy$CatchAndRecordFailures.onExecute(ExecutorPolicy.java:64)
        at org.gradle.internal.concurrent.ManagedExecutorImpl$1.run(ManagedExecutorImpl.java:48)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1130)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:630)
        at org.gradle.internal.concurrent.ThreadFactoryImpl$ManagedThreadRunnable.run(ThreadFactoryImpl.java:56)
        at java.base/java.lang.Thread.run(Thread.java:832)

> Task :spring-security-config:test FAILED
Exception in thread "main" java.lang.reflect.InvocationTargetException
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:64)
        at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.base/java.lang.reflect.Method.invoke(Method.java:564)
        at java.instrument/sun.instrument.InstrumentationImpl.loadClassAndStartAgent(InstrumentationImpl.java:513)
        at java.instrument/sun.instrument.InstrumentationImpl.loadClassAndCallPremain(InstrumentationImpl.java:525)
Caused by: java.lang.RuntimeException: Class java/lang/UnknownError could not be instrumented.
        at org.jacoco.agent.rt.internal_28bab1d.core.runtime.ModifiedSystemClassRuntime.createFor(ModifiedSystemClassRuntime.java:140)
        at org.jacoco.agent.rt.internal_28bab1d.core.runtime.ModifiedSystemClassRuntime.createFor(ModifiedSystemClassRuntime.java:101)
        at org.jacoco.agent.rt.internal_28bab1d.PreMain.createRuntime(PreMain.java:55)
        at org.jacoco.agent.rt.internal_28bab1d.PreMain.premain(PreMain.java:47)
        ... 6 more
Caused by: java.lang.NoSuchFieldException: $jacocoAccess
        at java.base/java.lang.Class.getField(Class.java:2092)
        at org.jacoco.agent.rt.internal_28bab1d.core.runtime.ModifiedSystemClassRuntime.createFor(ModifiedSystemClassRuntime.java:138)
        ... 9 more
*** java.lang.instrument ASSERTION FAILED ***: "result" with message agent load/premain call failed at ./open/src/java.instrument/share/native/libinstrument/JPLISAgent.c line: 422
FATAL ERROR in native method: processing of -javaagent failed, processJavaStart failed

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':spring-security-config:test'.
> Process 'Gradle Test Executor 6' finished with non-zero exit value 1

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. Run with --scan to get full insights.

* Get more help at https://help.gradle.org

Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/6.9/userguide/command_line_interface.html#sec:command_line_warnings

BUILD FAILED in 9s
67 actionable tasks: 1 executed, 66 up-to-date

A build scan was not published as you have not authenticated with server 'ge.spring.io'.

This error comes when I try to run the GlobalMethodSecurityConfigurationTests in IntelliJ as well as when I try to run the gradle command gradlew :spring-security-config:test --tests "org.springframework.security.config.annotation.method.configuration.GlobalMethodSecurityConfigurationTests" to execute the test.

And even after I would get the tests to run, I'm honestly not sure what I could do to write a reliable test for this case. I never had much points of contact with BeanPostProcessor. So I hope you or someone from the team could somehow contribute to this with a test. Or at least some concrete hints in that regard.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can help out with a test.

Also, though, let's see if we can get your build into a working state. I've seen this happen before on JDK versions. If you are able, I'd recommend running with JDK 11 - I don't think that Spring Security's build has been tested against higher versions like 15 and 16. I'll ping the team, too, to see if they've seen that build error before in other ways.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I downloaded JDK11 and tested the build. When adding org.gradle.java.home to .gradle/gradle.properties in my home it works with the gradlew command mentioned earlier. But IntelliJ seems to ignore that. Even when adding the path with -Dorg.gradle.java.home to the generated run config.

trying to run ./gradlew clean build integrationTest still fails lots of the convention tests with NPEs or UnexptectedBuildFailures. I disabled them and now the other tests run successfully (with the command).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My experience with IntelliJ is that I can go to Project Structure to set the Project SDK. When the project structure window comes up, it's under Project on the left menu. Is that pointing to JDK 11?

Regarding the command line, I think that Gradle looks at your JAVA_HOME environment variable. You could see what that is set to and then you can possibly remove the org.gradle.java.home setting.

I'm not sure about the NPEs in the convention tests as I haven't seen that before. You might consider posting a question to StackOverflow and sharing the link. We can dig into that one further over there.

@dodgex dodgex force-pushed the gh-9845 branch 2 times, most recently from a8e019f to 6162417 Compare June 8, 2021 08:01
@dodgex
Copy link
Contributor Author

dodgex commented Jun 8, 2021

I squashed the commits and updated the commit message, I hope it is ok. Also I rebased the PR onto main branch

Added missing infrastructure role to methodSecurityMetadataSource bean
and move the post processing of the defaultMethodExpressionHandler to
the end of afterSingletonsInstantiated.

Closes spring-projectsgh-9845
@dodgex
Copy link
Contributor Author

dodgex commented Jun 9, 2021

I updated the PR with the missing this. that broke the check build

@jzheaux jzheaux merged commit 7a233c4 into spring-projects:main Jun 9, 2021
@jzheaux jzheaux added this to the 5.6.0-M1 milestone Jun 9, 2021
@jzheaux
Copy link
Contributor

jzheaux commented Jun 9, 2021

Thanks, @dodgex! This is now merged into main. I also added a polish commit with a test in 5b49433

@dodgex
Copy link
Contributor Author

dodgex commented Jun 9, 2021

@jzheaux Awesome!

I'm always happy to make awesome stuff a bit more awesome, even if it is just some minor change. :)

Btw. while checking your commit I realized that the test class still has copyright header of 2019. :D

@jzheaux
Copy link
Contributor

jzheaux commented Jun 9, 2021

Thanks, the copyright header is updated now.

@dodgex dodgex deleted the gh-9845 branch June 10, 2021 06:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: config An issue in spring-security-config type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants