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

Do not obsolete the SiteOwner claim #17092

Closed
gvkries opened this issue Nov 28, 2024 · 9 comments
Closed

Do not obsolete the SiteOwner claim #17092

gvkries opened this issue Nov 28, 2024 · 9 comments
Assignees

Comments

@gvkries
Copy link
Contributor

gvkries commented Nov 28, 2024

          I would like to question this. Why did you obsolete the `SiteOwner` claim? As we don't have a fixed role for site owners, the information about which role is actually used for granting site owner permission is worth to be available in the claims. This doesn't mean that authorization should dependent on the claim alone.

Originally posted by @gvkries in #17089 (comment)

@kevinchalet kevinchalet self-assigned this Nov 28, 2024
@kevinchalet
Copy link
Member

We discussed why that claim was unnecessary here: #17087.

Hope it'll make things clearer 😃

@gvkries
Copy link
Contributor Author

gvkries commented Nov 28, 2024

Thanks, but I'm still not convinced. As we have removed all permission claims for administrators and we do not have a fixed administrator role name, by removing the site owner claim one cannot determine which role has specific permission from the claims alone. E.g. if you use Orchard in headless mode, there is no way to get that information.
Therefore we still need e.g. this claim. This doesn't mean that checking for authorization on the server side must use it.

@gvkries gvkries reopened this Nov 28, 2024
@kevinchalet
Copy link
Member

E.g. if you use Orchard in headless mode, there is no way to get that information.

The roles - including the administrator role - have always been and will always be stored as claims in both cookie and token principals, so you can simply do user.IsInRole(await _systemRoleNameProvider.GetAdminRoleAsync()) to achieve the same exact result.

Therefore we still need e.g. this claim.

No we don't. And given that it's no longer added/present/used in the 2.1.2 patch that shipped earlier today, it's extremely unlikely it will reappear in a future version.

It serves no purpose.

@gvkries
Copy link
Contributor Author

gvkries commented Nov 28, 2024

It serves no purpose.

It bears the information which role is the actual site owner role, as the name can be any. Of cause one can use the ISystemRoleNameProvider , but this information is not exposed anywhere as before in the claims.

@kevinchalet
Copy link
Member

It bears the information which role is the actual site owner role, as the name can be any.

No. It was a flag with a single value supported ("true") that was automatically added when the user had the administrator role... it didn't tell you what the administrator role name was... at all:

if (await _userManager.IsInRoleAsync(user, await _systemRoleNameProvider.GetAdminRoleAsync()))
{
    claims.AddClaim(StandardClaims.SiteOwner);
}
public static class StandardClaims
{
    /// <summary>
    /// This claim is assigned by the system during the login process if the user belongs to the Administrator role.
    /// </summary>
    public static readonly Claim SiteOwner = new("SiteOwner", "true");
}

@gvkries
Copy link
Contributor Author

gvkries commented Nov 28, 2024

Yes, that was badly expressed by me and not what I mean. I was trying to explain that having a claim of role X doesn't tell you, you have site owner permissions as before.

@kevinchalet
Copy link
Member

I was trying to explain that having a claim of role X doesn't tell you, you have site owner permissions as before.

If you have the administrator role claim, you're an administrator and you have the site owner permissions. It's as simple as that and that's exactly the information that "site owner" claim was conveying.

In any case, you shouldn't try to check for the claim yourself: you should instead use the authorization service to determine whether you're allowed to perform a specific action: if you're an administrator, it will do the role check for you and bypass all the permission checks.

@gvkries
Copy link
Contributor Author

gvkries commented Nov 28, 2024

I understand what you mean and I don't disagree in general. But the site owner claim was a specific replacement for removing all permission claims for users which previously had the site owner permission. Having all the permissions as claims may be questionable, but now without the site owner claim we have no distinction anymore, if a user has a specific permission or not when the administrator role name is not known.

@gvkries
Copy link
Contributor Author

gvkries commented Nov 28, 2024

Anyway, I've voiced my concerns, but I'm not going to fight it until the end ;-)

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

No branches or pull requests

2 participants