Add UI permissions based on the user role #6823

hishamco · 2020-08-04T01:16:40Z

hishamco · 2020-08-04T08:04:06Z

src/OrchardCore.Modules/OrchardCore.Contents/Views/Content.SaveDraftButton.cshtml

 @if (String.IsNullOrWhiteSpace(returnUrl))
 {
    <button class="primaryAction btn btn-primary" type="submit" name="submit.Save" value="submit.Save">@T["Save Draft"]</button>
 }
 else
 {
    <div class="btn-group">
-        <button class="btn btn-primary" type="submit" name="submit.Save" value="submit.Save">@T["Save Draft"]</button>
-        <button type="button" class="btn btn-primary dropdown-toggle dropdown-toggle-split" data-reference="parent" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">
+        <button class="btn btn-primary @((hasEditOwnContentPermission && contentItem?.Owner == User.Identity.Name) || hasEditContentPermission ? String.Empty : "disabled")" type="submit" name="submit.Save" value="submit.Save">@T["Save Draft"]</button>


I'm not quite sure why the Model.ContentItem is null

deanmarcussen · 2020-08-04T13:04:42Z

@hishamco I'm afraid this is not the correct solution.

The issue, as I explained to you on your last pr related to this, starts as a problem with the content type filter, which is designed to only list content types for which the user has the Edit permission.

The bug, or issue, is that when the filter is finds no editable content types, it is currently falling back to listing all content types.

That is the issue to resolve, and following that the issue would be how could a contributor list their own content.

One of the answers to that, is that they can't, and the role maybe more intended as a permission for contributors contributing via live writer, or other entry points to the system.

Note: This is how it was in O1 as well

hishamco · 2020-08-04T13:22:56Z

The bug, or issue, is that when the filter is finds no editable content types, it is currently falling back to listing all content types.

This is another issue, I started with the issue that raise during the demo of @agriffard last two weeks when he publish a content and got 404, then after checking the roles permissions I realize there is no issue with that, except the UI permissions so that's why I point to that in the original issue

I think we need to change the issue's title to avoid misunderstanding

hishamco · 2020-08-04T13:23:36Z

We need to open another PR for the issue that you mentioned

deanmarcussen · 2020-08-04T14:14:07Z

Ok maybe they are seperate issues.

Could you read the comments in the code on how to check for content permissions. This is not the correct way

hishamco · 2020-08-04T18:45:46Z

Ok maybe they are seperate issues.

Yep, this is another one, I remembered the one that you meant, there is an open PR that needs a revise

Could you read the comments in the code on how to check for content permissions. This is not the correct way

Which comments do you mean? If I'm not wrong AuthorizeAsync() is the way to check the permissions

src/OrchardCore.Modules/OrchardCore.Contents/Views/Content.PublishButton.cshtml

sebastienros · 2020-08-06T18:50:24Z

It's not clear what this PR is trying to fix, it seems unrelated to the issue that is linked. Can you open an issue and describe it?

hishamco · 2020-08-06T19:00:55Z

Sure

src/OrchardCore.Modules/OrchardCore.Contents/Views/Contents-Button-Actions.SummaryAdmin.cshtml

deanmarcussen · 2020-08-13T07:26:48Z

src/OrchardCore.Modules/OrchardCore.Contents/Views/ContentsAdminList-Create.cshtml

 <div class="form-group">
    <div class="btn-group">
        @if (Model.CreatableTypes.Any())
        {
+            ContentItem contentItem = await ContentManager.NewAsync(Model.CreatableTypes?.First().Value);
+            var hasEditContentPermission = await AuthorizationService.AuthorizeAsync(User, OrchardCore.Contents.Permissions.EditContent, contentItem);


this check should have already happened in the controller, and it shouldn't be in the CreatableTypes list.

If it isn't happening in the controller it should.

It would also have to happen on every type, not just the first one

Seems the check was there before I added my changes, I will remove it then

The list of CreatableTypes should just not contain the ones you can't create. And even that is a minor issue IMO if that complexifies the code too much (or make it too slow).

This code you've added needs to be in the controller, or not at all

@deanmarcussen AFAIK when the permission is not applied in the controller we returns Forbid, so in this case how can I check the permission If I move it to the controller?

See where the list is being created

OrchardCore/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs

Line 126 in c8e7ea5

creatableList.Add(new SelectListItem(new LocalizedString(contentTypeDefinition.DisplayName, contentTypeDefinition.DisplayName).Value, contentTypeDefinition.Name));

OrchardCore/src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs

Line 163 in c8e7ea5

var authorized = await _authorizationService.AuthorizeAsync(User, Permissions.EditContent, await _contentManager.NewAsync(ctd.Name));

I didn't dig into the code before, it's seems the check is already there, so I remove the one in the view

src/OrchardCore.Modules/OrchardCore.Contents/Views/Contents-Button-Actions.SummaryAdmin.cshtml

src/OrchardCore.Modules/OrchardCore.Contents/Views/Content.SummaryAdmin.cshtml

deanmarcussen · 2020-08-20T08:52:04Z

src/OrchardCore.Modules/OrchardCore.Contents/Views/Contents-Button-Edit.SummaryAdmin.cshtml

+@if (hasPublished)
+{
+    <a display-for="@contentItem" class="btn btn-success btn-sm" itemprop="">@T["View"]</a>
+}


Strange!! let me check again ...

Nah!! the condition get copied twice during merge conflict

Skrypt · 2020-09-17T23:14:23Z

src/OrchardCore.Modules/OrchardCore.Contents/Views/Content.PublishButton.cshtml

    var returnUrl = Context.Request.Query["returnUrl"];
+    var hasPublishContentPermission = await AuthorizationService.AuthorizeAsync(User, OrchardCore.Contents.Permissions.PublishContent, contentItem);


~~PublishOwnContent. These permissions are not taking in consideration when we want to use PublishOwn or EditOwn they need to be changed.~~

These permissions are not taking in consideration when we want to use PublishOwn or EditOwn

Why?

@Skrypt code is good here I think. a PublishContent permission request is morphed to PublishOwn. You shouldn't request PublishOwn directly (see comments in CommonPermissions)

It doesn't work if you have set a PublishOwn_ContentTypeName permission which is dynamic. So the issue is that the PR makes it impossible to have a role that has Publish/Edit/Delete own permission on a single custom content type. I have a PR coming up.

See https://github.com/OrchardCMS/OrchardCore/blob/dev/src/OrchardCore.Modules/OrchardCore.Contents/Security/ContentTypeAuthorizationHandler.cs#L58

jptissot · 2020-09-18T09:39:11Z

src/OrchardCore.Modules/OrchardCore.Contents/Views/Content.SummaryAdmin.cshtml

@@ -57,3 +57,15 @@
 {
    <div class="col primary">@await DisplayAsync(Model.Content)</div>
 }
+
+<script at="Foot" type="text/javascript">


Why not use a named script with a class selector ? Instead of having tons of instances of this code on the page ?

@netwavebe already fixed it @jptissot exactly as you suggested.

Ah cool, I did not realize this was an old pr :)

It got merged to dev a few days ago. See #7070

Thanks @netwavebe :)

