Skip to content
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

Support Smallrye Config property expression expansion for @RolesAllowed value #29935

Conversation

michalvavrik
Copy link
Member

closes: #25245

Support SmallRye Config property expressions expansion for the @RolesAllowed annotation value.

@michalvavrik
Copy link
Member Author

/cc @vsevel @radcortez @sberyozkin

@michalvavrik michalvavrik changed the title Smallrye Config property expression expansion for @RolesAllowed values Support Smallrye Config property expression expansion for @RolesAllowed values Dec 18, 2022
@michalvavrik michalvavrik changed the title Support Smallrye Config property expression expansion for @RolesAllowed values Support Smallrye Config property expression expansion for @RolesAllowed value Dec 18, 2022
@quarkus-bot

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/roles-allowed-config-property-expression-lang branch 3 times, most recently from c0a3361 to 604755f Compare December 18, 2022 13:49
@quarkus-bot

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/roles-allowed-config-property-expression-lang branch 2 times, most recently from 9d1f113 to 8badc44 Compare December 19, 2022 13:58
Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

@michalvavrik This is a nice feature to have, thanks, please look at the last single comment.
Let @stuartwdouglas to have a quick look if he gets a chance, I'll merge tomorrow, Stuart, it can be tweaked as always if you notice something

@michalvavrik michalvavrik force-pushed the feature/roles-allowed-config-property-expression-lang branch from 8badc44 to 8974be6 Compare December 19, 2022 15:18
@quarkus-bot

This comment has been minimized.

Copy link
Member

@radcortez radcortez left a comment

Choose a reason for hiding this comment

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

Just a few thoughts:

Did you consider generating a configuration source with generated keys and values with the property expressions? The source could be registered with the Config system, and you would get all features available from Config and not only expansion.

Another idea could be to support configuration properties, and instead of the expansion, we do a complete config lookup (which, of course, also supports expansion).

@michalvavrik
Copy link
Member Author

Just a few thoughts:

Did you consider generating a configuration source with generated keys and values with the property expressions? The source could be registered with the Config system, and you would get all features available from Config and not only expansion.

Another idea could be to support configuration properties, and instead of the expansion, we do a complete config lookup (which, of course, also supports expansion).

I didn't think of that, I actually took identical approach as Scheduler extension, but I like that very much. @sberyozkin I think Roberto's suggestion is going to be less prone to (future) compatibility issues and and we get Smallrye Config improvements for free with next Smallrye Config versions. I'll have a look at this, please hold with merging till I investigate how to do it, it shouldn't take long. Thank you

@michalvavrik
Copy link
Member Author

Okay, I found a way, I'll do it tomorrow.

@sberyozkin
Copy link
Member

Thanks @radcortez, @michalvavrik Sure, take your time please.

@radcortez
Copy link
Member

Great!

Here are some specific ideas. Feel free to take them or discard them :)

@RolesAllowed("security.role")
void single() {
}

@RolesAllowed("security.roles")
void multiple() {
}

@RolesAllowed("security.expressiom")
void expression() {
}

@RolesAllowed("actualRole") // We cannot find in in Config, so we use the value as is
void noConfig() {
}
security.role=role
security.roles=role1,role2
#or
security.role[0]=role1
security.role[1]=role2
security.expression=role_${number}
number=1

@michalvavrik michalvavrik force-pushed the feature/roles-allowed-config-property-expression-lang branch from 8974be6 to c4360ff Compare December 20, 2022 15:58
@michalvavrik
Copy link
Member Author

generating a configuration source with generated keys and values with the property expressions

I used your suggestion to generate a configuration source with generated keys and values with the property expressions and also to use RunTimeConfigBuilderBuildItem. There was something that surprised me - RunTimeConfigBuilderBuildItem don't work properly with classes generated through GeneratedClassGizmoAdaptor (it leads to class not found ex), while StaticInitConfigSourceProviderBuildItem does. For that reason I decided to collect properties for config source at STATIC INIT, no blocker.

And I kept previous behavior - I don't consider all @RolesAllowed to be config properties. For one, I think that would be breaking change and also using config expressions is something that users know from EAP.

Thank you very much for help

@radcortez
Copy link
Member

radcortez commented Dec 20, 2022

There was something that surprised me - RunTimeConfigBuilderBuildItem don't work properly with classes generated through GeneratedClassGizmoAdaptor (it leads to class not found ex), while StaticInitConfigSourceProviderBuildItem does.

Yes, I'm aware of this. The issue is that the builders are expected to be available in the bootstrap classloader (the isApplicationClass flag in GeneratedClassGizmoAdaptor). I've already changed this in #29842 to use the context ClassLoader.

@sberyozkin sberyozkin merged commit 6faa967 into quarkusio:main Dec 21, 2022
@quarkus-bot quarkus-bot bot added this to the 2.16 - main milestone Dec 21, 2022
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Dec 21, 2022
@sberyozkin
Copy link
Member

Thanks Michal, Roberto

@michalvavrik michalvavrik deleted the feature/roles-allowed-config-property-expression-lang branch December 21, 2022 12:24
@geoand
Copy link
Contributor

geoand commented Jan 6, 2023

@sberyozkin @michalvavrik this seems to have broken main.

For example io.quarkus.it.resteasy.reactive.elytron.FruitResourceIT is currently failing with:

ERROR: Failed to start application (with profile [prod])
java.lang.ClassNotFoundException: io.quarkus.security.runtime.QuarkusSecurityRolesAllowedConfigBuilder
        at [email protected]/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:52)
        at [email protected]/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
        at [email protected]/java.lang.ClassLoader.loadClass(ClassLoader.java:132)
        at io.quarkus.runtime.configuration.ConfigUtils.configBuilder(ConfigUtils.java:215)
        at io.quarkus.runtime.generated.Config.readConfig(Unknown Source)
        at io.quarkus.deployment.steps.RuntimeConfigSetup.deploy(Unknown Source)
        at io.quarkus.runner.ApplicationImpl.doStart(Unknown Source)
        at io.quarkus.runtime.Application.start(Application.java:101)
        at io.quarkus.runtime.ApplicationLifecycleManager.run(ApplicationLifecycleManager.java:108)
        at io.quarkus.runtime.Quarkus.run(Quarkus.java:71)
        at io.quarkus.runtime.Quarkus.run(Quarkus.java:44)
        at io.quarkus.runtime.Quarkus.run(Quarkus.java:124)
        at io.quarkus.runner.GeneratedMain.main(Unknown Source)

@geoand
Copy link
Contributor

geoand commented Jan 6, 2023

#30220 should fix the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EL Placeholder support in @RolesAllowed annotation
5 participants