Roles Updating and History

[jtkech]

Just started, not ready to be reviewed and merged, planned to fix #13024 and fix #13035

So to fix the dynamic permissions in the Controller and on starup to take into account the existing tenants that don't have yet a permissions history.

Then maybe other things to do, there are many scenarios, need to be sure I well understand what should be done, I have ideas but need more time to coordinate them, here some first thoughts.

• Not sure it"s good to assume that RoleStore rely on IDocumentManager, maybe too late.

• I would like, if possible, to prevent from evaluating the permission providers on each shell activation, by only using IFeatureEventHandler and maybe a new IRoleEventHandler, currently only IRoleRemovedEventHandler exists.

• Some Xxxed feature events are triggered in a deferred task, so a feature handler can be triggered when the feature that registers it is enabled. So could be used when Roles itself is enabled again.

---

• When we delete a role, should we remove its related history, currently not the case but I assume yes.

• When we lazily create a role, e.g. through the Admin UI, if it is a default role for some features, should we hydrate its permissions lazily, I assume yes, currently this is the case but only on the next startup.

• Hmm, maybe we only need to add to the history the permissions that we remove (to not be auto added again) but not the others as we do on startup.

• RoleStep: Hmm a little annoying, it specifies permissions (can be empty) that also rely on features activation, maybe the step should merge (not replace) permissions, at least when in a setup recipe, or maybe a new step for role creation only and if it already exists don't replace/clear its permissions.

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

@MikeAlhayek I will let you check if it still fits your needs.

[jtkech]

Nothing tried yet so not fully sure of the following, and as a reminder of my current analysis.

• We now execute the logic on feature Enabled, maybe not needed as when we enable a feature the shell is then activated and we also execute the logic on shell activation.

• Maybe we could still run the logic on feature Installed (only once) instead so that it works as before for existing roles, here also useless if we keep the execution on shell activation but I plan to not do it.

• Then maybe a Role Created event (only the Removed event exists) on which we could populate the role based on the providers of the current enabled features. So above we populate existing roles on a new feature Installed, and here we populate a new role by using enabled features.

  The Role Removed event already exists, we could use it to clear the related history.

• Hmm, but if we re-use the Feature Installing event, a given feature will not be able to re-assign a permission, as before unless that a feature Installed will not create anymore a role if it doesn't exist.

  Then if we use a new Role Created (maybe Creating) event, no problem to auto assign any permission on this new role.

• So it seems that we could re-use the code as it was before, just tweak the Feature Installing event and add a new Role Creating event, and it seems that we don't need to change the Controller and that we don't need anymore the PermissionGroups history in the RoleDocument.

---

Just for info about other things I saw and other thoughts I had previously.

• In the Controller maybe something not as expected, the only form checkbox keys that are bound are the checked ones, so if on a first edit we uncheck an auto assigned permission, on submit it will not be added in the history, we may not see this because the history is also updated on shell activation.

  In fact at some point I wanted to only add to the history the unchecked permissions that we don't want to auto assign again, and add a check when we auto assign to not add a duplicate, even if maybe easier to filter if we add all of them in the history as seems to be done on shell activation.

  This to only update the history in the Controller Edit where we may concretely uncheck permissions, no more on shell activation. Otherwise it seems that if we still execute the logic on shell activation, what is done in the Controller is not needed.

  Finally if we still add to a role history all auto assigned permissions per feature, maybe we would only need to add to the history the feature Ids for which it has been already auto assigned.

---

But as said in the first section, we may not need all of this if we re-use the Feature Installing event and a new Role Creation event, without anything on shell activation and without any history.

Oh yes forgot to talk about the case where this is the Roles feature itself that is disabled and then re-enabled, but as said in a previous comment.

• Some Xxxed feature events are triggered in a deferred task, so a feature handler can be triggered when the feature (that registers it) is e.g. Enabled. So could be used when Roles itself is enabled again.

  This to re-evaluate the providers of enabled features. Hmm, do we really need to support this case because here the need of a kind of history may come back, if we still don't want to re-assign a permission that we removed when the related feature was enabled and before Roles was disabled.

  If really needed maybe an history with only feature Ids for which a role was already auto assigned, and by still only using the event handlers discussed in the first section, still nothing on shell activation and in the controller, will see tomorrow.

[jtkech]

@sebastienros @MikeAlhayek

