Ignore user start nodes option in pickers #2441

dnwhte · 2018-02-09T22:51:57Z

This addresses the feature request at http://issues.umbraco.org/issue/U4-10147.

List of pickers supported:

Content Picker List View currently has an issue. Umbraco's user permissions do not work on the content picker list view ATM. http://issues.umbraco.org/issue/U4-10876

Update:
Issue preventing content picker list view from working is https://issues.umbraco.org/issue/U4-11488.

Results in a very basic working POC for the content picker

This reverts commit 2c6b2f0.

This reverts commit 53609dd.

…d rte

… permissions

Has to refactor how the prevalue was added to the RTE config so the grid could have a separate config.

This way we don't have to unknowingly add the canceler to the media request

…ssues

emmaburstow · 2018-06-25T19:40:39Z

Hi @danwhite85

Thanks for this PR! I've already started testing and am really pleased with what it does. I'll have a play this evening and let you know if there's anything I need from you at all. It's a really useful change.

Emma, Community Pull Request team

emmaburstow

I've tested this with each of the pickers you have put in the chart - it all works nicely! I've had a good look through the code too and I'm happy with it for my part.

I really like this feature. Excellent work!

Emma

nul800sebastiaan · 2019-03-23T11:51:48Z

Verbiage changed and I've redone all the testing, I think it all works as expected now. Which means...

PARTY! 🎉 🍰 🍻 🍾

Merged and ready to go in 7.15. 👍 🏅

Now I will have to do a v8 merge, that's going to be fun. 😉

Thanks so much @dnwhte for your patience and for the great job!

dnwhte · 2019-03-25T16:46:59Z

Woohoo! This is awesome. Thanks a lot for your time and work, @nul800sebastiaan!

nul800sebastiaan · 2019-04-17T12:47:00Z

This has now also been merged up to v8 🥵🥵😅

emmaburstow · 2019-04-17T12:52:47Z

Yay! Really chuffed about this one 👍

Shazwazza

I have added notes to the code. I'm not quite comfortable with some of these changes since it exposes the ability for users to request data they don't have access to outside of pickers or regardless of if pickers are configured for this behaviour. Perhaps it's not such a big deal that people get access to some of this data, but we don't know what type of data people have in their sites so i think it's naive to assume that it's ok to expose this as easily as passing in a query string?
thoughts?

Shazwazza · 2019-06-13T09:46:39Z

src/Umbraco.Web/WebApi/Filters/FilterAllowedOutgoingMediaAttribute.cs

@@ -77,7 +79,12 @@ public override void OnActionExecuted(HttpActionExecutedContext actionExecutedCo

        protected virtual void FilterItems(IUser user, IList items)
        {
-            FilterBasedOnStartNode(items, user);
+            bool.TryParse(HttpContext.Current.Request.QueryString.Get(TreeQueryStringParameters.IgnoreUserStartNodes), out var ignoreUserStartNodes);


HttpContext.Current should not be used here, you can get query string values from the action's HttpActionExecutedContext

Adding this logic here means that any action or controller that uses this filter will have the filter bypassed simply if a user adds this query string, this will have unintended side affects since you don't know where this filter might be used. I think we need to reconsider how this is put together.

Shazwazza · 2019-06-13T09:48:59Z

src/Umbraco.Web/Editors/MediaController.cs

@@ -264,11 +264,12 @@ protected int[] UserStartNodes
            string orderBy = "SortOrder",
            Direction orderDirection = Direction.Ascending,
            bool orderBySystemField = true,
-            string filter = "")
+            string filter = "",
+            bool ignoreUserStartNodes = false)


All of the changes to these controllers that now have a ignoreUserStartNodes parameter means that anyone logged into the back office can just make a request to these controllers and set this to true which will give them the data that they might not have access to.

So now, even if a site doesn't have any data type with this feature enabled now exposes the possibility for users to request data they don't have access to, or shouldn't have access to.

Shazwazza · 2019-06-13T09:55:03Z

src/Umbraco.Web/Trees/ContentTreeControllerBase.cs

