Feature #51570

Unpersisted changes in Safe Requests should throw an Exception

Added by Marc Neuhaus about 8 years ago. Updated over 7 years ago.

Status:
Resolved
Priority:
Should have
Assignee:
-
Category:
-
Target version:
-
Start date:
2013-08-30
Due date:
% Done:

100%

Estimated time:
PHP Version:
Has patch:
No
Complexity:

Description

With the recent change to not trigger persistAll in safe request methods (GET/HEAD) some users
stumble over not getting their entities saved because of this. Altough it's written to the log
and mentioned in the docs i think we should also throw an exception telling the developer about this
if the following conditions are met:

  • the current request method is a safe request method (GET/HEAD)
  • the current context is the development context
  • the user added/updated/removed an entity using a Repository or PeristenceManager
  • the user did not trigger persistAll using the PersistenceManager on it's own

Related issues

Related to TYPO3.Flow - Feature #47951: Warn if persistence stack is not empty at the end of a get-requestNew2013-05-05

Actions
#1

Updated by Gerrit Code Review about 8 years ago

  • Status changed from New to Under Review

Patch set 2 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/23484

#2

Updated by Gerrit Code Review about 8 years ago

Patch set 3 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/23484

#3

Updated by Gerrit Code Review about 8 years ago

Patch set 4 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/23484

#4

Updated by Gerrit Code Review almost 8 years ago

Patch set 5 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23484

#5

Updated by Gerrit Code Review almost 8 years ago

Patch set 6 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23484

#6

Updated by Gerrit Code Review almost 8 years ago

Patch set 7 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23484

#7

Updated by Gerrit Code Review almost 8 years ago

Patch set 8 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23484

#8

Updated by Gerrit Code Review almost 8 years ago

Patch set 9 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23484

#9

Updated by Gerrit Code Review over 7 years ago

Patch set 10 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23484

#10

Updated by Gerrit Code Review over 7 years ago

Patch set 11 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23484

#11

Updated by Gerrit Code Review over 7 years ago

Patch set 12 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23484

#12

Updated by Gerrit Code Review over 7 years ago

Patch set 13 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23484

#13

Updated by Gerrit Code Review over 7 years ago

Patch set 14 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23484

#14

Updated by Anonymous over 7 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100
#15

Updated by Marco Falkenberg over 7 years ago

Could you please NOT throw an exception and just log it?! Isn't it a little bit overdone accepting a breaking change just for a developer message?

What about if you wan't to persist the missing entities manually after the controller invocation? In my case I persist user messages entities, which I want to show sometimes also for safe requests.

#16

Updated by Marc Neuhaus over 7 years ago

Hey Marco :)

The Exception is only thrown if all these conditions are met:

- you are running inside the Development Context
- you added Entities to the PersistenceManger
- the request is a safe request which, by default does not persist anything that's added to the PersistenceManager
- you did not trigger persistenceManager->persistAll manually after adding the entities

We had several people quite confused because there changes weren't persisted in safe requests. The only case where
this could be considered "breaking" is where you might have a bug because the changes weren't persisted because of the
safe request.

We choose against a log entry because this would have destroyed the whole point of making novices aware of the
safe request behavior.

If i understand you correctly you have a safe request method, were you change/add entities and persist manually?
then this exception should not be thrown. do you use some kind of customer peristenceManager?

Cheers
Marc

#17

Updated by Marco Falkenberg over 7 years ago

Hey Marc. Thanks for your answer. No I don't have a custom persitence manager. The only thing i did was to persist particular entities (Notifications) after every controller invocation. To realize this behaviour I hooked into the slot AfterControllerInvocation which fits very well.

$dispatcher->connect('TYPO3\Flow\Mvc\Dispatcher', 'afterControllerInvocation', function($request) use($bootstrap) {
$myRepo->persistEntities();
});

But Flow now also uses this slot to throw the discussed exception.

#18

Updated by Marc Neuhaus over 7 years ago

ah, i see. yes that could fail then of course. usually the manual persisting is done right were you did the change
in cases like that.

But Robert introduced another thing quite recently: "Whitelisted objects"

https://review.typo3.org/#/c/28792/

http://docs.typo3.org/flow/TYPO3FlowDocumentation/latest/TheDefinitiveGuide/PartIII/Persistence.html#whitelisted-objects

Maybe that could help you?

#19

Updated by Bastian Waidelich over 7 years ago

