Create system roles by default #17341

hishamco · 2025-01-12T04:39:36Z

MikeAlhayek · 2025-01-12T16:23:06Z

@hishamco I know this is a draft. But I think you are missing the ask. Not all these roles are system role. There are currently only 3 system roles. Administration (this name could be changed), Anonymous, and Authorized.

You should inject ISystemRolePrpvider to get a list of the system roles.

OrchardCore/src/OrchardCore/OrchardCore.Roles.Abstractions/ISystemRoleNameProvider.cs

Line 5 in a2f45e5

public interface ISystemRoleNameProvider

In the roles module, create a SystemRolesMigrations file that would read the system roles from the provider and create them using the RoleManager. Then you can remove only these 3 roles from the recipes.

hishamco · 2025-01-12T18:26:24Z

Thanks it is still a draft, I added the system roles first, then added the rest accidentally

Using the provider is much better, I will react to your feedback

hishamco · 2025-01-13T00:06:16Z

@MikeAlhayek I didn't see a description there, or shall we make ISystemRoleProvider instead of ISystemRoleNameProvider?

hishamco · 2025-01-13T01:32:40Z

I tried the PR on a fresh installation and it works fine but I don't know why it fails in Linux

src/OrchardCore.Modules/OrchardCore.Roles/Migrations/SystemRolesMigrations.cs

src/OrchardCore.Themes/TheAdmin/Recipes/headless.recipe.json

src/OrchardCore.Modules/OrchardCore.Roles/Startup.cs

src/OrchardCore.Modules/OrchardCore.Roles/Migrations/SystemRolesMigrations.cs

Piedone · 2025-01-13T21:19:15Z

src/OrchardCore.Themes/TheAdmin/Recipes/blank.recipe.json

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

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

src/OrchardCore.Modules/OrchardCore.Roles/Migrations/SystemRolesMigrations.cs

MikeAlhayek · 2025-01-15T12:25:06Z

src/OrchardCore.Modules/OrchardCore.Settings/Services/SuperUserHandler.cs

+#pragma warning disable CS0618 // Type or member is obsolete
+#pragma warning disable IDE0060 // Remove unused parameter


Why were these added?

To avoid the warning cause OC treat them as errors

Don't inject the old interface.

It will be a breaking change

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?

How would it be a breaking change?

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

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

The no need to obsolete things :)

src/OrchardCore/OrchardCore.Roles.Abstractions/Extensions/SystemRoleProviderExtensions.cs

MikeAlhayek · 2025-01-15T12:29:16Z

src/OrchardCore/OrchardCore.Roles.Abstractions/Extensions/SystemRoleProviderExtensions.cs

+    public static async ValueTask<IRole> GetAdminRoleAsync(this ISystemRoleProvider systemRoleNameProvider)
+        => (await systemRoleNameProvider.GetSystemRolesAsync()).First();
+
+    public static async ValueTask<bool> IsSystemRoleAsync(this ISystemRoleProvider systemRoleNameProvider, string name)


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

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

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

src/OrchardCore/OrchardCore.Roles.Abstractions/ISystemRoleNameProvider.cs

src/OrchardCore/OrchardCore.Roles.Core/DefaultSystemRoleProvider.cs

src/OrchardCore/OrchardCore.Roles.Core/Services/DefaultSystemRoleNameProvider.cs

src/OrchardCore/OrchardCore.Roles.Core/ServiceCollectionExtensions.cs

src/OrchardCore.Modules/OrchardCore.Roles/Services/RoleClaimsProvider.cs

src/OrchardCore/OrchardCore.Roles.Core/DefaultSystemRoleProvider.cs

Co-authored-by: Mike Alhayek <[email protected]>

MikeAlhayek · 2025-01-18T01:11:53Z

@hishamco can you please finish one PR at a time? I am seeing more PRs coming in but a PR like this was put on the shelve. This is one of the reasons why many PR just sit.

src/OrchardCore/OrchardCore.Roles.Abstractions/Extensions/SystemRoleProviderExtensions.cs

