This Java & Spring Data JPA code is a disaster disguised as a login service. Recently, I shared this code without answers, giving an opportunity to challenge yourself. Let's crack this code. This post focused on problems in business logic and authentication logic. Most of them are critical, so let's start with the easiest one. 1. You probably know, in Java, `==` for String is not comparing values, it compares references. If your authentication depends on `u.getLogin() == login`, then login works by luck, not by logic. 2. Registration hashes the password with `md5(password)`, but login compares the stored password with the raw input. In other words, registration and authentication do not even agree on the same format. This login flow is broken by design. 3. MD5 for password storage is a bad idea. Password hashing should be slow and resistant to brute force. MD5 is fast, old, and loved by attackers. Use BCrypt or Argon2 instead. 4. Logging passwords is a free gift for hackers. `log.debug(... password ...)` should be enough to stop the review immediately. I have seen similar things in production code, and they always had to be fixed right away. 5. `findAll().stream()` to authenticate one user is painful to watch. The database can find one user by login, but instead, the code loads all users into memory and filters them in Java. Bad for performance, scalability, and design. 6. Input validation is inconsistent. `register()` trims the login, `login()` does not. There are no checks for null or blank values, or even validation for business logic - username length or complexity of password. Authentication code must be paranoid, because user input is never your friend. 7. `findByLogin(...).isPresent()` and then `save(...)` is not enough to guarantee uniqueness. Two concurrent requests can pass the same check and create duplicate users unless the database has a unique constraint. Business logic bugs also love concurrency. 8. Returning the full `User` entity after registration or login is risky. If it contains the password hash or other internal fields, one careless serialization step can leak data. 9. `loginCount = loginCount + 1` looks innocent, until concurrent logins arrive and you start losing updates. Even counters need proper thinking in multi-user systems. 10. Throwing `IllegalArgumentException` for everything is bad design. Authentication failure, duplicate user, and bad input are different problems, but here they are handled as if they were all the same. In other words, you have to carefully design, develop, and review security features like this, and not only them. Did you find one more problem?
The trimming on register but not login is so funny.
And there is findByLogin, and yet used the memory bomb. Just curious, is there any way to make this code asynchronous?
Isn’t it return Dto instead of entity?