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

Quickstart talks about `sub` claim, which doesn't exist #2968

Open
MikeBeaton opened this issue Jan 21, 2019 · 14 comments
Assignees
Labels

Comments

@MikeBeaton
Copy link
Contributor

@MikeBeaton MikeBeaton commented Jan 21, 2019

The second online quickstart Protecting an API using Passwords (which is generated from this file in the repository) contains the following text at the end of the page:

When you send the token to the identity API endpoint, you will notice one small but important difference compared to the client credentials grant. The access token will now contain a sub claim which uniquely identifies the user. This “sub” claim can be seen by examining the content variable after the call to the API and also will be displayed on the screen by the console application.

The presence (or absence) of the sub claim lets the API distinguish between calls on behalf of clients and calls on behalf of users.

This is not (or no longer) true. There is no sub claim showing the name (nor id) of the logged in user. There is a http://schemas.xmlsoap.org/ws/2005/05/identity/claims/nameidentifier claim which contains the id of the logged in user, and which changes correctly as one changes the username and password.

I've checked and the issue applies in the fully worked IdentityServer4.Samples solution (not just to my own attempt to implement things according to the quickstart guides).

Is there a way to get a sub claim to appear? Or has the name for this claim been renamed to the longer, schema-based name above? (In which case the docs need updating. I'd be happy to do a PR, but I'm not sure about some relevant issues. Are there other possible schemas? How come some claims are don't have schema-based names (e.g. aud, client_id) but some do?)

@MikeBeaton

This comment has been minimized.

Copy link
Contributor Author

@MikeBeaton MikeBeaton commented Jan 21, 2019

I believe this is almost certainly related to the issue discussed here IdentityServer/IdentityServer3.Samples#173 which suggests that some part of the Microsoft stack (which can be disabled?) is converting the some claim names to longer .NET claim names before the user code ever sees them.

@ri-ch

This comment has been minimized.

Copy link

@ri-ch ri-ch commented Jan 21, 2019

As @MikeBeaton mentioned, this is pure microsoft weirdness. On the JwtSecurityTokenHandler class, there are two static properties, DefaultInboundClaimTypeMap and DefaultOutboundClaimTypeMap. These map the internal microsoft claim types onto the claims that end up in the token (and vice versa). You can override them (we do it in a static constructor), to return the expected claims.

@brockallen brockallen added the question label Jan 21, 2019
@brockallen

This comment has been minimized.

Copy link
Member

@brockallen brockallen commented Jan 21, 2019

I'm confused. What's the issue or problem?

@MikeBeaton

This comment has been minimized.

Copy link
Contributor Author

@MikeBeaton MikeBeaton commented Jan 21, 2019

I've tried to be completely clear in my issue?

The current quickstart guide says that there will be a claim named sub when you do a username/password authentication (and that you can/should use the existence of this claim "to distinguish between calls on behalf of clients and calls on behalf of users").

But - after implementing the quickstart code (and on double-checking with the reference implementation of the quickstart) - there is no sub claim present.

(It turns out that (presumably since the quickstart guide was written?) things have been changed so that another part of the Microsoft stack renames the sub claim name to http://schemas.xmlsoap.org/ws/2005/05/identity/claims/nameidentifier. Thus, behind the scenes, a claim named sub is certainly still being passed around. But by the time the claim gets to the 'user' code in the quickstart, it has been renamed.)

How is that not a (minor) issue? The quickstart code does not do what the quickstart guide says it will do.

I've put the issue here since the quickstart guide is part of this repositiory.

Let me know what else to do.

@MikeBeaton

This comment has been minimized.

Copy link
Contributor Author

@MikeBeaton MikeBeaton commented Jan 21, 2019

@rich-zilla Even your solution is no longer correct, the field has since been renamed to DefaultInboundClaimTypeMap!

@brockallen

This comment has been minimized.

Copy link
Member

@brockallen brockallen commented Jan 21, 2019

The nameidentifier claim type is Microsoft's mapped version of the sub claim and it's wrong of them to have done what they do. Thankfully they have the DefaultInboundClaimTypeMap which we can use to clear their incorrect behavior. So are you saying that's not done in the QS code?

@MikeBeaton

This comment has been minimized.

Copy link
Contributor Author

@MikeBeaton MikeBeaton commented Jan 21, 2019

I am indeed saying that that is not done in the QS code. I didn't know what your preferred solution would be - maybe it would have been to go with the Microsoft naming, for all I knew! But no, it is not overridden in the sample code!

One problem with changing the mapping, though, is that - at least at first sight - it seems that doing that could potentially break other things if one was using other parts of the MS stack. E.g. the various older examples I've found which suggest overriding the inbound mapping all suggest doing AntiForgeryConfig.UniqueClaimTypeIdentifier = ClaimTypes.Subject at the same time.

@brockallen

This comment has been minimized.

Copy link
Member

@brockallen brockallen commented Jan 21, 2019

One problem with changing the mapping, though, is that - at least at first sight - it seems that doing that could potentially break other things if one was using other parts of the MS stack.

Yes, agreed, but they only designed it as a static. They're not quite on the ball with respect to a modern design there.

@MikeBeaton

This comment has been minimized.

Copy link
Contributor Author

@MikeBeaton MikeBeaton commented Jan 21, 2019

The easiest fix seems to be to add JwtSecurityTokenHandler.DefaultInboundClaimTypeMap = new Dictionary<string, string>(); in 2_ResourceOwnerPasswords/src/Api/Startup.cs. Maybe at the top of ConfigureServices(...)?

I've no idea whether that is safe or future proof! Or what else, exactly, needs doing as well, to make it more safe/future proof... e.g. does the outbound mapping need changing in Quickstarts/2_ResourceOwnerPasswords/src/IdentityServer/Startup.cs? (I've just changed the inbound mapping only and everything works with the simple names now being visible, so at least - I think?! - the sub claim isn't being renamed outbound, as at least one reference I've just seen suggested.)

The most 'future-proof' fix (of sorts) would presumably be to look for a claim with claim.Type == ClaimTypes.NameIdentifier, and give up on relying on it being called sub.

I do totally agree that Microsoft trying to force this non-standard naming on users is horrible.

@brockallen

This comment has been minimized.

Copy link
Member

@brockallen brockallen commented Jan 21, 2019

I'll label this as a docs fix and we'll put a solution in place. Thanks for letting us know.

@MikeBeaton

This comment has been minimized.

Copy link
Contributor Author

@MikeBeaton MikeBeaton commented Jan 21, 2019

Dear @brockallen - Having just gone on to the next section, it turns out that you do indeed include the relevant line JwtSecurityTokenHandler.DefaultInboundClaimTypeMap.Clear(); in quickstarts/3_interactive_login.html - which may explain your sense that you'd already done this! (If I correctly picked up that you had that sense!) But it's not in either of the first two sections which, of course, the assiduous newbie does first. Thanks!

@jimmi79

This comment has been minimized.

Copy link

@jimmi79 jimmi79 commented Mar 27, 2019

Thank you @MikeBeaton for raising this issue. I spent hours on this after seeing that the next QS showed a sub claim via the MVC client, but not through the API.

@brockallen all things considered, you've put together some great walk-throughs for a truly amazing service. I've gained a TON of knowledge from your documentation and talks on YouTube. A sincere kudos for your efforts!

@mattjcowan

This comment has been minimized.

Copy link
Contributor

@mattjcowan mattjcowan commented Jul 12, 2019

Thank you for the QS, it's a great write-up. I also ran into this issue and took me a few to figure out why the sub claim type didn't exist (instead of the 'http://schemas.xmlsoap.org/ws/2005/05/identity/claims/nameidentifier' type).

I think if you add JwtSecurityTokenHandler.DefaultInboundClaimTypeMap.Clear(); right before the app.UseAuthentication() in the api startup code on page 1 of the QS 1_client_credentials.rst, that would mitigate the issue for those following the QS. Otherwise, you'd have to explain that line on the bottom of page 2 of the QS.

@leastprivilege

This comment has been minimized.

Copy link
Member

@leastprivilege leastprivilege commented Aug 5, 2019

@LindaLawton could you have a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.