@@ -190,7 +190,7 @@ private int GetTopNodeId(IUmbracoEntity entity)

        protected abstract UmbracoObjectTypes UmbracoObjectType { get; }

-        protected IEnumerable<IUmbracoEntity> GetChildEntities(string id)
+        protected IEnumerable<IUmbracoEntity> GetChildEntities(string id, FormDataCollection queryStrings)


Changing protected members is an API breaking change, there might be a few of these in this PR that we might need to consider and/or add the breaking change tag for.

Ah shoot, I fixed all the public methods but didn't notice the protected ones. Will have a look.

dnwhte · 2019-06-13T17:14:20Z

@Shazwazza just for reference. In Jan 2018 we vaguely discussed some of these things (querystrings, and providing on context to FilterAllowedOutgoingMediaAttribute) (https://issues.umbraco.org/issue/U4-10147#comment=67-43737).

I agree with your concerns. Given your response in 2018, I'm not sure how to go about it without a querystring. It may not be much better, but what if instead of passing "ignoreUserStartNodes" as a querystring we pass the picker id and check the picker's settings in the handler?

Thanks for the feedback.

Shazwazza · 2019-06-14T00:52:19Z

Hi @dnwhte,

There's a bunch of code in this PR so I'm unsure how exactly it all is glue'd together but the main things that we cannot do is : bypass security on ContentController or MediaController. I'm unsure why these controllers are being requested for pickers, perhaps it's an issue with Umbraco in general, for example i can see in the code that mediaResource is being called for the mediaPicker controller by why? I mean surely we can get the children from the entityResource (this would also improve performance). Similarly contentResource is being called by linkPicker controller, but again why? This should also be using entityResource. ... if neither of these is 'possible' then we should re-think how we can make this possible.

Pickers should in theory only use EntityController. In this case the risk of exposing data that the user doesn't have access to is more minimal since an Entity doesn't contain much information ... well that's actually not entirely true in v7. In v7 and 8.0.x, The EntityController for media will return all media properties too which we cannot do. In 8.1 this will be fixed.

In my opinion, to implement this while maintaining reasonable security we would need to:

Trees

Don't use a query string like ignoreUserStartNodes and instead for pickers that are using the tree's, yes, please pass in the data type ID, then check if the tree is currently in dialog mode, then check if a data type id has been passed in, then lookup that data type, check if it is a type that has config like ignoreUserStartNodes, then check if that is allowed and proceed as necessary

Entity data

Unfortunately this isn't going to be the most straightforward thing to do due to how the EntityController and EntityService work pre 8.1 since it returns more data than we should be allowed to return if bypassing security. So a decision will need to be made - do we postpone this feature for 8.1+ or do we proceed with the below requirements?

First we need to ensure that security cannot be bypassed at all on MediaController/ContentController. Then we need to make sure that mediaResource and contentResource are not used for anything with that changes required for this feature - such as linkPicker and mediaPicker and instead determine if entityResource can be used instead and if not then document why so we can figure out how to make that work.

If we are going to proceed with this feature in a 7.x version, then various things will need to be done:

We will need to back port the EntityService changes done in 8.1 so that simple media can be looked up with it's media path (v8 EntityService changes #3498, V8/bugfix/3498 entity service updates #5658)
We will need to add another method to the EntityController like GetMediaChildren which will use the new EntityService method ensuring that the bare minimum media data is returned and in turn update the entityResource and mediaPicker to use this endpoint. This endpoint will work like the trees (see above) where you can request children on behalf of a picker by optionally passing in the picker Id and doing the checks like in trees

Result

The end result of this is that an authenticated user could still request data that they don't have access to given that they know a valid Data Type picker Id that has this option configured by making requests either to the content/media tree or to the entityResource.getById or the new entityResource.getMediaChildren (if we proceed with a 7.x version, else if we only target 8.1 then entityResource.getChildren will work). However the data returned will be a minimal subset of data for these entities which don't expose any property values. This also means that unless a Data Type is configured to bypass security there is no way that a user can simply request security to be bypassed.

dnwhte · 2019-06-17T23:15:10Z

Hi @Shazwazza -

I'm currently working on a v7 project for which this feature is required, so I'm willing to help however I can.

From what I can tell, the mediaPicker and linkPicker use the content/media resources in order to get extra properties that are not available with the entityResource. Media picker needs images and link picker needs the url and the properties array to pass to the tinymce service.

I'm not too familiar with the v8 codebase, but giving it a quick scan I'm assuming the general approach to do this would be to use entityResource and then supplement the data with mediaHelper and contentEditingHelper. Is this correct?

It looks like the media picker could go without the media resource by using mediaHelper.resolveFileFromEntity as seen here.

In order to not use the contentResource in linkPicker more data would probably need to be added to metaData in the entityResource response or additional helpers in contentEditingHelper. The linkPicker in v8 still uses the contentResource.

How would you like to proceed? I could start refactoring the ignoreUserStartNodes querystring to use the data type id instead.

Thanks again for your time.

This reverts commit cc9a7ff. # Conflicts: # src/Umbraco.Web/Editors/ContentController.cs # src/Umbraco.Web/Editors/EntityController.cs # src/Umbraco.Web/Search/UmbracoTreeSearcher.cs

)"" This reverts commit 994c6ee

