Implement login with DTO and secure password validation#7
Conversation
…ord configuration
…nd adds http status codes
📝 WalkthroughWalkthroughAdds password encoding and credential-based login: a BCrypt PasswordEncoder bean, AuthService that validates credentials against UserRepository, DTOs and AuthController exposing POST /auth/login, and a default role pre-persist on User. Changes
Sequence DiagramsequenceDiagram
actor Client
participant AuthController
participant AuthService
participant UserRepository
participant PasswordEncoder
participant Database
Client->>AuthController: POST /auth/login {username, password}
AuthController->>AuthService: login(username, password)
AuthService->>UserRepository: findByUsername(username)
UserRepository->>Database: query user by username
Database-->>UserRepository: user or empty
alt user not found
AuthService-->>AuthController: throw 401 Unauthorized
AuthController-->>Client: 401 Unauthorized
else user found
AuthService->>PasswordEncoder: matches(plain, storedHash)
PasswordEncoder-->>AuthService: boolean
alt mismatch
AuthService-->>AuthController: throw 401 Unauthorized
AuthController-->>Client: 401 Unauthorized
else match
AuthService-->>AuthController: User
AuthController->>AuthController: build LoginResponseDTO(username, role)
AuthController-->>Client: 200 OK {username, role}
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/org/example/alfs/dto/auth/LoginRequestDTO.java (1)
6-11: Consider adding bean validation annotations.The DTO lacks input validation. Adding
@NotBlankconstraints would reject malformed requests early at the controller layer, avoiding unnecessary service calls and database queries.♻️ Proposed validation enhancement
package org.example.alfs.dto.auth; import lombok.Getter; import lombok.Setter; +import jakarta.validation.constraints.NotBlank; `@Getter` `@Setter` public class LoginRequestDTO { + `@NotBlank`(message = "Username is required") private String username; + `@NotBlank`(message = "Password is required") private String password; }Note: Requires adding
@Validto the controller's@RequestBodyparameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/dto/auth/LoginRequestDTO.java` around lines 6 - 11, The LoginRequestDTO currently has no validation; annotate the username and password fields in class LoginRequestDTO with `@NotBlank` (from javax.validation.constraints.NotBlank or jakarta equivalent) to enforce non-empty input, and ensure the controller method that accepts this DTO uses `@Valid` on the `@RequestBody` parameter so validation is triggered; keep the existing `@Getter/`@Setter (Lombok) and add the necessary import for the NotBlank annotation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/org/example/alfs/dto/auth/LoginRequestDTO.java`:
- Around line 6-11: The LoginRequestDTO currently has no validation; annotate
the username and password fields in class LoginRequestDTO with `@NotBlank` (from
javax.validation.constraints.NotBlank or jakarta equivalent) to enforce
non-empty input, and ensure the controller method that accepts this DTO uses
`@Valid` on the `@RequestBody` parameter so validation is triggered; keep the
existing `@Getter/`@Setter (Lombok) and add the necessary import for the NotBlank
annotation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ddf5cf04-143a-437d-86b8-cb3ad7487c46
📒 Files selected for processing (6)
src/main/java/org/example/alfs/config/PasswordConfig.javasrc/main/java/org/example/alfs/controllers/AuthController.javasrc/main/java/org/example/alfs/dto/auth/LoginRequestDTO.javasrc/main/java/org/example/alfs/dto/auth/LoginResponseDTO.javasrc/main/java/org/example/alfs/repositories/UserRepository.javasrc/main/java/org/example/alfs/services/AuthService.java
|
Nice work on the PR! Will there be a |
Yesbox |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/alfs/controllers/AuthController.java`:
- Around line 34-37: In AuthController where you build the LoginResponseDTO from
user (the block calling user.getRole().name()), guard against a null role by
using a null-safe expression to pass either a default string (e.g., "UNKNOWN" or
"USER") or null to LoginResponseDTO when user.getRole() is null; update the code
that constructs the response in AuthController to check user.getRole() before
calling .name(); alternatively enforce non-null at the entity level by adding
`@Column`(nullable = false) to User.role and ensure a default role is set on user
creation (update User entity and user creation flow) so user.getRole() can never
be null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1bf90207-8d22-4aef-a834-d0cc4f4991c3
📒 Files selected for processing (4)
src/main/java/org/example/alfs/controllers/AuthController.javasrc/main/java/org/example/alfs/dto/auth/LoginRequestDTO.javasrc/main/java/org/example/alfs/dto/auth/LoginResponseDTO.javasrc/main/java/org/example/alfs/services/AuthService.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/example/alfs/dto/auth/LoginResponseDTO.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/example/alfs/dto/auth/LoginRequestDTO.java
- src/main/java/org/example/alfs/services/AuthService.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/alfs/entities/User.java`:
- Around line 30-31: The User.role field was changed to `@Column`(nullable =
false) which will break migrations if existing user rows have NULL and can cause
NPE in AuthController.login() where user.getRole().name() is called; before
enforcing this constraint, add a data migration (Flyway/Liquibase or raw SQL)
that sets a sensible default for existing null roles (e.g., update users set
role='REPORTER' where role is null) and then apply the schema change, ensuring
AuthController.login() and the User entity are safe from null roles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66251dff-d530-4472-abaa-6c7ce37f5dea
📒 Files selected for processing (1)
src/main/java/org/example/alfs/entities/User.java
| @Column(nullable = false) | ||
| private Role role; |
There was a problem hiding this comment.
Ensure existing users have roles before applying this constraint.
Adding nullable = false to an existing column requires that no null values exist in the database. If there are existing users with null roles, the schema migration will fail or existing users will cause NullPointerException in AuthController.login() when calling user.getRole().name().
Consider adding a data migration (e.g., Flyway/Liquibase script or a SQL update) to set a default role for any existing null values before this constraint is enforced:
UPDATE users SET role = 'REPORTER' WHERE role IS NULL;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/alfs/entities/User.java` around lines 30 - 31, The
User.role field was changed to `@Column`(nullable = false) which will break
migrations if existing user rows have NULL and can cause NPE in
AuthController.login() where user.getRole().name() is called; before enforcing
this constraint, add a data migration (Flyway/Liquibase or raw SQL) that sets a
sensible default for existing null roles (e.g., update users set role='REPORTER'
where role is null) and then apply the schema change, ensuring
AuthController.login() and the User entity are safe from null roles.
There was a problem hiding this comment.
We dont have any existing database or persisted users yet, so this wont cause issues in our current setup. We'll handle migrations properly when we introduce a real database.
There was a problem hiding this comment.
@addee1, that makes sense — since there's no existing database or persisted users yet, the nullable = false constraint won't cause any migration issues right now, and the @PrePersist callback already guards against null roles on new entities.
Just a reminder for when you introduce a real database: before applying this schema constraint in a migration script, make sure to run a backfill like the one suggested (e.g., UPDATE users SET role = 'REPORTER' WHERE role IS NULL;) before or alongside the ALTER TABLE step to avoid migration failures on any pre-existing data.
2 participants
🔐 Authentication (Login)
This PR implements a basic login flow for users.
✅ What’s included
📦 DTOs
Uses DTOs for request and response to ensure type safety and a clean API design.
🔒 Security improvements
🚀 Notes
Ready to extend with JWT authentication next 🔐
Summary by CodeRabbit
New Features
Bug Fixes