Allow method execution only if the caller is of certain type

Question

We have an XRepository which extends JpaRepository. Since deleting X entities got pretty ad-hoc recently we created an XDeletionService which contains lots of... stuff :-) and uses the XRepository.

Now we have an interesting idea to forbid the execution of any delete methods in the XRepository unless these are called from within the XDeletionService. E.g. if a colleague calls directly by mistake XRepository.delete(..) from within TheirService it will throw an exception.

We still cannot find an elegant solution for this idea. What we did so far is to create an aspect with a pointcut expression which matches the delete methods of the repository. This aspect throws an exception by consulting the stacktrace, e.g.:

boolean calledFromXDeletionService = Arrays.stream(Thread.currentThread().getStackTrace())
        .anyMatch(stackTraceElement ->
            XDeletionService.class.getName().equals(stackTraceElement.getClassName())); 
if (!calledFromXDeletionService)
   throw new ....

This seems pretty ugly though. Do you have a better idea how to implement this "feature"?

How about setting a threadlocal variable within `XDeletionService` on any call to `XRepository.delete(..)` , intercept the calls to `XRepository.delete(..)` and throw exception on validating the variable ? — R.G, Aug 21 '20 at 07:29
Not an answer since it's not a better way to do what you're doing but a different solution to your problem: you can use ArchUnit to ensure, at testing time, that no class except XDeletionService ever uses the XRepository class. See https://www.archunit.org/userguide/html/000_Index.html#_class_dependency_checks. (Note: I haven't personally done this, and I'm not 100% sure what exactly ArchUnit means by "accessed".) It's not much prettier but it's outside the code you run in production, which I'd consider a benefit. — neofelis, Aug 21 '20 at 10:09
Pretty old question, but interesting :) On the first place, I think that it would cause more evil, because this way you are creating a silent rule. The code will throw an exception in the runtime. I other words, it's not "calling by accident". The method itself does not say I cannot call it. 1) Another runtime solution is to have condition for injecting repositories of type `DeletionRepository` via `FactoryBean` 2) Something more compile-time. Just ask in the delete method for an object of type `DeletionService` and call it like `repo.delete(id, this)` — Royal Bg, Jun 24 '21 at 07:27

kriegaex · Answer 1 · 2020-08-23T01:05:37.983

I suggest to switch from Spring AOP to AspectJ, which can be used with or completely without Spring. I am going to post a stand-alone Java example, but the Spring manual also explains how you can configure AspectJ LTW (load-time weaving) for Spring.

Sample classes + driver application:

package de.scrum_master.app;

public class NormalType {
  public void callTargetMethod(Application application) {
    System.out.println("Normal caller");
    application.doSomething();
  }
}

package de.scrum_master.app;

public class SpecialType {
  public void callTargetMethod(Application application) {
    System.out.println("Special caller");
    application.doSomething();
  }
}

package de.scrum_master.app;

public class Application {
  public static void main(String[] args) {
    Application application = new Application();
    application.callTargetMethod(application);
    callTargetMethodStatic(application);
    new NormalType().callTargetMethod(application);
    new SpecialType().callTargetMethod(application);
  }

  public void callTargetMethod(Application application) {
    System.out.println("Normal caller");
    application.doSomething();
  }

  public static void callTargetMethodStatic(Application application) {
    System.out.println("Static caller");
    application.doSomething();
  }

  public void doSomething() {
    System.out.println("Doing something");
  }
}

The expectation is that when running the little driver application, only the call to Application.doSomething() issued from within the instance method SpecialType.callTargetMethod(..) will actually be intercepted, not calls from other classes' instance methods and also not calls from static methods (which can be intercepted in AspectJ in contrast to Spring AOP).

The solution is to use a call() pointcut which is a kind of counterpart to execution() and unavailable in Spring AOP. It intercepts a method call inside the caller class, not the corresponding execution in the callee. This is what we want because then we can use this() in order to determine or narrow down the caller class.

