Extend user permissions

[MikeAlhayek]

Fix #11762
Fix #12749

Replaces #11763

These changes were demonstrated during the Steering committee on 9/13/2022

Here is a summary of changes.

We add more permission to the Users module to make it more usable and robust for many use cases.

• List users in {0} role
• Edit users in {0} role
• Delete users in {0} role
• Assign users to {0} role
• Manage user profile settings

Also, we now have a settings that would allow the user to prevent username and/or email change on the edit screen.

Here is a screenshot of the new settings

Here is a screenshot of a user that is logged as admin.

Here is a screenshot of a user that can edit user's with Author role and only List users in role - Editor and do NOT have permission to Assign users. Note here the user has Edit permission to Auther role, but since the user has no way to assign role, he is unable to create a user.

Here is a screenshot of a user that can't view Administrator, can edit Author and Editor, also can delete Author only

[hishamco]

Why you merge the two fixes in the ssame PR, while one is related to roles?!!

[MikeAlhayek]

Because in this PR many roles related changes were done. But if you like more PR's, I can break this one into two :)

[hishamco]

I suggest to revert the roles changes and let the team decide what's the best approach before you waste a time and closing the PR

[MikeAlhayek]

@hishamco I split it into two PR.

[sebastienros]

Random: if something is disabled in the UI be sure to do the same check in the controller to prevent security issues.

[MikeAlhayek]

@sebastienros can we review this on next triage?

[MikeAlhayek]

@sebastienros I made all the requested changes and retested again. I also merged the 2 setting properties with LoginSettings

[jtkech]

@MikeAlhayek Okay, can't right now but will do another review asap.

[jtkech]

@MikeAlhayek Okay, so from memory before completing the review.

• First, I saw that the user role "Administrator" was not normalized on setup, It may explain in some places the need to be case insensitive, I opened a separate PR Normalize user "Administrator" role on setup. #13086 but also updated this PR.

• Okay, I gave up checking case sensitivity for Roles, looks like we were already mixing them with User Roles that are normalized. Currently it works because we use OrdinalIgnoreCase but normally we would need to use the registered normalizer without assuming that it just converts to uppercases.

  Otherwise, why using in some places RoleManager.NormalizeKey() or a ILookupNormalizer _keyNormalizer as in UserByRoleNameIndex. So maybe in a separate PR if worth to do.

  Note: Yes, at some point I was also suggesting to remove OrdinalIgnoreCase from SystemRoleNames = new(StringComparer.OrdinalIgnoreCase) but because of the above for now we can't.

• About using the existing ViewUsers in place of the new ListUsers, see Extend user permissions #12407 (comment), maybe the last thing to discuss but in the meantime better to remove what you just added to justify both permissions.

• From memory one last use case was when the user can't list any user but can edit his profile from the top right user dropdown menu => edit profile => click Cancel => Forbidden page because Cancel try to go to the users list, would need to return back from where the user was, maybe in a separate PR.

  For now we need at least to allow the user to list users of the same roles, so not only himself. Maybe we now are missing a ViewOwnUser OR a ListOwnUser permission (implied by the existing ManageOwnUserInformation), this to allow a user that can edit himself to see the users list page even if he only see himself. Hmm, with the solution allowing this user to see others (oops to list others), at least we can prevent him from editing these other users. So maybe in a separate PR if worth to do.

So looks like the only main remaining point is to discuss a little more to be sure of what is better to use, the existing ViewUsers or the new ListUsers. Personnaly I have no preference about the name, just because ViewUsers already exists and is already used by existing installations, particularly those having many users, still using ViewUsers would be more compatible.

[jtkech]

@MikeAlhayek Okay so from memory the last things would be.

• Okay, ListUsers does not bother me, I let you decide if better to remove what you added to not let ViewUsers unused.

• About normalizing RoleName(s) when comparing to User.RoleNames in place of just being case insensitive, this by using e.g. RoleManager.NormalizeKey() or just get role names with the right service method e.g. GetNormalizedRoleNamesAsync(), maybe worth to do, maybe at least in UserRoleDisplayDriver.

