Skip to content
This repository has been archived by the owner on Dec 20, 2018. It is now read-only.

Consider having the UserValidator ensure that SecurityStamp != null #1016

Closed
shima20 opened this issue Nov 5, 2016 · 7 comments
Closed

Consider having the UserValidator ensure that SecurityStamp != null #1016

shima20 opened this issue Nov 5, 2016 · 7 comments

Comments

@shima20
Copy link

shima20 commented Nov 5, 2016

When I called var principal = await _signInManager.CreateUserPrincipalAsync(user); , I was receiving an ArgumentNullException and had no idea what the issue was. Kevin from Openiddict told me to register UserClaimsPrincipalFactory to step in and see what field was null. This led me to find that my SecurityStamp was null and therefore caused the exception. Is this a bug?

if (UserManager.SupportsUserSecurityStamp)
            {
                id.AddClaim(new Claim(Options.ClaimsIdentity.SecurityStampClaimType,
                    await UserManager.GetSecurityStampAsync(user)));
            }
@HaoK
Copy link
Member

HaoK commented Nov 7, 2016

SecurityStamps should never be null for a store that supports user security stamp.

@HaoK HaoK added the discussion label Nov 7, 2016
@HaoK HaoK closed this as completed Nov 7, 2016
@kevinchalet
Copy link

SecurityStamps should never be null for a store that supports user security stamp.

Except it was, in this case (using the default EF Core store).

It's also worth mentioning that there's nothing that prevents setting SecurityStamp to null: https://github.com/aspnet/Identity/blob/dev/src/Microsoft.AspNetCore.Identity.EntityFrameworkCore/UserStore.cs#L1242-L1252

@HaoK
Copy link
Member

HaoK commented Nov 7, 2016

Typically this usage is updated by the manager, I mean sure you can set it to null directly but things will blow up.

https://github.com/aspnet/Identity/blob/dev/src/Microsoft.AspNetCore.Identity/UserManager.cs#L421

We could consider adding validation in the default user validator for this since this does seem to trip more than a few people up.

@HaoK HaoK reopened this Nov 7, 2016
@HaoK HaoK changed the title When SecurityStamp is null, ArgumentNullException is thrown Consider having the UserValidator ensure that SecurityStamp != null Nov 7, 2016
@kevinchalet
Copy link

@shima20's case is obviously an atypical case, as he somehow managed to get a null security stamp.

FWIW, I don't think the principal factory should shock on null values. It should be a bit smarter and ignore null claim values.

@HaoK
Copy link
Member

HaoK commented Nov 7, 2016

Not for something like this, the security stamp is critical well for 'security' :) Its a major error if its null... similar to having no user id/name etc, its the user validators job to make sure the user is in a good state

@kevinchalet
Copy link

kevinchalet commented Nov 7, 2016

Critical seems a bit excessive (it's only used for cookies/tokens invalidation), but okay. As long as there's a check somewhere that prevents this illegal case, I'm fine 😄

@HaoK HaoK added this to the 1.2.0 milestone Nov 10, 2016
@HaoK HaoK self-assigned this Nov 10, 2016
@HaoK
Copy link
Member

HaoK commented Nov 21, 2016

4dd38e8

@HaoK HaoK closed this as completed Nov 21, 2016
@HaoK HaoK added 3 - Done and removed 2 - Working labels Nov 21, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants