OpenAPI: enable auto security filter for auth policy via configuration #36460
Conversation
Signed-off-by: Michael Edgar <[email protected]>
|
@phillip-kruger PTAL If this meets the criteria for something that could be backported to 3.2, I'd be happy to do it 😄 |
| @@ -222,15 +223,15 @@ void registerAutoSecurityFilter(BuildProducer<SyntheticBeanBuildItem> syntheticB | |||
| SmallRyeOpenApiConfig openApiConfig, | |||
| OpenApiFilteredIndexViewBuildItem apiFilteredIndexViewBuildItem, | |||
| List<SecurityInformationBuildItem> securityInformationBuildItems, | |||
| OpenApiRecorder recorder) { | |||
| OpenApiRecorder recorder, | |||
| HttpBuildTimeConfig httpConfig) { | |||
There was a problem hiding this comment.
I wonder if it can complicate a planned migration of this configuration to Runtime ? CC @michalvavrik
There was a problem hiding this comment.
There will always need to be at least one build time property that will allow to you decide, if HTTP permissions are used not, for we need to decide if we produce beans (sure, we could also keep them there, in which case these changes are blocker). I'll comment below as well.
There was a problem hiding this comment.
Hmm, but we produce it now anyway, so no build time properties needed, sorry.
...eployment/src/main/java/io/quarkus/smallrye/openapi/deployment/SmallRyeOpenApiProcessor.java
Show resolved
Hide resolved
|
@MikeEdgar LGTM - I'll let @sberyozkin approve. W.r.t backport, the only thing you need to do is add the backport label |
| OASFilter autoSecurityFilter = null; | ||
| if (openApiConfig.autoAddSecurity) { | ||
|
|
||
| if (openApiConfig.autoAddSecurity |
There was a problem hiding this comment.
Now you are able to enable security with io.quarkus.smallrye.openapi.common.deployment.SmallRyeOpenApiConfig#autoAddSecurity and including security extension and either use RolessAlowed-Authenticated or HTTP permissions to have security enabled, correct?
I wonder what happens when user uses other HTTP Security Policies like Keycloak Authorization or has their own one because they implemented global policy due to our limited path matching capabilities? Users will have to specify securityScheme when they want it generated, rather then it being detected?
I can see why this is an improvement, but it will block moving HTTP permissions to runtime. Isn't that rather rare to have security extension added (you need to add it explicitly, right?) without securing anything? Why can't you simply add it always when security extension is present and if user don't want to add these schemes, they will set auto-add-security to false.
Also, the config property description says nothing about actually securing endpoints, therefore my proposed change would conform description This will automatically add security based on the security extension included (if any).. If you don't agree, please change the description.
There was a problem hiding this comment.
Now you are able to enable security with
io.quarkus.smallrye.openapi.common.deployment.SmallRyeOpenApiConfig#autoAddSecurityand including security extension and either use RolessAlowed-Authenticated or HTTP permissions to have security enabled, correct?
@michalvavrik , that's correct. For your question on other policies, I think it depends on what HttpBuildTimeConfig and the SecurityInformationBuildItem look like for other/custom security policies. I'm thinking that the answer is what you said - the user would need to specify their own securityScheme and this approach only works when SecurityInformationBuildItem has one of the known SecurityModels.
There was a problem hiding this comment.
Thanks for reply @MikeEdgar. I don't see any good reason why scheme shouldn't be generated when the security extension is present and SecurityInformationBuildItem is present as well. Just because someone doesn't use HTTP permissions and RBAC annotations, it doesn't mean they don't do security. Is there a point why you want to check HTTP permissions explicitly?
There was a problem hiding this comment.
Is there a point why you want to check HTTP permissions explicitly?
Only because it's what I needed for another project, but I think some more general approach would be better if it's possible.
There was a problem hiding this comment.
Well, it's not a deal break, I suppose we can still migrate HTTP permissions and just add some flag for OpenAPI, but I'd appreciate if it was possible to not use HTTP permissions during the build time unless you need to. Whatever works for you, thanks.
There was a problem hiding this comment.
No objections there :-) I simply relied on docs in this for that's as much as I know about this thing.
There was a problem hiding this comment.
Maybe we should fix the configuration property description then :)
I'm not following why this would be changed. If there are multiple ways to configure the security - annotations, configuration, KC authorizer, etc. - why wouldn't it be preferable to use auto-add-security=false to disable the automatic security when not desired?
There was a problem hiding this comment.
Auto security adds security (in OpenAPI) when nothing is configured in OpenAPI (annotations, configuration) but only if
- Security extensions are added
- Other (security) annotations and configuration are detected.
The second part are not in the description ....
There was a problem hiding this comment.
Ok, got it 👍. I think @michalvavrik's suggestion earlier was to remove the second part from the extension's code and allow users not wanting security to disable it using false. The issue I see is that if we require annotations, there are other possible ways to setup security that aren't supported.
There was a problem hiding this comment.
Also remember, auto security for OpenAPI only run if there is not OpenAPI Security annotations. The user can always still add their own OpenAPI Security annotation when it's a special case.
|
FYI @MikeEdgar permissions are now runtime properties #36874 |
|
Closing in favor of #38231 that does not depend on HTTP policy configurations. |
Allow the OpenAPI to have auto-added security scheme when an enabled HTTP auth policy is present. Currently, it requires auth to be enabled via
@RolesAllowedor@Authenticated.