• In UserInformationDisplayDriver.cs there is a remaining OrdinalIgnoreCase in IsCurrentUser().

[MikeAlhayek]

@jtkech

About normalizing RoleName(s) when comparing to User.RoleNames in place of just being case insensitive, this by using e.g. RoleManager.NormalizeKey() or just get role names with the right service method e.g. GetNormalizedRoleNamesAsync(), maybe worth to do, maybe at least in UserRoleDisplayDriver.

Strange, when I look at my production database, the UserNames list on the User object is stored as standard name not normalized. I don't understand how since the UserDisplayDriver pass normalized name to the UserStore. The problem here if we do check using case sensative, we cannot remove old roles. For example, if a user has Editor role, and we try to add EDITOR, it wont get added. That is because IsInRoleAsync actually does case-insensitive check. SoIsInRoleAsync will return true in both Editor or EDITOR case which prevent from adding EDITOR. I don't really understand how non-standard role names are stored in the database currently!

In UserInformationDisplayDriver.cs there is a remaining OrdinalIgnoreCase in IsCurrentUser().

Yes I changed this.

[jtkech]

@MikeAlhayek

When we use the aspnet UserManager to add a role we don't need to pass a normalized role, it is normalized by the user manager itself.

    public virtual async Task<IdentityResult> AddToRoleAsync(TUser user, string role)
    {
        ThrowIfDisposed();
        var userRoleStore = GetUserRoleStore();
        if (user == null)
        {
            throw new ArgumentNullException(nameof(user));
        }

        var normalizedRole = NormalizeName(role);
        if (await userRoleStore.IsInRoleAsync(user, normalizedRole, CancellationToken))
        {
            return UserAlreadyInRoleError(role);
        }
        await userRoleStore.AddToRoleAsync(user, normalizedRole, CancellationToken);
        return await UpdateUserAsync(user);
    }

Then this is the UserManager that calls the UserStore.AddToRoleAsync() to add the role that was normalized, normally not good to use the UserStore directly, see below how we check that it is a valid role from the RoleService by normalizing the role before the comparison.

    public async Task AddToRoleAsync(IUser user, string normalizedRoleName, CancellationToken cancellationToken)
    {
        if (user == null)
        {
            throw new ArgumentNullException(nameof(user));
        }

        if (user is User u)
        {
            var roleNames = await _roleService.GetRoleNamesAsync();

            if (!roleNames.Any(r => NormalizeKey(r) == normalizedRoleName))
            {
                throw new InvalidOperationException($"Role {normalizedRoleName} does not exist.");
            }

            u.RoleNames.Add(normalizedRoleName);
        }
    }

Yes not good to use the UserStore directly (idem for the RoleStore normally. Hmm, seems that we are doing it in UserRoleDisplayDriver through the IUserRoleStore interface.

I will have a meeting in 5 minutes, I will look at it later on.

[MikeAlhayek]

@jtkech yes I understand. Based on the code, normalized roles name should be store. But, when I look at my production database "using v1.5" I see Editor not EDITOR in the document table. Something is storing the names not in a normalized way.

[jtkech]

@MikeAlhayek

Strange, when I look at my production database, the UserNames list on the User object is stored as standard name not normalized.

Okay would be good to understand why, will see tomorrow

The problem here if we do check using case sensative, we cannot remove old roles. For example, if a user has Editor role, and we try to add EDITOR, it wont get added.

Yes I agree. Or maybe the opposite, because it is case insensitive it say that it already exists, need to be checked.

I don't really understand how non-standard role names are stored in the database currently!

Yes would be good to understand, will see tomorrow ;)

[jtkech]

Anyway, any existing installation has at least the admin user with a non normalized "Administrator" role, so I think that for now we will not change anything about comparing Roles and User.RoleNames.

But still worth to understand what hapenned in your case.

[MikeAlhayek]

@jtkech I changed the code in the driver and the views to do a case insensitive check. Everything is working as expected now at least for this PR. If you see no additional issues, lets finish it and merge it.

