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

UserManager.ConfirmEmailAsync not working #22

Closed JoshClose closed 4 years ago

JoshClose commented 4 years ago

I'm implementing an email address confirmation controller action. I get the user, call ConfirmEmailAsync, and see that User.EmailConfirmed = true, but it doesn't get saved in RavenDB. I tried adding Session.SaveChangesAsync(), but that didn't help. The only way I could get it working was to re-get the user from the Session directly, set User.EmailConfirmed = true and do SaveChangesAsync().

var user = await userManager.FindByEmailAsync(email);
if (user == null)
{
    return NotFound();
}

var result = await userManager.ConfirmEmailAsync(user, code);

if (result.Succeeded)
{
    user = await Session.Query<User>().SingleAsync(u => u.Email == email);
    user.EmailConfirmed = true;
    await Session.SaveChangesAsync();

    return Redirect("~/account/email-confirmed");
}

return Redirect("~/account/confirm-email-error");

I looked at UserManager.store and it's an instance of Raven.Identity.UserStore.

Here is a log of what goes through RavenDB when I don't have the extra 3 lines to re-get the user.

[
  {
    "TimeStamp": "2019-12-09T00:21:03.9275580Z",
    "RequestId": 27,
    "HttpMethod": "POST",
    "ElapsedMilliseconds": 0,
    "ResponseStatusCode": 304,
    "RequestUri": "http://127.0.0.1:8080/databases/InfinitWifi/queries?queryHash=3841586971965068908",
    "AbsoluteUri": "http://127.0.0.1:8080",
    "DatabaseName": "InfinitWifi",
    "CustomInfo": "from Users where Email = $p0 limit $p1, $p2\r\n{\"p0\":\"ME@GMAIL.COM\",\"p1\":0,\"p2\":1}",
    "Type": "Queries"
  },
  {
    "TimeStamp": "2019-12-09T00:21:03.9351362Z",
    "RequestId": 28,
    "HttpMethod": "GET",
    "ElapsedMilliseconds": 0,
    "ResponseStatusCode": 200,
    "RequestUri": "http://127.0.0.1:8080/databases/InfinitWifi/cmpxchg?&key=emails%2Fme%40gmail.com",
    "AbsoluteUri": "http://127.0.0.1:8080",
    "DatabaseName": "InfinitWifi",
    "CustomInfo": "N/A",
    "Type": "None"
  },
  {
    "TimeStamp": "2019-12-09T00:21:03.9454399Z",
    "RequestId": 29,
    "HttpMethod": "GET",
    "ElapsedMilliseconds": 0,
    "ResponseStatusCode": 304,
    "RequestUri": "http://127.0.0.1:8080/databases/InfinitWifi/docs?&id=users%2Fme%40gmail.com",
    "AbsoluteUri": "http://127.0.0.1:8080",
    "DatabaseName": "InfinitWifi",
    "CustomInfo": "users/me@gmail.com",
    "Type": "Documents"
  }
]
JudahGabriel commented 4 years ago

What is Session here? My first thought is that Session isn't the one used by Raven.Identity.

JoshClose commented 4 years ago

It's IAsyncDocumentSession.

services
    .AddRavenDbAsyncSession(CreateDocumentStore())
    .AddRavenDbIdentity<User, Role>();
private IDocumentStore CreateDocumentStore()
{
    var settings = Configuration.GetSection(nameof(RavenDb)).Get<RavenDb>();

    X509Certificate2? cert = null;
    if (!string.IsNullOrEmpty(settings.Certificate))
    {
        var certFile = Path.Combine(Directory.GetCurrentDirectory(), settings.Certificate);
        if (File.Exists(certFile))
        {
            cert = new X509Certificate2(certFile);
        }
    }

    if (cert == null)
    {
        Console.WriteLine($"No certificate was found for RavenDb.");
    }

    var store = new DocumentStore
    {
        Certificate = cert,
        Database = settings.Database,
        Urls = settings.Urls.ToArray(),
        Conventions =
        {
            CustomizeJsonSerializer = serializer =>
            {
                serializer.Converters.Add(new IPAddressConverter());
                serializer.Converters.Add(new MacAddressConverter());
            }
        }
    };

    store.Conventions.RegisterAsyncIdConvention<AccessPoint>((dbName, ap) => Task.FromResult(store.CreateId<AccessPoint>(ap.MacAddress)));

    store.Initialize().EnsureExists();

    IndexCreation.CreateIndexes(typeof(User).Assembly, store);

    return store;
}
JoshClose commented 4 years ago

I was thinking maybe UserManager was a singleton, but it's scoped. https://github.com/aspnet/Identity/blob/feedcb5c53444f716ef5121d3add56e11c7b71e5/src/Identity/IdentityServiceCollectionExtensions.cs#L91

JoshClose commented 4 years ago