chinasqzl · 2020-09-22T03:02:07Z

ContentsDriver.Edit
Dynamic("Content_PublishButton").Location("Actions:10")
results.Add(Dynamic("Content_SaveDraftButton").Location("Actions:20"));

ContentItem contentItem = Model.ContentItem;
var hasPublishContentPermission = await AuthorizationService.AuthorizeAsync(User, OrchardCore.Contents.Permissions.PublishContent, contentItem);

Model.ContentItem is null

hishamco · 2020-09-22T10:04:40Z

@Skrypt can you please check this in your PR?

Skrypt · 2020-09-22T17:09:05Z

The ContentItem passed to the ContentTypeAuthorizationHandler can sometimes be null but in this case here it's not supposed to.

In the ContentsDriver it assumes that it renders a ContentItem already. If the ContentItem == null then the ContentTypeAuthorizationHandler will simply return the previous Auth result without processing anything further. I looked last night and ContentTypeAuthorizationHandler is fine. Though here if the ContentItem == null then the ContentDriver should throw an exception and it's a different issue I think.

chinasqzl · 2020-09-29T06:29:29Z

@Skrypt

Skrypt · 2020-09-29T17:20:31Z

Yes please PR has moved to #7104 now.
Here the issue comes from the ContentTypeAuthorizationHandler if you are not seeing the proper buttons.
This commit should fix your issue : 7908293#diff-f32a3f59eaeef643f8774e25b903ff08R48
Though, I tested everything in #7104

