Conversation
…il is not verified
Email verification improvements
…4MasterServer into account-improvements
Added layer of abstraction to email service
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Conflicts have been resolved. |
| { | ||
| var filter = (Builders<Account>.Filter.BitsAnyClear(f => f.Flags, (long)AccountFlags.EmailVerified) | | ||
| Builders<Account>.Filter.Exists(f => f.Flags, false)) & | ||
| Builders<Account>.Filter.Lt(f => f.LastLoginAt, DateTime.UtcNow.AddMonths(-1)); |
There was a problem hiding this comment.
| Builders<Account>.Filter.Lt(f => f.LastLoginAt, DateTime.UtcNow.AddMonths(-1)); | |
| Builders<Account>.Filter.Lt(f => f.LastLoginAt, DateTime.UtcNow.AddMonths(-3)); |
Increase time to 3 months because we are not really in a hurry to delete unused accounts.
| public string? VerificationLinkGUID { get; set; } | ||
|
|
||
| [BsonIgnoreIfNull] | ||
| public DateTime? VerificationLinkExpiration { get; set; } |
There was a problem hiding this comment.
I think it would make more sense to have something like LastEmailChangeAt because that gives more information and control than just having expiry date.
LastEmailChangeAt can be set to null when not needed.
There was a problem hiding this comment.
These properties come in pair: GUID + ExpirationDate. Not sure if changing will help now because I used term "verification link" all over the place.
| public string? ResetLinkGUID { get; set; } | ||
|
|
||
| [BsonIgnoreIfNull] | ||
| public DateTime? ResetLinkExpiration { get; set; } |
There was a problem hiding this comment.
And something like LastPasswordChangeAt here.
LastPasswordChangeAt can be set to null when not needed.
There was a problem hiding this comment.
These properties come in pair: GUID + ExpirationDate. Not sure if changing will help now because I used term "reset password" all over the place.
| Host = applicationSettings.WebsiteDomain, | ||
| Port = applicationSettings.WebsitePort, | ||
| Path = "VerifyEmail", | ||
| Query = $"accountId={accountID}&guid={guid}" |
There was a problem hiding this comment.
Is accountId necessary? User should be logged in in order to verify email address.
When he is logged in, accountId is known.
There was a problem hiding this comment.
It is needed since verification link is accessed via anonymous endpoint.
| public string WebsiteScheme { get; set; } = string.Empty; | ||
|
|
||
| /// <summary> | ||
| /// Used just to redirect users to correct domain when UT4UU is being used. |
| awsSettings.Value.AccessKey, | ||
| awsSettings.Value.SecretKey, | ||
| Amazon.RegionEndpoint.GetBySystemName(awsSettings.Value.RegionName)); |
There was a problem hiding this comment.
I didn't check what happens, but server shouldn't crash if these are not correct. Instead errors should be logged.
There was a problem hiding this comment.
I don't think that server will crash, instead global error handler will catch the error.
|
Since this PR is named Account improvements we should also add |
|
Regarding the warning...
We should probably just add a static hardcoded DateTime check whether some date has passed and only activate deletion after that date. Before that date we would notify everyone on discord & send email to every existing account (with real email). |
3 participants
Closes #139
The following features are added/improved:
Warning:
Upon deployment to production, CleanupService will delete all accounts that didn't login for the past 1 month. This logic should be revised.
Steps to do before deployment: