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.

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

Create system roles by default #17341

Merged
merged 33 commits into from
Jan 21, 2025
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
bd65eed
Create system roles by default
hishamco Jan 12, 2025
151ad09
Add missing roles
hishamco Jan 12, 2025
6117d19
Remove roles from *.recipe.json
hishamco Jan 12, 2025
da19704
Address feedback
hishamco Jan 13, 2025
718d098
Merge branch 'main' into hishamco/system-roles
hishamco Jan 13, 2025
a79ade2
Create a separate data migration
hishamco Jan 13, 2025
5e21655
Remove system roles from migrations.recipe.json
hishamco Jan 13, 2025
e5e0c0d
Address feedback
hishamco Jan 13, 2025
915a554
Merge branch 'main' into hishamco/system-roles
hishamco Jan 13, 2025
5b70ef2
Remove roles from pages.recipe.json
hishamco Jan 14, 2025
784e912
Introduce ISystemRoleProvider
hishamco Jan 15, 2025
7ee0c37
Remove warnings
hishamco Jan 15, 2025
9d38a6c
Merge branch 'main' into hishamco/system-roles
hishamco Jan 15, 2025
56ffeb3
Apply suggestions from code review
hishamco Jan 15, 2025
cb0fd77
localize the description
hishamco Jan 18, 2025
0a49bae
Merge branch 'main' into hishamco/system-roles
hishamco Jan 18, 2025
7a2da40
Remove obsolete from DefaultSystemRoleNameProvider
hishamco Jan 18, 2025
b97848f
Don't depend on role position
hishamco Jan 18, 2025
9026f46
Add GetAdminRoleAsync() to ISystemRoleProvider
hishamco Jan 18, 2025
fa7775a
Merge branch 'main' into hishamco/system-roles
hishamco Jan 18, 2025
48a3719
Remove ISystemRoleNameProvider injection
hishamco Jan 18, 2025
3f104ab
Add more unit tests
hishamco Jan 18, 2025
674c862
Fix the build
hishamco Jan 18, 2025
748e6d8
localizer -> stringLocalizer
hishamco Jan 18, 2025
313651e
Obsolete SystemRoleNameProviderExtensions
hishamco Jan 18, 2025
d2245e3
Address feedback
hishamco Jan 19, 2025
2470fb7
Fix warnings
hishamco Jan 19, 2025
7b7ba19
Apply suggestions from code review
hishamco Jan 20, 2025
b3c15a6
Fix the build
hishamco Jan 20, 2025
26177f3
Remove unnecessary local field
hishamco Jan 20, 2025
fd7e657
Remove extra cast
hishamco Jan 20, 2025
ea580d0
cleanup for approval
MikeAlhayek Jan 21, 2025
1791300
Fix tests
MikeAlhayek Jan 21, 2025
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
using Microsoft.AspNetCore.Identity;
using Microsoft.Extensions.Logging;
using OrchardCore.Data.Migration;
using OrchardCore.Security;

namespace OrchardCore.Roles.Migrations;

