Unhandled exception when receiving old EntityFramework cookies with guids

[jli416]

When converting a site from EntityFramework Identity, clients submitting old cookies will crash aspnet.identity.mongo. I can't see a way to filter this at a higher level. Can we add a more graceful way of handling this so asp.net will just fail to authenticate and redirect to login?

See error output below.

Description: An unhandled exception occurred during the execution of the current web request. Please review the stack trace for more information about the error and where it originated in the code.

Exception Details: System.FormatException: '5ab35807-cbe1-408e-b214-a11088b08163' is not a valid 24 digit hex string.

Source Error:

An unhandled exception was generated during the execution of the current web request. Information regarding the origin and location of the exception can be identified using the exception stack trace below.

Stack Trace:

[FormatException: '5ab35807-cbe1-408e-b214-a11088b08163' is not a valid 24 digit hex string.] MongoDB.Bson.ObjectId.Parse(String s) +216 MongoDB.Bson.Serialization.Serializers.StringSerializer.SerializeValue(BsonSerializationContext context, BsonSerializationArgs args, String value) +106 MongoDB.Bson.Serialization.Serializers.SealedClassSerializerBase1.Serialize(BsonSerializationContext context, BsonSerializationArgs args, TValue value) +45 MongoDB.Bson.Serialization.Serializers.SerializerBase1.MongoDB.Bson.Serialization.IBsonSerializer.Serialize(BsonSerializationContext context, BsonSerializationArgs args, Object value) +81 MongoDB.Bson.Serialization.IBsonSerializerExtensions.Serialize(IBsonSerializer serializer, BsonSerializationContext context, Object value) +116 MongoDB.Driver.Linq.Expressions.ISerializationExpressionExtensions.SerializeValue(ISerializationExpression field, Object value) +335 MongoDB.Driver.Linq.Translators.PredicateTranslator.TranslateComparison(Expression variableExpression, ExpressionType operatorType, ConstantExpression constantExpression) +409 MongoDB.Driver.Linq.Translators.PredicateTranslator.Translate(Expression node) +461 MongoDB.Driver.Linq.Translators.PredicateTranslator.Translate(Expression node, IBsonSerializerRegistry serializerRegistry) +38 MongoDB.Driver.MongoCollectionImpl1.FindAsync(FilterDefinition1 filter, FindOptions2 options, CancellationToken cancellationToken) +416 MongoDB.Driver.FindFluent2.ToCursorAsync(CancellationToken cancellationToken) +125 MongoDB.Driver.IFindFluentExtensions.FirstOrDefaultAsync(IFindFluent2 find, CancellationToken cancellationToken) +198 AspNet.Identity.MongoDB.UserStore1.FindByIdAsync(String userId) +540 Microsoft.AspNet.Identity.Owin.<b1>d4.MoveNext() +592 System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +13847892 System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +61 Microsoft.Owin.Security.Cookies.d2.MoveNext() +2531 System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +13847892 System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +61 Microsoft.Owin.Security.Infrastructure.d0.MoveNext() +822 System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +13847892 System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +61 Microsoft.Owin.Security.Infrastructure.d0.MoveNext() +333 System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +13847892 System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +61 Microsoft.AspNet.Identity.Owin.d0.MoveNext() +450 System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +13847892 System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +61 Microsoft.AspNet.Identity.Owin.d0.MoveNext() +450 System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +13847892 System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +61 Microsoft.AspNet.Identity.Owin.d0.MoveNext() +450 System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +13847892 System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +61 Microsoft.Owin.Host.SystemWeb.IntegratedPipeline.d5.MoveNext() +202 System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) +13847892 System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) +61 Microsoft.Owin.Host.SystemWeb.IntegratedPipeline.d2.MoveNext() +193 Microsoft.Owin.Host.SystemWeb.IntegratedPipeline.StageAsyncResult.End(IAsyncResult ar) +96 System.Web.AsyncEventExecutionStep.System.Web.HttpApplication.IExecutionStep.Execute() +363 System.Web.HttpApplication.ExecuteStep(IExecutionStep step, Boolean& completedSynchronously) +137

Version Information: Microsoft .NET Framework Version:4.0.30319; ASP.NET Version:4.6.1055.0

[jli416]

FYI, I'm using this change in UserStore.cs as a hack right now

public virtual Task<TUser> FindByIdAsync(string userId)
{
    ObjectId oid;
    if (!ObjectId.TryParse(userId, out oid)) return Task.FromResult<TUser>(null);

    return _Users.Find(u => u.Id == userId).FirstOrDefaultAsync();
}

[g0t4]

Are you using guids as document ids? If so, just stick with guids if you aren't going to convert them and rewrite the FindById permanently. I wouldn't go half in half out on id type

On Wednesday, February 17, 2016, jli416 notifications@github.com wrote:

FYI, I'm using this change in UserStore.cs as a hack right now

