Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reduce password reset token expiration latency to 1 hour #3244

Merged
merged 2 commits into from
Sep 19, 2016
Merged

Reduce password reset token expiration latency to 1 hour #3244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a20eb5
Reduce password reset token expiration latency to 1 hour
skofman1 Sep 16, 2016
8f7f7cb
Changed the constant from DefaultPasswordResetTokenExpirationHours to…
skofman1 Sep 16, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/NuGetGallery/Constants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 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";
Expand Down
6 changes: 3 additions & 3 deletions src/NuGetGallery/Controllers/UsersController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ public virtual async Task<ActionResult> 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);
Expand All @@ -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();
}

Expand Down Expand Up @@ -450,7 +450,7 @@ public virtual async Task<ActionResult> 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);
}
Expand Down
9 changes: 5 additions & 4 deletions src/NuGetGallery/Services/MessageService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -283,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)
Expand Down
12 changes: 6 additions & 6 deletions src/NuGetGallery/Strings.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 6 additions & 6 deletions src/NuGetGallery/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,12 +241,12 @@
<value>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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this break if somebody makes their own instance of the gallery and sets the password reset token expiration to something else?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly if this is configurable it can be reduced to a lower number, and the message should apply. Perhaps we allow a minutes more to? CC @blowdart

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minutes are always good. But having been on the end of a flickr lockout because the time it took to send me the email was longer than the token expiry ... it does need configuring. I'd almost default to 15 minutes, but support hours in both code and text, but then I'm mean.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Password expiration time is a constant in our code, so if someone modifies it, he might as well modify the message.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the constant from DefaultPasswordResetTokenExpirationHours to PasswordResetTokenExpirationHours 😄


[{1}]({1})
[{0}]({0})

Thanks,
The {2} Team</value>
The {1} Team</value>
</data>
<data name="Emails_ForgotPassword_Subject" xml:space="preserve">
<value>[{0}] Please reset your password.</value>
Expand All @@ -255,12 +255,12 @@ The {2} Team</value>
<value>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</value>
The {1} Team</value>
</data>
<data name="Emails_SetPassword_Subject" xml:space="preserve">
<value>[{0}] Please set your password.</value>
Expand Down
10 changes: 5 additions & 5 deletions tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,15 +174,15 @@ public async Task SendsEmailWithPasswordResetUrl()
{
EmailAddress = "some@example.com",
PasswordResetToken = "confirmation",
PasswordResetTokenExpirationDate = DateTime.UtcNow.AddDays(1)
PasswordResetTokenExpirationDate = DateTime.UtcNow.AddHours(Constants.PasswordResetTokenExpirationHours)
};
GetMock<IMessageService>()
.Setup(s => s.SendPasswordResetInstructions(user, resetUrl, true));
GetMock<IUserService>()
.Setup(s => s.FindByEmailAddress("user"))
.Returns(user);
GetMock<AuthenticationService>()
.Setup(s => s.GeneratePasswordResetToken("user", 1440))
.Setup(s => s.GeneratePasswordResetToken("user", Constants.PasswordResetTokenExpirationHours * 60))
.CompletesWith(user);
var controller = GetController<UsersController>();
var model = new ForgotPasswordViewModel { Email = "user" };
Expand All @@ -198,7 +198,7 @@ public async Task RedirectsAfterGeneratingToken()
{
var user = new User { EmailAddress = "some@example.com", Username = "somebody" };
GetMock<AuthenticationService>()
.Setup(s => s.GeneratePasswordResetToken("user", 1440))
.Setup(s => s.GeneratePasswordResetToken("user", Constants.PasswordResetTokenExpirationHours * 60))
.CompletesWith(user)
.Verifiable();
var controller = GetController<UsersController>();
Expand All @@ -209,14 +209,14 @@ public async Task RedirectsAfterGeneratingToken()

Assert.NotNull(result);
GetMock<AuthenticationService>()
.Verify(s => s.GeneratePasswordResetToken("user", 1440));
.Verify(s => s.GeneratePasswordResetToken("user", Constants.PasswordResetTokenExpirationHours * 60));
}

[Fact]
public async Task ReturnsSameViewIfTokenGenerationFails()
{
GetMock<AuthenticationService>()
.Setup(s => s.GeneratePasswordResetToken("user", 1440))
.Setup(s => s.GeneratePasswordResetToken("user", Constants.PasswordResetTokenExpirationHours * 60))
.CompletesWithNull();
var controller = GetController<UsersController>();

Expand Down