Move role creation into the recipe instead of feature activation

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

@@ -10,8 +10,10 @@
 using Microsoft.Extensions.Localization;
 using OrchardCore.Data.Documents;
 using OrchardCore.DisplayManagement.Notify;
+using OrchardCore.Documents;
 using OrchardCore.Environment.Extensions;
 using OrchardCore.Environment.Extensions.Features;
+using OrchardCore.Roles.Models;
 using OrchardCore.Roles.ViewModels;
 using OrchardCore.Security;
 using OrchardCore.Security.Permissions;
@@ -25,6 +27,7 @@ public class AdminController : Controller
         private readonly IAuthorizationService _authorizationService;
         private readonly IStringLocalizer S;
         private readonly RoleManager<IRole> _roleManager;
+        private readonly IDocumentManager<RolesDocument> _rolesDocumentManager;
         private readonly IEnumerable<IPermissionProvider> _permissionProviders;
         private readonly ITypeFeatureProvider _typeFeatureProvider;
         private readonly IRoleService _roleService;
@@ -38,6 +41,7 @@ public AdminController(
             IStringLocalizer<AdminController> stringLocalizer,
             IHtmlLocalizer<AdminController> htmlLocalizer,
             RoleManager<IRole> roleManager,
+            IDocumentManager<RolesDocument> rolesDocumentManager,
             IRoleService roleService,
             INotifier notifier,
             IEnumerable<IPermissionProvider> permissionProviders
@@ -49,6 +53,7 @@ IEnumerable<IPermissionProvider> permissionProviders
             _typeFeatureProvider = typeFeatureProvider;
             _permissionProviders = permissionProviders;
             _roleManager = roleManager;
+            _rolesDocumentManager = rolesDocumentManager;
             S = stringLocalizer;
             _authorizationService = authorizationService;
             _documentStore = documentStore;
@@ -97,12 +102,12 @@ public async Task<IActionResult> Create(CreateRoleViewModel model)
 
                 if (model.RoleName.Contains('/'))
                 {
-                    ModelState.AddModelError(string.Empty, S["Invalid role name."]);
+                    ModelState.AddModelError(String.Empty, S["Invalid role name."]);
                 }
 
                 if (await _roleManager.FindByNameAsync(_roleManager.NormalizeKey(model.RoleName)) != null)
                 {
-                    ModelState.AddModelError(string.Empty, S["The role is already used."]);
+                    ModelState.AddModelError(String.Empty, S["The role is already used."]);
                 }
             }
 
@@ -120,7 +125,7 @@ public async Task<IActionResult> Create(CreateRoleViewModel model)
 
                 foreach (var error in result.Errors)
                 {
-                    ModelState.AddModelError(string.Empty, error.Description);
+                    ModelState.AddModelError(String.Empty, error.Description);
                 }
             }
 
@@ -171,8 +176,7 @@ public async Task<IActionResult> Edit(string id)
                 return Forbid();
             }
 
-            var role = (Role)await _roleManager.FindByNameAsync(_roleManager.NormalizeKey(id));
-            if (role == null)
+            if (await _roleManager.FindByNameAsync(_roleManager.NormalizeKey(id)) is not Role role)
             {
                 return NotFound();
             }
@@ -200,22 +204,52 @@ public async Task<IActionResult> EditPost(string id, string roleDescription)
                 return Forbid();
             }
 
-            var role = (Role)await _roleManager.FindByNameAsync(_roleManager.NormalizeKey(id));
-
-            if (role == null)
+            if (await _roleManager.FindByNameAsync(_roleManager.NormalizeKey(id)) is not Role role)
             {
                 return NotFound();
             }
 
             role.RoleDescription = roleDescription;
 
+            var rolesDocument = await _rolesDocumentManager.GetOrCreateMutableAsync();
+            var updateRolesDocument = false;
+
+            if (!rolesDocument.PermissionGroups.ContainsKey(role.RoleName))
+            {
+                rolesDocument.PermissionGroups.TryAdd(role.RoleName, new List<string>());
+            }
+
+            var permissionNames = _permissionProviders.SelectMany(x => x.GetDefaultStereotypes())
+                .SelectMany(y => y.Permissions ?? Enumerable.Empty<Permission>())
+                .Select(x => x.Name)
+                .ToList();
+
             // Save
             var rolePermissions = new List<RoleClaim>();
