Updates to display, editor and authz tag helpers #67
Conversation
MSingh-13
force-pushed
the
main
branch
2 times, most recently
from
63986a0 to
3e226be
Compare
|
Hi @DamianEdwards. I have incorporated your feedback and pushed a new commit. |
…butes. Updated global.json "rollForward": "latestMajor" because did not have sdk verison 7 installed.
…(using string resource with the specified policy) and roles (Principal.IsInRole()).
1. Using var to declare permissions list in Startup.cs 2. Updated the documentation text. 3. Sorted the using statements in HtmlHelperExtensions. Co-authored-by: Damian Edwards <[email protected]>
|
I just realised that my commits contained my work email. Since the changes had not been merged yet I decided to change the email by doing a rebase and amend on all my previous commits. I then force pushed them. I did not make any changes to the source code of the first 2 commits that you had reviewed a couple of days ago. The last commit incorporates the feedback from your review. I apologise for any inconvenience that this may have caused. |
|
Just checking in to see if you got a chance to review the updates. I also updated the samples to display source for asp-authz-permission and asp-authz-role examples. |
src/TagHelperPack/AuthzTagHelper.cs
Outdated
| private const string AspAuthzAttributeName = "asp-authz"; | ||
| private const string AspAuthzPolicyAttributeName = "asp-authz-policy"; | ||
| private const string AspAuthzRoleAttributeName = "asp-authz-role"; | ||
| private const string AspAuthzPolicyPermissionAttributeName = "asp-authz-permission"; |
There was a problem hiding this comment.
I'm not super comfortable with this name as it's too specific to the authorization handler implementation in the sample. The "permission" isn't a strongly-typed primitive/concept of the authorization system but instead is passed through the object resource parameter which is intended as an untyped escape-hatch to pass arbitrary information from the authorization site to the authorization handler. Perhaps your change could be updated to reflect that, e.g. change the name to asp-authz-resource and type it as object.
There was a problem hiding this comment.
I have renamed permission to resource.
See commit 1ae3e04
|
|
||
| showOutput = authorized; | ||
| } | ||
| else if (!string.IsNullOrEmpty(RequiredRole)) |
There was a problem hiding this comment.
Add a guard to ensure invalid combinations of attributes aren't being set and throw an InvalidOperationException in that case, e.g. if both a required role and policy are set.
There was a problem hiding this comment.
As suggested, I have added a condition to guard against RequiredRoles and RequiredPolicy being set at the same time.
See commit fbdab70
| /// <param name="newHtmlAttributesObject">new values</param> | ||
| /// <param name="existingHtmlAttributesObject">existing values</param> | ||
| /// <returns></returns> | ||
| internal static IDictionary<string, object> MergeHtmlAttributes(this IHtmlHelper helper, object newHtmlAttributesObject, object existingHtmlAttributesObject) |
There was a problem hiding this comment.
I haven't reviewed this fully but at first glance it seems quite a bit larger than a similar method in the ASP.NET Core itself that does something very similar: https://source.dot.net/#Microsoft.AspNetCore.Mvc.ViewFeatures/DefaultEditorTemplates.cs,192
Could it be simplified?
There was a problem hiding this comment.
I simplified HtmlHelperExtensions.MergeHtmlAttributes().
See commit 60a8260
…eing set at the same time in AuthzTagHelper.
…rce in AuthzTagHelper. Changed the type of RequiredPolicyResource property from string to object. Added a condition to ensure RequiredPolicy is set when RequiredPolicyResource is set.
Further simplified the method. Correctly overwriting the values of attributes other than class and style. Renamed the method to MergeHtmlAttributesObjects Moved it to a new class PublicHtmlHelperExtensions so that it could be made public and used in view templates.
Switched from an anonymous object to dictionary for htmlAttributes, so that only properties that have values can be added to the dictionary. Added Id property to set the id on the html element. Updated samples to demonstrate the merging of class, style and id supplied with editor tag. Also demonstrates the use for view-data-htmAttributes.
|
Added a new commit to further simplify the MergeHtmlAttributes method. I moved it to a new class in order to make it publicly accessible so that it can be used in templates. I updated the DisplayTagHelper and EditorTagHelper to include an Id property. |
2 participants
Hi @DamianEdwards
I would appreciate it if you would review and consider merging these updates into the next version of the library.
Updated editor and display tag helpers to accept class and style attributes.
This allows adding classes and styles to the tag helpers as normal html attributes (with IntelliSense support). The values are passed to the editor/display template as an anonymous object on the htmlAttributes property of the ViewData which is how they were passed when using editor templates in MVC5. This allowed me to reuse my existing editor/display templates from MVC5 with minimal changes. I used a modified version of the MergeHtmlAttributes extension created by Chris Pratt.
https://web.archive.org/web/20211205165309/https://cpratt.co/html-editorfor-and-htmlattributes/ (the original site is currently not accessible)
Updated AuthzTagHelper to handle authorizations based on User roles and permissions.
I have updated the TagHelperPack.Sample project to demo these use cases.
a) User role is checked using the Principal.IsInRole() method provided by the framework.
b) Permission is checked in an authorization policy based on the permissions associated with the current user role.