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

Deprecate SiteOwner permission and retain Administrator as system role #16781

Merged
merged 73 commits into from
Oct 3, 2024

Conversation

MikeAlhayek
Copy link
Member

@MikeAlhayek MikeAlhayek commented Sep 22, 2024

Fix #11920
Fix #16763

Site Owner Permission Deprecated, Administrator Role Retained as System Role

The SiteOwner permission has been deprecated and will be removed in future releases. To avoid breaking changes, a fast-track migration has been added that automatically assigns the Administrator role to any user with the SiteOwner permission.

The Recipes feature now authorizes against the new ManageRecipes permission, instead of relying on the deprecated SiteOwner.

If the existing Administrator did not have the SiteOwner permission, a new role will be generated and assigned as the system admin role. This role may be named Admin, SiteAdmin, SiteAdministration, SiteOwner, or Admin{N}, where {N} depending on availability to ensures a unique role name. Additionally, any user previously assigned the SiteOwner permission will automatically be granted this newly created role.

Recipes Feature

New 'Manage Recipes' Permission Added

Previously, only users with the SiteOwner permission could run recipes. Now, a new ManageRecipes permission allows you to grant recipe management capabilities to any role, providing greater flexibility in permission assignment.

Themes Feature

Users with 'Apply Theme' Permission Can List Themes

Previously, only users with the SiteOwner permission could list themes. Now, users with the existing ApplyTheme permission can also list and apply themes, enhancing theme management capabilities.

@MikeAlhayek MikeAlhayek marked this pull request as ready for review September 23, 2024 16:13
@Piedone
Copy link
Member

Piedone commented Sep 23, 2024

See #16763 (comment).

@gvkries
Copy link
Contributor

gvkries commented Sep 24, 2024

I don't think we should add a special flag like HasFullAccess to roles. Instead we may want to have a special system role for site owners though, to replace the SiteOwner permission with that. Adding something like HasFullAccess is quite similar to just continue using a special SiteOwner permission as we currently do. When we add a special system SiteOwnerRole, we would just use that for granting all access and it would be a clear separation of concerns. But I'm not sure if it's worth the effort.

Concerning the Type property, I don't see we would ever have anything else than System and Standard, so I would go for a simple flag (IsSystemRole or something like that). No need for another enum. And it doesn't have to be persisted as well.

@MikeAlhayek MikeAlhayek changed the title Add RoleType and HasFullAccess to IRole Add Type to IRole Sep 24, 2024
@MikeAlhayek
Copy link
Member Author

@gvkries the HasFullAccess property was replaced with Owner type. Let me know if you have anything else.

@sebastienros
Copy link
Member

the HasFullAccess property was replaced with Owner type. Let me know if you have anything else.

I am still seeing it in recipes. Maybe reflect it there too, or at least rename with IsOwner if there is absolutely no reason to use any other type in recipe or there is no way there will be a new type in the future. Otherwise use a RoleType string instead.

@MikeAlhayek
Copy link
Member Author

I am still seeing it in recipes. Maybe reflect it there too, or at least rename with IsOwner if there is absolutely no reason to use any other type in recipe or there is no way there will be a new type in the future. Otherwise use a RoleType string instead.

@sebastienros yes this way missed. Should be fixed now.

Copy link
Contributor

@gvkries gvkries left a comment

Choose a reason for hiding this comment

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

LGTM, I've only some minor notes/improvements if you don't mind.

Thanks @MikeAlhayek

@MikeAlhayek
Copy link
Member Author

@sebastienros do you like to add anything else here or you good with it?

continue;
}

var hasSiteOwner = r.RoleClaims.Any(x => x.ClaimValue == "SiteOwner");
Copy link
Member

Choose a reason for hiding this comment

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

We need to make it work for new sites.

Copy link
Member Author

Choose a reason for hiding this comment

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

It already does as during create roles will have no records so this logic will not be called. But for safety, I added a check against claims.

var roles = roleManager.Roles.ToList();

var adminRoles = new List<Role>();
var adminSystemRoleName = OrchardCoreConstants.Roles.Administrator;
Copy link
Member

Choose a reason for hiding this comment

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

Read settings["AdminRoleName] so users can define what they want to use.

Copy link
Member Author

Choose a reason for hiding this comment

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

This can now be configured from the settings using OrchardCore_Roles:AdminRoleName

@MikeAlhayek MikeAlhayek enabled auto-merge (squash) October 3, 2024 19:12
@MikeAlhayek MikeAlhayek merged commit c14840a into main Oct 3, 2024
7 checks passed
@MikeAlhayek MikeAlhayek deleted the ma/update-roles branch October 3, 2024 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants