Support security annotations on Jakarta Data repositories

[michalvavrik]

• closes: Jakarta Data repository methods cannot be secured with CDI security interceptors #48884

[quarkus-bot]

/cc @gsmet (hibernate-orm)

[github-actions]

🎊 PR Preview e25d7c6 has been successfully built and deployed to https://quarkus-pr-main-50987-preview.surge.sh/version/main/guides/

• Images of blog posts older than 3 months are not available.
• Newsletters older than 3 months are not available.

[sberyozkin]

This is a continuation of #50899, now applied specifically to Hibernate ORM, and tested thoroughly. LGTM

@yrodiere Please check as well

[yrodiere]

I couldn't (yet) look at the security part in details, but I assume @sberyozkin did that.

Here are a few comments on the Hibernate part.

And, agreed, that's a lot of testing, but I wouldn't have expected anything less 😁

[yrodiere]

Also, thanks a lot for working on this @michalvavrik :)

[michalvavrik]

@yrodiere I had a look at Hibernate Processor today, and I probably don't read that code 100 % right, but I think that "it is Hibernate repository" in following cases:

• it has Jakarta Data repository annotation https://github.com/hibernate/hibernate-orm/blob/018b8eeda3627e114ec25bd48407ccb9c47564ce/tooling/metamodel-generator/src/main/java/org/hibernate/processor/HibernateProcessor.java#L417
• it has HQL, SQL, FIND annotation https://github.com/hibernate/hibernate-orm/blob/018b8eeda3627e114ec25bd48407ccb9c47564ce/tooling/metamodel-generator/src/main/java/org/hibernate/processor/HibernateProcessor.java#L429
• it has any of following annotations https://github.com/hibernate/hibernate-orm/blob/018b8eeda3627e114ec25bd48407ccb9c47564ce/tooling/metamodel-generator/src/main/java/org/hibernate/processor/HibernateProcessor.java#L607

And again, maybe I just didn't find right sentence, but I am yet to find this behavior documented. So this is consistency nightmare, I cannot implement this in Quarkus reliably.

I can see hardcoded Quarkus stuff (like packages) in many places in the processor, including org.hibernate.processor.HibernateProcessor. I think org.hibernate.processor.annotation.AnnotationMetaEntity#inheritedAnnotations should just inherit jakarta.annotation.security and io.quarkus.security and that is it. But your solution is better for sure.

I'll close this as this is dead end.

[yrodiere]

• it has Jakarta Data repository annotation https://github.com/hibernate/hibernate-orm/blob/018b8eeda3627e114ec25bd48407ccb9c47564ce/tooling/metamodel-generator/src/main/java/org/hibernate/processor/HibernateProcessor.java#L417

• it has HQL, SQL, FIND annotation https://github.com/hibernate/hibernate-orm/blob/018b8eeda3627e114ec25bd48407ccb9c47564ce/tooling/metamodel-generator/src/main/java/org/hibernate/processor/HibernateProcessor.java#L429

Agreed, that seems to match what's documented in https://docs.hibernate.org/orm/7.1/repositories/html_single/

• it has any of following annotations https://github.com/hibernate/hibernate-orm/blob/018b8eeda3627e114ec25bd48407ccb9c47564ce/tooling/metamodel-generator/src/main/java/org/hibernate/processor/HibernateProcessor.java#L607

That seems related to entity mapping, which the same processor handles as well, so I think we can ignore that.

I'll close this as this is dead end.

Nooo! You were so close!

Could we possibly apply your solution to any class/interface that either:

• Is annotated with @Repository
• Has a method annotated with @HQL/@Find/SQL (we can hardcode this list, it should be fairly stable)
• Or has a superclass matching one of the conditions above

?

Do you need me to try to add that on top of your work? If so we can merge your work and add this later.

I can see hardcoded Quarkus stuff (like packages) in many places in the processor, including org.hibernate.processor.HibernateProcessor. I think org.hibernate.processor.annotation.AnnotationMetaEntity#inheritedAnnotations should just inherit jakarta.annotation.security and io.quarkus.security and that is it.

That... could actually work. We would just need to make sure we only do that in Quarkus applications, because the jakarta.annotation.security part would go against the Jakarta Data spec.