• So here the idea is to 1st re-use the Feature Installed event (but without creating any missing default role that now are created by recipes or later on) so that it works as before for default roles that already exist, here no need to have an history to prevent a permission re-assignment because the feature Installed event is triggered once per feature.

• Then use a new Role Created event so that we can update a default role that is created later on by using the permissions providers of the current enabled features, here also no need to have an history to prevent a permission re-assignment because it is a new role.

• Doing so I could revert the change we did in the AdminController which fixes Dynamic permission can't save in role #13024, and removed what we added on shell activation which fixes Role permission settings are overwritten if a tenant is started #13035. I could also remove the history management that was using directly the RoleDocument Manager.

---

The only thing is when we disable and re-enable the Roles itself, we don't update the roles for the features that were enabled in the meantime, because we would need again an history to not re-assign permissions to a given role that were explicitly removed before Roles was disabled.

Let me know if we really need this.

If so maybe an history but with only feature Ids for which a role was already auto assigned, and by still only using the event handlers, still nothing on shell activation and in the controller.

[MikeAlhayek]

@jtkech IMHO, I think we should to keep track of any permissions that are explicitly remove or added to a role. If an admin included/excluded a permission we should not revert their changes.

I also think, the permission provider should be evaluated when a feature is installed and also when it is enabled. This way when any feature is enabled we assign any permission provided by the feature that were not explicitly removed by an admin. Keep in mind, there maybe more permission provided by a feature after the initial installation. At the same time we should honor any specific changes an admin included/excluded.

Question for clarification, beside the bottom code change in the controller, what other issues are you trying to solve?

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

[jtkech]

If an admin included/excluded a permission we should not revert their changes.

This is the case here if you analyze my comments.

This way when any feature is enabled we assign any permission provided by the feature that were not explicitly removed by an admin.

Idem, this is the case here.

Keep in mind, there maybe more permission provided by a feature after the initial installation.

Yes I thought about this use case, would be one use case if we want to support this.

At the same time we should honor any specific changes an admin included/excluded.

This is the case here.

Question for clarification, based on the current changes in this PR "5 line changes"

You may need to refresh the Files changed page, there are much more lines changed.

what other issues are you trying to solve?

At least the current 2 breaking issues mentioned in my comments, for other improvements (if possible) as not running the logic on each shell activation, not use the role document directly, remove the useless code in the controller, reduce the role document that may be in a shared cache, reffer to my comments.

For all the above points, please read my previous comments and try it for confirmation.

---

For me there are only 2 cases where we would need an history, the one you mentioned when we upgrade a feature that is already installed and that provides more permissions. Then you didn't answer to the question if we really need to support the case when the Roles itself is disabled and re-enabled.

At least here we fix 2 issues and we did a lot of cleanups, then if we want to support one of the 2 above cases, as described in my previous comments I can re-use a kind of history but still in a simpler way.

[jtkech]

For infos 2 things I'm thinking about and that I already described in previous comments.

• In the current implementation no need to subscribe to both shell activation and feature enabled events, when we install / enable a feature, the shell is rebuilt and then activated again.

• In the controller only checked checkboxes are retrieved and then added to the history.

So just to say that anyway many things needed to be cleaned up.

Just saw your above edited comment.

Question for clarification, beside the bottom code change in the controller, ...

I don't use anymore this code, here the controller is as it was before we move role creation in recipes.

[MikeAlhayek]

@jtkech Let me know when you are done with all you changes and I'll test it out.

[jtkech]

@MikeAlhayek

You can already test it knowing that here the Controller and RoleUpdater are as they were before we moved roles creations in recipes, the only thing I added is the new Role Created event.

And that for now the 2 below points are not supported, because we would need to re-use the History.

• When we disable and re-enable OC.Roles itself, to take into account the features installed in the meantime. Hmm for now we break the site if we disable OC.Roles, so maybe in a future PR, but if it is confirmed that we really need to support this use case, I could re-use the History.

• When a feature is upgraded and provides new stereotypes, maybe here would also need to execute again some logic on each shell activation. Hmm maybe quite a rare use case, maybe we are missing a feature Upgrade event, so maybe in a future PR, but idem if it is confirmed that we really need it.

Hmm what we now support here is when we create a new role (having a default role name) from the UI, it is immediately populated with the auto-assigned permissions by the new Role Created event.

[MikeAlhayek]

@jtkech if this is ready you may want to remove the notready tag.

I just tested it and both #13035 and #13024 and I can't reproduce the issues. So the changes fixed them.