All my controller inherit a base controller. That is what Session is.

protected IAsyncDocumentSession Session { get; private set; }

public BaseController(IAsyncDocumentSession session)
{
    Session = session ?? throw new ArgumentNullException(nameof(session));
    Session.Advanced.WaitForIndexesAfterSaveChanges(TimeSpan.FromSeconds(30));
}
JudahGabriel commented 4 years ago

This still looks like the Session is not the same session that Raven.Identity is using.

Try this:

var user = await userManager.FindByEmailAsync(email);
var otherUser = await Session.Query<User>().SingleAsync(u => u.Email == email);

// This should print true
Console.WriteLine(user == otherUser);

Does this print true? If not, Session is not the session being used by RavenDB.Identity.

JoshClose commented 4 years ago

Surprisingly enough, it's true.

JoshClose commented 4 years ago

I think this has something to do with the .NET Core 3 upgrade. My setup script that creates a user and adds a role to them doesn't work. The user gets created, but the role isn't added to the user.

I have the RavenDB.Identity source back in my solution now and am debugging through it.

JoshClose commented 4 years ago

This commit 5a0373738e99d5b48e1863413803b1b08327b499 for issue #21 broke it.

UserManager.AddToRoleAsync will call UserStore.AddToRoleAsync and then UserStore.Update to save it. I had other creates happening after which was calling IAsyncDocumentSession.SaveChangesAsync so that's why it was working for me before.

The issue with the fix you did is that it it clears the session which sets IAsyncDocumentSession.Advanced.HasChanges from true to false. If you save changes immediately after that, nothing happens because it doesn't think there are any changes. There are other things on the user besides UserName that can change, so you can't just check for that. You can do the compare exchange stuff if the names are different, but all the other changes need to get saved if their values are different too.

The logic between CreateAsync and UpdateAsync is different too, which could cause a problem. CreateAsync users User.Email in the compare exchange, but UpdateAsync uses User.UserName, which may or may not be User.Email. Email seems like a better choice because it's probably going to change less often and will have less special characters, spaces, etc in it.

You will still need to update User.Email if that changes though.

I'm going to attempt a pull request to fix this. I did update the dependency from Microsoft.AspNetCore.Identity 2.2.0 to Microsoft.Extensions.Identity.Core 3.1.0 because everything from MS should be 3.1.0 now from what I understand. 2.2.0 and 3.0.0 both have short lived service.

JoshClose commented 4 years ago

Also looks like UserStore.FindByNameAsync shouldn't use the compare exchange key since the username and email can be different.

JoshClose commented 4 years ago

I just submitted a pull request that fixes all the things I mentioned. #24

JudahGabriel commented 4 years ago

Bummer that the fix to #21 broke this. :-(

With regards to email vs. user name, I'm on the fence. I agree we need to pick one and stick with it. Previously, we were using email, but a user requested a change here.

In my own apps, I use email as the user name. Occasionally, users have asked to update their email address. That occasionally happens; a user migrates to a new email service or ISP and has a new email. But its rare. We'll stick with email for now, as changing this would be a breaking fix.

I'll review your PR. Thanks for finding and reporting this.

JudahGabriel commented 4 years ago

Merged your fix. I also updated the samples and documentation to reflect the new API. Also, I changed the API slightly: AddRavenDBStores is now AddRavenDBIdentityStores, as I felt the former too ambiguous.

I've published the new package as RavenDB.Identity 7.0 to reflect the breaking API change.

JoshClose commented 4 years ago

Great, thanks!

djordjedjukic commented 4 years ago

@JoshClose hey is this fixed? I have the same issue like you with userManager.ConfirmEmailAsync(user, code); If I get user after in code EmailConfirmed is true, but in DB is false.

JoshClose commented 4 years ago

It was last December. :) Things could have changed since then.

JudahGabriel commented 4 years ago

@djordjedjukic are you calling SaveChangesAsync?

djordjedjukic commented 4 years ago

@JudahGabriel No. I thought it will do it inside method.

JudahGabriel commented 4 years ago

Nope, that’s left to your code for the sake of efficiency and control.

In the Getting started guide, we document that:

Call SaveChangesAsync after making changes. This is typically done in a controller base class.

The getting started guide has 2 examples, one for MVC/Web API and another for Razor Pages, which sure best practice for calling SaveChangesAsync.

djordjedjukic commented 4 years ago

I missed it. Just one question, should I call SaveChangesAsync() always? Because i.e _userManager.CreateAsync(user, password) works without SaveChangesAsync().

JudahGabriel commented 4 years ago

Always call SaveChangesAsync.

CreateAsync happens to internally call SaveChangesAsync because it's needed for the email uniqueness requirement.

But the good rule of thumb is, always call .SaveChangesAsync. I recommend using a RavenController base class for MVC/Web Api, or a filter for Razor Pages.