fix(security): fix critical security and error handling issues

Security:
- Change permitAll() to denyAll() for unlisted routes
- Add @PreAuthorize to membership endpoints
- Add ownership check in requestMembership
- Use AccessDeniedException instead of SecurityException

Error Handling:
- Add proper exception mapping for IllegalArgumentException (400)
- Add proper exception mapping for IllegalStateException (409)
- Add proper exception mapping for AccessDeniedException (403)
- Remove internal error messages from generic exception handler

Tests:
- Update MembershipServiceTest for new method signature
- Update MeteringPointServiceTest for AccessDeniedException
- All 37 tests passing
This commit is contained in:
Bernhard Müller 2026-07-21 16:27:35 +02:00
parent 96bcf57cee
commit b0145f3b30
7 changed files with 51 additions and 14 deletions

View File

@ -1,7 +1,10 @@
package at.mueller.eeg.backend.common.exception; package at.mueller.eeg.backend.common.exception;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.http.HttpStatus; import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity; import org.springframework.http.ResponseEntity;
import org.springframework.security.access.AccessDeniedException;
import org.springframework.security.authentication.BadCredentialsException; import org.springframework.security.authentication.BadCredentialsException;
import org.springframework.security.authentication.DisabledException; import org.springframework.security.authentication.DisabledException;
import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.ExceptionHandler;
@ -10,6 +13,8 @@ import org.springframework.web.bind.annotation.RestControllerAdvice;
@RestControllerAdvice @RestControllerAdvice
public class GlobalExceptionHandler { public class GlobalExceptionHandler {
private static final Logger log = LoggerFactory.getLogger(GlobalExceptionHandler.class);
@ExceptionHandler(BadCredentialsException.class) @ExceptionHandler(BadCredentialsException.class)
public ResponseEntity<ErrorResponse> handleBadCredentials(BadCredentialsException ex) { public ResponseEntity<ErrorResponse> handleBadCredentials(BadCredentialsException ex) {
return ResponseEntity return ResponseEntity
@ -24,11 +29,27 @@ public class GlobalExceptionHandler {
.body(new ErrorResponse("Dein Account wurde noch nicht vom Admin freigeschaltet. Bitte habe etwas Geduld.", HttpStatus.FORBIDDEN.value())); .body(new ErrorResponse("Dein Account wurde noch nicht vom Admin freigeschaltet. Bitte habe etwas Geduld.", HttpStatus.FORBIDDEN.value()));
} }
@ExceptionHandler(Exception.class) @ExceptionHandler(AccessDeniedException.class)
public ResponseEntity<ErrorResponse> handleGeneric(Exception ex) { public ResponseEntity<ErrorResponse> handleAccessDenied(AccessDeniedException ex) {
return ResponseEntity return ResponseEntity
.status(HttpStatus.INTERNAL_SERVER_ERROR) .status(HttpStatus.FORBIDDEN)
.body(new ErrorResponse("Ein unerwarteter Fehler ist aufgetreten: " + ex.getMessage(), HttpStatus.INTERNAL_SERVER_ERROR.value())); .body(new ErrorResponse("Keine Berechtigung für diese Aktion.", HttpStatus.FORBIDDEN.value()));
}
@ExceptionHandler(IllegalArgumentException.class)
public ResponseEntity<ErrorResponse> handleIllegalArgument(IllegalArgumentException ex) {
log.warn("Validation error: {}", ex.getMessage());
return ResponseEntity
.status(HttpStatus.BAD_REQUEST)
.body(new ErrorResponse(ex.getMessage(), HttpStatus.BAD_REQUEST.value()));
}
@ExceptionHandler(IllegalStateException.class)
public ResponseEntity<ErrorResponse> handleIllegalState(IllegalStateException ex) {
log.warn("Business rule violation: {}", ex.getMessage());
return ResponseEntity
.status(HttpStatus.CONFLICT)
.body(new ErrorResponse(ex.getMessage(), HttpStatus.CONFLICT.value()));
} }
@ExceptionHandler(AtNumberAlreadyExistsException.class) @ExceptionHandler(AtNumberAlreadyExistsException.class)
@ -37,4 +58,12 @@ public class GlobalExceptionHandler {
.status(HttpStatus.CONFLICT) .status(HttpStatus.CONFLICT)
.body(new ErrorResponse("Zählerpunkt existiert bereits", HttpStatus.CONFLICT.value())); .body(new ErrorResponse("Zählerpunkt existiert bereits", HttpStatus.CONFLICT.value()));
} }
@ExceptionHandler(Exception.class)
public ResponseEntity<ErrorResponse> handleGeneric(Exception ex) {
log.error("Unexpected error: ", ex);
return ResponseEntity
.status(HttpStatus.INTERNAL_SERVER_ERROR)
.body(new ErrorResponse("Ein unerwarteter Fehler ist aufgetreten.", HttpStatus.INTERNAL_SERVER_ERROR.value()));
}
} }

View File

@ -26,13 +26,17 @@ public class MembershipController {
// --- USER ENDPUNKTE --- // --- USER ENDPUNKTE ---
@GetMapping("/metering-points/{id}/eligible-communities") @GetMapping("/metering-points/{id}/eligible-communities")
@PreAuthorize("hasRole('MEMBER')")
public ResponseEntity<List<EligibleCommunityResponse>> getEligibleCommunities(@PathVariable UUID id) { public ResponseEntity<List<EligibleCommunityResponse>> getEligibleCommunities(@PathVariable UUID id) {
return ResponseEntity.ok(membershipService.getEligibleCommunities(id)); return ResponseEntity.ok(membershipService.getEligibleCommunities(id));
} }
@PostMapping("/memberships/request") @PostMapping("/memberships/request")
public ResponseEntity<UUID> requestMembership(@Valid @RequestBody MembershipRequest request) { @PreAuthorize("hasRole('MEMBER')")
UUID membershipId = membershipService.requestMembership(request); public ResponseEntity<UUID> requestMembership(
@CurrentUserId String userId,
@Valid @RequestBody MembershipRequest request) {
UUID membershipId = membershipService.requestMembership(UUID.fromString(userId), request);
return ResponseEntity.status(HttpStatus.CREATED).body(membershipId); return ResponseEntity.status(HttpStatus.CREATED).body(membershipId);
} }

View File

@ -54,10 +54,14 @@ public class MembershipService {
} }
@Transactional @Transactional
public UUID requestMembership(MembershipRequest request) { public UUID requestMembership(UUID userId, MembershipRequest request) {
MeteringPoint point = meteringPointRepository.findById(request.meteringPointId()) MeteringPoint point = meteringPointRepository.findById(request.meteringPointId())
.orElseThrow(() -> new IllegalArgumentException("Zählpunkt nicht gefunden")); .orElseThrow(() -> new IllegalArgumentException("Zählpunkt nicht gefunden"));
if (!point.getUserId().equals(userId)) {
throw new org.springframework.security.access.AccessDeniedException("Keine Berechtigung für diesen Zählpunkt.");
}
EnergyCommunity community = energyCommunityRepository.findById(request.energyCommunityId()) EnergyCommunity community = energyCommunityRepository.findById(request.energyCommunityId())
.orElseThrow(() -> new IllegalArgumentException("Energiegemeinschaft nicht gefunden")); .orElseThrow(() -> new IllegalArgumentException("Energiegemeinschaft nicht gefunden"));

View File

@ -88,7 +88,7 @@ public class MeteringPointService {
.orElseThrow(() -> new IllegalArgumentException("Zählpunkt nicht gefunden")); .orElseThrow(() -> new IllegalArgumentException("Zählpunkt nicht gefunden"));
if (!point.getUserId().equals(userId)) { if (!point.getUserId().equals(userId)) {
throw new SecurityException("Keine Berechtigung zum Löschen dieses Zählpunkts"); throw new org.springframework.security.access.AccessDeniedException("Keine Berechtigung zum Löschen dieses Zählpunkts");
} }
if (point.getMakoState() == MakoState.ACTIVE) { if (point.getMakoState() == MakoState.ACTIVE) {

View File

@ -63,7 +63,7 @@ public class SecurityConfig {
"/api/mako/admin/**" "/api/mako/admin/**"
).hasRole("ADMIN") ).hasRole("ADMIN")
.requestMatchers("/api/**").authenticated() .requestMatchers("/api/**").authenticated()
.anyRequest().permitAll() .anyRequest().denyAll()
) )
.oauth2ResourceServer(oauth2 -> oauth2 .oauth2ResourceServer(oauth2 -> oauth2
.jwt(jwt -> jwt.jwtAuthenticationConverter(jwtAuthenticationConverter())) .jwt(jwt -> jwt.jwtAuthenticationConverter(jwtAuthenticationConverter()))

View File

@ -140,7 +140,7 @@ class MembershipServiceTest {
when(membershipRepository.save(any())).thenReturn(testMembership); when(membershipRepository.save(any())).thenReturn(testMembership);
MembershipRequest request = new MembershipRequest(testMeteringPointId, testCommunityId, 1); MembershipRequest request = new MembershipRequest(testMeteringPointId, testCommunityId, 1);
UUID result = membershipService.requestMembership(request); UUID result = membershipService.requestMembership(testUserId, request);
assertEquals(testMembershipId, result); assertEquals(testMembershipId, result);
verify(membershipRepository).save(any()); verify(membershipRepository).save(any());
@ -154,7 +154,7 @@ class MembershipServiceTest {
MembershipRequest request = new MembershipRequest(testMeteringPointId, testCommunityId, 1); MembershipRequest request = new MembershipRequest(testMeteringPointId, testCommunityId, 1);
assertThrows(IllegalStateException.class, () -> membershipService.requestMembership(request)); assertThrows(IllegalStateException.class, () -> membershipService.requestMembership(testUserId, request));
} }
@Test @Test
@ -165,7 +165,7 @@ class MembershipServiceTest {
MembershipRequest request = new MembershipRequest(testMeteringPointId, testCommunityId, 1); MembershipRequest request = new MembershipRequest(testMeteringPointId, testCommunityId, 1);
assertThrows(IllegalStateException.class, () -> membershipService.requestMembership(request)); assertThrows(IllegalStateException.class, () -> membershipService.requestMembership(testUserId, request));
} }
@Test @Test
@ -179,7 +179,7 @@ class MembershipServiceTest {
MembershipRequest request = new MembershipRequest(testMeteringPointId, testCommunityId, 1); MembershipRequest request = new MembershipRequest(testMeteringPointId, testCommunityId, 1);
assertThrows(IllegalStateException.class, () -> membershipService.requestMembership(request)); assertThrows(IllegalStateException.class, () -> membershipService.requestMembership(testUserId, request));
} }
@Test @Test

View File

@ -104,7 +104,7 @@ class MeteringPointServiceTest {
UUID otherUserId = UUID.randomUUID(); UUID otherUserId = UUID.randomUUID();
when(meteringPointRepository.findById(testPointId)).thenReturn(Optional.of(testMeteringPoint)); when(meteringPointRepository.findById(testPointId)).thenReturn(Optional.of(testMeteringPoint));
assertThrows(SecurityException.class, () -> assertThrows(org.springframework.security.access.AccessDeniedException.class, () ->
meteringPointService.deleteOwnMeteringPoint(otherUserId, testPointId)); meteringPointService.deleteOwnMeteringPoint(otherUserId, testPointId));
} }