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

[Bug] ClaimsPrincipalExtensions.GetNameIdentifierId uses utid instead of sub claim #171

Closed
1 of 8 tasks
felickz opened this issue May 21, 2020 · 5 comments
Closed
1 of 8 tasks
Assignees
Labels
documentation Improvements or additions to documentation fixed web app
Milestone

Comments

@felickz
Copy link

felickz commented May 21, 2020

Which Version of Microsoft Identity Web are you using ?
Note that to get help, you need to run the latest version.
Microsoft Identity Web 0.1.3-preview
https://github.com/AzureAD/microsoft-identity-web/blob/master/src/Microsoft.Identity.Web/ClaimsPrincipalExtensions.cs
Where is the issue?

  • Web App
    • Sign-in users
    • Sign-in users and call web APIs
  • Web API
    • Protected web APIs (Validating tokens)
    • Protected web APIs (Validating scopes)
    • Protected web APIs call downstream web APIs
  • Token cache serialization
    • In Memory caches
    • Session caches
    • Distributed caches

Other? - please describe;

Is this a new or existing app?

Repro

        /// <summary>
        /// Gets the NameIdentifierId associated with the <see cref="ClaimsPrincipal"/>.
        /// </summary>
        /// <param name="claimsPrincipal">the <see cref="ClaimsPrincipal"/> from which to retrieve the sub claim.</param>
        /// <returns>Name identifier ID (sub) of the identity, or <c>null</c> if it cannot be found.</returns>
        public static string GetNameIdentifierId(this ClaimsPrincipal claimsPrincipal)
        {
            return claimsPrincipal.FindFirstValue(ClaimConstants.UniqueObjectIdentifier);
        }

Expected behavior
As per documentation - this should be looking for SUB claim?

Actual behavior
Uses

        /// <summary>
        /// UniqueObjectIdentifier: "utid".
        /// </summary>
        public const string UniqueObjectIdentifier = "utid";

Possible Solution

            return claimsPrincipal.FindFirstValue(ClaimConstants.Sub);
@felickz felickz changed the title [Bug] [Bug] ClaimsPrincipalExtensions.GetNameIdentifierId uses utid instead of sub claim May 21, 2020
@jennyf19 jennyf19 added bug Something isn't working web app labels May 22, 2020
@jmprieur
Copy link
Collaborator

@felickz: which documentation?

@jmprieur
Copy link
Collaborator

jmprieur commented Jun 2, 2020

@felickz ?

@felickz
Copy link
Author

felickz commented Jun 3, 2020

@jmprieur - the XML comment which indicates it pulls the "sub claim" - which is published to docs: https://docs.microsoft.com/en-us/dotnet/api/microsoft.identity.web.claimsprincipalextensions.getnameidentifierid?view=azure-dotnet-preview#parameters

Parameters
ClaimsPrincipal
the ClaimsPrincipal from which to retrieve the sub claim.

Only stumbled on this because our b2c integration code fell into a odd flow, if this would have been looking at the sub claim as documented - my code would have worked out of the box. I cannot find any documentation on what the utid is intended to be sent by - some internal MSAL claim?

Interesting though - that code has already been refactored and the method which caused that code to fall through is no longer there

string nameIdentifierId = claimsPrincipal.GetNameIdentifierId();

448ce07#diff-dd842d9acd1b68a77df061ba4429d4d6

felickz referenced this issue Jun 3, 2020
#177)

* use clientinfo endpoint to determine home object id and home tenant id for guest scenarios

* Update src/Microsoft.Identity.Web/ClaimsPrincipalExtensions.cs

Co-authored-by: pmaytak <34331512+pmaytak@users.noreply.github.com>

* Update src/Microsoft.Identity.Web/ClaimsPrincipalExtensions.cs

Co-authored-by: pmaytak <34331512+pmaytak@users.noreply.github.com>

* Update src/Microsoft.Identity.Web/ClaimsPrincipalExtensions.cs

Co-authored-by: pmaytak <34331512+pmaytak@users.noreply.github.com>

* Update src/Microsoft.Identity.Web/ClaimsPrincipalExtensions.cs

Co-authored-by: pmaytak <34331512+pmaytak@users.noreply.github.com>

* Update src/Microsoft.Identity.Web/ClaimsPrincipalExtensions.cs

Co-authored-by: pmaytak <34331512+pmaytak@users.noreply.github.com>

Co-authored-by: pmaytak <34331512+pmaytak@users.noreply.github.com>
@jmprieur
Copy link
Collaborator

jmprieur commented Jun 4, 2020

Thanks @felickz for your explanations

@jmprieur jmprieur added documentation Improvements or additions to documentation and removed bug Something isn't working more info needed labels Jun 4, 2020
@jmprieur jmprieur self-assigned this Jun 4, 2020
@jmprieur jmprieur added this to the 0.1.5-preview milestone Jun 4, 2020
jmprieur added a commit that referenced this issue Jun 10, 2020
…fierId

Fixes the XML documentation for ClaimsPrincipalExtensions.GetNameIdentifierId
therefore addressing #171
jmprieur added a commit that referenced this issue Jun 10, 2020
…fierId (#192)

Fixes the XML documentation for ClaimsPrincipalExtensions.GetNameIdentifierId
therefore addressing #171

Co-authored-by: Marsh Macy <mmacy@users.noreply.github.com>
@pmaytak
Copy link
Contributor

pmaytak commented Jun 16, 2020

Fixed in Microsoft Identity Web 0.1.5-preview release.

cc: @felickz

@pmaytak pmaytak closed this as completed Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation fixed web app
Projects
None yet
Development

No branches or pull requests

4 participants