Also, maybe we could report that to the Jakarta Data spec and get some changes. Not sure it would address the Quarkus part, but the Jakarta Security annotations, maybe.

But your solution is better for sure.

My solution was attractive in principle, but as @FroMage pointed out it would not work consistently in all contexts -- in particular it wouldn't work in IDEs, it wouldn't work when putting the processor in the annotation processor path (only in the classpath), ... So it's pretty bad in fact, and yours is great in that it actually works :)

[michalvavrik]

thanks for reply @yrodiere

Could we possibly apply your solution to any class/interface that either:
Is annotated with @quarkusio/workshop-writers
Has a method annotated with @HQL/@Find/SQL (we can hardcode this list, it should be fairly stable)
Or has a superclass matching one of the conditions above

I can do that. I could take it even further and detect any annotation from Hibernate or Jakarta Data package (won't do that, just saying it would cover everything).

My only worry is if it will cover absolutely everything you change in the future, like new variant. I'd suggest that:

1. I'll do what you suggest.
2. We document which cases we support and mention that is the scope of support.
3. We print warning if we detect security annotations on interfaces that also have some Hibernate / Jakarta Data annotations and we do not secure them. It could mean some false positives, but it is better than someone who don't test security having unsecured repositories?

Ok, I'll go back to it later this week, thanks for support.

[yrodiere]

• I'll do what you suggest.

• We document which cases we support and mention that is the scope of support.

• We print warning if we detect security annotations on interfaces that also have some Hibernate / Jakarta Data annotations and we do not secure them. It could mean some false positives, but it is better than someone who don't test security having unsecured repositories?

IMO we can replace steps 2 and 3 with a simple test, similar to ClassNamesTest, where we check that all annotations in the org.hibernate.annotations.processing package are either covered by your code (like @HQL) or explicitly ignored in the test because they are irrelevant (like @Pattern). You could optionally only consider method-targeting annotations -- see ClassNamesTest#findRuntimeAnnotationsByTargetType.
That way, if we (@lucamolteni or I) upgrade to a new version of Hibernate ORM that adds new annotations, we'll see the failing test, and will update the list of hardcoded annotation names or the test as necessary.

EDIT: Of course you can still document that we support both Jakarta Data repositories and "plain" Hibernate Data repositories -- it's just that the precise definition of what a Hibernate Data repository is, IMO, is an implementation detail -- any uncovered Hibernate Data repository would be a bug.

On my end I'll also open an issue in the Jakarta Data spec to see if more annotations could be considered as inherited. That will take a few months/years to make its way to user applications though, so your work is still very much useful. Plus I think your previous PR covered other interfaces, not just Jakarta Data repositories, right?

[yrodiere]

On my end I'll also open an issue in the Jakarta Data spec to see if more annotations could be considered as inherited. That will take a few months/years to make its way to user applications though, so your work is still very much useful. Plus I think your previous PR covered other interfaces, not just Jakarta Data repositories, right?

Fingers crossed: jakartaee/data#1293

[michalvavrik]

There are merge conflicts on this branch, I'll resolve them and rebase. No further changes, (so @sberyozkin doesn't need to re-check, the ball is on @yrodiere side now).

[yrodiere]

the ball is on @yrodiere side now

Yes I didn't forget, just... it'll take time. Sorry.

[michalvavrik]

the ball is on @yrodiere side now

Yes I didn't forget, just... it'll take time. Sorry.

No problem, frankly that is why I mentioned you (my hidden agenda). I am fine waiting, but not knowing if a reviewer missed notifications keeps me thinking. My bad.

[FroMage]

I'll regret this, but I could review if you want?

[michalvavrik]

I'll regret this, but I could review if you want?

I definitely do want it. I know very little about Hibernate repositories.

[FroMage]

Look great, even though I am not sure why some people would secure repositories 🤷

[FroMage]

I'm not sure this is generics-safe.

[michalvavrik]

I'm not sure this is generics-safe.

I found in https://docs.hibernate.org/stable/orm/repositories/html_single/ section 3.2. Dynamic sorting some generic parameters (Sort<Book> sort and Order<Book> order and Order<? super Book> order). I'll add a test with them and if it is failing, I'll find generic-safe solution.

[michalvavrik]

