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

ClaimsPrincipal and ClaimsIdentity type proxies hide properties on derived types #91526

Closed
JamesNK opened this issue Sep 3, 2023 · 5 comments · Fixed by #91649
Closed

ClaimsPrincipal and ClaimsIdentity type proxies hide properties on derived types #91526

JamesNK opened this issue Sep 3, 2023 · 5 comments · Fixed by #91649

Comments

@JamesNK
Copy link
Member

JamesNK commented Sep 3, 2023

Description

A PR to improve debugging claims debugging added type proxies for ClaimsPrincipal and ClaimsIdentity: #86424

These types aren't sealed, and WindowsPrincipal and WindowsIdentity inherit from them and add new properties. For example, WindowsPrincipal.DeviceClaims. These properties are hidden by the type proxies.

Two possible solutions:

  1. Remove type proxies. The proxies improve the display of some IEnumerable<Claims> properties, which I believe could be replaced by placing DebuggerDisplayAttribute on those properties. I haven't seen DebuggerDisplayAttribute used on properties before, but its attribute usage allows it.
  2. Add type proxies for WindowsPrincipal and WindowsIdentity. This would improve debugging of these types, but anyone else who inherits from the base claim types and adds new properties will still have them hidden.

Reproduction Steps

Create WindowsPrinicipal and debug.

Expected behavior

WindowsPrinicipal.DeviceClaims is visible in the default debugging view.

Actual behavior

Only ClaimsPrinicipal properties are visible in the default debugging view.

Regression?

Yes. These properties were visible by default when debugging .NET 7.

Known Workarounds

No response

Configuration

No response

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Sep 3, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 3, 2023
@JamesNK
Copy link
Member Author

JamesNK commented Sep 3, 2023

Should this be fixed in .NET 8 RC2?

@vcsjones vcsjones added area-System.Security and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 4, 2023
@ghost
Copy link

ghost commented Sep 4, 2023

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

A PR to improve debugging claims debugging added type proxies for ClaimsPrincipal and ClaimsIdentity: #86424

These types aren't sealed, and WindowsPrincipal and WindowsIdentity inherit from them and add new properties. For example, WindowsPrincipal.DeviceClaims. These properties are hidden by the type proxies.

Two possible solutions:

  1. Remove type proxies. The proxies improve the display of some IEnumerable<Claims> properties, which I believe could be replaced by placing DebuggerDisplayAttribute on those properties. I haven't seen DebuggerDisplayAttribute used on properties before, but its attribute usage allows it.
  2. Add type proxies for WindowsPrincipal and WindowsIdentity. This would improve debugging of these types, but anyone else who inherits from the base claim types and adds new properties will still have them hidden.

Reproduction Steps

Create WindowsPrinicipal and debug.

Expected behavior

WindowsPrinicipal.DeviceClaims is visible in the default debugging view.

Actual behavior

Only ClaimsPrinicipal properties are visible in the default debugging view.

Regression?

Yes. These properties were visible by default when debugging .NET 7.

Known Workarounds

No response

Configuration

No response

Other information

No response

Author: JamesNK
Assignees: -
Labels:

area-System.Security, untriaged

Milestone: -

@vcsjones
Copy link
Member

vcsjones commented Sep 4, 2023

2. Add type proxies for WindowsPrincipal and WindowsIdentity. This would improve debugging of these types, but anyone else who inherits from the base claim types and adds new properties will still have them hidden.

As a point of reference, looking across GitHub, I can see a number of places where 3rd parties are deriving from this type.

My vote would be a fix that benefits all folks that derive from these types.

@JamesNK
Copy link
Member Author

JamesNK commented Sep 4, 2023

  1. Remove type proxies. The proxies improve the display of some IEnumerable<Claims> properties, which I believe could be replaced by placing DebuggerDisplayAttribute on those properties. I haven't seen DebuggerDisplayAttribute used on properties before, but its attribute usage allows it.

Unfortunately, this doesn't work. There is a bug in Roslyn: dotnet/roslyn#4134

I don't see a good way to make IEnumerable + yield play well with the debugger. A tactical solution could be to change ClaimsPricinpial.Claims to create a List<Claim> and return that. If there are multiple identities with claims then a list will allocate slightly more than yield, but one identity is the typical use case.

Longer term, a way to make IEnumerable + yield show up well with the debugger should be investigated.

@jozkee jozkee added this to the 8.0.0 milestone Sep 5, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Sep 5, 2023
@jozkee
Copy link
Member

jozkee commented Sep 5, 2023

Removing the proxies and add DebuiggerDisplay to the Claims property after dotnet/roslyn#4134 gets fixed (if ever) seems best to me.

create a List and return that

Not sure if it's worth to add allocations for debugging purposes. cc @bartonjs

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 5, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 6, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants