Fix content composition blocking undo operation #7056
Conversation
|
I don‘t think this is the right solution. We should wrap the following line in a try/catch block instead: contao/core-bundle/contao/drivers/DC_Table.php Line 1918 in 110d32f This way, the undo operation would continue to restore all records except the faulty one. |
|
I disagree. This would silentry drop data from the database on restore. We don't warn the user, we don't give any option, and it would also delete the undo record meaning everything is just gone. My behaviour would undo the existing records as it was, which is what I would expect from an undo operation. |
aschempp
force-pushed
the
fix/content-composition-undo
branch
from
b88bbda
to
7cbe56c
Compare
|
I agree that we should not delete the undo record if the restore operation was not successful. And we should probably give feedback to the user in this case. Still your suggestion only handles one of many possible cases. We should have a more general solution IMHO. |
|
But in general, undoing something that now (with changed permissions in the meantime) is not allowed anymore, must be prohibited? Or should undoing bypass permissions completely? |
|
If the permissions are changed, I think it is correct that you cannot undo something. Using stored procedures would be a separate topic though (not sure if that already works, it didn't for me locally on MySQL 5.1). This specific use case is because we allow to create content that is later prohibited, I think that is valid to be restored. |
leofeyer
added
up for discussion
| @@ -55,6 +55,12 @@ public function vote(TokenInterface $token, $subject, array $attributes): int | |||
| return self::ACCESS_ABSTAIN; | |||
| } | |||
|
|
|||
| // If the record has an ID, it is most likely an undo operation, which should | |||
| // allowed if there were articles on a page without content composition. | |||
| if ($subject instanceof CreateAction && $subject->getNewId()) { | |||
There was a problem hiding this comment.
As discussed in the Contao call, the restore operation should be an update action instead of a create action.
Description ----------- This restores the permission handling we had in all previous Contao versions. After discussing with @Toflar, this is the only viable solution to fix our permission issues. Here are two examples that are imho impossible to fix otherwise. They are not a technical issue, but a logical one. 1. If you duplicate a page, its articles will be duplicated as well as its content elements. If you do not have access to some elements, the whole page and articles are duplicated, **but "random" content elements will be missing**. 2. even worse, when deleting a page, DC_Table tries to delete all child records and create an undo record for them. The ones that the user does not have access will not be deleted, **and will not be added to the undo record**. They are however still removed by our reviseTable, and will therefore be lost without the option to undo. In the same sense we could discuss to adjust the `undo` method instead of what we discussed in #7056 FYI @Toflar and me discussed that the same handling will not be possible in a regular REST/CRUD API. It would require special APIs for that, which can always be added if necessary. - Fixes #7101 - Probably also makes #7102 obsolete but correctly checking the IDs/updating the cache might still be correct. Commits ------- eb388c6 Skip permissions checks for child records 961033f Also remove permission check in undo operation 70d7577 Fixed duplicate variable name
Description ----------- This restores the permission handling we had in all previous Contao versions. After discussing with @Toflar, this is the only viable solution to fix our permission issues. Here are two examples that are imho impossible to fix otherwise. They are not a technical issue, but a logical one. 1. If you duplicate a page, its articles will be duplicated as well as its content elements. If you do not have access to some elements, the whole page and articles are duplicated, **but "random" content elements will be missing**. 2. even worse, when deleting a page, DC_Table tries to delete all child records and create an undo record for them. The ones that the user does not have access will not be deleted, **and will not be added to the undo record**. They are however still removed by our reviseTable, and will therefore be lost without the option to undo. In the same sense we could discuss to adjust the `undo` method instead of what we discussed in contao/contao#7056 FYI @Toflar and me discussed that the same handling will not be possible in a regular REST/CRUD API. It would require special APIs for that, which can always be added if necessary. - Fixes contao/contao#7101 - Probably also makes contao/contao#7102 obsolete but correctly checking the IDs/updating the cache might still be correct. Commits ------- eb388c66 Skip permissions checks for child records 961033f3 Also remove permission check in undo operation 70d75771 Fixed duplicate variable name
If you delete the whole page tree in the Contao demo, and try to restore it, the current system fails, because some pages contain articles that should not be allowed. This is because a page can be
regular, have an article, and then change its type to something that does not allow content composition.I think the best option would be to allow such a restore, because we don't want to loose data without telling the user. If a
CreateActionhas an ID, it most likely is a restore (at least internally, maybe not from an API). Since this will only allow "invalid" content composition, but still correctly check access permissions in other voters, I think that should be fine.