From 1a20eb5bc630fc2ce335881808f7ad05996179c0 Mon Sep 17 00:00:00 2001 From: skofman Date: Fri, 16 Sep 2016 14:35:00 -0700 Subject: [PATCH 1/2] Reduce password reset token expiration latency to 1 hour --- src/NuGetGallery/Constants.cs | 2 +- src/NuGetGallery/Services/MessageService.cs | 8 +++++--- src/NuGetGallery/Strings.Designer.cs | 12 ++++++------ src/NuGetGallery/Strings.resx | 12 ++++++------ .../Controllers/UsersControllerFacts.cs | 10 +++++----- 5 files changed, 23 insertions(+), 21 deletions(-) diff --git a/src/NuGetGallery/Constants.cs b/src/NuGetGallery/Constants.cs index 63bf4454ec..9be91c72c2 100644 --- a/src/NuGetGallery/Constants.cs +++ b/src/NuGetGallery/Constants.cs @@ -11,7 +11,7 @@ public static class Constants public const string AlphabeticSortOrder = "package-title"; public const int DefaultPackageListPageSize = 20; public const string DefaultPackageListSortOrder = "package-download-count"; - public const int DefaultPasswordResetTokenExpirationHours = 24; + public const int DefaultPasswordResetTokenExpirationHours = 1; public const int MaxEmailSubjectLength = 255; internal static readonly NuGetVersion MaxSupportedMinClientVersion = new NuGetVersion("3.4.0.0"); public const string PackageContentType = "binary/octet-stream"; diff --git a/src/NuGetGallery/Services/MessageService.cs b/src/NuGetGallery/Services/MessageService.cs index 8549931bf0..cfdfe38e98 100644 --- a/src/NuGetGallery/Services/MessageService.cs +++ b/src/NuGetGallery/Services/MessageService.cs @@ -264,14 +264,16 @@ changed from _{1}_ to _{2}_. public void SendPasswordResetInstructions(User user, string resetPasswordUrl, bool forgotPassword) { - string body = String.Format( + string body = string.Format( CultureInfo.CurrentCulture, forgotPassword ? Strings.Emails_ForgotPassword_Body : Strings.Emails_SetPassword_Body, - Constants.DefaultPasswordResetTokenExpirationHours, resetPasswordUrl, Config.GalleryOwner.DisplayName); - string subject = String.Format(CultureInfo.CurrentCulture, forgotPassword ? Strings.Emails_ForgotPassword_Subject : Strings.Emails_SetPassword_Subject, Config.GalleryOwner.DisplayName); + string subject = string.Format( + CultureInfo.CurrentCulture, forgotPassword ? Strings.Emails_ForgotPassword_Subject : Strings.Emails_SetPassword_Subject, + Config.GalleryOwner.DisplayName); + using (var mailMessage = new MailMessage()) { mailMessage.Subject = subject; diff --git a/src/NuGetGallery/Strings.Designer.cs b/src/NuGetGallery/Strings.Designer.cs index 8ee40a6484..9fec744f50 100644 --- a/src/NuGetGallery/Strings.Designer.cs +++ b/src/NuGetGallery/Strings.Designer.cs @@ -317,12 +317,12 @@ public static string Emails_CredentialRemoved_Subject { /// Looks up a localized string similar to The word on the street is you lost your password. Sorry to hear it! ///If you haven't forgotten your password you can safely ignore this email. Your password has not been changed. /// - ///Click the following link within the next {0} hours to reset your password: + ///Click the following link within the next hour to reset your password: /// - ///[{1}]({1}) + ///[{0}]({0}) /// ///Thanks, - ///The {2} Team. + ///The {1} Team. /// public static string Emails_ForgotPassword_Body { get { @@ -343,12 +343,12 @@ public static string Emails_ForgotPassword_Subject { /// Looks up a localized string similar to The word on the street is you want to set a password for your account. ///If you didn't request a password, you can safely ignore this message. A password has not yet been set. /// - ///Click the following link within the next {0} hours to set your password: + ///Click the following link within the next hour to set your password: /// - ///[{1}]({1}) + ///[{0}]({0}) /// ///Thanks, - ///The {2} Team. + ///The {1} Team. /// public static string Emails_SetPassword_Body { get { diff --git a/src/NuGetGallery/Strings.resx b/src/NuGetGallery/Strings.resx index d514b83fb8..db231aa5eb 100644 --- a/src/NuGetGallery/Strings.resx +++ b/src/NuGetGallery/Strings.resx @@ -241,12 +241,12 @@ The word on the street is you lost your password. Sorry to hear it! If you haven't forgotten your password you can safely ignore this email. Your password has not been changed. -Click the following link within the next {0} hours to reset your password: +Click the following link within the next hour to reset your password: -[{1}]({1}) +[{0}]({0}) Thanks, -The {2} Team +The {1} Team [{0}] Please reset your password. @@ -255,12 +255,12 @@ The {2} Team The word on the street is you want to set a password for your account. If you didn't request a password, you can safely ignore this message. A password has not yet been set. -Click the following link within the next {0} hours to set your password: +Click the following link within the next hour to set your password: -[{1}]({1}) +[{0}]({0}) Thanks, -The {2} Team +The {1} Team [{0}] Please set your password. diff --git a/tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs index dcc8025cb1..a6a75d426b 100644 --- a/tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs @@ -174,7 +174,7 @@ public async Task SendsEmailWithPasswordResetUrl() { EmailAddress = "some@example.com", PasswordResetToken = "confirmation", - PasswordResetTokenExpirationDate = DateTime.UtcNow.AddDays(1) + PasswordResetTokenExpirationDate = DateTime.UtcNow.AddHours(Constants.DefaultPasswordResetTokenExpirationHours) }; GetMock() .Setup(s => s.SendPasswordResetInstructions(user, resetUrl, true)); @@ -182,7 +182,7 @@ public async Task SendsEmailWithPasswordResetUrl() .Setup(s => s.FindByEmailAddress("user")) .Returns(user); GetMock() - .Setup(s => s.GeneratePasswordResetToken("user", 1440)) + .Setup(s => s.GeneratePasswordResetToken("user", Constants.DefaultPasswordResetTokenExpirationHours * 60)) .CompletesWith(user); var controller = GetController(); var model = new ForgotPasswordViewModel { Email = "user" }; @@ -198,7 +198,7 @@ public async Task RedirectsAfterGeneratingToken() { var user = new User { EmailAddress = "some@example.com", Username = "somebody" }; GetMock() - .Setup(s => s.GeneratePasswordResetToken("user", 1440)) + .Setup(s => s.GeneratePasswordResetToken("user", Constants.DefaultPasswordResetTokenExpirationHours * 60)) .CompletesWith(user) .Verifiable(); var controller = GetController(); @@ -209,14 +209,14 @@ public async Task RedirectsAfterGeneratingToken() Assert.NotNull(result); GetMock() - .Verify(s => s.GeneratePasswordResetToken("user", 1440)); + .Verify(s => s.GeneratePasswordResetToken("user", Constants.DefaultPasswordResetTokenExpirationHours * 60)); } [Fact] public async Task ReturnsSameViewIfTokenGenerationFails() { GetMock() - .Setup(s => s.GeneratePasswordResetToken("user", 1440)) + .Setup(s => s.GeneratePasswordResetToken("user", Constants.DefaultPasswordResetTokenExpirationHours * 60)) .CompletesWithNull(); var controller = GetController(); From 8f7f7cb795be8d5d5e319e731eba935897f0d68d Mon Sep 17 00:00:00 2001 From: skofman Date: Fri, 16 Sep 2016 15:23:01 -0700 Subject: [PATCH 2/2] Changed the constant from DefaultPasswordResetTokenExpirationHours to PasswordResetTokenExpirationHours --- src/NuGetGallery/Constants.cs | 2 +- src/NuGetGallery/Controllers/UsersController.cs | 6 +++--- src/NuGetGallery/Services/MessageService.cs | 1 - .../Controllers/UsersControllerFacts.cs | 10 +++++----- 4 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/NuGetGallery/Constants.cs b/src/NuGetGallery/Constants.cs index 9be91c72c2..18871c68bb 100644 --- a/src/NuGetGallery/Constants.cs +++ b/src/NuGetGallery/Constants.cs @@ -11,7 +11,7 @@ public static class Constants public const string AlphabeticSortOrder = "package-title"; public const int DefaultPackageListPageSize = 20; public const string DefaultPackageListSortOrder = "package-download-count"; - public const int DefaultPasswordResetTokenExpirationHours = 1; + public const int PasswordResetTokenExpirationHours = 1; public const int MaxEmailSubjectLength = 255; internal static readonly NuGetVersion MaxSupportedMinClientVersion = new NuGetVersion("3.4.0.0"); public const string PackageContentType = "binary/octet-stream"; diff --git a/src/NuGetGallery/Controllers/UsersController.cs b/src/NuGetGallery/Controllers/UsersController.cs index 84df6967f2..e148ab3b94 100644 --- a/src/NuGetGallery/Controllers/UsersController.cs +++ b/src/NuGetGallery/Controllers/UsersController.cs @@ -195,7 +195,7 @@ public virtual async Task ForgotPassword(ForgotPasswordViewModel m if (ModelState.IsValid) { - var user = await _authService.GeneratePasswordResetToken(model.Email, Constants.DefaultPasswordResetTokenExpirationHours * 60); + var user = await _authService.GeneratePasswordResetToken(model.Email, Constants.PasswordResetTokenExpirationHours * 60); if (user != null) { return SendPasswordResetEmail(user, forgotPassword: true); @@ -215,7 +215,7 @@ public virtual ActionResult PasswordSent() ViewData[Constants.ReturnUrlViewDataKey] = null; ViewBag.Email = TempData["Email"]; - ViewBag.Expiration = Constants.DefaultPasswordResetTokenExpirationHours; + ViewBag.Expiration = Constants.PasswordResetTokenExpirationHours; return View(); } @@ -450,7 +450,7 @@ public virtual async Task ChangePassword(AccountViewModel model) if (oldPassword == null) { // User is requesting a password set email - await _authService.GeneratePasswordResetToken(user, Constants.DefaultPasswordResetTokenExpirationHours * 60); + await _authService.GeneratePasswordResetToken(user, Constants.PasswordResetTokenExpirationHours * 60); return SendPasswordResetEmail(user, forgotPassword: false); } diff --git a/src/NuGetGallery/Services/MessageService.cs b/src/NuGetGallery/Services/MessageService.cs index cfdfe38e98..5d879e003f 100644 --- a/src/NuGetGallery/Services/MessageService.cs +++ b/src/NuGetGallery/Services/MessageService.cs @@ -285,7 +285,6 @@ public void SendPasswordResetInstructions(User user, string resetPasswordUrl, bo } } - public void SendPackageOwnerRequest(User fromUser, User toUser, PackageRegistration package, string confirmationUrl) { if (!toUser.EmailAllowed) diff --git a/tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs b/tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs index a6a75d426b..3ecc8e4f1b 100644 --- a/tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs +++ b/tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs @@ -174,7 +174,7 @@ public async Task SendsEmailWithPasswordResetUrl() { EmailAddress = "some@example.com", PasswordResetToken = "confirmation", - PasswordResetTokenExpirationDate = DateTime.UtcNow.AddHours(Constants.DefaultPasswordResetTokenExpirationHours) + PasswordResetTokenExpirationDate = DateTime.UtcNow.AddHours(Constants.PasswordResetTokenExpirationHours) }; GetMock() .Setup(s => s.SendPasswordResetInstructions(user, resetUrl, true)); @@ -182,7 +182,7 @@ public async Task SendsEmailWithPasswordResetUrl() .Setup(s => s.FindByEmailAddress("user")) .Returns(user); GetMock() - .Setup(s => s.GeneratePasswordResetToken("user", Constants.DefaultPasswordResetTokenExpirationHours * 60)) + .Setup(s => s.GeneratePasswordResetToken("user", Constants.PasswordResetTokenExpirationHours * 60)) .CompletesWith(user); var controller = GetController(); var model = new ForgotPasswordViewModel { Email = "user" }; @@ -198,7 +198,7 @@ public async Task RedirectsAfterGeneratingToken() { var user = new User { EmailAddress = "some@example.com", Username = "somebody" }; GetMock() - .Setup(s => s.GeneratePasswordResetToken("user", Constants.DefaultPasswordResetTokenExpirationHours * 60)) + .Setup(s => s.GeneratePasswordResetToken("user", Constants.PasswordResetTokenExpirationHours * 60)) .CompletesWith(user) .Verifiable(); var controller = GetController(); @@ -209,14 +209,14 @@ public async Task RedirectsAfterGeneratingToken() Assert.NotNull(result); GetMock() - .Verify(s => s.GeneratePasswordResetToken("user", Constants.DefaultPasswordResetTokenExpirationHours * 60)); + .Verify(s => s.GeneratePasswordResetToken("user", Constants.PasswordResetTokenExpirationHours * 60)); } [Fact] public async Task ReturnsSameViewIfTokenGenerationFails() { GetMock() - .Setup(s => s.GeneratePasswordResetToken("user", Constants.DefaultPasswordResetTokenExpirationHours * 60)) + .Setup(s => s.GeneratePasswordResetToken("user", Constants.PasswordResetTokenExpirationHours * 60)) .CompletesWithNull(); var controller = GetController();