When we disable and re-enable OC.Roles itself, to take into account the features installed in the meantime. Hmm for now we break the site if we disable OC.Roles, so maybe in a future PR, but if it is confirmed that we really need to support this use case, I could re-use the History.

PR #12420 should be merged once we get the failing tests to work. So I think we should account for it.

When a feature is upgraded and provides new stereotypes, maybe here would also need to execute again some logic on each shell activation. Hmm maybe quite a rare use case, maybe we are missing a feature Upgrade event, so maybe in a future PR, but idem if it is confirmed that we really need it.

I am not sure I understand here.

Anything specific you want me to test out?

[jtkech]

@MikeAlhayek

About OC.Roles, yes I know that it will be possible to disable it through #12420, I may want to look at this PR, but because it is not yet done maybe we could merge this PR first to fix the current issues.

Hmm but if you really want to support this use case, we could wait a little before merging, I will think about this use case and will let you know.

About the 2nd point I was referring to your comment

Keep in mind, there maybe more permission provided by a feature after the initial installation

Here I understood if the code implementation change => maybe more default permissions.
This for an already Installed feature, right?

If so, I will also think about this other use case and will let you know.

[jtkech]

@MikeAlhayek

Okay, as you moved some default services to core projects in #12420, maybe we could think about the same for all or part of the RoleUpdater so that we could still be hooked by the feature event(s) even if OC.Roles is disabled.

Hmm, but we would still need to use the Roles Document Manager directly to update the underlying roles even if they are not actives, but which is already the case in the current implementation, but no more the case at the moment in this PR, will see.

I could start on this when #12420 will get merged, hmm but if it takes some time maybe better to first also merge this PR, will see.

[jtkech]

@MikeAlhayek

Okay I merged #12420, will look at this one again, anyway the first goal is to merge something this week end to fix the 2 current issues.

Can you provide a patch to update #12953?

[MikeAlhayek]

@jtkech a patch for #12953 isn't blocking anything. However, I created a PR #13051 as a patch that I'll test it out more tomorrow.

[jtkech]

@MikeAlhayek

Okay I tried to move RoleUpdater to Users.Core, works well but by also moving RolesDocument it changes the assembly name of the type and will break existing sites as not retrieved by YesSql.

So I created a new feature OC.Roles.Updater marked as EnabledByDependencyOnly on which both Users and Roles depend, so that if OC.Roles is disabled, the underlying roles are still updated on feature installations, this by using the RolesDocument directly as before as I had no choice.

You can try it to see if all your needs are fit, the only remaining use case is when the code of a feature is upgraded AND provides more default permissions than before.

Edited: Hmm, the above would mean that this feature upgrade "installs" something new, so the recommendation could be to add to this feature a dependency on a new feature that will be auto Installed and that at least (can be used for other things) provides these new permissions.

---

2 quick questions, too lazy to verify atm.

• Is it documented that people using custom setup recipes should now use the RoleStep?

• I thought that when a feature is EnabledByDependencyOnly the disable button would be inactive, that's said when clicking it the feature is not disabled, so not critical.

[MikeAlhayek]

The issue with the disabled button is fixed in PR #12950

I'll look at the other stuff tomorrow

[MikeAlhayek]

@jtkech

Is it documented that people using custom setup recipes should now use the RoleStep?

No there isn't. The only place this is mentioned. I think the only place this is mentioned is in the PR itself. I think we should document this in 1.6 release notes. Also, we may want to add a note on https://docs.orchardcore.net/en/latest/docs/reference/modules/Recipes/#roles-step . Can you please make this part of this PR?

So I created a new feature OC.Roles.Updater marked as EnabledByDependencyOnly on which both Users and Roles depend, so that if OC.Roles is disabled, the underlying roles are still updated on feature installations, this by using the RolesDocument directly as before as I had no choice.

Glad to see you using EnabledByDependencyOnly flag :). There are two other options you could use here but EnabledByDependencyOnly may be better here

1. You could use a migration to update the Type column in the Document table by replacing the old type with the new type.
2. You can use the SimplifiedTypeName attribute which I am not a fan of.

If we you go the EnabledByDependencyOnly route, can we name it OrchardCore.Roles.Core also add the following services to it.

services.AddScoped<RoleManager<IRole>>();
services.AddScoped<IRoleService, RoleService>();

If you add them here, be sure to also remove them from OrchardCore.Roles and AddUsers() extension

