Commit 228fddb8cd92bf4ec89f16760aef7f190a45bb96
Committed by
GitHub
1 parent
3712d738
[PROD-678] Authorization and password reset vulnerability fix (#4569)
* Fixed vulnerabilities for password reset and authorization * Improvements to check user and credentials for null * Correct messages and logs * Improvements * Reset Password Test: added delay after resetting password to synchronize test with server * Executor removed from controller * Correct method calling * Formatting cleaned
Showing
10 changed files
with
50 additions
and
19 deletions
... | ... | @@ -135,7 +135,7 @@ public class AuthController extends BaseController { |
135 | 135 | } |
136 | 136 | } |
137 | 137 | |
138 | - @RequestMapping(value = "/noauth/activate", params = { "activateToken" }, method = RequestMethod.GET) | |
138 | + @RequestMapping(value = "/noauth/activate", params = {"activateToken"}, method = RequestMethod.GET) | |
139 | 139 | public ResponseEntity<String> checkActivateToken( |
140 | 140 | @RequestParam(value = "activateToken") String activateToken) { |
141 | 141 | HttpHeaders headers = new HttpHeaders(); |
... | ... | @@ -159,7 +159,7 @@ public class AuthController extends BaseController { |
159 | 159 | |
160 | 160 | @RequestMapping(value = "/noauth/resetPasswordByEmail", method = RequestMethod.POST) |
161 | 161 | @ResponseStatus(value = HttpStatus.OK) |
162 | - public void requestResetPasswordByEmail ( | |
162 | + public void requestResetPasswordByEmail( | |
163 | 163 | @RequestBody JsonNode resetPasswordByEmailRequest, |
164 | 164 | HttpServletRequest request) throws ThingsboardException { |
165 | 165 | try { |
... | ... | @@ -170,13 +170,13 @@ public class AuthController extends BaseController { |
170 | 170 | String resetUrl = String.format("%s/api/noauth/resetPassword?resetToken=%s", baseUrl, |
171 | 171 | userCredentials.getResetToken()); |
172 | 172 | |
173 | - mailService.sendResetPasswordEmail(resetUrl, email); | |
173 | + mailService.sendResetPasswordEmailAsync(resetUrl, email); | |
174 | 174 | } catch (Exception e) { |
175 | - throw handleException(e); | |
175 | + log.warn("Error occurred: {}", e.getMessage()); | |
176 | 176 | } |
177 | 177 | } |
178 | 178 | |
179 | - @RequestMapping(value = "/noauth/resetPassword", params = { "resetToken" }, method = RequestMethod.GET) | |
179 | + @RequestMapping(value = "/noauth/resetPassword", params = {"resetToken"}, method = RequestMethod.GET) | |
180 | 180 | public ResponseEntity<String> checkResetToken( |
181 | 181 | @RequestParam(value = "resetToken") String resetToken) { |
182 | 182 | HttpHeaders headers = new HttpHeaders(); | ... | ... |
... | ... | @@ -25,6 +25,7 @@ import org.springframework.security.authentication.BadCredentialsException; |
25 | 25 | import org.springframework.security.authentication.DisabledException; |
26 | 26 | import org.springframework.security.authentication.LockedException; |
27 | 27 | import org.springframework.security.core.AuthenticationException; |
28 | +import org.springframework.security.core.userdetails.UsernameNotFoundException; | |
28 | 29 | import org.springframework.security.web.access.AccessDeniedHandler; |
29 | 30 | import org.springframework.web.bind.annotation.ExceptionHandler; |
30 | 31 | import org.springframework.web.bind.annotation.RestControllerAdvice; |
... | ... | @@ -152,7 +153,7 @@ public class ThingsboardErrorResponseHandler extends ResponseEntityExceptionHand |
152 | 153 | |
153 | 154 | private void handleAuthenticationException(AuthenticationException authenticationException, HttpServletResponse response) throws IOException { |
154 | 155 | response.setStatus(HttpStatus.UNAUTHORIZED.value()); |
155 | - if (authenticationException instanceof BadCredentialsException) { | |
156 | + if (authenticationException instanceof BadCredentialsException || authenticationException instanceof UsernameNotFoundException) { | |
156 | 157 | mapper.writeValue(response.getWriter(), ThingsboardErrorResponse.of("Invalid username or password", ThingsboardErrorCode.AUTHENTICATION, HttpStatus.UNAUTHORIZED)); |
157 | 158 | } else if (authenticationException instanceof DisabledException) { |
158 | 159 | mapper.writeValue(response.getWriter(), ThingsboardErrorResponse.of("User account is not active", ThingsboardErrorCode.AUTHENTICATION, HttpStatus.UNAUTHORIZED)); | ... | ... |
... | ... | @@ -73,6 +73,9 @@ public class DefaultMailService implements MailService { |
73 | 73 | @Autowired |
74 | 74 | private TbApiUsageStateService apiUsageStateService; |
75 | 75 | |
76 | + @Autowired | |
77 | + private MailExecutorService mailExecutorService; | |
78 | + | |
76 | 79 | private JavaMailSenderImpl mailSender; |
77 | 80 | |
78 | 81 | private String mailFrom; |
... | ... | @@ -222,6 +225,17 @@ public class DefaultMailService implements MailService { |
222 | 225 | } |
223 | 226 | |
224 | 227 | @Override |
228 | + public void sendResetPasswordEmailAsync(String passwordResetLink, String email) { | |
229 | + mailExecutorService.execute(() -> { | |
230 | + try { | |
231 | + this.sendResetPasswordEmail(passwordResetLink, email); | |
232 | + } catch (ThingsboardException e) { | |
233 | + log.error("Error occurred: {} ", e.getMessage()); | |
234 | + } | |
235 | + }); | |
236 | + } | |
237 | + | |
238 | + @Override | |
225 | 239 | public void sendPasswordWasResetEmail(String loginLink, String email) throws ThingsboardException { |
226 | 240 | |
227 | 241 | String subject = messages.getMessage("password.was.reset.subject", null, Locale.US); | ... | ... |
... | ... | @@ -155,6 +155,7 @@ public abstract class BaseUserControllerTest extends AbstractControllerTest { |
155 | 155 | |
156 | 156 | doPost("/api/noauth/resetPasswordByEmail", resetPasswordByEmailRequest) |
157 | 157 | .andExpect(status().isOk()); |
158 | + Thread.sleep(1000); | |
158 | 159 | doGet("/api/noauth/resetPassword?resetToken={resetToken}", TestMailService.currentResetPasswordToken) |
159 | 160 | .andExpect(status().isSeeOther()) |
160 | 161 | .andExpect(header().string(HttpHeaders.LOCATION, "/login/resetPassword?resetToken=" + TestMailService.currentResetPasswordToken)); | ... | ... |
... | ... | @@ -51,7 +51,7 @@ public class TestMailService { |
51 | 51 | currentResetPasswordToken = passwordResetLink.split("=")[1]; |
52 | 52 | return null; |
53 | 53 | } |
54 | - }).when(mailService).sendResetPasswordEmail(Mockito.anyString(), Mockito.anyString()); | |
54 | + }).when(mailService).sendResetPasswordEmailAsync(Mockito.anyString(), Mockito.anyString()); | |
55 | 55 | return mailService; |
56 | 56 | } |
57 | 57 | ... | ... |
... | ... | @@ -25,7 +25,10 @@ import org.apache.commons.lang3.StringUtils; |
25 | 25 | import org.springframework.beans.factory.annotation.Value; |
26 | 26 | import org.springframework.context.ApplicationEventPublisher; |
27 | 27 | import org.springframework.context.annotation.Lazy; |
28 | +import org.springframework.security.authentication.DisabledException; | |
29 | +import org.springframework.security.core.userdetails.UsernameNotFoundException; | |
28 | 30 | import org.springframework.stereotype.Service; |
31 | +import org.thingsboard.common.util.JacksonUtil; | |
29 | 32 | import org.thingsboard.server.common.data.Customer; |
30 | 33 | import org.thingsboard.server.common.data.EntityType; |
31 | 34 | import org.thingsboard.server.common.data.Tenant; |
... | ... | @@ -49,7 +52,6 @@ import org.thingsboard.server.dao.service.DataValidator; |
49 | 52 | import org.thingsboard.server.dao.service.PaginatedRemover; |
50 | 53 | import org.thingsboard.server.dao.tenant.TbTenantProfileCache; |
51 | 54 | import org.thingsboard.server.dao.tenant.TenantDao; |
52 | -import org.thingsboard.common.util.JacksonUtil; | |
53 | 55 | |
54 | 56 | import java.util.HashMap; |
55 | 57 | import java.util.Map; |
... | ... | @@ -194,11 +196,11 @@ public class UserServiceImpl extends AbstractEntityService implements UserServic |
194 | 196 | DataValidator.validateEmail(email); |
195 | 197 | User user = userDao.findByEmail(tenantId, email); |
196 | 198 | if (user == null) { |
197 | - throw new IncorrectParameterException(String.format("Unable to find user by email [%s]", email)); | |
199 | + throw new UsernameNotFoundException(String.format("Unable to find user by email [%s]", email)); | |
198 | 200 | } |
199 | 201 | UserCredentials userCredentials = userCredentialsDao.findByUserId(tenantId, user.getUuidId()); |
200 | 202 | if (!userCredentials.isEnabled()) { |
201 | - throw new IncorrectParameterException("Unable to reset password for inactive user"); | |
203 | + throw new DisabledException(String.format("User credentials not enabled [%s]", email)); | |
202 | 204 | } |
203 | 205 | userCredentials.setResetToken(RandomStringUtils.randomAlphanumeric(DEFAULT_TOKEN_LENGTH)); |
204 | 206 | return saveUserCredentials(tenantId, userCredentials); |
... | ... | @@ -365,7 +367,8 @@ public class UserServiceImpl extends AbstractEntityService implements UserServic |
365 | 367 | JsonNode userPasswordHistoryJson; |
366 | 368 | if (additionalInfo.has(USER_PASSWORD_HISTORY)) { |
367 | 369 | userPasswordHistoryJson = additionalInfo.get(USER_PASSWORD_HISTORY); |
368 | - userPasswordHistoryMap = JacksonUtil.convertValue(userPasswordHistoryJson, new TypeReference<>(){}); | |
370 | + userPasswordHistoryMap = JacksonUtil.convertValue(userPasswordHistoryJson, new TypeReference<>() { | |
371 | + }); | |
369 | 372 | } |
370 | 373 | if (userPasswordHistoryMap != null) { |
371 | 374 | userPasswordHistoryMap.put(Long.toString(System.currentTimeMillis()), userCredentials.getPassword()); | ... | ... |
... | ... | @@ -31,22 +31,25 @@ public interface MailService { |
31 | 31 | void updateMailConfiguration(); |
32 | 32 | |
33 | 33 | void sendEmail(TenantId tenantId, String email, String subject, String message) throws ThingsboardException; |
34 | - | |
34 | + | |
35 | 35 | void sendTestMail(JsonNode config, String email) throws ThingsboardException; |
36 | - | |
36 | + | |
37 | 37 | void sendActivationEmail(String activationLink, String email) throws ThingsboardException; |
38 | - | |
38 | + | |
39 | 39 | void sendAccountActivatedEmail(String loginLink, String email) throws ThingsboardException; |
40 | - | |
40 | + | |
41 | 41 | void sendResetPasswordEmail(String passwordResetLink, String email) throws ThingsboardException; |
42 | 42 | |
43 | + void sendResetPasswordEmailAsync(String passwordResetLink, String email); | |
44 | + | |
43 | 45 | void sendPasswordWasResetEmail(String loginLink, String email) throws ThingsboardException; |
44 | 46 | |
45 | - void sendAccountLockoutEmail( String lockoutEmail, String email, Integer maxFailedLoginAttempts) throws ThingsboardException; | |
47 | + void sendAccountLockoutEmail(String lockoutEmail, String email, Integer maxFailedLoginAttempts) throws ThingsboardException; | |
46 | 48 | |
47 | 49 | void send(TenantId tenantId, CustomerId customerId, String from, String to, String cc, String bcc, String subject, String body, boolean isHtml, Map<String, String> images) throws ThingsboardException; |
48 | 50 | |
49 | 51 | void send(TenantId tenantId, CustomerId customerId, String from, String to, String cc, String bcc, String subject, String body, boolean isHtml, Map<String, String> images, JavaMailSender javaMailSender) throws ThingsboardException; |
50 | 52 | |
51 | 53 | void sendApiFeatureStateEmail(ApiFeature apiFeature, ApiUsageStateValue stateValue, String email, ApiUsageStateMailMessage msg) throws ThingsboardException; |
54 | + | |
52 | 55 | } | ... | ... |
... | ... | @@ -15,7 +15,8 @@ |
15 | 15 | limitations under the License. |
16 | 16 | |
17 | 17 | --> |
18 | -<div class="tb-request-password-reset-content mat-app-background tb-dark" fxLayout="row" fxLayoutAlign="center center" style="width: 100%;"> | |
18 | +<div class="tb-request-password-reset-content mat-app-background tb-dark" fxLayout="row" fxLayoutAlign="center center" | |
19 | + style="width: 100%;"> | |
19 | 20 | <mat-card fxFlex="initial" class="tb-request-password-reset-card"> |
20 | 21 | <mat-card-title class="layout-padding"> |
21 | 22 | <span translate class="mat-headline">login.request-password-reset</span> |
... | ... | @@ -38,7 +39,7 @@ |
38 | 39 | </mat-form-field> |
39 | 40 | <div fxLayout="column" fxLayout.gt-xs="row" fxLayoutGap="16px" fxLayoutAlign="start center" |
40 | 41 | fxLayoutAlign.gt-xs="center start"> |
41 | - <button mat-raised-button color="accent" type="submit" [disabled]="(isLoading$ | async)"> | |
42 | + <button mat-raised-button color="accent" type="submit" [disabled]="(isLoading$ | async) || this.clicked"> | |
42 | 43 | {{ 'login.request-password-reset' | translate }} |
43 | 44 | </button> |
44 | 45 | <button mat-raised-button color="primary" type="button" [disabled]="(isLoading$ | async)" | ... | ... |
... | ... | @@ -30,6 +30,8 @@ import { TranslateService } from '@ngx-translate/core'; |
30 | 30 | }) |
31 | 31 | export class ResetPasswordRequestComponent extends PageComponent implements OnInit { |
32 | 32 | |
33 | + clicked: boolean = false; | |
34 | + | |
33 | 35 | requestPasswordRequest = this.fb.group({ |
34 | 36 | email: ['', [Validators.email, Validators.required]] |
35 | 37 | }, {updateOn: 'submit'}); |
... | ... | @@ -44,8 +46,14 @@ export class ResetPasswordRequestComponent extends PageComponent implements OnIn |
44 | 46 | ngOnInit() { |
45 | 47 | } |
46 | 48 | |
49 | + disableInputs() { | |
50 | + this.requestPasswordRequest.disable(); | |
51 | + this.clicked = true; | |
52 | + } | |
53 | + | |
47 | 54 | sendResetPasswordLink() { |
48 | 55 | if (this.requestPasswordRequest.valid) { |
56 | + this.disableInputs(); | |
49 | 57 | this.authService.sendResetPasswordLink(this.requestPasswordRequest.get('email').value).subscribe( |
50 | 58 | () => { |
51 | 59 | this.store.dispatch(new ActionNotificationShow({ | ... | ... |
... | ... | @@ -2271,7 +2271,7 @@ |
2271 | 2271 | "expired-password-reset-message": "Your credentials has been expired! Please create new password.", |
2272 | 2272 | "new-password": "New password", |
2273 | 2273 | "new-password-again": "New password again", |
2274 | - "password-link-sent-message": "Password reset link was successfully sent!", | |
2274 | + "password-link-sent-message": "Reset link has been sent", | |
2275 | 2275 | "email": "Email", |
2276 | 2276 | "login-with": "Login with {{name}}", |
2277 | 2277 | "or": "or", | ... | ... |