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.

Sign up for GitHub

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

Mark duplicate permissions as Obsolete #16176

Merged
merged 7 commits into from
May 29, 2024
Merged

Mark duplicate permissions as Obsolete #16176

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2f83c14
Mark duplicate permissions as Obsolete
MikeAlhayek May 27, 2024
43fba18
address all warnings
MikeAlhayek May 28, 2024
57a148a
fix a missed warning
MikeAlhayek May 28, 2024
a6224ab
Merge branch 'main' into ma/obsolete-duplicate-permissions
MikeAlhayek May 28, 2024
b72bd16
Update permissions
MikeAlhayek May 28, 2024
e58540f
fix build
MikeAlhayek May 28, 2024
fb1c5f1
Removing mistaken "property"
Piedone May 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/OrchardCore.Modules/OrchardCore.Apis.GraphQL/AdminMenu.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public Task BuildNavigationAsync(string name, NavigationBuilder builder)
.Add(S["Configuration"], configuration => configuration
.Add(S["GraphiQL"], S["GraphiQL"].PrefixPosition(), graphiQL => graphiQL
.Action("Index", "Admin", "OrchardCore.Apis.GraphQL")
.Permission(Permissions.ExecuteGraphQL)
.Permission(CommonPermissions.ExecuteGraphQL)
.LocalNav()
)
);
Expand Down
2 changes: 1 addition & 1 deletion src/OrchardCore.Modules/OrchardCore.Apis.GraphQL/GraphQLMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public async Task InvokeAsync(HttpContext context, RequestDelegate next)
context.User = authenticateResult.Principal;
}
var authorizationService = context.RequestServices.GetService<IAuthorizationService>();
var authorized = await authorizationService.AuthorizeAsync(context.User, Permissions.ExecuteGraphQL);
var authorized = await authorizationService.AuthorizeAsync(context.User, CommonPermissions.ExecuteGraphQL);

if (authorized)
{
Expand Down
11 changes: 8 additions & 3 deletions src/OrchardCore.Modules/OrchardCore.Apis.GraphQL/Permissions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System;
using System.Collections.Generic;
using System.Threading.Tasks;
using OrchardCore.Security.Permissions;
Expand All @@ -6,13 +7,17 @@ namespace OrchardCore.Apis.GraphQL;

public class Permissions : IPermissionProvider
{

[Obsolete("This property will be removed in future release. Instead use 'CommonPermissions.ExecuteGraphQLMutations'.")]
Copy link
Member

Choose a reason for hiding this comment

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

These are not properties, but fields. And more importantly, a reference to the project/package name would help those consumers who don't have the whole of OC referenced:

Suggested change
[Obsolete("This property will be removed in future release. Instead use 'CommonPermissions.ExecuteGraphQLMutations'.")]
[Obsolete("This field will be removed in a future release. Instead use 'CommonPermissions.ExecuteGraphQLMutations' from OrchardCore.Apis.GraphQL.Abstractions.")]

Especially because there are many CommonPermissions classes.

Same for all the others.

Copy link
Member Author

Choose a reason for hiding this comment

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

field is used when you are declaring a private property. a property when it is public. + Does it really matter? I ask this because these changes consume time for the PR creator to do. During a review, we should ask yourself these questions so that the PR creator does not have to spent lots of time doing changes that add no value. At the end of the day, we want to merge acceptable PR without burden the creators with lots of changes "when possible".

Copy link
Member

Choose a reason for hiding this comment

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

A field and a property are different language features, and this one here is a field. Both can be public and private (or else) too. I'm only pointing this out because it specified "property"; if it were "This will be removed..." or "This member will be removed.." (which are all alternatives BTW) I wouldn't ask for it to include "field".

You can do a search and replace for this and fix it in seconds though, perhaps less than what it takes to debate this :).

Note the more important second part of my comment about the package though.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not about the debate here. The issues is that we need to think about the value of the comment if it out weight the effort. Many PR end up consuming lots of man hours from (reviewers and creator) due to this type of comment.

I have added the namespaces to the message for developers.

Copy link
Member

Choose a reason for hiding this comment

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

If it's not about this instance here, then let's take this conversation offline.

public static readonly Permission ExecuteGraphQLMutations = CommonPermissions.ExecuteGraphQLMutations;

[Obsolete("This property will be removed in future release. Instead use 'CommonPermissions.ExecuteGraphQL'.")]
public static readonly Permission ExecuteGraphQL = CommonPermissions.ExecuteGraphQL;

private readonly IEnumerable<Permission> _allPermissions =
[
ExecuteGraphQL,
ExecuteGraphQLMutations,
CommonPermissions.ExecuteGraphQL,
CommonPermissions.ExecuteGraphQLMutations,
];

public Task<IEnumerable<Permission>> GetPermissionsAsync()
Expand All @@ -25,7 +30,7 @@ public IEnumerable<PermissionStereotype> GetDefaultStereotypes() =>
Name = OrchardCoreConstants.Roles.Administrator,
Permissions =
[
ExecuteGraphQLMutations,
CommonPermissions.ExecuteGraphQLMutations,
],
},
];
Expand Down
2 changes: 1 addition & 1 deletion ...Core.Modules/OrchardCore.Apis.GraphQL/ValidationRules/RequiresPermissionValidationRule.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public async ValueTask<INodeVisitor> ValidateAsync(ValidationContext validationC

private async Task AuthorizeOperationAsync(ASTNode node, ValidationContext validationContext, GraphQLUserContext userContext, OperationType? operationType, string operationName)
{
if (operationType == OperationType.Mutation && !(await _authorizationService.AuthorizeAsync(userContext.User, Permissions.ExecuteGraphQLMutations)))
if (operationType == OperationType.Mutation && !(await _authorizationService.AuthorizeAsync(userContext.User, CommonPermissions.ExecuteGraphQLMutations)))
{
validationContext.ReportError(new ValidationError(
validationContext.Document.Source,
Expand Down
8 changes: 6 additions & 2 deletions src/OrchardCore.Modules/OrchardCore.AuditTrail/Permissions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System;
using System.Collections.Generic;
using System.Threading.Tasks;
using OrchardCore.Security.Permissions;
Expand All @@ -6,13 +7,16 @@ namespace OrchardCore.AuditTrail;

public class Permissions : IPermissionProvider
{
[Obsolete("This property will be removed in future release. Instead use 'AuditTrailPermissions.ViewAuditTrail'.")]
public static readonly Permission ViewAuditTrail = AuditTrailPermissions.ViewAuditTrail;

[Obsolete("This property will be removed in future release. Instead use 'AuditTrailPermissions.ManageAuditTrailSettings'.")]
public static readonly Permission ManageAuditTrailSettings = AuditTrailPermissions.ManageAuditTrailSettings;

private readonly IEnumerable<Permission> _allPermissions =
[
ViewAuditTrail,
ManageAuditTrailSettings,
AuditTrailPermissions.ViewAuditTrail,
AuditTrailPermissions.ManageAuditTrailSettings,
];

public Task<IEnumerable<Permission>> GetPermissionsAsync()
Expand Down
2 changes: 1 addition & 1 deletion src/OrchardCore.Modules/OrchardCore.Contents/AdminMenu.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ await content.AddAsync(S["Content Items"], S["Content Items"].PrefixPosition(),
{
if (!await _authorizationService.AuthorizeContentTypeDefinitionsAsync(context.User, CommonPermissions.ListContent, contentTypes, _contentManager))
{
contentItems.Permission(Permissions.ListContent);
contentItems.Permission(CommonPermissions.ListContent);
}

contentItems.Action(nameof(AdminController.List), typeof(AdminController).ControllerName(), _routeValues);
Expand Down
2 changes: 1 addition & 1 deletion src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ public async Task<IActionResult> List(
new SelectListItem(S["All versions"], nameof(ContentsStatus.AllVersions)),
];

if (await IsAuthorizedAsync(Permissions.ListContent))
if (await IsAuthorizedAsync(CommonPermissions.ListContent))
{
options.ContentStatuses.Insert(1, new SelectListItem(S["Owned by me"], nameof(ContentsStatus.Owner)));
}
Expand Down
101 changes: 65 additions & 36 deletions src/OrchardCore.Modules/OrchardCore.Contents/Permissions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System;
using System.Collections.Generic;
using System.Threading.Tasks;
using OrchardCore.Security.Permissions;
Expand All @@ -11,44 +12,72 @@ public class Permissions : IPermissionProvider

// EditOwn is the permission that is ultimately required to create new content. See how the Create() method is implemented in the AdminController.

[Obsolete("This property will be removed in future release. Instead use 'CommonPermissions.PublishContent'.")]
public static readonly Permission PublishContent = CommonPermissions.PublishContent;

[Obsolete("This property will be removed in future release. Instead use 'CommonPermissions.PublishOwnContent'.")]
public static readonly Permission PublishOwnContent = CommonPermissions.PublishOwnContent;

[Obsolete("This property will be removed in future release. Instead use 'CommonPermissions.EditContent'.")]
public static readonly Permission EditContent = CommonPermissions.EditContent;

[Obsolete("This property will be removed in future release. Instead use 'CommonPermissions.EditOwnContent'.")]
public static readonly Permission EditOwnContent = CommonPermissions.EditOwnContent;

[Obsolete("This property will be removed in future release. Instead use 'CommonPermissions.DeleteContent'.")]
public static readonly Permission DeleteContent = CommonPermissions.DeleteContent;

[Obsolete("This property will be removed in future release. Instead use 'CommonPermissions.DeleteOwnContent'.")]
public static readonly Permission DeleteOwnContent = CommonPermissions.DeleteOwnContent;

[Obsolete("This property will be removed in future release. Instead use 'CommonPermissions.ViewContent'.")]
public static readonly Permission ViewContent = CommonPermissions.ViewContent;

[Obsolete("This property will be removed in future release. Instead use 'CommonPermissions.ViewOwnContent'.")]
public static readonly Permission ViewOwnContent = CommonPermissions.ViewOwnContent;

[Obsolete("This property will be removed in future release. Instead use 'CommonPermissions.PreviewContent'.")]
public static readonly Permission PreviewContent = CommonPermissions.PreviewContent;

[Obsolete("This property will be removed in future release. Instead use 'CommonPermissions.PreviewOwnContent'.")]
public static readonly Permission PreviewOwnContent = CommonPermissions.PreviewOwnContent;

[Obsolete("This property will be removed in future release. Instead use 'CommonPermissions.CloneContent'.")]
public static readonly Permission CloneContent = CommonPermissions.CloneContent;

[Obsolete("This property will be removed in future release. Instead use 'CommonPermissions.CloneOwnContent'.")]
public static readonly Permission CloneOwnContent = CommonPermissions.CloneOwnContent;

[Obsolete("This property will be removed in future release. Instead use 'CommonPermissions.ListContent'.")]
public static readonly Permission ListContent = CommonPermissions.ListContent;

[Obsolete("This property will be removed in future release. Instead use 'CommonPermissions.EditContentOwner'.")]
public static readonly Permission EditContentOwner = CommonPermissions.EditContentOwner;

public static readonly Permission AccessContentApi = new("AccessContentApi", "Access content via the api");

private readonly IEnumerable<Permission> _readerPermissions =
[
ViewContent,
CommonPermissions.ViewContent,
];

private readonly IEnumerable<Permission> _allPermissions =
[
EditContent,
EditOwnContent,
PublishContent,
PublishOwnContent,
DeleteContent,
DeleteOwnContent,
ViewContent,
ViewOwnContent,
PreviewContent,
PreviewOwnContent,
CloneContent,
CloneOwnContent,
CommonPermissions.EditContent,
CommonPermissions.EditOwnContent,
CommonPermissions.PublishContent,
CommonPermissions.PublishOwnContent,
CommonPermissions.DeleteContent,
CommonPermissions.DeleteOwnContent,
CommonPermissions.ViewContent,
CommonPermissions.ViewOwnContent,
CommonPermissions.PreviewContent,
CommonPermissions.PreviewOwnContent,
CommonPermissions.CloneContent,
CommonPermissions.CloneOwnContent,
AccessContentApi,
ListContent,
EditContentOwner,
CommonPermissions.ListContent,
CommonPermissions.EditContentOwner,
];

public Task<IEnumerable<Permission>> GetPermissionsAsync()
Expand All @@ -61,49 +90,49 @@ public IEnumerable<PermissionStereotype> GetDefaultStereotypes() =>
Name = OrchardCoreConstants.Roles.Administrator,
Permissions =
[
PublishContent,
EditContent,
DeleteContent,
PreviewContent,
CloneContent,
CommonPermissions.PublishContent,
CommonPermissions.EditContent,
CommonPermissions.DeleteContent,
CommonPermissions.PreviewContent,
CommonPermissions.CloneContent,
AccessContentApi,
ListContent,
EditContentOwner,
CommonPermissions.ListContent,
CommonPermissions.EditContentOwner,
],
},
new PermissionStereotype
{
Name = OrchardCoreConstants.Roles.Editor,
Permissions =
[
PublishContent,
EditContent,
DeleteContent,
PreviewContent,
CloneContent,
ListContent,
CommonPermissions.PublishContent,
CommonPermissions.EditContent,
CommonPermissions.DeleteContent,
CommonPermissions.PreviewContent,
CommonPermissions.CloneContent,
CommonPermissions.ListContent,
],
},
new PermissionStereotype
{
Name = OrchardCoreConstants.Roles.Author,
Permissions =
[
PublishOwnContent,
EditOwnContent,
DeleteOwnContent,
PreviewOwnContent,
CloneOwnContent,
CommonPermissions.PublishOwnContent,
CommonPermissions.EditOwnContent,
CommonPermissions.DeleteOwnContent,
CommonPermissions.PreviewOwnContent,
CommonPermissions.CloneOwnContent,
],
},
new PermissionStereotype
{
Name = OrchardCoreConstants.Roles.Contributor,
Permissions =
[
EditOwnContent,
PreviewOwnContent,
CloneOwnContent,
CommonPermissions.EditOwnContent,
CommonPermissions.PreviewOwnContent,
CommonPermissions.CloneOwnContent,
],
},
new PermissionStereotype
Expand Down
6 changes: 3 additions & 3 deletions src/OrchardCore.Modules/OrchardCore.Deployment/AdminMenu.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,17 @@ public Task BuildNavigationAsync(string name, NavigationBuilder builder)
.Add(S["Import/Export"], S["Import/Export"].PrefixPosition(), import => import
.Add(S["Deployment Plans"], S["Deployment Plans"].PrefixPosition(), deployment => deployment
.Action("Index", "DeploymentPlan", "OrchardCore.Deployment")
.Permission(Permissions.Export)
.Permission(CommonPermissions.Export)
.LocalNav()
)
.Add(S["Package Import"], S["Package Import"].PrefixPosition(), deployment => deployment
.Action("Index", "Import", "OrchardCore.Deployment")
.Permission(Permissions.Import)
.Permission(CommonPermissions.Import)
.LocalNav()
)
.Add(S["JSON Import"], S["JSON Import"].PrefixPosition(), deployment => deployment
.Action("Json", "Import", "OrchardCore.Deployment")
.Permission(Permissions.Import)
.Permission(CommonPermissions.Import)
.LocalNav()
)
)
Expand Down
18 changes: 9 additions & 9 deletions src/OrchardCore.Modules/OrchardCore.Deployment/Controllers/DeploymentPlanController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,12 @@ public DeploymentPlanController(

public async Task<IActionResult> Index(ContentOptions options, PagerParameters pagerParameters)
{
if (!await _authorizationService.AuthorizeAsync(User, Permissions.ManageDeploymentPlan))
if (!await _authorizationService.AuthorizeAsync(User, CommonPermissions.ManageDeploymentPlan))
{
return Forbid();
}

if (!await _authorizationService.AuthorizeAsync(User, Permissions.Export))
if (!await _authorizationService.AuthorizeAsync(User, CommonPermissions.Export))
{
return Forbid();
}
Expand Down Expand Up @@ -129,7 +129,7 @@ public ActionResult IndexFilterPOST(DeploymentPlanIndexViewModel model)
[FormValueRequired("submit.BulkAction")]
public async Task<ActionResult> IndexBulkActionPOST(ContentOptions options, IEnumerable<long> itemIds)
{
if (!await _authorizationService.AuthorizeAsync(User, Permissions.ManageDeploymentPlan))
if (!await _authorizationService.AuthorizeAsync(User, CommonPermissions.ManageDeploymentPlan))
{
return Forbid();
}
Expand Down Expand Up @@ -158,7 +158,7 @@ public async Task<ActionResult> IndexBulkActionPOST(ContentOptions options, IEnu

public async Task<IActionResult> Display(long id)
{
if (!await _authorizationService.AuthorizeAsync(User, Permissions.ManageDeploymentPlan))
if (!await _authorizationService.AuthorizeAsync(User, CommonPermissions.ManageDeploymentPlan))
{
return Forbid();
}
Expand Down Expand Up @@ -199,7 +199,7 @@ public async Task<IActionResult> Display(long id)

public async Task<IActionResult> Create()
{
if (!await _authorizationService.AuthorizeAsync(User, Permissions.ManageDeploymentPlan))
if (!await _authorizationService.AuthorizeAsync(User, CommonPermissions.ManageDeploymentPlan))
{
return Forbid();
}
Expand All @@ -212,7 +212,7 @@ public async Task<IActionResult> Create()
[HttpPost]
public async Task<IActionResult> Create(CreateDeploymentPlanViewModel model)
{
if (!await _authorizationService.AuthorizeAsync(User, Permissions.ManageDeploymentPlan))
if (!await _authorizationService.AuthorizeAsync(User, CommonPermissions.ManageDeploymentPlan))
{
return Forbid();
}
Expand Down Expand Up @@ -246,7 +246,7 @@ public async Task<IActionResult> Create(CreateDeploymentPlanViewModel model)

public async Task<IActionResult> Edit(long id)
{
if (!await _authorizationService.AuthorizeAsync(User, Permissions.ManageDeploymentPlan))
if (!await _authorizationService.AuthorizeAsync(User, CommonPermissions.ManageDeploymentPlan))
{
return Forbid();
}
Expand All @@ -270,7 +270,7 @@ public async Task<IActionResult> Edit(long id)
[HttpPost]
public async Task<IActionResult> Edit(EditDeploymentPlanViewModel model)
{
if (!await _authorizationService.AuthorizeAsync(User, Permissions.ManageDeploymentPlan))
if (!await _authorizationService.AuthorizeAsync(User, CommonPermissions.ManageDeploymentPlan))
{
return Forbid();
}
Expand Down Expand Up @@ -316,7 +316,7 @@ public async Task<IActionResult> Edit(EditDeploymentPlanViewModel model)
[HttpPost]
public async Task<IActionResult> Delete(long id)
{
if (!await _authorizationService.AuthorizeAsync(User, Permissions.ManageDeploymentPlan))
if (!await _authorizationService.AuthorizeAsync(User, CommonPermissions.ManageDeploymentPlan))
{
return Forbid();
}
Expand Down
2 changes: 1 addition & 1 deletion src/OrchardCore.Modules/OrchardCore.Deployment/Controllers/ExportFileController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public ExportFileController(
[DeleteFileResultFilter]
public async Task<IActionResult> Execute(long id)
{
if (!await _authorizationService.AuthorizeAsync(User, Permissions.Export))
if (!await _authorizationService.AuthorizeAsync(User, CommonPermissions.Export))
{
return Forbid();
}
Expand Down
Loading