Open spring-projects-issues opened 11 years ago
Neale Upstone commented
This is exactly the issue I just came looking for.
Participating in the creation and/or execution of the queries for CrudRepository methods is one part of the equation, the other is being able to add argument resolvers (such as being able to extract values from the Spring Security principle, and expose them as a named parameters, such that we could use it in JQL queries)
Zoltan Altfatter commented
Would be great to have this functionality
Ian Duffy commented
Any ETA on this?
Pedro Vilaça commented
As this issue isn't in-progress yet, do you suggest anything as a workaround to add a filter to the queries for entities that has a specific annotation? we're working with spring security ACL and we're trying to find a way to do the filtering without need to write all the queries by hand
Brian Susko commented
I'd also be interested in any recommendations on this until such case this is implemented. It seems there should be a lower-level API we can tie into to add a dynamic criteria. In the end, it would be nice to get access to the entity type also, so we can check for things like "if typeof T, add this criterion with a base interface for all entities needed. Something like that. Just my $.02, thanks
Raghavendra Chary B commented
Currently we are using below as a workaround: https://github.com/charybr/spring-data-rest-acl.
Where we use CrudRepository so that Spring security ACL PostFilter can be applied:
@RepositoryRestResource(path = "book")
public interface BookRepository extends CrudRepository<Book, Long> {
@SuppressWarnings("unchecked")
@Override
@PreAuthorize("hasRole('ROLE_ADMIN') or hasPermission(#society, 'write')")
Book save(@P("book") Book book);
@Override
@PostFilter("hasPermission(filterObject, 'read') or hasPermission(filterObject, admin)")
Iterable<Book> findAll();
@PostFilter("hasPermission(filterObject, 'read') or hasPermission(filterObject, admin)")
List<Book> findByAuthor(@Param("author") String author);
}
Oliver Drotbohm commented
We'll be looking into this for the Gosling release train again. The biggest reason we haven't really gotten much farther is that we'd like to get more feedback on what we currently have. So everyone interested: there's actually a way to help us out with that:
DATAJPA-307-SNAPSHOT
, for Spring Data Commons refer to DATACMNS-293-SNAPSHOT
.JpaSoftDeleteQueryAugmentor
and it's super types to get an impression of how to actually implement a QueryAugmentor
.Jordi Llach Fernandez commented
After reviewing the changes done in sprin-data-commons and spring-data-jpa here are my thoughts.
As far as I've seen QueryAugmentor
's business logic is meant to be "ruled" by the values of its corresponding annotation, for instance QueryDslSoftDeleteQueryAugmentor
uses SoftDelete
, the same way as LockModeQueryAugmentor
uses Lock
I think that is approach is right as long as we have access to the entity type in the AnnotationBasedQueryAugmentor
's methods (prepareNativeQuery, prepareQuery, and prepareUpdate) through any kind of Context --> QueryContext
(QueryDslJpaQueryContext,JpaQueryContext,JpaCriteriaQueryContext) and UpdateContext
(QueryDslJpaUpdateContext,JpaUpdateContext).
Thus one could implement a custom augmentor that could modify the query by checking if an annotated field of the entity contains certain values.
Actually I've not reviewed all the QueryContext
or UpdateContext
implementations but some of them already let you access the repository interface which in turn holds the entity type.
Oliver Drotbohm commented
That's very useful, thanks. Indeed annotations will definitely play an important role in augmentation as they're a nice tool to declaratively define expected augmenting behavior. That said, as you can see from the fact that QueryAugmentor
doesn't say anything about annotations at all so an implementation that relies on more advanced configuration through a Spring Bean is possible, too.
The entity type is handed into the supports(…)
method and will be called before any kind of augmentation invocations. The primary use case here being that the repository infrastructure can detect the need for augmentation early and potentially avoid all kinds of checks and invocations for a type that doesn't need to be augmented at all. So for query augmentations you could decide whether to match or not early on. UpdateContext
actually even has the entity about to be updated, so that you could even consider special subtypes here.
Does that make sense?
Jordi Llach Fernandez commented
Actually I was planning to use AnnotationBasedQueryAugmentor
as a foundation class, but by looking at the implementation done in augmentQuery(...)
and augmentUpdate(...)
I think that these (unnecesary?) extra checks are already there, maybe I am missing something
Maybe QueryContext
could also hold a reference to the entity in order to ease of use.
Oliver Drotbohm commented
Just to make sure, we're on the same page here: AnnotationBasedQueryAugmentor
considers repository annotations on both the type level as well as on methods. So the annotation handling implemented in it guards against an annotation type defined in the corresponding sub-type
Jordi Llach Fernandez commented
My last comment about "(unnecesary?) extra checks" just tries to point out that IF supports(...)
method is called BEFORE (as you described yesterday) any augmentXXX(...)
method, those checks against the cache are useless, maybe I am missing something here, but to me they seem unnecesary
To make it clearer, the code I was expecting to find in AnnotationBasedQueryAugmentor
is something like
public final Q augmentQuery(Q context, MethodMetadata metadata) {
return prepareQuery(context, cache.get(metadata.getMethod()));
}
public final U augmentUpdate(U update, MethodMetadata metadata) {
return prepareUpdate(update, cache.get( metadata.getMethod() ));
}
About "AnnotationBasedQueryAugmentor considers repository annotations on both the type level as well as on methods" :
That's absolutely fine and I was considering to overwrite only prepareQuery(...)
and/or prepareUpdate(...)
, and inside these ones by using the received context (QueryContext
or UpdateContext
) try to inspect the bean type, retrieve custom annotations defined at FIELD level and augment the query as needed, thus only using annotations handled by AnnotationBasedQueryAugmentor
as a marker annotation to trigger the augmentor process. Does that make sense?
Oliver Drotbohm commented
The invocation of individual methods of the QueryAugmentor
are hidden behind QueryAugmentationEngine
. Clients might call augmentationNeeded(…)
defensively but don't necessarily have to. That's why we double check the annotation cache in the execution methods.
Also, keep in mind that the prepare…
methods actually take the annotation as argument. So the cache lookup is needed anyway. We're just applying some extra safety here
ken p commented
I haven't had the time to pull the aforementioned branch, so forgive me for asking prior to doing so, but will the JpaRespository#deleteInBatch(Iterable<T> entities)
as well the delete all in batch be augmented when appropriate?
Gazal Gaffoor commented
Any update on this issue? The code for DATAJPA-307 is ready, right? Is this issue pending on DATAMONGO-1151?
François Lecomte commented
Hi there. couldn't wait any longer for this to be fixed... so here's my proposal : a Spring Security extension with beans defining ACL strategies ; easy to plug with Post/PreAuthorize annotations, and able to inject ACL restrictions inside JPA queries (thx to Spring data JPA). noSQL databases are not yet supported, but that shouldn't hurt much. I'm interested in your feedback : https://github.com/lordlothar99/strategy-spring-security-acl
Terje Strand commented
As an alternative to François suggestion, I have been able to work around this issue by creating a custom implemented base repository class (i.e. use "@EnableJpaRepositories
(repositoryFactoryBeanClass = MyRepositoryFactoryBean.class)" on the configuration class, implement the factory JpaRepositoryFactoryBean and a new base repository (extend QueryDslJpaRepository).
By doing this, there are only 4 methods in QueryDslJpaRepository that needs to be extended/implemented in the base class: 'findAll(Pageable pageable)', 'findOne(ID id)', 'save(S entity)' and 'delete(ID id)'. The rest of the public methods I throw a 'not implemented exception'. I use QueryDsl to append predicates to queries to restrict the data set according to my data model.
Three notes:
@Query
' to get all the goodness of Hateoas treatment. A custom class per repository would then also be needed to implement these, which is very easy with DslQuery.Oliver Drotbohm commented
The problem unfortunately by far isn't that simple. If it was, we definitely wouldn't have postponed the inclusion of what we already have for so long.
Even François' solution has quite some significant security holes, which — especially when adding security features to an API — is kind of problematic. It might just work for him, but it's definitely not something we can actually ship with the framework. It's not even a bug in his implementation really but the way that JPA works: if you read an instance of an entity, it doesn't have to be explicitly saved again. The JPA provider might just flush back changes. This alone makes it pretty tricky as you need to use store specific mechanisms — i.e. EntityListener
instances — to add security checks again. Even if you do you're not really allowed to trigger additional queries that might check for permissions from such an EntityListener
. You can sort of work around that by tweaking the flush mode for the EntityManager
during that operations. But as it might become obvious by now, the more you get into the details, the more messy it gets. That's why we basically have limited our efforts on this one significantly.
That said, I think it's kind of dangerous to throw something together that covers the happy path for oneself, throw this out and create the impression it'd solve a much broader problem. Especially when it comes to security related topics
François Lecomte commented
yeah u r right, my implementation is not fully operational, I agree it can't be shipped that way, and actually it's not the purpose. I already use it anyway on several projects (with additional custom code in queries, and obviously a LOT of security-dedicated tests). I submitted a PR in order to ease injection of Predicate objects into queries created by spring data jpa (https://github.com/spring-projects/spring-data-jpa/pull/178). I totally agree this topic is very tricky... I'm very interested in your opinion on my implementation if you have time to give me feedback, in particular if you could point me the issues you saw... :)
Casey Link commented
We worked around this in a way similar to @terjestrand
.
We implemented our own base repository implementation from scratch (not relying on SimpleJpaRepository
). It also implements QueryDslPredicateExecutor
. We implement all standard query methods via querydsl (findOne, findAll, etc) regardless of the signature. Save/update/delete are implemented with EntityManager directly.
Then in this repository implementation we add a protected method like the following that is used by the createQuery
method.
// base implementation just passes the query through
protected Predicate augmentQuery(Predicate... predicate)
{
return ExpressionUtils.allOf(predicate);
}
Then we inherited from the base repository and implemented a new repository (along with a repository factory bean) that knows how/when to selectively augment the query based on the currently authenticated user.
This ensures all standard query methods can be wrapped by our access control system. There is no way for a developer to write a query that could accidentally bypass the access control (one of our main concerns).
It also plays nicely with spring data rest.
However there are two limitations:
findXX
methods that were automatically implemented by Spring are no longer possible, because there is no safe way to intercept and change them (hence this ticket!). We implement org.springframework.data.repository.query.QueryLookupStrategy
to throw an exception at boot time when a developer attempts to use them.org.springframework.data.jpa.repository.support.CrudMethodMetadataPostProcessor
is package private, which makes it impossible for our custom JpaRepositoryFactoryBean
to parse the @QueryHints
, @EntityGraph
, @Lock
annotations, which is not a dealbreaker but quite a shame. We'd rather not copy/paste that class into our codebase, so we just don't support those annotations at the moment.@OliverGierke
Would it be reasonable to request that org.springframework.data.jpa.repository.support.CrudMethodMetadataPostProcessor
be made public?
Connor Sadler commented
Is this still being worked on please? Thanks
I just stumbled upon this ticket as it's exactly what I was hoping was supported already out of box. Our use case, is that in a multi-tenancy system, we want to leverage authorization rules attached to roles that modify database queries - for example using the database to filter out documents that one might not have access to based on certain metadata values on those documents (and these fields we'd filter on are configurable by tenant).
Multi-tenancy and role-based query filtering is the exact use case I'm thinking about as well. Any updates for this?
Sure. I will add a comment for the yearly "Is this still being worked on?"
Leaving a comment here, so that I get notifications. I need this for a multi-tenancy use case in ReactiveMongoRepository. I like the spring boot magic of queries from interface method names, but shame that we cannot use it when doing multi-tenant.
Im facing currently the same problem as the guys before me, would be great to have this feature!
Same issue here, same use case. This is a blocker for using Spring ACL to its fullest capacity, as there is no effective way to query for domain objects that a user has access to without querying ACL itself for all ObjectIdentities, then making a second query for all objects in that list.
It seems that this issue is quiet old and has not seen any movement in a while, is there anything that we can do to make it more of a priority? I am happy to contribute if that will help get things moving. Seems like there are a number of interconnected issues across different spring projects that are not seeing any movement.
I reached out to the data-mongodb team about the above ticket, and they suggested that a common pre-execution hook with a defined api be added instead of making a method visible. Is this repository a proper place to add such an api?
Just checking back in for visibility here. The issue has been open for 11(!) years with no comment from the spring team. I am about to replace Spring ACL for a more supported library due to the lack of movement here, and I wanted to quickly see if the spring team has any plans on addressing this?
Just checking back in for visibility here. The issue has been open for 11(!) years with no comment from the spring team. I am about to replace Spring ACL for a more supported library due to the lack of movement here, and I wanted to quickly see if the spring team has any plans on addressing this?
I have begun the long process to learn an entire framework to address this. I cannot promise I have the skill or knowledge necessary to cover every use case, however it is my hope that I can address this and unblock a dozen use cases. Any help will be greatly appreciated.
Oliver Drotbohm opened DATACMNS-293 and commented
When working with repositories there might be use cases that involve the queries to be executed for that repository to contain some kind of global criteria. A good example for such a scenario is ACL based security to domain objects, e.g. with a global criteria causing the repository to only return domain objects created by the current user or only objects the current user is allowed to read.
A related example is working with data that has a soft-delete flag and essentially has to consider objects with that flag set to true non-existant.
Implementing these kinds of scenarios require the queries to the underlying store to be augmented with additional criterias. So we need to come up with some an SPI to prepare the query about to be executed and potentially even alter a
delete(…)
call into an update (for the soft-delete case).From a more general point of view we need access to the method the query is executed for and it's associated metadata
Issue Links:
SEC-2409 Spring Security / Spring Data Acl Integration ("is depended on by")
DATAMONGO-1151 Introduce ability to append queries against a particular collection ("is depended on by")
DATAJPA-307 Add support for soft deletes
DATAJPA-1551 Create an API to customize query creation and query processing
116 votes, 92 watchers