hishamco · 2020-09-29T17:32:41Z

Is it regression bug or something else related?

Skrypt · 2020-09-29T17:43:32Z

This PR fixes one part of the issues. I think we fixed both the same issues in 2 different PR's. Mine is going one step further by introducing 2 new permissions for the admin UI. I'm not sure it's a regression. I think the issue was there since a while and adding the permissions on the buttons just revealed the underlying issue.

hishamco marked this pull request as ready for review August 4, 2020 08:02

hishamco commented Aug 4, 2020

View reviewed changes

deanmarcussen reviewed Aug 5, 2020

View reviewed changes

src/OrchardCore.Modules/OrchardCore.Contents/Views/Content.PublishButton.cshtml Outdated Show resolved Hide resolved

deanmarcussen requested changes Aug 13, 2020

View reviewed changes

sebastienros reviewed Aug 13, 2020

View reviewed changes

src/OrchardCore.Modules/OrchardCore.Contents/Views/Contents-Button-Actions.SummaryAdmin.cshtml Outdated Show resolved Hide resolved

hishamco requested a review from deanmarcussen August 17, 2020 08:37

deanmarcussen reviewed Aug 17, 2020

View reviewed changes

src/OrchardCore.Modules/OrchardCore.Contents/Views/Content.SummaryAdmin.cshtml Outdated Show resolved Hide resolved

hishamco force-pushed the hishamco/ui-permissions branch from b43c294 to a637029 Compare August 17, 2020 10:27

hishamco added 2 commits August 17, 2020 13:36

Add UI permissions based on the user role

250bfb9

Add missing bracket

0a06083

hishamco force-pushed the hishamco/ui-permissions branch from a637029 to 0a06083 Compare August 17, 2020 21:05

Remove unnecessary check

5152b5e

deanmarcussen reviewed Aug 20, 2020

View reviewed changes

Remove unnecessary condition due merge conflict

cd9b537

deanmarcussen approved these changes Aug 27, 2020

View reviewed changes

agriffard merged commit 3c47ed2 into dev Aug 27, 2020

delete-merged-branch bot deleted the hishamco/ui-permissions branch August 27, 2020 12:15

Skrypt reviewed Sep 17, 2020

View reviewed changes

jptissot reviewed Sep 18, 2020

View reviewed changes

Skrypt mentioned this pull request Sep 19, 2020

Admin UI Permissions adjustments for EditOwnContent on a specific content type #7104

Merged

Skrypt added a commit that referenced this pull request Nov 2, 2020

Revert changes from #6823

c1ad7f2

		var returnUrl = Context.Request.Query["returnUrl"];
		var hasPublishContentPermission = await AuthorizationService.AuthorizeAsync(User, OrchardCore.Contents.Permissions.PublishContent, contentItem);

Add UI permissions based on the user role #6823

Add UI permissions based on the user role #6823

Conversation

hishamco commented Aug 4, 2020 • edited Loading

Choose a reason for hiding this comment

deanmarcussen commented Aug 4, 2020

hishamco commented Aug 4, 2020

hishamco commented Aug 4, 2020

deanmarcussen commented Aug 4, 2020

hishamco commented Aug 4, 2020

sebastienros commented Aug 6, 2020

hishamco commented Aug 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skrypt Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skrypt Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chinasqzl commented Sep 22, 2020

hishamco commented Sep 22, 2020

Skrypt commented Sep 22, 2020

chinasqzl commented Sep 29, 2020

Skrypt commented Sep 29, 2020

hishamco commented Sep 29, 2020

Skrypt commented Sep 29, 2020

hishamco commented Aug 4, 2020 •

edited

Loading

Skrypt Sep 17, 2020 •

edited

Loading

Skrypt Sep 18, 2020 •

edited

Loading