Marco Falkenberg wrote:

Hi Marco ;)

Just a little comment regarding:

$dispatcher->connect('TYPO3\Flow\Mvc\Dispatcher', 'afterControllerInvocation', function($request) use($bootstrap) {
$myRepo->persistEntities();
});

I guess you're aware of this, but for anyone else who might just copy that:
With this you circumvent our CSRF-protection. That means: You could change the server state (update, add, delete data) just by clicking a link (in an email for example) if you have the required privileges (e.g. you are logged in to the flow app).
That's because the CSRF token is only checked for "unsafe" requests.

While this might be perfectly fine in your case it's worth mentioning that there are ways around this:

For example you could check the CSRF-token explicitly for the affected actions (and make sure any link contains the token..).
Or you could encapsulate the functionality (in this case: notifications) in some service that persists changes immediately.
Or create those entities asynchronously via AJAX.

It depends on the use case but I usually prefer the last solution because it follows the HTTP spec and it allows for caching(!) - for example if you want to increase a view-counter on each view of a certain article

#20

Updated by Marco Falkenberg over 7 years ago

Marc Neuhaus wrote:

ah, i see. yes that could fail then of course. usually the manual persisting is done right were you did the change
in cases like that.

But Robert introduced another thing quite recently: "Whitelisted objects"

https://review.typo3.org/#/c/28792/

http://docs.typo3.org/flow/TYPO3FlowDocumentation/latest/TheDefinitiveGuide/PartIII/Persistence.html#whitelisted-objects

Maybe that could help you?

That's a clean (white) solution and fits perfect to me needs. Thanks Robert!

#21

Updated by Marco Falkenberg over 7 years ago

Bastian Waidelich wrote:

Marco Falkenberg wrote:

Hi Marco ;)

Just a little comment regarding:

$dispatcher->connect('TYPO3\Flow\Mvc\Dispatcher', 'afterControllerInvocation', function($request) use($bootstrap) {
$myRepo->persistEntities();
});

I guess you're aware of this, but for anyone else who might just copy that:
With this you circumvent our CSRF-protection. That means: You could change the server state (update, add, delete data) just by clicking a link (in an email for example) if you have the required privileges (e.g. you are logged in to the flow app).
That's because the CSRF token is only checked for "unsafe" requests.

While this might be perfectly fine in your case it's worth mentioning that there are ways around this:

For example you could check the CSRF-token explicitly for the affected actions (and make sure any link contains the token..).
Or you could encapsulate the functionality (in this case: notifications) in some service that persists changes immediately.
Or create those entities asynchronously via AJAX.

It depends on the use case but I usually prefer the last solution because it follows the HTTP spec and it allows for caching(!) - for example if you want to increase a view-counter on each view of a certain article

Hi Bastian,

many thanks for this hints and enforcing me to think about more often to follow the HTTP specs :)

Of course I thought about not persisting just everything. While ago I found something like this:

class NotificationRepository {
  ...

  public function persistEntities() {
    foreach ($this->entityManager->getUnitOfWork()->getIdentityMap() as $className => $entities) {
      if ($className === $this->entityClassName) {
        foreach ($entities as $entityToPersist) {
          $this->entityManager->flush($entityToPersist);
        }
        return;
      }
    }
  }
}

Please correct if I'm wrong. If I call NotificationRepository::persistEntities only entities of this certain type will be persisted? That's what I tried in code above. But you're right: at least for this entities the CSRF-protection is circumvented.

FYI: I tried the second solution (persisting immediately) with the mentioned special repository function. But then some changes for other objects get lost. After some debugging I could see why. Look at the last lines of \Doctrine\ORM\UnitOfWork::commit.

In the end I used the new introduced whitelist :)

Thanks anyway!

#22

Updated by Bastian Waidelich over 7 years ago

Marco Falkenberg wrote:

many thanks for this hints and enforcing me to think about more often to follow the HTTP specs :)

You're welcome ;)

If I call NotificationRepository::persistEntities only entities of this certain type will be persisted?

Correct, probably including the dependencies (for example product.category).

FYI: I tried the second solution (persisting immediately) with the mentioned special repository function. But then some changes for other objects get lost.
After some debugging I could see why. Look at the last lines of \Doctrine\ORM\UnitOfWork::commit.

Uh.. ok..

In the end I used the new introduced whitelist :)

Good choice ;)

Also available in: Atom PDF