Spring boot loadbyusername

Question

I have the following classes. I wonder if this structure makes sense in any terms especially the usage of the UserDetails class from Spring. Should I use loadbyusername method to create and remove methods in account service or is it okay? Then where should we use loadbyusername method?

Base class to cover fundamental properties like id, createdAt, etc.

@Accessors(chain = true)
@Data
@MappedSuperclass
public class Base {
@Id
private String id = UUID.randomUUID().toString();
@CreatedDate
private Date createdAt;
@LastModifiedDate
private Date updatedAt;
}

Account class

@Entity
@JsonIgnoreProperties(value = {"createdAt", "updatedAt"})
@Data
public class Account extends Base {
@NotNull
private String username;
@NotNull
private String password;
@NotNull
@Email
private String email;
@NotNull
private Role role;
}

Account details class implementing userdetails

public class AccountDetails implements UserDetails {
private Account account;
public AccountDetails(Account account) {
 this.account = account;
}
@Override
public Collection<? extends GrantedAuthority> getAuthorities() {
 return null;
}
@Override
public String getPassword() {
 return account.getPassword();
}
@Override
public String getUsername() {
 return account.getUsername();
}
@Override
public boolean isAccountNonExpired() {
 return true;
}
@Override
public boolean isAccountNonLocked() {
 return true;
}
@Override
public boolean isCredentialsNonExpired() {
 return true;
}
@Override
public boolean isEnabled() {
 return true;
}
}

Account service

@Service
public class AccountService {
@Autowired
AccountRepository accountRepository;
@Autowired
BCryptPasswordEncoder bCryptPasswordEncoder;
@Autowired
ModelMapper modelMapper;
public boolean create(AccountDTO accountDTO) {
 if (usernameExists(accountDTO.getUsername())) {
 throw new UsernameExistsException();
 }
 Account account = modelMapper.map(accountDTO, Account.class);
 account.setPassword(bCryptPasswordEncoder.encode(account.getPassword()));
 account.setId(UUID.randomUUID().toString());
 account.setRole(Role.USER);
 if (accountRepository.save(account) != null) {
 return true;
 } else {
 throw new RuntimeException();
 }
}
public boolean remove(AccountDTO accountDTO) {
 if (!usernameExists(accountDTO.getUsername())) {
 throw new UsernameNotFoundException("Account named " + accountDTO.getUsername() + " not found");
 }
 Optional<Account> account = accountRepository.findByUsername(accountDTO.getUsername());
 accountRepository.delete(account.get());
 if (!usernameExists(accountDTO.getUsername())) {
 return true;
 }
 return false;
}
public boolean usernameExists(String username) {
 return accountRepository.findByUsername(username).isPresent();
}
}

AccountDetailsService implementing userdetailsservice

@Service
public class AccountDetailsService implements UserDetailsService {
@Autowired
AccountRepository accountRepository;
@Override
public UserDetails loadUserByUsername(String s) throws UsernameNotFoundException {
 Optional<Account> optionalAccount = accountRepository.findByUsername(s);
 if (!optionalAccount.isPresent()) {
 throw new UsernameNotFoundException("Account named " + s + " not found");
 }
 return new AccountDetails(optionalAccount.get());
}
}

Answer 1

Some tips:

Tip 1

@Autowired
AccountRepository accountRepository;
@Autowired
BCryptPasswordEncoder bCryptPasswordEncoder;
...

Setter injection is considered as bad practice prefer constructor injection to gain more control and to avoid NullPointerExceptions. Why field injection is evil

Tip 2

@Id
private String id = UUID.randomUUID().toString();

Encapsulate id generation into some component class eg. IDGenerator.nextId() where IDGenerator is an interface and you would provide some implementation class for that. You'll have more control in testing.

Tip 3

Optional<Account> optionalAccount = accountRepository.findByUsername(s);
if (!optionalAccount.isPresent()) {
 throw new UsernameNotFoundException("Account named " + s + " not found");
}
return new AccountDetails(optionalAccount.get());

Using Optional like that is considered as bad practice. Something like that would be a lot better:

Account account = accountRepository.findByUsername(s).orElseThrow(UsernameNotFoundException::new)
return new AccountDetails(account)

Tip 4

throw new RuntimeException();

You should throw more specific exceptions.

Tip 5 UPDATE

I think that AccountService.remove(AccountDTO accountDTO) is too complex. I believe in that case would be better to just write own deletion method in repository:

public void remove(AccountDTO accountDTO) {
 Objects.requireNonNull(accountDTO);
 accountRepository.deleteByUsername(accountDTO.getUsername());
}

Tip 6

AccountService and AccountDetailsService could be as one class, these classes have similar responsibilities.

Tip 7

Then where should we use loadbyusername method?

Spring security needs class which implements this interface when we use some other user data store than in memory. When some client app try to login / generate token this method would be invoked by spring and whether if user exists or not spring would respond with 400 Bad Request or 200 OK.

• John Spring: Thanks for great tips. How can I reduce complexity of remove method in service?
• lukascode: @JohnSpring I updated Tip 5.
• John Spring: is it problematic if we dont check if the username is in db or not before deleting?
• lukascode: I think it depends on the business case but In general it is not a problem. Notice that we expect that after this remove method ends we should not see particular user account on the database and that is true, if account exists it would be deleted if not nothing happens and thats ok because that is expected behaviour (particular user account should not exists on db).
• John Spring: also do I still need accountdetails class after I merged accountservice and accountdetailsservice