DTGE is (even more) broken for normal editors on 8.18.12

[kasparboelkjeldsen]

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

8.18.12

Bug summary

8.18.11 introduced an unintended side-effect of breaking the add content dialog in DTGE for people who had a limited tree-view and not access to the root of the content tree. 8.18.12 might have fixed that, but introduced another big issue that we are seeing both on Umbraco Cloud and on non-cloud solutions.

Any user who doesn't have access to the Settings section cannot create or edit doctype grid editor content. If they try, PostSaveBluePrint will return a 401 and a challenge, which on Umbraco Cloud sends them to a login-screen and on non-cloud solutions sends them to whatever provider you have set up.

In short this isn't a viable solution ContentController.cs

Specifics

Doctype Grid Editor uses Blueprints as a means of functioning, and limiting PostSaveBluePrint to only people with access to "DocumentTypes" effectively locks out all editors from editing DTGE grids (unless you give your editors access to Settings, which I don't think is a good idea)

In effect, the issue is with this patch made worse than it was with 8.18.11, as this at least only affected editors with start-nodes other than root, where this effectively locks out all editors from editing.

I have uploaded an example video here showing the issue and how it is "resolved" by assigning the editors rights to the settings section. I can't upload it in this ticket as it's more than 10mb.

https://www.youtube.com/watch?v=6OKfK2CGx0M&feature=youtu.be

As is observed, the effect of trying to add DTGE content sends you to a login-screen, as you get a 401 from PostSaveBluePrint. I'm not sure what the correct solution is here, but we are forced to downgrade our non-cloud solutions to not break editing, and on cloud we are a little bit screwed right now, as the patches has been automatically applied to all our environments.

I realise DTGE is a plugin, and not official Umbraco, but seeing as an effort was made to "unbreak it" with .12, that made it even worse, I'd say this is a priority to fix now.

Steps to reproduce

1. Install DTGE on 8.18.12
2. Create at least 1 doctype grid element
3. Try to insert grid as an editor or other user without access to Settings
4. Observe 401 and potentially redirect

Expected result / actual result

Seeing as we were trying to unbreak what 8.18.11 broke in DTGE, the expected result should be that the user is able to create and edit content in DTGE.

[github-actions[bot]]

Hi there @kasparboelkjeldsen!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

• We'll assess whether this issue relates to something that has already been fixed in a later version of the release that it has been raised for.
• If it's a bug, is it related to a release that we are actively supporting or is it related to a release that's in the end-of-life or security-only phase?
• We'll replicate the issue to ensure that the problem is as described.
• We'll decide whether the behavior is an issue or if the behavior is intended.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face:

[wat-alwines]

Our agency is also experiencing this issue with an implementation of DTGE in Umbraco 8.18.12 leaving many website editors unable to make necessary changes. We are looking for any options that do not involve rolling back the security patch as this will re-expose vulnerabilities. We experience exactly the same behavior as shown in the gif on the associated bug ticket for 10. We are using DTGE 1.2.9 and we have also tried previous versions but all suffer the same error.

We also realized editors with Admin group permissions are still able to make changes but they are the exception.

[andr317c]

Hey! Thanks for posting this issue. This issue has something to do with the package DTGE, they will have to update their package.

A "workaround" for this is to give users access to DocumentTypes.

I will go ahead and close this issue, as it is not an issue on our end.

[kasparboelkjeldsen]

@andr317c I realize it's not my decision, but I disagree with you closing this. I would support the argument "no, it's an issue with DTGE, they will have to fix it", if the issue hadn't just been aggravated and made worse by the 8.18.12 release

In other words, Umbraco opened this box, now they shouldn't just close it without a fix. You've made it an issue on your end.

This will affect quite a lot of Umbraco instances, as DTGE is an incredibly common plugin to run, and giving editors settings access on the sites is not a viable solution for most bigger sites, and changing a core behavior in DTGE doesn't even seem as a viable option from my perspective.

Version 8.18.12 is effectively keeping all our Umbraco 8 sites stuck on 8.18.10 or 8.18.11 which is a huge problem security wise if something actually important comes up.

I would like to attempt to solve this if Umbraco won't, but then we should reopen this issue so I can make a pull request.

[ws-graham]

I agree with @kasparboelkjeldsen here, this is something that needs to be looked into. This update fundamentally breaks core functionality of Umbraco v8.

This is something that needs to be looked into for the next release, otherwise it will make it difficult for many users of v8 to upgrade if a critical update needs applying.

The current support of Umbraco 8 doesn't go into the security phase until February 24, 2024, so it should be resolved. Not to mention with a community member like @kasparboelkjeldsen willing to make a pull request and solve themselves - at least keep this open for sure.

[bergmania]

Hi @kasparboelkjeldsen...

Can you describe how you plan to fix it in Umbraco, without exposing logic to people without sufficient permissions? I basically think it's something that needs to be solved in DTGE, either by copying/pasting code from umbraco or by using reflection.

Then it is up to the users of DTGE to decide whether they want it or not.

[kasparboelkjeldsen]

@bergmania I'll take a closer look tomorrow - I agree in principle that it should have been fixed by DTGE, but DTGE relying on being able to make blueprints for content validation, does not seem like something you easily fix, and it is a way of working that has worked reliably up until patch 8.18.11, which specifically broke it so you couldn't create or edit DTGE content if you didn't have a root as your startnode - the "fix" in 8.18.12 then simply broke if for -everyone- except people with settings access.

Again, as I see it, Umbraco opened the box and just accepting that one of the most common plugins for Umbraco 8 has been broken doesn't seem like a viable solution to me. Then it would honestly be better to revert the api to 8.18.11 so it at least is only broken for some people.

If it turns out it isn't fixable without re-exposing logic, specifically for blueprints, that notably has been exposed since forever, then perhaps a I could provide a solution gated behind a configuration, where all of us still managing v8 installations have an option through a configuration-value, to enable DTGE to continue to function, and then accept the small decrease in security, but still be able to receive future security patches.

I manage a few solutions that we are currently keeping on 8.18.10 because the added "security" is severally diminished by the insecurity of letting 50something people roam around in the settings section. I also manage a few solutions where we litterally don't have a choice, as they graciously have been broken by the auto-update of Umbraco Cloud.

I imagine there are -many- solutions out there suffering from the same issue, and letting everyone into the settings area constitutes a far greater security risk than gating PostSaveBlueprint behind the added security.

[kasparboelkjeldsen]

@bergmania @andr317c here's a concrete suggestion for how to fix it https://github.com/KXCPH/Umbraco-CMS-Knowit/commit/bccf60690c149206b26a9ac7a0725f879d39d5e5#diff-2d2a06dde78180cda3a2a707ed012bd65a11e99e9acca60be4b662f42c30c58d

Issue is still closed so I haven't pull-requested.

In short - I create a new appsetting: "Umbraco.Web.DowngradeBlueprintSecurity" which can be true or false, default false so normale behavior is preserved A new OverridableAuthorizationAttribute "UmbracoBlueprintAuthorizeAttribute" which acts just like "UmbracoTreeAuthorizeAttribute" except that when DowngradeBlueprintSecurity is set to true, it defaults to allow access if you have access to Constants.Tree.Content

This way, all of us with a broken Umbraco site can add the key and have a grid that works, and for everyone else, it's business as usual.

Let me know what you think.

[ws-graham]

Yes, that sounds like a simple solution that will help us out for sure!

[bergmania]

@kasparboelkjeldsen.

We are currently investigating other options.. A configuration is really not something we want, and especially not something that we would leak into newer versions of Umbraco.

From what I can see, it seems DTGE can easily make an endpoint that does the same with less restrictions if we make a few classes public. On V10 (as I can't easily test the v8 on my mac) it would be ContentSaveValidationAttribute, ContentItemBinder, BlueprintItemBinder.

Thereby it would be possible in DTGE to make a API like this:

using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.Logging;
using Umbraco.Cms.Core.Actions;
using Umbraco.Cms.Core.Dictionary;
using Umbraco.Cms.Core.Events;
using Umbraco.Cms.Core.Mapping;
using Umbraco.Cms.Core.Models.ContentEditing;
using Umbraco.Cms.Core.PropertyEditors;
using Umbraco.Cms.Core.Routing;
using Umbraco.Cms.Core.Scoping;
using Umbraco.Cms.Core.Security;
using Umbraco.Cms.Core.Serialization;
using Umbraco.Cms.Core.Services;
using Umbraco.Cms.Core.Strings;
using Umbraco.Cms.Infrastructure.Persistence;
using Umbraco.Cms.Web.BackOffice.Controllers;
using Umbraco.Cms.Web.BackOffice.Filters;
using Umbraco.Cms.Web.BackOffice.ModelBinders;
using Umbraco.Cms.Web.Common.Authorization;

namespace WhatEver;

public class ProductsController : BackOfficeNotificationsController
{
    private readonly ContentController _contentController;

    public ProductsController(
        ICultureDictionary cultureDictionary,
        ILoggerFactory loggerFactory,
        IShortStringHelper shortStringHelper,
        IEventMessagesFactory eventMessages,
        ILocalizedTextService localizedTextService,
        PropertyEditorCollection propertyEditors,
        IContentService contentService,
        IUserService userService,
        IBackOfficeSecurityAccessor backofficeSecurityAccessor,
        IContentTypeService contentTypeService,
        IUmbracoMapper umbracoMapper,
        IPublishedUrlProvider publishedUrlProvider,
        IDomainService domainService,
        IDataTypeService dataTypeService,
        ILocalizationService localizationService,
        IFileService fileService,
        INotificationService notificationService,
        ActionCollection actionCollection,
        ISqlContext sqlContext,
        IJsonSerializer serializer,
        ICoreScopeProvider scopeProvider,
        IAuthorizationService authorizationService,
        IContentVersionService contentVersionService,
        ICultureImpactFactory cultureImpactFactory)
    {
        _contentController = new ContentController(cultureDictionary, loggerFactory, shortStringHelper, eventMessages,
            localizedTextService, propertyEditors, contentService, userService, backofficeSecurityAccessor,
            contentTypeService, umbracoMapper, publishedUrlProvider, domainService, dataTypeService,
            localizationService, fileService, notificationService, actionCollection, sqlContext, serializer,
            scopeProvider, authorizationService, contentVersionService, cultureImpactFactory);
    }

    [Authorize(Policy = AuthorizationPolicies.BackOfficeAccess)]
    [Route("umbraco/backoffice/dtge/whaterver/")]
    [FileUploadCleanupFilter]
    [ContentSaveValidation(skipUserAccessValidation:true)]
    public async Task<ActionResult<ContentItemDisplay<ContentVariantDisplay>?>?> PostSaveBlueprint(
        [ModelBinder(typeof(BlueprintItemBinder))] ContentItemSave contentItem)
    {
        ActionResult<ContentItemDisplay<ContentVariantDisplay>?>? result = await _contentController.PostSaveBlueprint(contentItem);
        return result;
    }
}

[kasparboelkjeldsen]

@bergmania if you are currently investigating other options, wouldn't you agree we should reopen the issue?

@skttl will you update DTGE with the changes if the needed classes are made public? Otherwise, we are left with having to fork DTGE in order to keep our sites running, or route-hijacking shenanigans.

[leekelleher]

@skttl will you update DTGE with the changes if the needed classes are made public? Otherwise, we are left with having to fork DTGE in order to keep our sites running, or route-hijacking shenanigans.

@kasparboelkjeldsen On a side note, if for any reason that @skttl doesn't want to do this, I do still have admin rights to the DTGE NuGet package, I'd be happy to add new maintainers. e.g. the source code (GitHub repo) could be forked/transferred, but at least we can keep the same DTGE NuGet package for the sanity of the community. 😁 Feel free to reach out to me if this is a route you (or anyone else) want to take.

[skttl]

Do a PR, and I will make it happen :)

[bergmania]

Okay, for V8 we will need to make the following classes public: BlueprintItemBinder, ContentItemBinder, ContentSaveValidationAttribute and FileUploadCleanupFilterAttribute.

Then the following will work:

using Umbraco.Core;
using Umbraco.Core.Cache;
using Umbraco.Core.Configuration;
using Umbraco.Core.Logging;
using Umbraco.Core.Persistence;
using Umbraco.Core.PropertyEditors;
using Umbraco.Core.Scoping;
using Umbraco.Core.Services;
using Umbraco.Web;
using Umbraco.Web.Editors;
using Umbraco.Web.Models.ContentEditing;
using Umbraco.Web.WebApi.Filters;
using Umbraco.Web.Editors.Filters;
using Umbraco.Web.Editors.Binders;
using System.Web.Http.ModelBinding;

namespace ClassLibrary2
{

    [Umbraco.Web.Mvc.PluginController("dtge")]
    [UmbracoApplicationAuthorize(Constants.Applications.Content)]
    public class WhateverController : BackOfficeNotificationsController
    {
        private ContentController _contentController;

        public WhateverController(PropertyEditorCollection propertyEditors, IGlobalSettings globalSettings,
     IUmbracoContextAccessor umbracoContextAccessor, ISqlContext sqlContext, ServiceContext services,
     AppCaches appCaches, IProfilingLogger logger, IRuntimeState runtimeState, UmbracoHelper umbracoHelper,
     IScopeProvider scopeProvider)
     : base(globalSettings, umbracoContextAccessor, sqlContext, services, appCaches, logger, runtimeState, umbracoHelper)
        {
            _contentController = new ContentController(propertyEditors, globalSettings, umbracoContextAccessor, sqlContext, services, appCaches,logger,runtimeState,umbracoHelper,scopeProvider);
        }

        [FileUploadCleanupFilter]
        [ContentSaveValidation(skipUserAccessValidation: true)]
        public ContentItemDisplay PostSaveBlueprint([ModelBinder(typeof(BlueprintItemBinder))] ContentItemSave contentItem)
        {
            return _contentController.PostSaveBlueprint(contentItem);
        }
    }
}

I'll see if we can squeeze in a release in next sprint.

Regarding whether this issue should be reopened, I really do not think so. This is a feature request not a bug in Umbraco. The feature request is covered by https://github.com/umbraco/Umbraco-CMS/discussions/15560.

[kasparboelkjeldsen]

@bergmania wasn't aware of #15560, whether it's a feature or a bug is probably a matter of which side of the fence you're standing on, but I'm happy regardless that we're getting a solution to the problem. Thank you very much. @skttl I'll provide a pull request ASAP, so we hopefully can get a new version out when 8.18.13 drops

[ws-graham]

Looks like we are on the right path with this now.

From what I can see we now have the Umbraco classes public, do we need to update the DTGE NuGet package? If so are @leekelleher or @skttl able do this, or will we need to wait for the release?

[kasparboelkjeldsen]

@skttl I'd make a pullrequest but your repository is archived. I've made a working example here https://github.com/kasparboelkjeldsen/umbraco-doc-type-grid-editor/tree/feature/custom-blueprint-endpoints

[skttl]

Thanks, I've unarchived so you can send the PR :)

[kasparboelkjeldsen]

@ws-graham we've got a pre-release ready, https://www.nuget.org/packages/Our.Umbraco.DocTypeGridEditor/1.3.0-blackknight @skttl much appreciated the help in solving this :)

[aochmann]

@ws-graham we've got a pre-release ready, https://www.nuget.org/packages/Our.Umbraco.DocTypeGridEditor/1.3.0-blackknight @skttl much appreciated the help in solving this :)

Hi @kasparboelkjeldsen and @skttl , great work on the fix. Do we know when it will be fully released (1.3.0)?

Cheers, Adrian

[skttl]

There will be no more releases

[Attila816-dev]

hello, unfortunately the issue still exists in Umbraco version 8.18.13. I've replaced Umbraco DocumentTypeGrid package to 1.3.0-blackknight version but after the upgrade when the user uploads a document he is still redirected to the start page because of 401 Unauthorized response from backoffice/UmbracoApi/Content/PostSaveBluePrint post action.

[niekvanderreest]

I can confirm the issue is fixed in Umbraco 8.18.13 when using version 1.3.0-blackknight