Support security annotations on Jakarta Data repositories #50987
Conversation
michalvavrik
commented
Nov 12, 2025
michalvavrik
commented
- closes: Jakarta Data repository methods cannot be secured with CDI security interceptors #48884
quarkus-bot
bot
added
area/docstyle
|
/cc @gsmet (hibernate-orm) |
This comment has been minimized.
This comment has been minimized.
|
🎊 PR Preview 5047c51 has been successfully built and deployed to https://quarkus-pr-main-50987-preview.surge.sh/version/main/guides/
|
michalvavrik
force-pushed
the
backup/security-checks-for-jakarta-data-repos
branch
from
19b0af3 to
e1bf352
Compare
Status for workflow
|
This comment has been minimized.
This comment has been minimized.
Status for workflow
|
yrodiere
left a comment
yrodiere
left a comment
There was a problem hiding this comment.
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 😁
| } | ||
| ---- | ||
| <1> Only the methods directly declared on the `MyRepository` interface require authentication. Methods inherited from `CrudRepository`, such as the `insert` method, are not secured by the interface-level annotation. |
There was a problem hiding this comment.
While I understand why it is that way, this is essentially the opposite of "secure by default", so I believe it would deserve an admonition/warning?
Also, I wonder if we should open an issue to have @Autenticated apply to all methods, including inherited ones, by default... Excluding only methods from Object.
Intuitively I would expect the annotation to mean "this component requires authentication (so all methods, even inherited, do)" rather than "this interface requires authentication (so only methods declared here, and perhaps those in subinterfaces, do)"...
There was a problem hiding this comment.
I completely missed this comment @yrodiere , sorry.
This is how inheritance of security annotations work. There have been great many discussions about this. Here are few links that can be of interest if you are interested in this topic:
- RBAC Security doesn't work with super classes #6251 (comment)
- RolesAllowed annotations on interface are ignored #16840 (comment) (or better from the same issue RolesAllowed annotations on interface are ignored #16840 (comment) )
- Security Annotations Does not work on JaxRS resource interface #22530 (comment)
- JAX-RS default security is applied to annotated, inherited endpoints #38754 (comment) (this one is mine so you can skip that 😀 )
I found them up just in few minutes, but FWIW I remember other discussions and I'd need more time to remember / find them if you require them. I need to store this somewhere because it is never-ending topic.
While I understand why it is that way, this is essentially the opposite of "secure by default", so I believe it would deserve an admonition/warning?
We cannot read users mind as for what they want to secure. Only scenario where we can say for certain that they misapplied security annotation is when they apply security annotation on interface with no methods. In such a case, I could fail the build. I though about it during the implementation and skipped it (I suppose I thought it wasn't trivial to add, no idea why I skipped it though). Would you like me to add such validation? I think it makes sense.
Also, I wonder if we should open an issue to have @Autenticated apply to all methods, including inherited ones, by default... Excluding only methods from Object.
You can do that, I definitely only speak for myself and my opinions can be wrong. I won't open such issue for reasons mentions in provided links.
Intuitively I would expect the annotation to mean "this component requires authentication (so all methods, even inherited, do)" rather than "this interface requires authentication (so only methods declared here, and perhaps those in subinterfaces, do)"...
I'd prefer to stick to the way class-level security inheritance works in the rest of Quarkus with one exception that we allow interface here.
There was a problem hiding this comment.
FWIW I also implemented support (to the certain extent) of security annotations on some (not all) interfaces where they are JAX-RS endpoints. We do not document it (actually, we document users to do the opposite), but we needed to support that to stay backward compatible and compatible between RESTEasy Classic/Reactive. And there, I also only implemented it for direct class members. So much for my personal opinion...
There was a problem hiding this comment.
I don't mean I want that. I don't use these annotations personally.
I don't even mean this is relevant to this PR (though the fact that external interfaces such as CrudRepository may now need to be secured sort of makes the problem worse).
I just mean there is surprising default behavior that someone (@sberyozkin ?) should consider regarding documentation (warnings) and regarding their plans for how security annotations should work -- if not now, perhaps in an eventual Quarkus 4.
If you think this is all fine, I won't press further.
There was a problem hiding this comment.
That was no hint at you @yrodiere . I am not sure what is the right behavior, I have formed my opinions by both reading the specs and by trusting opinions of likes of Stuart, Ladicek etc.
I think it makes sense to cover the missing validation when there is a class-level security annotation an no explicitly declared direct methods. I'll implement it right now.
As for the rest, I'll leave it to discussion of others because there is plenty of my comments on this topic out there.
...-orm/deployment/src/main/java/io/quarkus/hibernate/orm/deployment/HibernateOrmProcessor.java
Show resolved
Hide resolved
| @BuildStep | ||
| public void registerJakartaDataRepositorySecurityAnnotations(Capabilities capabilities, | ||
| BuildProducer<SecuredInterfaceAnnotationBuildItem> securedInterfaceAnnotationProducer) { | ||
| if (capabilities.isPresent(Capability.SECURITY)) { | ||
| securedInterfaceAnnotationProducer | ||
| .produce(new SecuredInterfaceAnnotationBuildItem(DotName.createSimple(JAKARTA_DATA_REPOSITORY_ANNOTATION))); | ||
| } | ||
| } |
There was a problem hiding this comment.
Note this will only detect repositories annotated with @Repository... which I don't think is mandatory for repositories that only rely on Hibernate ORM (and its stateful session)?
@gavinking and @FroMage would know better than I...
There was a problem hiding this comment.
There is no test running without @Repository in Quarkus repository and documentation doesn't mention it either. I'll write a test and verify it is not required. If so, I am going to fix it, thanks for mentioning this fact.
There was a problem hiding this comment.
So I removed @Repository from https://github.com/quarkusio/quarkus/blob/main/integration-tests/hibernate-orm-data/src/main/java/io/quarkus/it/hibernate/processor/data/pudefault/MyRepository.java and application build fails. Is there some specific condition when @Repository is not required?
There was a problem hiding this comment.
I checked https://jakarta.ee/specifications/data/1.0/jakarta-data-1.0.pdf and it says A Jakarta Data repository is a Java interface annotated with @Repository.. I can fix it for it working without this annotation, but it will require more work. No biggie though.
There was a problem hiding this comment.
So I removed
@Repositoryfrom https://github.com/quarkusio/quarkus/blob/main/integration-tests/hibernate-orm-data/src/main/java/io/quarkus/it/hibernate/processor/data/pudefault/MyRepository.java and application build fails. Is there some specific condition when@Repositoryis not required?
I would have preferred Stéphane or Gavin to chime in because they know this much better than I do, but I can offer this repository definition from Hibernate ORM's own test suite:
It does not use Jakarta Data annotations, but Hibernate-specific ones, and it doesn't seem to require @Repository but the implementation still seems to be integrated with CDI in the end:
Now I suppose you could say these are not Jakarta Data repositories, but they are still Hibernate Data repositories and the line is pretty thin... and about to get even thinner in Panache 2.
I checked https://jakarta.ee/specifications/data/1.0/jakarta-data-1.0.pdf and it says
A Jakarta Data repository is a Java interface annotated with @Repository.. I can fix it for it working without this annotation, but it will require more work. No biggie though.
Right. As I said above, this is true for Jakarta Data but:
I don't think [it] is mandatory for repositories that only rely on Hibernate ORM (and its stateful session)?
There was a problem hiding this comment.
It does not use Jakarta Data annotations, but Hibernate-specific ones, and it doesn't seem to require @repository but the implementation still seems to be integrated with CDI in the end
https://github.com/hibernate/hibernate-orm/blob/018b8eeda3627e114ec25bd48407ccb9c47564ce/tooling/metamodel-generator/src/quarkusOrmPanache/java/org/hibernate/processor/test/ormPanache/QuarkusBookRepository.java#L12-L18
In that case I will inspect Hibernate tests and try to infer what is supported. I tried to quickly find it in docs, but I am not expert on Hibernate. I checked examples in https://docs.hibernate.org/stable/orm/repositories/html_single/ and not a single one is missing the @Repository annotation, so I missed this test. Thanks for pointing it out.
I will try to figure supported scenarios and implement them. Making this PR draft until then.
|
Also, thanks a lot for working on this @michalvavrik :) |
3 participants
