Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings
This repository was archived by the owner on May 17, 2024. It is now read-only.

Update approach used to obtain principal ID in the API project#254

Merged
derisen merged 2 commits intoAzure-Samples:mainfromJohn-Garland:patch-1
Dec 9, 2022
Merged

Update approach used to obtain principal ID in the API project#254

derisen merged 2 commits intoAzure-Samples:mainfromJohn-Garland:patch-1
Dec 9, 2022

Conversation

@John-Garland
Copy link
Contributor

@John-GarlandJohn-Garland commentedDec 7, 2022
edited
Loading

Purpose

In the current incarnations of ASP.NET Core, B2C, etc, the user ID is being returned by default by the SUSI policy as the sub claim. ASP.NET Core is mapping this to the Name Identifier ID claim on the Claims Principal. As a result, the Home Object ID claim that the original code was looking for ("oid") is not present. The application seems to work, in that items are added/removed to/from the list, but in fact it is treating content with a "null" _currentPrincipalId. This can be seen by logging in as 1 user, creating a TODO item, then logging out and logging back in as another user - you will see the TODO item created by the first user in the second user's list.

Does this introduce a breaking change?

[ ] Yes[X] No (it is intended to fix broken functionality.)

Pull Request Type

What kind of change does this Pull Request introduce?

[X] Bugfix[ ] Feature[ ] Code style update (formatting, local variables)[ ] Refactoring (no functional changes, no api changes)[ ] Documentation content changes[ ] Other... Please describe:

How to Test

  • Get the code
git clone [repo-address]cd [repo-name]git checkout [branch-name]npm install
  • Test the code

See description, above for scenario and how to check for fix.

What to Check

Verify that the following are valid

  • User ID is obtained from the incoming B2C user's claims
  • List items are indexed by User ID and kept separate between users.

Other Information

Alternatively, can consider the following code, in case this behavior has diverged in B2C and both principal ID claim types are likely to occur, depending on when the tenant was created/configured:
_currentPrincipalId = _currentPrincipal.GetHomeObjectId() ?? _currentPrincipal.GetNameIdentifierId(); // use "sub" claim as a unique identifier in B2C

In the current incarnations of ASP.NET Core, B2C, etc, the user ID is being returned by default by the SUSI policy as the sub claim. ASP.NET Core is mapping this to the Name Identifier ID claim on the Claims Principal. As a result, the Home Object ID claim that the original code was looking for ("oid") is not present. The application seems to work, in that items are added/removed to/from the list, but in fact it is treating content with a "null" _currentPrincipalId. This can be seen by logging in as 1 user, creating a TODO item, then logging out and logging back in as another user - you will see the TODO item created by the first user in the second user's list.
@derisenderisen self-requested a reviewDecember 7, 2022 23:09
if(!IsAppOnlyToken()&&_currentPrincipal!=null)
{
_currentPrincipalId=_currentPrincipal.GetHomeObjectId();// use "sub" claim as a unique identifier in B2C
_currentPrincipalId=_currentPrincipal.GetNameIdentifierId();// use "sub" claim as a unique identifier in B2C
Copy link
Contributor

@derisenderisenDec 7, 2022
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@John-Garland thank you so much for catching this -I can reproduce it. I feel like instead of using.GetNameIdentifierId() for retrievingsub, we can be more explicit with something like.FindFirstValue(ClaimConstants.Sub);. In that case we would also need to set theJwtSecurityTokenHandler.DefaultMapInboundClaims tofalse inStartup.cs though -happy to discuss!

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Good question. My inclination would be to keep the sample as small/self-contained as possible. I worry that turning off claims mapping would imply it as a necessary step for using MSAL/MSIW with B2C. Perhaps, however we can achieve the goal of being explicit about the origin of the claim (I agree that sub-->nameidentifier is hardly obvious) with some strategic comments calling out the expected conversion and also suggesting the approach above as an alternative? (If that idea resonates, I can update the PR accordingly.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Comments sound good to me! As long as we explain what we're after and what alternatives out there we're all good (I just came across a good discussion on this issuehere)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Good thread. I guess this takes me back to my original question - should these samples advocate for a "cleaner" approach to handling JWT token claims and recommend that claims mapping be disabled across the board? I could see using this as an opportunity to retrofit the other exercises in this sequence as well (though done as a separate PR...)

Thinking about it, I'm going to use the comment approach I mentioned, but I would love to have a broader conversation about disabling the mapping and if it should eb something we advocate across the samples (and elsewhere!)

Copy link
Contributor

@derisenderisenDec 9, 2022
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Claims mapping usually functions well in AAD use cases, at least I don't remember an instance where it led us astray. But for B2C use cases it might be a good idea, since it has various idiosyncrasies like this. Let me bring this up with my team, at least for B2C samples cc@kalyankrishna1

Add a comment indicating why/how the "sub" claim is mapped to a different name in the ClaimsPrincipal object. Also describe an alternative approach whereby the mapping behavior is disabled and the Sub claim is found by name.
Copy link
Contributor

@derisenderisen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM, thanks@John-Garland !

@John-Garland
Copy link
ContributorAuthor

Thank you@derisen - do you have Merge permission on this repository / can you complete the PR (I do not have permission to do so.)

@derisen
Copy link
Contributor

@John-Garland oh sorry, let me merge it

@derisenderisen merged commit2fd9f26 intoAzure-Samples:mainDec 9, 2022
@John-GarlandJohn-Garland deleted the patch-1 branchDecember 9, 2022 22:29
@sa-android-adminsa-android-admin mentioned this pull requestFeb 3, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

1 more reviewer

@derisenderisenderisen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@John-Garland@derisen

[8]ページ先頭

©2009-2025 Movatter.jp