-            foreach (string key in Request.Form.Keys)
+            foreach (var key in Request.Form.Keys)
             {
-                if (key.StartsWith("Checkbox.", StringComparison.Ordinal) && Request.Form[key] == "true")
+                var permissionName = key;
+
+                if (key.StartsWith("Checkbox.", StringComparison.Ordinal))
+                {
+                    permissionName = key.Substring("Checkbox.".Length);
+                }
+
+                if (!permissionNames.Contains(permissionName, StringComparer.OrdinalIgnoreCase))
+                {
+                    // The request contains an invalid permission, let's ignore it
+                    continue;
+                }
+
+                if (!rolesDocument.PermissionGroups[role.RoleName].Contains(permissionName, StringComparer.OrdinalIgnoreCase))
+                {
+                    // Save the permission in the permissions history so it never gets auto assigned to this role.
+                    rolesDocument.PermissionGroups[role.RoleName].Add(permissionName);
+                    updateRolesDocument = true;
+                }
+
+                if (String.Equals(Request.Form[key], "true", StringComparison.OrdinalIgnoreCase))
                 {
-                    string permissionName = key.Substring("Checkbox.".Length);
                     rolePermissions.Add(new RoleClaim { ClaimType = Permission.ClaimType, ClaimValue = permissionName });
                 }
             }
@@ -225,6 +259,11 @@ public async Task<IActionResult> EditPost(string id, string roleDescription)
 
             await _roleManager.UpdateAsync(role);
 
+            if (updateRolesDocument)
+            {
+                await _rolesDocumentManager.UpdateAsync(rolesDocument);
+            }
+
             await _notifier.SuccessAsync(H["Role updated successfully."]);
 
             return RedirectToAction(nameof(Index));

src/OrchardCore.Modules/OrchardCore.Roles/Models/RolesDocument.cs

@@ -1,11 +1,19 @@
+using System;
 using System.Collections.Generic;
 using OrchardCore.Data.Documents;
 using OrchardCore.Security;
+using OrchardCore.Security.Permissions;
 
 namespace OrchardCore.Roles.Models
 {
     public class RolesDocument : Document
     {
         public List<Role> Roles { get; set; } = new List<Role>();
+
+
+        /// <summary>
+        /// Keeps track of all permission that were automaticly assigned to a role using <see cref="IPermissionProvider"></see>/>
+        /// </summary>
+        public Dictionary<string, List<string>> PermissionGroups { get; set; } = new(StringComparer.OrdinalIgnoreCase);
     }
 }

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

@@ -1,41 +1,50 @@
+using System;
 using System.Collections.Generic;
 using System.Linq;
 using System.Security.Claims;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Identity;
 using Microsoft.Extensions.Logging;
-using OrchardCore.Environment.Extensions;
+using OrchardCore.Documents;
 using OrchardCore.Environment.Extensions.Features;
 using OrchardCore.Environment.Shell;
+using OrchardCore.Modules;
+using OrchardCore.Roles.Models;
 using OrchardCore.Security;
 using OrchardCore.Security.Permissions;
