Skip to content

Commit 4aca8e7

Browse files
authored
fix: add JSON auth/updatePassword endpoint for expired passwords [DHIS2-21120] (2.43) (#24525)
1 parent 8c5821e commit 4aca8e7

8 files changed

Lines changed: 537 additions & 1 deletion

File tree

dhis-2/dhis-api/src/main/java/org/hisp/dhis/user/UserAccountService.java

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import org.hisp.dhis.common.auth.UserInviteParams;
3636
import org.hisp.dhis.common.auth.UserRegistrationParams;
3737
import org.hisp.dhis.feedback.BadRequestException;
38+
import org.hisp.dhis.feedback.ForbiddenException;
3839

3940
/**
4041
* Service that handles user account activities, e.g. create/update account. The Validation can be
@@ -83,4 +84,25 @@ void validateUserRegistration(RegistrationParams params, String remoteIpAddress)
8384
*/
8485
void validateInvitedUser(RegistrationParams params, String remoteIpAddress)
8586
throws BadRequestException, IOException;
87+
88+
/**
89+
* Self-service change of an <b>expired</b> password. The caller is not logged in, so the method
90+
* is self-guarding: it only proceeds for an expired account whose current password is supplied
91+
* correctly. It deliberately does not establish a session; once the password is changed the
92+
* account is no longer expired, and the caller logs in through the regular login endpoint.
93+
*
94+
* <p>The current-password check runs <b>before</b> the expiry check, so "Account is not expired"
95+
* can only be observed by a caller who already knows the password (no username enumeration), and
96+
* repeated attempts against one account are throttled via the shared account-recovery lockout
97+
* (mirrors the forgot-password flow).
98+
*
99+
* @param username username identifying the account
100+
* @param oldPassword the current (expired) password
101+
* @param newPassword new password, subject to the password policy
102+
* @throws BadRequestException when input is missing, credentials are wrong, the account is not
103+
* expired, or the new password is not acceptable
104+
* @throws ForbiddenException when the account is temporarily locked due to too many attempts
105+
*/
106+
void updateExpiredPassword(String username, String oldPassword, String newPassword)
107+
throws BadRequestException, ForbiddenException;
86108
}

