Simplify how to check if the user is an admin #16866
Conversation
|
This pull request has merge conflicts. Please resolve those before requesting a review. |
| @@ -31,21 +29,10 @@ public static async ValueTask<FluidValue> HasClaim(FluidValue input, FilterArgum | |||
| return BooleanValue.True; | |||
| } | |||
|
|
|||
| if (string.Equals(claimType, Permission.ClaimType, StringComparison.OrdinalIgnoreCase)) | |||
| if (string.Equals(claimType, Permission.ClaimType, StringComparison.OrdinalIgnoreCase) && | |||
There was a problem hiding this comment.
But this would be removed if we have a custom filter for authorization, right?
There was a problem hiding this comment.
I don't think we would. Because we should still return true if the user is an admin. has_authorization would use the IAutheorizationService to do similar check without evaluating the claims explicitly.
If we need to remove it, we'll have to document it as a breaking change for 3.0 and offer has_authorization as a better way to authorize user
There was a problem hiding this comment.
Because we should still return true if the user is an admin
In 2.x because it would be a breaking change otherwise. So we can remove it in 3.0.
There was a problem hiding this comment.
So are you saying to keep this and at somepoint add has_authorization. then in 3.0, we can remove this check from has_claim, correct?
There was a problem hiding this comment.
@sebastienros actually we already have has_permission filter that one can already use. So I don't think there is a need to add has_authorization
There was a problem hiding this comment.
I also think this should be removed e.g. in 3.0, but lets keep it for backward compatibility until then.
There was a problem hiding this comment.
I added a note so we can remove it in 3.0
| /// <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 SiteOwnerClaim = new("OrchardCorePermissions", "all-permissions"); |
There was a problem hiding this comment.
I think the type name should be more clear and maybe not related to our permissions only. In the future, we may want to use this claim for other checks, beside checking for the existing permissions.
Something like IsSuperUser = true or similar.
There was a problem hiding this comment.
I changed it to Claim SiteOwner = new("SiteOwner", "true"); and moved it to a new StandardClaims class instead
|
@MikeAlhayek This is awesome 👍 |
No description provided.