(umbraCollab) Check media Parent for permissions when setting correct MediaType

[cornehoskam]

Prerequisites

• I have added steps to test this contribution in the description below

If there's an existing issue for this PR then this fixes #7735 & the underlying issue of uploading disallowed mediaTypes.

Description

Ever since Umbraco version 8.14, the way media gets uploaded has changed. In the past, uploading media directly from a MediaPicker would respect the permissions set on the upload folder when not using the default mediaType of "Image". Ever since version 8.14 this has changed, and even if the folder does not allow the default "Image" item to be uploaded, it still forces the uploaded image to be that media type (and same with File).

With this pullrequest, as created and demonstrated during today's umbraCollab, we not just upload an image with the default 'Image' Media Type, but first try to determine if the folder we are trying to upload the item to has any specific permissions set that have the "umbracoFile" property. If that is the case, we want to use that Media Type instead of the default!

For Folders we are not able to distinguish between custom folders or the default Folder, so best practice for now would be to not allow the creation of the default Folder if the parent doesn't explicitly allow this, so that we can't create folders where they aren't allowed!

To test this Pullrequest, simply try out any mediapicker that points towards a media folder that has specific permissions set up, upload a new image, and it will upload it to the correct media type! If it does not find any items within it's permitted children with a property of alias umbracoFile, it will continue with the rest of the Umbraco logic and upload to the default type!

[umbrabot]

Hi there @cornehoskam, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[mikecp]

Approved at the end of umbracollab but needs final reviewing from @nul800sebastiaan

[cornehoskam]

@nul800sebastiaan Any updates on the review process so far? Let me know if there's something you feel is missing! 🙂

[nul800sebastiaan]

@cornehoskam and @mikecp thanks very much for the work so far! I have given it a go and noticed a few things were missing which I could've just sent you feedback about but while I was experimenting I accidentally kinda fixed them myself. One of the surprising things about #7735 was that you can drag a whole folder onto the dropzone. That scenario is now also covered.

I also "fixed" when you create a folder with the + button in a media picker that we now return an error message instead of just not creating it. This has a drawback that the progress indicator under the folder path keeps going, but I feel like it's better to actually show a message, we can see if we can fix that last part later.

Finally, and most importantly, it seems like for whatever reason you were not rejecting files if they were going to get a mediaType not allowed under that parent. I swear I saw that working earlier, but it seemed missing so I added that aroung line 879 (if (allowedContentTypes.Any(x => x.Alias == mediaTypeAlias) == false)).

A video with different attempts:

11858.mp4

Now I have kind of a moral dilemma, technically these are breaking changes, suddenly you're no longer allowed to create folders where you were allowed to make them before. But I would argue that we're just implementing expected behavior.. The infamous "unbreaking breaking change". I'll have to talk to the team to see if this is a problem. Personally I think it's good to make it explicit that folder creation is not allowed when it's not configured to be allowed and the previous behavior was a bug.

[mikecp]

Thanks for checking further @nul800sebastiaan 😁
I tend to agree with you, to me the previous behavior was wrong, so this should be seen as a bugfix and not a "feature".

[nul800sebastiaan]

Thanks @nzdev for the additional comments. Saved a bunch of calls to the MediaTypeService, very nice indeed! 👍

I agree that this whole class needs a good cleanup but we'll save that for another day.

It looks like everyone was happy with the changes to make the uploader more restrictive, also known as: fixing the existing bugs to the uploader.

So.. finally time to merge this one, thanks again for getting it started and all the work @cornehoskam!

And look at that shiny new contributor badge on your profile as well! https://our.umbraco.com/members/cornehoskam 🏅

[bachicool]

@nul800sebastiaan This is still happening in Umbraco 8.18.0. Is there a different bug/issue open for Umbraco 8?

[nul800sebastiaan]

@bachicool This has only been fixed for v9. We're in support mode for v8 since the 8.18.0 release, which means we will fix regression and security issues on v8.

[m4nickroll]

@nul800sebastiaan This regressed in 8.14 so should arguably be fixed in v8 too? We've got a client with a media-heavy site and it's totally changed the behaviour (to be incorrect) when dragging and dropping images since upgrading to 8.17 from pre 8.14

[nul800sebastiaan]

I understand, we have many things that arguably should be fixed, but only so much time. At the moment we classify regressions as bugs introduced in the last 3 minors and unfortunately that makes 8.14 bugs no longer eligible.

[m4nickroll]

@nul800sebastiaan I have to say that's quite frustrating given the fix is known. It seems arbitrary to look at regression only from 8.15 onwards when this is quite a significant one. The minor versions in question are only around 6 weeks apart

[nul800sebastiaan]

@m4nickroll Understood! And of course we'll consider exceptions to the rule where they make sense.

However.. maybe I wasn't clear about this, this scenario exists in all of v7 and v8 with the default media picker as well, we've never adhered very well to what was allowed where. Consider the following Custom Folder that only allows media items of type Custom Image to be created under it in v8.11 - it just creates images of the original type "Image" without question, it also just creates a folder which is not allowed here:

11858.mp4

[bachicool]

@nul800sebastiaan, Agree, that this bug does exist in the earlier versions.

But in the earlier versions (< 8.14), it is working in the media section. Upgrading to the latest version we may need to remove the drag and drop area (maybe using CSS) in the media section. As files will be associated with the wrong media type. But if we remove the drag and drop functionality it will take away the end users/editor's best experience in using Umbraco; as they will need to create the images one by one. And this will restrict most of us with custom folders from upgrading Umbraco any higher than 8.13. And I hope you appreciate the fact that on a website media and content are equally important.

In my humble opinion, this bug needs to be addressed and is one of those exceptions; due to its disruptiveness in the end-user experience.

