I almost have completed #963

Sasha1256 · 2025-08-15T11:21:43Z

No description provided.

mateacademy-ai-mentor

Review Summary

REQUEST_CHANGES

Thank you for your hard work on this assignment! There are several critical issues that must be addressed before your submission can be approved, as they block core functionality and violate explicit checklist requirements:

1. Incorrect DAO Method Signature and Logic

The UserDao and UserDaoImpl use getByUsername(String username) instead of the required findByEmail(String email). This breaks user lookup by email, which is essential for registration and login.
The implementation retrieves by primary key, not by email. You must use a query to find by the email field and return an Optional<User> using uniqueResultOptional().
All DAO methods must have proper exception handling (try-catch-finally with transaction rollback where needed).

2. Exception Handling and Declaration

AuthenticationException extends RuntimeException, but it must extend Exception (checked exception).
The AuthenticationService interface and its implementation must declare throws AuthenticationException for login and throws RegistrationException for register.
In AuthenticationServiceImpl.login(), combine the user existence and password checks so that only one AuthenticationException is thrown if either fails.

3. Registration Logic

In AuthenticationServiceImpl.register(), you must check if the email is already used (via findByEmail) and throw RegistrationException if so. This prevents duplicate registrations.

4. Sensitive Data in User.toString()

The User class's toString() method currently includes the password field. For security, remove password (and salt, if present) from the output.

5. Utility Class Best Practices

The HashUtil class must have a private constructor to prevent instantiation.

6. Minor but Important

Consider adding a uniqueness constraint to the email field in the User entity (e.g., @Column(unique = true)).
Fix the typo in HashUtil (ALGORITM → ALGORITHM).

Next Steps:

Update all method names and signatures as required.
Implement the correct logic for finding users by email.
Ensure all exceptions are handled and declared properly.
Remove sensitive information from toString().
Add a private constructor to HashUtil.

Once these changes are made, your code will be much closer to meeting all requirements. You're on the right track—keep going! If you have any questions about the requirements or how to implement these changes, feel free to ask.

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

Important

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

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

mateacademy-ai-mentor · 2025-08-15T11:25:21Z

src/main/java/mate/academy/dao/UserDao.java