public sealed class SystemRolesMigrations : DataMigration
{
private readonly ISystemRoleProvider _systemRoleProvider;
private readonly RoleManager<IRole> _roleManager;
private readonly ILogger _logger;

public SystemRolesMigrations(
ISystemRoleProvider systemRoleProvider,
RoleManager<IRole> roleManager,
ILogger<RolesMigrations> logger)
{
_systemRoleProvider = systemRoleProvider;
_roleManager = roleManager;
_logger = logger;
}

public async Task<int> CreateAsync()
{
var systemRoles = await _systemRoleProvider.GetSystemRolesAsync();

foreach (var systemRole in systemRoles)
{
var role = await _roleManager.FindByNameAsync(systemRole.RoleName);

if (role is null)
{
await _roleManager.CreateAsync(role);
}
}

_logger.LogInformation("The system roles have been created successfully.");

return 1;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,15 @@ namespace OrchardCore.Roles.Recipes;
public sealed class RolesStep : NamedRecipeStepHandler
{
private readonly RoleManager<IRole> _roleManager;
private readonly ISystemRoleNameProvider _systemRoleNameProvider;
private readonly ISystemRoleProvider _systemRoleProvider;

public RolesStep(
RoleManager<IRole> roleManager,
ISystemRoleNameProvider systemRoleNameProvider)
ISystemRoleProvider systemRoleProvider)
: base("Roles")
{
_roleManager = roleManager;
_systemRoleNameProvider = systemRoleNameProvider;
_systemRoleProvider = systemRoleProvider;
}

protected override async Task HandleAsync(RecipeExecutionContext context)
Expand Down Expand Up @@ -59,7 +59,7 @@ protected override async Task HandleAsync(RecipeExecutionContext context)
r.RoleClaims.RemoveAll(c => c.ClaimType == Permission.ClaimType);
}

if (!await _systemRoleNameProvider.IsAdminRoleAsync(roleName))
if (!await _systemRoleProvider.IsAdminRoleAsync(roleName))
{
if (roleEntry.PermissionBehavior == PermissionBehavior.Remove)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,23 @@ public class RoleClaimsProvider : IUserClaimsProvider
{
private readonly UserManager<IUser> _userManager;
private readonly RoleManager<IRole> _roleManager;
private readonly ISystemRoleNameProvider _systemRoleNameProvider;
private readonly ISystemRoleProvider _systemRoleProvider;
private readonly IdentityOptions _identityOptions;

public RoleClaimsProvider(
UserManager<IUser> userManager,
RoleManager<IRole> roleManager,
#pragma warning disable CS0618 // Type or member is obsolete
#pragma warning disable IDE0060 // Remove unused parameter
hishamco marked this conversation as resolved.
Show resolved Hide resolved
ISystemRoleNameProvider systemRoleNameProvider,
#pragma warning restore IDE0060 // Remove unused parameter
#pragma warning restore CS0618 // Type or member is obsolete
ISystemRoleProvider systemRoleProvider,
IOptions<IdentityOptions> identityOptions)
{
_userManager = userManager;
_roleManager = roleManager;
_systemRoleNameProvider = systemRoleNameProvider;
_systemRoleProvider = systemRoleProvider;
_identityOptions = identityOptions.Value;
}

Expand All @@ -39,7 +44,7 @@ public async Task GenerateAsync(IUser user, ClaimsIdentity claims)

foreach (var roleName in roleNames)
{
if (await _systemRoleNameProvider.IsAdminRoleAsync(roleName))
if (await _systemRoleProvider.IsAdminRoleAsync(roleName))
{
isAdministrator = true;
break;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ namespace OrchardCore.Roles.Services;
public class RoleStore : IRoleClaimStore<IRole>, IQueryableRoleStore<IRole>
{
private readonly IServiceProvider _serviceProvider;
private readonly ISystemRoleNameProvider _systemRoleProvider;
private readonly ISystemRoleProvider _systemRoleProvider;
private readonly IDocumentManager<RolesDocument> _documentManager;
protected readonly IStringLocalizer S;
private readonly ILogger _logger;
Expand All @@ -22,7 +22,7 @@ public class RoleStore : IRoleClaimStore<IRole>, IQueryableRoleStore<IRole>

public RoleStore(
IServiceProvider serviceProvider,
ISystemRoleNameProvider systemRoleProvider,
ISystemRoleProvider systemRoleProvider,
IDocumentManager<RolesDocument> documentManager,
IStringLocalizer<RoleStore> stringLocalizer,
ILogger<RoleStore> logger)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ public class RoleUpdater : FeatureEventHandler, IRoleCreatedEventHandler, IRoleR
private readonly ShellDescriptor _shellDescriptor;
private readonly IExtensionManager _extensionManager;
private readonly IDocumentManager<RolesDocument> _documentManager;
private readonly ISystemRoleNameProvider _systemRoleNameProvider;
private readonly ISystemRoleProvider _systemRoleProvider;
private readonly IEnumerable<IPermissionProvider> _permissionProviders;
private readonly ITypeFeatureProvider _typeFeatureProvider;
private readonly ILogger _logger;
Expand All @@ -26,15 +26,18 @@ public RoleUpdater(
ShellDescriptor shellDescriptor,
IExtensionManager extensionManager,
IDocumentManager<RolesDocument> documentManager,
ISystemRoleNameProvider systemRoleNameProvider,
#pragma warning disable IDE0060 // Remove unused parameter
ISystemRoleProvider systemRoleNameProvider,
#pragma warning restore IDE0060 // Remove unused parameter
ISystemRoleProvider systemRoleProvider,
IEnumerable<IPermissionProvider> permissionProviders,
ITypeFeatureProvider typeFeatureProvider,
ILogger<RoleUpdater> logger)
{
_shellDescriptor = shellDescriptor;
_extensionManager = extensionManager;
_documentManager = documentManager;
_systemRoleNameProvider = systemRoleNameProvider;
_systemRoleProvider = systemRoleProvider;
_permissionProviders = permissionProviders;
_typeFeatureProvider = typeFeatureProvider;
_logger = logger;
Expand Down Expand Up @@ -204,7 +207,7 @@ private Task<bool> UpdateRolesForEnabledFeatureAsync(Role role, IEnumerable<IPer

private async Task<bool> UpdatePermissionsAsync(Role role, IEnumerable<string> permissions)
{
if (await _systemRoleNameProvider.IsAdminRoleAsync(role.RoleName))
if (await _systemRoleProvider.IsAdminRoleAsync(role.RoleName))
{
// Don't update claims for admin role.
return true;
Expand Down
5 changes: 4 additions & 1 deletion src/OrchardCore.Modules/OrchardCore.Roles/Startup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,10 @@ public Startup(IShellConfiguration shellConfiguration)
public override void ConfigureServices(IServiceCollection services)
{
services.AddScoped<IUserClaimsProvider, RoleClaimsProvider>();
services.AddDataMigration<RolesMigrations>();

services.AddDataMigration<SystemRolesMigrations>()
hishamco marked this conversation as resolved.
Show resolved Hide resolved
.AddDataMigration<RolesMigrations>();

services.AddScoped<RoleStore>();
services.Replace(ServiceDescriptor.Scoped<IRoleClaimStore<IRole>>(sp => sp.GetRequiredService<RoleStore>()));
services.Replace(ServiceDescriptor.Scoped<IRoleStore<IRole>>(sp => sp.GetRequiredService<RoleStore>()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,19 @@ namespace OrchardCore.Settings.Services;
public class SuperUserHandler : IAuthorizationHandler
{
private readonly ISiteService _siteService;
private readonly ISystemRoleNameProvider _systemRoleNameProvider;
private readonly ISystemRoleProvider _systemRoleProvider;

public SuperUserHandler(
ISiteService siteService,
ISystemRoleNameProvider systemRoleNameProvider)
#pragma warning disable CS0618 // Type or member is obsolete
#pragma warning disable IDE0060 // Remove unused parameter
Copy link
Member

Choose a reason for hiding this comment

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

Why were these added?

Copy link
Member Author

Choose a reason for hiding this comment

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

To avoid the warning cause OC treat them as errors

Copy link
Member

Choose a reason for hiding this comment

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

Don't inject the old interface.

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 will be a breaking change

Copy link
Member

Choose a reason for hiding this comment

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

How would it be a breaking change? You should stop using the old interface and use the new one instead. Why do you need the old interface injected here?

Copy link
Member Author

Choose a reason for hiding this comment

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

How would it be a breaking change?

What if someone subclass one of those classes that rely on the old interface?

Copy link
Member

Choose a reason for hiding this comment

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

If that is the case, you are already causing a breaking change by injecting a new dependency. Anyway, I would not worry about that and remove the injection of the old interface. This is going to be part pf 3.0

Copy link
Member Author

Choose a reason for hiding this comment

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

The no need to obsolete things :)

ISystemRoleNameProvider systemRoleNameProvider,
#pragma warning restore IDE0060 // Remove unused parameter
#pragma warning restore CS0618 // Type or member is obsolete
ISystemRoleProvider systemRoleProvider)
{
_siteService = siteService;
_systemRoleNameProvider = systemRoleNameProvider;
_systemRoleProvider = systemRoleProvider;
}

public async Task HandleAsync(AuthorizationHandlerContext context)
Expand All @@ -30,7 +35,9 @@ public async Task HandleAsync(AuthorizationHandlerContext context)
return;
}

if (user.IsInRole(await _systemRoleNameProvider.GetAdminRoleAsync()))
var adminRole = await _systemRoleProvider.GetAdminRoleAsync();

if (user.IsInRole(adminRole.RoleName))
{
SucceedAllRequirements(context);

Expand Down
15 changes: 0 additions & 15 deletions src/OrchardCore.Themes/TheAdmin/Recipes/blank.recipe.json
Copy link
Member

Choose a reason for hiding this comment

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

The SaaS recipe still has these. pages.recipe.json too but I'm not sure how that's used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Me too :) but seems used the module for test project to test Assembly attributes if I recall

Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,6 @@
{
"name": "Roles",
"Roles": [
{
"Name": "Administrator",
"Description": "A system role that grants all permissions to the assigned users.",
"Permissions": []
},
{
"Name": "Moderator",
"Description": "Grants users the ability to moderate content.",
Expand All @@ -93,16 +88,6 @@
"Name": "Contributor",
"Description": "Grants users the ability to contribute content.",
"Permissions": []
},
{
"Name": "Authenticated",
"Description": "A system role representing all authenticated users.",
"Permissions": []
},
{
"Name": "Anonymous",
"Description": "A system role representing all non-authenticated users.",
"Permissions": []
}
]
}
Expand Down
10 changes: 0 additions & 10 deletions src/OrchardCore.Themes/TheAdmin/Recipes/headless.recipe.json
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,6 @@
{
"name": "Roles",
"Roles": [
{
"Name": "Administrator",
"Description": "A system role that grants all permissions to the assigned users.",
"Permissions": []
},
{
"Name": "Moderator",
"Description": "Grants users the ability to moderate content.",
Expand Down Expand Up @@ -97,11 +92,6 @@
"ExecuteGraphQL",
"ExecuteApiAll"
]
},
{
"Name": "Anonymous",
"Description": "A system role representing all non-authenticated users.",
"Permissions": []
}
]
},
Expand Down
15 changes: 0 additions & 15 deletions src/OrchardCore.Themes/TheAgencyTheme/Recipes/agency.recipe.json
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,6 @@
{
"name": "Roles",
"Roles": [
{
"Name": "Administrator",
"Description": "A system role that grants all permissions to the assigned users.",
"Permissions": []
},
{
"Name": "Moderator",
"Description": "Grants users the ability to moderate content.",
Expand All @@ -97,16 +92,6 @@
"Name": "Contributor",
"Description": "Grants users the ability to contribute content.",
"Permissions": []
},
{
"Name": "Authenticated",
"Description": "A system role representing all authenticated users.",
"Permissions": []
},
{
"Name": "Anonymous",
"Description": "A system role representing all non-authenticated users.",
"Permissions": []
}
]
},
Expand Down
15 changes: 0 additions & 15 deletions src/OrchardCore.Themes/TheBlogTheme/Recipes/blog.recipe.json
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,6 @@
{
"name": "Roles",
"Roles": [
{
"Name": "Administrator",
"Description": "A system role that grants all permissions to the assigned users.",
"Permissions": []
},
{
"Name": "Moderator",
"Description": "Grants users the ability to moderate content.",
Expand All @@ -111,16 +106,6 @@
"Name": "Contributor",
"Description": "Grants users the ability to contribute content.",
"Permissions": []
},
{
"Name": "Authenticated",
"Description": "A system role representing all authenticated users.",
"Permissions": []
},
{
"Name": "Anonymous",
"Description": "A system role representing all non-authenticated users.",
"Permissions": []
}
]
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,6 @@
{
"name": "Roles",
"Roles": [
{
"Name": "Administrator",
"Description": "A system role that grants all permissions to the assigned users.",
"Permissions": []
},
{
"Name": "Moderator",
"Description": "Grants users the ability to moderate content.",
Expand All @@ -94,16 +89,6 @@
"Name": "Contributor",
"Description": "Grants users the ability to contribute content.",
"Permissions": []
},
{
"Name": "Authenticated",
"Description": "A system role representing all authenticated users.",
"Permissions": []
},
{
"Name": "Anonymous",
"Description": "A system role representing all non-authenticated users.",
"Permissions": []
}
]
},
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
using OrchardCore.Security;

namespace OrchardCore.Roles;

public static class SystemRoleProviderExtensions
{
public static async ValueTask<IRole> GetAdminRoleAsync(this ISystemRoleProvider systemRoleNameProvider)
{
var systemRoles = await systemRoleNameProvider.GetSystemRolesAsync();

return systemRoles.SingleOrDefault(role => role.RoleName.Equals(OrchardCoreConstants.Roles.Administrator, StringComparison.OrdinalIgnoreCase));
hishamco marked this conversation as resolved.
Show resolved Hide resolved
}

public static async ValueTask<bool> IsSystemRoleAsync(this ISystemRoleProvider systemRoleNameProvider, string name)
Copy link
Member

Choose a reason for hiding this comment

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

If this was moved to the interface as suggested originally, you can optimize this by adding a case insensitive dictionary.

Copy link
Member

Choose a reason for hiding this comment

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

This is not addressed yet. Move it to the interface so we have more efficient lookup than having to enumerate over the collection every single time. Use a frozen Dictionary internally with a case insensitive look up

Copy link
Member Author

@hishamco hishamco Jan 18, 2025

Choose a reason for hiding this comment

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

I might be against this approach, why do we need to use case insensitive, I remember RoleManager uses ILookupNormalizer

https://github.com/dotnet/aspnetcore/blob/2bf439cdf50ed98a218e00c32a444f113a8d1b4a/src/Identity/Extensions.Core/src/RoleManager.cs

{
ArgumentException.ThrowIfNullOrEmpty(name, nameof(name));

var systemRoles = await systemRoleNameProvider.GetSystemRolesAsync();

return systemRoles.Any(role => role.RoleName.Equals(name, StringComparison.OrdinalIgnoreCase));
}

public static async ValueTask<bool> IsAdminRoleAsync(this ISystemRoleProvider systemRoleNameProvider, string name)
{
ArgumentException.ThrowIfNullOrEmpty(name, nameof(name));
Copy link
Member

Choose a reason for hiding this comment

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

No need for the second argument

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the role that we need to check?

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure what you are asking. I am saying, you don't the second parameter nameof(name)

Copy link
Member Author

@hishamco hishamco Jan 20, 2025

Choose a reason for hiding this comment

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

The exception needs the argument value and argument name

Copy link
Contributor

Choose a reason for hiding this comment

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

The exception needs the argument value and argument name

The paramName argument has the CallerArgumentExpressionAtribute and it takes the argument parameter. So you only need to give it the first parameter (argument):

ThrowIfNullOrEmpty([NotNull] string? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null)


var adminRole = await systemRoleNameProvider.GetAdminRoleAsync();

return adminRole.RoleName.Equals(name, StringComparison.OrdinalIgnoreCase);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace OrchardCore.Roles;

[Obsolete("This interface has been deprecated, use ISystemRoleProvider instead.")]
public interface ISystemRoleNameProvider
{
ValueTask<string> GetAdminRoleAsync();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
using OrchardCore.Security;

namespace OrchardCore.Roles;

public interface ISystemRoleProvider
{
ValueTask<IEnumerable<IRole>> GetSystemRolesAsync();
}
Loading
Loading