-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Roles Updating and History #13040
Roles Updating and History #13040
Changes from 16 commits
f21945b
8490c74
7c815c3
852a9e7
d8bff9b
d51cc5d
927d28b
e093666
218dbf9
34eb7fc
ed418eb
3b12fb3
989383f
c3320b8
03772af
c225543
cb5ce55
af27eac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,19 +1,12 @@ | ||
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); | ||
public List<Role> Roles { get; set; } = new(); | ||
public Dictionary<string, List<string>> MissingFeaturesByRole { get; set; } = new(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,10 +67,17 @@ public async Task<IdentityResult> CreateAsync(IRole role, CancellationToken canc | |
throw new ArgumentNullException(nameof(role)); | ||
} | ||
|
||
var roleToCreate = (Role)role; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe here we should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't want to change the behavior but will check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just used the same pattern as used below for the existing
|
||
|
||
var roles = await LoadRolesAsync(); | ||
roles.Roles.Add((Role)role); | ||
roles.Roles.Add(roleToCreate); | ||
await UpdateRolesAsync(roles); | ||
|
||
var roleCreatedEventHandlers = _serviceProvider.GetRequiredService<IEnumerable<IRoleCreatedEventHandler>>(); | ||
|
||
await roleCreatedEventHandlers.InvokeAsync((handler, roleToCreate) => | ||
handler.RoleCreatedAsync(roleToCreate.RoleName), roleToCreate, _logger); | ||
|
||
return IdentityResult.Success; | ||
} | ||
|
||
|
@@ -90,7 +97,9 @@ public async Task<IdentityResult> DeleteAsync(IRole role, CancellationToken canc | |
} | ||
|
||
var roleRemovedEventHandlers = _serviceProvider.GetRequiredService<IEnumerable<IRoleRemovedEventHandler>>(); | ||
await roleRemovedEventHandlers.InvokeAsync((handler, roleToRemove) => handler.RoleRemovedAsync(roleToRemove.RoleName), roleToRemove, _logger); | ||
|
||
await roleRemovedEventHandlers.InvokeAsync((handler, roleToRemove) => | ||
handler.RoleRemovedAsync(roleToRemove.RoleName), roleToRemove, _logger); | ||
|
||
var roles = await LoadRolesAsync(); | ||
roleToRemove = roles.Roles.FirstOrDefault(r => r.RoleName == roleToRemove.RoleName); | ||
|
@@ -252,8 +261,10 @@ public async Task<IdentityResult> UpdateAsync(IRole role, CancellationToken canc | |
|
||
#endregion IRoleClaimStore<IRole> | ||
|
||
#pragma warning disable CA1816 | ||
public void Dispose() | ||
{ | ||
} | ||
#pragma warning restore CA1816 | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems weird to track missing items instead of what we know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my last comment for the logic where we only use feature / role events, when creating a role we enlist the missing features (installed but not enabled) and only those defining providers, so that when re-enabling such a feature we can lazily update roles that missed it on creation.
Not the only reason but for example doing the opposite would be harder to manage existing tenants, would need a "migration" path to init the history for all existing roles assuming that all installed features was already used, so only using the events would not be sufficient.
Also doing this way, on startup nothing is done on existing tenants until a new role is created, also on a fresh setup this history is just empty, only populated when we disable a feature that provide perms for a given default role that doesn't exist yet, and then create this default role.
In summary when only using events, we need these missing features while creating a role, before it was not needed as default roles was auto created by features.
That's why I first kept the feature Installed event as before, and then because we now can create a default role later on, we need these missing features (installed but no more enabled) to lazily "install" them for this role when they are re-activated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's said I may have done something wrong as there are so many scenarios I tried to keep while still fixing the 2 current issues. YOLO ;)