Normalize user "Administrator" role on setup.

[jtkech]

On setup in the SetupEventHandler we set the user.RoleNames to a string array in place of a list, and with the Administrator role name in place of the normalized ADMINISTRATOR.

So, when saved in the database document we have.

"RoleNames":{"$type":"System.String[], System.Private.CoreLib","$values":["Administrator"]}

If we save again, we no longer see the string array type, but the role is still not normalized.

"RoleNames":["Administrator"]

If we add a role, only the new role is normalized.

"RoleNames":["Administrator","AUTHOR"]

---

After the fix, the role is normalized from the beginning, on setup.

"RoleNames":["ADMINISTRATOR"]

It may explain in some places the need to be case insensitive when comparing normalized role names to user roles, and/or in other places maybe some unexpected behaviors.

[MikeAlhayek]

@jtkech I am not sure about this PR any more. Currently, we store the role name in the RoleNames collection not the normalized.

RokeNames is not stored as normalized

[jtkech]

@MikeAlhayek

See my comment #12407 (comment)

Hmm, normally better to use the UserManager but I thought that on setup we need to do that immediately. Can't right now but I will try it.

[MikeAlhayek]

Yes I understand. I am not using UserStore directly. In your database run the following query and inspect the RoleNames property in the User object.

  SELECT d.Content
  FROM [UserIndex] AS u
  INNER JOIN [Document] AS d ON d.Id = u.DocumentId

[jtkech]

Look directly User records in the Document table, sorry need to join my meeting

[jtkech]

@MikeAlhayek

Yes I understand. I am not using UserStore directly

No problem, I meant that e.g. in UserRoleDisplayDriver we already use IUserRoleStore so we have to normalize roles, this in place of using UserManager directly.

await _userRoleStore.AddToRoleAsync(user, _userManager.NormalizeName(role), default);

Why not just using

await _userManager.AddToRoleAsync(user, role);

At least we inject IUserRoleStore not UserStore so not assuming that UserStore is the current IUserRoleStore. Okay maybe intended, to not re-do for each role all what the UserManager does.

    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);
    }

So that's okay.

---

About the User.RoleNames not stored as normalized, this is not what I have (see my first comment), unless for "Administrator" because of this issue. Can you retry after a fresh setup and then add roles.

Just retried with the current fix and then adding roles to a given user, what I got in the User Document.

"RoleNames":["ADMINISTRATOR"]
"RoleNames":["AUTHOR","CONTRIBUTOR","ROLE 1"]

---

That said one question, now that we add roles through recipes, is it a problem to still add the "Administrator" role in this setup event handler? Maybe features should still automatically add some system roles?

[MikeAlhayek]

Aside from refactoring or which service to use, I can't explain why my database has non-standard names. As mentioned before, one of my production instances is using OC v1.5 and it does not have normalized names.

[jtkech]

Aside from refactoring or which service to use, I can't explain why my database has non-standard names. As mentioned before, one of my production instances is using OC v1.5 and it does not have normalized names.

Is it migrated from an older version? How did you created your users?

Anyway, as commented above we will not change which services are used directly or not.

Hmm, even if in some places we are using OrdinalIgnoreCase to compare Roles to User.RoleNames, as I remember there are some places where we first normalize the Roles. Need to be clarified.

What about my above question, is still okay to assign here the "Administrator" role that now may not exist?

[jtkech]

Also found this code in UserByRoleNameIndexProvider

                if (!user.RoleNames.Any())
                {
                    return new UserByRoleNameIndex[]
                    {
                        new UserByRoleNameIndex
                        {
                            RoleName = NormalizeKey("Authenticated"),
                            Count = 1
                        }
                    };
                }

Maybe we need at least the 2 system roles Administrator and Authenticated and maybe Anonymous.

[MikeAlhayek]

I think if you wan to use .Add() you should check if the role exists first or not. Otherwise we could have another handler setting up the same role.

[jtkech]

I agree, did it too quickly for testing

[jtkech]

Oh yes, I thought about it but didn't do it as here we create a very new User().

[jtkech]

Also, what you think about this knowing that now the "Administrator" role may not exist.

For now I think that's okay because when we lookup User RoleNames, as I remember we compare them to existing Roles.

[MikeAlhayek]

@jtkech
Oh yes I noticed the user is created in the event. Maybe don't use .Add then it would be safer.

Good point on the roles. I think it is safe to add it since it won't add/remove value unless the roles module is enabled. But, maybe we can set the SuperUser property to true here on the new object here and don't set any role. Then we can add a new implementation to the Users event that would add Administrator rote only when the roles module is enabled. This new event would only be registered when Roles module is enabled

[jtkech]

Yes I used Add to not realloc a List.

Okay will look at the SuperUser property

[jtkech]

I'm closing this one, integrated in #13104