Demo where Drag and drop working in version 8.12.1 under the media section:

2022-03-20_21-18-04.mp4

[nul800sebastiaan]

@bachicool Fair enough, that is a different problem that I actually didn't look at very much, I was focused on the media picker instead. I didn't really look in the media section very much for this one.

For now, you could solve this by using our event handlers and reject things that are not allowed. However, if anyone would like to try to port this PR to v8 then we'd be happy to have a look (target branch: v8/dev).

[FraserConnolly]

@nul800sebastiaan , thanks for your work on this. Could I ask if this was identified as a breaking change in Umbraco 8.14? If not could that notice please be added.

I recently upgraded from 8.12 to 8.18 and didn't spot this issue until it was too late, now I'm in real trouble trying to bodge in a fix using Event Handlers to stop the default Image type being saved.

[nul800sebastiaan]

Honestly, I didn't realize there was a breaking change until two weeks ago when we talked about this, but I've marked it as a breaking change now referring to this PR. #9461

[OwainJ]

Hi @nul800sebastiaan, I've just had a go at porting this PR to v8, the code has been ported across mostly as-is, but I did tweak it slightly so it would work with v8's codebase.

The PR: #12233

[jgettrup-ng]

Thanks everyone for their work on this. After installing and testing v8.18.4 there is still an issue with this that we think represents a breaking change. We jumped from v8.12 to v8.18 so we don't know exactly when it changed.

We have a custom media type called CustomFolder that inherits from Folder and another called CustomImage that has an umbracoFile property. The permissions on CustomFolder allow just two child node types: CustomImage and CustomFolder (to allow for subfolder structures).

When we drag an image onto the upload control we get the disallowedMediaType message ("Cannot upload this file, the media type with alias 'Image' is not allowed here"). The expected behaviour is that a CustomImage item is created and the image file is uploaded into it.

The issue is that there are two allowed child node types (folder and image), so the code does not allow the automatic creation of the image item. I would argue that there is only one "valid" child node type (the one with the umbracoFile property), and that an item of that type should be created automatically. We believe this used to be the behaviour in v8.12.

I suggest one possible solution below, which checks for the number of allowed media types that have the umbracoFile property:

src/Umbraco.Web/Editors/MediaController.cs: line 711

if (parentId != Constants.System.Root)
{
    var mediaFolderItem = mediaService.GetById(parentId);
    var mediaFolderType = allMediaTypes.FirstOrDefault(x => x.Alias == mediaFolderItem.ContentType.Alias);
+   int numAllowedContentTypesWithFileProperty = 0;

    if (mediaFolderType != null)
    {
        IMediaType mediaTypeItem = null;

        foreach (ContentTypeSort allowedContentType in mediaFolderType.AllowedContentTypes)
        {
            IMediaType checkMediaTypeItem = allMediaTypes.FirstOrDefault(x => x.Id == allowedContentType.Id.Value);
            allowedContentTypes.Add(checkMediaTypeItem);

            var fileProperty = checkMediaTypeItem?.CompositionPropertyTypes.FirstOrDefault(x => x.Alias == Constants.Conventions.Media.File);
            if (fileProperty != null)
            {
                mediaTypeItem = checkMediaTypeItem;
 +              numAllowedContentTypesWithFileProperty++;
            }
        }

        //Only set the permission-based mediaType if we only allow 1 specific file under this parent.
 -      if (allowedContentTypes.Count == 1 && mediaTypeItem != null)
 +      if (numAllowedContentTypesWithFileProperty == 1 && mediaTypeItem != null)
        {
            mediaTypeAlias = mediaTypeItem.Alias;
        }
    }
}
else
{
    var typesAllowedAtRoot = allMediaTypes.Where(x => x.AllowedAsRoot).ToList();
    allowedContentTypes.UnionWith(typesAllowedAtRoot);
}

[Chris-N1]

We have also just come across this issue in Umbraco 10.3.1 (migrating from 8.6.5, v10 currently in UAT which is how we discovered this)

We have exactly the same structure as @jgettrup-ng - a custom folder with a custom media type where the custom media type is the only allowed media type in the folder (but the folder type is also allowed to allow for sub-folders). When dragging and dropping a file we get the error about not being allowed to create the "file" type.

[jgettrup-ng]

It's a pity this one hasn't been fixed yet. I did suggest to our editors that they use the "Create" button to create the media item first, and then drag the image file into the newly-created item, but they really missed the speed and simplicity of the drag and drop to create. So, I ended up creating a new single-level "leaf" folder type that could only contain the image item and not the folder item. This allows the editors to drag and drop images again, but not create any further levels of subfolder. For our use case, the editors were happy with that workaround. From: Chris Norwood ***@***.***> Sent: 25 October 2022 17:58 To: umbraco/Umbraco-CMS ***@***.***> Cc: Jim Gettrup ***@***.***>; Mention ***@***.***> Subject: [EXTERNAL] Re: [umbraco/Umbraco-CMS] (umbraCollab) Check media Parent for permissions when setting correct MediaType (PR #11858) This Message originated outside your organization.

…

________________________________ We have also just come across this issue in Umbraco 10.3.1 (migrating from 8.6.5, v10 currently in UAT which is how we discovered this) We have exactly the same structure as @jgettrup-ng<https://github.com/jgettrup-ng> - a custom folder with a custom media type where the custom media type is the only allowed media type in the folder (but the folder type is also allowed to allow for sub-folders). When dragging and dropping a file we get the error about not being allowed to create the "file" type. - Reply to this email directly, view it on GitHub<#11858 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AOPHQZLSDLRRGNWMDL74W5LWFAGRDANCNFSM5L4PLUEA>. You are receiving this because you were mentioned.Message ID: ***@***.******@***.***>>