@FroMage I have added io.quarkus.it.hibernate.processor.data.HibernateOrmDataSecurityTest#testSecuredMethodWithGenericMethodParams which tests following repository methods:

    @RolesAllowed("${donald}")
    @Find
    Stream<MyEntity> findAllForDonald(Order<MyEntity> order);

    @PermissionsAllowed("find-all")
    @PermissionsAllowed("find-for-donald")
    @Find
    Stream<MyEntity> findAllForDonald(@Pattern String name, Sort<MyEntity> sort);

    // public, unannotated
    @Find
    Stream<MyEntity> findAllForDonald(Sort<MyEntity> order, @Pattern String name);

and it works. If it stops working we will know thanks to the test.

[FroMage]

I don't mean that, I mean this:

interface TopClass<T> {
    @RolesAllowed("${donald}")
    @Find
    Stream<T> findAllForDonald(T order);
}
class BottomClass extends TopClass<Order> {
    @Overrides
    Stream<Order> findAllForDonald(Order order) { … }
}

Unless you go up the hierarchy while keeping track of what the generic parameters are, you can't compare the two methods's parameter types.

[FroMage]

This is the most tricky thing to do with generics: substitution. It's perfectly fine to leave this to a later PR, but we have to document this limitation.

[FroMage]

We have a lot of places where we implement this type substitution in Quarkus. It's even possible that we already have code that tells you if a MethodInfo overrides another one (doing substitution), so you don't have to do it yourself, though I am not sure if one exists myself.

Perhaps @mkouba @Ladicek or @geoand recall a util method? Jandex would be a good place to check if one MethodInfo overrides another, I suppose, but I could not find any such API at a glance.

We do have JandexUtil in Quarkus that we use for type substitution, but it is probably too low level for this.

[geoand]

All I remember is the various JandexUtil methods that resolve the type parameter. I don't remember any code that checks if a method is an override that also takes generic types into account

[Ladicek]

All places in Quarkus that try to determine overriding ignore one thing or another; to the best of my knowledge, none of them determine actual overriding per the JLS. Most of the times, visibility is ignored completely. I agree Jandex would be a good place for this kind of thing, but there's several variations that are needed and are different in subtle details, so I just keep postponing this :-) For example, you can have 2 methods that have the exact same signature and are present in a subclass and superclass and yet one does not override the other. We sometimes have to treat those two methods as "overriding" and sometimes we may not.

I just filed smallrye/jandex#625 for tracking this on the Jandex side.

[michalvavrik]

Thanks everyone.

I have tried to implement it today, but I wasn't sure I am able to cover every scenario, mostly because I don't know every combination. For example I didn't manage to write a Hibernate repository method with type variable on method level like <T> T findSomething..., it only worked for me on class level type variable. There are also wildcards, and something called UNRESOLVED_TYPE_VARIABLE and TYPE_VARIABLE_REFERENCE.

So if it is fine as @FroMage mentioned, I'll just fail validation if someone tries to use a security annotation with type variables and we can wait for feature requests. I just need to write a test and push it. I'll also document it.

[michalvavrik]

Done - I have added validation and documented this restriction.

cc @sberyozkin in case you want me to tweak the docs

[FroMage]

I'm not sure this is generics-safe.

[michalvavrik]

I have tried it, see #50987 (comment) for more info. Hope I didn't misunderstand you.

[michalvavrik]

I have tried it, see #50987 (comment) for more info. Hope I didn't misunderstand you.

It wasn't. Fixed by adding validation and documentation.

[michalvavrik]

Re CI failures: I have seen same failures in my other (unrelated) PR:

• Fix CDI request context activation race when Quarkus REST and Reactive Routes are used during the same request #51299 (comment)
• Fix CDI request context activation race when Quarkus REST and Reactive Routes are used during the same request #51299 (comment)

So they are not related.

[michalvavrik]

Hello @yrodiere , please put this on your review list when you can. I'd like to finish this and @FroMage provided a lot of useful feedback already, so maybe it won't be much left to review. Thank you.

[FroMage]

This is not very clear to me, because it's lacking an example of how I would do that.

[michalvavrik]

This is not very clear to me, because it's lacking an example of how I would do that.

