Check permission for deleting versions

[BlackbitDevs]

Linked the “Publish” context menu item of a version to the “Publish” permission
Linked permission to "Delete" a version to the "Delete" permission

[kingjia90]

LGTM, just wondering if we need to add the permission check on the controller as well, like

admin-ui-classic-bundle/src/Controller/Admin/Asset/AssetController.php

Line 955 in eb7b668

if ($currentAsset->isAllowed('publish')) {

to

admin-ui-classic-bundle/src/Controller/Admin/ElementController.php

Lines 708 to 733 in e414e6b

	public function deleteVersionAction(Request $request): JsonResponse
	{
	$version = Model\Version::getById((int) $request->get('id'));
	$version->delete();
	return $this->adminJson(['success' => true]);
	}
	/**
	* @Route("/element/delete-all-versions", name="pimcore_admin_element_deleteallversion", methods={"DELETE"})
	*
	* @param Request $request
	*
	* @return JsonResponse
	*/
	public function deleteAllVersionAction(Request $request): JsonResponse
	{
	$elementId = $request->request->getInt('id');
	$elementModificationdate = $request->request->get('date');
	$versions = new Model\Version\Listing();
	$versions->setCondition('cid = ' . $versions->quote($elementId) . ' AND date <> ' . $versions->quote($elementModificationdate));
	foreach ($versions->load() as $vkey => $version) {
	$version->delete();
	}

[kingjia90]

Wondering if it should be considered a bug or even security instead, the permission is meant to make versions tab available but any of the feature/actions in there (like publish,delete) should be still following the main permissions rules.
I guess it was an superuser-only tool but at some point it became so useful that then was almost commonly granted to every one.

Looks like a sort of "vertical elevation of privilege" in the context of element workspace permissions.

[fashxp]

Well, publishing a version and deleting a version are two different things in terms of consequence.

When publishing a version, it means changing the public accessible data of the affected element. And that's why it is bound to the pulish permission.

When deleting a version, it is just about the version and not about deleting the element. So it has no effect on the public available data. That's why it is not bound to the delete permission (as delete permission so far means deleting the element), but possible with version permission - when you have access to versions you also can delete them.
Binding that to delete permission might also have unwanted side effects, as then users would need to have delete permission if they should be able to clean up versions and so now would have the possibility to (accidently) delete the whole element.

Not sure if we should do that? Any other opinnions/aspects?

If we decide to change that, we
a) need to make sure it is consistent to documents and assets as well,
b) also modify server-side checks (as suggested by @kingjia90 ) and
c) check portal engine as well.

Please do not merge yet.

[brusch]

I agree with @fashxp here.
We need to differentiate between version management and actions that have direct impact on the linked element, such as "publish".
So deleting versions has no direct impact, while publish does, so we need to check against the publish permission.

[kingjia90]

Mhh, that's a good point, same word but dealing with different aspects with different impacts, delete is relatively higher level, it intrinsically delete the versions eg, https://github.com/pimcore/pimcore/blob/b4ec3bacbe72ca115c28cac6b41f1115dd41b28b/models/Asset.php#L963-L966

What about adding a new permission definition eg. "Versions - Manage" (a general one instead of workspace specific), where it allows the current deletion of versions and is more an administrative tool, but still limited only to the workspaces one is allowed to see by versions.

By safely assuming that the purpose of normal user to delete was not about concerning server storage usages, rather to have a cleaner and shorter list with relevant changes to browse and work with, i would replace the delete with archive for them.

Under the hood, it only "hides", just needs a column in versions table and tweak the listing DAO.
For Admins, need to enable filtering between archived and all versions eg. by adding a checkbox, extending the context menu (delete all archived) and flush command option to target archived.

[APochmann]

Just a comment as I ran into exactly the same problem:

In my implementation it's essential to have the history information to know exactly who changed what and when. Therefore the users must not be able to delete history entries and they are also not allowed to delete objects.

In the past (up to pimcore v10) I removed the delete menu entries in JavaScript code by overwriting onRowContextmenu of pimcore.object.versions. With pimcore 11 this is not possible anymore as only events may be used, unfortunately there is no event for that.

So for pimcore 11 I'm not able to get rid of the menu entry and I had to overwrite the deleteVersionAction to throw a AccessDeniedHttpException which is a terrible solution although it works.

I really would appreciate to have the possibility again to remove the delete and deleteAll menu entries from that context menu and maybe there are more implementation out there which used that possibility. Although for my use case the deletes of object and version are more or less equal I understand the difference you see and would support the proposal from @kingjia90 to have a generic user permission to manage version entries