mrellipse / toucan

Boilerplate template using Vue.js, TypeScript and .NET Core 2.1, based on SOLID design principles
MIT License
212 stars 36 forks source link

Expired verifications causes primary key conflict when attempting to verify #20

Closed bjammin closed 6 years ago

bjammin commented 6 years ago

If there are any expired verifications for a user, the system still attempts to insert a new one if the user requests verification. This causes a primary key conflict.

I fixed this by adding this code in GetPendingVerificationForUser to first delete any expired verifications for the user. Not sure if this is the best approach - if so, please add to baseline. If not, please tell me a better way to fix it :)

    private async Task<Verification> GetPendingVerificationForUser(IUser user)
    {
        // first get any expires ones
        var expiredVerifications =
        await (from v in this.db.Verification.Include(o => o.User).Include(o => o.User.Roles)
            where v.UserId == user.UserId && v.RedeemedAt == null &&
            v.IssuedAt <= DateTime.UtcNow.AddMinutes(-30)
            select v).ToListAsync();

        // and delete them
        foreach (var verification in expiredVerifications)
        {
            this.db.Verification.Remove(verification);
            this.db.SaveChanges();
        }
mrellipse commented 6 years ago

hi @bjammin, thanks for all the recent feedback!

no need for above code change - the underlying cause is incorrect instructions in the ef scaffolding

it was accidentally setting the 'user id' field as the primary key, rather than the 'code field' (which is a guid string).

easily fixed!

mrellipse commented 6 years ago

this one has actually highlighted a bigger issue which I need to think about for a bit =S

how you supply dates when writing ef queries varies depending on the target provider (postgres or mssql). pgsql ef provider expects a local datetime as input. mssql ef provider expects a utc datetime as input.

once you scaffold out a solution, you tie it to one particular ef provider, and it's not a problem going forwards. all the code you write will be consistent in it's use of local or utc when writing queries.

but 'system' code has to be agnostic about what ef provider it is querying. i will have to do some research, and think about what is appropriate. probably just an extension method :)

if you have any thoughts, let me know!

mrellipse commented 6 years ago

coded a fix. will include in deployment for next release.