You cannot test it, because I have added validation failure that fails the build if generic methods are detected. I am sorry, until I re-read this https://docs.oracle.com/javase/tutorial/extra/generics/methods.html again, a method is not called generic unless there is type variable like T. They are only referring to type parameters, so I also thought you could call any method with parameter like Order<MyEntity> generic.

I have dropped this sentence and only left information that generic methods are not supported yet.

[FroMage]

That's now what I meant. I meant that I'm lacking an example of how I would "ensure that generic interface methods are authorized as you intended".

How do I secure them, then?

[michalvavrik]

How do I secure them, then?

Simple util method would do. Let me add it an example. I'll ping you here to re-check it then.

[michalvavrik]

I have add an example.

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 1329209.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 1329209.

Failing Jobs

Status	Name	Step	Failures	Logs	Raw logs	Build scan
✔️	JVM Integration Tests - JDK 17			Logs	Raw logs	🚧
✔️	JVM Integration Tests - JDK 17 Windows			Logs	Raw logs	🚧
✔️	JVM Integration Tests - JDK 21			Logs	Raw logs	🚧
❌	JVM Integration Tests - JDK 21 Semeru	Build	Failures	Logs	Raw logs	🚧
✔️	JVM Integration Tests - JDK 25			Logs	Raw logs	🚧

You can consult the Develocity build scans.

Failures

⚙️ JVM Integration Tests - JDK 21 Semeru #

- Failing: integration-tests/observability-lgtm 

📦 integration-tests/observability-lgtm

❌ Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.5.4:test (default-test) on project quarkus-integration-test-observability-lgtm:

See /home/runner/_work/quarkus/quarkus/integration-tests/observability-lgtm/target/surefire-reports for the individual test results.
See dump files (if any exist) [date].dump, [date]-jvmRun[N].dump and [date].dumpstream.
There was an error in the forked process

---

Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/opentelemetry/deployment

❌ io.quarkus.opentelemetry.deployment.logs.LoggingFrameworkTest.testLog4jLogging - History

• Expected log to have body <ValueString{Log4j Logging message}> but was <ValueString{Installed features: [agroal, cdi, compose, hibernate-validator, jdbc-h2, narayana-jta, opentelemetry, reactive-routes, resteasy-client, security, smallrye-context-propagation, smallrye-graphql, smallrye-health, vertx]}> - org.opentest4j.AssertionFailedError

org.opentest4j.AssertionFailedError: Expected log to have body <ValueString{Log4j Logging message}> but was <ValueString{Installed features: [agroal, cdi, compose, hibernate-validator, jdbc-h2, narayana-jta, opentelemetry, reactive-routes, resteasy-client, security, smallrye-context-propagation, smallrye-graphql, smallrye-health, vertx]}>
	at io.quarkus.opentelemetry.deployment.logs.LoggingFrameworkTest.testLog4jLogging(LoggingFrameworkTest.java:95)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:532)
	at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:446)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)

---

⚙️ MicroProfile TCKs Tests

📦 tcks/microprofile-lra

❌ org.eclipse.microprofile.lra.tck.TckRecoveryTests.testCancelWhenParticipantIsUnavailable - History

• Expecting the metric Compensated callback was called Expected: a value equal to or greater than <1> but: <0> was less than <1> - java.lang.AssertionError

java.lang.AssertionError: 
Expecting the metric Compensated callback was called
Expected: a value equal to or greater than <1>
     but: <0> was less than <1>
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.eclipse.microprofile.lra.tck.TckRecoveryTests.assertMetricCallbackCalled(TckRecoveryTests.java:210)
	at org.eclipse.microprofile.lra.tck.TckRecoveryTests.testCancelWhenParticipantIsUnavailable(TckRecoveryTests.java:195)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

---

⚙️ JVM Integration Tests - JDK 17 Windows

📦 integration-tests/opentelemetry-minimal

❌ io.quarkus.it.opentelemetry.minimal.HelloServiceTest.testHello - History

• Expected: a value equal to or greater than <1> but: <0> was less than <1> - java.lang.AssertionError

java.lang.AssertionError: 

Expected: a value equal to or greater than <1>
     but: <0> was less than <1>
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
	at io.quarkus.it.opentelemetry.minimal.HelloServiceTest.testHello(HelloServiceTest.java:24)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)