Solution #32
Solution #32
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! While your code meets many of the task requirements, there are a few critical security issues that need to be addressed before we can approve the pull request. Specifically, passwords are currently being stored without hashing, which is a significant security vulnerability. It's essential to hash passwords using a library like bcrypt to protect user data. Additionally, consider improving user feedback by differentiating error messages for expired and invalid tokens. Once these issues are resolved, your code will be in great shape! 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.
| try: | ||
| user = UserModel( | ||
| email=user_data.email, | ||
| password=user_data.password, |
There was a problem hiding this comment.
Security Issue: Passwords should be hashed before being stored in the database. Consider using a hashing library like bcrypt to hash the password before assigning it to the user object.
| ) | ||
|
|
||
| try: | ||
| user.password = user_data.password |
There was a problem hiding this comment.
Security Issue: Passwords should be hashed before being stored in the database. Ensure that the new password is hashed before being assigned to the user object.
| if not compare_digest(user_data.token, activation_token.token): | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail="Invalid or expired activation token." |
There was a problem hiding this comment.
Consideration: The error message for an invalid or expired activation token is the same. It might be beneficial to differentiate between an invalid token and an expired token for better user feedback.
|
|
||
| if not reset_password or user_data.token != reset_password.token: | ||
| if reset_password: | ||
| db.delete(reset_password) | ||
| db.commit() | ||
|
|
||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail="Invalid email or token." |
There was a problem hiding this comment.
Consideration: The error message for an invalid or expired password reset token is the same. Differentiating between these cases can provide better user feedback.
| @@ -208,7 +208,7 @@ def create_movie( | |||
| return MovieDetailSchema.model_validate(movie) | |||
| except IntegrityError: | |||
| db.rollback() | |||
| raise HTTPException(status_code=400, detail=f"Invalid input data.") | |||
| raise HTTPException(status_code=400, detail="Invalid input data.") | |||
There was a problem hiding this comment.
Consideration: While handling IntegrityError, it might be useful to log the exception details for better debugging and traceability.
| @@ -394,6 +394,6 @@ | |||
| db.refresh(movie) | |||
| except IntegrityError: | |||
| db.rollback() | |||
| raise HTTPException(status_code=400, detail=f"Invalid input data.") | |||
| raise HTTPException(status_code=400, detail="Invalid input data.") | |||
There was a problem hiding this comment.
Consideration: While handling IntegrityError, it might be useful to log the exception details for better debugging and traceability.
No description provided.