Fix SiteOwner claim for OpenId

[MikeAlhayek]

Fix #17083

[MikeAlhayek]

@Piedone I tested this with your repo.

[kevinchalet]

Now... it's not clear to me why this "site owner" has a special treatment and isn't exposed as a regular role...

[MikeAlhayek]

@kevinchalet The "Site Owner" claim was introduced in version 2.1 and is automatically assigned to any user in the Administrator role. This claim grants the user access to all permissions globally, as we do not assign additional permission claims to Administrator users.

In previous versions, "Site Owner" was a permission that could be assigned to any role. However, in version 2.1, this was changed, and the "Site Owner" claim now specifically applies to the Administrator role. Each site can only have one Administrator role, which can be renamed via settings before the tenant is created.

Please let me know if you need further clarification.

[kevinchalet]

Please let me know if you need further clarification.

It's still unclear why you need an "administrator role" (which should already be materialized as a role claim in both cookies and tokens) + a new "site owner claim", since that new claim is always and only added for administrators. Why don't you just skip the permissions checks when the principal has the administrator role?

[Piedone]

Thank you for the quick fix! I also checked and works for me too. I'd wait for Kévin's feedback too, though.

[MikeAlhayek]

It's still unclear why you need an "administrator role" (which should already be materialized as a role claim in both cookies and tokens) + a new "site owner claim", since that new claim is always and only added for administrators. Why don't you just skip the permissions checks when the principal has the administrator role?

@kevinchalet
when you are an admin, we do not materialize all the permissions which is the difference. Instead, we grant you the "Site owner" claim and skip all other permissions. So these permissions will never be stores in the cookies and tokens but only "Site owner" will.

[kevinchalet]

when you are an admin, we do not materialize all the permissions which is the difference.

You don't need any permission claim in a principal to tell you you're an administrator: the role claims already contain that piece of information. An IsInRole() check - doing a lookup based on the role claims - should be enough: why do you need that site owner claim?

[MikeAlhayek]

You don't need any permission claim in a principal to tell you you're an administrator: the role claims already contain that piece of information. An IsInRole() check - doing a lookup based on the role claims - should be enough: why do you need that site owner claim?

Yes. But evaluating claims is much cheaper and faster than doing IsInRole check on every request. So when you login and IsInRole(admin), we add the site owner claim instead of assigning all the permissions that are assigned to the role. If you are IsInRole(non-admin), we take every permission the role has, create a claim out of it then store it in the principal during sign in.

[kevinchalet]

Yes. But evaluating claims is much cheaper and faster than doing IsInRole check on every request.

Urgh, that's at best... a bold claim. If you take a look at the IsInRole() implementation, you'll see it's nothing more than a HasClaim() call... something you need to do anyway with your "site owner claim" to check whether it's there or not to skip the permissions checks: https://source.dot.net/#System.Security.Claims/System/Security/Claims/ClaimsPrincipal.cs,485

IMHO, that "site owner claim" thing shouldn't exist (at all): it makes cookies and tokens bigger for no reason (because the information you need is already there) and it makes the roles logic leak in places where it shouldn't... like in the OpenID module (which caused the referenced bug, BTW).

... and it's a bit worrying it was motivated by a wrong perf' assumption/premature optimization 🤣

[MikeAlhayek]

@kevinchalet pref was not the motive for introducing "site owner" claim. The motive was the following goals AFAIR:

1. Remove "Site Owner" permission from being assigned at the role level
2. Make "Administrator" role a system role where anyone with this role have access to everything.
3. Editing Administrator role from the UI will not allow changing permissions
4. When a user with Administrator role logs in, we don't need to inflate their cookie and request size (as we did before) by storing all of the claims in the cookies. This will reduce the size of the cookie not inflate it.

I don't recall why we added the "SiteOwner" claim as it was added after the initial implementation.

From performance perspective, wouldn't it be better to do this

 if (user.HasClaim(StandardClaims.SiteOwner.Type, StandardClaims.SiteOwner.Value))

instead of

if (await _userManager.IsInRoleAsync(user, await _systemRoleNameProvider.GetAdminRoleAsync()))

on every request?

[kevinchalet]

From performance perspective, wouldn't it be better to do this

 if (user.HasClaim(StandardClaims.SiteOwner.Type, StandardClaims.SiteOwner.Value))

instead of

if (await _userManager.IsInRoleAsync(user, await _systemRoleNameProvider.GetAdminRoleAsync()))

That's absolutely not at all what I suggested tho' 😅

Using _userManager.IsInRoleAsync() basically means you're hitting the DB (which is a great way to know whether a user still has the specified role without re-authenticating, but has a cost, obviously).

What I suggested is using a principal.IsInRole(_systemRoleOptions.SystemAdminRoleName) call to determine whether a ClaimsPrincipal has the administrator (or whatever it's called if you renamed it) role attached. This way, you don't need a superfluous "site owner claim" that conveys the same exact information.

Maybe it will be more obvious with a screenshot? 🤣

I don't recall why we added the "SiteOwner" claim as it was added after the initial implementation.

That's unfortunate. If it serves no actual purpose, it should be removed ASAP before folks start depending on this thing.

[MikeAlhayek]

That's unfortunate. If it serves no actual purpose, it should be removed ASAP before folks start depending on this thing.

@kevinchalet I understand. What I think we need to do at this point is merge this PR to fix the problem on hand and release 2.1.2 today. then I can review the code again and see how can we remove the SiteOwner claim as per your suggestion.

If you see no additional issues with this PR, please approve so we can ship the patch,

[kevinchalet]

If you see no additional issues with this PR, please approve so we can ship the patch,

Merge it if you want, but I'm not convinced it's the right fix, even for a patch: the fix should be implemented in the authorization handlers (so they use the administrator role to bypass the permissions checks), not in the OpenID module.

Edit: to be more specific, this PR is problematic because it adds that "site owner" claim to the access and identity tokens, which increases chances of breaking users if we later decide to remove that claim from everywhere. It's not the direction we should be heading to.

[kevinchalet]

BTW, I'm absolutely not against removing permission claims (it's even something I suggested 6 years ago, actually: #929 (comment)). But removing them for a single user - the administrator - while keeping them for everyone else is such a weird and inconsistent move 😨

[MikeAlhayek]

@kevinchalet I actually removed the SiteOwner claim as per you suggestion. I also agree that we don't need it.

But removing them for a single user - the administrator - while keeping them for everyone else is such a weird and inconsistent move

We don't register permission claim for any Admin user not just the super-user. so if we have 5 users with the Administrator role, we don't register permission for any of them. Other non-admin users, will have explicit permission claims so we know what can they can can't they do.

[MikeAlhayek]

I created similar patch for release/2.1 to avoid a breaking change. #17089

@kevinchalet if you approve this one, I'll merge the two PR into their designated branches.

[kevinchalet]

It's a much, much, much better approach!

Thanks.

[kevinchalet]

To avoid making 2 DB calls, you can use the list of roles returned by await _userManager.GetRolesAsync(user) to determine whether await _systemRoleNameProvider.GetAdminRoleAsync() is there instead of asking the user manager/store to the check 😃