fix(tariff): fix critical security and correctness issues from code review
- Fix UserTariffController: use @CurrentUserId instead of broken extractUserId() - Fix UserTariffController: use @PathVariable instead of @RequestParam for userId in delete - Add @PreAuthorize annotations to all tariff endpoints (ADMIN for community, MEMBER for user) - Add duplicate tariff prevention for (source, target) pairs - Optimize membership check: replace N+1 query with efficient isActiveMemberOfCommunity() - Update tests to match new membership check API
This commit is contained in:
parent
0dfaa2d507
commit
5949359216
@ -39,4 +39,7 @@ public interface MembershipRepository extends JpaRepository<Membership, UUID> {
|
|||||||
|
|
||||||
@Query("SELECT mp.userId FROM Membership m JOIN m.meteringPoint mp WHERE m.energyCommunity.id = :communityId AND m.status != 'INACTIVE'")
|
@Query("SELECT mp.userId FROM Membership m JOIN m.meteringPoint mp WHERE m.energyCommunity.id = :communityId AND m.status != 'INACTIVE'")
|
||||||
List<UUID> findActiveMemberUserIdsByCommunityId(@Param("communityId") UUID communityId);
|
List<UUID> findActiveMemberUserIdsByCommunityId(@Param("communityId") UUID communityId);
|
||||||
|
|
||||||
|
@Query("SELECT COUNT(m) > 0 FROM Membership m JOIN m.meteringPoint mp WHERE mp.userId = :userId AND m.energyCommunity.id = :communityId AND m.status != 'INACTIVE'")
|
||||||
|
boolean isActiveMemberOfCommunity(@Param("userId") UUID userId, @Param("communityId") UUID communityId);
|
||||||
}
|
}
|
||||||
@ -8,6 +8,7 @@ import lombok.RequiredArgsConstructor;
|
|||||||
import org.springframework.http.HttpStatus;
|
import org.springframework.http.HttpStatus;
|
||||||
import org.springframework.http.MediaType;
|
import org.springframework.http.MediaType;
|
||||||
import org.springframework.http.ResponseEntity;
|
import org.springframework.http.ResponseEntity;
|
||||||
|
import org.springframework.security.access.prepost.PreAuthorize;
|
||||||
import org.springframework.web.bind.annotation.*;
|
import org.springframework.web.bind.annotation.*;
|
||||||
|
|
||||||
import java.util.UUID;
|
import java.util.UUID;
|
||||||
@ -20,6 +21,7 @@ public class TariffController {
|
|||||||
private final TariffService tariffService;
|
private final TariffService tariffService;
|
||||||
|
|
||||||
@PutMapping(value = "/{communityId}/tariff", produces = MediaType.APPLICATION_JSON_VALUE)
|
@PutMapping(value = "/{communityId}/tariff", produces = MediaType.APPLICATION_JSON_VALUE)
|
||||||
|
@PreAuthorize("hasRole('ADMIN')")
|
||||||
public ResponseEntity<CommunityTariffResponse> createOrUpdateCommunityTariff(
|
public ResponseEntity<CommunityTariffResponse> createOrUpdateCommunityTariff(
|
||||||
@PathVariable UUID communityId,
|
@PathVariable UUID communityId,
|
||||||
@Valid @RequestBody CommunityTariffRequest request) {
|
@Valid @RequestBody CommunityTariffRequest request) {
|
||||||
@ -28,6 +30,7 @@ public class TariffController {
|
|||||||
}
|
}
|
||||||
|
|
||||||
@GetMapping(value = "/{communityId}/tariff", produces = MediaType.APPLICATION_JSON_VALUE)
|
@GetMapping(value = "/{communityId}/tariff", produces = MediaType.APPLICATION_JSON_VALUE)
|
||||||
|
@PreAuthorize("hasRole('ADMIN')")
|
||||||
public ResponseEntity<CommunityTariffResponse> getCommunityTariff(@PathVariable UUID communityId) {
|
public ResponseEntity<CommunityTariffResponse> getCommunityTariff(@PathVariable UUID communityId) {
|
||||||
return ResponseEntity.ok(tariffService.getCommunityTariff(communityId));
|
return ResponseEntity.ok(tariffService.getCommunityTariff(communityId));
|
||||||
}
|
}
|
||||||
|
|||||||
@ -1,5 +1,6 @@
|
|||||||
package at.mueller.eeg.backend.tariff.api;
|
package at.mueller.eeg.backend.tariff.api;
|
||||||
|
|
||||||
|
import at.mueller.eeg.backend.common.security.CurrentUserId;
|
||||||
import at.mueller.eeg.backend.tariff.api.dto.UserTariffRequest;
|
import at.mueller.eeg.backend.tariff.api.dto.UserTariffRequest;
|
||||||
import at.mueller.eeg.backend.tariff.api.dto.UserTariffResponse;
|
import at.mueller.eeg.backend.tariff.api.dto.UserTariffResponse;
|
||||||
import at.mueller.eeg.backend.tariff.service.TariffService;
|
import at.mueller.eeg.backend.tariff.service.TariffService;
|
||||||
@ -8,6 +9,7 @@ import lombok.RequiredArgsConstructor;
|
|||||||
import org.springframework.http.HttpStatus;
|
import org.springframework.http.HttpStatus;
|
||||||
import org.springframework.http.MediaType;
|
import org.springframework.http.MediaType;
|
||||||
import org.springframework.http.ResponseEntity;
|
import org.springframework.http.ResponseEntity;
|
||||||
|
import org.springframework.security.access.prepost.PreAuthorize;
|
||||||
import org.springframework.web.bind.annotation.*;
|
import org.springframework.web.bind.annotation.*;
|
||||||
|
|
||||||
import java.util.List;
|
import java.util.List;
|
||||||
@ -21,41 +23,37 @@ public class UserTariffController {
|
|||||||
private final TariffService tariffService;
|
private final TariffService tariffService;
|
||||||
|
|
||||||
@PostMapping(produces = MediaType.APPLICATION_JSON_VALUE)
|
@PostMapping(produces = MediaType.APPLICATION_JSON_VALUE)
|
||||||
|
@PreAuthorize("hasRole('MEMBER')")
|
||||||
public ResponseEntity<UserTariffResponse> createUserTariff(
|
public ResponseEntity<UserTariffResponse> createUserTariff(
|
||||||
|
@CurrentUserId String userId,
|
||||||
@Valid @RequestBody UserTariffRequest request) {
|
@Valid @RequestBody UserTariffRequest request) {
|
||||||
// userId would come from SecurityContext in real app; here passed as query param or header
|
UserTariffResponse response = tariffService.createUserTariff(UUID.fromString(userId), request);
|
||||||
// For now, we accept it from X-User-Id header (set by gateway or test)
|
|
||||||
UUID userId = extractUserId(request);
|
|
||||||
UserTariffResponse response = tariffService.createUserTariff(userId, request);
|
|
||||||
return ResponseEntity.status(HttpStatus.CREATED).body(response);
|
return ResponseEntity.status(HttpStatus.CREATED).body(response);
|
||||||
}
|
}
|
||||||
|
|
||||||
@PutMapping(value = "/{tariffId}", produces = MediaType.APPLICATION_JSON_VALUE)
|
@PutMapping(value = "/{tariffId}", produces = MediaType.APPLICATION_JSON_VALUE)
|
||||||
|
@PreAuthorize("hasRole('MEMBER')")
|
||||||
public ResponseEntity<UserTariffResponse> updateUserTariff(
|
public ResponseEntity<UserTariffResponse> updateUserTariff(
|
||||||
|
@CurrentUserId String userId,
|
||||||
@PathVariable UUID tariffId,
|
@PathVariable UUID tariffId,
|
||||||
@Valid @RequestBody UserTariffRequest request) {
|
@Valid @RequestBody UserTariffRequest request) {
|
||||||
UUID userId = extractUserId(request);
|
UserTariffResponse response = tariffService.updateUserTariff(UUID.fromString(userId), tariffId, request);
|
||||||
UserTariffResponse response = tariffService.updateUserTariff(userId, tariffId, request);
|
|
||||||
return ResponseEntity.ok(response);
|
return ResponseEntity.ok(response);
|
||||||
}
|
}
|
||||||
|
|
||||||
@DeleteMapping("/{tariffId}")
|
@DeleteMapping("/{tariffId}")
|
||||||
|
@PreAuthorize("hasRole('MEMBER')")
|
||||||
public ResponseEntity<Void> deleteUserTariff(
|
public ResponseEntity<Void> deleteUserTariff(
|
||||||
@PathVariable UUID tariffId,
|
@CurrentUserId String userId,
|
||||||
@RequestParam UUID userId) {
|
@PathVariable UUID tariffId) {
|
||||||
tariffService.deleteUserTariff(userId, tariffId);
|
tariffService.deleteUserTariff(UUID.fromString(userId), tariffId);
|
||||||
return ResponseEntity.noContent().build();
|
return ResponseEntity.noContent().build();
|
||||||
}
|
}
|
||||||
|
|
||||||
@GetMapping(value = "/community/{communityId}", produces = MediaType.APPLICATION_JSON_VALUE)
|
@GetMapping(value = "/community/{communityId}", produces = MediaType.APPLICATION_JSON_VALUE)
|
||||||
|
@PreAuthorize("hasRole('MEMBER')")
|
||||||
public ResponseEntity<List<UserTariffResponse>> getUserTariffsForCommunity(
|
public ResponseEntity<List<UserTariffResponse>> getUserTariffsForCommunity(
|
||||||
@PathVariable UUID communityId) {
|
@PathVariable UUID communityId) {
|
||||||
return ResponseEntity.ok(tariffService.getUserTariffsForCommunity(communityId));
|
return ResponseEntity.ok(tariffService.getUserTariffsForCommunity(communityId));
|
||||||
}
|
}
|
||||||
|
|
||||||
private UUID extractUserId(UserTariffRequest request) {
|
|
||||||
// In production, extract from SecurityContext. For now, derive from source metering point.
|
|
||||||
// This is a placeholder - real implementation would use @AuthenticationPrincipal
|
|
||||||
return null;
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|||||||
@ -74,6 +74,13 @@ public class TariffService {
|
|||||||
throw new IllegalStateException("Quell- und Ziel-Zählpunkt müssen unterschiedlich sein.");
|
throw new IllegalStateException("Quell- und Ziel-Zählpunkt müssen unterschiedlich sein.");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
userTariffRepository.findBySourceMeteringPointIdAndTargetMeteringPointId(
|
||||||
|
request.sourceMeteringPointId(), request.targetMeteringPointId())
|
||||||
|
.ifPresent(existing -> {
|
||||||
|
throw new IllegalStateException(
|
||||||
|
"Es existiert bereits ein Tarif zwischen diesen beiden Zählpunkten.");
|
||||||
|
});
|
||||||
|
|
||||||
MeteringPoint sourcePoint = meteringPointRepository.findById(request.sourceMeteringPointId())
|
MeteringPoint sourcePoint = meteringPointRepository.findById(request.sourceMeteringPointId())
|
||||||
.orElseThrow(() -> new IllegalArgumentException(
|
.orElseThrow(() -> new IllegalArgumentException(
|
||||||
"Quell-Zählpunkt nicht gefunden: " + request.sourceMeteringPointId()));
|
"Quell-Zählpunkt nicht gefunden: " + request.sourceMeteringPointId()));
|
||||||
@ -102,10 +109,10 @@ public class TariffService {
|
|||||||
UUID sourceUserId = sourcePoint.getUserId();
|
UUID sourceUserId = sourcePoint.getUserId();
|
||||||
UUID targetUserId = targetPoint.getUserId();
|
UUID targetUserId = targetPoint.getUserId();
|
||||||
|
|
||||||
if (!isMemberOfCommunity(sourceUserId, request.energyCommunityId())) {
|
if (!membershipRepository.isActiveMemberOfCommunity(sourceUserId, request.energyCommunityId())) {
|
||||||
throw new IllegalStateException("Quell-Benutzer ist kein Mitglied der Energiegemeinschaft.");
|
throw new IllegalStateException("Quell-Benutzer ist kein Mitglied der Energiegemeinschaft.");
|
||||||
}
|
}
|
||||||
if (!isMemberOfCommunity(targetUserId, request.energyCommunityId())) {
|
if (!membershipRepository.isActiveMemberOfCommunity(targetUserId, request.energyCommunityId())) {
|
||||||
throw new IllegalStateException("Ziel-Benutzer ist kein Mitglied der Energiegemeinschaft.");
|
throw new IllegalStateException("Ziel-Benutzer ist kein Mitglied der Energiegemeinschaft.");
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -173,10 +180,10 @@ public class TariffService {
|
|||||||
UUID newSourceUserId = newSourcePoint.getUserId();
|
UUID newSourceUserId = newSourcePoint.getUserId();
|
||||||
UUID newTargetUserId = newTargetPoint.getUserId();
|
UUID newTargetUserId = newTargetPoint.getUserId();
|
||||||
|
|
||||||
if (!isMemberOfCommunity(newSourceUserId, request.energyCommunityId())) {
|
if (!membershipRepository.isActiveMemberOfCommunity(newSourceUserId, request.energyCommunityId())) {
|
||||||
throw new IllegalStateException("Quell-Benutzer ist kein Mitglied der Energiegemeinschaft.");
|
throw new IllegalStateException("Quell-Benutzer ist kein Mitglied der Energiegemeinschaft.");
|
||||||
}
|
}
|
||||||
if (!isMemberOfCommunity(newTargetUserId, request.energyCommunityId())) {
|
if (!membershipRepository.isActiveMemberOfCommunity(newTargetUserId, request.energyCommunityId())) {
|
||||||
throw new IllegalStateException("Ziel-Benutzer ist kein Mitglied der Energiegemeinschaft.");
|
throw new IllegalStateException("Ziel-Benutzer ist kein Mitglied der Energiegemeinschaft.");
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -228,12 +235,6 @@ public class TariffService {
|
|||||||
.toList();
|
.toList();
|
||||||
}
|
}
|
||||||
|
|
||||||
private boolean isMemberOfCommunity(UUID userId, UUID communityId) {
|
|
||||||
return membershipRepository.findByUserIdWithDetails(userId).stream()
|
|
||||||
.anyMatch(m -> m.getEnergyCommunity().getId().equals(communityId)
|
|
||||||
&& m.getStatus() != MembershipStatus.INACTIVE);
|
|
||||||
}
|
|
||||||
|
|
||||||
private CommunityTariffResponse toCommunityTariffResponse(CommunityTariff tariff) {
|
private CommunityTariffResponse toCommunityTariffResponse(CommunityTariff tariff) {
|
||||||
return new CommunityTariffResponse(
|
return new CommunityTariffResponse(
|
||||||
tariff.getId(),
|
tariff.getId(),
|
||||||
|
|||||||
@ -179,10 +179,8 @@ class TariffServiceTest {
|
|||||||
.thenReturn(Optional.of(targetPoint));
|
.thenReturn(Optional.of(targetPoint));
|
||||||
when(communityTariffRepository.findByEnergyCommunityId(communityId))
|
when(communityTariffRepository.findByEnergyCommunityId(communityId))
|
||||||
.thenReturn(Optional.of(communityTariff));
|
.thenReturn(Optional.of(communityTariff));
|
||||||
when(membershipRepository.findByUserIdWithDetails(sourceUserId))
|
when(membershipRepository.isActiveMemberOfCommunity(sourceUserId, communityId)).thenReturn(true);
|
||||||
.thenReturn(List.of(sourceMembership));
|
when(membershipRepository.isActiveMemberOfCommunity(targetUserId, communityId)).thenReturn(true);
|
||||||
when(membershipRepository.findByUserIdWithDetails(targetUserId))
|
|
||||||
.thenReturn(List.of(targetMembership));
|
|
||||||
|
|
||||||
UserTariff savedTariff = new UserTariff();
|
UserTariff savedTariff = new UserTariff();
|
||||||
savedTariff.setId(UUID.randomUUID());
|
savedTariff.setId(UUID.randomUUID());
|
||||||
@ -317,8 +315,7 @@ class TariffServiceTest {
|
|||||||
.thenReturn(Optional.of(targetPoint));
|
.thenReturn(Optional.of(targetPoint));
|
||||||
when(communityTariffRepository.findByEnergyCommunityId(communityId))
|
when(communityTariffRepository.findByEnergyCommunityId(communityId))
|
||||||
.thenReturn(Optional.of(communityTariff));
|
.thenReturn(Optional.of(communityTariff));
|
||||||
when(membershipRepository.findByUserIdWithDetails(sourceUserId))
|
when(membershipRepository.isActiveMemberOfCommunity(sourceUserId, communityId)).thenReturn(false);
|
||||||
.thenReturn(List.of());
|
|
||||||
|
|
||||||
UserTariffRequest request = new UserTariffRequest(
|
UserTariffRequest request = new UserTariffRequest(
|
||||||
communityId, sourceMeteringPointId, targetMeteringPointId,
|
communityId, sourceMeteringPointId, targetMeteringPointId,
|
||||||
@ -336,10 +333,8 @@ class TariffServiceTest {
|
|||||||
.thenReturn(Optional.of(targetPoint));
|
.thenReturn(Optional.of(targetPoint));
|
||||||
when(communityTariffRepository.findByEnergyCommunityId(communityId))
|
when(communityTariffRepository.findByEnergyCommunityId(communityId))
|
||||||
.thenReturn(Optional.of(communityTariff));
|
.thenReturn(Optional.of(communityTariff));
|
||||||
when(membershipRepository.findByUserIdWithDetails(sourceUserId))
|
when(membershipRepository.isActiveMemberOfCommunity(sourceUserId, communityId)).thenReturn(true);
|
||||||
.thenReturn(List.of(sourceMembership));
|
when(membershipRepository.isActiveMemberOfCommunity(targetUserId, communityId)).thenReturn(false);
|
||||||
when(membershipRepository.findByUserIdWithDetails(targetUserId))
|
|
||||||
.thenReturn(List.of());
|
|
||||||
|
|
||||||
UserTariffRequest request = new UserTariffRequest(
|
UserTariffRequest request = new UserTariffRequest(
|
||||||
communityId, sourceMeteringPointId, targetMeteringPointId,
|
communityId, sourceMeteringPointId, targetMeteringPointId,
|
||||||
@ -368,10 +363,8 @@ class TariffServiceTest {
|
|||||||
.thenReturn(Optional.of(targetPoint));
|
.thenReturn(Optional.of(targetPoint));
|
||||||
when(communityTariffRepository.findByEnergyCommunityId(communityId))
|
when(communityTariffRepository.findByEnergyCommunityId(communityId))
|
||||||
.thenReturn(Optional.of(communityTariff));
|
.thenReturn(Optional.of(communityTariff));
|
||||||
when(membershipRepository.findByUserIdWithDetails(sourceUserId))
|
when(membershipRepository.isActiveMemberOfCommunity(sourceUserId, communityId)).thenReturn(true);
|
||||||
.thenReturn(List.of(sourceMembership));
|
when(membershipRepository.isActiveMemberOfCommunity(targetUserId, communityId)).thenReturn(true);
|
||||||
when(membershipRepository.findByUserIdWithDetails(targetUserId))
|
|
||||||
.thenReturn(List.of(targetMembership));
|
|
||||||
when(userTariffRepository.save(any(UserTariff.class))).thenReturn(existingTariff);
|
when(userTariffRepository.save(any(UserTariff.class))).thenReturn(existingTariff);
|
||||||
|
|
||||||
UserTariffRequest request = new UserTariffRequest(
|
UserTariffRequest request = new UserTariffRequest(
|
||||||
@ -513,10 +506,8 @@ class TariffServiceTest {
|
|||||||
.thenReturn(Optional.of(targetPoint));
|
.thenReturn(Optional.of(targetPoint));
|
||||||
when(communityTariffRepository.findByEnergyCommunityId(communityId))
|
when(communityTariffRepository.findByEnergyCommunityId(communityId))
|
||||||
.thenReturn(Optional.of(communityTariff));
|
.thenReturn(Optional.of(communityTariff));
|
||||||
when(membershipRepository.findByUserIdWithDetails(sourceUserId))
|
when(membershipRepository.isActiveMemberOfCommunity(sourceUserId, communityId)).thenReturn(true);
|
||||||
.thenReturn(List.of(sourceMembership));
|
when(membershipRepository.isActiveMemberOfCommunity(targetUserId, communityId)).thenReturn(true);
|
||||||
when(membershipRepository.findByUserIdWithDetails(targetUserId))
|
|
||||||
.thenReturn(List.of(targetMembership));
|
|
||||||
|
|
||||||
UserTariff savedTariff = new UserTariff();
|
UserTariff savedTariff = new UserTariff();
|
||||||
savedTariff.setId(UUID.randomUUID());
|
savedTariff.setId(UUID.randomUUID());
|
||||||
@ -558,8 +549,7 @@ class TariffServiceTest {
|
|||||||
.thenReturn(Optional.of(targetPoint));
|
.thenReturn(Optional.of(targetPoint));
|
||||||
when(communityTariffRepository.findByEnergyCommunityId(communityId))
|
when(communityTariffRepository.findByEnergyCommunityId(communityId))
|
||||||
.thenReturn(Optional.of(communityTariff));
|
.thenReturn(Optional.of(communityTariff));
|
||||||
when(membershipRepository.findByUserIdWithDetails(sourceUserId))
|
when(membershipRepository.isActiveMemberOfCommunity(sourceUserId, communityId)).thenReturn(true);
|
||||||
.thenReturn(List.of(sourceMembership));
|
|
||||||
|
|
||||||
UserTariffRequest request = new UserTariffRequest(
|
UserTariffRequest request = new UserTariffRequest(
|
||||||
communityId, sourceMeteringPointId, targetMeteringPointId,
|
communityId, sourceMeteringPointId, targetMeteringPointId,
|
||||||
@ -598,10 +588,8 @@ class TariffServiceTest {
|
|||||||
.thenReturn(Optional.of(targetPoint));
|
.thenReturn(Optional.of(targetPoint));
|
||||||
when(communityTariffRepository.findByEnergyCommunityId(communityId))
|
when(communityTariffRepository.findByEnergyCommunityId(communityId))
|
||||||
.thenReturn(Optional.of(communityTariff));
|
.thenReturn(Optional.of(communityTariff));
|
||||||
when(membershipRepository.findByUserIdWithDetails(sourceUserId))
|
when(membershipRepository.isActiveMemberOfCommunity(sourceUserId, communityId)).thenReturn(true);
|
||||||
.thenReturn(List.of(sourceMembership));
|
when(membershipRepository.isActiveMemberOfCommunity(targetUserId, communityId)).thenReturn(true);
|
||||||
when(membershipRepository.findByUserIdWithDetails(targetUserId))
|
|
||||||
.thenReturn(List.of(targetMembership));
|
|
||||||
|
|
||||||
UserTariff savedTariff = new UserTariff();
|
UserTariff savedTariff = new UserTariff();
|
||||||
savedTariff.setId(UUID.randomUUID());
|
savedTariff.setId(UUID.randomUUID());
|
||||||
|
|||||||
@ -110,12 +110,6 @@ paths:
|
|||||||
schema:
|
schema:
|
||||||
type: string
|
type: string
|
||||||
format: uuid
|
format: uuid
|
||||||
- name: userId
|
|
||||||
in: query
|
|
||||||
required: true
|
|
||||||
schema:
|
|
||||||
type: string
|
|
||||||
format: uuid
|
|
||||||
responses:
|
responses:
|
||||||
"200":
|
"200":
|
||||||
description: OK
|
description: OK
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user