+using OrchardCore.Security.Services;
 
 namespace OrchardCore.Roles.Services
 {
-    public class RoleUpdater : IFeatureEventHandler
+    public class RoleUpdater : ModularTenantEvents, IFeatureEventHandler
     {
         private readonly RoleManager<IRole> _roleManager;
         private readonly IEnumerable<IPermissionProvider> _permissionProviders;
-        private readonly ITypeFeatureProvider _typeFeatureProvider;
         private readonly ILogger _logger;
+        private readonly IRoleService _roleService;
+        private readonly IDocumentManager<RolesDocument> _rolesDocumentManager;
+
+        private bool _updateInProgress;
 
         public RoleUpdater(
             RoleManager<IRole> roleManager,
             IEnumerable<IPermissionProvider> permissionProviders,
-            ITypeFeatureProvider typeFeatureProvider,
-            ILogger<RoleUpdater> logger)
+            ILogger<RoleUpdater> logger,
+            IRoleService roleService,
+            IDocumentManager<RolesDocument> rolesDocumentManager)
         {
-            _typeFeatureProvider = typeFeatureProvider;
             _roleManager = roleManager;
             _permissionProviders = permissionProviders;
             _logger = logger;
+            _roleService = roleService;
+            _rolesDocumentManager = rolesDocumentManager;
         }
 
         Task IFeatureEventHandler.InstallingAsync(IFeatureInfo feature) => Task.CompletedTask;
 
-        Task IFeatureEventHandler.InstalledAsync(IFeatureInfo feature) => AddDefaultRolesForFeatureAsync(feature);
+        Task IFeatureEventHandler.InstalledAsync(IFeatureInfo feature) => Task.CompletedTask;
 
-        Task IFeatureEventHandler.EnablingAsync(IFeatureInfo feature) => Task.CompletedTask;
+        Task IFeatureEventHandler.EnablingAsync(IFeatureInfo feature) => AssignPermissionsToRolesAsync();
 
         Task IFeatureEventHandler.EnabledAsync(IFeatureInfo feature) => Task.CompletedTask;
 
@@ -47,68 +56,109 @@ public RoleUpdater(
 
         Task IFeatureEventHandler.UninstalledAsync(IFeatureInfo feature) => Task.CompletedTask;
 
-        public async Task AddDefaultRolesForFeatureAsync(IFeatureInfo feature)
+        /// <summary>
+        /// This event is called of the very first request to the tenant after a tenant is built/rebuilt.
+        /// Using this event will ensure that any new permission were added are auto assigned to a role.
+        /// </summary>
+        /// <returns></returns>
+        public override Task ActivatedAsync() => AssignPermissionsToRolesAsync();
+
+        /// <summary>
+        /// Checks all available permissions to role mapping from any available <see cref="IPermissionProvider"/>.
+        /// When a new permission is found, auto assign it to the mapped role. If the permission was previously assigned,
+        /// do not assign the permission. This method could get called multiple time from the same request,
+        /// so it should not be executed again if the parameter "_updateInProgress" is set to true.
+        /// </summary>
+        /// <returns></returns>
+        private async Task AssignPermissionsToRolesAsync()
         {
-            // when another module is being enabled, locate matching permission providers
-            var providersForEnabledModule = _permissionProviders
-                .Where(x => _typeFeatureProvider.GetFeatureForDependency(x.GetType()).Id == feature.Id);
+            if (_updateInProgress)
+            {
+                return;
+            }
+
+            _updateInProgress = true;
+
+            var roleNames = await _roleService.GetRoleNamesAsync();
+
+            if (!roleNames.Any())
+            {
+                // Site roles are initially added using the "RolesStep" handler, while no role names are availble. This means
+                // that RoleUpdater handler was called before the "RolesStep".
+                // Its likley to be coming from a tenant setup request. Nothing to do.
+                return;
+            }
+
+            var rolesDocument = await _rolesDocumentManager.GetOrCreateMutableAsync();
+            var updateRolesDocument = false;
+
+            // Get all the available permissions grouped by role name as defined by IPermissionProvider.
+            var groups = _permissionProviders.SelectMany(x => x.GetDefaultStereotypes())
+                .GroupBy(stereotype => stereotype.Name)
+                .Select(x => new
+                {
+                    RoleName = x.Key,
+                    PermissionNames = x.SelectMany(y => y.Permissions ?? Enumerable.Empty<Permission>()).Select(x => x.Name)
+                });
 
-            if (_logger.IsEnabled(LogLevel.Debug))
+            foreach (var group in groups)
             {
-                if (providersForEnabledModule.Any())
+                if (!roleNames.Any(roleName => roleName.Equals(group.RoleName, StringComparison.OrdinalIgnoreCase))
+                    || await _roleManager.FindByNameAsync(group.RoleName) is not Role role)
                 {
-                    _logger.LogDebug("Configuring default roles for feature '{FeatureName}'", feature.Id);
+                    // A role is mapped in IPermissionProvider, yet it isn't available for the tenant, ignore it.
+                    continue;
                 }
-                else
+
+                var currentPermissionNames = role.RoleClaims.Where(x => x.ClaimType == Permission.ClaimType).Select(x => x.ClaimValue);
+
+                var distinctPermissionNames = currentPermissionNames
+                    .Union(group.PermissionNames)
+                    .Distinct()
+                    .ToList();
+
+                if (!rolesDocument.PermissionGroups.ContainsKey(group.RoleName))
                 {
-                    _logger.LogDebug("No default roles for feature '{FeatureName}'", feature.Id);
+                    rolesDocument.PermissionGroups.TryAdd(group.RoleName, new List<string>());
                 }
-            }
 
-            foreach (var permissionProvider in providersForEnabledModule)
-            {
-                // get and iterate stereotypical groups of permissions
-                var stereotypes = permissionProvider.GetDefaultStereotypes();
-                foreach (var stereotype in stereotypes)
+                // Get all available permission names that isn't already assigned to the role.
+                var additionalPermissionNames = distinctPermissionNames.Except(currentPermissionNames);
+
+                foreach (var permissionName in additionalPermissionNames)
                 {
-                    // turn those stereotypes into roles
-                    var role = await _roleManager.FindByNameAsync(stereotype.Name);
-                    if (role == null)
+                    if (_logger.IsEnabled(LogLevel.Debug))
                     {
-                        if (_logger.IsEnabled(LogLevel.Information))
-                        {
-                            _logger.LogInformation("Defining new role '{RoleName}' for permission stereotype", stereotype.Name);
-                        }
-
-                        role = new Role { RoleName = stereotype.Name, RoleDescription = stereotype.Name + " role" };
-                        await _roleManager.CreateAsync(role);
+                        _logger.LogDebug("Default role '{Role}' granted permission '{Permission}'", group.RoleName, permissionName);
                     }
 
-                    // and merge the stereotypical permissions into that role
-                    var stereotypePermissionNames = (stereotype.Permissions ?? Enumerable.Empty<Permission>()).Select(x => x.Name);
-                    var currentPermissionNames = ((Role)role).RoleClaims.Where(x => x.ClaimType == Permission.ClaimType).Select(x => x.ClaimValue);
-
-                    var distinctPermissionNames = currentPermissionNames
-                        .Union(stereotypePermissionNames)
-                        .Distinct();
+                    if (rolesDocument.PermissionGroups[group.RoleName].Contains(permissionName, StringComparer.OrdinalIgnoreCase))
+                    {
+                        // The permission was previously assigned to the role, we can't assign it again.
+                        continue;
+                    }
 
-                    // update role if set of permissions has increased
-                    var additionalPermissionNames = distinctPermissionNames.Except(currentPermissionNames);
+                    await _roleManager.AddClaimAsync(role, new Claim(Permission.ClaimType, permissionName));
+                }
 
-                    if (additionalPermissionNames.Any())
+                foreach (var distinctPermissionName in distinctPermissionNames)
+                {
+                    if (rolesDocument.PermissionGroups[group.RoleName].Contains(distinctPermissionName, StringComparer.OrdinalIgnoreCase))
                     {
-                        foreach (var permissionName in additionalPermissionNames)
-                        {
-                            if (_logger.IsEnabled(LogLevel.Debug))
-                            {
-                                _logger.LogDebug("Default role '{Role}' granted permission '{Permission}'", stereotype.Name, permissionName);
-                            }
-
-                            await _roleManager.AddClaimAsync(role, new Claim(Permission.ClaimType, permissionName));
-                        }
+                        continue;
                     }
+
+                    rolesDocument.PermissionGroups[group.RoleName].Add(distinctPermissionName);
+                    updateRolesDocument = true;
                 }
             }
+
+            if (updateRolesDocument)
+            {
+                await _rolesDocumentManager.UpdateAsync(rolesDocument);
+            }
+
+            _updateInProgress = false;
         }
     }
 }

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

@@ -42,6 +42,7 @@ public override void ConfigureServices(IServiceCollection services)
             services.AddRecipeExecutionStep<RolesStep>();
 
             services.AddScoped<IFeatureEventHandler, RoleUpdater>();
+            services.AddScoped<IModularTenantEvents, RoleUpdater>();
             services.AddScoped<IAuthorizationHandler, RolesPermissionsHandler>();
             services.AddScoped<INavigationProvider, AdminMenu>();
             services.AddScoped<IPermissionProvider, Permissions>();