To introduce my question, I have a few service methods that I use for creating new entities and modifying existing entities, as there is often side effect work that needs to be done at the same time. As a side note, I’ll look into entity listeners and the like in the future, but for now, at least, this is how I’ve been doing it.
When I make a service call, some parameters are entities themselves. Sometimes I pass the exact entity I want to make changes to, and sometimes I pass a related entity from which to gather information or with which to associate the new entity.
However, I’m noticing that these entities can be edited on the client by a malicious user before the service method is called, potentially resulting in associations or entity data that should just not occur.
In addition, the entity passed may not contain the data that would exist if the entity were obtained in the service method itself (not passed in a parameter).
Is there a standard way to ensure that the entity being passed is a replicate of the managed version of that entity?
I imagine it would be useful to allow changes to be made to the entity before passing it to a service, but it would also be nice to be able to ensure that no changes have occurred.
Two workaround options I’ve considered are to pass the ID of the entity as a parameter instead of the entity itself, or to pass the entity and replace it in the service method by calling entityManager.find() with the ID, but both of these options seem a bit unwieldy - the latter being more so than the former.
Hi
I can share a bit of my own code here, taken from an updateProfile method I implemented for a UserProfileService I wrote (it lets a remote logged in user to change its own properties and update the UserSession in the cluster with the updated User entity)
the method is annotated with @Validated, for JSR Bean Validation to be applied on method parameters, like so:
@Validated
void updateProfile(@NotNull User user) throws ValidationException;
but I’m not adding custom constraints, because I run a custom validation technique in the body.
This is an excerpt of the body (note that viewName is determined by a configuration property, and it is a view that I use to determine which properties to consider when accepting a remote entity from the REST client, and FILTERED_USER_PROPERTIES is a constant that excludes by default system properties like version, createTs and so on):
// check if the user's id is the same (if present) of the one present in user session
if (user.getId() != null) {
if (!sessionUser.getId().equals(user.getId())) {
throw new CustomValidationException("User id is different from logged in user");
}
} else {
user.setId(sessionUser.getId());
}
final View updateView = metadata.getViewRepository().getView(User.class, viewName);
//view.setLoadPartialEntities(true);
for (ViewProperty prop : updateView.getProperties()) {
if (!FILTERED_USER_PROPERTIES.contains(prop.getName())) {
viewProperties.add(prop.getName());
}
}
// validates the User entity, but exclude validation errors for excluded properties
Validator validator = beanValidation.getValidator();
Set<ConstraintViolation<User>> violations = validator.validate(user, Default.class);
violations = violations.stream().filter(violation -> {
for (Path.Node node : violation.getPropertyPath()) {
if (node.getKind() == ElementKind.PROPERTY &&
viewProperties.contains(node.getName())) {
return true;
}
}
return false;
}).collect(Collectors.toSet());
if (!violations.isEmpty()) {
throw new EntityValidationException(violations);
}
// update the record on db
try (Transaction tx = persistence.getTransaction()) {
EntityManager em = persistence.getEntityManager();
// load existing user from db
User originalUser = em.find(User.class, user.getId(), updateView);
// copy only permitted properties from the passed in user to the originalUser
for (String propName : viewProperties) {
originalUser.setValue(propName, user.getValue(propName));
}
// flush the changes and reload the changed entity
em.flush();
user = em.reload(originalUser, View.LOCAL);
tx.commit();
}
The culprit here is that I never trust the entity passed in by the client, and I find for the entity first, and then I merge only permitted properties in the loaded entity. Then I commit back the changed entity.
The first check is a very custom one, where I validate that the client did not pass a random (but existing) User entity, different from the one found in the current UserSession (but I could simply erase the id and use the user’s id found in session).
I’m using that technique only in one method, so I didn’t generalize it, but the mechanism can be easily refactored as an utility class.
Hope this helps,
Paolo
Hey, that looks pretty good! Your find() is essentially what I was thinking of doing (and have now settled with), minus the use of the view. Glad to know that that is an accepted method.
I’m typically using service methods that make use of additional parameters instead of direct edits to the entity, but I am considering using that approach yet for this project, so your example for doing that part is very appreciated.
Thanks!
Out of curiousity, is that flush() before the reload() necessary? Given that the entity is currently managed, the prior changes should have already taken affect. Or does reload() take the entity from the database instead of persistence? My knowledge of transactions and entity managers is not yet bountiful.
Hi Caden,
that usage of an Entity as a parameter of a REST service method is something I consider an “edge” case.
I wanted a method shared by both the web client tier AND the REST tier, so I arranged a method that could work easily when called from both layers.
In other cases, I normally decouple the core logic of a service function in a method that deals with managed entities, that is then consumed by the web and middle tiers. For the REST clients, I implement a wrapper that accepts and exposes only POJOs, and delegates the actual operation to the other (full-blown) method.
It is only this last method (using POJOs) that is listed in the rest-services.xml file.
For REST clients I also usually avoid returning null from a service method invocation, because null is something usually expected for void (POST) operations (that is operations that usually returns with 200 result, or some other meaningful HTTP error code).
Another thing to consider is to wrap any meaningful “validation” error in one of the supported ValidationException extended classes (like CustomValidationException or EntityValidationException). You should then annotate the interface method with @Validated and and ValidationException to throw clause.
This is the only reliable way (as of CUBA today) to return an error different from a generic 500 one, that is not so meaningful for a consuming client. By making use of ValidationExceptions you could also create your custom validation errors, that are nicely converted to a JSON response to the client.
Regarding calling flush() before reload(), it is there to avoid an optimistic lock exception (when using versioned entities, alongside with the VERSION column on the table) when reload tries to SELECT the instance from database, but conflicts with the already modified entity in the persistence context (to be more clear: if the entity in memory contains for ex. version==2, the entity that is reloaded will have version==1, and hence you’ll have an optimistic lock exception).
Just for your information: in the upcoming platform version 6.7 we have added Id and Ids classes for more typesafe work with identifiers passed to business methods: https://youtrack.cuba-platform.com/issue/PL-9196
Essentially I’m using the Entity as a REST service parameter in such a way that using Id and Ids classes that Konstantin has revealed for us (which looks great, by the way - can’t wait to try it out!) would be a direct replacement for my usage.
Currently my services are accepting primitives where possible, but Lists and Maps where not.
I had tried the @Validated annotations, but for some reason my return values on error remained empty. I failed to specify throws ValidationException, so I will be trying it again, but in the meantime I’ve opted for returning a class I made, ServiceResult, which basically has a success field, two types of error fields, and a result field if success is true. I don’t like returning null, so I make sure I always return something on service methods.
I had not considered optimistic lock exceptions, as I have not yet used version entities. Good to know if I decide to implement that at some point. Thanks for the explanation!
And thanks Konstantin for the info! Looking forward even more to 6.7 now.