From 59493592162bcaf33a0f1694d7aebfafdfe02405 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bernhard=20M=C3=BCller?= Date: Wed, 22 Jul 2026 10:11:12 +0200 Subject: [PATCH] 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 --- .../repository/MembershipRepository.java | 3 ++ .../backend/tariff/api/TariffController.java | 3 ++ .../tariff/api/UserTariffController.java | 28 +++++++-------- .../backend/tariff/service/TariffService.java | 21 +++++------ .../tariff/service/TariffServiceTest.java | 36 +++++++------------ eeg_frontend/openapi.yaml | 6 ---- 6 files changed, 42 insertions(+), 55 deletions(-) diff --git a/eeg_backend/src/main/java/at/mueller/eeg/backend/community/repository/MembershipRepository.java b/eeg_backend/src/main/java/at/mueller/eeg/backend/community/repository/MembershipRepository.java index 3ff2264..0768e8e 100644 --- a/eeg_backend/src/main/java/at/mueller/eeg/backend/community/repository/MembershipRepository.java +++ b/eeg_backend/src/main/java/at/mueller/eeg/backend/community/repository/MembershipRepository.java @@ -39,4 +39,7 @@ public interface MembershipRepository extends JpaRepository { @Query("SELECT mp.userId FROM Membership m JOIN m.meteringPoint mp WHERE m.energyCommunity.id = :communityId AND m.status != 'INACTIVE'") List 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); } \ No newline at end of file diff --git a/eeg_backend/src/main/java/at/mueller/eeg/backend/tariff/api/TariffController.java b/eeg_backend/src/main/java/at/mueller/eeg/backend/tariff/api/TariffController.java index fbc90c1..b682780 100644 --- a/eeg_backend/src/main/java/at/mueller/eeg/backend/tariff/api/TariffController.java +++ b/eeg_backend/src/main/java/at/mueller/eeg/backend/tariff/api/TariffController.java @@ -8,6 +8,7 @@ import lombok.RequiredArgsConstructor; import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; import org.springframework.http.ResponseEntity; +import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.web.bind.annotation.*; import java.util.UUID; @@ -20,6 +21,7 @@ public class TariffController { private final TariffService tariffService; @PutMapping(value = "/{communityId}/tariff", produces = MediaType.APPLICATION_JSON_VALUE) + @PreAuthorize("hasRole('ADMIN')") public ResponseEntity createOrUpdateCommunityTariff( @PathVariable UUID communityId, @Valid @RequestBody CommunityTariffRequest request) { @@ -28,6 +30,7 @@ public class TariffController { } @GetMapping(value = "/{communityId}/tariff", produces = MediaType.APPLICATION_JSON_VALUE) + @PreAuthorize("hasRole('ADMIN')") public ResponseEntity getCommunityTariff(@PathVariable UUID communityId) { return ResponseEntity.ok(tariffService.getCommunityTariff(communityId)); } diff --git a/eeg_backend/src/main/java/at/mueller/eeg/backend/tariff/api/UserTariffController.java b/eeg_backend/src/main/java/at/mueller/eeg/backend/tariff/api/UserTariffController.java index fef2cf1..e53c3b9 100644 --- a/eeg_backend/src/main/java/at/mueller/eeg/backend/tariff/api/UserTariffController.java +++ b/eeg_backend/src/main/java/at/mueller/eeg/backend/tariff/api/UserTariffController.java @@ -1,5 +1,6 @@ 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.UserTariffResponse; import at.mueller.eeg.backend.tariff.service.TariffService; @@ -8,6 +9,7 @@ import lombok.RequiredArgsConstructor; import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; import org.springframework.http.ResponseEntity; +import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.web.bind.annotation.*; import java.util.List; @@ -21,41 +23,37 @@ public class UserTariffController { private final TariffService tariffService; @PostMapping(produces = MediaType.APPLICATION_JSON_VALUE) + @PreAuthorize("hasRole('MEMBER')") public ResponseEntity createUserTariff( + @CurrentUserId String userId, @Valid @RequestBody UserTariffRequest request) { - // userId would come from SecurityContext in real app; here passed as query param or header - // 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); + UserTariffResponse response = tariffService.createUserTariff(UUID.fromString(userId), request); return ResponseEntity.status(HttpStatus.CREATED).body(response); } @PutMapping(value = "/{tariffId}", produces = MediaType.APPLICATION_JSON_VALUE) + @PreAuthorize("hasRole('MEMBER')") public ResponseEntity updateUserTariff( + @CurrentUserId String userId, @PathVariable UUID tariffId, @Valid @RequestBody UserTariffRequest request) { - UUID userId = extractUserId(request); - UserTariffResponse response = tariffService.updateUserTariff(userId, tariffId, request); + UserTariffResponse response = tariffService.updateUserTariff(UUID.fromString(userId), tariffId, request); return ResponseEntity.ok(response); } @DeleteMapping("/{tariffId}") + @PreAuthorize("hasRole('MEMBER')") public ResponseEntity deleteUserTariff( - @PathVariable UUID tariffId, - @RequestParam UUID userId) { - tariffService.deleteUserTariff(userId, tariffId); + @CurrentUserId String userId, + @PathVariable UUID tariffId) { + tariffService.deleteUserTariff(UUID.fromString(userId), tariffId); return ResponseEntity.noContent().build(); } @GetMapping(value = "/community/{communityId}", produces = MediaType.APPLICATION_JSON_VALUE) + @PreAuthorize("hasRole('MEMBER')") public ResponseEntity> getUserTariffsForCommunity( @PathVariable UUID 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; - } } diff --git a/eeg_backend/src/main/java/at/mueller/eeg/backend/tariff/service/TariffService.java b/eeg_backend/src/main/java/at/mueller/eeg/backend/tariff/service/TariffService.java index a6f94a3..cef11cc 100644 --- a/eeg_backend/src/main/java/at/mueller/eeg/backend/tariff/service/TariffService.java +++ b/eeg_backend/src/main/java/at/mueller/eeg/backend/tariff/service/TariffService.java @@ -74,6 +74,13 @@ public class TariffService { 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()) .orElseThrow(() -> new IllegalArgumentException( "Quell-Zählpunkt nicht gefunden: " + request.sourceMeteringPointId())); @@ -102,10 +109,10 @@ public class TariffService { UUID sourceUserId = sourcePoint.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."); } - if (!isMemberOfCommunity(targetUserId, request.energyCommunityId())) { + if (!membershipRepository.isActiveMemberOfCommunity(targetUserId, request.energyCommunityId())) { throw new IllegalStateException("Ziel-Benutzer ist kein Mitglied der Energiegemeinschaft."); } @@ -173,10 +180,10 @@ public class TariffService { UUID newSourceUserId = newSourcePoint.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."); } - if (!isMemberOfCommunity(newTargetUserId, request.energyCommunityId())) { + if (!membershipRepository.isActiveMemberOfCommunity(newTargetUserId, request.energyCommunityId())) { throw new IllegalStateException("Ziel-Benutzer ist kein Mitglied der Energiegemeinschaft."); } @@ -228,12 +235,6 @@ public class TariffService { .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) { return new CommunityTariffResponse( tariff.getId(), diff --git a/eeg_backend/src/test/java/at/mueller/eeg/backend/tariff/service/TariffServiceTest.java b/eeg_backend/src/test/java/at/mueller/eeg/backend/tariff/service/TariffServiceTest.java index 8e7e752..968bcd7 100644 --- a/eeg_backend/src/test/java/at/mueller/eeg/backend/tariff/service/TariffServiceTest.java +++ b/eeg_backend/src/test/java/at/mueller/eeg/backend/tariff/service/TariffServiceTest.java @@ -179,10 +179,8 @@ class TariffServiceTest { .thenReturn(Optional.of(targetPoint)); when(communityTariffRepository.findByEnergyCommunityId(communityId)) .thenReturn(Optional.of(communityTariff)); - when(membershipRepository.findByUserIdWithDetails(sourceUserId)) - .thenReturn(List.of(sourceMembership)); - when(membershipRepository.findByUserIdWithDetails(targetUserId)) - .thenReturn(List.of(targetMembership)); + when(membershipRepository.isActiveMemberOfCommunity(sourceUserId, communityId)).thenReturn(true); + when(membershipRepository.isActiveMemberOfCommunity(targetUserId, communityId)).thenReturn(true); UserTariff savedTariff = new UserTariff(); savedTariff.setId(UUID.randomUUID()); @@ -317,8 +315,7 @@ class TariffServiceTest { .thenReturn(Optional.of(targetPoint)); when(communityTariffRepository.findByEnergyCommunityId(communityId)) .thenReturn(Optional.of(communityTariff)); - when(membershipRepository.findByUserIdWithDetails(sourceUserId)) - .thenReturn(List.of()); + when(membershipRepository.isActiveMemberOfCommunity(sourceUserId, communityId)).thenReturn(false); UserTariffRequest request = new UserTariffRequest( communityId, sourceMeteringPointId, targetMeteringPointId, @@ -336,10 +333,8 @@ class TariffServiceTest { .thenReturn(Optional.of(targetPoint)); when(communityTariffRepository.findByEnergyCommunityId(communityId)) .thenReturn(Optional.of(communityTariff)); - when(membershipRepository.findByUserIdWithDetails(sourceUserId)) - .thenReturn(List.of(sourceMembership)); - when(membershipRepository.findByUserIdWithDetails(targetUserId)) - .thenReturn(List.of()); + when(membershipRepository.isActiveMemberOfCommunity(sourceUserId, communityId)).thenReturn(true); + when(membershipRepository.isActiveMemberOfCommunity(targetUserId, communityId)).thenReturn(false); UserTariffRequest request = new UserTariffRequest( communityId, sourceMeteringPointId, targetMeteringPointId, @@ -368,10 +363,8 @@ class TariffServiceTest { .thenReturn(Optional.of(targetPoint)); when(communityTariffRepository.findByEnergyCommunityId(communityId)) .thenReturn(Optional.of(communityTariff)); - when(membershipRepository.findByUserIdWithDetails(sourceUserId)) - .thenReturn(List.of(sourceMembership)); - when(membershipRepository.findByUserIdWithDetails(targetUserId)) - .thenReturn(List.of(targetMembership)); + when(membershipRepository.isActiveMemberOfCommunity(sourceUserId, communityId)).thenReturn(true); + when(membershipRepository.isActiveMemberOfCommunity(targetUserId, communityId)).thenReturn(true); when(userTariffRepository.save(any(UserTariff.class))).thenReturn(existingTariff); UserTariffRequest request = new UserTariffRequest( @@ -513,10 +506,8 @@ class TariffServiceTest { .thenReturn(Optional.of(targetPoint)); when(communityTariffRepository.findByEnergyCommunityId(communityId)) .thenReturn(Optional.of(communityTariff)); - when(membershipRepository.findByUserIdWithDetails(sourceUserId)) - .thenReturn(List.of(sourceMembership)); - when(membershipRepository.findByUserIdWithDetails(targetUserId)) - .thenReturn(List.of(targetMembership)); + when(membershipRepository.isActiveMemberOfCommunity(sourceUserId, communityId)).thenReturn(true); + when(membershipRepository.isActiveMemberOfCommunity(targetUserId, communityId)).thenReturn(true); UserTariff savedTariff = new UserTariff(); savedTariff.setId(UUID.randomUUID()); @@ -558,8 +549,7 @@ class TariffServiceTest { .thenReturn(Optional.of(targetPoint)); when(communityTariffRepository.findByEnergyCommunityId(communityId)) .thenReturn(Optional.of(communityTariff)); - when(membershipRepository.findByUserIdWithDetails(sourceUserId)) - .thenReturn(List.of(sourceMembership)); + when(membershipRepository.isActiveMemberOfCommunity(sourceUserId, communityId)).thenReturn(true); UserTariffRequest request = new UserTariffRequest( communityId, sourceMeteringPointId, targetMeteringPointId, @@ -598,10 +588,8 @@ class TariffServiceTest { .thenReturn(Optional.of(targetPoint)); when(communityTariffRepository.findByEnergyCommunityId(communityId)) .thenReturn(Optional.of(communityTariff)); - when(membershipRepository.findByUserIdWithDetails(sourceUserId)) - .thenReturn(List.of(sourceMembership)); - when(membershipRepository.findByUserIdWithDetails(targetUserId)) - .thenReturn(List.of(targetMembership)); + when(membershipRepository.isActiveMemberOfCommunity(sourceUserId, communityId)).thenReturn(true); + when(membershipRepository.isActiveMemberOfCommunity(targetUserId, communityId)).thenReturn(true); UserTariff savedTariff = new UserTariff(); savedTariff.setId(UUID.randomUUID()); diff --git a/eeg_frontend/openapi.yaml b/eeg_frontend/openapi.yaml index acb58fe..101eaa1 100644 --- a/eeg_frontend/openapi.yaml +++ b/eeg_frontend/openapi.yaml @@ -110,12 +110,6 @@ paths: schema: type: string format: uuid - - name: userId - in: query - required: true - schema: - type: string - format: uuid responses: "200": description: OK