+public interface UserDao {
+    User save(User user);
+    User get(Long id);
+    Optional<User> getByUsername(String username);


This violates checklist item #2: 'Implement a UserDao with at least: ... A method Optional findByEmail(String email) that returns an Optional.'

The method here is named getByUsername and uses username instead of findByEmail and email. You must rename this method to findByEmail(String email) and ensure it matches the required signature.

mateacademy-ai-mentor · 2025-08-15T11:25:22Z

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

+    }
+
+    @Override
+    public Optional<User> getByUsername(String username) {


This violates checklist item #2: 'Implement a UserDao with at least: ... A method Optional findByEmail(String email) that returns an Optional.'

The method here is named getByUsername and uses username instead of findByEmail and email. You must rename this method to findByEmail(String email) and ensure it matches the required signature.

mateacademy-ai-mentor · 2025-08-15T11:25:22Z

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

+    @Override
+    public Optional<User> getByUsername(String username) {
+        Session session = HibernateUtil.getSessionFactory().openSession();
+        return Optional.of(session.get(User.class, username));


The implementation here is incorrect. session.get(User.class, username) retrieves by primary key, not by email. You need to use a query to find a user by their email field, and use uniqueResultOptional() as suggested in the requirements.

mateacademy-ai-mentor · 2025-08-15T11:25:22Z

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

+    public User get(Long id) {
+        Session session = HibernateUtil.getSessionFactory().openSession();
+        return session.get(User.class, id);
+    }
+
+    @Override
+    public Optional<User> getByUsername(String username) {
+        Session session = HibernateUtil.getSessionFactory().openSession();
+        return Optional.of(session.get(User.class, username));
+    }


This violates checklist item #5: 'All DAO operations must have catch blocks for all types of operations.'

The get and getByUsername methods do not have exception handling or transaction management. You should add try-catch-finally blocks similar to the save method.

mateacademy-ai-mentor · 2025-08-15T11:25:22Z

src/main/java/mate/academy/exception/AuthenticationException.java

@@ -0,0 +1,7 @@
+package mate.academy.exception;
+
+public class AuthenticationException extends RuntimeException {


This violates checklist item #3: 'AuthenticationException and RegistrationException must NOT extend RuntimeException (they must be checked exceptions).'

You should extend Exception instead of RuntimeException.

mateacademy-ai-mentor · 2025-08-15T11:25:23Z

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

+        if (byEmail.isEmpty()) {
+            throw new AuthenticationException("Invalid email");
+        }
+        User user = byEmail.get();
+        String hashedPassword = HashUtil.hashPassword(password, user.getSalt());
+        if (!hashedPassword.equals(user.getPassword())) {
+            throw new AuthenticationException("Invalid password");


This violates checklist item #9: 'In login() method, only one condition should throw AuthenticationException (combine user existence and password match checks).'

You throw AuthenticationException separately for invalid email and invalid password. You should combine these checks and throw a single exception if either condition fails.

mateacademy-ai-mentor · 2025-08-15T11:25:23Z

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

+    }
+
+    @Override
+    public User register(String email, String password) {


This violates checklist item #4: 'Implement an AuthenticationService interface with: User login(String email, String password) that throws AuthenticationException; User register(String email, String password) that throws RegistrationException.'

The register method does not declare throws RegistrationException in its signature.

mateacademy-ai-mentor · 2025-08-15T11:25:23Z

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

+    public User register(String email, String password) {
+        User user = new User();
+        user.setEmail(email);
+        user.setPassword(password);
+        return userService.add(user);


This violates checklist item #2: 'During registration, check that the email is unique (i.e., not already used).' and checklist item #4: 'User register(String email, String password) that throws RegistrationException.'

You must check if the email is already used and throw RegistrationException if so.

mateacademy-ai-mentor · 2025-08-15T11:25:23Z

src/main/java/mate/academy/service/impl/UserServiceImpl.java

+
+    @Override
+    public Optional<User> findByEmail(String email) {
+        return userDao.getByUsername(email);


This violates checklist item #2: 'Implement a UserDao with at least: ... A method Optional findByEmail(String email) that returns an Optional.'

You are calling userDao.getByUsername(email), but the required method is findByEmail(String email). Update this call once the DAO is corrected.

mateacademy-ai-mentor · 2025-08-15T11:25:23Z

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

+import java.security.NoSuchAlgorithmException;
+import java.security.SecureRandom;
+
+public class HashUtil {


This violates checklist item #4: 'Add a private constructor to HashUtil class.'

You must add a private constructor to prevent instantiation of this utility class.

mateacademy-ai-mentor

Review Summary

REQUEST_CHANGES

Thank you for your hard work on this assignment! You have a solid foundation, but there are several critical issues that must be fixed before your code can be approved. These issues directly impact core functionality and violate explicit checklist requirements:

1. Incorrect findByEmail Implementation in DAO

In UserDaoImpl, findByEmail uses session.get(User.class, email), which looks up by primary key, not by email. You must use a query to search by the email field and retrieve the result with uniqueResultOptional(). Example:
```
session.createQuery("FROM User u WHERE u.email = :email", User.class)
    .setParameter("email", email)
    .uniqueResultOptional();
```

2. Missing Exception Handling in DAO Methods

All DAO methods (including findByEmail and get) must have try-catch blocks to handle exceptions and roll back transactions if needed. This is required for robust transaction management.

3. Email Uniqueness Not Enforced

The email field in the User entity must be annotated with a uniqueness constraint, e.g., @Column(unique = true), to ensure the database enforces unique emails.
During registration (in AuthenticationServiceImpl), you must check if the email is already used (via userService.findByEmail) and throw a RegistrationException if it is.

4. Incorrect Exception Signatures

The register method in both AuthenticationService and AuthenticationServiceImpl must declare throws RegistrationException.

5. Login Exception Logic

In AuthenticationServiceImpl.login, you currently check for user existence and password match separately, throwing exceptions in both cases. You must combine these checks and throw a single AuthenticationException if either the user does not exist or the password does not match, as per the checklist.

6. Service Layer Calls Wrong DAO Method

In UserServiceImpl, findByEmail should call userDao.findByEmail(email), not getByUsername(email).

7. HashUtil Issues

There is a typo in the constant name: ALGORITM should be ALGORITHM.
The password hashing logic does not use the salt correctly. You should call messageDigest.update(salt) before digesting the password bytes, so the salt is included in the hash computation.

Next Steps:

Fix the above issues, focusing on correct DAO queries, exception handling, enforcing email uniqueness, and proper password hashing.
Double-check all method signatures against the checklist.

You're very close! Once these blockers are resolved, your code will be robust and ready for approval. Keep up the good work and let me know if you have any questions about these changes.

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

Important

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

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag³.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

mateacademy-ai-mentor · 2025-08-15T12:17:36Z

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

+    @Override
+    public Optional<User>  findByEmail(String email) {
+        try (Session session = HibernateUtil.getSessionFactory().openSession()) {
+            return Optional.of(session.get(User.class, email));


The findByEmail method incorrectly uses session.get(User.class, email), which retrieves by primary key, not by the email field. You must use a query to find by the email field and use uniqueResultOptional() as required by the checklist: "In findByEmail() DAO method, use uniqueResultOptional() to retrieve the user."

mateacademy-ai-mentor · 2025-08-15T12:17:36Z

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

+    public Optional<User>  findByEmail(String email) {
+        try (Session session = HibernateUtil.getSessionFactory().openSession()) {
+            return Optional.of(session.get(User.class, email));
+        }
+    }


The findByEmail method lacks exception handling. All DAO operations must have catch blocks for exceptions, as required by the checklist: "Always add catch blocks for all DAO operations."

mateacademy-ai-mentor · 2025-08-15T12:17:36Z

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

+    @Id
+    @GeneratedValue(strategy = GenerationType.IDENTITY)
+    private Long id;
+    private String email;


The email field should be annotated with a uniqueness constraint (e.g., @jakarta.persistence.Column(unique = true)) to ensure email uniqueness at the database level, as required by the checklist: "The User email must be unique in the database."

mateacademy-ai-mentor · 2025-08-15T12:17:36Z

src/main/java/mate/academy/security/AuthenticationService.java

+public interface AuthenticationService {
+    User login(String email, String password) throws AuthenticationException;
+
+    User register(String email, String password);


The register method must declare throws RegistrationException in its signature, as required by the checklist: "AuthenticationService interface with methods: User login(String email, String password) throws AuthenticationException; User register(String email, String password) throws RegistrationException;"

mateacademy-ai-mentor · 2025-08-15T12:17:37Z

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

+    }
+
+    @Override
+    public User register(String email, String password) {


The register method must declare throws RegistrationException in its signature, as required by the checklist: "AuthenticationService interface with methods: User login(String email, String password) throws AuthenticationException; User register(String email, String password) throws RegistrationException;"

mateacademy-ai-mentor · 2025-08-15T12:17:37Z

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

+    public User register(String email, String password) {
+        User user = new User();
+        user.setEmail(email);
+        user.setPassword(password);
+        return userService.add(user);


The register method must check if the email is already used (via userService.findByEmail) and throw RegistrationException if so, as required by the checklist: "During registration, check that the email is unique (i.e., not already used)."

mateacademy-ai-mentor · 2025-08-15T12:17:37Z

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

+    public User login(String email, String password) throws AuthenticationException {
+        Optional<User> byEmail = userService.findByEmail(email);
+        if (byEmail.isEmpty()) {
+            throw new AuthenticationException("Invalid email");
+        }
+        User user = byEmail.get();
+        String hashedPassword = HashUtil.hashPassword(password, user.getSalt());
+        if (!hashedPassword.equals(user.getPassword()) || byEmail.isEmpty()) {
+            throw new AuthenticationException("Invalid password or email");


In the login method, you check for byEmail.isEmpty() twice (once before password check, and again in the password check condition). The checklist requires that only one AuthenticationException is thrown if either the user does not exist or the password does not match, and the check should be combined.

mateacademy-ai-mentor · 2025-08-15T12:17:37Z

src/main/java/mate/academy/service/impl/UserServiceImpl.java

+
+    @Override
+    public Optional<User> findByEmail(String email) {
+        return userDao.getByUsername(email);


The findByEmail method incorrectly calls userDao.getByUsername(email). It must call userDao.findByEmail(email) to match the interface and requirements: "findByEmail() must accept only a single String parameter (the email)" and "findByEmail() must return Optional on both DAO and Service layers."

mateacademy-ai-mentor · 2025-08-15T12:17:37Z

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

+    private HashUtil() {
+    }
+
+    private static final String ALGORITM = "SHA-256";


There is a typo in the constant name: ALGORITM should be ALGORITHM. This could cause a runtime error if the algorithm name is used as-is.

mateacademy-ai-mentor · 2025-08-15T12:17:37Z

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

+        try {
+            MessageDigest messageDigest = MessageDigest.getInstance(ALGORITM);
+            byte[] digest = messageDigest.digest(password.getBytes());
+            messageDigest.update(salt);


The password hashing logic is incorrect: messageDigest.update(salt) should be called before digest(password.getBytes()), or the salt should be included in the digest computation. As written, the salt is not actually used in the hash computation. This violates the requirement to use salt and password hashing for user passwords.

mateacademy-ai-mentor

Review Summary

This is an educational approval to help you continue progressing in your learning journey!

What you did well:

You implemented the core structure of the application, including interfaces, service, and DAO layers.
Secure password handling with hashing and salt is correctly set up in your service layer.
Exception handling and custom checked exceptions are used as required.
You enforced email uniqueness both in the database and in your registration logic.

Areas for improvement:

In UserDaoImpl.java, the findByEmail method should use a query to search by the email field, not session.get(User.class, email). This ensures users are found by their email, not just by primary key.
Hint: Use a query like:
```
session.createQuery("FROM User u WHERE u.email = :email", User.class)
    .setParameter("email", email)
    .uniqueResultOptional();
```
In AuthenticationServiceImpl.java, the login method should combine the user existence and password check into a single condition that throws AuthenticationException. This makes your code cleaner and matches the requirements.
Double-check that password hashing and salt generation are only handled in UserService.add(), not directly in the registration method.

Keep going!
You’re making great progress and showing a solid understanding of Java, Hibernate, and secure authentication practices. Review the comments for specific improvements, and keep up the good work—each iteration brings you closer to mastery!

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

Rate AI review example ↩

mateacademy-ai-mentor · 2025-08-16T11:21:16Z

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

+    @Override
+    public Optional<User>  findByEmail(String email) {
+        try (Session session = HibernateUtil.getSessionFactory().openSession()) {
+            return Optional.ofNullable(session.get(User.class, email));


The implementation of findByEmail is incorrect. It uses session.get(User.class, email), which looks up by primary key, not by the email field. According to the requirements, you must use a query to search by the email field and retrieve the result with uniqueResultOptional(). Example:

return session.createQuery("FROM User u WHERE u.email = :email", User.class) .setParameter("email", email) .uniqueResultOptional();

This violates checklist item: 'In UserDaoImpl, findByEmail uses session.get(User.class, email), which looks up by primary key, not by email. You must use a query to search by the email field and retrieve the result with uniqueResultOptional().'

mateacademy-ai-mentor · 2025-08-16T11:21:16Z

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

+        if (!hashedPassword.equals(user.getPassword()) || byEmail.isEmpty()) {
+            throw new AuthenticationException("Invalid password or email");


The login method currently throws an AuthenticationException for user not found and then again for password mismatch. The checklist requires that only one condition should throw AuthenticationException (combine user existence and password match checks). Also, the check byEmail.isEmpty() in the second condition is redundant since it's already checked above. Refactor to combine these checks into a single condition as required.

mateacademy-ai-mentor · 2025-08-16T11:21:16Z

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

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


In the register method, you set the password directly on the user object. According to the requirements, password hashing and salt generation must be performed securely in UserService.add(). Please ensure that UserService.add() handles this. If not, update the logic accordingly.

I almost have completed

5ab18a2

mateacademy-ai-mentor suggested changes Aug 15, 2025

View reviewed changes

Sasha1256 added 2 commits August 15, 2025 14:34

I almost have completed

f125717

I almost have completed

41d4307

Sasha1256 requested a review from mateacademy-ai-mentor August 15, 2025 12:13

mateacademy-ai-mentor suggested changes Aug 15, 2025

View reviewed changes

I almost have completed

ebb5a6b

Sasha1256 requested a review from mateacademy-ai-mentor August 16, 2025 11:18

mateacademy-ai-mentor approved these changes Aug 16, 2025

View reviewed changes

Sasha1256 added 3 commits August 16, 2025 14:27

I almost have completed

1a7d740

I almost have completed

9b520b9

I almost have completed

98b058f

		@@ -0,0 +1,7 @@
		package mate.academy.exception;

		public class AuthenticationException extends RuntimeException {

		if (!hashedPassword.equals(user.getPassword()) \|\| byEmail.isEmpty()) {
		throw new AuthenticationException("Invalid password or email");

I almost have completed #963

Are you sure you want to change the base?

I almost have completed #963

Uh oh!

Conversation

Sasha1256 commented Aug 15, 2025

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

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!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

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!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

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!