Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Media is still accessible via it's original URL after its been sent to the recycle bin #3568

Closed
wants to merge 5 commits into from

Conversation

stevetemple
Copy link
Contributor

@stevetemple stevetemple commented Nov 8, 2018

I have created a PR as a proof of concept on the approach to discuss. It's a little hacky, but based on discussion with @nul800sebastiaan it seemed like the least breaking.

#2931

Description

When media is trashed, I'm appending a guid to the filename to make the media 404 for the existing requests. Through security by obfuscation it should then be impossible for outside users to guess the url of the media.

This process is reversed by moving out of the recycle bin

@emmaburstow
Copy link
Contributor

Hi there @stevetemple 👋

I'll try and get to this today and let you know if you need to change anything at all.

Em

Copy link
Contributor

@emmaburstow emmaburstow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this workaround... It works as advertised too. You do get an ugly error page though - or maybe that's because I test locally? Looking to @nul800sebastiaan for his thoughts on the architecture of this solution but I can say for sure that it works. Nice work @stevetemple!

@nul800sebastiaan
Copy link
Member

Thanks again @stevetemple - this looks good!

It would be great if you could verify that this works with load balancing and with a blob storage provider. From what I can see you're using MediaFileSystem so should be good, but I would love some confirmation. 👍

@stevetemple
Copy link
Contributor Author

Thanks again @stevetemple - this looks good!

It would be great if you could verify that this works with load balancing and with a blob storage provider. From what I can see you're using MediaFileSystem so should be good, but I would love some confirmation. 👍

Good call, Azure Blob Storage doesn't like one or more of the characters used in the guids/seperator, so I'll address that. Also ImageCropper stores stuff differently in umbracoFile so it'll also need to support that data type. Will take a look over the next few days.

@nul800sebastiaan
Copy link
Member

@stevetemple Good one on the cropper! We have just merged some parsing in this PR, might be good to centralize it: https://github.com/umbraco/Umbraco-CMS/pull/3940/files#diff-45a03e06ffad5ec2f41aae2bb48cd0e8R298

Support for imagecropper
@stevetemple
Copy link
Contributor Author

@nul800sebastiaan I've moved the umbracoFile property logic to a class called UmbracoFileProperty which can cope with image cropper or just a file path.

If you like that approach I can update the other referenced place where it's used and it might make it easier for people to work with in future. I know my IntelligentMedia module would benefit from reusing that code.

@zpqrtbnk zpqrtbnk changed the base branch from dev-v7 to v7/dev March 31, 2019 16:33
@nul800sebastiaan
Copy link
Member

Hey there @stevetemple !

I'm writing today after announcing at Codegarden, our yearly conference, that the upcoming version 7.15.0 will be the last Umbraco v7 release with new features in it. From now on all our efforts will be focused on version 8 instead. 🎱

We're wrapping up the 7.15 version at the moment and unfortunately these proposed changes won't be able to make it in there.

We'd like to extend a big thank you the efforts on this so far and we would love it if you could have a look at porting this feature over to v8 instead. Of course if you have any questions on where to start then feel free to ask on the forum, we monitor all questions there and we're happy to assist.

Thanks again for the work in this PR and we hope to see you contributing to v8 some time soon!

Best, Sebastiaan and the Umbraco CMS team.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants