implement task #905

skotsurskiy · 2024-12-14T12:46:45Z

No description provided.

mateacademy-ai-mentor

Thank you for your hard work on this pull request! While you've made great progress, there are a few critical issues that need to be addressed before we can approve it:

Email Uniqueness Check: The current implementation lacks a check for email uniqueness during user registration, which is a core requirement. This needs to be implemented to prevent duplicate user entries.
Password Hashing: Ensure that passwords are hashed before being stored. This is crucial for maintaining user security.
Sensitive Information in toString(): The toString() method in the User class currently exposes sensitive information like passwords. This should be removed to prevent security risks.
Exception Handling: Using getSingleResult() can lead to exceptions if no results are found. Consider using getResultList() to handle such cases more gracefully.
HashUtil Constructor: The HashUtil class should have a private constructor to prevent instantiation since all methods are static.

These changes are important to ensure the functionality and security of the application. Once these issues are addressed, we can proceed with approving your pull request. Keep up the great work! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

mateacademy-ai-mentor · 2024-12-14T12:48:37Z

src/main/java/mate/academy/Main.java

@@ -44,12 +52,28 @@ public static void main(String[] args) {
        yesterdayMovieSession.setMovie(fastAndFurious);
        yesterdayMovieSession.setShowTime(LocalDateTime.now().minusDays(1L));

-        MovieSessionService movieSessionService = null;
+        MovieSessionService movieSessionService
+                = (MovieSessionService) injector.getInstance(MovieSessionService.class);
        movieSessionService.add(tomorrowMovieSession);
        movieSessionService.add(yesterdayMovieSession);

        System.out.println(movieSessionService.get(yesterdayMovieSession.getId()));


Check that movieSessionService.get(yesterdayMovieSession.getId()) does not return null to prevent a NullPointerException.

mateacademy-ai-mentor · 2024-12-14T12:48:37Z

src/main/java/mate/academy/Main.java

@@ -44,12 +52,28 @@ public static void main(String[] args) {
        yesterdayMovieSession.setMovie(fastAndFurious);
        yesterdayMovieSession.setShowTime(LocalDateTime.now().minusDays(1L));

-        MovieSessionService movieSessionService = null;
+        MovieSessionService movieSessionService
+                = (MovieSessionService) injector.getInstance(MovieSessionService.class);
        movieSessionService.add(tomorrowMovieSession);
        movieSessionService.add(yesterdayMovieSession);

        System.out.println(movieSessionService.get(yesterdayMovieSession.getId()));
        System.out.println(movieSessionService.findAvailableSessions(
                        fastAndFurious.getId(), LocalDate.now()));


Ensure that movieSessionService.findAvailableSessions(fastAndFurious.getId(), LocalDate.now()) handles cases where no sessions are found, to avoid potential issues when printing the result.

mateacademy-ai-mentor · 2024-12-14T12:48:37Z

src/main/java/mate/academy/Main.java

+        AuthenticationService authenticationService =
+                (AuthenticationService) injector.getInstance(AuthenticationService.class);
+        try {
+            authenticationService.register("121212@gmail.com", "qwerty");


Consider adding validation for the email and password before calling authenticationService.register to ensure they meet any required criteria.

mateacademy-ai-mentor · 2024-12-14T12:48:37Z

src/main/java/mate/academy/Main.java

+        }
+        User user;
+        try {
+            user = authenticationService.login("121212@gmail.com", "qwerty");


Ensure that the login credentials are correct and handle cases where authenticationService.login might return null or throw an exception, to avoid runtime errors.

mateacademy-ai-mentor · 2024-12-14T12:48:37Z

src/main/java/mate/academy/dao/impl/UserDaoImpl.java

+                    "from User u where u.email = :email", User.class
+            );
+            query.setParameter("email", email);
+            return Optional.ofNullable(query.getSingleResult());


Using getSingleResult() can throw a NoResultException if no user is found. Consider using getResultList() and checking if the list is empty to avoid this exception.

mateacademy-ai-mentor · 2024-12-14T12:48:38Z

src/main/java/mate/academy/model/User.java

+        return "User{"
+                + "id=" + id
+                + ", email='" + email + '\''
+                + ", password='" + password + '\''


The toString method currently includes the password, which is sensitive information. Consider removing the password from the toString method to avoid exposing it in logs or other outputs.

mateacademy-ai-mentor · 2024-12-14T12:48:38Z

src/main/java/mate/academy/security/impl/AuthenticationServiceImpl.java

+        }
+        User user = new User();
+        user.setEmail(email);
+        user.setPassword(password);


The password should be hashed before being set on the user object. Consider using HashUtil.hashPassword(password, salt) to hash the password and store the salt in the user object.

mateacademy-ai-mentor · 2024-12-14T12:48:38Z

src/main/java/mate/academy/security/impl/AuthenticationServiceImpl.java

+        if (email == null || password == null) {
+            throw new RegistrationException("Invalid email or password");


Consider adding more validation for the email format and password strength to ensure they meet security standards.

mateacademy-ai-mentor · 2024-12-14T12:48:38Z

src/main/java/mate/academy/util/HashUtil.java

+    public HashUtil() {
+    }


The constructor is unnecessary since all methods are static. Consider making the class final and adding a private constructor to prevent instantiation.

mateacademy-ai-mentor

Great job on implementing the core functionalities of your application! 🎉 While the Pull Request is approved to help you move forward with your learning, there are some critical issues that need attention to ensure the security and robustness of your code:

Email Uniqueness Check: The register method in AuthenticationServiceImpl should verify if the email already exists before creating a new user. This will prevent duplicate user entries and is crucial for maintaining data integrity.
Password Hashing: Ensure that passwords are hashed before being stored. The UserServiceImpl class correctly implements password hashing, which is essential for user security. Make sure this practice is consistently applied.
Sensitive Information Exposure: The toString() method in the User class currently exposes the password, which is a security risk. Modify this method to exclude sensitive information.
Exception Handling in findByEmail: The findByEmail method in UserDaoImpl uses getSingleResult(), which can throw an exception if no result is found. Consider using getResultList() and checking if the list is empty to handle this more gracefully.

Please address these issues in future iterations to enhance the security and functionality of your application. Keep up the good work and continue learning from these experiences! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

mateacademy-ai-mentor · 2024-12-15T11:46:49Z

src/main/java/mate/academy/Main.java

+        AuthenticationService authenticationService =
+                (AuthenticationService) injector.getInstance(AuthenticationService.class);


Ensure that the register method in AuthenticationService checks for email uniqueness to prevent duplicate user entries. This is a critical requirement for user registration.

mateacademy-ai-mentor · 2024-12-15T11:46:49Z

src/main/java/mate/academy/Main.java

+        AuthenticationService authenticationService =
+                (AuthenticationService) injector.getInstance(AuthenticationService.class);
+        try {
+            authenticationService.register("121212@gmail.com", "qwerty");


Verify that the password is hashed before being stored during registration. This is essential for maintaining user security.

mateacademy-ai-mentor · 2024-12-15T11:46:49Z

src/main/java/mate/academy/Main.java

+        } catch (AuthenticationException e) {
+            throw new RuntimeException(e);
+        }
+        System.out.println(user);


Ensure that sensitive information, such as passwords, is not exposed when printing the User object. Consider overriding the toString() method in the User class to exclude sensitive data.

mateacademy-ai-mentor · 2024-12-15T11:46:49Z

src/main/java/mate/academy/dao/impl/UserDaoImpl.java

+                    "from User u where u.email = :email", User.class
+            );
+            query.setParameter("email", email);
+            return Optional.ofNullable(query.getSingleResult());


Using getSingleResult() can throw an exception if no user is found with the given email. Consider using getResultList() and checking if the list is empty to handle this case more gracefully.

mateacademy-ai-mentor · 2024-12-15T11:46:49Z

src/main/java/mate/academy/model/User.java

+        return "User{"
+                + "id=" + id
+                + ", email='" + email + '\''
+                + ", password='" + password + '\''


Including the password in the toString() method is a security risk. Consider removing the password field from the toString() output to prevent exposing sensitive information.

mateacademy-ai-mentor · 2024-12-15T11:46:50Z

src/main/java/mate/academy/security/impl/AuthenticationServiceImpl.java

+    @Override
+    public void register(String email, String password) throws RegistrationException {


The register method should include a check for email uniqueness to prevent duplicate user entries. Consider using userService.findByEmail(email) to check if a user with the given email already exists.

mateacademy-ai-mentor · 2024-12-15T11:46:50Z

src/main/java/mate/academy/security/impl/AuthenticationServiceImpl.java

+        }
+        User user = new User();
+        user.setEmail(email);
+        user.setPassword(password);


Ensure that the password is hashed before being stored. Use HashUtil.hashPassword(password, salt) to hash the password and store the hash instead of the plain password.

implement task

4b01d10

mateacademy-ai-mentor suggested changes Dec 14, 2024

View reviewed changes

fix

c3d7ad4

skotsurskiy requested a review from mateacademy-ai-mentor December 15, 2024 11:44

mateacademy-ai-mentor approved these changes Dec 15, 2024

View reviewed changes

		if (email == null \|\| password == null) {
		throw new RegistrationException("Invalid email or password");

		AuthenticationService authenticationService =
		(AuthenticationService) injector.getInstance(AuthenticationService.class);

		@Override
		public void register(String email, String password) throws RegistrationException {

implement task #905

Are you sure you want to change the base?

implement task #905

Uh oh!

Conversation

skotsurskiy commented Dec 14, 2024

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!