[jtkech]

Okay for OrchardCore.Roles.Core feature, done.

Do you want to test it out before merging ?

In the meantime I will add something in the doc.

[MikeAlhayek]

I'll test it now.

[MikeAlhayek]

Small feedback while I am waiting on the project to build

[MikeAlhayek]

@jtkech for some I can't get your branch to run locally. I have to leave here soon and do not want to block your PR.

A code review seems to be good. If you tested it and feel good about it, feel free to merge it. I can do more testing once it's merged.

[jtkech]

Hmm, I think I missed one use case.

• Install a feature (first enabling).

• Disable this feature.

• Create a Default role.

• Enable the feature.

Would need to know the roles that didn't saw the installation of a given feature, so that enabling again this feature would be like a pseudo installation for these roles. I will think about it.

But still worth to try the other use cases, and if they work maybe first merge this PR, then I will work on the above case, but only if we really need to support it.

[jtkech]

@MikeAlhayek

Okay just saw your last comment, no problem I will merge soon this PR to fix the current issues. About the last use case, I think I found a solution that would be consistent with existing tenants, so I'm waiting one day more before merging, will see tomorrow.

[MikeAlhayek]

@jtkech thank you. Once you merge this one, I'll fix new conflict in #12407 and get it tested once again. I have to finish #12407. I don't want to keep resolving conflicts :)

[jtkech]

@MikeAlhayek

Okay I implemented the last use case.

We persit the missing features per role, those that are installed but not currently enabled while creating a role, and only those defining permission providers (a few ones). When re-enabling such a feature we update the roles that missed it on creation and update the cache. When removing a Role we remove the missing features for this role.

Let me know if you want to try it.

[MikeAlhayek]

I was having hard time building the project for some reason. I think if you feel good about this PR, you should merge it as the code looks good. I'll test it more once it's in the main branch.

[MikeAlhayek]

Maybe here we should use is directive

[jtkech]

Didn't want to change the behavior but will check

[jtkech]

Just used the same pattern as used below for the existing IRoleRemovedEventHandler.

        var roleToRemove = (Role)role;

[MikeAlhayek]

It seems weird to track missing items instead of what we know.

[jtkech]

See my last comment for the logic where we only use feature / role events, when creating a role we enlist the missing features (installed but not enabled) and only those defining providers, so that when re-enabling such a feature we can lazily update roles that missed it on creation.

Not the only reason but for example doing the opposite would be harder to manage existing tenants, would need a "migration" path to init the history for all existing roles assuming that all installed features was already used, so only using the events would not be sufficient.

Also doing this way, on startup nothing is done on existing tenants until a new role is created, also on a fresh setup this history is just empty, only populated when we disable a feature that provide perms for a given default role that doesn't exist yet, and then create this default role.

In summary when only using events, we need these missing features while creating a role, before it was not needed as default roles was auto created by features.

That's why I first kept the feature Installed event as before, and then because we now can create a default role later on, we need these missing features (installed but no more enabled) to lazily "install" them for this role when they are re-activated.

[jtkech]

That's said I may have done something wrong as there are so many scenarios I tried to keep while still fixing the 2 current issues. YOLO ;)

[mariojsnunes]

@jtkech Can you summarize the changes of this PR?
I have some tests that started breaking, seemingly when I add a user to a role, the role is now converted to uppercase.

[jtkech]

@mariojsnunes

This PR is only related to Roles and their Permissions, not about adding RoleNames to user.

the role is now converted to uppercase.

This is the right behavior relying on aspnetcore security services, as I have tried this is now the case unless for the "Administrator" user role because of another issue #13086.

But working with @MikeAlhayek he said to me that he has an existing installation where User.RoleNames are not normalized (not all uppercases), and it seems to be the case for you.

So maybe having normalized User.RoleNames (which is the right behavior) has been introduced at some point, we are investigating.

[MikeAlhayek]

@jtkech I thought maybe an issue with the normalizer, but that one uses ToUpperCaseInvarient() so it should not change. I tried to locate any changes around the UserDisplayDriver, UserStore and the ILookupNormalizer in recent changes but I do not see what could have changed.

Maybe something changed in the invariant culture in YesSql/sterilizer or in OC that I am not aware of?

[jtkech]

@MikeAlhayek See #12407 (comment)

[jtkech]

@mariojsnunes Fixed by #13104

[mariojsnunes]

Awesome, thanks!!