nul800sebastiaan · 2019-07-10T06:24:12Z

Tagged as breaking since in in change 966b07b IUmbracoEntity GetByKey(Guid key, bool loadBaseType = true); has been changed to IUmbracoEntity GetByKey(Guid key);.

PeteDuncanson · 2019-07-11T13:43:05Z

@nul800sebastiaan I'm a little confused by the last few comments on this one. Have @Shazwazza concerns about security be addressed/implemented or are they shipped as it in the latest v7.15 and v8.1 releases? What state is this now in?

Shazwazza · 2019-07-11T13:45:20Z

@PeteDuncanson yes the concerns were addressed and much had to be refactored. Many pickers had to be adjusted to use entityResource (which is what they should have been using anyways) and you cannot request data that you don't have access to unless you have a data type configured to allow that scenario, the data returned is minimal (i.e. entityResource results)

Dan White and others added 28 commits January 26, 2018 17:07

Start POC with content picker

0e53822

Add BypassUserPermissions checks in ContentTreeControllerBase

277d472

Results in a very basic working POC for the content picker

Remove uneeded check for bypassing

7bad157

Add Bypass field to MNTP editor

b606b91

Fallback to -1 if $scope.model.config.startNode.id is not set

2c6b2f0

Start testing related links

47b2c59

Refactor treepicker.initTree() a bit and POC for related links

4999c70

Revert "Fallback to -1 if $scope.model.config.startNode.id is not set"

72d17c4

This reverts commit 2c6b2f0.

Add prevalue field to RTE

53609dd

POC for RTE

34fbc2d

Revert "Add prevalue field to RTE"

70eb532

This reverts commit 53609dd.

Use rte.prevalues.html for setting Bypass to make compatible with gri…

907d77d

…d rte

Add bypass prevalue to Media Picker

6fc28e9

Basic POC for media picker

89e9712

Maintain folderlocking and breadcrumbs in media picker when bypassing…

12aef65

… permissions

POC for content picker search

6630ac0

POC for media picker search

67ca032

POC for search in MNTP media

c8f816a

POC for related links search

a5f178d

POC for RTE content link search

8466ba1

POC for RTE media link picker

a847897

POC for RTE and Grid RTE media pickers

0427073

Has to refactor how the prevalue was added to the RTE config so the grid could have a separate config.

Reword prevalue description

2cfce6b

Refactor to support options object in searchService and entityResource

c30e435

This way we don't have to unknowingly add the canceler to the media request

Remove unnecessary check

ce59916

Get usersStartNodes before kicking everything off, so we don't have i…

ea24649

…ssues

Merge branch 'dev-v7' into U4-10147

335843b

Merge branch 'dev-v7' into U4-10147

12545f0

emmaburstow approved these changes Jun 25, 2018

View reviewed changes

