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

fix role update #13025

Closed
wants to merge 3 commits into from
Closed

fix role update #13025

wants to merge 3 commits into from

Conversation

hyzx86
Copy link
Contributor

@hyzx86 hyzx86 commented Jan 2, 2023

fix : #13024

Planned to also fix #13035.

chrome_HV9oeSsdLF.mp4

@jtkech
Copy link
Member

jtkech commented Jan 3, 2023

@hyzx86

Thanks for re-opening this PR but maybe I will open a new PR as I already started to work on it locally to check different things as per my comment #13024 (comment)

@MikeAlhayek maybe a different approach for example to not execute the RoleUpdater on each shell activation if possible, I think I will ask you some questions to confirm what was intended to be done.

Hmm, one question on the fly, yes in the controller, dynamic permissions was not taken into account, hmm maybe we need to take them into account also in the RoleUpdater, will see.

@MikeAlhayek
Copy link
Member

@jtkech I in the RoleUpdater I think we have to call GetDefaultStereotypes() to group the permissions role this way we know which permissions should belong to Stereotype/role. If the grouped role is known to the current site, we then assign it. Once we assign a permission, we save the reference into RolesDocument.PermissionGroups this way we can know which permissions were auto assigned vs the assignments that were manually made "a user checking/unchecking" permissions.

In the Admin I agree we should not be using GetDefaultStereotypes()

We should be using the following as per your suggestions.

var permissionNames = new List<string>();
foreach (var permissionProvider in _permissionProviders)
{
    var permissions = await permissionProvider.GetPermissionsAsync();
    permissionNames.AddRange(permissions.Select(permission => permission.Name));
}

or

var allPermissions = await Task.WhenAll(_permissionProviders.Select(x => x.GetPermissionsAsync()));

@jtkech
Copy link
Member

jtkech commented Jan 4, 2023

Even if here all GetPermissionsAsync() seem to be synchronous (using Task.FromResult), and in that case it would be useless, I never use Task.WhenAll() ;) Unless I'm sure all are thread safe, which is not the case e.g. with ISession.

So okay the other breaking change, as stated by @gvkries #13035 (comment), is that on an existing installation not having yet the permissions history, some permissions get back to some roles on startup.

@jtkech
Copy link
Member

jtkech commented Jan 5, 2023

Okay I'm closing this one as I started to work on it on another branch #13040.

@hyzx86 @gvkries I will let you know when it will be ready to be tested.

@jtkech jtkech closed this Jan 5, 2023
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.

Role permission settings are overwritten if a tenant is started Dynamic permission can't save in role
3 participants