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 :-) )

[rmanibus]

any update about this @sberyozkin @rsvoboda ? it is conflicted again, are you still planning to merge it ?

[rsvoboda]

Some security tests aare failing, could be related

@sberyozkin could you please take a look at the proposed changes and decide?

[rmanibus]

just rebased this

[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?