Only for call() there is a difference between the values of this() (caller) and target() (callee). For execution() both values are the same, which is why Spring AOP cannot be used for this purpose without resorting to stack trace inspection or more elegantly and efficiently the Stack Walking API (https://www.baeldung.com/java-9-stackwalking-api) in Java 9+.

Aspect:

package de.scrum_master.aspect;

import org.aspectj.lang.JoinPoint;
import org.aspectj.lang.annotation.Aspect;
import org.aspectj.lang.annotation.Before;

@Aspect
public class MyAspect {
  @Before(
    "call(void de.scrum_master.app.Application.doSomething()) && " +
    "this(de.scrum_master.app.SpecialType)"
  )
  public void myAdvice(JoinPoint joinPoint) {
    System.out.println(joinPoint);
  }
}

Console log:

Normal caller
Doing something
Static caller
Doing something
Normal caller
Doing something
Special caller
call(void de.scrum_master.app.Application.doSomething())
Doing something

If you also want to log the caller instance, you can modify the aspect like this, binding it to an advice method parameter:

package de.scrum_master.aspect;

import org.aspectj.lang.JoinPoint;
import org.aspectj.lang.annotation.Aspect;
import org.aspectj.lang.annotation.Before;

import de.scrum_master.app.SpecialType;

@Aspect
public class MyAspect {
  @Before(
    "call(void de.scrum_master.app.Application.doSomething()) && " +
    "this(specialType)"
  )
  public void myAdvice(JoinPoint joinPoint, SpecialType specialType) {
    System.out.println(joinPoint + " -> " + specialType);
  }
}

The console log would then be:

Normal caller
Doing something
Static caller
Doing something
Normal caller
Doing something
Special caller
call(void de.scrum_master.app.Application.doSomething()) -> de.scrum_master.app.SpecialType@402a079c
Doing something

Update: You may also want to experiment with adding a JoinPoint.EnclosingStaticPart enclosingStaticPart parameter to your advice, then print and/or inspect it. It helps you find out more information about the caller for call() pointcuts without having to resort to stack traces or stack walking API.

I think it would be worth to mention the possibility to **declare an error** when `call(X+.delete(..)) && !this(DeletionService)`, which would shift the runtime exception to compile-time checking. — Hawk, Aug 23 '20 at 23:57
That was not my focus here, I was trying to show to the OP how with AspectJ he can create pointcuts to match what he was looking for during runtime. But I do agree with you that compile-time checks for this kind of issue are preferable to runtime checks. However, it is already a shift from Spring AOP to AspectJ and usually in Spring users apply load-time weaving, as I said in my answer. In a LTW scenario there is no compile-time check, though. For that the user needs to switch to compile-time weaving via AspectJ compiler (e.g. using AspectJ Maven plugin) first. — kriegaex, Aug 24 '20 at 02:13
Another problem with your suggestion is that in a `declare error` statement (native AspectJ syntax) or `@DeclareError` annotation (annotation style syntax) you cannot use pointcut designators like `this()` or `target()` but only pointcuts which can be determined statically during compile time, which for these two is not the case. Maybe it would make sense to test your ideas before posting. ;-) — kriegaex, Aug 24 '20 at 02:22
I am just throwing ideas, writing from my phone, they don't need to be 100% :D I rely on OP trying it out. Well if `this()` cannot be combined with `declare error`, I imagine `within()` looks like it could work with. — Hawk, Aug 24 '20 at 09:14

score 0 · Answer 2 · answered Aug 21 '20 at 07:02

0

The TheirService should use an Interface that doesn't have the delete methods, this is called the Interface Segregation Principle.

answered Aug 21 '20 at 07:02

Edgar Domingues

930
1
8
17

1

Of course, but the above question is still valid :-) – Lachezar Balev Aug 21 '20 at 07:05

Allow method execution only if the caller is of certain type

2 Answers2

Linked