Deleting a file with draft and published versions only delete the draft version

[maxime-rainville]

Description

If you have a file that has different published and draft version, and you try deleting it, the published version won't be deleted.

Steps to reproduce

• Upload an image in asset-admin
• Publish it.
• Click on the preview to open the file in a new tab. You should see your image.
• Replace the file with different image.
• Save your changes. Your File DB entry should be kicked into a modified state.
• Click the preview to open the draft file in a different tab. You see the second image.
• Go back to tab displaying the original image and refresh it. You still see the original image.
• Back in asset-admin, delete the File DB entry.
• Go to the tab displaying the second unpublished image and refresh, you'll get a 404.
• Go to the tab displaying the first published image and refresh.

Expected result: You should get a 404. Actual result: The file is still available and has not been deleted.

Video show case in Silverstripe CMS 4.4

Affected version

• Confirmed on 4.4 and 4.5

Note

This only happens if someone has used the "replace file" feature to upload new version of the file without publishing it. My guess is that when deleting the file, asset discovers the draft version first and delete that without looking to see if there's a live version to delete as well.

I'm thinking this might be related to https://github.com/silverstripe/silverstripe-assets/issues/345

Acceptance criteria

• When you delete a File DataObject, either through the UI or the code, the physical live file is no longer accessible.
• When keep_archived is enabled, the live file is archived to the protected store.
• A follow up issue is created to address orphan files.

===

PRs

• [x] https://github.com/silverstripe/silverstripe-versioned/pull/291 (code fix only in onUnpublish)
• [x] https://github.com/silverstripe/silverstripe-assets/pull/412 (unit tests)

Travis kitchen-sink with new fix + assets units tests

• https://travis-ci.org/github/creative-commoners/cwp-recipe-kitchen-sink/builds/715358218

New issue created to delete files

• [x] https://github.com/silverstripe/silverstripe-assets/issues/413

===

Others PRs to close after merge

• [x] https://github.com/silverstripe/silverstripe-assets/pull/416 (no good)
• [x] https://github.com/silverstripe/silverstripe-versioned/pull/286 (original code fix in onArchive and onUnpublish)

Travis kitchen-sink with original code fix + assets unit tests

• https://travis-ci.org/github/creative-commoners/cwp-recipe-kitchen-sink/builds/714668360

[brynwhyman]

Agree with the high impact rating here. Good find @maxime-rainville!

[Leapfrognz]

Very high impact. Especially if the client is paying for extra disk space, yet cannot delete the files that are using it. High impact bug but havent seen any movement on it?

[michalkleiner]

It's nearly a security issue, not just a bug.

[dhensby]

If this is affecting you or your clients, contributions / PRs are welcome.

[emteknetnz]

Fixed in https://github.com/silverstripe/silverstripe-versioned/pull/286 Unit test PR (requires versioned PR above to be merged first) https://github.com/silverstripe/silverstripe-assets/pull/412

New issue for deleting physical files https://github.com/silverstripe/silverstripe-assets/issues/413

[emteknetnz]

Linked PR's have been merged and merged-up from 1.5 -> 1.6 -> 1

A new issue has been created to retrospectively fix up any modified assets that were published and later deleted. We don't have an ETA of when that issue will be actioned.

It was determined that this was not a security issue since any assets in the public store were there because a CMS author had at some point consciously chosen to publish the asset and make it publicly accessible

[Leapfrognz]

"It was determined that this was not a security issue since any assets in the public store were there because a CMS author had at some point consciously chosen to publish the asset and make it publicly accessible"

Sorry but this is a huge assumption. I can think of many examples where the security/sensitivity level of a file changes over time. Something that is not seinsitive one day could indeed be sensitive the next, and should be able to be deleted from the assets store (in all its aspects) with trust that the physical file and versions have been deleted as well.