Add infrastructure for generic query augmentation [DATACMNS-293]

[spring-projects-issues]

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

[spring-projects-issues]

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)

[spring-projects-issues]

Zoltan Altfatter commented

Would be great to have this functionality

[spring-projects-issues]

Ian Duffy commented

Any ETA on this?

[spring-projects-issues]

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

[spring-projects-issues]

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

[spring-projects-issues]

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

}

[spring-projects-issues]

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:

• Grab the feature branch builds we have for the Spring Data JPA reference implementation for soft-deletes that builds on the foundation here. The version to pull in for Spring Data JPA is DATAJPA-307-SNAPSHOT, for Spring Data Commons refer to DATACMNS-293-SNAPSHOT.
• Have a look at JpaSoftDeleteQueryAugmentor and it's super types to get an impression of how to actually implement a QueryAugmentor.
• Provide feedback about the API or whether the stuff works for you at all, you'd like to see improvements etc

[spring-projects-issues]

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.

[spring-projects-issues]

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?

[spring-projects-issues]

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.

[spring-projects-issues]

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

[spring-projects-issues]

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?

[spring-projects-issues]

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

[spring-projects-issues]

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?

[spring-projects-issues]

Gazal Gaffoor commented

Any update on this issue? The code for DATAJPA-307 is ready, right? Is this issue pending on DATAMONGO-1151?

[spring-projects-issues]

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

[spring-projects-issues]

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:

• All findXxX(..) custom repository methods (in repository interfaces) must also be custom implemented, otherwise they will also return data without modification. For some reason, I do not know why, they must be annotated with '@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.
• By doing this, I am able to get very efficient data access and only required items are fetch from the DB as expected.
• This is specifically for a Hateoas use case, for other Repository uses, more methods (other than the 4) would of course need to be (manually) implemented too...

[spring-projects-issues]

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

[spring-projects-issues]

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 (spring-projects/spring-data-jpa#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... :)

[spring-projects-issues]

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?

[spring-projects-issues]

Connor Sadler commented

Is this still being worked on please? Thanks

[GrantGochnauer]

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

[dinvlad]

Multi-tenancy and role-based query filtering is the exact use case I'm thinking about as well. Any updates for this?

[koenbeckers]

Sure. I will add a comment for the yearly "Is this still being worked on?"

[arpanpreneur]

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.

[Ditscheridou]

Im facing currently the same problem as the guys before me, would be great to have this feature!

[heanssgen-troy]

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.

[oscarbrookssynacor]

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.

[oscarbrookssynacor]

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?

[heanssgen-troy]

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?

[heanssgen-troy]

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.