dhis-2/dhis-services/dhis-service-core/src/main/java/org/hisp/dhis/user/DefaultUserAccountService.java

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,15 @@
3737
import java.util.Collection;
3838
import lombok.RequiredArgsConstructor;
3939
import lombok.extern.slf4j.Slf4j;
40+
import org.apache.commons.lang3.StringUtils;
4041
import org.hisp.dhis.common.auth.RegistrationParams;
4142
import org.hisp.dhis.common.auth.UserInviteParams;
4243
import org.hisp.dhis.common.auth.UserRegistrationParams;
4344
import org.hisp.dhis.configuration.ConfigurationService;
4445
import org.hisp.dhis.feedback.BadRequestException;
46+
import org.hisp.dhis.feedback.ForbiddenException;
4547
import org.hisp.dhis.organisationunit.OrganisationUnit;
48+
import org.hisp.dhis.security.PasswordManager;
4649
import org.hisp.dhis.security.spring2fa.TwoFactorAuthenticationProvider;
4750
import org.hisp.dhis.security.spring2fa.TwoFactorWebAuthenticationDetails;
4851
import org.hisp.dhis.setting.SystemSettingsProvider;
@@ -62,11 +65,20 @@
6265
@RequiredArgsConstructor
6366
public class DefaultUserAccountService implements UserAccountService {
6467

68+
/**
69+
* Valid bcrypt hash (of a random string, same cost as the configured encoder) used to spend one
70+
* password verification on the unknown-username path of {@link #updateExpiredPassword}, so that
71+
* unknown and known usernames respond in similar time (no enumeration via timing).
72+
*/
73+
private static final String TIMING_EQUALIZATION_HASH =
74+
"$2a$10$4TXauPu06PhCTK8Up3oHi.0Y7SXLeu8ISJ6jq1GYpaaQOsSL5FOxG"; // NOSONAR not a secret
75+
6576
private final UserService userService;
6677
private final ConfigurationService configService;
6778
private final TwoFactorAuthenticationProvider twoFactorAuthProvider;
6879
private final SystemSettingsProvider settingsProvider;
6980
private final PasswordValidationService passwordValidationService;
81+
private final PasswordManager passwordManager;
7082

7183
@Override
7284
public void validateUserRegistration(RegistrationParams params, String remoteAddress)
@@ -134,6 +146,85 @@ public void confirmUserInvite(UserInviteParams params, HttpServletRequest reques
134146
authenticate(user.getUsername(), params.getPassword(), user.getAuthorities(), request);
135147
}
136148

149+
@Override
150+
@Transactional
151+
public void updateExpiredPassword(String username, String oldPassword, String newPassword)
152+
throws BadRequestException, ForbiddenException {
153+
if (StringUtils.isBlank(username)
154+
|| StringUtils.isBlank(oldPassword)
155+
|| StringUtils.isBlank(newPassword)) {
156+
throw new BadRequestException("Username, old password and new password are required");
157+
}
158+
159+
User user = userService.getUserByUsername(username);
160+
if (user == null) {
161+
// Generic message on purpose: do not reveal whether the username exists. Spend one password
162+
// verification (result deliberately ignored) so this path takes as long as the known-user
163+
// path below and the timing does not reveal it either.
164+
passwordManager.matches(oldPassword, TIMING_EQUALIZATION_HASH);
165+
throw new BadRequestException("Invalid username or password");
166+
}
167+
168+
// Throttle repeated attempts against a single account (brute-force protection), reusing the
169+
// account-recovery lockout. Registers an attempt and rejects once the threshold is exceeded.
170+
checkRecoveryLock(user.getUsername());
171+
172+
// Caller must know the current (expired) password. Checked before the expiry guard below so
173+
// that "Account is not expired" can only be observed by someone who knows the password.
174+
if (!passwordManager.matches(oldPassword, user.getPassword())) {
175+
throw new BadRequestException("Invalid username or password");
176+
}
177+
178+
// This self-service path is ONLY for expired accounts.
179+
if (userService.userNonExpired(user)) {
180+
throw new BadRequestException("Account is not expired");
181+
}
182+
183+
// The old password was verified against the stored hash above, so plain equality is enough.
184+
if (newPassword.equals(oldPassword)) {
185+
throw new BadRequestException("New password must be different from the old password");
186+
}
187+
188+
validateNewPassword(user, newPassword);
189+
190+
userService.encodeAndSetPassword(user, newPassword);
191+
userService.updateUser(user, new SystemUser());
192+
193+
log.info("Expired password updated for user: {}", user.getUsername());
194+
}
195+
196+
/** Rejects a new password that is equal to the username or fails the password policy. */
197+
private void validateNewPassword(User user, String newPassword) throws BadRequestException {
198+
if (newPassword.trim().equals(user.getUsername().trim())) {
199+
throw new BadRequestException("Password cannot be equal to username");
200+
}
201+
202+
PasswordValidationResult result =
203+
passwordValidationService.validate(
204+
CredentialsInfo.builder()
205+
.username(user.getUsername())
206+
.password(newPassword)
207+
.email(StringUtils.trimToEmpty(user.getEmail()))
208+
.newUser(false)
209+
.build());
210+
if (!result.isValid()) {
211+
throw new BadRequestException(result.getErrorMessage());
212+
}
213+
}
214+
215+
/** Registers a recovery attempt and rejects once the account has exceeded the allowed rate. */
216+
private void checkRecoveryLock(String username) throws ForbiddenException {
217+
if (userService.isRecoveryLocked(username)) {
218+
throw new ForbiddenException(
219+
"The account recovery operation for the given user is temporarily locked due to too "
220+
+ "many calls to this endpoint in the last '"
221+
+ UserConstants.RECOVERY_LOCKOUT_MINS
222+
+ "' minutes. Username:"
223+
+ username);
224+
}
225+
userService.registerRecoveryAttempt(username);
226+
}
227+
137228
private User validateRestoreLinkAndToken(UserInviteParams params) throws BadRequestException {
138229
String[] idAndRestoreToken = userService.decodeEncodedTokens(params.getToken());
139230
String idToken = idAndRestoreToken[0];

dhis-2/dhis-services/dhis-service-core/src/test/java/org/hisp/dhis/user/UserAccountServiceTest.java

Lines changed: 120 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,11 @@
3131

3232
import static org.junit.jupiter.api.Assertions.assertEquals;
3333
import static org.junit.jupiter.api.Assertions.assertThrows;
34+
import static org.mockito.ArgumentMatchers.any;
35+
import static org.mockito.ArgumentMatchers.anyString;
36+
import static org.mockito.ArgumentMatchers.eq;
37+
import static org.mockito.Mockito.never;
38+
import static org.mockito.Mockito.verify;
3439
import static org.mockito.Mockito.when;
3540

3641
import java.io.IOException;
@@ -40,6 +45,8 @@
4045
import org.hisp.dhis.common.auth.UserRegistrationParams;
4146
import org.hisp.dhis.configuration.ConfigurationService;
4247
import org.hisp.dhis.feedback.BadRequestException;
48+
import org.hisp.dhis.feedback.ForbiddenException;
49+
import org.hisp.dhis.security.PasswordManager;
4350
import org.hisp.dhis.security.spring2fa.TwoFactorAuthenticationProvider;
4451
import org.hisp.dhis.setting.SystemSettings;
4552
import org.hisp.dhis.setting.SystemSettingsService;
@@ -59,6 +66,7 @@ class UserAccountServiceTest {
5966
@Mock private TwoFactorAuthenticationProvider twoFactorAuthProvider;
6067
@Mock private SystemSettingsService settingsService;
6168
@Mock private PasswordValidationService passwordValidationService;
69+
@Mock private PasswordManager passwordManager;
6270

6371
@BeforeEach
6472
public void init() {
@@ -68,7 +76,8 @@ public void init() {
6876
configService,
6977
twoFactorAuthProvider,
7078
settingsService,
71-
passwordValidationService);
79+
passwordValidationService,
80+
passwordManager);
7281
}
7382

7483
@Test
@@ -95,6 +104,116 @@ void failedRecaptchaResponseUserRegTest() throws IOException {
95104
"Recaptcha validation failed: [invalid challenge received]", exception.getMessage());
96105
}
97106

107+
@Test
108+
@DisplayName("updateExpiredPassword with unknown username is generic and burns one verification")
109+
void updateExpiredPasswordUnknownUserTest() {
110+
when(userService.getUserByUsername("ghost")).thenReturn(null);
111+
112+
BadRequestException exception =
113+
assertThrows(
114+
BadRequestException.class,
115+
() -> userAccountService.updateExpiredPassword("ghost", "Old_pw1!", "New_pw1!"));
116+
117+
assertEquals("Invalid username or password", exception.getMessage());
118+
// The timing-equalization verification must run, so unknown and known usernames respond in
119+
// similar time; no recovery attempt is registered for a nonexistent account.
120+
verify(passwordManager).matches(eq("Old_pw1!"), anyString());
121+
verify(userService, never()).registerRecoveryAttempt(anyString());
122+
}
123+
124+
@Test
125+
@DisplayName("updateExpiredPassword rejects a recovery-locked account before checking passwords")
126+
void updateExpiredPasswordLockedTest() {
127+
User user = expiredPasswordUser();
128+
when(userService.getUserByUsername("mia")).thenReturn(user);
129+
when(userService.isRecoveryLocked("mia")).thenReturn(true);
130+
131+
assertThrows(
132+
ForbiddenException.class,
133+
() -> userAccountService.updateExpiredPassword("mia", "Old_pw1!", "New_pw1!"));
134+
135+
verify(userService, never()).registerRecoveryAttempt(anyString());
136+
verify(passwordManager, never()).matches(anyString(), anyString());
137+
}
138+
139+
@Test
140+
@DisplayName("updateExpiredPassword with wrong old password is generic and registers an attempt")
141+
void updateExpiredPasswordWrongOldPasswordTest() {
142+
User user = expiredPasswordUser();
143+
when(userService.getUserByUsername("mia")).thenReturn(user);
144+
when(userService.isRecoveryLocked("mia")).thenReturn(false);
145+
when(passwordManager.matches("Wrong_pw1!", "encoded-old")).thenReturn(false);
146+
147+
BadRequestException exception =
148+
assertThrows(
149+
BadRequestException.class,
150+
() -> userAccountService.updateExpiredPassword("mia", "Wrong_pw1!", "New_pw1!"));
151+
152+
assertEquals("Invalid username or password", exception.getMessage());
153+
verify(userService).registerRecoveryAttempt("mia");
154+
// Guard order: expiry state must not be consulted before the password is verified, so
155+
// "Account is not expired" is only observable by a caller who knows the password.
156+
verify(userService, never()).userNonExpired(any(User.class));
157+
}
158+
159+
@Test
160+
@DisplayName("updateExpiredPassword rejects a non-expired account")
161+
void updateExpiredPasswordNonExpiredTest() {
162+
User user = expiredPasswordUser();
163+
when(userService.getUserByUsername("mia")).thenReturn(user);
164+
when(userService.isRecoveryLocked("mia")).thenReturn(false);
165+
when(passwordManager.matches("Old_pw1!", "encoded-old")).thenReturn(true);
166+
when(userService.userNonExpired(user)).thenReturn(true);
167+
168+
BadRequestException exception =
169+
assertThrows(
170+
BadRequestException.class,
171+
() -> userAccountService.updateExpiredPassword("mia", "Old_pw1!", "New_pw1!"));
172+
173+
assertEquals("Account is not expired", exception.getMessage());
174+
}
175+
176+
@Test
177+
@DisplayName("updateExpiredPassword rejects a new password equal to the old one")
178+
void updateExpiredPasswordSameAsOldTest() {
179+
User user = expiredPasswordUser();
180+
when(userService.getUserByUsername("mia")).thenReturn(user);
181+
when(userService.isRecoveryLocked("mia")).thenReturn(false);
182+
when(passwordManager.matches("Old_pw1!", "encoded-old")).thenReturn(true);
183+
when(userService.userNonExpired(user)).thenReturn(false);
184+
185+
BadRequestException exception =
186+
assertThrows(
187+
BadRequestException.class,
188+
() -> userAccountService.updateExpiredPassword("mia", "Old_pw1!", "Old_pw1!"));
189+
190+
assertEquals("New password must be different from the old password", exception.getMessage());
191+
}
192+
193+
@Test
194+
@DisplayName("updateExpiredPassword sets and persists a valid new password")
195+
void updateExpiredPasswordOkTest() throws BadRequestException, ForbiddenException {
196+
User user = expiredPasswordUser();
197+
when(userService.getUserByUsername("mia")).thenReturn(user);
198+
when(userService.isRecoveryLocked("mia")).thenReturn(false);
199+
when(passwordManager.matches("Old_pw1!", "encoded-old")).thenReturn(true);
200+
when(userService.userNonExpired(user)).thenReturn(false);
201+
when(passwordValidationService.validate(any(CredentialsInfo.class)))
202+
.thenReturn(PasswordValidationResult.VALID);
203+
204+
userAccountService.updateExpiredPassword("mia", "Old_pw1!", "New_pw1!");
205+
206+
verify(userService).encodeAndSetPassword(user, "New_pw1!");
207+
verify(userService).updateUser(eq(user), any(SystemUser.class));
208+
}
209+
210+
private User expiredPasswordUser() {
211+
User user = new User();
212+
user.setUsername("mia");
213+
user.setPassword("encoded-old");
214+
return user;
215+
}
216+
98217
@Test
99218
@DisplayName("Failed recaptcha response during user invite throws an exception")
100219
void failedRecaptchaResponseUserInviteTest() throws IOException {

0 commit comments

Comments
 (0)