Skip to content

Issue a new password reset token on each request #14655

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

Open
MitchDrage wants to merge 1 commit into
base: 4.22
Choose a base branch
from
Open

Issue a new password reset token on each request #14655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions engine/schema/src/main/java/org/apache/cloudstack/resourcedetail/UserDetailVO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ public class UserDetailVO implements ResourceDetail {
public static final String Setup2FADetail = "2FASetupStatus";
public static final String PasswordResetToken = "PasswordResetToken";
public static final String PasswordResetTokenExpiryDate = "PasswordResetTokenExpiryDate";
public static final String PasswordResetTokenIssuedDate = "PasswordResetTokenIssuedDate";

public UserDetailVO() {
}
Expand Down
6 changes: 6 additions & 0 deletions server/src/main/java/org/apache/cloudstack/user/UserPasswordResetManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ public interface UserPasswordResetManager {
"TTL in minutes for the token generated to reset the ACS user's password", true,
ConfigKey.Scope.Global);

ConfigKey<Long> UserPasswordResetEmailCooldown = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED, Long.class,
"user.password.reset.email.cooldown", "300",
"Minimum time in seconds between password reset emails for the same user. " +
"Requests within this time send no email. Set to 0 to disable.", true,
ConfigKey.Scope.Global);

ConfigKey<String> UserPasswordResetEmailSender = new ConfigKey<>(ConfigKey.CATEGORY_ADVANCED,
String.class, "user.password.reset.email.sender", null,
"Sender for emails sent to the user to reset ACS user's password ", true,
Expand Down
87 changes: 61 additions & 26 deletions server/src/main/java/org/apache/cloudstack/user/UserPasswordResetManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
import static org.apache.cloudstack.config.ApiServiceConfiguration.ManagementServerAddresses;
import static org.apache.cloudstack.resourcedetail.UserDetailVO.PasswordResetToken;
import static org.apache.cloudstack.resourcedetail.UserDetailVO.PasswordResetTokenExpiryDate;
import static org.apache.cloudstack.resourcedetail.UserDetailVO.PasswordResetTokenIssuedDate;

public class UserPasswordResetManagerImpl extends ManagerBase implements UserPasswordResetManager, Configurable {

Expand Down Expand Up @@ -90,6 +91,7 @@ public ConfigKey<?>[] getConfigKeys() {
return new ConfigKey<?>[]{
UserPasswordResetEnabled,
UserPasswordResetTtl,
UserPasswordResetEmailCooldown,
UserPasswordResetEmailSender,
UserPasswordResetSMTPHost,
UserPasswordResetSMTPPort,
Expand Down Expand Up @@ -136,24 +138,39 @@ private String getKey(String namespace, String config) {
}


protected boolean validateExistingToken(UserAccount userAccount) {

Map<String, String> details = userDetailsDao.listDetailsKeyPairs(userAccount.getId());

String resetToken = details.get(PasswordResetToken);
String resetTokenExpiryTimeString = details.getOrDefault(PasswordResetTokenExpiryDate, "0");
/**
* Removes every row for the detail before writing it. addDetail only replaces one row, so
* duplicates left by parallel requests would otherwise persist and break every later link.
*/
private void replaceDetail(UserAccount userAccount, String name, String value) {
userDetailsDao.removeDetail(userAccount.getId(), name);
userDetailsDao.addDetail(userAccount.getId(), name, value, false);
}

protected long getEmailCooldownSeconds() {
return UserPasswordResetEmailCooldown.value();
}

if (StringUtils.isNotEmpty(resetToken) && StringUtils.isNotEmpty(resetTokenExpiryTimeString)) {
final Date resetTokenExpiryTime = new Date(Long.parseLong(resetTokenExpiryTimeString));
final Date currentTime = new Date();
if (currentTime.after(resetTokenExpiryTime)) {
return true;
}
} else if (StringUtils.isEmpty(resetToken)) {
return true;
protected boolean isWithinEmailCooldown(UserAccount userAccount) {
long cooldown = getEmailCooldownSeconds();
if (cooldown <= 0) {
return false;
}
UserDetailVO issuedDate = userDetailsDao.findDetail(userAccount.getId(), PasswordResetTokenIssuedDate);
if (issuedDate == null || StringUtils.isEmpty(issuedDate.getValue())) {
return false;
}
long issuedAt;
try {
issuedAt = Long.parseLong(issuedDate.getValue());
} catch (NumberFormatException e) {
logger.warn("Ignoring invalid password reset issue time [{}] for user {}", issuedDate.getValue(), userAccount);
return false;
}
return false;
// A negative value means the issue time is in the future, from clock skew between
// management servers. Ignore it so the user is not blocked.
long elapsed = System.currentTimeMillis() - issuedAt;
return elapsed >= 0 && elapsed / 1000 < cooldown;
}

public void setResetTokenAndSend(UserAccount userAccount) {
Expand All @@ -163,20 +180,27 @@ public void setResetTokenAndSend(UserAccount userAccount) {
"Failed to reset token and send email. SMTP mail sender is not configured");
}

if (!validateExistingToken(userAccount)) {
logger.debug(String.format(
"Failed to reset token and send email. Password reset token is already set for user %s in " +
"domain id: %s with account %s and email %s",
userAccount.getUsername(), userAccount.getDomainId(),
userAccount.getAccountName(), userAccount.getEmail()));
if (isWithinEmailCooldown(userAccount)) {
logger.info("Password reset email not sent for user {} account id: {} domain id: {}. " +
"A reset was requested less than {} seconds ago.",
userAccount, userAccount.getAccountId(), userAccount.getDomainId(),
getEmailCooldownSeconds());
return;
}

final String resetToken = UUID.randomUUID().toString();
final Date resetTokenExpiryTime = new Date(System.currentTimeMillis() + UserPasswordResetTtl.value() * 60 * 1000);
// Claim the cooldown before sending, so a request arriving during the send is caught.
// Truly parallel requests can still both pass the check; the cooldown is best effort.
final long now = System.currentTimeMillis();
replaceDetail(userAccount, PasswordResetTokenIssuedDate, String.valueOf(now));

// The claim is kept even if the send fails, so a mail outage cannot be used to
// bypass the cooldown. The previous link still works because no new token is saved.
issueTokenAndSend(userAccount, now);
}

userDetailsDao.addDetail(userAccount.getId(), PasswordResetToken, resetToken, false);
userDetailsDao.addDetail(userAccount.getId(), PasswordResetTokenExpiryDate, String.valueOf(resetTokenExpiryTime.getTime()), false);
private void issueTokenAndSend(UserAccount userAccount, long now) {
final String resetToken = UUID.randomUUID().toString();
final Date resetTokenExpiryTime = new Date(now + UserPasswordResetTtl.value() * 60 * 1000);

final String email = userAccount.getEmail();
final String username = userAccount.getUsername();
Expand Down Expand Up @@ -216,7 +240,17 @@ public void setResetTokenAndSend(UserAccount userAccount) {

mailProperties.setRecipients(addresses);

mailSender.sendMail(mailProperties);
// Save the token only once the email is sent. If the send fails, the previous
// link keeps working and the user can retry without waiting for the cooldown.
if (!mailSender.trySendMail(mailProperties)) {
logger.error("Failed to send password reset email for user {} account id: {} domain id: {}. The existing token was kept.",
userAccount, userAccount.getAccountId(), userAccount.getDomainId());
return;
}

// The new token replaces any existing one, so only the latest email's link works.
replaceDetail(userAccount, PasswordResetToken, resetToken);
replaceDetail(userAccount, PasswordResetTokenExpiryDate, String.valueOf(resetTokenExpiryTime.getTime()));
logger.debug("User password reset email for user {} account id: {} domain id: {} sent to {} with token expiry at {}",
userAccount, userAccount.getAccountId(), userAccount.getDomainId(), email, resetTokenExpiryTime);
}
Expand Down Expand Up @@ -265,6 +299,7 @@ void resetPassword(UserAccount userAccount, String password) {

userDetailsDao.removeDetail(userAccount.getId(), PasswordResetToken);
userDetailsDao.removeDetail(userAccount.getId(), PasswordResetTokenExpiryDate);
userDetailsDao.removeDetail(userAccount.getId(), PasswordResetTokenIssuedDate);

userDao.persist(user);
}
Expand Down
160 changes: 141 additions & 19 deletions server/src/test/java/org/apache/cloudstack/user/UserPasswordResetManagerImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,25 +16,28 @@
// under the License.
package org.apache.cloudstack.user;

import com.cloud.user.AccountManager;
import com.cloud.user.UserAccount;
import com.cloud.user.UserVO;
import com.cloud.user.dao.UserDao;
import org.apache.cloudstack.api.ServerApiException;
import org.apache.cloudstack.framework.config.ConfigKey;
import org.apache.cloudstack.resourcedetail.UserDetailVO;
import org.apache.cloudstack.resourcedetail.dao.UserDetailsDao;
import org.apache.cloudstack.utils.mailing.SMTPMailSender;
import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.InOrder;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.Spy;
import org.mockito.junit.MockitoJUnitRunner;

import java.util.Collections;
import java.util.Map;

import static org.apache.cloudstack.resourcedetail.UserDetailVO.PasswordResetToken;
import static org.apache.cloudstack.resourcedetail.UserDetailVO.PasswordResetTokenExpiryDate;
import static org.apache.cloudstack.resourcedetail.UserDetailVO.PasswordResetTokenIssuedDate;

@RunWith(MockitoJUnitRunner.class)
public class UserPasswordResetManagerImplTest {
Expand All @@ -45,6 +48,15 @@ public class UserPasswordResetManagerImplTest {
@Mock
private UserDetailsDao userDetailsDao;

@Mock
private SMTPMailSender mailSender;

@Mock
private UserDao userDao;

@Mock
private AccountManager accountManager;

@Test
public void testGetMessageBody() {
ConfigKey<String> passwordResetMailTemplate = Mockito.mock(ConfigKey.class);
Expand Down Expand Up @@ -116,35 +128,145 @@ public void testValidateAndResetPassword() {
Mockito.verify(passwordReset, Mockito.times(1)).resetPassword(userAccount, "new_password");
}

@Test
public void testValidateExistingTokenFirstRequest() {
private UserAccount mockUser() {
UserAccount userAccount = Mockito.mock(UserAccount.class);
Mockito.when(userAccount.getId()).thenReturn(1L);
Mockito.when(userDetailsDao.listDetailsKeyPairs(1L)).thenReturn(Collections.emptyMap());
return userAccount;
}

Assert.assertTrue(passwordReset.validateExistingToken(userAccount));
private void mockIssuedDate(String value) {
UserDetailVO issuedDate = Mockito.mock(UserDetailVO.class);
Mockito.when(issuedDate.getValue()).thenReturn(value);
Mockito.when(userDetailsDao.findDetail(1L, PasswordResetTokenIssuedDate)).thenReturn(issuedDate);
}

@Test
public void testValidateExistingTokenSecondRequestExpired() {
public void testIsWithinEmailCooldownFirstRequest() {
UserAccount userAccount = mockUser();
Mockito.doReturn(60L).when(passwordReset).getEmailCooldownSeconds();
Mockito.when(userDetailsDao.findDetail(1L, PasswordResetTokenIssuedDate)).thenReturn(null);

Assert.assertFalse(passwordReset.isWithinEmailCooldown(userAccount));
}

@Test
public void testIsWithinEmailCooldownRecentRequest() {
UserAccount userAccount = mockUser();
Mockito.doReturn(60L).when(passwordReset).getEmailCooldownSeconds();
mockIssuedDate(String.valueOf(System.currentTimeMillis() - 10 * 1000));

Assert.assertTrue(passwordReset.isWithinEmailCooldown(userAccount));
}

@Test
public void testIsWithinEmailCooldownOldRequest() {
UserAccount userAccount = mockUser();
Mockito.doReturn(60L).when(passwordReset).getEmailCooldownSeconds();
mockIssuedDate(String.valueOf(System.currentTimeMillis() - 5 * 60 * 1000));

Assert.assertFalse(passwordReset.isWithinEmailCooldown(userAccount));
}

@Test
public void testIsWithinEmailCooldownDisabled() {
UserAccount userAccount = Mockito.mock(UserAccount.class);
Mockito.when(userAccount.getId()).thenReturn(1L);
Mockito.when(userDetailsDao.listDetailsKeyPairs(1L)).thenReturn(Map.of(
PasswordResetToken, "reset_token",
PasswordResetTokenExpiryDate, String.valueOf(System.currentTimeMillis() - 5 * 60 * 1000)));
Mockito.doReturn(0L).when(passwordReset).getEmailCooldownSeconds();

Assert.assertTrue(passwordReset.validateExistingToken(userAccount));
Assert.assertFalse(passwordReset.isWithinEmailCooldown(userAccount));
Mockito.verify(userDetailsDao, Mockito.never()).findDetail(Mockito.anyLong(), Mockito.anyString());
}

@Test
public void testIsWithinEmailCooldownIssueTimeInFuture() {
UserAccount userAccount = mockUser();
Mockito.doReturn(60L).when(passwordReset).getEmailCooldownSeconds();
mockIssuedDate(String.valueOf(System.currentTimeMillis() + 2 * 60 * 1000));

Assert.assertFalse(passwordReset.isWithinEmailCooldown(userAccount));
}

@Test
public void testValidateExistingTokenSecondRequestUnexpired() {
public void testIsWithinEmailCooldownLargeValue() {
UserAccount userAccount = mockUser();
Mockito.doReturn(Long.MAX_VALUE / 100).when(passwordReset).getEmailCooldownSeconds();
mockIssuedDate(String.valueOf(System.currentTimeMillis() - 10 * 1000));

Assert.assertTrue(passwordReset.isWithinEmailCooldown(userAccount));
}

@Test
public void testIsWithinEmailCooldownInvalidIssueTime() {
UserAccount userAccount = mockUser();
Mockito.doReturn(60L).when(passwordReset).getEmailCooldownSeconds();
mockIssuedDate("not-a-number");

Assert.assertFalse(passwordReset.isWithinEmailCooldown(userAccount));
}

@Test
public void testSetResetTokenAndSendWithinCooldown() {
UserAccount userAccount = Mockito.mock(UserAccount.class);
Mockito.when(userAccount.getId()).thenReturn(1L);
Mockito.when(userDetailsDao.listDetailsKeyPairs(1L)).thenReturn(Map.of(
PasswordResetToken, "reset_token",
PasswordResetTokenExpiryDate, String.valueOf(System.currentTimeMillis() + 5 * 60 * 1000)));
Mockito.doReturn(true).when(passwordReset).isWithinEmailCooldown(userAccount);

passwordReset.setResetTokenAndSend(userAccount);

Mockito.verify(userDetailsDao, Mockito.never()).addDetail(Mockito.anyLong(), Mockito.anyString(), Mockito.anyString(), Mockito.anyBoolean());
Mockito.verify(mailSender, Mockito.never()).trySendMail(Mockito.any());
}

private UserAccount mockUserForSend() {
UserAccount userAccount = mockUser();
Mockito.when(userAccount.getUsername()).thenReturn("test_user");
Mockito.when(userAccount.getEmail()).thenReturn("[email protected]");
Mockito.doReturn(false).when(passwordReset).isWithinEmailCooldown(userAccount);
Mockito.doReturn("body").when(passwordReset).getMessageBody(Mockito.eq(userAccount), Mockito.anyString(), Mockito.anyString());
return userAccount;
}

@Test
public void testSetResetTokenAndSendIssuesNewToken() {
UserAccount userAccount = mockUserForSend();
Mockito.when(mailSender.trySendMail(Mockito.any())).thenReturn(true);

passwordReset.setResetTokenAndSend(userAccount);

// Each detail has all existing rows removed before the new row is written,
// so duplicates from parallel requests do not survive the next request.
InOrder order = Mockito.inOrder(userDetailsDao);
order.verify(userDetailsDao).removeDetail(1L, PasswordResetTokenIssuedDate);
order.verify(userDetailsDao).addDetail(Mockito.eq(1L), Mockito.eq(PasswordResetTokenIssuedDate), Mockito.anyString(), Mockito.eq(false));
order.verify(userDetailsDao).removeDetail(1L, PasswordResetToken);
order.verify(userDetailsDao).addDetail(Mockito.eq(1L), Mockito.eq(PasswordResetToken), Mockito.anyString(), Mockito.eq(false));
order.verify(userDetailsDao).removeDetail(1L, PasswordResetTokenExpiryDate);
order.verify(userDetailsDao).addDetail(Mockito.eq(1L), Mockito.eq(PasswordResetTokenExpiryDate), Mockito.anyString(), Mockito.eq(false));
Mockito.verify(userDetailsDao, Mockito.times(1)).removeDetail(1L, PasswordResetTokenIssuedDate);
}

@Test
public void testSetResetTokenAndSendFailureKeepsExistingTokenAndCooldown() {
UserAccount userAccount = mockUserForSend();
Mockito.when(mailSender.trySendMail(Mockito.any())).thenReturn(false);

passwordReset.setResetTokenAndSend(userAccount);

Mockito.verify(userDetailsDao, Mockito.never()).addDetail(Mockito.eq(1L), Mockito.eq(PasswordResetToken), Mockito.anyString(), Mockito.anyBoolean());
Mockito.verify(userDetailsDao, Mockito.never()).addDetail(Mockito.eq(1L), Mockito.eq(PasswordResetTokenExpiryDate), Mockito.anyString(), Mockito.anyBoolean());
Mockito.verify(userDetailsDao, Mockito.never()).removeDetail(1L, PasswordResetToken);
Mockito.verify(userDetailsDao).addDetail(Mockito.eq(1L), Mockito.eq(PasswordResetTokenIssuedDate), Mockito.anyString(), Mockito.eq(false));
// Removed once while claiming the cooldown, and not released after the failure.
Mockito.verify(userDetailsDao, Mockito.times(1)).removeDetail(1L, PasswordResetTokenIssuedDate);
}

@Test
public void testResetPasswordRemovesResetDetails() {
UserAccount userAccount = mockUser();
UserVO user = Mockito.mock(UserVO.class);
Mockito.when(userDao.getUser(1L)).thenReturn(user);

passwordReset.resetPassword(userAccount, "new_password");

Assert.assertFalse(passwordReset.validateExistingToken(userAccount));
Mockito.verify(userDetailsDao).removeDetail(1L, PasswordResetToken);
Mockito.verify(userDetailsDao).removeDetail(1L, PasswordResetTokenExpiryDate);
Mockito.verify(userDetailsDao).removeDetail(1L, PasswordResetTokenIssuedDate);
}
}
2 changes: 1 addition & 1 deletion ui/public/locales/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3556,7 +3556,7 @@
"message.failed.to.add": "Failed to add",
"message.failed.to.assign.vms": "Failed to assign Instances",
"message.failed.to.remove": "Failed to remove",
"message.forgot.password.success": "An email has been sent to your email address with instructions on how to reset your password.",
"message.forgot.password.success": "If the account exists and has an email address, reset instructions have been sent. Check your spam folder, and wait a few minutes before requesting another email.",
"message.generate.keys": "Please confirm that you would like to generate new API/Secret keys for this User.",
"message.chart.statistic.info": "The shown charts are self-adjustable, that means, if the value gets close to the limit or overpass it, it will grow to adjust the shown value",
"message.chart.statistic.info.hypervisor.additionals": "The metrics data depend on the hypervisor plugin used for each hypervisor. The behavior can vary across different hypervisors. For instance, with KVM, metrics are real-time statistics provided by libvirt. In contrast, with VMware, the metrics are averaged data for a given time interval controlled by configuration.",
Expand Down
Loading