Springboot - Authentication Service

Question

I am developing a Web application using Spring boot (I am a beginner). How can I make my code better?

This code is from "AuthenticationService.java". I first made an interface for defining all methods, and then I implemented that interface in this AuthenticationService class.

package com.discwords.discwords.service; import com.discwords.discwords.model.*; import com.discwords.discwords.repository.ProfileRepo; import com.discwords.discwords.repository.SessionRepo; import com.discwords.discwords.repository.UserRepo; import com.discwords.discwords.repository.UserSecretRepo; import com.google.api.client.googleapis.auth.oauth2.GoogleIdToken; import com.google.api.client.googleapis.auth.oauth2.GoogleIdTokenVerifier; import com.google.api.client.googleapis.javanet.GoogleNetHttpTransport; import com.google.api.client.json.gson.GsonFactory; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Value; import org.springframework.http.HttpStatus; import org.springframework.security.crypto.bcrypt.BCrypt; import org.springframework.stereotype.Service; import org.springframework.web.server.ResponseStatusException; import java.util.Collections; import java.util.Date; import java.util.Optional; @Service public class AuthorizationServiceImpl implements AuthorizationService { @Autowired private UserRepo userRepo; @Autowired private UserSecretRepo userSecretRepo; @Autowired private SessionRepo sessionRepo; @Autowired private ProfileRepo profileRepo; @Autowired JWTService jwtService; @Value("${google_client_id}") private String CLIENT_ID; @Override public UserSessionDTO loginUser(UserDTO user) { Optional<User> userRes = userRepo.findByEmail(user.getEmail()); if (userRes.isEmpty()) { System.out.println("User not found"); return null; } User existingUser = userRes.get(); Optional<Profile> profileRes = profileRepo.findByUserId(existingUser.getUserId()); if(profileRes.isEmpty()){ throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "No Profile with this user id"); } Profile profile = profileRes.get(); Optional<UserSession> userSessionRes = sessionRepo.findByUserId(existingUser.getUserId()); if (userSessionRes.isPresent()) { UserSession userSession = userSessionRes.get(); if (userSession.getSessionEndTime().before(new Date())) { System.out.println("Deleting User session for new"); sessionRepo.delete(userSession); } else { System.out.println("User already logged in"); return new UserSessionDTO(profile, userSession.getToken()); } } Optional<UserSecret> userSecretRes = userSecretRepo.findById(existingUser.getUserId()); if (userSecretRes.isEmpty()) { System.out.println("User secret is empty"); return null; } UserSecret userSecret = userSecretRes.get(); if (BCrypt.checkpw(user.getPassword(), userSecret.getPassword())) { String jwtToken = jwtService.generateToken(existingUser.getEmail(), Long.toString(existingUser.getUserId())); int newSessionId = (int) (System.currentTimeMillis() / 100000); UserSession newUserSession = new UserSession( existingUser.getUserId(), jwtToken, new Date(), jwtService.extractExpiryDate(jwtToken), newSessionId ); UserSession userSession = sessionRepo.save(newUserSession); UserSessionDTO userSessionDTO = new UserSessionDTO(profile, userSession.getToken()); return userSessionDTO; } return null; } @Override public UserSessionDTO signupUser(UserDTO user) { // checking if email already exists Optional<User> userRes = userRepo.findByEmail(user.getEmail()); if (!userRes.isEmpty()) { throw new ResponseStatusException(HttpStatus.CONFLICT, "User already exist"); } // checking if username already exists Optional<User> tempUserRes = userRepo.findByUsername(user.getUsername()); if (!tempUserRes.isEmpty()) { throw new ResponseStatusException(HttpStatus.CONFLICT, "Username exist"); } String password = BCrypt.hashpw(user.getPassword(), BCrypt.gensalt()); Long userId = generateId(); User newUser = new User(userId, user.getEmail(), user.getUsername()); userRepo.save(newUser); UserSecret newUserSecret = new UserSecret(userId, password); userSecretRepo.save(newUserSecret); //creating profile Profile profile = new Profile(); profile.setUserId(newUser.getUserId()); profile.setEmail(newUser.getEmail()); System.out.println(newUser.getUsername()); profile.setUsername(newUser.getUsername()); profileRepo.save(profile); //token generation String jwtToken = jwtService.generateToken(newUser.getEmail(), Long.toString(newUser.getUserId())); int newSessionId = (int) (System.currentTimeMillis() / 100000); UserSession newUserSession = new UserSession( newUser.getUserId(), jwtToken, new Date(), jwtService.extractExpiryDate(jwtToken), newSessionId ); UserSession userSession = sessionRepo.save(newUserSession); UserSessionDTO userSessionDTO = new UserSessionDTO(profile, userSession.getToken()); return userSessionDTO; } @Override public UserSessionDTO signupUserWithGoogle(TokenRequestDTO tokenRequestDTO) throws Exception { GoogleIdTokenVerifier verifier = new GoogleIdTokenVerifier.Builder(GoogleNetHttpTransport.newTrustedTransport(), GsonFactory.getDefaultInstance()) .setAudience(Collections.singletonList(CLIENT_ID)).build(); GoogleIdToken idToken = verifier.verify(tokenRequestDTO.getTokenId()); if (idToken != null) { GoogleIdToken.Payload payload = idToken.getPayload(); String userEmail = payload.getEmail(); Optional<User> userRes = userRepo.findByEmail(userEmail); if (userRes.isPresent()) { return loginUserWithGoogle(tokenRequestDTO); } long userId = generateId(); String username = userEmail.split("@")[0]; User newUser = new User(userId, userEmail, username); userRepo.save(newUser); Profile profile = new Profile(); profile.setUserId(newUser.getUserId()); profile.setEmail(newUser.getEmail()); System.out.println(newUser.getUsername()); profile.setUsername(newUser.getUsername()); profileRepo.save(profile); String jwtToken = jwtService.generateToken(newUser.getEmail(), Long.toString(newUser.getUserId())); int newSessionId = (int) (System.currentTimeMillis() / 100000); UserSession newUserSession = new UserSession( newUser.getUserId(), jwtToken, new Date(), jwtService.extractExpiryDate(jwtToken), newSessionId ); UserSession userSession = sessionRepo.save(newUserSession); UserSessionDTO userSessionDTO = new UserSessionDTO(profile, userSession.getToken()); return userSessionDTO; } return null; } @Override public UserSessionDTO loginUserWithGoogle(TokenRequestDTO tokenRequestDTO) throws Exception { GoogleIdTokenVerifier verifier = new GoogleIdTokenVerifier.Builder(GoogleNetHttpTransport.newTrustedTransport(), GsonFactory.getDefaultInstance()) .setAudience(Collections.singletonList(CLIENT_ID)).build(); GoogleIdToken idToken = verifier.verify(tokenRequestDTO.getTokenId()); if (idToken != null) { GoogleIdToken.Payload payload = idToken.getPayload(); String userEmail = payload.getEmail(); Optional<User> userRes = userRepo.findByEmail(userEmail); if (userRes.isEmpty()) { return signupUserWithGoogle(tokenRequestDTO); } User existingUser = userRes.get(); Optional<Profile> profileRes = profileRepo.findByUserId(existingUser.getUserId()); if(profileRes.isEmpty()){ throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "No Profile with this user id"); } Profile profile = profileRes.get(); Optional<UserSession> userSessionRes = sessionRepo.findByUserId(existingUser.getUserId()); if (userSessionRes.isPresent()) { UserSession userSession = userSessionRes.get(); if (userSession.getSessionEndTime().before(new Date())) { sessionRepo.delete(userSession); } else { System.out.println("User already logged in"); return new UserSessionDTO(profile, userSession.getToken()); } } else { String jwtToken = jwtService.generateToken(existingUser.getEmail(), Long.toString(existingUser.getUserId())); int newSessionId = (int) (System.currentTimeMillis() / 100000); UserSession newUserSession = new UserSession( existingUser.getUserId(), jwtToken, new Date(), jwtService.extractExpiryDate(jwtToken), newSessionId ); UserSession userSession = sessionRepo.save(newUserSession); UserSessionDTO userSessionDTO = new UserSessionDTO(profile, userSession.getToken()); return userSessionDTO; } } return null; } //using token @Override public boolean authorizeUser(String jwtToken) throws Exception { long userId = (long) jwtService.extractUserId(jwtToken); Date tokenExpiryDate = (Date) jwtService.extractExpiryDate(jwtToken); Optional<UserSession> userSessionRes = sessionRepo.findByUserId(userId); if(userSessionRes.isEmpty()){ return false; } UserSession userSession = userSessionRes.get(); if(userSession.getSessionEndTime().before(new Date()) || tokenExpiryDate.before(new Date())){ sessionRepo.delete(userSession); return false; } return true; } //TODO - Making Util package for all these stuff private Long generateId() { Long time = System.currentTimeMillis(); time = time % 1000000000; time = time * 1000; time = time + (long) (Math.random() * 1000); return time; } } 

Answer 1

SRP violation

Low-level logic for creating and persisting sessions, creating Profiles, generating IDs does not belong to this Service.

Spring Security

Consider introducing Spring Security project into your application instead of doing things like managing sessions and implementing OIDC manually.

Constructor Injection

It's recommended practice to use constructor-based dependency injection instead of applying of @Autowired on fields.

Constructor makes all required dependencies explicit and guarantees the constructed instance will be fully initialed. This approach follows the spirit of OOP. While field injection breaks the encapsulation and opens the dour to creation of non-initialized instances.

It's less noisy than field injection. If the class defines only one constructor, then no annotations required (otherwise, you need to tell which one Spring should use by annotating it with @Autowired).

And with constructor injection, you can make fields final.

---

Field injection is only suitable for test classes, but not in production code.

Don't use java.util.Date

This class is legacy, and it's still there only for backward compatibility reasons.

With the inception of Java 8 all legacy date-time classes were superseded by types from the java.time package.

In this case, a better option will be to use java.time.Instant which represent a specific moment on the timeline in UTC.

Use ControllerAdvice

Don't return null. Don't propagate the problem, forcing the caller to perform null-checks.

If it's not possible to return a meaningful value from a method, throw an Exception.

Use ControllerAdvice to define a global exception-handling mechanism and generate appropriate HTTP-responses based on the exception type and supplementary data which you can embed into your application-specific exception.

Answer 2

For a "fresher" it looks pretty good so far. Good method names, exception handling, and overall design choice.

The first thing improvement I noticed was some code was duplicated and could be reduced.

private UserSessionDTO handleUserSession(User existingUser, Profile profile) { if (existingUser == null) { // Handle the case where existingUser is null return null; // or throw an appropriate exception } Optional<UserSession> userSessionRes = sessionRepo.findByUserId(existingUser.getUserId()); if (userSessionRes.isPresent()) { UserSession userSession = userSessionRes.get(); if (userSession.getSessionEndTime().before(new Date())) { sessionRepo.delete(userSession); } else { return new UserSessionDTO(profile, userSession.getToken()); } } return null; } 

Some of the logic could also be moved into utility classes, especially good if it can be used elsewhere.

public class IdUtils { public static Long generateId() { return System.currentTimeMillis() % 1_000_000_000 * 1_000 + (long)(Math.random() * 1_000); } } 

In my projects I personally use custom exceptions for more clarity in debugging and logging.

public class UserNotFoundException extends RuntimeException { public UserNotFoundException(String email) { super("User not found with email: " + email); } } 

You can then throw this exception in your methods:

if (userRes.isEmpty()) { throw new UserNotFoundException(user.getEmail()); } 

That being said I would be leveraging a logging framework (SLF4J, Log4j, etc..) instead of System.out. For small personal development projects I'm guilty of using System.out but I feel like it's best practice to get used to logging regularly.

if (userRes.isEmpty()) { throw new UserNotFoundException(user.getEmail()); } 

Your use of the optional property could also be problematic in certain scenarios. orElseThrow or map is generally considered safer and cleaner.

User existingUser = userRepo.findByEmail(user.getEmail()).orElseThrow(() -> new UserNotFoundException(user.getEmail())) 

Another example of a problematic use of Optional:

Optional<User> userOptional = userRepo.findByEmail(user.getEmail()); if (userOptional.isPresent()) { User user = userOptional.get(); // At this point, we're sure the user exists. // Additional logic here } else { // Handle case where user is not found } 

In this example, we check if the Optional contains a value and then call .get(). However, if you forget to check for presence and directly call .get(), you’ll run into a NoSuchElementException.

Here’s the cleaner approach using orElseThrow() which avoids that:

User user = userRepo.findByEmail(user.getEmail()) .orElseThrow(() -> new UserNotFoundException(user.getEmail())); 

This way, if the Optional is empty, it automatically throws a custom exception. It's cleaner and more expressive.

Another example is that I've also had problems using nested optionals.

Optional<Profile> profileOptional = profileRepo.findByUserId(existingUser.getUserId()); if (profileOptional.isPresent()) { Profile profile = profileOptional.get(); // ... do something with the profile } 

Instead, you could flatten this using ifPresent() or map():

profileRepo.findByUserId(existingUser.getUserId()) .ifPresent(profile -> { // Handle found profile }); 

or with map() for chaining:

User user = userRepo.findByEmail(user.getEmail()) .map(existingUser -> { return profileRepo.findByUserId(existingUser.getUserId()) .orElseThrow(() -> new ProfileNotFoundException(existingUser.getUserId())); }) .orElseThrow(() -> new UserNotFoundException(user.getEmail()));