As far as why my databases have standard name in the roleNames, I can't really explain it. The fact that I can't explain it is driving me nuts.

[jtkech]

@MikeAlhayek

I had no time to answer to your other comments, I will do.

But for now

As far as why my databases have standard name in the roleNames, I can't really explain it. The fact that I can't explain it is driving me nuts.

No problem, I could repro by resetting to an old version, so normally I will be able to find since when and why the RoleNames now are normalized (which is the right behavior).

[MikeAlhayek]

@jtkech no problem. I'll wait your respond. What version did you use to reproduce the name invariance issue? I want to see if we can spot the change the caused the change.

Normalized name is the right one to store, but if we keep this change, we should also update the UserStore or event make RoleNames case-insensitive so we are not doing case insensitive check all over the place. Any additional changes, would be in a separate PR. I don't want to block this PR.

[jtkech]

@MikeAlhayek Here the history

07 janvier => Not normalized
    In AddToRoleAsync we were using ((User)user).RoleNames.Add(roleName);
08 janvier => Can't save User Roles, there is an issue.
10 janvier 02:36 => Fix the above issue but
    In AddToRoleAsync we now use u.RoleNames.Add(normalizedRoleName);

So now we understand things, just need to think about what is now the best compromise to do.

or event make RoleNames case-insensitive

We can't define a case-insensitive in the Document, when deserializing a json Document this is something that is not preserved.

[jtkech]

So in the previous AddToRoleAsync() we were normalizing the existing Roles to compare them to the normalizedRoleName passed as a parameter (e.g. called by the aspnet service), but in fact in the User Document we were enlisting non normalized RoleNames, the same names as existing Roles.

public async Task AddToRoleAsync(IUser user, string normalizedRoleName, CancellationToken cancellationToken)
{
    if (user == null)
    {
        throw new ArgumentNullException(nameof(user));
    }

    var roleNames = await _roleService.GetRoleNamesAsync();
    var roleName = roleNames?.FirstOrDefault(r => NormalizeKey(r) == normalizedRoleName);

    if (string.IsNullOrWhiteSpace(roleName))
    {
        throw new InvalidOperationException($"Role {normalizedRoleName} does not exist.");
    }

    ((User)user).RoleNames.Add(roleName);
}

[MikeAlhayek]

@jtkech I see the issue is caused by the change in the UserStore. I think we should submit a PR that would return the same behavior we had prior the last change. Then, maybe we can introduce a new PR that would change thing everywhere. I don't want to change the case sensitive check in this PR. If you agree with this approach, I can submit a quick fix for the UserStore unless this is something you are already working on.

[jtkech]

@MikeAlhayek

• Okay I merged the main branch, this reverted the SetupEventHandler change that we did here.

• Do we need to keep what you added just to have something that requires ViewUsers?

• So, because user role names are populated with existing roles (or have existed), I think it is better to be case sensitive, but yes it may break recent sites created since around one week, normally not so many sites and knowing that the "Administrator" user role was still not normalized during this period.

  Anyway will not block this PR but tomorrow I will see what happens if we are case sensitive and if some user roles names are already normalized in the database, if there is a simple way to replace them.

Need to leave, will see tomorrow, anyway this PR will be merged tomorrow.

[MikeAlhayek]

@jtkech thanks for the fix in the other PR.

Do we need to keep what you added just to have something that requires ViewUsers?

Yes I think they are all good additions. We now provide View profile function along with ListUsers permissions. I think this PR is ready. I'll give it another test tomorrow morning just to make sure everything is still working after the latest changes. I'll ping you again after I validate one last time.

[jtkech]

@MikeAlhayek

Yes, no luck about the case sensitivity, because of possible installations created since the last 10 days, but that's okay, there are not so many remaining OrdinalIgnoreCase.

Just committed a minor change for a missing usage of RoleHelper.SystemRoleNames in the AdminController, you can review my last commit.

Then I will approve and let you retry it before merging.

[MikeAlhayek]

@jtkech I noticed a minor issue in the UI that I fixed. I'll merge it once the CLI is done.