Custom roles overwritten after upgrading to OC 2.1.x

[intellimedhu]

I am using custom roles where RoleClaims is empty (director, sales manager):

After upgrading to Orchard Core 2.1.x (from 2.0.2), I noticed that all users are assigned the Administrator role.

I suspect the issue is related to the following line of code:

OrchardCore/src/OrchardCore.Modules/OrchardCore.Roles/Migrations/RolesMigrations.cs

Line 46 in 4ab7c34

r.RoleClaims.Count == 0 ||

[MikeAlhayek]

Did any of the roles director sales manager has the OwnerPermission granted to it? https://docs.orchardcore.net/en/latest/releases/2.1.0/#site-owner-permission-deprecated-administrator-role-retained-as-a-system-role

Also it would be great if you can provide reproduction steps. Do you import a recipe?

[intellimedhu]

Roles are not assigned any permissions, including OwnerPermission.

Steps to reproduce:

1. Using OC 2.0.2 create roles in the admin: Director, Sales Manager
2. Do not assign any permissions to these roles.
3. Still in the admin dashboard, create some users and assign them only the Director or Sales Manager role (no other roles are given).
4. Upgrade to OC 2.1.0 (or higher), build, and run the application.
5. The Administrator role is now assigned to the users created in step 3.

[gvkries]

This is a security critical bug, it adds users to the admin role during migration unintentionally!

Note that it looks like PR #17089 also added the same buggy behavior here too, because the SiteOwner claim is also required by the RolesMigrations and is now never assigned - I told you, rushing the removal of that claim is a mistake ;-)

[MikeAlhayek]

This is a security critical bug, it adds users to the admin role during migration unintentionally

It is a bad security bug that will needs to be patched today.

Note that it looks like PR #17089 also added the same buggy behavior here too, because the SiteOwner claim is also required by the RolesMigrations and is now never assigned - I told you, rushing the removal of that claim is a mistake ;-)

It actually has nothing to do with that PR. It's a bug introduced by the original PR https://github.com/OrchardCMS/OrchardCore/pull/16781/files#diff-b72d3d57af6cb699ce3d2df36090731745456797fac627681ae3714a08c85cb9R44-R47

[gvkries]

It actually has nothing to do with that PR. It's a bug introduced by the original PR https://github.com/OrchardCMS/OrchardCore/pull/16781/files#diff-b72d3d57af6cb699ce3d2df36090731745456797fac627681ae3714a08c85cb9R44-R47

You're right, I got confused by reading SiteOwner claim type, but it's the value... Sorry :-)

[intellimedhu]

Thank you!