resolve #906
resolve #906
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this pull request! 😊 However, there are a few critical issues that need to be addressed before we can proceed:
-
Incomplete
AuthenticationServiceImplImplementation: Theloginandregistermethods are not implemented, which is a core requirement of the task. These methods need to handle user authentication and registration logic, ensuring email uniqueness and secure password storage. -
Custom Exceptions: The
AuthenticationExceptionandRegistrationExceptionshould not extendRuntimeException. Consider using a more appropriate exception type to align with the task requirements. -
Password Hashing and Salt: Ensure that the
UserServiceadd()method implements password hashing and salting, as this is crucial for secure user data handling. -
Unique Email Check: Implement a check to ensure that the email is unique during user registration, as specified in the task requirements.
-
Error Handling in
UserDaoImpl: Improve the error message in theaddmethod by including exception details for better debugging. -
Dependency Injection Issue: The re-declaration of service variables within the
mainmethod overrides the class-level instances initialized with theInjector, leading to potentialNullPointerException. This needs to be corrected.
Please address these issues to meet the core functionality and requirements of the task. Keep up the great effort, and feel free to reach out if you need any assistance! 🚀
✨ 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.
src/main/java/mate/academy/security/AuthenticationServiceImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/mate/academy/security/AuthenticationServiceImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: 7]
If the issue is within your control, please address it.
The next review attempt will automatically launch in 1 hour.
If you believe this is an error or need assistance, please contact the support team.
There was a problem hiding this comment.
Thank you for your submission! 🎉 While your code has been approved to help you move forward with your learning journey, there are several important issues that need to be addressed to ensure the functionality and security of your application:
-
Password Security: Ensure that passwords are hashed and salted before storing them. Use
HashUtil.hashPasswordin bothAuthenticationServiceImplandUserServiceImplto securely handle user passwords. -
Custom Exceptions: The
AuthenticationExceptionandRegistrationExceptionshould not extendRuntimeException. Consider using checked exceptions to align with best practices. -
Dependency Injection: In
Main.java, avoid re-declaring service variables within themainmethod to prevent potentialNullPointerException. Ensure services are properly initialized using theInjector. -
Error Messages: Improve error messages for better debugging. Include specific details, such as whether an email is already taken or if fields are empty. Also, consider adding user details like email or ID for context.
-
Unique Email Check: While the
Usermodel correctly enforces unique emails, ensure that theregistermethod inAuthenticationServiceImplchecks for email uniqueness before proceeding with registration.
Please review these comments and make the necessary improvements to enhance your code's quality and security. Keep up the great work, and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| if (transaction != null) { | ||
| transaction.rollback(); | ||
| } | ||
| throw new DataProcessingException("can't save user to db" + user, e); |
There was a problem hiding this comment.
Consider improving the error message by adding more context, such as the user details, to help with debugging. For example, include the user's email or ID if available.
| @@ -0,0 +1,7 @@ | |||
| package mate.academy.exception; | |||
|
|
|||
| public class AuthenticationException extends RuntimeException { | |||
There was a problem hiding this comment.
The AuthenticationException should not extend RuntimeException. Consider extending a more appropriate exception type, such as Exception, to align with the task requirements.
| @@ -0,0 +1,7 @@ | |||
| package mate.academy.exception; | |||
|
|
|||
| public class RegistrationException extends RuntimeException { | |||
There was a problem hiding this comment.
The RegistrationException should not extend RuntimeException. Consider extending a more appropriate exception type, such as Exception, to align with the task requirements.
| if (userFromDb.isPresent() || email.isEmpty() || password.isEmpty()) { | ||
| throw new RegistrationException("This email is already taken :" + email); |
There was a problem hiding this comment.
The registration method should hash the password before storing the user. Use HashUtil.hashPassword to hash the password and store the hashed password in the User object.
| public User register(String email, String password) throws RegistrationException { | ||
| Optional<User> userFromDb = userService.findByEmail(email); | ||
| if (userFromDb.isPresent() || email.isEmpty() || password.isEmpty()) { | ||
| throw new RegistrationException("This email is already taken :" + email); |
There was a problem hiding this comment.
The error message should be more descriptive. Consider specifying whether the email is taken or if the email/password is empty.
| public User add(User user) { | ||
| return userDao.add(user); |
There was a problem hiding this comment.
Ensure that the user's password is hashed before calling userDao.add(user). Use HashUtil.hashPassword to hash the password and set it in the User object.
No description provided.