Skip to content

Remove MapIdentityApi's InfoResponse.Claims property #51177

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

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

halter73
Copy link
Member

@halter73 halter73 commented Oct 6, 2023

Description

This is a public API change to remove the InfoResponse.Claims property which was first made public in RC2. The API was approved last week during API review (Notes).

The API was already broken in common scenarios where a user has multiple roles causing multiple claims with the same type. This cause an ArgumentException in the /manage/info endpoint when it attempted to add duplicate Key in the now-deleted public IDictionary<string, string> Claims property.

We had considered trying to fix the Claims property by making it an array or a dictionary of arrays, but @mconnew brought up the possibility that developers would not always expect clients to read their own claims. With cookies and JWE tokens claims are encrypted and potentially secret even to the authenticated user. When we consulted @blowdart, he agreed that we should avoid exposing claims from MapIdentityApi<TUser>() to be safe even though he hadn't personally seen an example of a claim that needed to be kept secret from an authenticated client.
 
Fixes #50037

Customer Impact

Prior to this change, the /manage/info endpoint added by the new MapIdentityApi<TUser>() method introduced in .NET 8, would return an empty 500 response when the user making the request had a ClaimsPrincipal with multiple claims of the same type. This is very common when a user is in multiple roles and was reported by a customer shortly after the /manage/info endpoint was added in preview 7. This made the entire endpoint unusable for these users.

Regression?

  • Yes
  • No

[If yes, specify the version the behavior has regressed from]

Risk

  • High
  • Medium
  • Low

This is simply removing new API that was first added in .NET 8 preview 7 and made public in RC 2.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@ghost ghost added the area-identity Includes: Identity and providers label Oct 6, 2023
@halter73 halter73 added the Servicing-consider Shiproom approval is required for the issue label Oct 6, 2023
@ghost
Copy link

ghost commented Oct 6, 2023

Hi @halter73. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@mkArtakMSFT mkArtakMSFT added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Oct 10, 2023
@ghost
Copy link

ghost commented Oct 10, 2023

Hi @halter73. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed.

@mkArtakMSFT mkArtakMSFT merged commit 0d0aa33 into release/8.0 Oct 11, 2023
@mkArtakMSFT mkArtakMSFT deleted the halter73/50037 branch October 11, 2023 16:14
@ghost ghost added this to the 8.0.0 milestone Oct 11, 2023
@augustevn
Copy link

augustevn commented Nov 17, 2023

I think you broke the Blazor WASM way of informing our AuthenticationStateProvider of our claims. At least you broke the seamless integration of that with the Identity Endpoints. So now we must expose the claims ourselves using a custom endpoint... What did you/we win?

More of my thoughts on this: #52142

I'm having doubts on how you test and validate these authentication flows from front to back.
I think it's time to pull your heads out of your back-ends.

@ghost
Copy link

ghost commented Nov 17, 2023

Hi @augustevn. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@martincostello
Copy link
Member

@augustevn Please follow the code of conduct and speak respectfully to the team.

@ghost
Copy link

ghost commented Nov 17, 2023

Hi @martincostello. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@augustevn
Copy link

It's just a pun, I understand you may have a lot on your plates and have to support more than just the bearer token scenario.

@ghost
Copy link

ghost commented Nov 17, 2023

Hi @augustevn. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-identity Includes: Identity and providers Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants