Make SecurityCheckStorage bean unremoveable

[geoand]

Fixes: #28732

[sberyozkin]

Hi @geoand, #28732 mentions that making the bean unremovable causes JAX-RS custom exception mappers for Forbidden and Unauthorized being ignored, I know @michalvavrik has added a few tests for the custom mappers, yeah, actually, I think Michal added the one for AuthenticationFailedException, but may be worth double checking

[michalvavrik]

There is already io.quarkus.resteasy.reactive.server.test.security.CustomExceptionMapperTest added by Georgios for Unauthorized that should do.

[geoand]

Now you folks confused me :).

Do we or do we not need to do anything else for this?

[michalvavrik]

The issue with reproducer is that RBAC annotations aren't inherited from interface, actual hello endpoint is on implementation GreetingResourceImpl and there they are not applied. I tried to add ServerExceptionMapper for UnauthorizedException and when you annotate implementation, it works. So, it's possible @davidfrickert misunderstood the situation (I am just guessing of course :-)).

[geoand]

That might be the case, but why is this bean not unremovable?

It seems very fishy to me that the proper functionality depends on the bean not being present...

[geoand]

And just to add to the previous comment, if what has been done here is problematic, why do no tests fail?

[davidfrickert]

The issue with reproducer is that RBAC annotations aren't inherited from interface, actual hello endpoint is on implementation GreetingResourceImpl and there they are not applied. I tried to add ServerExceptionMapper for UnauthorizedException and when you annotate implementation, it works. So, it's possible @davidfrickert misunderstood the situation (I am just guessing of course :-)).

I suspected this was the case and in practice to solve my problem I just eliminated the interface.

I opened this issue to understand why they aren't inherited. This error is also a bit obscure.

[michalvavrik]

@geoand EagerSecurityHandler is still applied on endpoints, request programmatically SecurityCheckStorage to find out whether there are security checks (they are not as interface is annotated, not impl.). However security interceptors are not applied (and they transitively inject SecurityCheckStorage, so the bean is not removed). Tests are not failing because there, there is not dichotomy between annotations on interface and endpoint impl. - to be sure, I'd have to create more tests.

[michalvavrik]

@davidfrickert agree. @geoand IMHO bean should be unremovable.

[sberyozkin]

Yeah, looks like @davidfrickert may not have done it correctly, as Michal pointed out as, indeed, as far as I recall, RBAC annotations are not inherited. I propose to wait a little bit for @davidfrickert to clarify, before we merge the fix which indeed looks good

[geoand]

There is no reason I can think of why a bean in Quarkus that is obtained programmatically by Quarkus itself would not be made unremoveable.

[sberyozkin]

Cool, so all is good

[geoand]

So yeah, I agree the usage was not correct to begin with, and this fix only oncovers the real issue

[davidfrickert]

Thanks for the fix @geoand.

@michalvavrik @sberyozkin - about the RBAC annotations, why are they not inherited to the class, is that a limitation or on purpose? In any case, should it not be documented in the Quarkus Security guides? Like this one - https://quarkus.io/guides/security-openid-connect - for example.

About my problem with the exception mappers, it seems to actually be related to quarkus.http.auth.policy + quarkus.http.auth.permission. It seems that when using these configs, custom exception mappers are not executed. Is this expected?

[geoand]

Thanks for the fix @geoand.

🙏🏼

[michalvavrik]

@michalvavrik @sberyozkin - about the RBAC annotations, why are they not inherited to the class, is that a limitation or on purpose? In any case, should it not be documented in the Quarkus Security guides? Like this one - https://quarkus.io/guides/security-openid-connect - for example.

@davidfrickert
It's Java EE annotation Specifying it on a method means that it is applicable to that method only. https://javaee.github.io/javaee-spec/javadocs/javax/annotation/security/RolesAllowed.html So to me it's clear, however I understand we can read it differently. I'd suggest you can create issue (or PR :-)) that request change of behavior or better docs. Could you kindly do that (you can mention me there)?

[michalvavrik]

ah @davidfrickert there is already one #22540

[michalvavrik]

#22530

[davidfrickert]

@michalvavrik Didn't check the annotation so that was my bad. But it's nice that there is already an issue and PR for it, thanks!