OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.37k stars 2.38k forks source link

Remove SiteOwner permission #16763

Open MikeAlhayek opened 2 days ago

MikeAlhayek commented 2 days ago

I think SiteOwner permission should be removed and not used any more. At someone point it probably made sense to have that permission, but I don't think we need it any more.

github-actions[bot] commented 2 days ago

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

hishamco commented 2 days ago

What about the current services that rely on it?

MikeAlhayek commented 2 days ago

They should stop relying on it. Each thing should have it's own permission instead of using this.

Piedone commented 1 day ago

Let's leave assigning the milestone to the triage meeting, please. This is not at all an obvious thing but rather something that should be discussed.

Also, more rationale would be good: why do you think it should be removed, why nothing should rely on it? What's the problem with it?

MikeAlhayek commented 1 day ago

this was discussed a while back on a triage and agreed that it should be removed. So I am logging these things in the next major release.

@sebastienros

Piedone commented 1 day ago

What was discussed there? Because I'd be interested in the rationale.

I don't think it should be removed, at most made internal, i.e. something that you can't authorize against. Because the Site Owner permission has two uses:

MikeAlhayek commented 1 day ago

I don't remember the exact details since this was discussed a while back. I am okay with removing the milestone and discuss this again.

But if you are trying to accomplish your scenario "God power" would be a role not a permission. For example, anonymous is a role not a permission. In this case, being an Administrator by default gives you all the possible permissions.

If you look at how we use SiteAdmin, you'll notice that we check it in specific places in addition to global. For example recipes, what if we want a specific user to import recipes but they are not the owner.

Piedone commented 17 hours ago

If you want a role to be able to do anything, then currently, you only need to give it the Site Owner permissions. Using a role is no substitute for this, since the alternative would be to click every single permission under a given role, as well as do this for any future permission when new ones are added. You can't depend on permission defaults to be always suitable for you (not all new features of OC may add new permissions to the Administrator role, nor can we expect third-party features to do this), especially if your role is not named "Administrator". This is a worse experience for maintaining a site.

To put it otherwise, IMO DefaultPermissionGrantingService should stay as-is. And again, for the places where Site Owner is directly authorized against (i.e. used as _authorizationService.AuthorizeAsync(User, StandardPermissions.SiteOwner)) it should be removed in favor or a functionality-specific permission.

MikeAlhayek commented 16 hours ago

I think for now, we should not be checking for SiteOwner explicitly like we do in couple places. As for as not needing to select permission, I’ll have a PR around roles that would help with this.