JudahGabriel / RavenDB.Identity

RavenDB Identity provider for ASP.NET Core. Let RavenDB manage your users and logins.
https://www.nuget.org/packages/RavenDB.Identity/1.0.0
MIT License
61 stars 29 forks source link

UserStore: UpdateAsync and other methods not updating documents? #36

Closed germanftorres closed 3 years ago

germanftorres commented 3 years ago

Hi,

After configuring the RavenDB.Identity stores, I'm trying to add a user and also some claims to the user.

I see that the document representing the user get's created in the database, but the claims collection is still empty after adding the claims.

    alice = new ApplicationUser
    {
        UserName = "alice",
        Email = "AliceSmith@email.com",
        EmailConfirmed = true,
    };

    // This creates the document in RavenDB as SaveChangesAsync is called
    // internally in the UserStore
    var result = userMgr.CreateAsync(alice, "Pass123$").Result;
    if (!result.Succeeded)
    {
        throw new Exception(result.Errors.First().Description);
    }

    // This doesn't update the ApplicationUser document, as
    // the UserStore implementation based on Raven only updates
    // the entity, but doesn't try to call SaveChangesAsync
    result = userMgr.AddClaimsAsync(alice, new Claim[]{
            new Claim(JwtClaimTypes.Name, "Alice Smith"),
            new Claim(JwtClaimTypes.GivenName, "Alice"),
            new Claim(JwtClaimTypes.FamilyName, "Smith"),
            new Claim(JwtClaimTypes.WebSite, "http://alice.com"),
        }).Result;
    if (!result.Succeeded)
    {
        throw new Exception(result.Errors.First().Description);
    }

    // Even forcing an update of the user does not call internally DbSession.SaveChangesAsync()
    // as only email changes are tracked to try to update the email reservation.
    userMgr.UpdateAsync(alice).Wait();

    Log.Debug("alice created");

Following the code in UserStore.cs, I'm a little bit confused that only CreateAsync and DeleteAsync actually persist immediatly calling ravenSession.SaveChangesAsync. I wonder if this behavior is something documented or expected by aspnet identity UserManager. Shouldn't all of these methods try to update the database as eagerly as possible?

Thanks!

Germán

JudahGabriel commented 3 years ago

Shouldn't all of these methods try to update the database as eagerly as possible?

As described in the documentation, you must call .SaveChangesAsync when you're done. The reason for this is transactions, really. Apps might need to do more than one thing -- e.g. update a Doctor and a Patient record together -- if one of them fails, we want the whole thing to rollback.

There are some caveats, as you've discovered: creating a user saves changes eagerly. The reason for that is, user email reservation is a cluster-wide operation, outside the scope of a single transaction. This is necessary for RavenDB.Identity to ensure email uniqueness when running in a cluster.

The best practice is, always call .SaveChangesAsync when you're done. As our documentation states,

In your controller actions, call .SaveChangesAsync() when you're done making changes. Typically this is done via a RavenController base class for MVC/WebAPI projects or via a page filter for Razor Pages projects.

ganmuru commented 3 years ago

I have a similar issue. The userManager.UpdateAsync() call fails with changes not getting persisted.

I added the RaveDb.Identity project directly to my solution and tracked the "usermanager.UpdateAsync(user)" call. In UserStore.UpdateAsync() - var changes = DbSession.Advanced.WhatChanged(); is 0 (no changes) while the user object has the updated values. Hence none of the changes are getting persisted.

dbSession.SaveChanges() is called after the UpdateAsync call. Looks like the IAsyncDocumentSession is not getting registered. Any idea why?

JudahGabriel commented 3 years ago

@ganmuru Show me your code that loads the user and updates the user.

ganmuru commented 3 years ago

GetUsers() var users = await _manager.Users.ToListAsync();

AppUser has the updated IdentityUser details.

public async Task<IActionResult> Update([FromForm] AppUser model)
        {
            try
            {
                var result = await _manager.UpdateAsync(model);
                if (result.Succeeded)
                {
                    await _dbSession.SaveChangesAsync();
                    return NoContent();
                }
                return BadRequest(result);
            }
            catch (Exception ex)
            {
                return BadRequest(ex);
            }
        }
JudahGabriel commented 3 years ago

I think the reason is, you're not loading an existing user and updating it; you're instead taking a user directly from the HTTP POST and updating.

To fix this, you should do something like this:

public async Task<IActionResult> Update([FromForm] AppUser model)
{
    try
    {
                var existingUser = await _dbSession.LoadAsync(model.Id);
                existingUser.UpdateFrom(model); // you implement this
        var result = await _manager.UpdateAsync(existingUser);
        if (result.Succeeded)
        {
            await _dbSession.SaveChangesAsync();
            return NoContent();
        }
        return BadRequest(result);
    }
    catch (Exception ex)
    {
        return BadRequest(ex);
    }
}

This might be considered a bug, as I can see how it's an unexpected event here. I may update the behavior in the future to be as you expect.

ganmuru commented 3 years ago

Thanks for the suggestion. I ended up using https://www.nuget.org/packages/ValueInjecter/ to copy the object and it works fine.

 var existingUser = await _dbSession.LoadAsync<AppUser>(model.Id);
                existingUser.InjectFrom(model);                
                var result = await _manager.UpdateAsync(existingUser);

I also submitted a PR https://github.com/JudahGabriel/RavenDB.Identity/pull/37 for few changes related to IdentityRole and RoleStore. Can you please review and approve?

JudahGabriel commented 3 years ago

Approved, merged, and pushed out as RavenDB.Identity 8.0.5