Make Security annotations work on interfaces

[rmanibus]

Fix #22530

[quarkus-bot]

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 0649a92

Status	Name	Step	Failures	Logs	Raw logs
✖	Initial JDK 11 Build	Build	Failures	Logs	Raw logs

Failures

⚙️ Initial JDK 11 Build #

- Failing: extensions/security/deployment 
! Skipped: core/test-extension/deployment devtools/bom-descriptor-json docs and 309 more

📦 extensions/security/deployment

✖ Failed to execute goal net.revelc.code:impsort-maven-plugin:1.6.2:check (check-imports) on project quarkus-security-deployment: Imports are not sorted in /home/runner/work/quarkus/quarkus/extensions/security/deployment/src/test/java/io/quarkus/security/test/cdi/CDIAccessDenyUnannotatedTest.java

[famod]

FTR, crosslink: #22530 (comment)

[geoand]

I am not sure about this TBH given what @famod posted

[rmanibus]

It's still a step toward the second point: harmonize bean interface annotation handling across features. And it's not enabled by default !

[geoand]

I'll leave it up to @sberyozkin and @michalszynkiewicz

[sberyozkin]

I'm off to the airport in an hour or so, so I'll comment when I'm back; also CC @stuartwdouglas

[rmanibus]

hello @sberyozkin, any news on that topic ?

[gsmet]

@mkouba I think your experience with CDI annotations and inheritance rules could be useful here.

[gsmet]

@sberyozkin I agree it would be nice to have @stuartwdouglas 's opinion on this. I'm assigning this to you but it's really so that you coordinate the work on this. Thanks!

[sberyozkin]

@gsmet Thanks, I definitely recall Stuart having some reservations, I'll try to find where he commented as well

[sberyozkin]

Here is another issue, #16840. I see some CDI related concerns are raised there, Stuart does not have objections, @mkouba and @manovotn, @Ladicek, @michalszynkiewicz, can you comment on this PR please

[manovotn]

Hm, do we really want a CDI-breaking feature like this? Personally I am -1 on this, at least in default settings (but then again, if not default, who's gonna use it :-/)

BTW if we want to implement it, this approach is fine but we could also just wire it directly into Arc, i.e. make it 'scan' interfaces for annotations.

[Ladicek]

My personal opinion is that JAX-RS screwed up by ignoring Common Annotations (though they probably had good reasons), and now we have to live with it. If so, I'd say it would be better to do it consistently in the CDI implementation, and not drive it by configuration, but by code. (Something like class MyResource implements @InheritCdiAnnotations MyJaxrsInterface -- yes I've recently learned too much about type annotations :-) )

[sberyozkin]

@rmanibus @rsvoboda Apologies for a delay, we are talking this very issue right now with the team (though as part of a different issue's discussion), let me CC @michalvavrik for now

[sberyozkin]

@rmanibus Can you please give us a favor and check if it works as expected with the latest Quarkus release, for example, 3.9.3, and if it does not, please show the test hierarchy which does not work (interface and implementation class)

[rmanibus]

@stuartwdouglas I was missing a null check, only one test is failing now:

[ERROR] io.quarkus.resteasy.reactive.server.test.security.DenyAllJaxRsTest.shouldDenyUnannotatedOverriddenOnInterfaceImplementor -- Time elapsed: 0.006 s <<< FAILURE!
java.lang.AssertionError: 
1 expectation failed.
Expected status code <403> but was <200>.

I am wondering if we should just remove this test. or at least we should execute it with this feature deactivated

[rmanibus]

Is there a way I could trigger the CI myself ?

[rmanibus]

@sberyozkin I disabled this feature for the failing test, please tell me if it's ok for you. Build is passing now

[rmanibus]

@sberyozkin following up with this one

[rmanibus]

@sberyozkin are you still interested in merging this ?

[sberyozkin]

@rmanibus Sorry for keeping missing your pings, if I don't reply, ping me anytime on Zulip.

As far as this and similar issues are concerned, we've been analyzing and looking into them. It is hard to have a consistent treatment of this issue across REST and Resteasy stacks, but the effort is underway, it can take time but sooner or later we'll get the framework for supporting such cases in shape

[cescoffier]

@sberyozkin What should we do here?

[cescoffier]

@sberyozkin ping?

[yrodiere]

FWIW not having this merged is making it impossible to use security features on Jakarta Data repositories at the moment. See #48884

[michalvavrik]

FWIW not having this merged is making it impossible to use security features on Jakarta Data repositories at the moment. See #48884

Issue with this PR is that only subset of security (annotation) checks are actually performed by CDI as it causes issues and we need to perform most of the checks before actual method invocation, so that checks stays async and runs very early. I never said we shouldn't merge this after rework, but we need to define and document expected behavior, you can find more information in the linked issue and other discussions linked to this.

Plus this PR description says it fixes #22530 which it does not, that's not how security checks work.

[michalvavrik]

@rmanibus I think the easiest solution that doesn't bring that much extra work to what you actually have is that you use annotation store. I remember using the Arc annotation store brought some circular dependency issues, which is why we can't reflect the result of annotation transformations everywhere (just as a fallback). I am not sure if it is still true, we changed a lot of things. If it is, we would have to define our annotation store for security and that one you can enhance with your transformations as well.

[rmanibus]

Hey, sorry this was 4 years ago it's a bit rusty in my head, but isn't using the annotation store exactly what I did here ?
I mean the only thing this PR is doing is using an annotation transformer to virtually declare an annotation on the class of an annotated interface

[rmanibus]

I think I understand what you want, I will try to look into it a bit more in depth

[michalvavrik]

I mean the only thing this PR is doing is using an annotation transformer to virtually declare an annotation on the class of an annotated interface

So that would be true if everything that setup security could rely on Arc annotation store, which it doesn't and cannot. One example - wherever we use io.quarkus.security.spi.SecurityTransformerUtils and io.quarkus.vertx.http.deployment.HttpSecurityUtils would give wrong answers. I think I tried and using Arc annotation stores brings too many circular dependencies, I'd not go there.

I think I understand what you want, I will try to look into it a bit more in depth

Sure, thanks. Please when you have time, check the linked issue. I left there some context. At the very least, this would require extensive test coverage, but I don't want to repeat everything I wrote elsewhere.

[cescoffier]

@rmanibus any update?