public virtual Task FindByIdAsync(string userId) { ObjectId oid; if (!ObjectId.TryParse(userId, out oid)) return Task.FromResult(null);

return _Users.Find(u => u.Id == userId).FirstOrDefaultAsync();

}

— Reply to this email directly or view it on GitHub https://github.com/g0t4/aspnet-identity-mongo/issues/14#issuecomment-185163103 .

[jli416]

Thanks, I agree on the datatype. Maybe you can add a note in the docs on this as people moving production systems from entity identity to this library will encounter the same problem.

I'd like to suggest that since the userId is passed around as a string, maybe the library should handle when the string doesn't parse (to whatever the underlying datatype) more gracefully. Right now the exception raised within the mongo driver bubbles all the way up aspnet.identity unhandled rather than just returning that the user doesn't exist so aspnet.identity can redirect to login.

[g0t4]

Thanks for pointing this out, would you be willing to take notes of what you did in your conversion process and send those to me, or send a PR with updates to the docs? If so I'd be happy to share that with others.

The bubbling up is inescapable as this plugs in to the identity framework, perhaps it could say invalid id in the exception that is raised but I think it should continue to throw an exception instead of simply taking users to a login page. Consumers of this library could then handle that exception however they see fit, I can see someone confused by the redirect to the login page.

On Thu, Feb 18, 2016 at 4:50 AM, jli416 notifications@github.com wrote:

Thanks, I agree on the datatype. Maybe you can add a note in the docs on this as people moving production systems from entity identity to this library will encounter the same problem.

I'd like to suggest that since the userId is passed around as a string, maybe the library should handle when the string doesn't parse (to whatever the underlying datatype) more gracefully. Right now the exception raised within the mongo driver bubbles all the way up aspnet.identity unhandled rather than just returning that the user doesn't exist so aspnet.identity can redirect to login.

— Reply to this email directly or view it on GitHub https://github.com/g0t4/aspnet-identity-mongo/issues/14#issuecomment-185632215 .

[kdubau]

Is there any known way to overcome this issue without having to fork this library?

[g0t4]

@kdubau this isn't a problem with this library, it's a problem with the underlying data. First step here, are you sure you're having the same problem?

[jli416]

If bad data (guid vs objectid) is passed into the library, while its probably correct to just throw the exception up, the identity framework doesn't provide an obvious place to intercept that exception and deal with it at a higher level. This means users are going to get an IIS exception which isn't desirable.

On the other hand, since the FindByIdAsync() signature only specifies a string argument, it would also be reasonable for developers to assume that FindByIdAsync() would return the user or null as long as a non-empty userId string is passed in. There isn't anything in the function signature that would imply that the string has to be a valid objectId. If that was the intended behaviour, the function signature should be changed to accept an objectId type, but then other dependencies would break.

Can really be argued both ways, and while this isn't a problem with the library per se, the latter approach happens to save some pain for those in production deployments moving from guids to objectids, which would make the library more robust for at least one real-world edge case. We had to fork because there wasn't a good alternative solution.

[jli416]

Just wanted to add that FindByIdAsync() doesn't need to redirect. It just needs to return null when it can't find the specified userId rather than throw an exception (even in the case where the userId string isn't what you expected). Identity framework's default behaviour is to redirect if FindByIdAsync() returns null.

[kdubau]

@g0t4 , yep same issue. As @jli416, I understand its not an issue with the library per se, but it is a poor experience for users when we migrate since there is no good way to intercept the exception. +1 the library should return null in this case opposed to bubbling the exception.

[g0t4]

Guys I need some time to get back to this, I'm a bit crammed right now, if you have a code proposal, please send it my way for me to consider. I'm happy to make the library more flexible.

On Sun, Aug 7, 2016 at 9:05 AM, Kyle White notifications@github.com wrote:

@g0t4 https://github.com/g0t4 , yep same issue. As @jli416 https://github.com/jli416, I understand its not an issue with the library per se, but it is a poor experience for users when we migrate since there is no good way to intercept the exception. +1 the library should return null in this case opposed to bubbling the exception.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/g0t4/aspnet-identity-mongo/issues/14#issuecomment-238081379, or mute the thread https://github.com/notifications/unsubscribe-auth/AAK_3e8VHx5lCE9bxAdUP4zV8lpMdpNbks5qddgPgaJpZM4Hb_BQ .

[jli416]

I'm currently using the code I mentioned earlier in the thread:

public virtual Task<TUser> FindByIdAsync(string userId)
{
    ObjectId oid;
    if (!ObjectId.TryParse(userId, out oid)) return Task.FromResult<TUser>(null);

    return _Users.Find(u => u.Id == userId).FirstOrDefaultAsync();
}

[kdubau]

@g0t4 I'm happy to open a PR with @jli416's above code if you are willing to accept this change.

[g0t4]

This is fixed in v3, will look into v2 support when I get a free moment