feat: AuthService och AuthController för login och register#204
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 40 minutes and 25 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds authentication feature: new Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant AuthController
participant AuthService
participant AuthenticationManager
participant JwtService
participant UserRepository
Client->>AuthController: POST /api/auth/login (email,password)
AuthController->>AuthService: login(AuthRequest)
AuthService->>AuthenticationManager: authenticate(UsernamePasswordAuthenticationToken)
AuthenticationManager-->>AuthService: Authentication (User principal)
AuthService->>JwtService: generateToken(user)
JwtService-->>AuthService: JWT
AuthService-->>AuthController: AuthResponse (token,user)
AuthController-->>Client: 200 OK
sequenceDiagram
actor Client
participant AuthController
participant AuthService
participant UserRepository
participant PasswordEncoder
participant JwtService
Client->>AuthController: POST /api/auth/register (name,email,password,role)
AuthController->>AuthService: register(RegisterRequest)
AuthService->>UserRepository: findByEmail(email)
alt email exists
UserRepository-->>AuthService: existing User
AuthService-->>AuthController: throw BusinessRuleException("Email används redan")
AuthController-->>Client: 422 Unprocessable Entity
else email unique
UserRepository-->>AuthService: null
AuthService->>PasswordEncoder: encode(password)
PasswordEncoder-->>AuthService: encoded password
AuthService->>UserRepository: save(new User)
UserRepository-->>AuthService: saved User
AuthService->>JwtService: generateToken(user)
JwtService-->>AuthService: JWT
AuthService-->>AuthController: AuthResponse (token,user)
AuthController-->>Client: 201 Created
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/org/example/vet1177/security/AuthService.java (1)
49-69: Small DRY refactor opportunity for AuthResponse creation.
new AuthResponse(...)is duplicated in both flows; extracting a private mapper improves maintainability.♻️ Optional refactor
- return new AuthResponse(token, user.getId(), user.getName(), user.getEmail(), user.getRole()); + return toAuthResponse(token, user); ... - return new AuthResponse(token, user.getId(), user.getName(), user.getEmail(), user.getRole()); + return toAuthResponse(token, user); + +private AuthResponse toAuthResponse(String token, User user) { + return new AuthResponse(token, user.getId(), user.getName(), user.getEmail(), user.getRole()); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/security/AuthService.java` around lines 49 - 69, Extract the duplicated AuthResponse construction into a single private helper (e.g., private AuthResponse mapToAuthResponse(User user, String token) or createAuthResponse) and replace both places that call new AuthResponse(...) (see the return in register and the other AuthResponse return elsewhere) to call this helper; ensure the helper returns new AuthResponse(token, user.getId(), user.getName(), user.getEmail(), user.getRole()) so behavior remains identical and update imports/logging if needed.
🤖 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/vet1177/security/AuthService.java`:
- Around line 53-56: AuthService throws BusinessRuleException with a specific
message ("Email används redan") but GlobalExceptionHandler currently returns a
fixed "Business rule violation" message, so the specific text never reaches
clients; update the BusinessRuleException handler in GlobalExceptionHandler (the
method that handles BusinessRuleException) to return the exception's message
(e.g., exception.getMessage()) or include it in the response body instead of the
fixed string, ensuring BusinessRuleException instances thrown from AuthService
(and elsewhere) propagate their specific messages to clients.
- Around line 53-66: AuthService.register() currently uses a TOCTOU check via
userRepository.findByEmail(...) then userRepository.save(...), which can race
and surface an unhandled DataIntegrityViolationException; remove the redundant
findByEmail check and wrap the userRepository.save(user) (and subsequent
jwtService.generateToken(user) if needed) in a try-catch that catches
org.springframework.dao.DataIntegrityViolationException and translates it into
the existing BusinessRuleException("Email används redan"); follow the pattern
used in UserService.create() to ensure constraint violations are handled
gracefully while preserving passwordEncoder.encode(...) and token generation.
---
Nitpick comments:
In `@src/main/java/org/example/vet1177/security/AuthService.java`:
- Around line 49-69: Extract the duplicated AuthResponse construction into a
single private helper (e.g., private AuthResponse mapToAuthResponse(User user,
String token) or createAuthResponse) and replace both places that call new
AuthResponse(...) (see the return in register and the other AuthResponse return
elsewhere) to call this helper; ensure the helper returns new
AuthResponse(token, user.getId(), user.getName(), user.getEmail(),
user.getRole()) so behavior remains identical and update imports/logging if
needed.
🪄 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: ce445890-4fd7-436a-94fa-74b4ad8eac44
📒 Files selected for processing (4)
src/main/java/org/example/vet1177/exception/GlobalExceptionHandler.javasrc/main/java/org/example/vet1177/security/AuthController.javasrc/main/java/org/example/vet1177/security/AuthService.javasrc/main/java/org/example/vet1177/services/UserService.java
💤 Files with no reviewable changes (1)
- src/main/java/org/example/vet1177/services/UserService.java
| request.name(), | ||
| request.email(), | ||
| passwordEncoder.encode(request.password()), | ||
| Role.OWNER |
There was a problem hiding this comment.
Bra jobbat med AuthService och AuthController login-flödet ser najs ut !
i register():
User user = new User(
request.name(),
request.email(),
passwordEncoder.encode(request.password()),
Role.OWNER // hårdkodat
);
Här är rollen hårdkodad till OWNER, men RegisterRequest har ett role-fält som frontend skickar med. Just nu ignoreras det fältet helt det kan bli förvirrande
Vi behöver bestämma en av dessa två vägar:
Alt 1: Bara OWNER kan registrera sig
Ta bort role-fältet från RegisterRequest helt?Då är det tydligt att register alltid skapar en OWNER. VET och ADMIN skapas av admin via andra endpoints.
Alt 2: OWNER och VET kan registrera sig
Använd request.role() istället för Role.OWNER, men en validering innan:
if (request.role() == Role.ADMIN) {
throw new BusinessRuleException("Admin-konton kan inte skapas via registrering");
}
Har vi tänkt alternativ 1?
There was a problem hiding this comment.
Jag tror alternativ 1 är det bästa alternativet! En VET och ADMIN bör enbart kunna skapas av en admin via POST /api/users, känns väl rimligast i en sån här applikation? Att vanliga användare som registrerar sig via register() får alltid rollen OWNER :)
There was a problem hiding this comment.
Håller med ovan, endast admin kan skapa en VET och en Admin.
…aIntegrityViolationException handling in AuthService
:wq#:wq Ange ett incheckningsmeddelande för att förklara varför sammanslagningen
3 participants
Implementerar autentiseringsflödet med login och registrering, kopplat till det befintliga JWT- och Spring Security-upplägget.
Ändringar
AuthService.java, affärslogik för autentisering:
AuthController.java — REST-endpoints:
GlobalExceptionHandler.java - lagt till handler för BadCredentialsException som returnerar 401 Unauthorized vid fel lösenord
Säkerhet
Lösenord loggas aldrig
Lösenord hashas med BCrypt
JWT genereras via befintlig JwtService
Closes #198
Closes #199
Summary by CodeRabbit
New Features
Bug Fixes
Chores