MikeAlhayek · 2025-01-18T18:58:47Z

src/OrchardCore/OrchardCore.Roles.Abstractions/ISystemRoleProvider.cs

+{
+    ValueTask<IEnumerable<IRole>> GetSystemRolesAsync();
+
+    ValueTask<IRole> GetAdminRoleAsync();


Add bool IsSystemRole function.

Also, I don't think all th functions here should not be async

Add bool IsSystemRole function.

There is no need for it could be an extension method as I did because it could be computed from GetSystemRolesAsync()

I understand. But it's less efficient. Use a frozen dictionary and a look up agains that dictionary is faster what we need in this case.

It was working well because you was stored strings previously

You can still use FrozenDictionary<string, IRole> in the implementation so the lookup by name is fast.

Did you check the implementation that I linked here?

What should I look for? Please review my original suggestion for the interface and stick to it. Only thing I would change is replace the async calls with sync.

To check the implementation, coz your implementation will expose FrozenSet if you intend to ignore the casing we could use the sort of Normalize() function. I'm still suggesting exposing IEnumerable<IRole> like RoleManager is a good option

In terms of PERF, let's hear @sebastienros thought about this

Uh... it's a singleton object and we check if a role is a system role often (maybe on every single request) so optimization would be great here. You don't need to expose a FrozenSet, you can expose IEnumerable. But, expose IsSystemRole method that would accept a string parameter and returns Boolean. Internally you'll have a frozen set to manage the system roles. A 10 minute PR is going to take 1+ month. Please only request my review when my feedback addresses so we are not wasting time with the unnecessary back and forth.

Please only request my review when my feedback addresses so we are not wasting time with the unnecessary back and forth.

Please don't take it in person, the idea is that I'm reacting to the feedback when it makes sense to me because each one thinks in a different way

I will try to use FrozenSet but moving IsSystemRole() back to the interface is not make sense

src/OrchardCore/OrchardCore.Roles.Core/DefaultSystemRoleProvider.cs

src/OrchardCore/OrchardCore.Roles.Core/SystemRoleNameProviderExtensions.cs

MikeAlhayek · 2025-01-20T00:21:42Z

src/OrchardCore/OrchardCore.Roles.Abstractions/Extensions/SystemRoleProviderExtensions.cs

+{
+    public static bool IsAdminRole(this ISystemRoleProvider systemRoleNameProvider, string name)
+    {
+        ArgumentException.ThrowIfNullOrEmpty(name, nameof(name));


No need for the second argument

What's the role that we need to check?

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

The exception needs the argument value and argument name

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)

src/OrchardCore/OrchardCore.Roles.Abstractions/ISystemRoleProvider.cs

src/OrchardCore/OrchardCore.Roles.Core/DefaultSystemRoleProvider.cs

Co-authored-by: Mike Alhayek <[email protected]>

hishamco · 2025-01-20T22:56:05Z

Oops, last code suggestion break the build

MikeAlhayek

@hishamco I just made minor cleanup commit to approve the PR for you.

Piedone · 2025-01-21T18:24:48Z

I wanted to wait until you two have finished discussing this, but now I see Georg also approved, so nothing to add.

Create system roles by default

bd65eed

hishamco marked this pull request as draft January 12, 2025 04:40

hishamco added 2 commits January 12, 2025 07:50

Add missing roles

151ad09

Remove roles from *.recipe.json

6117d19

hishamco added 2 commits January 13, 2025 03:06

Address feedback

da19704

Merge branch 'main' into hishamco/system-roles

718d098

hishamco marked this pull request as ready for review January 13, 2025 00:08

Create a separate data migration

a79ade2

hishamco requested a review from MikeAlhayek January 13, 2025 00:13

MikeAlhayek reviewed Jan 13, 2025

View reviewed changes

src/OrchardCore.Modules/OrchardCore.Roles/Migrations/SystemRolesMigrations.cs Outdated Show resolved Hide resolved

Remove system roles from migrations.recipe.json

5e21655

hishamco requested a review from MikeAlhayek January 13, 2025 05:29

MikeAlhayek requested changes Jan 13, 2025

View reviewed changes

hishamco added 2 commits January 13, 2025 18:38

Address feedback

e5e0c0d

Merge branch 'main' into hishamco/system-roles

915a554

hishamco requested review from MikeAlhayek and Piedone January 13, 2025 15:40

Piedone requested changes Jan 13, 2025

View reviewed changes

hishamco added 4 commits January 14, 2025 06:14

Remove roles from pages.recipe.json

5b70ef2

Introduce ISystemRoleProvider

784e912

Remove warnings

7ee0c37

Merge branch 'main' into hishamco/system-roles

9d38a6c

hishamco mentioned this pull request Jan 15, 2025

Release v2.1.4 #17354

Closed

10 tasks

MikeAlhayek requested changes Jan 15, 2025

View reviewed changes

Apply suggestions from code review

56ffeb3

Co-authored-by: Mike Alhayek <[email protected]>

hishamco added 3 commits January 18, 2025 04:43

Merge branch 'main' into hishamco/system-roles

0a49bae

Remove obsolete from DefaultSystemRoleNameProvider

7a2da40

Don't depend on role position

b97848f

MikeAlhayek reviewed Jan 18, 2025

View reviewed changes

src/OrchardCore/OrchardCore.Roles.Abstractions/Extensions/SystemRoleProviderExtensions.cs Outdated Show resolved Hide resolved

hishamco added 5 commits January 18, 2025 07:06

Add GetAdminRoleAsync() to ISystemRoleProvider

9026f46

Merge branch 'main' into hishamco/system-roles

fa7775a

Remove ISystemRoleNameProvider injection

48a3719

Add more unit tests

3f104ab

Fix the build

674c862

hishamco requested review from Piedone and MikeAlhayek January 18, 2025 18:50

MikeAlhayek requested changes Jan 18, 2025

View reviewed changes

hishamco added 4 commits January 18, 2025 22:08

localizer -> stringLocalizer

748e6d8

Obsolete SystemRoleNameProviderExtensions

313651e

Address feedback

d2245e3

Fix warnings

2470fb7

hishamco requested a review from MikeAlhayek January 19, 2025 22:00

MikeAlhayek requested changes Jan 20, 2025

View reviewed changes

Apply suggestions from code review

7b7ba19

Co-authored-by: Mike Alhayek <[email protected]>

hishamco and others added 4 commits January 21, 2025 02:02

Fix the build

b3c15a6

Remove unnecessary local field

26177f3

Remove extra cast

fd7e657

cleanup for approval

ea580d0

MikeAlhayek approved these changes Jan 21, 2025

View reviewed changes

gvkries approved these changes Jan 21, 2025

View reviewed changes

Fix tests

1791300

Piedone merged commit 0c5fb0e into main Jan 21, 2025
7 checks passed

Piedone deleted the hishamco/system-roles branch January 21, 2025 18:25

		#pragma warning disable CS0618 // Type or member is obsolete
		#pragma warning disable IDE0060 // Remove unused parameter

Create system roles by default #17341

Create system roles by default #17341

Conversation

hishamco commented Jan 12, 2025 • edited Loading

MikeAlhayek commented Jan 12, 2025

hishamco commented Jan 12, 2025

hishamco commented Jan 13, 2025

hishamco commented Jan 13, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hishamco Jan 18, 2025 • edited Loading

Choose a reason for hiding this comment

MikeAlhayek commented Jan 18, 2025

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

hishamco Jan 18, 2025 • 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

hishamco Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hishamco commented Jan 20, 2025

MikeAlhayek left a comment

Choose a reason for hiding this comment

Piedone commented Jan 21, 2025

hishamco commented Jan 12, 2025 •

edited

Loading

hishamco Jan 18, 2025 •

edited

Loading

hishamco Jan 18, 2025 •

edited

Loading

hishamco Jan 20, 2025 •

edited

Loading