flow-development-collection
flow-development-collection copied to clipboard
neos
BUGFIX: Failed validation prevents resource from being persisted
This is achieved by deferring the importing and persisting of resources until after validation.
Fixes #769
When reviewing, please take a look at the commit https://github.com/neos/flow-development-collection/pull/1931/commits/5120bd76c14fd6bdaf4559ee51f7c50ff147555e first - it only includes the functional changes required for this. All others are only distracting, but useful docblock type changes.
What about importResource(), it is obviously left unchanged in this PR. How do those changes affect that? Do they? Why and why not? I think there should be some explanation on that, ideally close to the code involved…
What about importResource()?
That's a fair question. It (and sibling methods) are indeed unchanged, because an actual import of a Resource should make it persistent (in storage) immediately. Then again, those methods take arbitrary forms of data and convert that into a PersistentResource, while the new persistResource method takes a (deferred) Resource and only makes it persistent. Also, this makes sure b/c is guaranteed.
Now in regard to where to place this information closer to the code - I'll give it a thought and add something. Thanks for the feedback!
After thinking around I can't seem to find a good place or wording to describe the non-effect on the import* methods, other then only describing the "deferred" concept better in the PersistentResource constructor: it is only intended for framework purposes and should not be manually used (unless you know what you do). So I'm happy for any ideas.
Noticed I missed handling arrays correctly and fixed that, but I think no upload ends up as an array in the converter any more since the PSR-7 change. Have to check that - maybe we can get rid of that for next major.
ping @kitsunet - did you have time to give it another thought as we discussed?
As said in our unicorn weekly I even think this could be considered a security issue, in which case we would need to target 4.3 - I'd take care if we agree on this.
@kitsunet @kdambekalns @robertlemke @bwaidelich @sorenmalling @daniellienert (sorry for the roundhouse mention)
PS: And yes, this is not an optimal solution. Let's keep improving this in master further to avoid the "deffered persisted resource" paradox, but this will inevitably be a breaking change then.
It's a solution to the original issue, and as you write
PS: And yes, this is not an optimal solution. Let's keep improving this in master further to avoid the "deffered persisted resource" paradox, but this will inevitably be a breaking change then.
Did our possibilities change with the introduction of PSR-15 - can we handle a GC of uploaded resources, that is up for deletion due to validation failure once the request "is done"?
can we handle a GC of uploaded resources, that is up for deletion due to validation failure once the request "is done"?
The core issue is really that the resource is added for persistence before validation in the first place. It's non-trivial to "un-add" cleanly. And the fact that validation is only run directly before invoking the action method inside the controller, so there is not much room for "in between" layer that does this (after the action is too late, because the action might call persistAll()).
ping @kdambekalns @robertlemke @daniellienert @bwaidelich still would need someone of you to have a close look and approve or reject this. Maybe @kitsunet could remove the veto unless the concerns are still not alleviated
Okay everyone - I had an idea for an alternative implementation, that will not introduce a new concept of a "deferred" PersistentResource and be much less breaking. It is partly inspired by @sorenmalling's question here and @bwaidelich's onStep() callback implementation we used for PSR-15 middleware chain to hook into the steps to update our RequestHandler's $request instance. So props to them both.
Please take a look there (#2348) - if we can agree on that, this can be closed for good.
Just some general thoughts here: We had this in several Pen-Tests reported to be an issue (importing a file into local storage, even if not validated). Our approach has been to write a validating ResourceConverter, which can be extended in some way, to make the testers happy.
In general I really dislike the behavior that the database gets modified by a TypeConverter - persisting changes should be a task the controller has full control of - so I would love a solution, which do not persist the resource already. But I understand why this seems "desired", when it comes to resources. But it leads to all sort of "unexpected" behavior. For example just receiving a "file object" you will only work with ONCE in your controllers action will be saved in storage and database and rot there unrelated. Something we should probably at least address in documentation.
Yeah, the best solution would be if a resource is not persisted at all (neither storage, nor DB) until the controller decides it wants to. The storage persistence could be solved by a onFlush listener and the controller would need to do $this->persistenceManager->persist($uploadedResource) (or persist the parent entity). That would be a breaking change though, but maybe something for Flow 8?
Anyway, regarding this BUGFIX - as mentioned in the other PR I'm leaning a bit more towards this solution right now, but with a few adjustments. And I agree to documenting the behaviour - maybe with a suggestion on how to deal with "temporary" file uploads properly. Hopefully after the 7.2 release I can find some time to continue the work on this.