Skip to content
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

Don't normalize User.RoleNames #13104

Merged
merged 3 commits into from Jan 18, 2023
Merged

Don't normalize User.RoleNames #13104

merged 3 commits into from Jan 18, 2023

Conversation

jtkech
Copy link
Member

@jtkech jtkech commented Jan 17, 2023

As it was before.

@@ -462,13 +462,14 @@ public async Task AddToRoleAsync(IUser user, string normalizedRoleName, Cancella
if (user is User u)
{
var roleNames = await _roleService.GetRoleNamesAsync();
var roleName = roleNames?.FirstOrDefault(r => NormalizeKey(r) == normalizedRoleName);

if (!roleNames.Any(r => NormalizeKey(r) == normalizedRoleName))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!roleNames.Any(r => NormalizeKey(r) == normalizedRoleName))
if (String.IsNullOrEmpty(roleName))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we check RoleNames not RoleName

For now I just copy paste the code as it was before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jtkech yes I understand. you don't need to check names all over again since you are doing the same check in the previous line.

The code should look like this if you want to drop ? since the GetRoleNameAsync() would return collection not nullable.

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

                var roleName = roleNames.FirstOrDefault(name => NormalizeKey(name) == normalizedRoleName);

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

                u.RoleNames.Add(roleName);
            }
        }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But yes better, done

@@ -462,13 +462,14 @@ public async Task AddToRoleAsync(IUser user, string normalizedRoleName, Cancella
if (user is User u)
{
var roleNames = await _roleService.GetRoleNamesAsync();
var roleName = roleNames?.FirstOrDefault(r => NormalizeKey(r) == normalizedRoleName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var roleName = roleNames?.FirstOrDefault(r => NormalizeKey(r) == normalizedRoleName);
var roleName = roleNames.FirstOrDefault(r => NormalizeKey(r) == normalizedRoleName);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will check, for now here it is as it was before

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done locally

@@ -482,13 +483,14 @@ public async Task RemoveFromRoleAsync(IUser user, string normalizedRoleName, Can
if (user is User u)
{
var roleNames = await _roleService.GetRoleNamesAsync();
var roleName = roleNames?.FirstOrDefault(r => NormalizeKey(r) == normalizedRoleName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var roleName = roleNames?.FirstOrDefault(r => NormalizeKey(r) == normalizedRoleName);
var roleName = roleNames.FirstOrDefault(r => NormalizeKey(r) == normalizedRoleName);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -482,13 +483,14 @@ public async Task RemoveFromRoleAsync(IUser user, string normalizedRoleName, Can
if (user is User u)
{
var roleNames = await _roleService.GetRoleNamesAsync();
var roleName = roleNames?.FirstOrDefault(r => NormalizeKey(r) == normalizedRoleName);

if (!roleNames.Any(r => NormalizeKey(r) == normalizedRoleName))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!roleNames.Any(r => NormalizeKey(r) == normalizedRoleName))
if (String.IsNullOrEmpty(roleName))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idem

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

EmailConfirmed = true
};

user.RoleNames.Add("Administrator");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is safer to use RoleNames = new string[] { "Administrator" }, but up to you. Maybe just undo the change to this class completely.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the little fix is to not set the IList RoleNames to a string array, otherwise it saves in the json document the following.

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

In place of

"RoleNames":["Administrator"]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we add a role to an empty list because the user is created just before.

Copy link
Member

@MikeAlhayek MikeAlhayek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jtkech I think this should be merged ASAP. I am approving this change.

@jtkech
Copy link
Member Author

jtkech commented Jan 18, 2023

Okay, just waiting for unit tests

@jtkech
Copy link
Member Author

jtkech commented Jan 18, 2023

Please wait, I'm testing it

@jtkech jtkech merged commit 07698e4 into main Jan 18, 2023
@jtkech jtkech deleted the jtkech/user-roles branch January 18, 2023 00:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants