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.

Sign up for GitHub

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

Jump to bottom

Update permissions when a role name is changed #2599

Merged
merged 3 commits into from
Jan 13, 2020
Merged

Update permissions when a role name is changed #2599

merged 3 commits into from
Jan 13, 2020

Conversation

mperk
Copy link
Contributor

@mperk mperk commented Jan 9, 2020

Resolve #215
When the role name is changed, the event handler update permissions

@hikalkan hikalkan added this to the 2.0 milestone Jan 9, 2020
@hikalkan hikalkan requested review from maliming and hikalkan January 9, 2020 16:00
@hikalkan
Copy link
Member

hikalkan commented Jan 9, 2020

Thank you @mperk :) We'll review this.

@maliming
Copy link
Member

hi @mperk
I think even if the role name can be changed, we should add DisplayName to the IdentityRole. This way we can provide a description for the role. what do you think?

maliming
maliming reviewed Jan 10, 2020
View reviewed changes
...nManagement.Domain.Identity/Volo/Abp/PermissionManagement/Identity/RoleUpdateEventHandler.cs Outdated Show resolved Hide resolved
...nManagement.Domain.Identity/Volo/Abp/PermissionManagement/Identity/RoleUpdateEventHandler.cs
var permissionGrantsInRole = await PermissionGrantRepository.GetListAsync("R", eventData.OldName).ConfigureAwait(false);
foreach (var permissionGrant in permissionGrantsInRole)
{
await PermissionManager.UpdateProviderKeyAsync(permissionGrant, eventData.IdentityRole.Name).ConfigureAwait(false);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that the IPermissionManager should add the UpdateProviderKeyAsync method.

Maybe it can use IPermissionGrantRepository directly. so some changes below can be undone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. I can move in the repo.

Copy link
Member

Choose a reason for hiding this comment

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

It seems that we have ignored the above discussion. : )

@mperk
Copy link
Contributor Author

mperk commented Jan 10, 2020

hi @mperk
I think even if the role name can be changed, we should add DisplayName to the IdentityRole. This way we can provide a description for the role. what do you think?

I am not sure. When the system is using, the role names may be confused after a while.
This senario can be a problem:
Ex. a role before change:
Name: admin
NormalizedName: ADMIN
DisplayName: admin

after change:
Name: admin
NormalizedName: ADMIN
DisplayName: readonly admin

@hikalkan hikalkan merged commit e7e9152 into abpframework:dev Jan 13, 2020
@hikalkan
Copy link
Member

Thank you @mperk for your contribution.

@hikalkan hikalkan changed the title #215 role name should be unchangeable Update permissions when a role name is changed Jan 13, 2020
@maliming maliming mentioned this pull request Feb 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maliming maliming maliming left review comments

@hikalkan hikalkan Awaiting requested review from hikalkan

Assignees
No one assigned
Labels
abp-module-identity enhancement
Projects
None yet
Milestone
2.0
Development

Successfully merging this pull request may close these issues.

Role name should be unchangeable
3 participants
@mperk @hikalkan @maliming