From 474ab344b6a53fb8349965deb1f80fd0df239a8c Mon Sep 17 00:00:00 2001 From: Pavel Papou Date: Thu, 12 Oct 2023 09:23:24 -0400 Subject: [PATCH 1/2] Code review name remarks - Impl name titles have been removed --- .../example/secureapplication/rest/AppController.java | 6 +++--- ...ailsServiceImpl.java => AppUserDetailsService.java} | 10 +++++----- .../user/{UserDetailsImpl.java => AppUserDetails.java} | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) rename src/main/java/com/example/secureapplication/service/{UserDetailsServiceImpl.java => AppUserDetailsService.java} (76%) rename src/main/java/com/example/secureapplication/user/{UserDetailsImpl.java => AppUserDetails.java} (96%) diff --git a/src/main/java/com/example/secureapplication/rest/AppController.java b/src/main/java/com/example/secureapplication/rest/AppController.java index 6cbb454..832695a 100644 --- a/src/main/java/com/example/secureapplication/rest/AppController.java +++ b/src/main/java/com/example/secureapplication/rest/AppController.java @@ -3,7 +3,7 @@ import com.example.secureapplication.data.AboutApp; import com.example.secureapplication.data.AppInfo; -import com.example.secureapplication.service.UserDetailsServiceImpl; +import com.example.secureapplication.service.AppUserDetailsService; import com.example.secureapplication.user.User; import lombok.extern.slf4j.Slf4j; import org.springframework.web.bind.annotation.GetMapping; @@ -21,9 +21,9 @@ public class AppController { private final AppInfo appInfo; private final AboutApp aboutApp; - private final UserDetailsServiceImpl userDetailsService; + private final AppUserDetailsService userDetailsService; - public AppController(AppInfo appInfo, AboutApp aboutApp, UserDetailsServiceImpl userDetailsService) { + public AppController(AppInfo appInfo, AboutApp aboutApp, AppUserDetailsService userDetailsService) { this.appInfo = appInfo; this.aboutApp = aboutApp; diff --git a/src/main/java/com/example/secureapplication/service/UserDetailsServiceImpl.java b/src/main/java/com/example/secureapplication/service/AppUserDetailsService.java similarity index 76% rename from src/main/java/com/example/secureapplication/service/UserDetailsServiceImpl.java rename to src/main/java/com/example/secureapplication/service/AppUserDetailsService.java index 41cf8c1..4792261 100644 --- a/src/main/java/com/example/secureapplication/service/UserDetailsServiceImpl.java +++ b/src/main/java/com/example/secureapplication/service/AppUserDetailsService.java @@ -2,17 +2,17 @@ import com.example.secureapplication.repo.UserRepository; import com.example.secureapplication.user.User; -import com.example.secureapplication.user.UserDetailsImpl; +import com.example.secureapplication.user.AppUserDetails; 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; @Service -public class UserDetailsServiceImpl implements UserDetailsService { +public class AppUserDetailsService implements UserDetailsService { private final UserRepository userRepository; - public UserDetailsServiceImpl(UserRepository userRepository) { + public AppUserDetailsService(UserRepository userRepository) { this.userRepository = userRepository; } @@ -22,11 +22,11 @@ public UserDetails loadUserByUsername(String username) throws UsernameNotFoundEx if (user == null) { throw new UsernameNotFoundException("User not found"); } - return new UserDetailsImpl(user); + return new AppUserDetails(user); } public UserDetails createUser(User user) { User newUser = userRepository.save(user); - return new UserDetailsImpl(newUser); + return new AppUserDetails(newUser); } } \ No newline at end of file diff --git a/src/main/java/com/example/secureapplication/user/UserDetailsImpl.java b/src/main/java/com/example/secureapplication/user/AppUserDetails.java similarity index 96% rename from src/main/java/com/example/secureapplication/user/UserDetailsImpl.java rename to src/main/java/com/example/secureapplication/user/AppUserDetails.java index f271af4..256e328 100644 --- a/src/main/java/com/example/secureapplication/user/UserDetailsImpl.java +++ b/src/main/java/com/example/secureapplication/user/AppUserDetails.java @@ -13,7 +13,7 @@ @AllArgsConstructor @NoArgsConstructor -public class UserDetailsImpl implements UserDetails { +public class AppUserDetails implements UserDetails { private User user ; @Override public Collection getAuthorities() { From 8b76ec03be15003f519c6d0a3d2bc227eb94d338 Mon Sep 17 00:00:00 2001 From: Pavel Papou Date: Sat, 14 Oct 2023 15:10:24 -0400 Subject: [PATCH 2/2] Review points - meaningfull classes names - rest and sevice layers have been decoupled --- .../service/AccountMapperImpl.java | 55 +++++++++++++++++++ .../config/MappingConfig.java | 8 +++ ...ecurityConfig.java => SecurityConfig.java} | 8 +-- .../{UserRepository.java => AccountDB.java} | 6 +- .../repo/{RoleRepository.java => RoleDB.java} | 2 +- .../secureapplication/rest/AppController.java | 12 ++-- .../service/AccountDetails.java | 33 +++++++++++ .../service/AccountMapper.java | 15 +++++ .../service/AccountServiceLayerRecord.java | 24 ++++++++ .../service/AppUserDetailsService.java | 32 ----------- ...ppUserDetails.java => AccountDetails.java} | 10 ++-- .../user/{User.java => AccountRecord.java} | 6 +- 12 files changed, 152 insertions(+), 59 deletions(-) create mode 100644 build/generated/sources/annotationProcessor/java/main/com/example/secureapplication/service/AccountMapperImpl.java create mode 100644 src/main/java/com/example/secureapplication/config/MappingConfig.java rename src/main/java/com/example/secureapplication/config/{SecSecurityConfig.java => SecurityConfig.java} (82%) rename src/main/java/com/example/secureapplication/repo/{UserRepository.java => AccountDB.java} (52%) rename src/main/java/com/example/secureapplication/repo/{RoleRepository.java => RoleDB.java} (69%) create mode 100644 src/main/java/com/example/secureapplication/service/AccountDetails.java create mode 100644 src/main/java/com/example/secureapplication/service/AccountMapper.java create mode 100644 src/main/java/com/example/secureapplication/service/AccountServiceLayerRecord.java delete mode 100644 src/main/java/com/example/secureapplication/service/AppUserDetailsService.java rename src/main/java/com/example/secureapplication/user/{AppUserDetails.java => AccountDetails.java} (83%) rename src/main/java/com/example/secureapplication/user/{User.java => AccountRecord.java} (85%) diff --git a/build/generated/sources/annotationProcessor/java/main/com/example/secureapplication/service/AccountMapperImpl.java b/build/generated/sources/annotationProcessor/java/main/com/example/secureapplication/service/AccountMapperImpl.java new file mode 100644 index 0000000..8867fb8 --- /dev/null +++ b/build/generated/sources/annotationProcessor/java/main/com/example/secureapplication/service/AccountMapperImpl.java @@ -0,0 +1,55 @@ +package com.example.secureapplication.service; + +import com.example.secureapplication.user.AccountRecord; +import com.example.secureapplication.user.Role; +import java.util.LinkedHashSet; +import java.util.Set; +import javax.annotation.processing.Generated; +import org.springframework.stereotype.Component; + +@Generated( + value = "org.mapstruct.ap.MappingProcessor", + date = "2023-10-15T09:59:06-0400", + comments = "version: 1.5.5.Final, compiler: IncrementalProcessingEnvironment from gradle-language-java-8.2.1.jar, environment: Java 17.0.8.1 (Amazon.com Inc.)" +) +@Component +public class AccountMapperImpl implements AccountMapper { + + @Override + public AccountServiceLayerRecord toService(AccountRecord user) { + if ( user == null ) { + return null; + } + + AccountServiceLayerRecord.AccountServiceLayerRecordBuilder accountServiceLayerRecord = AccountServiceLayerRecord.builder(); + + accountServiceLayerRecord.id( user.getId() ); + accountServiceLayerRecord.username( user.getUsername() ); + accountServiceLayerRecord.password( user.getPassword() ); + accountServiceLayerRecord.email( user.getEmail() ); + accountServiceLayerRecord.enabled( user.isEnabled() ); + + return accountServiceLayerRecord.build(); + } + + @Override + public AccountRecord toRepo(AccountServiceLayerRecord user) { + if ( user == null ) { + return null; + } + + AccountRecord accountRecord = new AccountRecord(); + + accountRecord.setId( user.getId() ); + accountRecord.setUsername( user.getUsername() ); + accountRecord.setPassword( user.getPassword() ); + accountRecord.setEmail( user.getEmail() ); + Set set = user.getRoles(); + if ( set != null ) { + accountRecord.setRoles( new LinkedHashSet( set ) ); + } + accountRecord.setEnabled( user.isEnabled() ); + + return accountRecord; + } +} diff --git a/src/main/java/com/example/secureapplication/config/MappingConfig.java b/src/main/java/com/example/secureapplication/config/MappingConfig.java new file mode 100644 index 0000000..6aed838 --- /dev/null +++ b/src/main/java/com/example/secureapplication/config/MappingConfig.java @@ -0,0 +1,8 @@ +package com.example.secureapplication.config; + +import org.mapstruct.MapperConfig; +import org.springframework.context.annotation.Configuration; + +@Configuration +@MapperConfig(componentModel = "spring") +public class MappingConfig {} \ No newline at end of file diff --git a/src/main/java/com/example/secureapplication/config/SecSecurityConfig.java b/src/main/java/com/example/secureapplication/config/SecurityConfig.java similarity index 82% rename from src/main/java/com/example/secureapplication/config/SecSecurityConfig.java rename to src/main/java/com/example/secureapplication/config/SecurityConfig.java index 66aa4c2..174f42e 100644 --- a/src/main/java/com/example/secureapplication/config/SecSecurityConfig.java +++ b/src/main/java/com/example/secureapplication/config/SecurityConfig.java @@ -5,16 +5,10 @@ import org.springframework.context.annotation.Configuration; import org.springframework.security.config.annotation.web.builders.HttpSecurity; import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; -import org.springframework.security.core.userdetails.User; -import org.springframework.security.core.userdetails.UserDetails; import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder; import org.springframework.security.crypto.password.PasswordEncoder; -import org.springframework.security.provisioning.InMemoryUserDetailsManager; import org.springframework.security.web.SecurityFilterChain; -import org.springframework.security.web.servlet.util.matcher.MvcRequestMatcher; -import org.springframework.web.servlet.handler.HandlerMappingIntrospector; -import static org.springframework.security.config.Customizer.withDefaults; import static org.springframework.security.web.util.matcher.AntPathRequestMatcher.antMatcher; //TODO:Remove the logger @@ -23,7 +17,7 @@ @Slf4j @Configuration @EnableWebSecurity -public class SecSecurityConfig { +public class SecurityConfig { /* @Bean public InMemoryUserDetailsManager userDetailsService() { diff --git a/src/main/java/com/example/secureapplication/repo/UserRepository.java b/src/main/java/com/example/secureapplication/repo/AccountDB.java similarity index 52% rename from src/main/java/com/example/secureapplication/repo/UserRepository.java rename to src/main/java/com/example/secureapplication/repo/AccountDB.java index ec13e61..ab5af57 100644 --- a/src/main/java/com/example/secureapplication/repo/UserRepository.java +++ b/src/main/java/com/example/secureapplication/repo/AccountDB.java @@ -1,12 +1,12 @@ package com.example.secureapplication.repo; -import com.example.secureapplication.user.User; +import com.example.secureapplication.user.AccountRecord; import org.springframework.data.jpa.repository.JpaRepository; import org.springframework.data.jpa.repository.JpaSpecificationExecutor; import org.springframework.stereotype.Repository; @Repository -public interface UserRepository extends JpaRepository, JpaSpecificationExecutor { - User findByUsername(String username); +public interface AccountDB extends JpaRepository, JpaSpecificationExecutor { + AccountRecord findByUsername(String username); } diff --git a/src/main/java/com/example/secureapplication/repo/RoleRepository.java b/src/main/java/com/example/secureapplication/repo/RoleDB.java similarity index 69% rename from src/main/java/com/example/secureapplication/repo/RoleRepository.java rename to src/main/java/com/example/secureapplication/repo/RoleDB.java index f20490c..6ee9e7c 100644 --- a/src/main/java/com/example/secureapplication/repo/RoleRepository.java +++ b/src/main/java/com/example/secureapplication/repo/RoleDB.java @@ -3,5 +3,5 @@ import com.example.secureapplication.user.Role; import org.springframework.data.jpa.repository.JpaRepository; -public interface RoleRepository extends JpaRepository { +public interface RoleDB extends JpaRepository { } diff --git a/src/main/java/com/example/secureapplication/rest/AppController.java b/src/main/java/com/example/secureapplication/rest/AppController.java index 832695a..bda157a 100644 --- a/src/main/java/com/example/secureapplication/rest/AppController.java +++ b/src/main/java/com/example/secureapplication/rest/AppController.java @@ -3,8 +3,8 @@ import com.example.secureapplication.data.AboutApp; import com.example.secureapplication.data.AppInfo; -import com.example.secureapplication.service.AppUserDetailsService; -import com.example.secureapplication.user.User; +import com.example.secureapplication.service.AccountDetails; +import com.example.secureapplication.service.AccountServiceLayerRecord; import lombok.extern.slf4j.Slf4j; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PostMapping; @@ -21,9 +21,9 @@ public class AppController { private final AppInfo appInfo; private final AboutApp aboutApp; - private final AppUserDetailsService userDetailsService; + private final AccountDetails userDetailsService; - public AppController(AppInfo appInfo, AboutApp aboutApp, AppUserDetailsService userDetailsService) { + public AppController(AppInfo appInfo, AboutApp aboutApp, AccountDetails userDetailsService) { this.appInfo = appInfo; this.aboutApp = aboutApp; @@ -47,7 +47,7 @@ public void getUserInfo(){ } @PostMapping("/user") - public void createUser(@RequestBody User user) { - userDetailsService.createUser(user); + public void createUser(@RequestBody AccountServiceLayerRecord accountRecord) { + userDetailsService.createUser(accountRecord); } } diff --git a/src/main/java/com/example/secureapplication/service/AccountDetails.java b/src/main/java/com/example/secureapplication/service/AccountDetails.java new file mode 100644 index 0000000..4a11274 --- /dev/null +++ b/src/main/java/com/example/secureapplication/service/AccountDetails.java @@ -0,0 +1,33 @@ +package com.example.secureapplication.service; + +import com.example.secureapplication.repo.AccountDB; +import com.example.secureapplication.user.AccountRecord; +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; + +@Service +public class AccountDetails implements UserDetailsService { + private final AccountDB accountDB; + private final AccountMapper mapper; + + public AccountDetails(AccountDB accountDB, AccountMapper mapper) { + this.accountDB = accountDB; + this.mapper = mapper; + } + + @Override + public UserDetails loadUserByUsername(String username) throws UsernameNotFoundException { + AccountRecord accountRecord = accountDB.findByUsername(username); + if (accountRecord == null) { + throw new UsernameNotFoundException("User not found"); + } + return new com.example.secureapplication.user.AccountDetails(accountRecord); + } + + public UserDetails createUser(AccountServiceLayerRecord user) { + AccountRecord newAccountRecord = accountDB.save(mapper.toRepo(user)); + return new com.example.secureapplication.user.AccountDetails(newAccountRecord); + } +} \ No newline at end of file diff --git a/src/main/java/com/example/secureapplication/service/AccountMapper.java b/src/main/java/com/example/secureapplication/service/AccountMapper.java new file mode 100644 index 0000000..ccc0239 --- /dev/null +++ b/src/main/java/com/example/secureapplication/service/AccountMapper.java @@ -0,0 +1,15 @@ +package com.example.secureapplication.service; + + +import com.example.secureapplication.config.MappingConfig; +import com.example.secureapplication.user.AccountRecord; +import org.mapstruct.Mapper; +import org.springframework.stereotype.Component; + +@Mapper(config = MappingConfig.class) +@Component +public interface AccountMapper { + AccountServiceLayerRecord toService(AccountRecord user); + AccountRecord toRepo(AccountServiceLayerRecord user); + +} diff --git a/src/main/java/com/example/secureapplication/service/AccountServiceLayerRecord.java b/src/main/java/com/example/secureapplication/service/AccountServiceLayerRecord.java new file mode 100644 index 0000000..9fd533a --- /dev/null +++ b/src/main/java/com/example/secureapplication/service/AccountServiceLayerRecord.java @@ -0,0 +1,24 @@ +package com.example.secureapplication.service; + + +import com.example.secureapplication.user.Role; +import lombok.Builder; +import lombok.EqualsAndHashCode; +import lombok.Value; + +import java.util.HashSet; +import java.util.Set; + +@Value +@Builder +@EqualsAndHashCode +public class AccountServiceLayerRecord { + + @EqualsAndHashCode.Include + Long id; + String username; + String password; + String email; + Set roles = new HashSet<>(); + boolean enabled; +} diff --git a/src/main/java/com/example/secureapplication/service/AppUserDetailsService.java b/src/main/java/com/example/secureapplication/service/AppUserDetailsService.java deleted file mode 100644 index 4792261..0000000 --- a/src/main/java/com/example/secureapplication/service/AppUserDetailsService.java +++ /dev/null @@ -1,32 +0,0 @@ -package com.example.secureapplication.service; - -import com.example.secureapplication.repo.UserRepository; -import com.example.secureapplication.user.User; -import com.example.secureapplication.user.AppUserDetails; -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; - -@Service -public class AppUserDetailsService implements UserDetailsService { - private final UserRepository userRepository; - - public AppUserDetailsService(UserRepository userRepository) { - this.userRepository = userRepository; - } - - @Override - public UserDetails loadUserByUsername(String username) throws UsernameNotFoundException { - User user = userRepository.findByUsername(username); - if (user == null) { - throw new UsernameNotFoundException("User not found"); - } - return new AppUserDetails(user); - } - - public UserDetails createUser(User user) { - User newUser = userRepository.save(user); - return new AppUserDetails(newUser); - } -} \ No newline at end of file diff --git a/src/main/java/com/example/secureapplication/user/AppUserDetails.java b/src/main/java/com/example/secureapplication/user/AccountDetails.java similarity index 83% rename from src/main/java/com/example/secureapplication/user/AppUserDetails.java rename to src/main/java/com/example/secureapplication/user/AccountDetails.java index 256e328..e19efce 100644 --- a/src/main/java/com/example/secureapplication/user/AppUserDetails.java +++ b/src/main/java/com/example/secureapplication/user/AccountDetails.java @@ -13,11 +13,11 @@ @AllArgsConstructor @NoArgsConstructor -public class AppUserDetails implements UserDetails { - private User user ; +public class AccountDetails implements UserDetails { + private AccountRecord accountRecord; @Override public Collection getAuthorities() { - Set roles = user.getRoles(); + Set roles = accountRecord.getRoles(); List authorities = new ArrayList<>(); for (Role role : roles) { @@ -29,12 +29,12 @@ public Collection getAuthorities() { @Override public String getPassword() { - return user.getPassword(); + return accountRecord.getPassword(); } @Override public String getUsername() { - return user.getUsername(); + return accountRecord.getUsername(); } @Override diff --git a/src/main/java/com/example/secureapplication/user/User.java b/src/main/java/com/example/secureapplication/user/AccountRecord.java similarity index 85% rename from src/main/java/com/example/secureapplication/user/User.java rename to src/main/java/com/example/secureapplication/user/AccountRecord.java index a267e53..0adeaa0 100644 --- a/src/main/java/com/example/secureapplication/user/User.java +++ b/src/main/java/com/example/secureapplication/user/AccountRecord.java @@ -11,11 +11,7 @@ @Data @Entity @Table(name = "users") -/*@AllArgsConstructor -@NoArgsConstructor -@Getter -@Setter*/ -public class User implements Serializable { +public class AccountRecord implements Serializable { @Id @GeneratedValue(strategy = GenerationType.IDENTITY)