From e615d5826242b238b241111d4e2f2af0c6115115 Mon Sep 17 00:00:00 2001 From: kapcake Date: Sun, 14 May 2023 00:42:53 +0200 Subject: [PATCH] Make user not accessible from UserDetails --- .../security/UserDetailsImpl.java | 11 +++++- .../security/UserDetailsServiceImpl.java | 2 - .../services/PaymentService.java | 7 ++-- .../validation/PaymentValidator.java | 14 +++---- .../services/PaymentServiceTest.java | 6 +-- .../validation/PaymentValidatorTest.java | 37 ++++++++----------- 6 files changed, 37 insertions(+), 40 deletions(-) diff --git a/src/main/java/net/kapcake/bankingservice/security/UserDetailsImpl.java b/src/main/java/net/kapcake/bankingservice/security/UserDetailsImpl.java index 29e5971..125d513 100644 --- a/src/main/java/net/kapcake/bankingservice/security/UserDetailsImpl.java +++ b/src/main/java/net/kapcake/bankingservice/security/UserDetailsImpl.java @@ -1,12 +1,16 @@ package net.kapcake.bankingservice.security; +import lombok.AllArgsConstructor; import net.kapcake.bankingservice.model.domain.User; +import org.springframework.security.core.CredentialsContainer; import org.springframework.security.core.GrantedAuthority; import org.springframework.security.core.userdetails.UserDetails; import java.util.Collection; -public record UserDetailsImpl(User user) implements UserDetails { +@AllArgsConstructor +public class UserDetailsImpl implements UserDetails, CredentialsContainer { + private User user; @Override public Collection getAuthorities() { @@ -42,4 +46,9 @@ public record UserDetailsImpl(User user) implements UserDetails { public boolean isEnabled() { return true; } + + @Override + public void eraseCredentials() { + user.setPassword(null); + } } diff --git a/src/main/java/net/kapcake/bankingservice/security/UserDetailsServiceImpl.java b/src/main/java/net/kapcake/bankingservice/security/UserDetailsServiceImpl.java index 471cde5..4fb6c9e 100644 --- a/src/main/java/net/kapcake/bankingservice/security/UserDetailsServiceImpl.java +++ b/src/main/java/net/kapcake/bankingservice/security/UserDetailsServiceImpl.java @@ -6,7 +6,6 @@ import org.springframework.security.core.userdetails.UserDetails; import org.springframework.security.core.userdetails.UserDetailsService; import org.springframework.security.core.userdetails.UsernameNotFoundException; import org.springframework.stereotype.Service; -import org.springframework.transaction.annotation.Transactional; @Service public class UserDetailsServiceImpl implements UserDetailsService { @@ -17,7 +16,6 @@ public class UserDetailsServiceImpl implements UserDetailsService { } @Override - @Transactional public UserDetails loadUserByUsername(String username) throws UsernameNotFoundException { User user = userRepository.findByUsername(username) .orElseThrow(() -> new UsernameNotFoundException("User (" + username + ") does not exist")); diff --git a/src/main/java/net/kapcake/bankingservice/services/PaymentService.java b/src/main/java/net/kapcake/bankingservice/services/PaymentService.java index 50c7c64..de6c316 100644 --- a/src/main/java/net/kapcake/bankingservice/services/PaymentService.java +++ b/src/main/java/net/kapcake/bankingservice/services/PaymentService.java @@ -45,7 +45,8 @@ public class PaymentService { public PaymentDTO createPayment(UserDetailsImpl authenticatedUser, PaymentDTO paymentDTO) { Payment payment = paymentConverter.mapToEntity(paymentDTO); - paymentValidator.validate(authenticatedUser.user(), payment); + List userBankAccounts = bankAccountRepository.findAllByUsers_username(authenticatedUser.getUsername()); + paymentValidator.validate(userBankAccounts, payment); BankAccount giverAccount = payment.getGiverAccount(); Optional beneficiaryAccount = bankAccountRepository.findByAccountNumber(payment.getBeneficiaryAccountNumber()); @@ -79,7 +80,7 @@ public class PaymentService { } public List getPaymentsForUser(UserDetailsImpl authenticatedUser, PaymentFilter paymentFilter) { - List userBankAccounts = authenticatedUser.user().getBankAccounts(); + List userBankAccounts = bankAccountRepository.findAllByUsers_username(authenticatedUser.getUsername()); List userPayments = paymentRepository.findAllByGiverAccountIn(userBankAccounts); List filteredPayments = getFilteredPayments(paymentFilter, userPayments); @@ -111,7 +112,7 @@ public class PaymentService { } public void deletePayment(UserDetailsImpl authenticatedUser, Long id) { - List userBankAccounts = authenticatedUser.user().getBankAccounts(); + List userBankAccounts = bankAccountRepository.findAllByUsers_username(authenticatedUser.getUsername()); Payment payment = paymentRepository.findById(id).orElseThrow(() -> new ResourceNotFoundException("A payment with the given id does not exist")); if (userBankAccounts.stream().noneMatch(account -> account.getId().equals(payment.getGiverAccount().getId()))) { throw new AccessDeniedException("The payment with id [" + id + "] is not owned by the authenticated user"); diff --git a/src/main/java/net/kapcake/bankingservice/validation/PaymentValidator.java b/src/main/java/net/kapcake/bankingservice/validation/PaymentValidator.java index 1c69d6e..d3aad7b 100644 --- a/src/main/java/net/kapcake/bankingservice/validation/PaymentValidator.java +++ b/src/main/java/net/kapcake/bankingservice/validation/PaymentValidator.java @@ -1,11 +1,7 @@ package net.kapcake.bankingservice.validation; import net.kapcake.bankingservice.exceptions.ValidationException; -import net.kapcake.bankingservice.model.domain.BalanceType; -import net.kapcake.bankingservice.model.domain.IbanValidationResponse; -import net.kapcake.bankingservice.model.domain.Balance; -import net.kapcake.bankingservice.model.domain.Payment; -import net.kapcake.bankingservice.model.domain.User; +import net.kapcake.bankingservice.model.domain.*; import org.springframework.beans.factory.annotation.Value; import org.springframework.stereotype.Component; import org.springframework.web.client.RestClientException; @@ -29,14 +25,14 @@ public class PaymentValidator { } - public void validate(User user, Payment payment) throws ValidationException { - validateGiverAccount(user, payment); + public void validate(List userBankAccounts, Payment payment) throws ValidationException { + validateGiverAccount(userBankAccounts, payment); validateBeneficiaryAccount(payment); validateAccountBalance(payment); } - private static void validateGiverAccount(User user, Payment payment) { - boolean userOwnsGiverAccount = user.getBankAccounts().stream().anyMatch(bankAccount -> bankAccount.getId().equals(payment.getGiverAccount().getId())); + private static void validateGiverAccount(List userBankAccounts, Payment payment) { + boolean userOwnsGiverAccount = userBankAccounts.stream().anyMatch(bankAccount -> bankAccount.getId().equals(payment.getGiverAccount().getId())); if (!userOwnsGiverAccount) { throw new ValidationException("Giver account not owned by authenticated user."); } diff --git a/src/test/java/net/kapcake/bankingservice/services/PaymentServiceTest.java b/src/test/java/net/kapcake/bankingservice/services/PaymentServiceTest.java index 99b0dba..2ca4ac4 100644 --- a/src/test/java/net/kapcake/bankingservice/services/PaymentServiceTest.java +++ b/src/test/java/net/kapcake/bankingservice/services/PaymentServiceTest.java @@ -90,7 +90,6 @@ class PaymentServiceTest { @Test void getPaymentsForUser() { List bankAccounts = List.of(bankAccount1); - user1.setBankAccounts(bankAccounts); UserDetailsImpl authenticatedUser = new UserDetailsImpl(user1); Payment payment1 = new Payment().setId(1L).setCreationDate(new GregorianCalendar(2023, Calendar.MAY, 11, 12, 0).getTime()).setAmount(BigDecimal.valueOf(50)).setCurrency(Currency.EUR).setBeneficiaryAccountNumber(ACCOUNT_NUMBER_2).setGiverAccount(bankAccount1); PaymentDTO paymentDTO1 = new PaymentDTO().setId(1L).setCreationDate(new GregorianCalendar(2023, Calendar.MAY, 11, 12, 0).getTime()).setAmount(BigDecimal.valueOf(50)).setCurrency(Currency.EUR).setBeneficiaryAccountNumber(ACCOUNT_NUMBER_2).setGiverAccount(1L); @@ -99,6 +98,7 @@ class PaymentServiceTest { Payment payment3 = new Payment().setId(3L).setCreationDate(new GregorianCalendar(2023, Calendar.MAY, 11, 12, 2).getTime()).setAmount(BigDecimal.valueOf(50)).setCurrency(Currency.EUR).setBeneficiaryAccountNumber(ACCOUNT_NUMBER_3).setGiverAccount(bankAccount1); PaymentDTO paymentDTO3 = new PaymentDTO().setId(3L).setCreationDate(new GregorianCalendar(2023, Calendar.MAY, 11, 12, 2).getTime()).setAmount(BigDecimal.valueOf(50)).setCurrency(Currency.EUR).setBeneficiaryAccountNumber(ACCOUNT_NUMBER_3).setGiverAccount(1L); + when(bankAccountRepository.findAllByUsers_username(TEST_USER_1)).thenReturn(bankAccounts); when(paymentRepository.findAllByGiverAccountIn(bankAccounts)).thenReturn(Arrays.asList(payment1, payment2, payment3)); when(paymentConverter.mapToDTO(payment1)).thenReturn(paymentDTO1); when(paymentConverter.mapToDTO(payment2)).thenReturn(paymentDTO2); @@ -180,14 +180,14 @@ class PaymentServiceTest { @Test void deletePayment() { List bankAccounts1 = List.of(bankAccount1); - user1.setBankAccounts(bankAccounts1); List bankAccounts2 = List.of(bankAccount2); - user2.setBankAccounts(bankAccounts2); UserDetailsImpl authenticatedUser1 = new UserDetailsImpl(user1); UserDetailsImpl authenticatedUser2 = new UserDetailsImpl(user2); Payment payment1 = new Payment().setId(1L).setAmount(BigDecimal.valueOf(50)).setCurrency(Currency.EUR).setBeneficiaryAccountNumber(ACCOUNT_NUMBER_2).setGiverAccount(bankAccount1); Payment payment2 = new Payment().setId(2L).setAmount(BigDecimal.valueOf(50)).setCurrency(Currency.EUR).setBeneficiaryAccountNumber(ACCOUNT_NUMBER_2).setGiverAccount(bankAccount1).setStatus(PaymentStatus.EXECUTED); + when(bankAccountRepository.findAllByUsers_username(TEST_USER_1)).thenReturn(bankAccounts1); + when(bankAccountRepository.findAllByUsers_username(TEST_USER_2)).thenReturn(bankAccounts2); when(paymentRepository.findById(1L)).thenReturn(Optional.of(payment1)); when(paymentRepository.findById(2L)).thenReturn(Optional.of(payment2)); when(paymentRepository.findById(3L)).thenReturn(Optional.empty()); diff --git a/src/test/java/net/kapcake/bankingservice/validation/PaymentValidatorTest.java b/src/test/java/net/kapcake/bankingservice/validation/PaymentValidatorTest.java index 23747e0..850facb 100644 --- a/src/test/java/net/kapcake/bankingservice/validation/PaymentValidatorTest.java +++ b/src/test/java/net/kapcake/bankingservice/validation/PaymentValidatorTest.java @@ -4,13 +4,7 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import net.kapcake.bankingservice.config.BankingServiceConfig; import net.kapcake.bankingservice.exceptions.ValidationException; -import net.kapcake.bankingservice.model.domain.BalanceType; -import net.kapcake.bankingservice.model.domain.Currency; -import net.kapcake.bankingservice.model.domain.IbanValidationResponse; -import net.kapcake.bankingservice.model.domain.Balance; -import net.kapcake.bankingservice.model.domain.BankAccount; -import net.kapcake.bankingservice.model.domain.Payment; -import net.kapcake.bankingservice.model.domain.User; +import net.kapcake.bankingservice.model.domain.*; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -49,8 +43,8 @@ class PaymentValidatorTest { private final ObjectMapper objectMapper; private BankAccount bankAccount; private Payment payment; - private User user; private Balance balance; + private List userBankAccounts; @Autowired public PaymentValidatorTest(RestTemplate restTemplate, MockRestServiceServer server) { @@ -61,15 +55,16 @@ class PaymentValidatorTest { @BeforeEach void setUp() throws JsonProcessingException { - user = new User().setId(1L).setUsername(USERNAME); + User user = new User().setId(1L).setUsername(USERNAME); balance = new Balance().setId(1L).setType(BalanceType.AVAILABLE); bankAccount = new BankAccount().setId(1L).setBalances(Collections.singletonList(balance)); payment = new Payment(); // Set up valid payment + userBankAccounts = Collections.singletonList(bankAccount); bankAccount.setAccountNumber(GIVER_ACCOUNT_NUMBER).setUsers(Collections.singletonList(user)); balance.setCurrency(Currency.EUR).setAmount(BigDecimal.valueOf(5.5)); - user.setBankAccounts(Collections.singletonList(bankAccount)); + user.setBankAccounts(userBankAccounts); payment.setGiverAccount(bankAccount).setCurrency(Currency.EUR).setBeneficiaryAccountNumber(VALID_BENEFICIARY).setAmount(BigDecimal.valueOf(4)); server @@ -81,14 +76,12 @@ class PaymentValidatorTest { @Test void validate_correctPayment() { - paymentValidator.validate(user, payment); + paymentValidator.validate(userBankAccounts, payment); } @Test void validate_invalidGiverAccount() { - user.setBankAccounts(Collections.emptyList()); - - ValidationException validationException = Assertions.assertThrows(ValidationException.class, () -> paymentValidator.validate(user, payment)); + ValidationException validationException = Assertions.assertThrows(ValidationException.class, () -> paymentValidator.validate(Collections.emptyList(), payment)); Assertions.assertEquals("Giver account not owned by authenticated user.", validationException.getMessage()); } @@ -101,7 +94,7 @@ class PaymentValidatorTest { .andExpect(method(HttpMethod.GET)) .andRespond(withServiceUnavailable()); - ValidationException validationException = Assertions.assertThrows(ValidationException.class, () -> paymentValidator.validate(user, payment)); + ValidationException validationException = Assertions.assertThrows(ValidationException.class, () -> paymentValidator.validate(userBankAccounts, payment)); Assertions.assertEquals("Beneficiary account could not be validated.", validationException.getMessage()); server.reset(); @@ -110,7 +103,7 @@ class PaymentValidatorTest { .andExpect(method(HttpMethod.GET)) .andRespond(withSuccess()); - validationException = Assertions.assertThrows(ValidationException.class, () -> paymentValidator.validate(user, payment)); + validationException = Assertions.assertThrows(ValidationException.class, () -> paymentValidator.validate(userBankAccounts, payment)); Assertions.assertEquals("Beneficiary account could not be validated.", validationException.getMessage()); } @@ -123,7 +116,7 @@ class PaymentValidatorTest { .andExpect(method(HttpMethod.GET)) .andRespond(withSuccess(objectMapper.writeValueAsString(new IbanValidationResponse(false, Collections.singletonList("Invalid"), VALID_BENEFICIARY)), MediaType.APPLICATION_JSON)); - ValidationException validationException = Assertions.assertThrows(ValidationException.class, () -> paymentValidator.validate(user, payment)); + ValidationException validationException = Assertions.assertThrows(ValidationException.class, () -> paymentValidator.validate(userBankAccounts, payment)); Assertions.assertEquals("Beneficiary account not valid: [Invalid]", validationException.getMessage()); } @@ -131,7 +124,7 @@ class PaymentValidatorTest { void validate_invalidBeneficiaryAccount_giverAndBeneficiarySame() { bankAccount.setAccountNumber(VALID_BENEFICIARY); - ValidationException validationException = Assertions.assertThrows(ValidationException.class, () -> paymentValidator.validate(user, payment)); + ValidationException validationException = Assertions.assertThrows(ValidationException.class, () -> paymentValidator.validate(userBankAccounts, payment)); Assertions.assertEquals("Beneficiary and giver account are the same.", validationException.getMessage()); } @@ -144,7 +137,7 @@ class PaymentValidatorTest { .andExpect(method(HttpMethod.GET)) .andRespond(withSuccess(objectMapper.writeValueAsString(new IbanValidationResponse(true, Collections.emptyList(), FORBIDDEN_ACCOUNT_1)), MediaType.APPLICATION_JSON)); - ValidationException validationException = Assertions.assertThrows(ValidationException.class, () -> paymentValidator.validate(user, payment)); + ValidationException validationException = Assertions.assertThrows(ValidationException.class, () -> paymentValidator.validate(userBankAccounts, payment)); Assertions.assertEquals("Beneficiary account is forbidden.", validationException.getMessage()); } @@ -152,7 +145,7 @@ class PaymentValidatorTest { void validate_invalidAccountBalance_notFound() { balance.setType(BalanceType.END_OF_DAY); - ValidationException validationException = Assertions.assertThrows(ValidationException.class, () -> paymentValidator.validate(user, payment)); + ValidationException validationException = Assertions.assertThrows(ValidationException.class, () -> paymentValidator.validate(userBankAccounts, payment)); Assertions.assertEquals("Unable to retrieve available account balance.", validationException.getMessage()); } @@ -160,7 +153,7 @@ class PaymentValidatorTest { void validate_invalidAccountBalance_differentCurrencies() { balance.setCurrency(Currency.USD); - ValidationException validationException = Assertions.assertThrows(ValidationException.class, () -> paymentValidator.validate(user, payment)); + ValidationException validationException = Assertions.assertThrows(ValidationException.class, () -> paymentValidator.validate(userBankAccounts, payment)); Assertions.assertEquals("Payment and account currency must be the same.", validationException.getMessage()); } @@ -168,7 +161,7 @@ class PaymentValidatorTest { void validate_invalidAccountBalance_insufficientBalance() { balance.setAmount(BigDecimal.valueOf(2)); - ValidationException validationException = Assertions.assertThrows(ValidationException.class, () -> paymentValidator.validate(user, payment)); + ValidationException validationException = Assertions.assertThrows(ValidationException.class, () -> paymentValidator.validate(userBankAccounts, payment)); Assertions.assertEquals("Available account balance not sufficient.", validationException.getMessage()); } } \ No newline at end of file