fix: code review findings — dead catch blocks, notification email, role verification
- Remove dead IllegalArgumentException catch blocks in TenantPortalController (delegated methods now throw ResponseStatusException, handled by Spring) - Add password reset notification email in VendorAdminService.resetAdminPassword - Add verifyIsVendorAdmin guard to resetAdminPassword and resetAdminMfa to prevent platform admins from resetting arbitrary non-admin users Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -107,12 +107,8 @@ public class TenantPortalController {
|
|||||||
@PostMapping("/password")
|
@PostMapping("/password")
|
||||||
public ResponseEntity<Void> changeOwnPassword(@AuthenticationPrincipal Jwt jwt,
|
public ResponseEntity<Void> changeOwnPassword(@AuthenticationPrincipal Jwt jwt,
|
||||||
@RequestBody PasswordChangeRequest body) {
|
@RequestBody PasswordChangeRequest body) {
|
||||||
try {
|
portalService.changePassword(jwt.getSubject(), body.password());
|
||||||
portalService.changePassword(jwt.getSubject(), body.password());
|
return ResponseEntity.noContent().build();
|
||||||
return ResponseEntity.noContent().build();
|
|
||||||
} catch (IllegalArgumentException e) {
|
|
||||||
return ResponseEntity.badRequest().build();
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
@PostMapping("/team/{userId}/password")
|
@PostMapping("/team/{userId}/password")
|
||||||
@@ -203,23 +199,15 @@ public class TenantPortalController {
|
|||||||
if (name == null || name.isBlank()) {
|
if (name == null || name.isBlank()) {
|
||||||
return ResponseEntity.badRequest().build();
|
return ResponseEntity.badRequest().build();
|
||||||
}
|
}
|
||||||
try {
|
portalService.renamePasskey(jwt.getSubject(), id, name);
|
||||||
portalService.renamePasskey(jwt.getSubject(), id, name);
|
return ResponseEntity.noContent().build();
|
||||||
return ResponseEntity.noContent().build();
|
|
||||||
} catch (IllegalArgumentException e) {
|
|
||||||
return ResponseEntity.notFound().build();
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
@DeleteMapping("/mfa/webauthn/{id}")
|
@DeleteMapping("/mfa/webauthn/{id}")
|
||||||
public ResponseEntity<Void> deletePasskey(@AuthenticationPrincipal Jwt jwt,
|
public ResponseEntity<Void> deletePasskey(@AuthenticationPrincipal Jwt jwt,
|
||||||
@PathVariable String id) {
|
@PathVariable String id) {
|
||||||
try {
|
portalService.deletePasskey(jwt.getSubject(), id);
|
||||||
portalService.deletePasskey(jwt.getSubject(), id);
|
return ResponseEntity.noContent().build();
|
||||||
return ResponseEntity.noContent().build();
|
|
||||||
} catch (IllegalArgumentException e) {
|
|
||||||
return ResponseEntity.notFound().build();
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
@PostMapping("/mfa/method-preference")
|
@PostMapping("/mfa/method-preference")
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ package net.siegeln.cameleer.saas.vendor;
|
|||||||
|
|
||||||
import net.siegeln.cameleer.saas.account.AccountService;
|
import net.siegeln.cameleer.saas.account.AccountService;
|
||||||
import net.siegeln.cameleer.saas.identity.LogtoManagementClient;
|
import net.siegeln.cameleer.saas.identity.LogtoManagementClient;
|
||||||
|
import net.siegeln.cameleer.saas.notification.PasswordResetNotificationService;
|
||||||
import org.slf4j.Logger;
|
import org.slf4j.Logger;
|
||||||
import org.slf4j.LoggerFactory;
|
import org.slf4j.LoggerFactory;
|
||||||
import org.springframework.http.HttpStatus;
|
import org.springframework.http.HttpStatus;
|
||||||
@@ -20,13 +21,16 @@ public class VendorAdminService {
|
|||||||
private final LogtoManagementClient logtoClient;
|
private final LogtoManagementClient logtoClient;
|
||||||
private final AccountService accountService;
|
private final AccountService accountService;
|
||||||
private final EmailConnectorService emailConnectorService;
|
private final EmailConnectorService emailConnectorService;
|
||||||
|
private final PasswordResetNotificationService passwordNotificationService;
|
||||||
|
|
||||||
public VendorAdminService(LogtoManagementClient logtoClient,
|
public VendorAdminService(LogtoManagementClient logtoClient,
|
||||||
AccountService accountService,
|
AccountService accountService,
|
||||||
EmailConnectorService emailConnectorService) {
|
EmailConnectorService emailConnectorService,
|
||||||
|
PasswordResetNotificationService passwordNotificationService) {
|
||||||
this.logtoClient = logtoClient;
|
this.logtoClient = logtoClient;
|
||||||
this.accountService = accountService;
|
this.accountService = accountService;
|
||||||
this.emailConnectorService = emailConnectorService;
|
this.emailConnectorService = emailConnectorService;
|
||||||
|
this.passwordNotificationService = passwordNotificationService;
|
||||||
}
|
}
|
||||||
|
|
||||||
// --- Records ---
|
// --- Records ---
|
||||||
@@ -107,13 +111,36 @@ public class VendorAdminService {
|
|||||||
}
|
}
|
||||||
|
|
||||||
public void resetAdminPassword(String userId, String newPassword) {
|
public void resetAdminPassword(String userId, String newPassword) {
|
||||||
|
verifyIsVendorAdmin(userId);
|
||||||
accountService.validatePassword(newPassword);
|
accountService.validatePassword(newPassword);
|
||||||
logtoClient.updateUserPassword(userId, newPassword);
|
logtoClient.updateUserPassword(userId, newPassword);
|
||||||
log.info("Reset password for vendor admin {}", userId);
|
log.info("Reset password for vendor admin {}", userId);
|
||||||
|
|
||||||
|
// Send notification email
|
||||||
|
try {
|
||||||
|
var user = logtoClient.getUser(userId);
|
||||||
|
if (user != null) {
|
||||||
|
String email = String.valueOf(user.getOrDefault("primaryEmail", ""));
|
||||||
|
if (!email.isBlank()) {
|
||||||
|
passwordNotificationService.sendNotification(email);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
} catch (Exception e) {
|
||||||
|
log.warn("Failed to send password reset notification: {}", e.getMessage());
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
public void resetAdminMfa(String userId) {
|
public void resetAdminMfa(String userId) {
|
||||||
|
verifyIsVendorAdmin(userId);
|
||||||
logtoClient.deleteAllMfaVerifications(userId);
|
logtoClient.deleteAllMfaVerifications(userId);
|
||||||
log.info("Reset MFA for vendor admin {}", userId);
|
log.info("Reset MFA for vendor admin {}", userId);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private void verifyIsVendorAdmin(String userId) {
|
||||||
|
boolean isAdmin = listAdmins().stream()
|
||||||
|
.anyMatch(a -> userId.equals(a.userId()));
|
||||||
|
if (!isAdmin) {
|
||||||
|
throw new ResponseStatusException(HttpStatus.NOT_FOUND, "User is not a platform administrator");
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user