Feat/idp implement #42
Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
WalkthroughThis pull request implements updates to enhance WebAuthn (passkey) authentication in a Spring Boot application. Key changes include upgrading the Spring Boot plugin, adding new controllers, services, and DTOs, and modifying security configurations. New endpoints for passkey login, registration, and credential management are introduced, along with client-side JavaScript to facilitate WebAuthn interactions. Additionally, features for password recovery, including OTP verification, are implemented. These updates collectively aim to improve the application's authentication mechanisms by offering a modern alternative to traditional username/password logins. Changes
Sequence DiagramsequenceDiagram
participant User
participant Browser
participant PasskeyController
participant AuthenticatorService
participant UserAuthenticatorRepository
User->>Browser: Initiate Passkey Login
Browser->>PasskeyController: POST /passkey/login
PasskeyController->>AuthenticatorService: authenticate(credentials)
AuthenticatorService->>UserAuthenticatorRepository: Find user authenticator
UserAuthenticatorRepository-->>AuthenticatorService: Return authenticator
AuthenticatorService-->>PasskeyController: Validate credentials
PasskeyController->>Browser: Redirect to /account
Possibly Related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
GiaBaorr
force-pushed
the
feat/idp-implement
branch
from
99fc939 to
ba96e1a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (13)
sep490-idp/src/main/resources/templates/login.html (1)
44-44: Use a descriptive label or ARIA attributes for accessibility.
While the “Sign in by Passkey” button is clear to sighted users, consider adding an ARIA label or more descriptive text for assistive technologies.sep490-idp/src/main/java/sep490/idp/service/impl/AuthenticatorServiceImpl.java (2)
47-61: Refine error handling and improve testability.
- The method catches broad exceptions (Exception e) and simply logs them. This can hide potential issues and make debugging difficult.
- Consider rethrowing or mapping exceptions to user-friendly error messages or relevant HTTP status codes.
- Add unit tests verifying behavior under different error scenarios (e.g., DataConversionException, VerificationException, and generic exceptions).
77-79: Sign count check suggests possible security alert.We log "A cloned authenticator may exist!" if the sign counts do not match, but no additional security measure is taken (e.g., flagging the authenticator, invalidating the session, or blocking login). Validate if you want to incorporate stronger responses for potential cloned authenticators.
sep490-idp/src/main/java/sep490/idp/repository/UserAuthenticatorRepository.java (1)
9-14: Naming & usage consistency.
- Method name “findUserAuthenticatorByUser” is descriptive but consider unifying naming patterns with other repositories (e.g., “findByUser(UserEntity user)”).
- “deleteByIdAndUser” ensures a user can delete only their own credentials, which is good for security. Please confirm you handle the case of non-existing IDs gracefully.
sep490-idp/src/main/java/sep490/idp/entity/UserAuthenticator.java (1)
6-26: Validate uniqueness & performance constraints.
- Confirm that the ‘id’ field has suitable uniqueness constraints in the database.
- If you plan to store large attestation objects, consider any performance overhead or space limitations at scale.
- Also consider adding an index on "user" if queries by user are frequent.
sep490-idp/src/main/java/sep490/idp/service/AuthenticatorService.java (2)
7-11: Add more comprehensive documentation.While the class-level documentation provides a good reference, consider adding more details about the WebAuthn flow and security implications.
Add the following documentation:
/** * Interface for managing the registration and verification of authenticators. * Reference: https://webauthn4j.github.io/webauthn4j/en/#feature + * + * This service handles the core WebAuthn operations: + * - Registration of new passkeys + * - Authentication using existing passkeys + * - Credential management (deletion) + * + * Security considerations: + * - Credentials are bound to specific users + * - No listing of credentials to prevent enumeration attacks + */
13-17: Add method-level documentation and consider validation method.The interface methods should be documented for better maintainability. Also, consider adding a method to validate credential ownership.
Add the following documentation and method:
+ /** + * Stores new WebAuthn credentials during registration. + * @param registration The registration data from the authenticator + * @param user The user to associate the credentials with + */ void saveCredentials(CredentialsRegistration registration, UserEntity user); + /** + * Authenticates a user using their WebAuthn credentials. + * @param verification The verification data from the authenticator + * @return The authenticated user entity + * @throws AuthenticationException if verification fails + */ UserEntity authenticate(CredentialsVerification verification); + /** + * Deletes a specific credential for a user. + * @param user The user owning the credential + * @param credentialId The ID of the credential to delete + * @return true if deletion was successful + */ boolean deleteCredential(UserEntity user, String credentialId); + /** + * Validates if a credential belongs to a user. + * @param user The user to check + * @param credentialId The credential ID to validate + * @return true if the credential belongs to the user + */ + boolean validateCredentialOwnership(UserEntity user, String credentialId);sep490-idp/src/main/java/sep490/idp/service/impl/LoginService.java (1)
16-17: Consider security implications of direct servlet access.Injecting HttpServletRequest/Response directly might expose unnecessary HTTP context. Consider using a service abstraction for session management.
Consider creating a SessionManager service:
@Service public class SessionManager { private final HttpServletRequest request; private final HttpServletResponse response; public void createSession(Authentication auth) { SecurityUtils.storeAuthenticationToContext(auth, request, response); } public void invalidateSession() { // Session cleanup logic } }sep490-idp/src/main/java/sep490/idp/security/UserAuthenticationToken.java (1)
7-7: Add serialVersionUID for Serializable implementationSince the class implements Serializable, add a serialVersionUID to ensure proper serialization compatibility across different versions.
+ private static final long serialVersionUID = 1L;sep490-idp/src/main/java/sep490/idp/utils/SecurityUtils.java (1)
16-16: Consider making SecurityContextRepository configurableThe SecurityContextRepository is hardcoded. Consider making it configurable through application properties or Spring configuration.
sep490-idp/src/main/java/sep490/idp/controller/PasskeyController.java (1)
19-22: Add request mapping prefixConsider adding a @RequestMapping prefix to group all passkey endpoints:
@Controller +@RequestMapping("/passkey") @RequiredArgsConstructor @Slf4j public class PasskeyController {sep490-idp/src/main/java/sep490/idp/controller/MainController.java (2)
5-5: Add logging statements since @slf4j is declaredThe @slf4j annotation is added but no logging statements are implemented. Consider adding appropriate logging for authentication events and errors.
Also applies to: 20-21
66-74: Add null checks and logging for account page accessWhile the implementation is good, consider adding:
- Null checks for userContextData and authenticators
- Logging for account page access and authenticator retrieval
@GetMapping("/account") public String accountPage(@AuthenticationPrincipal UserContextData userContextData, Model model) { + log.info("Account page accessed by user: {}", userContextData.getUserEntity().getEmail()); model.addAttribute("challenge", UUID.randomUUID().toString()); + if (userContextData == null || userContextData.getUserEntity() == null) { + log.error("Invalid user context data"); + return "redirect:/login"; + } var authenticators = authenticatorRepository.findUserAuthenticatorByUser(userContextData.getUserEntity()); + log.debug("Retrieved {} authenticators for user", authenticators.size()); model.addAttribute("userId", userContextData.getUserEntity().getId()); model.addAttribute("email", userContextData.getUserEntity().getEmail()); model.addAttribute("authenticators", authenticators);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
sep490-idp/src/main/resources/static/favicon.icois excluded by!**/*.ico
📒 Files selected for processing (15)
sep490-idp/build.gradle(2 hunks)sep490-idp/src/main/java/sep490/idp/configs/SecurityConfig.java(3 hunks)sep490-idp/src/main/java/sep490/idp/controller/MainController.java(3 hunks)sep490-idp/src/main/java/sep490/idp/controller/PasskeyController.java(1 hunks)sep490-idp/src/main/java/sep490/idp/dto/CredentialsRegistration.java(1 hunks)sep490-idp/src/main/java/sep490/idp/dto/CredentialsVerification.java(1 hunks)sep490-idp/src/main/java/sep490/idp/entity/UserAuthenticator.java(1 hunks)sep490-idp/src/main/java/sep490/idp/repository/UserAuthenticatorRepository.java(1 hunks)sep490-idp/src/main/java/sep490/idp/security/UserAuthenticationToken.java(1 hunks)sep490-idp/src/main/java/sep490/idp/service/AuthenticatorService.java(1 hunks)sep490-idp/src/main/java/sep490/idp/service/impl/AuthenticatorServiceImpl.java(1 hunks)sep490-idp/src/main/java/sep490/idp/service/impl/LoginService.java(1 hunks)sep490-idp/src/main/java/sep490/idp/utils/SecurityUtils.java(1 hunks)sep490-idp/src/main/resources/templates/account-page.html(1 hunks)sep490-idp/src/main/resources/templates/login.html(3 hunks)
🔇 Additional comments (12)
sep490-idp/src/main/resources/templates/login.html (1)
6-7: Ensure the meta tags align with Thymeleaf configuration.
Using Thymeleaf expressions to expose CSRF header and token as meta tags is a common pattern; just verify that server-side CSRF handling works in tandem with these fields.
sep490-idp/build.gradle (1)
46-46:
Security: Update WebAuthn4J to the latest version
The specified WebAuthn4J version (0.28.3.RELEASE) is significantly outdated. Using older versions may expose your application to known security vulnerabilities.
Let's verify the latest version and any security advisories:
Apply this update:
-implementation 'com.webauthn4j:webauthn4j-core:0.28.3.RELEASE'
+implementation 'com.webauthn4j:webauthn4j-core:0.95.1.RELEASE'sep490-idp/src/main/java/sep490/idp/service/impl/AuthenticatorServiceImpl.java (4)
37-43: Ensure concurrency safety of shared fields & session usage.
The HttpSession and service-level fields (e.g., webAuthnManager, attestationConverter) are referenced in multiple methods. If multiple concurrent requests are expected to interact with this service, ensure proper session-handling or concurrency measures (e.g., the challenge is stored in session). For example, consider storing per-session or per-request challenge data in a more scoped context to avoid potential concurrency issues between users sharing the same session or for parallel requests.
73-74: Verify server property configuration in AuthenticationParameters.
Passing null for the serverProperty.getRpIdHash() may be valid, but double-check that it won't break signature verification logic. Additionally, confirm that the booleans "true, true" for user verification and user presence checks meet your intended security requirements.
110-110: Confirm credential ID encoding.
Using verification.id().getBytes() might introduce encoding inconsistencies if the credential ID contains special or multibyte characters. Verify that this matches the actual credential ID representation used during registration.
127-132: Restrictive algorithm in registration parameters.
You are limiting possible algorithms to only ES256. Verify that your passkey devices or clients only produce ES256 signatures. If you want broader support (e.g., RS256, EdDSA), consider adding them to the list.
sep490-idp/src/main/java/sep490/idp/dto/CredentialsVerification.java (1)
1-6: Data mapping for WebAuthn verification looks good.
The record structure and field naming align well with WebAuthn data objects. Ensure that callers properly Base64Url-decode the data provided in these fields.
sep490-idp/src/main/java/sep490/idp/security/UserAuthenticationToken.java (1)
28-31: Review the always-authenticated behavior
The token being always authenticated (isAuthenticated() always returns true) could pose a security risk. Consider implementing proper authentication state management.
Let's verify the usage pattern:
✅ Verification successful
The always-authenticated behavior is appropriate in this context
The UserAuthenticationToken is used correctly within a secure authentication flow. It's only created after a successful login through Spring Security's authentication process, as evidenced by:
- The token is only instantiated in
LoginServiceafter user validation - The security configuration (
SecurityConfig.java) properly sets up protected routes and authentication requirements - The token is stored in Spring Security's context through
SecurityUtils.storeAuthenticationToContext - The application uses Spring Authorization Server with proper OAuth2/OIDC configuration
The isAuthenticated() returning true is acceptable because this token represents an already authenticated session, and the setAuthenticated() method is properly locked down to prevent tampering.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for UserAuthenticationToken instantiation to understand the authentication flow
rg -A 5 "new UserAuthenticationToken"
Length of output: 467
Script:
#!/bin/bash
# Let's check the LoginService implementation to understand the authentication flow
rg -A 10 "class LoginService" sep490-idp/src/main/java/sep490/idp/service/impl/LoginService.java
# Also check where SecurityUtils.storeAuthenticationToContext is used
rg -A 5 "storeAuthenticationToContext"
# Let's see the full UserAuthenticationToken class
cat sep490-idp/src/main/java/sep490/idp/security/UserAuthenticationToken.java
Length of output: 3091
Script:
#!/bin/bash
# Let's check if there are any security filters or authentication providers that might validate this token
rg -l "AuthenticationProvider|SecurityFilter"
# Also check for any Spring Security configuration
rg -l "WebSecurityConfigurerAdapter|SecurityFilterChain|SecurityConfiguration"
# Check for any JWT or token validation related code
rg -l "JwtToken|TokenValidator|AuthenticationManager"
Length of output: 365
Script:
#!/bin/bash
# Let's examine the SecurityConfig to understand the authentication setup
cat sep490-idp/src/main/java/sep490/idp/configs/SecurityConfig.java
# Also check if there are any custom authentication filters
rg -l "extends.*Filter"
Length of output: 8270
sep490-idp/src/main/java/sep490/idp/controller/MainController.java (2)
37-37: LGTM! Secure challenge generation
The UUID provides a cryptographically secure challenge for WebAuthn authentication.
63-64: LGTM! Proper post-authentication redirect
The redirect prevents form resubmission and follows security best practices.
sep490-idp/src/main/resources/templates/account-page.html (1)
6-7: LGTM! Proper CSRF protection implementation
CSRF tokens are correctly implemented using meta tags.
sep490-idp/src/main/java/sep490/idp/configs/SecurityConfig.java (1)
85-89: LGTM! Proper security configuration for WebAuthn endpoints
The security configuration correctly permits WebAuthn-related endpoints while maintaining security for other routes.
sep490-idp/src/main/java/sep490/idp/service/impl/AuthenticatorServiceImpl.java
Show resolved
Hide resolved
sep490-idp/src/main/java/sep490/idp/service/impl/AuthenticatorServiceImpl.java
Outdated
Show resolved
Hide resolved
sep490-idp/src/main/java/sep490/idp/configs/SecurityConfig.java
Outdated
Show resolved
Hide resolved
GiaBaorr
force-pushed
the
feat/idp-implement
branch
from
ba96e1a to
1f6124e
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (2)
sep490-idp/src/main/java/sep490/idp/configs/SecurityConfig.java (1)
98-101:⚠️ Potential issueExternalize WebAuthn configuration
The WebAuthn configuration contains hardcoded development values that should be externalized to configuration files.
Additionally, ensure that the rpId matches your production domain when deploying to production, as WebAuthn security model relies on correct relying party identification.
sep490-idp/src/main/java/sep490/idp/service/impl/AuthenticatorServiceImpl.java (1)
120-123:⚠️ Potential issuePotential NullPointerException on session attribute.
The code directly calls toString() on the session attribute without null checking.
private ServerProperty buildServerPropertyFromSessionChallenge() { - String challenge = httpSession.getAttribute("challenge").toString(); + Object challengeAttr = httpSession.getAttribute("challenge"); + if (challengeAttr == null) { + throw new IllegalStateException("No challenge found in session"); + } + String challenge = challengeAttr.toString(); return new ServerProperty(getOrigin(), getRpId(), new DefaultChallenge(challenge.getBytes()), null); }
🧹 Nitpick comments (5)
sep490-idp/src/main/java/sep490/idp/service/impl/AuthenticatorServiceImpl.java (2)
41-42: Consider abstracting HTTP request/session dependencies.Direct injection of
HttpServletRequestandHttpSessionmakes the service harder to test and tightly couples it to the web layer. Consider creating a service interface for session management.Example abstraction:
public interface WebAuthnSessionManager { String getChallenge(); String getRpId(); Origin getOrigin(); }
88-92: Enhance cloned authenticator detection.The current implementation only logs a warning when a cloned authenticator is detected. Consider implementing a more robust security measure.
Suggestions:
- Track failed sign count checks
- Implement a policy to disable the credential after multiple failures
- Notify the user of potential security breach
sep490-idp/src/main/resources/templates/account-page.html (1)
17-25: Consider enhancing account information display.The current implementation only shows the user's email. Consider adding more relevant account information such as:
- Account creation date
- Last login time
- Security settings status
sep490-idp/src/main/java/sep490/idp/controller/MainController.java (2)
37-37: Consider adding debug logging for authentication attemptsSince you have
@Slf4jconfigured, consider adding debug logging to track login attempts and generated challenges. This would help with security monitoring and debugging.@GetMapping("/login") public String loginPage(@RequestParam(value = "error", required = false) String error, Model model) { + log.debug("Login page accessed. Error present: {}", error != null); model.addAttribute("challenge", UUID.randomUUID().toString());
63-64: Consider adding success loggingAdd debug logging to track successful operations for better monitoring and debugging capabilities.
@GetMapping("/success") public String success() { + log.debug("Successful operation, redirecting to account page"); return "redirect:/account"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
sep490-idp/src/main/resources/static/favicon.icois excluded by!**/*.ico
📒 Files selected for processing (15)
sep490-idp/build.gradle(2 hunks)sep490-idp/src/main/java/sep490/idp/configs/SecurityConfig.java(3 hunks)sep490-idp/src/main/java/sep490/idp/controller/MainController.java(3 hunks)sep490-idp/src/main/java/sep490/idp/controller/PasskeyController.java(1 hunks)sep490-idp/src/main/java/sep490/idp/dto/CredentialsRegistration.java(1 hunks)sep490-idp/src/main/java/sep490/idp/dto/CredentialsVerification.java(1 hunks)sep490-idp/src/main/java/sep490/idp/entity/UserAuthenticator.java(1 hunks)sep490-idp/src/main/java/sep490/idp/repository/UserAuthenticatorRepository.java(1 hunks)sep490-idp/src/main/java/sep490/idp/security/UserAuthenticationToken.java(1 hunks)sep490-idp/src/main/java/sep490/idp/service/AuthenticatorService.java(1 hunks)sep490-idp/src/main/java/sep490/idp/service/impl/AuthenticatorServiceImpl.java(1 hunks)sep490-idp/src/main/java/sep490/idp/service/impl/LoginService.java(1 hunks)sep490-idp/src/main/java/sep490/idp/utils/SecurityUtils.java(1 hunks)sep490-idp/src/main/resources/templates/account-page.html(1 hunks)sep490-idp/src/main/resources/templates/login.html(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- sep490-idp/src/main/java/sep490/idp/entity/UserAuthenticator.java
- sep490-idp/src/main/java/sep490/idp/service/AuthenticatorService.java
- sep490-idp/src/main/java/sep490/idp/service/impl/LoginService.java
- sep490-idp/build.gradle
- sep490-idp/src/main/java/sep490/idp/security/UserAuthenticationToken.java
- sep490-idp/src/main/java/sep490/idp/dto/CredentialsVerification.java
- sep490-idp/src/main/resources/templates/login.html
- sep490-idp/src/main/java/sep490/idp/dto/CredentialsRegistration.java
- sep490-idp/src/main/java/sep490/idp/repository/UserAuthenticatorRepository.java
- sep490-idp/src/main/java/sep490/idp/controller/PasskeyController.java
🔇 Additional comments (11)
sep490-idp/src/main/java/sep490/idp/configs/SecurityConfig.java (2)
63-64: Address deprecated OAuth2 authorization server configuration
The use of OAuth2AuthorizationServerConfiguration.applyDefaultSecurity is deprecated and should be replaced with the modern OAuth2AuthorizationServerConfigurer approach as previously suggested.
85-89: Verify the security implications of the new permitted paths
The addition of /passkey/login to permitted paths is necessary for WebAuthn implementation. However, let's verify that no sensitive endpoints are accidentally exposed.
✅ Verification successful
Security configuration for /passkey/login is appropriate and safe
The verification confirms that the security configuration is correctly implemented:
/passkey/loginendpoint in PasskeyController only handles WebAuthn authentication- The endpoint is properly secured through WebAuthn configuration in SecurityConfig
- The implementation in AuthenticatorServiceImpl includes proper verification steps and security checks
- No sensitive operations are exposed through this public endpoint
- The authentication flow follows WebAuthn security standards with proper challenge verification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all endpoints and their security constraints
# Check for any controller endpoints that might conflict with the permitted paths
ast-grep --pattern 'RequestMapping($path)'
# Check for any additional security annotations on these paths
rg -l "@PreAuthorize|@Secured|@RolesAllowed"
Length of output: 202
Script:
#!/bin/bash
# Let's try a more comprehensive check for endpoints and security configurations
# Find all Controller classes and their mappings
ast-grep --pattern 'class $_ {
$$$
@$_Mapping($$$)
$$$
}'
# Search for PasskeyController specifically
rg -A 5 "class.*PasskeyController"
# Check SecurityConfig for all security rules
rg -A 10 "requestMatchers|antMatcher" src/main/java/sep490/idp/configs/SecurityConfig.java
# Look for any WebAuthn related endpoints
rg -l "passkey|webauthn" --type java
Length of output: 1388
Script:
#!/bin/bash
# Let's examine the PasskeyController endpoints and their implementation
# Check PasskeyController content
cat sep490-idp/src/main/java/sep490/idp/controller/PasskeyController.java
# Look for any authentication-related services implementation
cat sep490-idp/src/main/java/sep490/idp/service/impl/AuthenticatorServiceImpl.java
# Check the actual security config content
cat sep490-idp/src/main/java/sep490/idp/configs/SecurityConfig.java
Length of output: 17415
sep490-idp/src/main/java/sep490/idp/utils/SecurityUtils.java (3)
3-9: LGTM! Appropriate imports for security context handling
The added imports properly support the new HTTP session-based security context management functionality.
16-16: LGTM! Thread-safe security context repository implementation
The HttpSessionSecurityContextRepository is a thread-safe implementation suitable for managing security contexts across HTTP sessions.
30-44: LGTM! Robust implementation with proper error handling
The implementation addresses previous concerns by including:
- Null parameter validation
- Proper error handling with context cleanup
- Thread-safe context management
sep490-idp/src/main/resources/templates/account-page.html (3)
1-10: LGTM! Proper HTML structure with security measures.
The HTML structure follows best practices with appropriate meta tags and CSRF protection.
44-45: Add input validation for passkey name.
The passkey name field still lacks proper validation.
93-115: Improve error handling and user feedback.
The current implementation lacks proper error handling and user feedback.
sep490-idp/src/main/java/sep490/idp/controller/MainController.java (3)
5-6: LGTM! Well-structured logging and session management setup
The addition of @Slf4j for logging and @SessionAttributes("challenge") for managing WebAuthn challenges follows security best practices.
Also applies to: 20-21
27-27: LGTM! Proper dependency injection
The repository is correctly injected using constructor injection via @RequiredArgsConstructor, following Spring best practices.
66-74: 🛠️ Refactor suggestion
Consider architectural improvements for the account page implementation
While the basic functionality is correct, there are several areas for improvement:
- The repository access should be moved to a service layer to maintain proper separation of concerns.
- Add error handling for the repository call.
- Consider adding logging for monitoring and debugging.
Here's a suggested refactor:
@GetMapping("/account")
public String accountPage(@AuthenticationPrincipal UserContextData userContextData, Model model) {
+ log.debug("Accessing account page for user: {}", userContextData.getUserEntity().getEmail());
model.addAttribute("challenge", UUID.randomUUID().toString());
- var authenticators = authenticatorRepository.findUserAuthenticatorByUser(userContextData.getUserEntity());
+ try {
+ var authenticators = userService.getUserAuthenticators(userContextData.getUserEntity());
+ model.addAttribute("userId", userContextData.getUserEntity().getId());
+ model.addAttribute("email", userContextData.getUserEntity().getEmail());
+ model.addAttribute("authenticators", authenticators);
+ } catch (Exception e) {
+ log.error("Error fetching user authenticators", e);
+ model.addAttribute(ERROR_MSG, "Error loading account details");
+ return "error";
+ }
return "account-page";
}Let's verify if the UserService already has similar methods we can reuse:
sep490-idp/src/main/java/sep490/idp/service/impl/AuthenticatorServiceImpl.java
Show resolved
Hide resolved
sep490-idp/src/main/java/sep490/idp/service/impl/AuthenticatorServiceImpl.java
Outdated
Show resolved
Hide resolved
sep490-idp/src/main/java/sep490/idp/service/impl/AuthenticatorServiceImpl.java
Show resolved
Hide resolved
sep490-idp/src/main/java/sep490/idp/service/impl/AuthenticatorServiceImpl.java
Show resolved
Hide resolved
GiaBaorr
force-pushed
the
feat/idp-implement
branch
from
1f6124e to
112118e
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
sep490-idp/src/main/java/sep490/idp/utils/SecurityUtils.java (1)
22-28:⚠️ Potential issueAdd null checks to prevent NullPointerException
The method is susceptible to NullPointerException when accessing the security context chain. This issue was previously identified and should be addressed.
Apply this defensive implementation:
public static Optional<UserContextData> getUserContextData() { + SecurityContext context = SecurityContextHolder.getContext(); + if (context == null || context.getAuthentication() == null) { + return Optional.empty(); + } Object principal = SecurityContextHolder.getContext().getAuthentication().getPrincipal(); if (principal instanceof UserContextData currentUser) { return Optional.of(currentUser); } return Optional.empty(); }sep490-idp/src/main/java/sep490/idp/service/impl/AuthenticatorServiceImpl.java (3)
43-43:⚠️ Potential issueConsider using strict WebAuthnManager for production.
The non-strict WebAuthnManager skips important security validations.
47-60:⚠️ Potential issueImprove error handling in saveCredentials method.
The current implementation swallows DataConversionException and VerificationException, which could hide registration failures from the user.
120-123:⚠️ Potential issueAdd null check for session challenge.
Direct toString() call on session attribute could cause NullPointerException.
🧹 Nitpick comments (4)
sep490-idp/src/main/resources/templates/login.html (1)
44-44: Enhance button accessibility.While changing to a button element is good, consider adding accessibility attributes:
-<button type="button" id="sign-in-by-passkey" class="text-green-500 hover:underline" th:text="#{login.btn.signInByPasskey}"></button> +<button + type="button" + id="sign-in-by-passkey" + class="text-green-500 hover:underline" + th:text="#{login.btn.signInByPasskey}" + aria-label="Sign in with passkey" + role="button"> +</button>sep490-idp/src/main/java/sep490/idp/repository/UserAuthenticatorRepository.java (1)
11-17: Consider adding an index on frequently queried fieldsSince
credentialIdis presumably queried often (e.g., finding or deleting credentials by ID), consider verifying that a database index is applied. Typically, unique columns (likecredential_id) are automatically indexed, but ensure that this is confirmed in your schema to optimize performance.sep490-idp/src/main/java/sep490/idp/entity/UserAuthenticator.java (1)
27-28: Consider using @lob for large binary dataIf
attestationObjectcan contain large binary data, applying@Lob(depending on the JPA provider) can help ensure appropriate handling of large objects.sep490-idp/src/main/java/sep490/idp/controller/MainController.java (1)
20-21: Consider using SecureRandom for challenge generation.While UUID.randomUUID() provides good randomness, for WebAuthn challenges it's recommended to use SecureRandom to generate the challenge bytes directly. This ensures optimal security for the authentication process.
-model.addAttribute("challenge", UUID.randomUUID().toString()); +byte[] challengeBytes = new byte[32]; +new SecureRandom().nextBytes(challengeBytes); +model.addAttribute("challenge", Base64.getUrlEncoder().withoutPadding().encodeToString(challengeBytes));Also applies to: 37-37
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
sep490-idp/src/main/resources/static/favicon.icois excluded by!**/*.ico
📒 Files selected for processing (16)
sep490-idp/build.gradle(2 hunks)sep490-idp/src/main/java/sep490/idp/configs/SecurityConfig.java(3 hunks)sep490-idp/src/main/java/sep490/idp/controller/MainController.java(3 hunks)sep490-idp/src/main/java/sep490/idp/controller/PasskeyController.java(1 hunks)sep490-idp/src/main/java/sep490/idp/dto/CredentialsRegistration.java(1 hunks)sep490-idp/src/main/java/sep490/idp/dto/CredentialsVerification.java(1 hunks)sep490-idp/src/main/java/sep490/idp/entity/UserAuthenticator.java(1 hunks)sep490-idp/src/main/java/sep490/idp/repository/UserAuthenticatorRepository.java(1 hunks)sep490-idp/src/main/java/sep490/idp/security/UserAuthenticationToken.java(1 hunks)sep490-idp/src/main/java/sep490/idp/service/AuthenticatorService.java(1 hunks)sep490-idp/src/main/java/sep490/idp/service/impl/AuthenticatorServiceImpl.java(1 hunks)sep490-idp/src/main/java/sep490/idp/service/impl/LoginService.java(1 hunks)sep490-idp/src/main/java/sep490/idp/utils/SecurityUtils.java(1 hunks)sep490-idp/src/main/resources/db/migration/V0.0.1.1__UserAuthenticators.sql(1 hunks)sep490-idp/src/main/resources/templates/account-page.html(1 hunks)sep490-idp/src/main/resources/templates/login.html(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- sep490-idp/src/main/java/sep490/idp/dto/CredentialsVerification.java
- sep490-idp/build.gradle
- sep490-idp/src/main/java/sep490/idp/service/AuthenticatorService.java
- sep490-idp/src/main/java/sep490/idp/service/impl/LoginService.java
- sep490-idp/src/main/java/sep490/idp/configs/SecurityConfig.java
- sep490-idp/src/main/java/sep490/idp/security/UserAuthenticationToken.java
- sep490-idp/src/main/java/sep490/idp/dto/CredentialsRegistration.java
- sep490-idp/src/main/resources/templates/account-page.html
🔇 Additional comments (14)
sep490-idp/src/main/java/sep490/idp/utils/SecurityUtils.java (3)
3-8: LGTM! Appropriate imports and field declaration.
The new imports and SecurityContextRepository field declaration are well-structured and appropriate for managing security contexts in a web environment.
Also applies to: 16-16
30-44: Add logging for security audit trail
The implementation is robust with proper null checks, error handling, and context cleanup. However, for security audit purposes, logging should be added for authentication events.
Add logging at key points:
public static void storeAuthenticationToContext(UserAuthenticationToken authenticationToken,
HttpServletRequest request, HttpServletResponse response) {
if (authenticationToken == null || request == null || response == null) {
+ log.warn("Attempt to store null authentication context");
throw new IllegalArgumentException("Parameters cannot be null");
}
try {
SecurityContext newContext = SecurityContextHolder.createEmptyContext();
newContext.setAuthentication(authenticationToken);
SecurityContextHolder.setContext(newContext);
securityContextRepository.saveContext(SecurityContextHolder.getContext(), request, response);
+ log.info("Successfully stored authentication context for user: {}",
+ authenticationToken.getPrincipal().getUsername());
} catch (Exception e) {
SecurityContextHolder.clearContext();
+ log.error("Failed to store authentication context", e);
throw new SecurityException("Failed to store authentication context", e);
}
}1-44: Verify proper usage of SecurityUtils across the codebase
Let's verify that this utility class is being used correctly in authentication workflows.
✅ Verification successful
SecurityUtils implementation and usage verified as correct
The verification shows that SecurityUtils is being used appropriately across the codebase:
- LoginService correctly uses
storeAuthenticationToContextwith proper parameter handling getUserContextDatais properly utilized in AuditorAwareImpl for auditing purposes- No direct usage of SecurityContextHolder outside of SecurityUtils class
- Proper error handling is in place with try-catch blocks and context clearing
- SecurityContextHolder operations are properly encapsulated within the utility methods
The implementation follows security best practices by:
- Centralizing security context management
- Providing null-safety checks
- Including proper error handling
- Using appropriate Spring Security components (SecurityContextRepository)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for SecurityUtils usage patterns
echo "=== Searching for SecurityUtils.storeAuthenticationToContext usage ==="
rg -A 3 "SecurityUtils\.storeAuthenticationToContext"
echo -e "\n=== Checking for potential missing error handling ==="
ast-grep --pattern 'try {
$$$
SecurityUtils.$_($$$)
$$$
}'
echo -e "\n=== Checking for direct SecurityContextHolder usage that should use SecurityUtils ==="
rg "SecurityContextHolder\.getContext\(\)\.setAuthentication"
Length of output: 933
Script:
#!/bin/bash
# Let's check the implementation of LoginService to verify proper error handling
echo "=== Checking LoginService implementation ==="
rg -A 10 "class LoginService" sep490-idp/src/main/java/sep490/idp/service/impl/LoginService.java
echo -e "\n=== Checking for getUserContextData usage ==="
rg -A 3 "SecurityUtils\.getUserContextData"
echo -e "\n=== Checking for other potential SecurityContextHolder direct usage ==="
rg "SecurityContextHolder" --type java
Length of output: 2154
sep490-idp/src/main/resources/templates/login.html (3)
6-7: LGTM! CSRF protection properly implemented.
The CSRF token implementation follows Spring Security best practices with proper meta tags and Thymeleaf expressions.
81-81: Validate credential response before sending.
The entire credentials object is being sent to the server.
91-91: Remove informal console logging.
The "yep, done" console log is informal and unnecessary.
sep490-idp/src/main/java/sep490/idp/controller/PasskeyController.java (5)
1-2: File and Package Annotation Looks Good
The package declaration and initial setup are appropriate. No issues identified here regarding naming or placement.
19-22: Annotations and Constructor Injection
Using @Controller, @RequiredArgsConstructor, and @Slf4j together is a good practice. Constructor-based dependency injection is recommended over field injection for easier testing and clarity.
27-34: Add security measures to login endpoint
This repeats a previous suggestion about adding security hardening measures—CSRF protection, basic rate limiting, session invalidation, etc. It helps prevent session fixation attacks and provides more robust security.
36-41: Add validation for credential registration
This reiterates the recommendation to validate the incoming CredentialsRegistration object. Use appropriate checks (e.g., null checks or format checks) to ensure integrity of the data before saving.
43-44: Rename method to reflect credential deletion
This method is named login but actually handles passkey/credential deletion. Use a more descriptive name such as deleteCredential to avoid confusion and reflect the true functionality.
sep490-idp/src/main/resources/db/migration/V0.0.1.1__UserAuthenticators.sql (2)
1-10: Confirm table naming conventions and constraints
- Table Name: Ensure that naming the table
authenticatoraligns with your overall schema naming standards. - Constraints: The unique constraint on
credential_idoffers performance benefits by implicitly creating an index, but ensure this matches your domain requirements (no duplicates allowed). - Validate
gen_random_uuid(): Verify that your environment or DB version fully supports thegen_random_uuid()function.
11-14: Foreign key references
The foreign key constraint referencing users(id) is correct. Make sure there's a migration or existing schema that defines the users table with a matching id column of type UUID to maintain referential integrity.
sep490-idp/src/main/java/sep490/idp/entity/UserAuthenticator.java (1)
33-44: Equality check best practices
Overriding equals and hashCode to depend on credentialId is acceptable, but ensure that credentialId is truly unique and never changes throughout the entity’s lifecycle. Otherwise, it can lead to unexpected behavior if credentialId is updated at runtime.
sep490-idp/src/main/java/sep490/idp/service/impl/AuthenticatorServiceImpl.java
Show resolved
Hide resolved
sep490-idp/src/main/java/sep490/idp/service/impl/AuthenticatorServiceImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (23)
sep490-idp/src/main/java/sep490/idp/utils/MessageUtil.java (2)
14-17: Consider specifying a fallback message to avoid potentialNoSuchMessageException.
If a code is missing or incorrect in your message source, an exception would be raised. Providing a fallback ensures the application can gracefully handle missing message keys.- return messageSource.getMessage(code, args, LocaleContextHolder.getLocale()); + return messageSource.getMessage(code, args, "Default message", LocaleContextHolder.getLocale());
19-22: Apply the same fallback practice here.
For consistency, specify a default message to avoid exceptions on unidentified message keys.- return messageSource.getMessage(code, null, LocaleContextHolder.getLocale()); + return messageSource.getMessage(code, null, "Default message", LocaleContextHolder.getLocale());sep490-idp/src/main/resources/templates/forgot-password.html (3)
1-8: Add security headers and improve SEO metadataConsider enhancing the head section with security headers and SEO metadata.
<head> <meta charset="UTF-8"> <meta name="viewport" content="width=device-width, initial-scale=1.0"> + <meta name="_csrf" th:content="${_csrf.token}"/> + <meta name="_csrf_header" th:content="${_csrf.headerName}"/> + <meta name="description" th:content="#{forgotPassword.description}"> <title th:text="#{forgotPassword.title}">Forgot Password</title> <link rel="stylesheet" th:href="@{/css/main.css}"/> + <link rel="icon" type="image/x-icon" th:href="@{/favicon.ico}"> </head>
32-39: Improve language selection accessibilityAdd aria labels and visual indication of the current language.
<div class="text-center mt-3"> - <a th:href="@{''(lang=en)}" class="text-green-500 hover:underline" th:text="#{language.english}">English</a> | - <a th:href="@{''(lang=vi)}" class="text-green-500 hover:underline" th:text="#{language.vietnamese}">Vietnamese</a> + <a th:href="@{''(lang=en)}" + class="text-green-500 hover:underline" + th:classappend="${#locale.language == 'en'} ? 'font-bold' : ''" + aria-label="Switch to English" + th:text="#{language.english}">English</a> | + <a th:href="@{''(lang=vi)}" + class="text-green-500 hover:underline" + th:classappend="${#locale.language == 'vi'} ? 'font-bold' : ''" + aria-label="Switch to Vietnamese" + th:text="#{language.vietnamese}">Vietnamese</a> </div>
10-14: Optimize image loading and improve responsive layoutConsider the following improvements for better performance and mobile experience:
-<div class="flex items-center justify-center min-h-screen px-4"> +<div class="flex items-center justify-center min-h-screen px-4 py-8 sm:px-6 lg:px-8"> <div class="flex flex-col lg:flex-row items-center w-full max-w-5xl"> <!-- Forgot Password Form Section --> - <div class="bg-white p-6 rounded w-full lg:w-1/2"> + <div class="bg-white p-6 sm:p-8 rounded shadow-sm w-full lg:w-1/2"> <!-- Forgot Password Image Section --> <div class="hidden lg:flex lg:w-1/2 justify-center items-center"> - <img th:src="@{/img/logo.svg}" alt="City Globe" class="max-w-full h-auto"> + <img th:src="@{/img/logo.svg}" + th:alt="#{common.logo.alt}" + loading="lazy" + class="max-w-full h-auto"> </div>Also applies to: 42-47
sep490-idp/src/main/java/sep490/idp/service/impl/ForgotPasswordServiceImpl.java (3)
37-41: Consider adding success/failure logs for reset password email flow.
Providing logs upon success or failure of this operation can help with troubleshooting and monitoring.
53-55: Guard against possible null OTP code.
IfuserOTP.getOtpCode()happens to be null,userOTP.getOtpCode().equals(otpCode)can lead to a NullPointerException. Consider swapping the equality check tootpCode.equals(userOTP.getOtpCode())or adding a null check foruserOTP.getOtpCode().
73-86: Enable locale-specific templates if intended.
Currently, the template name is hardcoded as"forgot-password-otp.ftl". If the application intends to support multiple locales (e.g.,_en,_vi), consider passing the locale or constructing the template name accordingly to avoid repetitive code.sep490-idp/src/main/java/sep490/idp/utils/EmailUtil.java (2)
29-29: Use a valid “from” email address.
Here,EMAIL_HOST_FROMis set to"SEP490", which may not be recognized by some mail servers as a valid “from” address. Consider using a proper email format (e.g.,noreply@mydomain.com) to avoid potential email deliverability issues.-private static final String EMAIL_HOST_FROM = "SEP490"; +private static final String EMAIL_HOST_FROM = "noreply@mydomain.com";
40-45: Add context to thrown exception.
When themailSender.send(helper.getMimeMessage())or related steps fail, consider adding more context to the error message (like the recipient email or subject) to streamline troubleshooting.sep490-idp/src/main/java/sep490/idp/utils/IEmailUtil.java (1)
3-5: Interface naming can be more descriptive.
IEmailUtilmight be renamed to something likeEmailServiceorEmailSenderfor clarity and alignment with typical Spring naming conventions. However, this is mainly a style preference and not a functional issue.sep490-idp/src/main/java/sep490/idp/service/ForgotPasswordService.java (1)
6-10: Provide Javadoc for each method.
Methods likesendResetPasswordEmail,verifyOtp, andchangePasswordwould benefit from method-level Javadoc explaining their expected behavior, possible return values, and conditions under which they can fail.sep490-idp/src/main/java/sep490/idp/repository/UserOTPRepository.java (1)
13-13: Consider indexing the userEmail column.
Frequent lookups onuserEmailsuggest that adding a database index or unique constraint (if appropriate) to the underlying table can improve query performance.sep490-idp/src/main/java/sep490/idp/dto/ForgotResetPasswordDTO.java (1)
11-17: Recommend validating that password and confirmPassword match
While the@Patternannotation enforces a certain format, consider adding a cross-field validator or additional logic to ensure thatpasswordmatchesconfirmPassword.sep490-common/src/main/java/sep490/common/api/utils/SEPUtils.java (1)
13-22: Potential improvement: make OTP length configurable
The current method strictly generates an OTP of the requested size without bounds. Consider adding an upper limit, or make this size configurable through application properties, preventing overly large requests from triggering performance issues.sep490-idp/src/main/java/sep490/idp/entity/UserOTP.java (1)
32-36: Use a configurable expiration window instead of a hard-coded 10 minutes
Integrating a configurable property for OTP expiration length (e.g., from application.yml) would give more flexibility.sep490-idp/src/main/java/sep490/idp/configs/MailConfig.java (1)
15-25: Consider exposing more configuration options.
ThefreeMarkerConfigurermethod is properly set up and configured with UTF-8 encoding and localized lookup. If you plan to use more advanced FreeMarker features (e.g., custom settings, shared variables, loaders), you might want to further expose or parameterize these configurations.sep490-idp/src/main/java/sep490/idp/controller/ForgotPasswordController.java (2)
42-54: Check email domain and error handling paths.
TheprocessForgotPasswordmethod correctly checks if an email is eligible for reset. However, consider validating email domain or pattern if necessary. Also, review the redirect paths to ensure that all error handling scenarios (e.g., issues with the mail server) are properly surfaced and the user is informed.
90-105: Validate password complexity and match in one place.
TheresetPasswordmethod checks if the provided password meets basic rules and matches the confirmation. For best practice, unify password-complexity checks with other user signup logic, if relevant (to keep rules consistent). Also consider clearing or invalidating the session data once the process completes to prevent stale sessions.sep490-idp/src/main/resources/db/migration/V0.0.1.2__UserOTP.sql (2)
1-8: Consider indexing frequently queried columns.
Creating theuser_otptable withNOT NULLcolumns is good. If queries frequently filter byuser_idorexpired_time, consider adding corresponding indexes for improved lookup performance.
9-14: Ensure foreign key strategy aligns with user cleanup policies.
The foreign key tiesuser_otprecords tousers. Make sure that if a user record is removed or becomes inactive, you have a strategy for handling orphanuser_otprows. Options might include cascading deletions or manual cleanup strategies.sep490-idp/src/main/resources/i18n/messages.properties (1)
35-42: Add security-related messages for rate limiting and account lockoutConsider adding messages for:
- Maximum attempts reached
- Account temporary lockout
- Rate limiting notifications
Example additions:
+forgotPassword.error.maxAttempts=Too many attempts. Please try again later. +forgotPassword.error.accountLocked=Account temporarily locked. Please contact support. +forgotPassword.error.tooManyRequests=Please wait before requesting another OTP.sep490-idp/src/main/resources/templates/enter-otp.html (1)
22-24: Add remaining attempts and expiration timeDisplay remaining attempts and OTP expiration countdown to improve user experience.
<div th:if="${errorMessage}" class="bg-red-100 border border-red-400 text-red-700 px-4 py-3 rounded relative mb-4"> <span th:text="${errorMessage}"></span> </div> +<div th:if="${remainingAttempts}" class="text-gray-600 text-sm mb-4"> + <span th:text="#{enterOtp.label.remainingAttempts(${remainingAttempts})}">Attempts remaining: </span> +</div> +<div class="text-gray-600 text-sm mb-4"> + <span>Code expires in: </span> + <span id="countdown">05:00</span> +</div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
sep490-common/src/main/java/sep490/common/api/utils/SEPUtils.java(1 hunks)sep490-idp/build.gradle(3 hunks)sep490-idp/src/main/java/sep490/idp/configs/MailConfig.java(1 hunks)sep490-idp/src/main/java/sep490/idp/configs/SecurityConfig.java(2 hunks)sep490-idp/src/main/java/sep490/idp/controller/ForgotPasswordController.java(1 hunks)sep490-idp/src/main/java/sep490/idp/dto/ForgotPasswordDTO.java(1 hunks)sep490-idp/src/main/java/sep490/idp/dto/ForgotResetPasswordDTO.java(1 hunks)sep490-idp/src/main/java/sep490/idp/dto/OtpDTO.java(1 hunks)sep490-idp/src/main/java/sep490/idp/entity/UserOTP.java(1 hunks)sep490-idp/src/main/java/sep490/idp/repository/UserOTPRepository.java(1 hunks)sep490-idp/src/main/java/sep490/idp/service/ForgotPasswordService.java(1 hunks)sep490-idp/src/main/java/sep490/idp/service/impl/ForgotPasswordServiceImpl.java(1 hunks)sep490-idp/src/main/java/sep490/idp/utils/EmailUtil.java(1 hunks)sep490-idp/src/main/java/sep490/idp/utils/IEmailUtil.java(1 hunks)sep490-idp/src/main/java/sep490/idp/utils/IMessageUtil.java(1 hunks)sep490-idp/src/main/java/sep490/idp/utils/MessageUtil.java(1 hunks)sep490-idp/src/main/java/sep490/idp/utils/SEPMailMessage.java(1 hunks)sep490-idp/src/main/resources/application.yml(1 hunks)sep490-idp/src/main/resources/db/migration/V0.0.1.2__UserOTP.sql(1 hunks)sep490-idp/src/main/resources/i18n/messages.properties(1 hunks)sep490-idp/src/main/resources/i18n/messages_vi.properties(2 hunks)sep490-idp/src/main/resources/mailTemplates/forgot-password-otp_en.ftl(1 hunks)sep490-idp/src/main/resources/mailTemplates/forgot-password-otp_vi.ftl(1 hunks)sep490-idp/src/main/resources/templates/enter-otp.html(1 hunks)sep490-idp/src/main/resources/templates/forgot-password.html(1 hunks)sep490-idp/src/main/resources/templates/forgot-reset-password.html(1 hunks)sep490-idp/src/main/resources/templates/login.html(4 hunks)
✅ Files skipped from review due to trivial changes (4)
- sep490-idp/src/main/java/sep490/idp/dto/OtpDTO.java
- sep490-idp/src/main/java/sep490/idp/dto/ForgotPasswordDTO.java
- sep490-idp/src/main/resources/mailTemplates/forgot-password-otp_en.ftl
- sep490-idp/src/main/resources/mailTemplates/forgot-password-otp_vi.ftl
🚧 Files skipped from review as they are similar to previous changes (3)
- sep490-idp/src/main/resources/templates/login.html
- sep490-idp/build.gradle
- sep490-idp/src/main/java/sep490/idp/configs/SecurityConfig.java
🔇 Additional comments (14)
sep490-idp/src/main/java/sep490/idp/utils/IMessageUtil.java (4)
1-1: Interface Packaging Looks Good
The package declaration sep490.idp.utils is consistent with a utilities package structure.
3-3: Good Use of an Interface for Message Retrieval
Declaring an interface to handle message text retrieval encourages cleaner, more modular code by abstracting localization and message handling away from business logic.
4-4: Flexible Method Signature for Formatting
The method signature String getMessage(String code, Object... args) allows for flexible message formatting, which is crucial for localization and dynamic placeholders.
5-5: Additional Overload for Simpler Use Cases
Providing a simpler method without extra arguments underscores good design: it caters to messages that do not require dynamic placeholders.
sep490-idp/src/main/resources/application.yml (1)
21-28: Verify mail configuration integration
Let's verify the mail configuration is properly integrated with the application:
✅ Verification successful
Let me check the implementation of the mail-related classes to ensure proper integration.
Let me check one final piece - the usage in ForgotPasswordServiceImpl to ensure the mail configuration is being used properly.
Mail configuration is properly integrated and implemented
The verification shows a well-structured email implementation:
- Spring Mail is properly configured with SMTP settings in
application.yml EmailUtilimplements email sending with proper error handling, template support, and attachment capabilitiesSEPMailMessageextends Spring'sSimpleMailMessagewith additional features- The implementation is actively used in
ForgotPasswordServiceImplfor sending password reset emails
The only suggestion would be to consider adding email-related tests to ensure reliability of the email functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for mail-related components and configuration usage
# Check for mail dependency in build files
echo "Checking for mail dependency..."
rg -l "spring-boot-starter-mail|spring-mail" --type gradle --type maven
# Check for mail service/utility classes
echo "Checking for mail service implementations..."
fd -e java -e kt | xargs rg -l "JavaMailSender|SimpleMailMessage|MimeMessage"
# Check for mail-related environment variables in deployment configs
echo "Checking for mail configuration in deployment..."
fd -e yaml -e yml -e properties | xargs rg "SMTP_(HOST|PORT|USERNAME|PASSWORD)"
Length of output: 816
Script:
#!/bin/bash
# Check the implementation of mail-related classes
echo "Content of SEPMailMessage.java:"
cat sep490-idp/src/main/java/sep490/idp/utils/SEPMailMessage.java
echo -e "\nContent of EmailUtil.java:"
cat sep490-idp/src/main/java/sep490/idp/utils/EmailUtil.java
# Check for usage of these mail utilities
echo -e "\nChecking usage of mail utilities:"
rg "SEPMailMessage|EmailUtil" -l --type java
Length of output: 4971
Script:
#!/bin/bash
# Check the implementation of ForgotPasswordServiceImpl
echo "Content of ForgotPasswordServiceImpl.java:"
cat sep490-idp/src/main/java/sep490/idp/service/impl/ForgotPasswordServiceImpl.java
# Check for any mail-related test files
echo -e "\nChecking for mail-related test files:"
fd -e java -e kt | xargs rg -l "test.*Email|Email.*test"
Length of output: 3415
sep490-idp/src/main/java/sep490/idp/utils/MessageUtil.java (1)
8-10: Use of @Component and @RequiredArgsConstructor is appropriate and promotes cleaner construction.
No concerns regarding dependency injection. The class is well-structured for Spring's auto-detection and constructor injection.
sep490-idp/src/main/resources/templates/forgot-password.html (1)
1-49: Implementation aligns well with IDP requirements
The forgot password page implementation is well-structured and follows good practices for form handling, internationalization, and responsive design. The suggested improvements will enhance accessibility, security, and user experience, but the core functionality is solid.
sep490-idp/src/main/java/sep490/idp/utils/SEPMailMessage.java (1)
13-21: Consider thread-safety when extending SimpleMailMessage
SimpleMailMessage is typically used as a one-off instance. Please ensure that the SEPMailMessage object and its mutable fields (e.g., attachments, templateModels) are not shared across threads or reused inadvertently.
sep490-idp/src/main/java/sep490/idp/configs/MailConfig.java (1)
1-2: Package naming convention looks appropriate.
No issues identified with the package declaration; it follows typical Java conventions.
sep490-idp/src/main/java/sep490/idp/controller/ForgotPasswordController.java (3)
34-40: Validate session usage and sanitize user input.
The forgotPasswordPage method initializes session attributes and a ForgotPasswordDTO but does not check for existing session data. Ensure sensitive data stored in session is handled properly and that user input is sanitized to avoid potential injection issues.
56-64: Gate OTP page access more strictly.
Currently, the method ensures SESSION_OTP_SENT is set to true. Confirm that no edge cases allow bypassing. For example, repeated access to this endpoint might disrupt the workflow if the session is not fully cleared after a successful or canceled attempt.
79-88: Confirm OTP usage once verified.
The showResetPasswordPage method checks if SESSION_OTP_VERIFIED is set. Ensure that the verified OTP cannot be used multiple times or remain valid longer than expected, preventing replay attacks.
sep490-idp/src/main/resources/i18n/messages_vi.properties (1)
Line range hint 24-53: Localization content appears consistent and well-structured.
You have added new keys for validation and password recovery. The translations look appropriate for Vietnamese. Ensure the same keys exist in other properties files for parity across multiple languages. Also, confirm that no keys remain unused in the source code.
sep490-idp/src/main/resources/templates/forgot-reset-password.html (1)
15-39: 🛠️ Refactor suggestion
Verify CSRF protection and add password validation
Ensure CSRF protection is properly implemented and add client-side password validation.
Add CSRF token and client-side validation:
<form id="reset-password-form" th:object="${resetPasswordDTO}" role="form" th:action="@{/forgot-reset-password}" method="post">
+ <input type="hidden" th:name="${_csrf.parameterName}" th:value="${_csrf.token}" />
+ <script th:inline="javascript">
+ document.getElementById('reset-password-form').addEventListener('submit', function(e) {
+ const password = document.getElementById('password').value;
+ const confirmPassword = document.getElementById('confirmPassword').value;
+
+ if (password !== confirmPassword) {
+ e.preventDefault();
+ // Show error using existing error display mechanism
+ return;
+ }
+
+ if (!/^(?=.*[A-Za-z])(?=.*\d)[A-Za-z\d]{8,}$/.test(password)) {
+ e.preventDefault();
+ // Show error using existing error display mechanism
+ return;
+ }
+ });
+ </script>
sep490-idp/src/main/java/sep490/idp/service/impl/ForgotPasswordServiceImpl.java
Show resolved
Hide resolved
sep490-idp/src/main/java/sep490/idp/controller/ForgotPasswordController.java
Show resolved
Hide resolved
GiaBaorr
force-pushed
the
feat/idp-implement
branch
from
afa91de to
f397bb4
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
sep490-idp/src/main/resources/templates/enter-otp.html (3)
3-8: Add security headers and faviconConsider enhancing the security and user experience:
<head> <meta charset="UTF-8"> <meta name="viewport" content="width=device-width, initial-scale=1.0"> + <meta http-equiv="X-Content-Type-Options" content="nosniff"> + <meta http-equiv="X-Frame-Options" content="DENY"> + <meta http-equiv="Content-Security-Policy" content="default-src 'self'; style-src 'self' 'unsafe-inline';"> <title th:text="#{enterOtp.title}">Enter OTP</title> <link rel="stylesheet" th:href="@{/css/main.css}"/> + <link rel="icon" type="image/x-icon" th:href="@{/favicon.ico}"> </head>
18-25: Add client-side validation feedbackThe input field implementation looks good with proper security attributes. Consider enhancing the user experience with client-side validation feedback:
<input type="text" required pattern="[0-9]*" inputmode="numeric" maxlength="6" autocomplete="one-time-code" autofocus class="w-full mt-1 p-2 border border-gray-300 rounded focus:ring focus:ring-green-200 focus:outline-none" - id="otp" th:field="*{otpCode}" name="otpCode"> + id="otp" th:field="*{otpCode}" name="otpCode" + oninput="this.value = this.value.replace(/[^0-9]/g, '')" + title="Please enter a 6-digit code"> +<div class="text-sm text-red-500 mt-1 hidden" id="otp-validation-message"> + Please enter exactly 6 digits +</div>Add this script at the end of the body:
<script> document.getElementById('otp').addEventListener('input', function() { const validationMessage = document.getElementById('otp-validation-message'); validationMessage.classList.toggle('hidden', this.value.length === 6 || this.value.length === 0); }); </script>
49-49: Localize image alt textConsider using localized alt text for better accessibility:
-<img th:src="@{/img/logo.svg}" alt="City Globe" class="max-w-full h-auto"> +<img th:src="@{/img/logo.svg}" th:alt="#{logo.altText}" class="max-w-full h-auto">Add these keys to your message properties files:
# messages.properties logo.altText=City Globe Logo # messages_vi.properties logo.altText=Logo City Globesep490-idp/src/main/java/sep490/idp/controller/ForgotPasswordController.java (2)
28-30: Evaluate session-based approach for ephemeral data.
These constants and the subsequent usage of the HTTP session store ephemeral state. For enhanced security and scalability, consider a token-based approach or ephemeral storage mechanisms, especially if multiple parallel sessions or tabs might cause confusion.
42-54: Add rate-limiting measures to mitigate spamming of password reset requests.
Repeated or automated calls to the forgot password endpoint can be leveraged in malicious scenarios. Introduce rate-limiting or add a minimal cooldown to deter brute force or spam attempts.sep490-idp/src/main/resources/i18n/messages_vi.properties (1)
40-40: Avoid revealing user existence.
“Email không tồn tại” indicates that the user doesn’t exist. This can inadvertently allow malicious actors to check for valid accounts. Consider a more generic error message that doesn’t confirm or deny the existence of the email in your system.sep490-idp/src/main/resources/i18n/messages.properties (1)
40-40: Use generic phrasing to protect user privacy.
Stating “No user found with that email address” can inadvertently confirm that an account does not exist. Instead, consider a message such as “An error occurred. Please check your email or try again,” which avoids user enumeration risks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
sep490-common/src/main/java/sep490/common/api/utils/SEPUtils.java(1 hunks)sep490-idp/build.gradle(3 hunks)sep490-idp/src/main/java/sep490/idp/configs/MailConfig.java(1 hunks)sep490-idp/src/main/java/sep490/idp/configs/SecurityConfig.java(2 hunks)sep490-idp/src/main/java/sep490/idp/controller/ForgotPasswordController.java(1 hunks)sep490-idp/src/main/java/sep490/idp/dto/ForgotPasswordDTO.java(1 hunks)sep490-idp/src/main/java/sep490/idp/dto/ForgotResetPasswordDTO.java(1 hunks)sep490-idp/src/main/java/sep490/idp/dto/OtpDTO.java(1 hunks)sep490-idp/src/main/java/sep490/idp/entity/UserOTP.java(1 hunks)sep490-idp/src/main/java/sep490/idp/repository/UserOTPRepository.java(1 hunks)sep490-idp/src/main/java/sep490/idp/service/ForgotPasswordService.java(1 hunks)sep490-idp/src/main/java/sep490/idp/service/impl/ForgotPasswordServiceImpl.java(1 hunks)sep490-idp/src/main/java/sep490/idp/utils/EmailUtil.java(1 hunks)sep490-idp/src/main/java/sep490/idp/utils/IEmailUtil.java(1 hunks)sep490-idp/src/main/java/sep490/idp/utils/IMessageUtil.java(1 hunks)sep490-idp/src/main/java/sep490/idp/utils/MessageUtil.java(1 hunks)sep490-idp/src/main/java/sep490/idp/utils/SEPMailMessage.java(1 hunks)sep490-idp/src/main/resources/application.yml(1 hunks)sep490-idp/src/main/resources/db/migration/V0.0.1.2__UserOTP.sql(1 hunks)sep490-idp/src/main/resources/i18n/messages.properties(1 hunks)sep490-idp/src/main/resources/i18n/messages_vi.properties(2 hunks)sep490-idp/src/main/resources/mailTemplates/forgot-password-otp_en.ftl(1 hunks)sep490-idp/src/main/resources/mailTemplates/forgot-password-otp_vi.ftl(1 hunks)sep490-idp/src/main/resources/templates/enter-otp.html(1 hunks)sep490-idp/src/main/resources/templates/forgot-password.html(1 hunks)sep490-idp/src/main/resources/templates/forgot-reset-password.html(1 hunks)sep490-idp/src/main/resources/templates/login.html(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (23)
- sep490-idp/src/main/java/sep490/idp/utils/IEmailUtil.java
- sep490-idp/src/main/java/sep490/idp/dto/OtpDTO.java
- sep490-idp/src/main/java/sep490/idp/dto/ForgotResetPasswordDTO.java
- sep490-idp/src/main/java/sep490/idp/dto/ForgotPasswordDTO.java
- sep490-idp/src/main/java/sep490/idp/utils/IMessageUtil.java
- sep490-idp/src/main/java/sep490/idp/repository/UserOTPRepository.java
- sep490-idp/src/main/resources/mailTemplates/forgot-password-otp_en.ftl
- sep490-idp/src/main/java/sep490/idp/entity/UserOTP.java
- sep490-common/src/main/java/sep490/common/api/utils/SEPUtils.java
- sep490-idp/src/main/resources/templates/forgot-reset-password.html
- sep490-idp/src/main/java/sep490/idp/utils/MessageUtil.java
- sep490-idp/src/main/java/sep490/idp/service/ForgotPasswordService.java
- sep490-idp/src/main/java/sep490/idp/configs/SecurityConfig.java
- sep490-idp/src/main/java/sep490/idp/configs/MailConfig.java
- sep490-idp/src/main/java/sep490/idp/utils/SEPMailMessage.java
- sep490-idp/src/main/resources/mailTemplates/forgot-password-otp_vi.ftl
- sep490-idp/src/main/resources/application.yml
- sep490-idp/src/main/resources/templates/forgot-password.html
- sep490-idp/src/main/resources/templates/login.html
- sep490-idp/src/main/java/sep490/idp/service/impl/ForgotPasswordServiceImpl.java
- sep490-idp/build.gradle
- sep490-idp/src/main/java/sep490/idp/utils/EmailUtil.java
- sep490-idp/src/main/resources/db/migration/V0.0.1.2__UserOTP.sql
🔇 Additional comments (2)
sep490-idp/src/main/java/sep490/idp/controller/ForgotPasswordController.java (2)
35-40: Ensure session attributes are cleared upon flow completion.
Once users finalize or abandon the forgot-password flow, consider resetting or removing these session attributes to avoid stale state issues and potential confusion when re-attempting the flow.
66-77: Log unsuccessful OTP attempts.
This comment repeats the past suggestion for security auditing. Logging invalid OTP submissions and potentially implementing lockouts help mitigate further compromise.
sep490-idp/src/main/java/sep490/idp/controller/ForgotPasswordController.java
Show resolved
Hide resolved
GiaBaorr
requested review from
huynhlephcvinh and
thongdanghoang
and removed request for
thongdanghoang
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores