Consistent comparing of role names #15076
Conversation
gvkries
requested review from
sfmskywalker,
deanmarcussen,
kevinchalet,
hishamco,
Jetski5822,
carlwoodhouse and
agriffard
as code owners
|
Sorry for the large PR out of the blue, I thought it might be useful 😇 |
| @@ -102,7 +103,7 @@ public async Task<IdentityResult> DeleteAsync(IRole role, CancellationToken canc | |||
| handler.RoleRemovedAsync(roleToRemove.RoleName), roleToRemove, _logger); | |||
|
|
|||
| var roles = await LoadRolesAsync(); | |||
| roleToRemove = roles.Roles.FirstOrDefault(r => r.RoleName == roleToRemove.RoleName); | |||
| roleToRemove = roles.Roles.FirstOrDefault(r => string.Equals(r.RoleName, roleToRemove.RoleName, StringComparison.OrdinalIgnoreCase)); | |||
| @@ -115,7 +116,7 @@ public async Task<IRole> FindByIdAsync(string roleId, CancellationToken cancella | |||
| // While updating find a role from the loaded document being mutated. | |||
| var roles = _updating ? await LoadRolesAsync() : await GetRolesAsync(); | |||
|
|
|||
| var role = roles.Roles.FirstOrDefault(x => x.RoleName == roleId); | |||
| var role = roles.Roles.FirstOrDefault(x => string.Equals(x.RoleName, roleId, StringComparison.OrdinalIgnoreCase)); | |||
There was a problem hiding this comment.
this changes the behavior. Why?
There was a problem hiding this comment.
Yes, it alters behavior, but enhances the reliability of the RoleStore. The comparison of roles by name was handled inconsistently across the entire source code. If anyone used multiple roles that differed only in casing, they are already facing issues.
The specific problem I encountered was that default permissions were not applied to roles in UpdateAsync due to the case sensitivity. Merely making UpdateAsync case-insensitive wouldn't have been a logical solution.
There was a problem hiding this comment.
What should be done is storing the role name in the database as normalized and always use == to compare normalize values. I don't think we should use invariant check all over the place
There was a problem hiding this comment.
The normalized value is already stored in NormalizedRoleName which is used by MS Identity. But I don't think we gain anything by using that. It would always require to either normalize all parameters or also using case insensitive comparing. The normalized value is also managed by MS Identity and not by OC on its own.
As it is currently implemented, RoleName is used as the ID for roles and it is a user-provided value. That makes it kind of fragile and I think case insensitive comparing is the least intrusive solution.
There was a problem hiding this comment.
@gvkries
I don't remember all the details, I would have to look at all the places we use role name to provide more feedback. Which is why I suggested to replace string with constants in this PR.
But from what I remember is that we do not store normalized roles in the Users properties. I think we need to change how to store the role name and add migration to update existing stored names. Then, anytime we do comparisons, we would normalize the name and do a normalized name comparison.
src/OrchardCore.Modules/OrchardCore.Users/Drivers/UserRoleDisplayDriver.cs
Outdated
Show resolved
Hide resolved
src/OrchardCore/OrchardCore.Infrastructure.Abstractions/Security/RoleNames.cs
Show resolved
Hide resolved
| public const string Editor = "Editor"; | ||
| public const string Moderator = "Moderator"; | ||
|
|
||
| public static IEnumerable<string> SystemRoles => s_systemRoleNames; |
There was a problem hiding this comment.
I think these helper should stay where they were and keep role name in here.
There was a problem hiding this comment.
I have changed that because the RoleHelper class had very few references internally and I don't expect many uses outside of the internal projects. Also I think it makes sense to bundle system roles and the role names together and I thought RoleNames would be easier to discover, additionally because I changed the namespace to match the one of IRole too.
There was a problem hiding this comment.
RoleNames should be constant variable whereas RoleHelper is a class for helper function. If someone is using the RoleNames, they'll have a breaking change
There was a problem hiding this comment.
Sorry, I don't understand what you mean. Do you want to move RoleNames inside RoleHelper? Breaking is the removal of RoleHelper, but I don't think that matters. It contains one static field and has 4 internal references. I don't expect any external reference.
I messed up the namespace of RoleNames anyway. It was meant to be in OrchardCore.Security side-by-side with IRole and Role.
There was a problem hiding this comment.
@gvkries
I suggest to keep the existing RoleHelper in addition to the new RoleNames.
I also, think you should to only replace role name with the new constants in this PR. Then using a follow up PR maybe suggest the casing changes. This way the logic of changing the casing will be much easier to follow.
There was a problem hiding this comment.
No need to take this, it was just a suggestion to make things easier in future. I'm going to close this PR and provide the fix only.
…layDriver.cs Co-authored-by: Mike Alhayek <[email protected]>
|
@gvkries please don't ask all to review the PR, I know it might happen acciedently when you push a PR :) |
|
As expected :) |
Always compare role names case insensitive. Also refactored all role names into a static helper.
Fixes #15057