nul800sebastiaan added 4 commits March 23, 2019 10:46

Rename all "bypass" references to "Ignore user start nodes"

9d1d313

Some more updates to wording and a few code style changes

947f889

grammar

4836a10

Make sure search still works

abee0fa

nul800sebastiaan merged commit cc9a7ff into umbraco:dev-v7 Mar 23, 2019

nul800sebastiaan removed the state/hq-discussion-cms label Mar 23, 2019

nul800sebastiaan added community/pr type/feature release/7.15.0 labels Mar 23, 2019

zpqrtbnk mentioned this pull request Mar 25, 2019

Merge v7 to v8 #5061

Closed

nul800sebastiaan added the release/8.1.0 label Apr 17, 2019

nul800sebastiaan mentioned this pull request Apr 29, 2019

Media Picker UX: Quickly pick most recently uploaded media #5260

Closed

Shazwazza reviewed Jun 13, 2019

View reviewed changes

bergmania added a commit that referenced this pull request Jun 20, 2019

Revert "Revert "U4 10147 - Bypass User Security option in pickers (#2441

1dd2c1c

)"" This reverts commit 994c6ee

nul800sebastiaan changed the title ~~U4 10147 - Bypass User Security option in pickers~~ Ignore user start nodes option in pickers Jul 3, 2019

nul800sebastiaan added the category/breaking label Jul 10, 2019

nul800sebastiaan mentioned this pull request Jul 10, 2019

Change to Entity Service GetByKey isn't breaking in 7.15 #5822

Closed

This was referenced Aug 13, 2019

Media thumbnails not shown after 7.14.0 -> 7.15.0 upgrade #5847

Closed

7.11.1 Navigation of restricted media content with common ancestor #2999

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore user start nodes option in pickers #2441

Ignore user start nodes option in pickers #2441

dnwhte commented Feb 9, 2018 •

edited

Loading

emmaburstow commented Jun 25, 2018

emmaburstow left a comment

nul800sebastiaan commented Mar 23, 2019

dnwhte commented Mar 25, 2019

nul800sebastiaan commented Apr 17, 2019

emmaburstow commented Apr 17, 2019

Shazwazza left a comment

Shazwazza Jun 13, 2019

Shazwazza Jun 13, 2019

Shazwazza Jun 13, 2019

Shazwazza Jun 13, 2019

nul800sebastiaan Jun 13, 2019

dnwhte commented Jun 13, 2019 •

edited

Loading

Shazwazza commented Jun 14, 2019

dnwhte commented Jun 17, 2019

nul800sebastiaan commented Jul 10, 2019

PeteDuncanson commented Jul 11, 2019

Shazwazza commented Jul 11, 2019

Ignore user start nodes option in pickers #2441

Ignore user start nodes option in pickers #2441

Conversation

dnwhte commented Feb 9, 2018 • edited Loading

emmaburstow commented Jun 25, 2018

emmaburstow left a comment

Choose a reason for hiding this comment

nul800sebastiaan commented Mar 23, 2019

dnwhte commented Mar 25, 2019

nul800sebastiaan commented Apr 17, 2019

emmaburstow commented Apr 17, 2019

Shazwazza left a comment

Choose a reason for hiding this comment

Shazwazza Jun 13, 2019

Choose a reason for hiding this comment

Shazwazza Jun 13, 2019

Choose a reason for hiding this comment

Shazwazza Jun 13, 2019

Choose a reason for hiding this comment

Shazwazza Jun 13, 2019

Choose a reason for hiding this comment

nul800sebastiaan Jun 13, 2019

Choose a reason for hiding this comment

dnwhte commented Jun 13, 2019 • edited Loading

Shazwazza commented Jun 14, 2019

Trees

Entity data

Result

dnwhte commented Jun 17, 2019

nul800sebastiaan commented Jul 10, 2019

PeteDuncanson commented Jul 11, 2019

Shazwazza commented Jul 11, 2019

dnwhte commented Feb 9, 2018 •

edited

Loading

dnwhte commented Jun 13, 2019 •

edited

Loading