Conversation
chore: trigger GitOps pipeline
…nd VerificationToken - Implemented RoleRepositoryTest with tests for find, save, update, and delete operations. - Created UserPreferencesRepositoryTest covering save, find, update, and delete functionalities. - Developed UserRepositoryTest to validate user retrieval, existence checks, and updates. - Added VerificationTokenRepositoryTest to ensure proper handling of verification tokens, including expiration and usage checks. - Each test class includes edge cases and verifies database constraints.
…cation, registration, and token management
…gement, role assignments, and permissions
…r TokenService covering token creation, validation, and management
…for improved readability
…P address and null for user agent in AuthServiceTest
…and UserServiceTest
Authentication mvn test
…rification emails upon creation.
…serControllerTest for user profile retrieval
WalkthroughThis pull request enhances the authentication service by introducing employee and admin user creation endpoints with integrated email verification, refactoring controller-level security annotations to method-level in UserController, adding fullName parameter support to user creation workflows, and establishing comprehensive repository and service test coverage across the auth service module. Changes
Sequence DiagramsequenceDiagram
participant Client
participant AuthController
participant UserService
participant UserRepository
participant TokenService
participant EmailService
Client->>AuthController: POST /users/employee (CreateEmployeeRequest)
AuthController->>UserService: createEmployee(username, email, password, fullName)
UserService->>UserRepository: save(User)
UserRepository-->>UserService: User (persisted)
UserService->>TokenService: createVerificationToken(user, EMAIL_VERIFICATION)
TokenService-->>UserService: VerificationToken
UserService->>EmailService: sendVerificationEmail(user, token)
EmailService-->>UserService: email sent
UserService-->>AuthController: User
AuthController-->>Client: 201 Created
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
auth-service/src/main/java/com/techtorque/auth_service/controller/AuthController.java (1)
255-263:/testendpoint Javadoc says “for authenticated users” but is currently publicThe
test()endpoint’s comment describes it as “for authenticated users”, but there is no@PreAuthorizeor other access restriction on the method. As a result, it’s publicly callable, which may not match the intent and could inadvertently expose a probeable endpoint.If you truly want it to be auth‑only, add something like:
@PreAuthorize("isAuthenticated()")(or the appropriate role constraint). If you intend it to be public, update the Javadoc to avoid confusion.
♻️ Duplicate comments (1)
auth-service/src/main/java/com/techtorque/auth_service/dto/request/CreateEmployeeRequest.java (1)
22-27: Field redundancy noted (same as CreateAdminRequest).This DTO has the same
fullNamevsfirstName/lastNameredundancy issue flagged inCreateAdminRequest.java. Please address consistency across both DTOs.
🧹 Nitpick comments (27)
auth-service/src/test/java/com/techtorque/auth_service/repository/UserRepositoryTest.java (2)
32-33: Remove unusedRoleRepositorydependency
RoleRepositoryis injected but never used in this test class. You can safely remove it to reduce noise and avoid confusion about its purpose here.
86-96: Re‑check case‑sensitivity expectations against production DB collationThe tests
findByUsername_CaseSensitive_ShouldReturnEmptyandfindByEmail_CaseSensitive_ShouldReturnEmptyassume strict case‑sensitive matching. Depending on your production DB and its collation (e.g., some MySQL collations are case‑insensitive by default), behavior might differ from the in‑memory test DB, making these tests misleading.Confirm the intended business rule and DB collation; if lookups must be case‑insensitive, adjust repository queries and tests accordingly (e.g., normalize to lower case).
Also applies to: 121-131
auth-service/src/test/java/com/techtorque/auth_service/repository/PermissionRepositoryTest.java (3)
189-212: Exact size/count assertions may become brittle if test DB is seeded
findAll_ShouldReturnAllPermissionsandcount_ShouldReturnCorrectCountassume there are exactly two rows in the table. This is accurate today because each test runs in isolation and you only persist two entities here, but it can break if:
- The test profile starts seeding default permissions (via Flyway/Liquibase/import scripts), or
- Additional setup data is introduced later.
To make these tests more resilient, consider:
- Asserting that the returned collection contains the permissions you persisted (via
extracting("name").contains(...)), and- Relaxing the exact
hasSize(2)/isEqualTo(2)in favor of at-least checks or verifying just the presence of the expected entities.Also applies to: 214-230
180-187: Avoid magic IDs for “non‑existent” lookupsUsing hard‑coded IDs like
999Lfor non‑existent entities is generally safe here, but it’s a bit magic and can be fragile if any future setup ever creates many records.Minor improvement options:
- Introduce a constant like
private static final long NON_EXISTENT_ID = Long.MAX_VALUE;and reuse it, or- Use a negative ID (if your ID generation strategy never produces negatives).
This keeps intent clearer and reduces the chance of accidental collisions.
Also applies to: 308-315
90-103: Reduce duplication inPermissionconstructionThere are several blocks that build
Permissioninstances with different names/descriptions. The tests are still clear, but you could DRY them up slightly with a small factory/helper, e.g.:private Permission permission(String name, String description) { return Permission.builder() .name(name) .description(description) .build(); }Then each test can call
permission("READ_USER", "Permission to read user data"), improving readability and reducing the chance of copy‑paste errors.Also applies to: 192-200, 219-223, 265-268, 282-285
auth-service/src/test/java/com/techtorque/auth_service/repository/UserPreferencesRepositoryTest.java (2)
147-147: Consider removing redundant @transactional annotation.The
@DataJpaTestannotation on the class already provides transactional behavior for all test methods. Adding@Transactionalto this individual test method is redundant unless there's a specific transaction isolation or propagation requirement.- @Test - @Transactional + @Test void deleteByUser_WhenPreferencesExist_ShouldDeleteUserPreferences() {
302-331: Consider simplifying the bulk language test.Creating 10 users and asserting the same conditions in each iteration is somewhat excessive for a unit test. Consider testing 2-3 representative languages and using a final assertion to verify all were persisted correctly.
Example simplification:
@Test void save_WithDifferentLanguages_ShouldPersistSuccessfully() { // Given - String[] languages = { "en", "es", "fr", "de", "it", "pt", "ja", "zh", "ko", "ar" }; + String[] languages = { "en", "es", "zh" }; // Representative sample for (int i = 0; i < languages.length; i++) { User user = User.builder() .username("user" + i) .password("password") .email("user" + i + "@example.com") .enabled(true) .emailVerified(false) .createdAt(LocalDateTime.now()) .build(); user = entityManager.persistAndFlush(user); UserPreferences preferences = UserPreferences.builder() .user(user) .language(languages[i]) .build(); - - // When - UserPreferences savedPreferences = userPreferencesRepository.save(preferences); - - // Then - assertThat(savedPreferences.getId()).isNotNull(); - assertThat(savedPreferences.getLanguage()).isEqualTo(languages[i]); - assertThat(savedPreferences.getUser().getId()).isEqualTo(user.getId()); + userPreferencesRepository.save(preferences); } + + // When/Then - verify all were saved + List<UserPreferences> allPrefs = userPreferencesRepository.findAll(); + assertThat(allPrefs).extracting("language") + .containsExactlyInAnyOrder(languages); }auth-service/src/main/java/com/techtorque/auth_service/service/AuthService.java (1)
106-109: Consider extracting IP/User-Agent logic to reduce duplication.The IP and User-Agent extraction logic using nested ternaries appears multiple times (lines 106-109, 144-145, 162-163, 286-287). Consider extracting this to a private helper method to improve readability and reduce duplication.
Example:
+private String extractIp(HttpServletRequest request) { + if (request == null) return null; + String forwarded = request.getHeader("X-Forwarded-For"); + return forwarded != null ? forwarded : request.getRemoteAddr(); +} + +private String extractUserAgent(HttpServletRequest request) { + return request != null ? request.getHeader("User-Agent") : null; +}Then replace the inline extraction:
-String ip = request != null ? (request.getHeader("X-Forwarded-For") == null ? request.getRemoteAddr() - : request.getHeader("X-Forwarded-For")) : null; -String ua = request != null ? request.getHeader("User-Agent") : null; +String ip = extractIp(request); +String ua = extractUserAgent(request);auth-service/src/test/java/com/techtorque/auth_service/controller/UserControllerTest.java (1)
1-46: Good test structure, but consider expanding coverage.The test class is well-structured with proper use of
@WebMvcTest,@MockBean, and@WithMockUser. However, it currently only covers one happy path scenario.Consider adding tests for:
- User not found scenario (service returns empty Optional)
- Different user roles (ADMIN, EMPLOYEE)
- Unauthorized access attempts
- Error handling paths
Example:
@Test @WithMockUser(username = "testuser", roles = {"CUSTOMER"}) void getCurrentUserProfile_whenUserNotFound_shouldReturn404() throws Exception { when(userService.findByUsername(anyString())).thenReturn(Optional.empty()); mockMvc.perform(get("/users/me")) .andExpect(status().isNotFound()); }auth-service/src/test/java/com/techtorque/auth_service/repository/RoleRepositoryTest.java (1)
101-116: Optional: assert aggregate expectations to complement per‑entity assertionsIn
save_AllRoleNames_ShouldPersistAllEnumValues, you verify each saved entity individually. If you ever want stronger regression protection, you could additionally assert thatroleRepository.findAll()has sizeRoleName.values().lengthand contains all enum names; this would catch accidental filtering or misconfiguration. Not required, just a possible enhancement.auth-service/src/test/java/com/techtorque/auth_service/service/AuthServiceTest.java (1)
317-336: Registration test stubs may hide issues with which user data is used for emailsIn
registerUser_WhenValidRequest_ShouldCreateUserAndSendVerificationEmail,userRepository.saveis stubbed to always returntestUser, and the email expectation assertssendVerificationEmail("test@example.com", "testuser", "verification-token"). In production,saveshould return the newly created user (with"new@example.com"etc.), so this setup can:
- Diverge from real behavior, and
- Allow the test to pass even if
AuthServiceaccidentally uses unexpected user data for emails.A more robust pattern is:
when(userRepository.save(any(User.class))) .thenAnswer(invocation -> invocation.getArgument(0));and then asserting that
sendVerificationEmailis called with the new user’s email/username. This keeps the test aligned with real persistence behavior.auth-service/src/main/java/com/techtorque/auth_service/controller/AuthController.java (4)
168-188: Change‑password error responses use a “success” wrapper and generic RuntimeException catchThe
changePasswordendpoint wraps both success and error responses inApiSuccessand catches a broadRuntimeException. This has two downsides:
- Errors are semantically represented as “success” in the payload.
- Any unchecked exception from downstream code gets exposed as a
400with its message, which can make API contracts brittle.If you plan further refactors, consider introducing a dedicated error body and narrowing the catch to expected exception types (e.g., invalid current password vs other internal errors). Not blocking for this PR.
190-220: New employee-creation endpoint looks correct but can better align docs with implementation
createEmployeecorrectly:
- Restricts access with
@PreAuthorize("hasRole('ADMIN') or hasRole('SUPER_ADMIN')").- Delegates to
userService.createEmployee(username, email, password, fullName).Two small polish points:
- The
@Operationdescription says “Requires ADMIN role”, but the SpEL also allows SUPER_ADMIN; consider clarifying that in the description.- The Javadoc
@paramstill mentions only username/email/password; includefullNamefor consistency with the DTO and service signature.Behavior itself looks fine.
222-243: Admin-creation endpoint lacks OpenAPI metadata and could mirror employee endpoint style
createAdminis wired correctly (SUPER_ADMIN‑only, delegates touserService.createAdminincluding fullName), but unlikecreateEmployeeit has no@Operation/@ApiResponsesannotations. For consistency in your OpenAPI documentation and client generation, consider adding similar annotations describing:
- SUPER_ADMIN‑only access,
- 201 on success,
- 400/401/403 error conditions.
Also update the Javadoc
@paramto mention fullName, matching the new DTO field.
245-253: Health endpoint is simple and fine; consider whether it should be excluded from public API docsThe
/healthendpoint returns a simple status message and is often used by infra rather than client applications. If you don’t want it surfaced prominently in OpenAPI/Swagger, you might annotate it accordingly or move it to an actuator health check. Current implementation is otherwise fine.auth-service/src/test/java/com/techtorque/auth_service/service/UserServiceTest.java (1)
562-627: Security-context based admin-role assignment tests are good; consider safer cleanupThe tests around
assignRoleToUserand SUPER_ADMIN checks correctly:
- Enforce that only SUPER_ADMIN can assign ADMIN.
- Cover both denial and success paths.
- Use
SecurityContextHolderto simulate the current user.One small robustness tweak: instead of manually calling
SecurityContextHolder.clearContext()after assertions, wrap the test body in atry/finallyin case of future assertion failures, ensuring the global context is always reset. Not required, but helps avoid cross-test leakage.auth-service/src/test/java/com/techtorque/auth_service/service/TokenServiceTest.java (1)
359-365: Consider also asserting verification-token cleanup incleanupExpiredTokens
cleanupExpiredTokens_ShouldDeleteExpiredTokenscurrently only verifiesrefreshTokenRepository.deleteExpiredTokens(...). IfTokenService.cleanupExpiredTokens()is also expected to remove expired verification/password-reset tokens, it may be worth adding an assertion onverificationTokenRepositorytoo, to lock in that behavior. If not, the test is fine as-is.auth-service/src/main/java/com/techtorque/auth_service/controller/UserController.java (1)
45-53: Method‑level admin guards look correct; consider deduping and updating docsMoving from class‑level to per‑method
@PreAuthorize("hasRole('ADMIN') or hasRole('SUPER_ADMIN')")on these admin operations keeps them properly locked down while allowing/users/meand profile/preferences endpoints to honor their broader role rules. From this file, no admin‑only operation appears unprotected.To reduce duplication and future drift, consider extracting this expression into a shared constant or custom meta‑annotation (e.g.,
@AdminOnly) and reusing it across these methods. Also, the class‑level JavaDoc (“Admin/Super Admin only”) is now slightly misleading given the user‑level endpoints; you may want to clarify that only specific methods are admin‑only.Also applies to: 58-64, 69-92, 107-137, 143-153
auth-service/src/main/java/com/techtorque/auth_service/service/UserService.java (3)
167-199: Employee/admin creation + verification flow is reasonable; double‑check enabled/emailVerified semantics
createEmployeeandcreateAdminnow:
- enforce unique username/email,
- populate
fullName,- save the user,
- create a verification token, and
- send a verification email via
EmailService.This is a sensible flow. One thing to double‑check: both methods set
enabled(true)on the new users while also issuing verification tokens. If your security model expects email verification to gate login or privileged use, you may want to either initialize these accounts as disabled until verified or ensure downstream code actually enforcesemailVerified(not justenabled) on authentication/authorization paths.If the current design intentionally allows login before verification and only uses the token for softer validation, then the implementation here is fine as‑is.
Also applies to: 212-244
490-524: Role assignment/revocation rules are well‑guarded; consider minor refactor for reuseThe additional checks:
- only allowing a
SUPER_ADMIN(viaROLE_SUPER_ADMINauthority) to assign theADMINrole, and- preventing a user from revoking their own
SUPER_ADMINrole,are good hardening measures and align with least‑privilege principles. The validation of
roleNameviaRoleName.valueOf(roleName.toUpperCase())and subsequentRolelookup is also clear.There is some minor duplication between
assignRoleToUserandrevokeRoleFromUseraround parsingroleNameand loading theRoleentity; if this logic grows further, you could extract a small helper to keep the two methods tighter.Also applies to: 533-567
572-587: Profile update helpers behave predictably; confirm null vs empty semantics
updateProfileandupdateProfilePhotolook straightforward: they load the user, update fields, and save. InupdateProfile, anullparameter means “leave the field unchanged”, which also means callers can’t clear a value (e.g., remove a phone number) unless they explicitly pass an empty string and you treat that as “clear”.If that behavior matches your API contract, these methods are good to go. If you intend to support clearing fields, you may want to distinguish between “not provided” and “provided but blank” at the DTO level and handle that here.
Also applies to: 592-598
auth-service/src/test/java/com/techtorque/auth_service/repository/RefreshTokenRepositoryTest.java (6)
37-68: Fixture setup is good; consider small reuse and determinism tweaksThe role/user/token fixture built in
setUpis clear and representative. If this class grows further, consider extracting small helpers likecreateRefreshToken(...)and optionally basingcreatedAt/expiryDateoff a sharedLocalDateTime baseTimefield to keep time-related expectations deterministic across tests.
70-84: Strengthensave_WhenValidToken_ShouldPersistSuccessfullyby asserting on a DB round‑tripRight now the assertions are performed on the entity returned directly from
save, which may hide mapping issues (e.g., misconfigured columns) because you never re-read from the database.You can make this test more robust by flushing, clearing the persistence context, and asserting on an entity loaded from the DB:
@Test void save_WhenValidToken_ShouldPersistSuccessfully() { // When - RefreshToken savedToken = refreshTokenRepository.save(testToken); - - // Then - assertThat(savedToken.getId()).isNotNull(); - assertThat(savedToken.getToken()).isEqualTo("test-refresh-token"); - assertThat(savedToken.getUser().getId()).isEqualTo(testUser.getId()); - assertThat(savedToken.getExpiryDate()).isNotNull(); - assertThat(savedToken.getCreatedAt()).isNotNull(); - assertThat(savedToken.getIpAddress()).isEqualTo("192.168.1.1"); - assertThat(savedToken.getUserAgent()).isEqualTo("Test User Agent"); - assertThat(savedToken.getRevokedAt()).isNull(); + RefreshToken savedToken = refreshTokenRepository.save(testToken); + entityManager.flush(); + entityManager.clear(); + + RefreshToken persistedToken = + entityManager.find(RefreshToken.class, savedToken.getId()); + + // Then + assertThat(persistedToken.getId()).isNotNull(); + assertThat(persistedToken.getToken()).isEqualTo("test-refresh-token"); + assertThat(persistedToken.getUser().getId()).isEqualTo(testUser.getId()); + assertThat(persistedToken.getExpiryDate()).isNotNull(); + assertThat(persistedToken.getCreatedAt()).isNotNull(); + assertThat(persistedToken.getIpAddress()).isEqualTo("192.168.1.1"); + assertThat(persistedToken.getUserAgent()).isEqualTo("Test User Agent"); + assertThat(persistedToken.getRevokedAt()).isNull(); }
110-159: Fine-graineddeleteByUsercheck; align secondary user data and drop redundant@TransactionalThe logic of creating two tokens for
testUserplus one foranotherUserand asserting only the former are deleted is solid.Two small tweaks to consider:
- Populate
fullNameforanotherUseras well, to keep test data consistent and avoid surprises iffullNameis (or becomes) non-nullable:- User anotherUser = User.builder() - .username("anotheruser") - .password("password") - .email("another@example.com") - .enabled(true) - .emailVerified(false) - .createdAt(LocalDateTime.now()) - .build(); + User anotherUser = User.builder() + .username("anotheruser") + .password("password") + .email("another@example.com") + .fullName("Another User") + .enabled(true) + .emailVerified(false) + .createdAt(LocalDateTime.now()) + .build();
@Transactionalis redundant under@DataJpaTest— each test is already transactional and rolled back by default, so you can safely omit the method-level annotation unless you need a different propagation/rollback behavior:- @Test - @Transactional + @Test void deleteByUser_WhenUserHasTokens_ShouldDeleteAllUserTokens() {
161-198: Use a single reference time indeleteExpiredTokensto avoid subtle time driftThe test currently calls
LocalDateTime.now()multiple times for building tokens and again when invokingdeleteExpiredTokens. While it’s unlikely to break with the current +/-1 day/hour offsets, capturing onenowimproves determinism and readability:@Test - @Transactional void deleteExpiredTokens_WhenExpiredTokensExist_ShouldDeleteOnlyExpiredTokens() { // Given - RefreshToken expiredToken1 = RefreshToken.builder() + LocalDateTime now = LocalDateTime.now(); + + RefreshToken expiredToken1 = RefreshToken.builder() .token("expired-token-1") .user(testUser) - .expiryDate(LocalDateTime.now().minusDays(1)) // Expired - .createdAt(LocalDateTime.now().minusDays(2)) + .expiryDate(now.minusDays(1)) // Expired + .createdAt(now.minusDays(2)) .build(); RefreshToken expiredToken2 = RefreshToken.builder() .token("expired-token-2") .user(testUser) - .expiryDate(LocalDateTime.now().minusHours(1)) // Expired - .createdAt(LocalDateTime.now().minusDays(1)) + .expiryDate(now.minusHours(1)) // Expired + .createdAt(now.minusDays(1)) .build(); RefreshToken validToken = RefreshToken.builder() .token("valid-token") .user(testUser) - .expiryDate(LocalDateTime.now().plusDays(7)) // Not expired - .createdAt(LocalDateTime.now()) + .expiryDate(now.plusDays(7)) // Not expired + .createdAt(now) .build(); @@ - // When - refreshTokenRepository.deleteExpiredTokens(LocalDateTime.now()); + // When + refreshTokenRepository.deleteExpiredTokens(now);This also lets you drop the extra
@Transactionalhere, as with the previous test.
200-232: Entity-levelisExpired/isRevokedtests are clear; placement is a style choiceThe four tests around
isExpired/isRevokedclearly exercise the entity’s boolean helpers for both true/false cases. They’re easy to read and don’t rely on persistence state, so they’ll be cheap to run.If you later introduce a dedicated entity test class (e.g.,
RefreshTokenTestunder a domain/model test package), these would be good candidates to move there so this repository test class focuses purely on repository interactions — but that’s optional and purely organizational.Also applies to: 234-268
293-307: Ensure revoked updates are validated against the database, not just the in-memory entityDetaching and then saving the token is a nice way to simulate an update. As with the save test, you currently assert on the returned entity, which doesn’t fully prove the
revokedAtmapping is persisted correctly.You can make this stronger by flushing, clearing, and reloading before assertions:
@Test void update_WhenTokenIsRevoked_ShouldUpdateRevokedAt() { @@ - LocalDateTime revokedTime = LocalDateTime.now(); - savedToken.setRevokedAt(revokedTime); - RefreshToken updatedToken = refreshTokenRepository.save(savedToken); - - // Then - assertThat(updatedToken.getRevokedAt()).isEqualTo(revokedTime); - assertThat(updatedToken.isRevoked()).isTrue(); + LocalDateTime revokedTime = LocalDateTime.now(); + savedToken.setRevokedAt(revokedTime); + refreshTokenRepository.save(savedToken); + entityManager.flush(); + entityManager.clear(); + + RefreshToken persistedToken = + entityManager.find(RefreshToken.class, savedToken.getId()); + + // Then + assertThat(persistedToken.getRevokedAt()).isEqualTo(revokedTime); + assertThat(persistedToken.isRevoked()).isTrue(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
auth-service/pom.xml(2 hunks)auth-service/src/main/java/com/techtorque/auth_service/controller/AuthController.java(3 hunks)auth-service/src/main/java/com/techtorque/auth_service/controller/UserController.java(7 hunks)auth-service/src/main/java/com/techtorque/auth_service/dto/request/CreateAdminRequest.java(1 hunks)auth-service/src/main/java/com/techtorque/auth_service/dto/request/CreateEmployeeRequest.java(1 hunks)auth-service/src/main/java/com/techtorque/auth_service/service/AuthService.java(13 hunks)auth-service/src/main/java/com/techtorque/auth_service/service/UserService.java(15 hunks)auth-service/src/test/java/com/techtorque/auth_service/controller/UserControllerTest.java(1 hunks)auth-service/src/test/java/com/techtorque/auth_service/repository/LoginLockRepositoryTest.java(1 hunks)auth-service/src/test/java/com/techtorque/auth_service/repository/LoginLogRepositoryTest.java(1 hunks)auth-service/src/test/java/com/techtorque/auth_service/repository/PermissionRepositoryTest.java(1 hunks)auth-service/src/test/java/com/techtorque/auth_service/repository/RefreshTokenRepositoryTest.java(1 hunks)auth-service/src/test/java/com/techtorque/auth_service/repository/RoleRepositoryTest.java(1 hunks)auth-service/src/test/java/com/techtorque/auth_service/repository/UserPreferencesRepositoryTest.java(1 hunks)auth-service/src/test/java/com/techtorque/auth_service/repository/UserRepositoryTest.java(1 hunks)auth-service/src/test/java/com/techtorque/auth_service/repository/VerificationTokenRepositoryTest.java(1 hunks)auth-service/src/test/java/com/techtorque/auth_service/service/AuthServiceTest.java(1 hunks)auth-service/src/test/java/com/techtorque/auth_service/service/TokenServiceTest.java(1 hunks)auth-service/src/test/java/com/techtorque/auth_service/service/UserServiceTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
auth-service/src/test/java/com/techtorque/auth_service/repository/LoginLockRepositoryTest.java (7)
auth-service/src/test/java/com/techtorque/auth_service/repository/LoginLogRepositoryTest.java (1)
DataJpaTest(22-240)auth-service/src/test/java/com/techtorque/auth_service/repository/PermissionRepositoryTest.java (1)
DataJpaTest(21-316)auth-service/src/test/java/com/techtorque/auth_service/repository/RefreshTokenRepositoryTest.java (1)
DataJpaTest(24-357)auth-service/src/test/java/com/techtorque/auth_service/repository/RoleRepositoryTest.java (1)
DataJpaTest(20-268)auth-service/src/test/java/com/techtorque/auth_service/repository/UserPreferencesRepositoryTest.java (1)
DataJpaTest(25-395)auth-service/src/test/java/com/techtorque/auth_service/repository/UserRepositoryTest.java (1)
DataJpaTest(22-382)auth-service/src/test/java/com/techtorque/auth_service/repository/VerificationTokenRepositoryTest.java (1)
DataJpaTest(24-388)
auth-service/src/test/java/com/techtorque/auth_service/repository/LoginLogRepositoryTest.java (7)
auth-service/src/test/java/com/techtorque/auth_service/repository/LoginLockRepositoryTest.java (1)
DataJpaTest(22-286)auth-service/src/test/java/com/techtorque/auth_service/repository/PermissionRepositoryTest.java (1)
DataJpaTest(21-316)auth-service/src/test/java/com/techtorque/auth_service/repository/RefreshTokenRepositoryTest.java (1)
DataJpaTest(24-357)auth-service/src/test/java/com/techtorque/auth_service/repository/RoleRepositoryTest.java (1)
DataJpaTest(20-268)auth-service/src/test/java/com/techtorque/auth_service/repository/UserPreferencesRepositoryTest.java (1)
DataJpaTest(25-395)auth-service/src/test/java/com/techtorque/auth_service/repository/UserRepositoryTest.java (1)
DataJpaTest(22-382)auth-service/src/test/java/com/techtorque/auth_service/repository/VerificationTokenRepositoryTest.java (1)
DataJpaTest(24-388)
🔇 Additional comments (21)
auth-service/src/test/java/com/techtorque/auth_service/repository/UserRepositoryTest.java (1)
18-60: Well-structured, comprehensive repository test coverageThis class gives very solid coverage of
UserRepositorybehavior (CRUD, existence checks, roles loading, profile photo fields, and updates) and usesTestEntityManagerappropriately to control DB state. Good separation of “given/when/then” and clear method naming make the tests easy to understand and maintain.auth-service/src/test/java/com/techtorque/auth_service/repository/PermissionRepositoryTest.java (2)
17-31: Solid JPA test setup and scopeUsing
@DataJpaTestwithTestEntityManagerand a dedicatedtestprofile is a good, focused way to exercisePermissionRepositorybehavior in isolation. Class-level visibility and wiring look correct.
75-85: Be explicit about intended case‑sensitive semantics
findByName_CaseSensitive_ShouldReturnEmptyasserts that lookups are case‑sensitive. That’s fine, but it tightly couples behavior to DB collation and business rules. If you later decide names should be case‑insensitive (or the test DB uses a different collation than prod), this test will start failing.Consider either:
- Documenting that permission names are intentionally case‑sensitive, or
- Adjusting repository logic + test if the domain really needs case‑insensitive matching.
auth-service/src/test/java/com/techtorque/auth_service/repository/UserPreferencesRepositoryTest.java (1)
1-36: LGTM! Well-structured test class setup.The test class properly uses
@DataJpaTestfor repository testing with an activated test profile. The use ofTestEntityManagerfor fixture management and proper autowiring of the repository follows Spring testing best practices.auth-service/pom.xml (1)
180-191: LGTM! Explicit Lombok annotation processing configured.The maven-compiler-plugin configuration correctly enables Lombok annotation processing. The version for the Lombok artifact in
annotationProcessorPathswill be inherited from the dependency management section (line 59-61), which is the standard approach.auth-service/src/main/java/com/techtorque/auth_service/service/AuthService.java (2)
67-72: LGTM! Configurable lock duration enhances flexibility.The addition of
lockDurationMinutesas a configurable property allows runtime customization of account lock duration, improving operational flexibility without code changes.
242-242: LGTM! Using fullName improves email personalization.Sending the verification email with the user's full name instead of username creates a better user experience.
auth-service/src/test/java/com/techtorque/auth_service/repository/LoginLogRepositoryTest.java (1)
1-240: LGTM! Comprehensive repository test coverage.Excellent test suite with thorough coverage of:
- CRUD operations (save, find, delete, count)
- Edge cases (null optional fields, multiple users)
- Custom query methods (deleteByUsername)
- Proper use of
@Transactionalfor delete operations- Clear Given-When-Then structure
The test suite follows Spring Data JPA testing best practices and provides strong confidence in repository behavior.
auth-service/src/test/java/com/techtorque/auth_service/repository/LoginLockRepositoryTest.java (1)
1-286: LGTM! Excellent test coverage with valuable edge case testing.Outstanding test suite that covers:
- CRUD operations with comprehensive assertions
- Default values (failedAttempts defaults to 0, lockUntil nullable)
- Case sensitivity validation (important for security)
- Multiple user scenarios
- Transactional delete operations
The case-sensitivity tests (lines 96-105) are particularly valuable for security validation.
auth-service/src/test/java/com/techtorque/auth_service/repository/VerificationTokenRepositoryTest.java (1)
1-388: LGTM! Exemplary token lifecycle test coverage.Exceptional test suite with excellent coverage of:
- Complete token lifecycle (creation, usage, expiration)
- Multiple token types (EMAIL_VERIFICATION, PASSWORD_RESET)
- State transition validation (isExpired, isUsed)
- Data isolation (lines 148-198: ensures one user's token deletion doesn't affect others)
- Proper entity relationship setup (Role -> User -> Token)
The token state tests (lines 201-272) effectively validate business logic embedded in the entity. This is a model test suite for token management.
auth-service/src/test/java/com/techtorque/auth_service/repository/RoleRepositoryTest.java (1)
20-268: Good coverage and isolation of RoleRepository behaviorThe test suite is well-structured and covers the full CRUD surface (including enum value coverage and null/empty description edge cases) with proper use of
@DataJpaTestandTestEntityManager. Nothing blocking here.auth-service/src/test/java/com/techtorque/auth_service/service/UserServiceTest.java (3)
30-117: Comprehensive and realistic UserService test fixtureThe setup builds a coherent role hierarchy (CUSTOMER/EMPLOYEE/ADMIN/SUPER_ADMIN) and a realistic
testUserwith contact fields, which makes the subsequent tests easy to understand and extend. This is a solid base for the rest of the suite.
181-235: Registration tests clearly encode uniqueness and role-not-found semanticsThe
registerCustomertests correctly capture the expected behavior for:
- Happy path with username/email uniqueness and CUSTOMER role resolution.
- Username and email conflict cases.
- Missing CUSTOMER role signaled via
EntityNotFoundException.These make the contract of
registerCustomervery explicit and are well-structured.
381-395: Delete-user test nicely captures related data cleanup responsibilities
deleteUser_WhenUserExists_ShouldDeleteUserAndRelatedDataasserts that roles are cleared and all related artifacts (refresh tokens, verification tokens, locks, logs) are removed before deleting the user. This is a good safeguard against regressions in cleanup behavior.auth-service/src/test/java/com/techtorque/auth_service/service/TokenServiceTest.java (2)
43-79: Good use of configurable expiries and realistic token fixturesUsing
ReflectionTestUtilsto setverificationExpiryHours,passwordResetExpiryHours, andrefreshTokenExpiryDayskeeps tests decoupled from default config and makes edge-case tests easier. ThetestUser,verificationToken, andrefreshTokenfixtures are minimal yet realistic.
81-142: Expiry window assertions are sensible and unlikely to be flakyIn the creation tests, you assert expiry times relative to
LocalDateTime.now()with comfortable margins (e.g.,isAfter(now.plusHours(23))when configured for 24h). This is a good compromise between verifying correct configuration usage and avoiding brittle, millisecond-sensitive comparisons.auth-service/src/main/java/com/techtorque/auth_service/service/UserService.java (1)
40-64: Constructor injection for new services looks sound; ensure configuration/tests updatedAdding
EmailServiceandTokenServiceasfinalconstructor‑injected deps is idiomatic and keepsUserServicecohesive. Just ensure any Spring config and unit tests that instantiateUserServiceare updated to provide these new collaborators (or mocks) so wiring doesn’t break at runtime.auth-service/src/test/java/com/techtorque/auth_service/repository/RefreshTokenRepositoryTest.java (4)
24-35: Solid JPA slice configuration and wiringUsing
@DataJpaTestwith a dedicatedtestprofile plus autowiredTestEntityManagerandRefreshTokenRepositoryis an idiomatic and focused setup for repository tests; no changes needed here.
86-108: Positive/negativefindByTokencoverage looks correctThe two tests cover both the token-present and token-absent scenarios and assert on key fields and relationships; nothing stands out as problematic here.
270-291: Good coverage offindByIdfor existing and missing tokensPersisting a token via
TestEntityManagerand then verifying both the successful lookup and the empty result for a non-existent ID gives solid coverage of the repository’sfindByIdbehavior.
309-356:findAllandcounttests nicely round out repository coverageThe
findAll_ShouldReturnAllTokenstest verifies both the number of rows and their tokens in a stable, order-insensitive way, andcount_ShouldReturnCorrectCountconfirms the aggregate behavior. Together they give good confidence that the repository behaves correctly over multiple rows.
| @RestController | ||
| // Class-level request mapping removed — gateway strips prefixes before forwarding | ||
| // Class-level request mapping removed — gateway strips prefixes before | ||
| // forwarding | ||
| // @RequestMapping("/api/v1/auth") | ||
| // CORS handled at the API Gateway; remove @CrossOrigin to avoid conflicts | ||
| // @CrossOrigin(origins = "*", maxAge = 3600) | ||
| @Tag(name = "Authentication", description = "Authentication and user management endpoints") | ||
| public class AuthController { | ||
|
|
||
| @Autowired | ||
| private AuthService authService; | ||
|
|
||
| // --- NEW DEPENDENCY --- | ||
| // We need UserService to call the createEmployee method | ||
| @Autowired | ||
| private UserService userService; | ||
|
|
There was a problem hiding this comment.
Class-level mapping removal changes public paths; confirm no direct clients depend on old prefix
With the @RequestMapping("/api/v1/auth") removed and only method-level mappings left, all endpoints move from /api/v1/auth/... to just /... for callers that bypass the gateway. You note the gateway rewrites paths, which is fine, but it’s worth double‑checking that:
- No other internal services or tests rely on the old prefixed paths, and
- Any direct integration documentation is updated accordingly.
If everything goes through the gateway, behavior is consistent, but this is a potentially breaking change for direct callers.
🤖 Prompt for AI Agents
auth-service/src/main/java/com/techtorque/auth_service/controller/AuthController.java
lines 25-41: the class-level @RequestMapping("/api/v1/auth") was removed which
shifts all controller endpoints from /api/v1/auth/... to /...; verify and either
restore the class-level mapping or update callers/tests/docs. Specifically,
search the codebase and integration tests for any direct calls to /api/v1/auth/*
and update them to the new paths or re-add @RequestMapping to preserve the old
prefix, run the full test suite and any API integration tests, and update API
documentation and internal service contracts to reflect the effective public
paths.
| private String fullName; | ||
|
|
||
| // Optional: Additional admin-specific fields | ||
| private String firstName; | ||
| private String lastName; |
There was a problem hiding this comment.
Clarify the relationship between fullName and firstName/lastName fields.
The DTO contains both fullName and individual firstName/lastName fields. This creates ambiguity about which fields should be populated and how they relate to each other. Consider:
- Using only
fullNameif the service layer handles name parsing - Using only
firstName/lastNameif they should be stored separately - Making the relationship explicit (e.g.,
fullNameis derived fromfirstName + lastName, or vice versa)
Similar issue exists in CreateEmployeeRequest.java at lines 22-26.
🤖 Prompt for AI Agents
In
auth-service/src/main/java/com/techtorque/auth_service/dto/request/CreateAdminRequest.java
around lines 22 to 26, the DTO defines both fullName and firstName/lastName
which is ambiguous; update the DTOs to pick a single canonical representation or
make the relationship explicit: either remove fullName and require firstName and
lastName, or keep fullName and remove firstName/lastName, or add clear
Javadoc/validation and a transient getter that derives fullName from
firstName+lastName (or a setter that splits fullName into first/last) so the
contract is unambiguous; apply the same change to CreateEmployeeRequest.java at
the corresponding lines.
| User anotherUser = User.builder() | ||
| .username("anotheruser") | ||
| .password("password") | ||
| .email("another@example.com") | ||
| .enabled(true) | ||
| .emailVerified(false) | ||
| .createdAt(LocalDateTime.now()) | ||
| .build(); | ||
| anotherUser = entityManager.persistAndFlush(anotherUser); |
There was a problem hiding this comment.
Inconsistent user creation pattern across test methods.
The setUp method (line 57) establishes a pattern of associating users with roles via testUser.addRole(testRole), but multiple test methods create users without any role association. For consistency and maintainability, consider following the same user creation pattern throughout all tests, even if roles are optional.
Apply this pattern consistently when creating test users:
User anotherUser = User.builder()
.username("anotheruser")
.password("password")
.email("another@example.com")
.enabled(true)
.emailVerified(false)
.createdAt(LocalDateTime.now())
.build();
+anotherUser.addRole(testRole); // or create and add a new role
anotherUser = entityManager.persistAndFlush(anotherUser);Also applies to: 154-162, 230-248, 279-287, 308-316
🤖 Prompt for AI Agents
In
auth-service/src/test/java/com/techtorque/auth_service/repository/UserPreferencesRepositoryTest.java
around lines 129 to 137 (and similarly at 154-162, 230-248, 279-287, 308-316),
the tests create User instances without attaching roles which is inconsistent
with setUp where testUser.addRole(testRole) is used; modify each user creation
to call addRole(testRole) (or the appropriate test role instance) before
persisting so all test users follow the same role-association pattern for
consistency and maintainability.
| void authenticateUser_WhenUserNotFound_ShouldCheckByEmail() { | ||
| // Given | ||
| when(userRepository.findByUsername("test@example.com")).thenReturn(Optional.empty()); | ||
| lenient().when(userRepository.findByEmail("test@example.com")).thenReturn(Optional.of(testUser)); | ||
| when(loginLockRepository.findByUsername("test@example.com")).thenReturn(Optional.of(loginLock)); | ||
| when(authenticationManager.authenticate(any(UsernamePasswordAuthenticationToken.class))) | ||
| .thenReturn(authentication); | ||
| when(authentication.getPrincipal()).thenReturn(userDetails); | ||
| when(userDetails.getUsername()).thenReturn("test@example.com"); | ||
| when(userDetails.getAuthorities()).thenReturn((Collection) java.util.Collections | ||
| .singletonList(new SimpleGrantedAuthority("ROLE_CUSTOMER"))); | ||
|
|
||
| loginRequest.setUsername("test@example.com"); | ||
|
|
||
| // When/Then | ||
| when(jwtUtil.generateJwtToken(eq(userDetails), anyList())).thenReturn("jwt-token"); | ||
| when(userRepository.findByUsername("test@example.com")).thenReturn(Optional.of(testUser)); | ||
| when(tokenService.createRefreshToken(any(User.class), eq("192.168.1.1"), isNull())) | ||
| .thenReturn("refresh-token"); | ||
|
|
||
| LoginResponse response = authService.authenticateUser(loginRequest, request); | ||
| assertThat(response).isNotNull(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Email fallback login test currently does not exercise the fallback path
In authenticateUser_WhenUserNotFound_ShouldCheckByEmail, you first stub findByUsername("test@example.com") to return empty, but later (Line 224) you override it to return Optional.of(testUser). This means the actual call in authenticateUser will see a found user by username, and findByEmail is never required for the test to pass. To truly cover the fallback, consider:
- Using sequential stubbing, e.g.
when(userRepository.findByUsername("test@example.com")).thenReturn(Optional.empty(), Optional.of(testUser));, and - Verifying that
userRepository.findByEmail("test@example.com")is invoked.
As written, the test name and its effective behavior are misaligned.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the AuthService class and its authenticateUser method
fd -t f -e java "AuthService\.java" | grep -v Test | head -5
Length of output: 79
🏁 Script executed:
#!/bin/bash
# Get the implementation of authenticateUser method
fd -t f -e java "AuthService\.java" | grep -v Test | xargs cat | grep -A 50 "authenticateUser"Length of output: 2656
🏁 Script executed:
#!/bin/bash
# Show the complete test file content around lines 208-230 with more context
fd -t f "AuthServiceTest\.java" | xargs sed -n '200,240p'Length of output: 2322
Fix test to actually exercise the email fallback path
The test authenticateUser_WhenUserNotFound_ShouldCheckByEmail has a double-stubbing issue that prevents the fallback from being tested. Line 209 stubs findByUsername("test@example.com") to return empty, but line 224 overrides it to return Optional.of(testUser). Since later stubs overwrite earlier ones in Mockito, when authenticateUser() executes, it will find the user by username and never invoke the findByEmail() fallback.
To fix this:
- Use sequential stubbing:
when(userRepository.findByUsername("test@example.com")).thenReturn(Optional.empty(), Optional.of(testUser)); - Verify the fallback was invoked:
verify(userRepository).findByEmail("test@example.com");
🤖 Prompt for AI Agents
In
auth-service/src/test/java/com/techtorque/auth_service/service/AuthServiceTest.java
around lines 208 to 230, the test double-stubs userRepository.findByUsername
which causes the email fallback never to be exercised; replace the two separate
stubbings with a single sequential stub (make findByUsername return
Optional.empty() on first call and Optional.of(testUser) on second) and remove
the later overriding when(...) call, then add a
verify(userRepository).findByEmail("test@example.com") assertion to ensure the
fallback path was invoked.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.