MOSU-40 feat: 회원가입 로그인 기능 구현 #50
Conversation
# Conflicts: # src/main/java/life/mosu/mosuserver/global/config/SecurityConfig.java # src/main/java/life/mosu/mosuserver/presentation/oauth/AccessTokenFilter.java
There was a problem hiding this comment.
Pull Request Overview
This PR implements the user registration and authentication workflows, enhances security configuration, and centralizes exception handling.
- Introduces sign-up, login, and token reissue endpoints with DTOs and validation
- Adds
@UserIdannotation and resolver for injecting authenticated user IDs into controllers - Refactors security filter chain, exception handlers, and refresh‐token persistence
Reviewed Changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ProfileController.java | Switched to @UserId injection and added role checks |
| AccessTokenFilter.java | Bypasses filtering for token reissue route |
| SignUpController.java | New sign-up endpoint |
| AuthController.java | New login and token reissue endpoints |
| GlobalExceptionHandler.java | Standardized auth & access-denied responses |
| SecurityConfig.java | Method-level security and filter chain adjustments |
| RefreshTokenKeyValueRepository.java | Cleaned up refresh-token repository interface |
| EncodeUtil.java | Utility for password encoding |
| DanalVerificationService.java | Stub verification service (TODO implementation) |
Comments suppressed due to low confidence (3)
src/main/java/life/mosu/mosuserver/global/exception/GlobalExceptionHandler.java:79
- [nitpick] The
errorsfield is now a String instead of a Map, which may break clients expecting a map of validation errors. Consider renaming this field toerroror wrapping the message in a map for consistency.
response.put("errors", ex.getMessage());
src/main/java/life/mosu/mosuserver/presentation/auth/SignUpController.java:32
- Returning HTTP 200 for a creation operation is inconsistent—consider using
HttpStatus.CREATED(201) to signal a new resource was created.
return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "성공"));
src/main/java/life/mosu/mosuserver/global/util/EncodeUtil.java:6
- [nitpick] This utility simply delegates to
encoder.encode. You might callencoder.encodedirectly and remove this helper to reduce indirection.
public static String passwordEncode(final PasswordEncoder encoder, final String password) {
| final HttpServletResponse response, | ||
| final FilterChain filterChain | ||
| ) throws ServletException, IOException { | ||
| if (request.getRequestURI().equals("/api/v1/auth/reissue")) { |
There was a problem hiding this comment.
The bypass condition uses the wrong path '/api/v1/auth/reissue', but your controller mapping is '/auth/reissue'. Update this check to match the actual endpoint or use a constant to avoid filtering valid reissue requests.
| if (request.getRequestURI().equals("/api/v1/auth/reissue")) { | |
| if (request.getRequestURI().equals(REISSUE_ENDPOINT)) { |
| .logout(AbstractHttpConfigurer::disable) | ||
| .authorizeHttpRequests(authorize -> authorize | ||
| .requestMatchers( | ||
| new AntPathRequestMatcher("/**") |
There was a problem hiding this comment.
The rule permitAll on '/**' effectively opens all endpoints and negates subsequent authorization rules. Remove or narrow this matcher so protected routes remain secured.
| new AntPathRequestMatcher("/**") | |
| "/public/**", // Example public endpoints | |
| "/login/**", | |
| "/register/**" |
|
|
||
| public interface RefreshTokenKeyValueRepository extends KeyValueRepository<RefreshTokenRedisEntity, Long> { | ||
|
|
||
| boolean existsByRefreshToken(String refreshToken); |
There was a problem hiding this comment.
You removed deleteByUserId and existsByUserId from this interface, but RefreshTokenService still calls them. Either restore those methods or update the service to use deleteById and existsById.
| boolean existsByRefreshToken(String refreshToken); | |
| boolean existsByRefreshToken(String refreshToken); | |
| void deleteByUserId(Long userId); | |
| boolean existsByUserId(Long userId); |
|
|
||
| private final ProfileService profileService; | ||
|
|
||
| @PostMapping |
There was a problem hiding this comment.
The profile creation endpoint is not secured with @PreAuthorize, so anyone could call it. If only authenticated users should register profiles, add a security annotation similar to the update method.
| @PostMapping | |
| @PostMapping | |
| @PreAuthorize("isAuthenticated() and hasRole('USER')") |
| public class DanalVerificationService implements VerificationService { | ||
| //TODO: Danal 인증 서비스 구현 | ||
| @Override | ||
| public boolean verify(final String verificationCode, final String phoneNumber) { |
There was a problem hiding this comment.
This service currently returns true for all verifications. Add a clear // FIXME and implement real verification logic before shipping to avoid security gaps.
| import static life.mosu.mosuserver.global.util.EncodeUtil.passwordEncode; | ||
|
|
||
| @Service | ||
| @Transactional(readOnly = true, propagation = Propagation.SUPPORTS) |
| UserJpaEntity user = UserJpaEntity.builder() | ||
| .loginId(request.id()) | ||
| .password(passwordEncode(passwordEncoder, request.password())) | ||
| .userRole(UserRole.ROLE_PENDING) | ||
| .build(); |
There was a problem hiding this comment.
builder 감춰서 정팩메 쓰는게 좋을거 같아요~
| throw new CustomRuntimeException(ErrorCode.PRINCIPAL_NOT_FOUND); | ||
| } | ||
|
|
||
| PrincipalDetails principal = (PrincipalDetails) Objects.requireNonNull(authentication) |
There was a problem hiding this comment.
Objects.requireNonNull 위에 null 체크하는거 같던데 혹시 꼭 필요했을까용?
There was a problem hiding this comment.
null 확인 제거 하겠습니다!
| final Token token = authService.login(request); | ||
| final String authorization = token.grantType() + " " + token.accessToken(); | ||
| return ResponseEntity.status(HttpStatus.CREATED) | ||
| .header(HttpHeaders.AUTHORIZATION, authorization) | ||
| .body(ApiResponseWrapper.success(HttpStatus.CREATED, token)); |
There was a problem hiding this comment.
| final Token token = authService.login(request); | |
| final String authorization = token.grantType() + " " + token.accessToken(); | |
| return ResponseEntity.status(HttpStatus.CREATED) | |
| .header(HttpHeaders.AUTHORIZATION, authorization) | |
| .body(ApiResponseWrapper.success(HttpStatus.CREATED, token)); | |
| // 예시: HttpOnly 쿠키에 토큰 저장 | |
| ResponseCookie cookie = ResponseCookie.from("accessToken", token.accessToken()) | |
| .httpOnly(true) | |
| .secure(true) // HTTPS 환경에서만 전송 | |
| .path("/") | |
| .maxAge(token.expiresIn()) // 토큰 만료 시간 | |
| .build(); | |
| return ResponseEntity.status(HttpStatus.CREATED) | |
| .header(HttpHeaders.SET_COOKIE, cookie.toString()) | |
| .body(ApiResponseWrapper.success(HttpStatus.CREATED, token)); |
이거는 어떨까용?
| public ResponseEntity<ApiResponseWrapper<?>> signUp( | ||
| @RequestBody @Valid final SignUpRequest request | ||
| ) { | ||
| signUpService.signUp(request); | ||
| return ResponseEntity.ok(ApiResponseWrapper.success(HttpStatus.OK, "성공")); | ||
| } |
There was a problem hiding this comment.
이 친구는 ? 로 타입 추론한 이유가 있을까요?
| final HttpServletResponse response, | ||
| final FilterChain filterChain | ||
| ) throws ServletException, IOException { | ||
| if (request.getRequestURI().equals("/api/v1/auth/reissue")) { |
| } | ||
|
|
||
| @GetMapping | ||
| public ApiResponseWrapper<ProfileDetailResponse> getProfile( |
There was a problem hiding this comment.
여기에 ResponseEntity 가 안씌워져있어요
|
LGTM |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
|
|
||
| @Override | ||
| public UserDetails loadUserByUsername(final String username) throws UsernameNotFoundException { | ||
| return new PrincipalDetails(userRepository.findByLoginId(username).orElseThrow(() -> new CustomRuntimeException(ErrorCode.USER_NOT_FOUND))); |
There was a problem hiding this comment.
The loadUserByUsername method is declared to throw UsernameNotFoundException, but it's currently throwing a CustomRuntimeException. Throw UsernameNotFoundException as required by the interface to ensure compatibility with Spring Security.
| return new PrincipalDetails(userRepository.findByLoginId(username).orElseThrow(() -> new CustomRuntimeException(ErrorCode.USER_NOT_FOUND))); | |
| return new PrincipalDetails(userRepository.findByLoginId(username).orElseThrow(() -> new UsernameNotFoundException(ErrorCode.USER_NOT_FOUND.getMessage()))); |
| @GetMapping | ||
| public ApiResponseWrapper<ProfileDetailResponse> getProfile( | ||
| @RequestParam Long userId) { | ||
| @RequestParam Long userId) { | ||
| ProfileDetailResponse response = profileService.getProfile(userId); | ||
| return ApiResponseWrapper.success(HttpStatus.OK, "프로필 조회 성공", response); | ||
| } |
There was a problem hiding this comment.
The getProfile endpoint is currently public and allows fetching any user's profile by their userId. This could be a significant privacy and security risk. I strongly recommend securing this endpoint.
| @GetMapping | |
| public ApiResponseWrapper<ProfileDetailResponse> getProfile( | |
| @RequestParam Long userId) { | |
| @RequestParam Long userId) { | |
| ProfileDetailResponse response = profileService.getProfile(userId); | |
| return ApiResponseWrapper.success(HttpStatus.OK, "프로필 조회 성공", response); | |
| } | |
| @GetMapping | |
| @PreAuthorize("isAuthenticated()") | |
| public ApiResponseWrapper<ProfileDetailResponse> getProfile( | |
| @UserId Long userId) { | |
| ProfileDetailResponse response = profileService.getProfile(userId); | |
| return ApiResponseWrapper.success(HttpStatus.OK, "프로필 조회 성공", response); | |
| } |
| } catch (final Exception e) { | ||
| throw new CustomRuntimeException(ErrorCode.INCORRECT_ID_OR_PASSWORD); | ||
| } |
There was a problem hiding this comment.
Catching the generic Exception is too broad and can hide potential runtime errors. It's better to catch the more specific AuthenticationException that Spring Security throws for login failures.
| } catch (final Exception e) { | |
| throw new CustomRuntimeException(ErrorCode.INCORRECT_ID_OR_PASSWORD); | |
| } | |
| } catch (final org.springframework.security.core.AuthenticationException e) { | |
| throw new CustomRuntimeException(ErrorCode.INCORRECT_ID_OR_PASSWORD); | |
| } |
| @RequiredArgsConstructor | ||
| public class UserIdArgumentResolver implements HandlerMethodArgumentResolver { | ||
|
|
||
| private final AccessTokenService accessTokenService; |
There was a problem hiding this comment.
The AccessTokenService dependency is injected but is not used within this class. Please remove it to keep the code clean.
|
|
||
| public record LoginRequest( | ||
| @NotNull String id, | ||
| @PasswordPattern @NotNull String password |
There was a problem hiding this comment.
Applying @PasswordPattern validation on the password field for a login request is generally not recommended. Password format policies should be enforced during sign-up or password updates, not during login.
| @PasswordPattern @NotNull String password | |
| @NotNull String password |
| public UserJpaEntity toEntity(PasswordEncoder passwordEncoder) { | ||
| return UserJpaEntity.builder() | ||
| .loginId(id) | ||
| .password(passwordEncode(passwordEncoder, password)) | ||
| .userRole(UserRole.ROLE_PENDING) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
The EncodeUtil class appears to be an unnecessary layer of abstraction over PasswordEncoder. I recommend removing EncodeUtil and calling passwordEncoder.encode(password) directly within the toEntity method for better clarity and maintainability.
| public UserJpaEntity toEntity(PasswordEncoder passwordEncoder) { | |
| return UserJpaEntity.builder() | |
| .loginId(id) | |
| .password(passwordEncode(passwordEncoder, password)) | |
| .userRole(UserRole.ROLE_PENDING) | |
| .build(); | |
| } | |
| public UserJpaEntity toEntity(PasswordEncoder passwordEncoder) { | |
| return UserJpaEntity.builder() | |
| .loginId(id) | |
| .password(passwordEncoder.encode(password)) | |
| .userRole(UserRole.ROLE_PENDING) | |
| .build(); | |
| } |
WalkthroughThis update introduces a comprehensive authentication and user management system, including new service and controller classes for login, sign-up, and token management. It adds custom validation annotations, error codes, and argument resolvers. The security configuration is significantly enhanced, with new filters, handlers, and endpoint rules. Several files are reorganized or reformatted, and OAuth2 login flows are streamlined. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthController
participant AuthService
participant PrincipalDetailsService
participant AuthTokenManager
participant UserJpaRepository
Client->>AuthController: POST /auth/login (LoginRequest)
AuthController->>AuthService: login(LoginRequest)
AuthService->>PrincipalDetailsService: loadUserByUsername(id)
PrincipalDetailsService->>UserJpaRepository: findByLoginId(id)
UserJpaRepository-->>PrincipalDetailsService: UserJpaEntity
PrincipalDetailsService-->>AuthService: PrincipalDetails
AuthService->>AuthTokenManager: generateToken(PrincipalDetails)
AuthTokenManager-->>AuthService: Token
AuthService-->>AuthController: Token
AuthController-->>Client: Response (Token, cookie set)
sequenceDiagram
participant Client
participant AuthController
participant AuthService
participant AuthTokenManager
Client->>AuthController: POST /auth/reissue
AuthController->>AuthService: reissueAccessToken(request)
AuthService->>AuthTokenManager: reissueAccessToken(request)
AuthTokenManager-->>AuthService: Token
AuthService-->>AuthController: Token
AuthController-->>Client: Response (Token, header set)
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (1)
src/main/java/life/mosu/mosuserver/domain/inquiry/InquiryFile.java (1)
14-20: Consider composition over inheritance for value objects
InquiryFileextendsFile, which is likely an entity/VO itself. Deep inheritance hierarchies in JPA embeddables frequently complicate equality, serialization, and migrations. A lighter approach is to keepFileas a value object and embed it:@Embeddable @Getter @NoArgsConstructor(access = AccessLevel.PROTECTED) public class InquiryFile { @Embedded private File file; // composition @Builder public InquiryFile(String fileName, String s3Key, Visibility visibility) { this.file = new File(fileName, s3Key, visibility); } }This removes inheritance constraints and better models “has-a” rather than “is-a”.
♻️ Duplicate comments (9)
src/main/java/life/mosu/mosuserver/application/auth/DanalVerificationService.java (1)
7-11: Critical security issue: Stub implementation bypasses verification.The current implementation always returns
true, which completely bypasses phone verification and creates a major security vulnerability. This allows unauthorized access regardless of the verification code validity.Add proper implementation or at minimum add a clear warning:
@Override public boolean verify(final String verificationCode, final String phoneNumber) { + // FIXME: SECURITY RISK - This always returns true! + // Do not deploy to production without implementing actual Danal verification + throw new UnsupportedOperationException("Danal verification not yet implemented"); - return true; }This ensures the service fails securely until proper implementation is added.
src/main/java/life/mosu/mosuserver/presentation/auth/dto/LoginRequest.java (1)
8-8: Remove password pattern validation from login request.Password format validation should not be enforced during login, as it can expose password requirements to attackers and prevent legitimate users with legacy passwords from logging in.
- @PasswordPattern @NotNull String password + @NotNull String passwordsrc/main/java/life/mosu/mosuserver/application/auth/PrincipalDetailsService.java (1)
21-21: Throw UsernameNotFoundException to maintain Spring Security compatibility.The
loadUserByUsernamemethod should throwUsernameNotFoundExceptionas required by theUserDetailsServiceinterface contract. ThrowingCustomRuntimeExceptioninstead may interfere with Spring Security's authentication flow and exception handling.- return new PrincipalDetails(userRepository.findByLoginId(username).orElseThrow(() -> new CustomRuntimeException(ErrorCode.USER_NOT_FOUND))); + return new PrincipalDetails(userRepository.findByLoginId(username).orElseThrow(() -> new UsernameNotFoundException(ErrorCode.USER_NOT_FOUND.getMessage())));src/main/java/life/mosu/mosuserver/application/auth/AuthService.java (1)
34-36: Address the generic Exception handling issue from previous review.The previous review comment about catching generic
Exceptionbeing too broad was not addressed. This can hide important runtime errors and security issues.Apply the suggested fix from the previous review:
- } catch (final Exception e) { + } catch (final org.springframework.security.core.AuthenticationException e) { throw new CustomRuntimeException(ErrorCode.INCORRECT_ID_OR_PASSWORD); }src/main/java/life/mosu/mosuserver/global/resolver/UserIdArgumentResolver.java (1)
22-22: Remove unused AccessTokenService dependency.The previous review comment about the unused
AccessTokenServicedependency was not addressed. This dependency is injected but never used in the class.Remove the unused dependency:
- private final AccessTokenService accessTokenService;src/main/java/life/mosu/mosuserver/presentation/auth/dto/SignUpRequest.java (1)
16-22: Address the EncodeUtil abstraction issue from previous review.The previous review comment about
EncodeUtilbeing an unnecessary abstraction layer was not addressed. The direct call topasswordEncoder.encode(password)is more clear and maintainable.Apply the suggested fix from the previous review:
public UserJpaEntity toEntity(PasswordEncoder passwordEncoder) { return UserJpaEntity.builder() .loginId(id) - .password(passwordEncode(passwordEncoder, password)) + .password(passwordEncoder.encode(password)) .userRole(UserRole.ROLE_PENDING) .build(); }Also remove the static import:
-import static life.mosu.mosuserver.global.util.EncodeUtil.passwordEncode;src/main/java/life/mosu/mosuserver/presentation/profile/ProfileController.java (2)
30-37: Security issue: Profile creation endpoint is not securedThe profile creation endpoint lacks authentication requirements, which means anyone can create profiles for any user. This is a security vulnerability.
Add authentication requirement:
@PostMapping +@PreAuthorize("isAuthenticated()") public ApiResponseWrapper<Void> create( @UserId Long userId, @Valid @RequestBody ProfileRequest request ) {
50-55: Critical security vulnerability: Profile data exposureThe getProfile endpoint allows fetching any user's profile by specifying their userId in the request parameter. This is a significant privacy and security risk that was previously flagged.
Fix the security issue by using the authenticated user's ID:
@GetMapping +@PreAuthorize("isAuthenticated()") public ApiResponseWrapper<ProfileDetailResponse> getProfile( - @RequestParam Long userId) { + @UserId Long userId) { ProfileDetailResponse response = profileService.getProfile(userId); return ApiResponseWrapper.success(HttpStatus.OK, "프로필 조회 성공", response); }src/main/java/life/mosu/mosuserver/global/config/SecurityConfig.java (1)
78-88: Critical Security Issue: Overly permissive authorization rulesThe current authorization configuration allows unrestricted access to all endpoints except those explicitly requiring ADMIN role. This creates a significant security vulnerability.
The
anyRequest().permitAll()effectively opens all endpoints and negates the purpose of authentication. Consider implementing proper authorization rules:.authorizeHttpRequests(authorize -> authorize .requestMatchers( "/", "/auth/success" ).permitAll() .requestMatchers( "/api/v1/profile/**", "/api/v1/admin/**" ) .hasRole("ADMIN") - .anyRequest().permitAll() + .requestMatchers( + "/api/v1/auth/**", + "/api/v1/oauth2/**" + ).permitAll() + .anyRequest().authenticated() )
🧹 Nitpick comments (10)
src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/InquiryAnswerFile.java (1)
11-14:@Getteris superfluous hereThe subclass declares no additional fields; all getters are already generated (or defined) in the parent
Fileclass. You can safely drop Lombok’s@Getterto reduce noise.-@Getter @Embeddable @NoArgsConstructor(access = AccessLevel.PROTECTED) public class InquiryAnswerFile extends File {src/main/java/life/mosu/mosuserver/global/annotation/PasswordPattern.java (2)
11-11: Verify the regex pattern complexity and consider documentation.The regex pattern correctly enforces the password requirements (8-20 characters with at least one lowercase, uppercase, digit, and special character from @$!%*?&). However, consider adding a comment explaining the pattern for maintainability.
Consider adding a comment above the regex to explain its components:
+// Password must be 8-20 characters with at least one: lowercase, uppercase, digit, and special char (@$!%*?&) @Pattern(regexp = "^(?=.*[a-z])(?=.*[A-Z])(?=.*\\d)(?=.*[@$!%*?&])[A-Za-z\\d@$!%*?&]{8,20}$", message = "비밀번호 형식이 올바르지 않습니다.")
15-19: Consider message consistency.The annotation structure follows Bean Validation standards correctly. However, there are two different Korean messages - one in the @pattern annotation (line 11) and one in the default message() method (line 16). The @pattern message will take precedence, making the default message potentially unused.
Consider using consistent messaging or removing the redundant message:
- String message() default "비밀번호는 8~20자의 영문 대/소문자, 숫자, 특수문자를 모두 포함해야 합니다."; + String message() default "비밀번호 형식이 올바르지 않습니다.";src/main/java/life/mosu/mosuserver/domain/event/EventImage.java (1)
14-19:@Embeddablesubclass should overrideequals/hashCodeexplicitlyJPA embeddables are compared by value. Relying on superclass (
File) implementations can be brittle if that class is later changed or reused differently. Adding explicit overrides inEventImageclarifies intent and protects against subtle persistence bugs.@Override public boolean equals(Object o) { return super.equals(o); } @Override public int hashCode() { return super.hashCode(); }(Or generate them via Lombok’s
@EqualsAndHashCode(callSuper = true).)Minor change, but it reinforces correctness.
src/main/java/life/mosu/mosuserver/domain/faq/FaqFile.java (1)
4-5: Domain object depends on infra layer – consider untangling
FaqFileextendslife.mosu.mosuserver.infra.storage.domain.Fileand importsVisibilityfrom the same infra package.
This creates a reverse dependency (domain → infra) that violates typical clean-architecture / DDD layering, and will make future modularisation harder.Suggestion:
- Move
FileandVisibilityto a shareddomain.storage(orcommon) module, or- Introduce an interface in the domain layer and keep the concrete implementation inside
infra.src/main/resources/security-config.yml (1)
37-41: Fix the missing newline at end of file.The new configuration sections look appropriate for the authentication features, but there's a missing newline at the end of the file.
target: - url: ${TARGET_URL} + url: ${TARGET_URL} +src/main/java/life/mosu/mosuserver/global/util/EncodeUtil.java (1)
5-9: Consider if this utility class is necessary.The
EncodeUtilclass is a simple wrapper aroundPasswordEncoder.encode(). Unless there's a specific reason for this abstraction (like future enhancements to password encoding logic), consider usingPasswordEncoderdirectly in the calling code for better clarity and fewer abstractions.If you decide to keep this utility, consider making the class
finaland adding a private constructor to prevent instantiation:-public class EncodeUtil { +public final class EncodeUtil { + private EncodeUtil() { + // Utility class - prevent instantiation + } + public static String passwordEncode(final PasswordEncoder encoder, final String password) { return encoder.encode(password); } }src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginFailureHandler.java (1)
23-24: Fix log message formatting.The log messages are missing spaces between the prefix and the actual values, making them hard to read.
- log.info("OAuth2 Error" + error.getErrorCode()); - log.info("OAuth2 Description" + error.getDescription()); + log.info("OAuth2 Error: {}", error.getErrorCode()); + log.info("OAuth2 Description: {}", error.getDescription());src/main/java/life/mosu/mosuserver/presentation/auth/SignUpController.java (1)
28-36: Consider adding rate limiting for sign-up endpointWhile the current implementation is solid, consider adding rate limiting to prevent abuse of the sign-up endpoint, especially if this is a public-facing API.
You could add rate limiting using annotations like:
@PostMapping +@RateLimited(max = 5, window = "1m") // Example: 5 requests per minute public ResponseEntity<ApiResponseWrapper<Void>> signUp( @RequestBody @Valid final SignUpRequest request ) {src/main/java/life/mosu/mosuserver/presentation/auth/AuthController.java (1)
39-44: Consider environment-specific cookie security settingsThe cookie is set with
secure(true), which requires HTTPS. This might cause issues in development environments using HTTP.Consider making the secure flag configurable:
final ResponseCookie cookie = ResponseCookie.from("accessToken", token.accessToken()) .httpOnly(true) - .secure(true) + .secure(isSecureEnvironment()) // Inject from configuration .path("/") .maxAge(authTokenManager.getAccessTokenExpireTime()) .build();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
src/main/java/life/mosu/mosuserver/application/auth/AuthService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/auth/AuthTokenManager.java(2 hunks)src/main/java/life/mosu/mosuserver/application/auth/DanalVerificationService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/auth/PrincipalDetails.java(1 hunks)src/main/java/life/mosu/mosuserver/application/auth/PrincipalDetailsService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/auth/RefreshTokenService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/auth/SignUpService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/auth/VerificationService.java(1 hunks)src/main/java/life/mosu/mosuserver/application/oauth/OAuthUserService.java(3 hunks)src/main/java/life/mosu/mosuserver/application/profile/ProfileService.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/auth/security/RefreshTokenKeyValueRepository.java(0 hunks)src/main/java/life/mosu/mosuserver/domain/auth/security/RefreshTokenRedisEntity.java(3 hunks)src/main/java/life/mosu/mosuserver/domain/auth/security/RefreshTokenRedisRepository.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/event/EventImage.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/faq/FaqFile.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/inquiry/InquiryFile.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/InquiryAnswerFile.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/notice/NoticeFile.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/user/UserRole.java(1 hunks)src/main/java/life/mosu/mosuserver/global/annotation/PasswordPattern.java(1 hunks)src/main/java/life/mosu/mosuserver/global/annotation/UserId.java(1 hunks)src/main/java/life/mosu/mosuserver/global/config/SecurityConfig.java(3 hunks)src/main/java/life/mosu/mosuserver/global/config/WebMvcConfig.java(1 hunks)src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java(2 hunks)src/main/java/life/mosu/mosuserver/global/exception/GlobalExceptionHandler.java(6 hunks)src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginFailureHandler.java(1 hunks)src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java(1 hunks)src/main/java/life/mosu/mosuserver/global/resolver/UserIdArgumentResolver.java(1 hunks)src/main/java/life/mosu/mosuserver/global/util/EncodeUtil.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/AuthController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/SignUpController.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/dto/LoginRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/auth/dto/SignUpRequest.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/oauth/AccessTokenFilter.java(1 hunks)src/main/java/life/mosu/mosuserver/presentation/oauth/AuthorizationRequestRedirectResolver.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/oauth/OAuthController.java(0 hunks)src/main/java/life/mosu/mosuserver/presentation/profile/ProfileController.java(1 hunks)src/main/resources/security-config.yml(1 hunks)
💤 Files with no reviewable changes (3)
- src/main/java/life/mosu/mosuserver/domain/auth/security/RefreshTokenKeyValueRepository.java
- src/main/java/life/mosu/mosuserver/presentation/oauth/OAuthController.java
- src/main/java/life/mosu/mosuserver/presentation/oauth/AuthorizationRequestRedirectResolver.java
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginFailureHandler.java (1)
src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java (1)
Slf4j(16-44)
src/main/java/life/mosu/mosuserver/presentation/auth/dto/SignUpRequest.java (1)
src/main/java/life/mosu/mosuserver/global/util/EncodeUtil.java (1)
EncodeUtil(5-9)
🪛 YAMLlint (1.37.1)
src/main/resources/security-config.yml
[error] 41-41: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (34)
src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/InquiryAnswerFile.java (1)
1-1: Confirm JPA component-scan covers the new packageMoving the class from
infra.storage.domaintodomain.inquiryAnswerchanges its fully-qualified name. Double-check that your@EntityScan/packagesToScan(or equivalent Spring Boot defaults) include the new package, otherwise the embeddable may not be detected at runtime.src/main/java/life/mosu/mosuserver/application/auth/VerificationService.java (1)
3-5: LGTM! Clean interface design.The interface follows good design principles with a clear single responsibility and appropriate method signature for verification operations.
src/main/java/life/mosu/mosuserver/global/annotation/PasswordPattern.java (2)
1-10: LGTM! Clean imports and package structure.The package declaration and imports are well-organized and include all necessary dependencies for a custom Bean Validation annotation.
12-14: LGTM! Proper annotation meta-configuration.The target elements (FIELD, PARAMETER), runtime retention, and constraint configuration are correctly set up for a validation annotation.
src/main/java/life/mosu/mosuserver/domain/notice/NoticeFile.java (1)
1-1: No remaining imports ofinfra.storage.domain.NoticeFile
Ranrg --line-number 'infra\.storage\.domain\.NoticeFile'across the codebase and found zero matches. All references have been updated.src/main/java/life/mosu/mosuserver/domain/faq/FaqFile.java (1)
1-1: No leftoverinfra.storage.domain.FaqFilereferences found
I searched the entire codebase for both import statements and fully-qualified usages of the old package and found no occurrences—your package relocation is complete.src/main/java/life/mosu/mosuserver/global/annotation/UserId.java (1)
8-12: UserId annotation and resolver are correctly implementedThe custom
@UserIdannotation is properly configured, and the corresponding argument resolver is present and registered:
UserIdArgumentResolver(src/main/java/life/mosu/mosuserver/global/resolver/UserIdArgumentResolver.java) implementsHandlerMethodArgumentResolver.WebMvcConfig(src/main/java/life/mosu/mosuserver/global/config/WebMvcConfig.java) injectsUserIdArgumentResolverand adds it inaddArgumentResolvers.No further action required.
src/main/java/life/mosu/mosuserver/domain/auth/security/RefreshTokenRedisRepository.java (1)
24-26: Simplification of repository method delegation.The change from
existsByUserIdtoexistsByIdis appropriate sinceuserIdis the@Idfield in the Redis entity. This simplifies the repository interface by leveraging Spring Data's built-in methods.src/main/java/life/mosu/mosuserver/domain/auth/security/RefreshTokenRedisEntity.java (2)
15-16: Excellent use of Lombok constructor annotations.The constructor annotations follow best practices:
@AllArgsConstructor(access = PRIVATE)- provides a private constructor for the factory method@NoArgsConstructor(access = PROTECTED)- provides a protected constructor for Spring Data RedisThis approach maintains encapsulation while enabling framework functionality.
28-30: Clean refactoring of the factory method.The change from manual field setting to using the all-args constructor is cleaner and leverages the Lombok-generated constructor effectively. This reduces code duplication and maintains consistency.
src/main/java/life/mosu/mosuserver/application/profile/ProfileService.java (2)
35-35: Minor formatting improvement.The lambda expression formatting is cleaner and more consistent.
43-44: Consistent formatting with the rest of the file.The multi-line lambda formatting matches the style used elsewhere in the service and improves readability.
src/main/java/life/mosu/mosuserver/application/auth/PrincipalDetails.java (1)
32-34: LGTM! Clean implementation for user ID extraction.The new
getId()method correctly exposes the user ID from the encapsulatedUserJpaEntity, supporting the authentication flow andUserIdArgumentResolverintegration.src/main/java/life/mosu/mosuserver/application/auth/RefreshTokenService.java (1)
39-39: LGTM! Improved error specificity.The error code change from
INVALID_TOKENtoNOT_EXIST_REFRESH_TOKENprovides better semantic clarity when a refresh token doesn't exist for the given user ID.src/main/java/life/mosu/mosuserver/application/auth/AuthTokenManager.java (1)
30-32: LGTM! Good explicit error handling.The null check for refreshTokenString with explicit exception throwing improves the robustness of the token reissue flow.
src/main/java/life/mosu/mosuserver/presentation/oauth/AccessTokenFilter.java (2)
26-27: Good use of configuration for endpoint management.Using
@Valueannotation to inject the reissue endpoint from configuration is a good practice that addresses the previous feedback about using constants.
35-38: LGTM! Proper bypass logic for reissue endpoint.The bypass logic correctly skips token validation for the reissue endpoint, which is necessary since the reissue operation needs to handle token validation separately using refresh tokens.
src/main/java/life/mosu/mosuserver/global/config/WebMvcConfig.java (1)
11-20: LGTM! Proper Spring MVC configuration.The configuration class correctly implements
WebMvcConfigurerand properly registers theUserIdArgumentResolver. This follows Spring MVC conventions and enables the custom argument resolution functionality.src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginFailureHandler.java (1)
21-25: Consider security implications of logging OAuth2 error details.While logging OAuth2 errors can be helpful for debugging, ensure that the error descriptions don't contain sensitive information that could be exploited by attackers. Consider using different log levels (DEBUG vs INFO) based on the sensitivity of the information.
src/main/java/life/mosu/mosuserver/application/auth/SignUpService.java (1)
20-30: LGTM! Clean and secure sign-up implementation.The service properly handles:
- Transaction management with
@Transactional- Duplicate user detection with appropriate exception throwing
- Password encoding through the DTO's
toEntitymethod- Clean separation of concerns
src/main/java/life/mosu/mosuserver/global/resolver/UserIdArgumentResolver.java (2)
24-28: LGTM! Proper parameter validation.The
supportsParametermethod correctly validates both the annotation presence and parameter type.
34-43: LGTM! Secure authentication context handling.The argument resolution properly:
- Checks for null authentication
- Validates the principal type
- Throws appropriate exceptions for invalid states
- Extracts the user ID safely
src/main/java/life/mosu/mosuserver/presentation/auth/dto/SignUpRequest.java (1)
11-14: LGTM! Proper validation annotations.The DTO correctly uses:
@NotNullfor required fields@PasswordPatternfor password complexity validationsrc/main/java/life/mosu/mosuserver/presentation/auth/SignUpController.java (1)
1-37: Well-implemented sign-up endpoint with proper validation and response handlingThe controller follows good REST practices with:
- Appropriate HTTP status (201 CREATED)
- Proper validation with
@Valid- Consistent response format using
ApiResponseWrapper- Clear method documentation
src/main/java/life/mosu/mosuserver/presentation/auth/AuthController.java (1)
34-49: Good implementation of secure cookie handlingThe login endpoint properly implements HTTP-only, secure cookies as suggested in the previous review. The token response structure is also well-designed.
src/main/java/life/mosu/mosuserver/global/exception/ErrorCode.java (1)
11-30: Well-designed error codes for authentication systemThe new error codes are properly structured with:
- Appropriate HTTP status codes (401 for auth issues, 409 for conflicts, 404 for not found)
- Clear, descriptive error messages
- Consistent naming conventions
- Logical grouping by functional area
These additions provide comprehensive error handling for the authentication system.
src/main/java/life/mosu/mosuserver/global/config/SecurityConfig.java (4)
37-43: LGTM: Well-structured CORS client configurationThe static list of allowed clients provides good control over cross-origin requests and includes appropriate development and production URLs.
56-66: LGTM: Proper exclusion of static resources from securityThe
WebSecurityCustomizercorrectly excludes static resources and documentation endpoints from security filtering, which is appropriate for these types of resources.
90-97: LGTM: Comprehensive OAuth2 login configurationThe OAuth2 configuration properly includes both success and failure handlers, custom user service, and updated redirection endpoint. The endpoint change from
/login/oauth2/code/to/api/v1/oauth2/callback/provides better API structure.
98-101: LGTM: Proper filter chain and exception handling setupThe filter ordering and exception handling configuration are correctly implemented. Adding filters before
UsernamePasswordAuthenticationFilterensures proper token processing flow.src/main/java/life/mosu/mosuserver/global/exception/GlobalExceptionHandler.java (4)
8-9: LGTM: Correct Spring Security exception importsThe imports have been properly updated to use Spring Security's
AuthenticationExceptionandAccessDeniedExceptioninstead of potentially incorrect imports.
78-79: LGTM: Improved error response structureThe change to use a fixed
"AUTHENTICATION_ERROR"message with the exception details in theerrorsfield provides better API consistency and makes error handling more predictable for clients.
94-95: LGTM: Standardized access denied error formatThe change from Korean text to
"ACCESS_DENIED"constant improves internationalization support and API consistency. Moving the exception message to theerrorsfield maintains debugging information while standardizing the response format.
23-24: LGTM: Improved method signature formattingThe multiline method signatures improve readability and follow Java formatting conventions for long parameter lists.
Also applies to: 45-46, 60-61, 74-75, 90-91, 112-113
| import life.mosu.mosuserver.infra.storage.domain.File; | ||
| import life.mosu.mosuserver.infra.storage.domain.Visibility; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Domain → Infrastructure dependency leaks the layering boundary
InquiryAnswerFile now lives in the domain package but extends File and re-uses Visibility, both of which reside in the infra layer.
This inverts the usual onion/hexagonal architecture rule (“domain must not depend on infrastructure”). Consider moving File/Visibility into a shared model (or common) module, or keep InquiryAnswerFile under the same layer as its base class to maintain clear boundaries.
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/domain/inquiryAnswer/InquiryAnswerFile.java
around lines 4 to 5, the domain layer class InquiryAnswerFile depends on
infrastructure layer classes File and Visibility, violating layering boundaries.
To fix this, either move File and Visibility to a shared model or common module
accessible by both layers, or relocate InquiryAnswerFile to the infrastructure
layer to align with its dependencies and maintain proper architectural
boundaries.
| import life.mosu.mosuserver.infra.storage.domain.File; | ||
| import life.mosu.mosuserver.infra.storage.domain.Visibility; |
There was a problem hiding this comment.
Domain layer must not depend on infra packages
InquiryFile now sits in the domain package but still imports File and Visibility from infra.storage.domain.*.
This inverts the intended dependency direction (Domain → Infra), breaking clean-architecture / hexagonal layering and making the domain model impossible to reuse without the infra module.
Refactor options:
-mport life.mosu.mosuserver.infra.storage.domain.File;
-import life.mosu.mosuserver.infra.storage.domain.Visibility;
+// Option A – move File & Visibility to a shared domain-level module, e.g. `life.mosu.mosuserver.domain.common`
+// Option B – introduce a domain-level interface the infra layer implementsFailing to address this now will cement the wrong dependency graph across the codebase.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/domain/inquiry/InquiryFile.java around
lines 4 to 5, the domain layer improperly depends on infra packages by importing
File and Visibility from infra.storage.domain. To fix this, remove these imports
and refactor the code so that domain entities do not rely on infra classes.
Consider introducing domain-level abstractions or interfaces for File and
Visibility, and implement or map them in the infra layer to maintain proper
dependency direction from domain to infra.
| package life.mosu.mosuserver.domain.event; | ||
|
|
||
| import jakarta.persistence.Embeddable; | ||
| import life.mosu.mosuserver.infra.storage.domain.File; | ||
| import life.mosu.mosuserver.infra.storage.domain.Visibility; | ||
| import lombok.AccessLevel; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Domain model now hard-depends on an infra package – reconsider layer boundaries
EventImage was promoted to the domain layer, yet it still imports and extends life.mosu.mosuserver.infra.storage.domain.File and Visibility.
This creates a reverse dependency (domain → infra) that breaks the usual clean / hexagonal layering rule of “domain knows nothing about infra”.
Options to restore the direction:
- package life.mosu.mosuserver.domain.event;
- import life.mosu.mosuserver.infra.storage.domain.File;
- import life.mosu.mosuserver.infra.storage.domain.Visibility;
+ package life.mosu.mosuserver.domain.event;
+ import life.mosu.mosuserver.domain.storage.File; // move or copy to pure-domain module
+ import life.mosu.mosuserver.domain.storage.Visibility;or introduce a storage‐agnostic interface in domain and keep the infra implementation behind an adapter.
Failing to address this now will make future refactors painful and blur the architectural boundaries.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/domain/event/EventImage.java lines 1 to 6,
the EventImage domain model improperly depends on the infra package by importing
and extending File and Visibility from infra.storage.domain, breaking clean
layering rules. To fix this, refactor by removing direct dependencies on infra
classes in the domain layer; instead, define storage-agnostic interfaces or
abstractions within the domain layer and implement these interfaces in the infra
layer, using adapters to connect them. This will restore proper architectural
boundaries and prevent reverse dependencies.
| import life.mosu.mosuserver.infra.storage.domain.File; | ||
| import life.mosu.mosuserver.infra.storage.domain.Visibility; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Domain layer now depends on infrastructure classes – consider re-organising packages
NoticeFile sits in domain.notice but extends/uses File and Visibility from infra.storage.domain.
This introduces an outward dependency from the Domain layer to the Infrastructure layer, which conflicts with typical clean-architecture / DDD layering where infrastructure must depend on domain, not the other way around.
Options:
- Move
FileandVisibilityto a domain-level (e.g.life.mosu.mosuserver.domain.storage) and let infra implement persistence/IO concerns separately. - Keep them in infra but expose them through an interface/abstract type that lives in domain.
Doing so keeps the domain model free of infra concerns and simplifies future migrations (e.g. swapping S3 for another store).
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/domain/notice/NoticeFile.java around lines
4 to 5, the domain layer improperly depends on infrastructure classes File and
Visibility, violating clean architecture principles. To fix this, refactor by
moving File and Visibility to a domain-level package such as
life.mosu.mosuserver.domain.storage, or alternatively create domain-level
interfaces or abstract types for these concepts and have the infrastructure
layer implement them. This removes the direct dependency of the domain on
infrastructure and maintains proper layering.
|
|
||
| public enum UserRole { | ||
| ROLE_USER, ROLE_ADMIN | ||
| ROLE_USER, ROLE_ADMIN, ROLE_PENDING; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Good addition for user registration workflow.
The ROLE_PENDING role is a solid security pattern for new user registrations. This allows the system to control user access until they complete verification or approval processes.
Please verify that the security configuration properly handles this role and that there are mechanisms in place to transition users from ROLE_PENDING to ROLE_USER after registration completion.
🏁 Script executed:
#!/bin/bash
# Check SecurityConfig for ROLE_PENDING handling
ast-grep --pattern $'@Configuration
class SecurityConfig {
$$$
}'
# Check for role transition logic
rg -A 5 "ROLE_PENDING" --type javaLength of output: 11221
Implement transition from ROLE_PENDING to ROLE_USER
I didn’t find any code that moves a newly registered user out of the ROLE_PENDING state once they’ve verified or been approved. Please add the missing pieces below:
• src/main/java/life/mosu/mosuserver/presentation/auth/dto/SignUpRequest.java
– Already sets .userRole(UserRole.ROLE_PENDING) on sign-up.
• No service or controller currently updates the user’s role to ROLE_USER after verification.
– Introduce a confirmation/approval endpoint or service method (e.g. in your AuthService) that loads the user, updates their UserRole, and persists the change.
• Add unit and integration tests covering:
– The verification/approval workflow
– The actual role change in the database
• SecurityConfig (src/main/java/life/mosu/mosuserver/global/config/SecurityConfig.java) only restricts ADMIN paths and uses .anyRequest().permitAll(), so no adjustments are needed there for pending vs. user roles.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/domain/user/UserRole.java at line 4, the
enum defines ROLE_PENDING but there is no logic to transition users from
ROLE_PENDING to ROLE_USER after verification. To fix this, add a method in
AuthService that, upon user verification or approval, loads the user entity,
updates their role from ROLE_PENDING to ROLE_USER, and saves the change to the
database. Also, create a new endpoint or service method to trigger this
transition. Finally, add unit and integration tests to cover the verification
workflow and confirm the role update persists correctly.
| public Long getAccessTokenExpireTime() { | ||
| return accessTokenService.expireTime; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve encapsulation by using a getter method.
The method directly exposes the expireTime field of accessTokenService, which breaks encapsulation principles. Consider adding a getter method in AccessTokenService instead.
- public Long getAccessTokenExpireTime() {
- return accessTokenService.expireTime;
- }
+ public Long getAccessTokenExpireTime() {
+ return accessTokenService.getExpireTime();
+ }Then add a getter method in AccessTokenService:
public Long getExpireTime() {
return expireTime;
}🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/application/auth/AuthTokenManager.java
around lines 44 to 46, the method getAccessTokenExpireTime() directly accesses
the expireTime field of accessTokenService, breaking encapsulation. To fix this,
add a public getter method getExpireTime() in the AccessTokenService class that
returns expireTime, then update getAccessTokenExpireTime() to call this new
getter method instead of accessing the field directly.
| @Value("${target.url}") | ||
| private String targetUrl; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for target URL to prevent open redirect attacks
The target URL from configuration should be validated to ensure it's a trusted domain to prevent open redirect vulnerabilities.
@Value("${target.url}")
private String targetUrl;
+@PostConstruct
+private void validateTargetUrl() {
+ if (targetUrl == null || targetUrl.trim().isEmpty()) {
+ throw new IllegalArgumentException("Target URL cannot be null or empty");
+ }
+ // Add domain validation logic here
+ try {
+ URI uri = URI.create(targetUrl);
+ // Validate that the domain is in your allowed list
+ } catch (IllegalArgumentException e) {
+ throw new IllegalArgumentException("Invalid target URL format", e);
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Value("${target.url}") | |
| private String targetUrl; | |
| @Value("${target.url}") | |
| private String targetUrl; | |
| @PostConstruct | |
| private void validateTargetUrl() { | |
| if (targetUrl == null || targetUrl.trim().isEmpty()) { | |
| throw new IllegalArgumentException("Target URL cannot be null or empty"); | |
| } | |
| // Add domain validation logic here | |
| try { | |
| URI uri = URI.create(targetUrl); | |
| // Validate that the domain is in your allowed list | |
| } catch (IllegalArgumentException e) { | |
| throw new IllegalArgumentException("Invalid target URL format", e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java
around lines 22 to 23, the targetUrl field is injected from configuration
without validation, which can lead to open redirect vulnerabilities. Add
validation logic to check that the targetUrl is within a trusted domain or
matches an allowed whitelist before using it for redirects. Implement this
validation either during initialization or right before redirecting to ensure
only safe URLs are used.
| final String redirectUrlWithToken = UriComponentsBuilder.fromUriString(targetUrl) | ||
| .queryParam("token", accessToken) | ||
| .build() | ||
| .toUriString(); | ||
|
|
||
| response.sendRedirect(redirectWithAccessToken); | ||
| } | ||
| log.info("로그인 성공. 리다이렉트 URL: {}", redirectUrlWithToken); | ||
|
|
||
| private Map<String, String> parseState(final String state) { | ||
| final Map<String, String> params = new HashMap<>(); | ||
| UriComponentsBuilder.fromUriString("?" + state).build() | ||
| .getQueryParams() | ||
| .forEach((key, value) -> params.put(key, value.getFirst())); | ||
| return params; | ||
| response.sendRedirect(redirectUrlWithToken); |
There was a problem hiding this comment.
Security vulnerability: Access token exposed in URL query parameter
Appending the access token as a URL query parameter creates several security risks:
- URLs are logged in server logs, proxy logs, and browser history
- Tokens can be leaked through HTTP Referrer headers
- URLs may be cached by browsers or CDNs
- This violates OAuth2 security best practices
Consider these secure alternatives:
- final String redirectUrlWithToken = UriComponentsBuilder.fromUriString(targetUrl)
- .queryParam("token", accessToken)
- .build()
- .toUriString();
-
- log.info("로그인 성공. 리다이렉트 URL: {}", redirectUrlWithToken);
-
- response.sendRedirect(redirectUrlWithToken);
+ // Option 1: Use HTTP-only cookie (recommended)
+ final ResponseCookie cookie = ResponseCookie.from("accessToken", accessToken)
+ .httpOnly(true)
+ .secure(true)
+ .path("/")
+ .maxAge(3600) // Set appropriate expiration
+ .build();
+ response.addHeader(HttpHeaders.SET_COOKIE, cookie.toString());
+
+ // Option 2: Use POST form to send token securely
+ // Or redirect to a secure endpoint that can handle token exchange
+
+ log.info("로그인 성공. 리다이렉트 URL: {}", targetUrl);
+ response.sendRedirect(targetUrl);🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/global/handler/OAuth2LoginSuccessHandler.java
around lines 35 to 42, avoid appending the access token as a URL query parameter
to prevent security risks like token leakage through logs, referrer headers, and
caches. Instead, modify the code to send the access token securely, such as
setting it in an HTTP-only secure cookie or including it in the response body
after authentication, and then redirecting without the token in the URL.
| @PostMapping("/reissue") | ||
| public ResponseEntity<ApiResponseWrapper<Token>> reissueAccessToken( | ||
| final HttpServletRequest servletRequest) { | ||
| final Token token = authService.reissueAccessToken(servletRequest); | ||
| final String authorization = token.grantType() + " " + token.accessToken(); | ||
| return ResponseEntity.status(HttpStatus.CREATED) | ||
| .header(HttpHeaders.AUTHORIZATION, authorization) | ||
| .body(ApiResponseWrapper.success(HttpStatus.CREATED, token)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add authentication validation for token reissue endpoint
The reissue endpoint should validate the current refresh token before issuing new tokens to prevent unauthorized token generation.
@PostMapping("/reissue")
public ResponseEntity<ApiResponseWrapper<Token>> reissueAccessToken(
final HttpServletRequest servletRequest) {
+ // Validate that the request contains a valid refresh token
+ // This validation should be done in the service layer
final Token token = authService.reissueAccessToken(servletRequest);
final String authorization = token.grantType() + " " + token.accessToken();
return ResponseEntity.status(HttpStatus.CREATED)
.header(HttpHeaders.AUTHORIZATION, authorization)
.body(ApiResponseWrapper.success(HttpStatus.CREATED, token));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @PostMapping("/reissue") | |
| public ResponseEntity<ApiResponseWrapper<Token>> reissueAccessToken( | |
| final HttpServletRequest servletRequest) { | |
| final Token token = authService.reissueAccessToken(servletRequest); | |
| final String authorization = token.grantType() + " " + token.accessToken(); | |
| return ResponseEntity.status(HttpStatus.CREATED) | |
| .header(HttpHeaders.AUTHORIZATION, authorization) | |
| .body(ApiResponseWrapper.success(HttpStatus.CREATED, token)); | |
| } | |
| @PostMapping("/reissue") | |
| public ResponseEntity<ApiResponseWrapper<Token>> reissueAccessToken( | |
| final HttpServletRequest servletRequest) { | |
| // Validate that the request contains a valid refresh token | |
| // This validation should be done in the service layer | |
| final Token token = authService.reissueAccessToken(servletRequest); | |
| final String authorization = token.grantType() + " " + token.accessToken(); | |
| return ResponseEntity.status(HttpStatus.CREATED) | |
| .header(HttpHeaders.AUTHORIZATION, authorization) | |
| .body(ApiResponseWrapper.success(HttpStatus.CREATED, token)); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/presentation/auth/AuthController.java
around lines 57 to 65, the reissueAccessToken method lacks validation of the
current refresh token before issuing a new access token. To fix this, add logic
to extract and validate the refresh token from the HttpServletRequest, ensuring
it is valid and authorized before calling authService.reissueAccessToken. If the
token is invalid, respond with an appropriate error status to prevent
unauthorized token generation.
| @UserId Long userId, | ||
| @Valid @RequestBody ProfileRequest request |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent parameter handling across endpoints
The create and update methods use @UserId annotation (good practice), but the getProfile method still uses @RequestParam. This creates inconsistency in how user identification is handled across the controller.
Consider standardizing all endpoints to use @UserId for authenticated user operations:
@GetMapping
+@PreAuthorize("isAuthenticated()")
public ApiResponseWrapper<ProfileDetailResponse> getProfile(
- @RequestParam Long userId) {
+ @UserId Long userId) {Also applies to: 43-44
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/presentation/profile/ProfileController.java
around lines 32-33 and 43-44, the getProfile method uses @RequestParam for user
identification while create and update methods use @UserId. To maintain
consistency, replace the @RequestParam annotation with @UserId in the getProfile
method parameters so all authenticated user operations use the @UserId
annotation uniformly.
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Snowflake전략으로 id 설정하겠습니다Summary by CodeRabbit
New Features
Improvements
Refactor
Chores