FindAsync assumes incorrect document structure

[MortenChristiansen]

It seems that the method RavenUserStore.FindAsync assumes an incorrect document structure, where RavenUserLogins are stored seperately from RavenUsers when in fact they are just an array property in the RavenUser document. The method does not find the user when the correct login provider and provider key are present. While it might not be the most effective approach, I got the method working by changing it to the following:

    public async Task<TUser> FindAsync(UserLoginInfo login)
    {
        if (login == null) throw new ArgumentNullException("login");

        string keyToLookFor = RavenUserLogin.GenerateKey(login.LoginProvider, login.ProviderKey);
        return await _documentSession.Query<TUser>().Where(u => u.Logins.Any(l => l.Id == keyToLookFor)).SingleOrDefaultAsync();
    }

This is what the user looks like in the database, after creating it with the standard code in the VS2013 web template:

{
    "UserName": "MortenChristiansen",
    "Email": null,
    "PhoneNumber": null,
    "PasswordHash": null,
    "SecurityStamp": "...",
    "IsLockoutEnabled": false,
    "IsTwoFactorEnabled": false,
    "AccessFailedCount": 0,
    "LockoutEndDate": null,
    "Claims": [],
    "Logins": [
      {
          "Id": "RavenUserLogins/Google/https://www.google.com/accounts/o8/id?id=...",
          "UserId": "Morten Christiansen",
          "LoginProvider": "Google",
          "ProviderKey": "https://www.google.com/accounts/o8/id?id=..."
      }
    ]
}

[tugberkugurlu]

@clone1985 thanks for pointing out this issue. I pushed a new version of the library: https://www.nuget.org/packages/AspNet.Identity.RavenDB/2.0.0-pre-06 can you try with the new version and see if the issue continues. It should be solved.

The reason why I don't prefer to implement FindAsync as you did is to avoid unnecessary index creation. In my implementation, I'm doing two queries but all are performed on the document Id.

[MortenChristiansen]

Thanks, that did the trick.

[tugberkugurlu]

Nice! Thanks for confirming!

[MortenChristiansen]

It appears I was too quick to close the issue. It suddenly stopped working again without me having done anything to provoke it, so I think I was still running with my modified version cached for a while.

[tugberkugurlu]

Can you provide the error message that you get or repro steps so that I can reproduce the exact same problem?

[MortenChristiansen]

The RavenDB log says:

Document with key 'RavenUserLogins/Google/https://www.google.com/accounts/o8/id?id=MYKEY' was not found

This makes sense to me (and my limited understanding of RavenDB) since there is no document with an id like that (see the original bug description). Note that the id it searches for matches the one in the Logins collection ('Id') perfectly, so that is not the problem. So my original conclusion still stands that the way you look for the login does not work as intended.

I created a repro project that is the default VS 2013 SPA template where I just replaced the persistence to use the newest AspNet.Identity.RavenDB package. You just need to enter a valid connection string in the web.config and try to register a Google account. After registering it you should see the same log in the RavenDB Studio that the document could not be found. https://onedrive.live.com/redir?resid=48F0E076F4DD5267!19633&authkey=!ALSCar9Ktmtu1NI&ithint=file%2c.zip

[tugberkugurlu]

@clone1985 thanks, looking into it now.

[tugberkugurlu]

@clone1985 I figured out the problem.

// POST api/Account/AddExternalLogin
[Route("AddExternalLogin")]
public async Task<IHttpActionResult> AddExternalLogin(AddExternalLoginBindingModel model)
{
    if (!ModelState.IsValid)
    {
        return BadRequest(ModelState);
    }

    Authentication.SignOut(DefaultAuthenticationTypes.ExternalCookie);

    AuthenticationTicket ticket = AccessTokenFormat.Unprotect(model.ExternalAccessToken);

    if (ticket == null || ticket.Identity == null || (ticket.Properties != null
        && ticket.Properties.ExpiresUtc.HasValue
        && ticket.Properties.ExpiresUtc.Value < DateTimeOffset.UtcNow))
    {
        return BadRequest("External login failure.");
    }

    ExternalLoginData externalData = ExternalLoginData.FromIdentity(ticket.Identity);

    if (externalData == null)
    {
        return BadRequest("The external login is already associated with an account.");
    }

    IdentityResult result = await UserManager.AddLoginAsync(User.Identity.GetUserId(),
        new UserLoginInfo(externalData.LoginProvider, externalData.ProviderKey));

    IHttpActionResult errorResult = GetErrorResult(result);

    if (errorResult != null)
    {
        return errorResult;
    }

    return Ok();
}

Inside this method, UserManager.AddLoginAsync is called and UserManager.UpdateAsync is not called. RavenUserStore does not save changes on every call. It only does it with UpdateAsync method.

On the other hand, I would suggest to work with the UoW pattern as Ayende described here: http://ayende.com/blog/163170/building-async-unit-of-work-with-mvc-4 By going with this option, you shouldn't make any configuration changes to the AccountController class which comes with the project templates.

[tugberkugurlu]

I'm going to write a blog post about how to make RavenDB port of ASP.NET identity work with the default templates soon.

[MortenChristiansen]

I'm afraid that is not the problem I have, since I do not use the AddExternalLogin method at all (though it is nice to know since I will likely be using it in the future). If there should be a problem with my code, it would likely be in RegisterExternal or GetExternalLogin.

[tugberkugurlu]

I see your problem now:

    // POST api/Account/RegisterExternal
    [OverrideAuthentication]
    [HostAuthentication(DefaultAuthenticationTypes.ExternalBearer)]
    [Route("RegisterExternal")]
    public async Task<IHttpActionResult> RegisterExternal(RegisterExternalBindingModel model)
    {
        if (!ModelState.IsValid)
        {
            return BadRequest(ModelState);
        }

        ExternalLoginData externalLogin = ExternalLoginData.FromIdentity(User.Identity as ClaimsIdentity);

        if (externalLogin == null)
        {
            return InternalServerError();
        }

        RavenUser user = new RavenUser(model.UserName);
        user.AddLogin(new RavenUserLogin(externalLogin.UserName, new UserLoginInfo(externalLogin.LoginProvider, externalLogin.ProviderKey)));
        IdentityResult result = await UserManager.CreateAsync(user);
        IHttpActionResult errorResult = GetErrorResult(result);

        if (errorResult != null)
        {
            return errorResult;
        }

        return Ok();
    }

Why are you calling user.AddLogin there? You should call UserManager.AddLoginAsync instead right after you call UserManager.CreateUserAync (just like I did here: https://github.com/tugberkugurlu/AspNet.Identity.RavenDB/commit/2dc4108b4512ac7d079a2f964d218eb410b2a7f2).

Here is the correctly implemented version of that method:

// POST api/Account/RegisterExternal
[OverrideAuthentication]
[HostAuthentication(DefaultAuthenticationTypes.ExternalBearer)]
[Route("RegisterExternal")]
public async Task<IHttpActionResult> RegisterExternal(RegisterExternalBindingModel model)
{
    if (!ModelState.IsValid)
    {
        return BadRequest(ModelState);
    }

    ExternalLoginData externalLogin = ExternalLoginData.FromIdentity(User.Identity as ClaimsIdentity);

    if (externalLogin == null)
    {
        return InternalServerError();
    }

    RavenUser user = new RavenUser(model.UserName);
    IdentityResult result = await UserManager.CreateAsync(user);
    if (result.Succeeded)
    {
        result = await UserManager.AddLoginAsync(user.Id, new UserLoginInfo(externalLogin.LoginProvider, externalLogin.ProviderKey));
        if (result.Succeeded)
        {
            return Ok();
        }
    }

    return GetErrorResult(result);
}

[MortenChristiansen]

Yes, that works. The reason I did it that way is due to the original code which was defined like this:

...
IdentityUser user = new IdentityUser
{
    UserName = model.UserName
};
user.Logins.Add(new IdentityUserLogin
{
    LoginProvider = externalLogin.LoginProvider,
    ProviderKey = externalLogin.ProviderKey
});
IdentityResult result = await UserManager.CreateAsync(user);
...

It only seemed natural to continue the same pattern. If you write up a blog post about it make sure to point this out. While it turned out that there was no bug after all, I think it served to highlight that a bit of guidance would be helpful.

Thanks for looking into my problems :)

[tugberkugurlu]

Thanks for that code snippet.

That's the wrong way of doing it. I filed an issue on ASP.NET Identity repository: https://aspnetidentity.codeplex.com/workitem/2165 You can vote it up.