Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
0be5c3d
feat(bucket4j) : 의존성 추가
jsoonworld Jun 14, 2025
bc72e2c
feat(JwtAuthenticationFilter): Rate Limit 기능 추가를 위한 Bucket4j 의존성 설정
jsoonworld Jun 15, 2025
b61c39e
refactor(GlobalExceptionHandler): RateLimitException 핸들러 제거
jsoonworld Jun 15, 2025
322ccb3
feat(CustomJwtAuthenticationEntryPoint): 인증 실패 시 401 JSON 에러 응답을 직접 생…
jsoonworld Jun 15, 2025
79058ab
feat(JwtAuthenticationFilter): 잘못된 JWT 토큰 요청에 대한 Rate Limit 적용
jsoonworld Jun 15, 2025
6b7feab
refactor(JwtTokenVerifier): 예외 발생 시 호출자에게 책임을 위임하도록 변경
jsoonworld Jun 15, 2025
e6ec620
feat(RateLimitingService): Bucket4j를 이용한 IP 기반 Rate Limiting 서비스 구현
jsoonworld Jun 15, 2025
d94aaef
feat(IpAddressUtil): 클라이언트 IP 주소 추출 유틸리티 구현
jsoonworld Jun 15, 2025
0b01fcf
refactor(Company): 전체 필드를 포함하는 생성자 추가
jsoonworld Jun 15, 2025
dd54ab1
refactor(InternshipAnnouncement): 전체 필드를 초기화하는 생성자 추가
jsoonworld Jun 15, 2025
c4cc53a
test(JwtAuthenticationFilter): Rate Limit 기능에 대한 통합 테스트 추가
jsoonworld Jun 15, 2025
e1dc0b0
style(ScrapServiceTest): 코드 스타일 수정
jsoonworld Jun 15, 2025
d21b1da
refactor(JwtAuthenticationFilter): 불필요한 예외 처리 로직 제거 및 코드 간소화
jsoonworld Jun 15, 2025
e05ae06
refactor(JwtAuthenticationFilter): 잘못된 토큰 요청 시 401 응답을 즉시 반환하도록 수정
jsoonworld Jun 15, 2025
019d77c
refactor(JwtFilter) : 코드 리뷰 반영
jsoonworld Jun 16, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 7 additions & 22 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ dependencies {
implementation 'io.jsonwebtoken:jjwt-api:0.11.5'
implementation 'io.jsonwebtoken:jjwt-impl:0.11.5'
implementation 'io.jsonwebtoken:jjwt-jackson:0.11.5'
// implementation 'com.nimbusds:nimbus-jose-jwt:3.10'

// Gson
implementation 'com.google.code.gson:gson:2.8.6'
Expand All @@ -70,35 +69,21 @@ dependencies {
// Spring Batch
implementation 'org.springframework.boot:spring-boot-starter-batch'

// Bucket4j
implementation 'com.bucket4j:bucket4j-core:8.1.0'
}

//QueryDSL 초기 설정
//1. Q-Class를 생성할 디렉토리 경로를 설정합니다.
def queryDslSrcDir = 'src/main/generated/querydsl/'
def generatedDir = 'src/main/generated'

//2. JavaCompile Task를 수행하는 경우 생성될 소스코드의 출력 디렉토리를 queryDslSrcDir로 설정합니다.
tasks.withType(JavaCompile).configureEach {
options.getGeneratedSourceOutputDirectory().set(file(queryDslSrcDir))
}

//3. 소스 코드로 인식할 디렉토리 경로에 Q-Class 파일을 추가합니다. 이렇게 하면 Q-Class가 일반 Java 클래스처럼 취급되어 컴파일과 실행 시 classPath에 포함됩니다.
sourceSets {
main.java.srcDirs += [queryDslSrcDir]
main.java.srcDirs += [generatedDir]
}

//4. clean Task를 수행하는 경우 지정한 디렉토리를 삭제하도록 설정합니다. -> 자동 생성된 Q-Class를 제거합니다.
clean {
delete file(queryDslSrcDir)
}

//5. QueryDSL과 관련된 라이브러리들이 컴파일 시점에만 필요하도록 설정합니다. 또한, QueryDSL 설정을 컴파일 클래스 패스에 추가합니다.
configurations {
compileOnly {
extendsFrom annotationProcessor
}
querydsl.extendsFrom compileClasspath
delete file(generatedDir)
}
// =================================================

tasks.named('test') {
useJUnitPlatform()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,25 +47,23 @@ public ResponseEntity<ErrorResponse> handleAuthException(JwtException e) {
}

@ExceptionHandler(Exception.class)
public ResponseEntity handleException(Exception e){
public ResponseEntity<ErrorResponse> handleException(Exception e){
log.warn("[Exception] cause: {} , message: {}", NestedExceptionUtils.getMostSpecificCause(e), e.getMessage());
ErrorMessage errorCode = ErrorMessage.INTERNAL_SERVER_ERROR;
return ResponseEntity
.status(errorCode.getStatus())
.body(ErrorResponse.of(errorCode.getStatus(), errorCode.getMessage()));
}

//메소드가 잘못되었거나 부적합한 인수를 전달했을 경우 -> 필수 파라미터 없을 때
@ExceptionHandler(IllegalArgumentException.class)
public ResponseEntity handleIllegalArgumentException(IllegalArgumentException e){
public ResponseEntity<ErrorResponse> handleIllegalArgumentException(IllegalArgumentException e){
log.warn("[IlleagalArgumentException] cause: {} , message: {}", NestedExceptionUtils.getMostSpecificCause(e), e.getMessage());
ErrorMessage errorCode = ErrorMessage.ILLEGAL_ARGUMENT_ERROR;
return ResponseEntity
.status(errorCode.getStatus())
.body(ErrorResponse.of(errorCode.getStatus(), errorCode.getMessage()));
}

//@Valid 유효성 검사에서 예외가 발생했을 때 -> requestbody에 잘못 들어왔을 때
@ExceptionHandler(MethodArgumentNotValidException.class)
public ResponseEntity<ErrorResponse> handleMethodArgumentNotValidException(MethodArgumentNotValidException e){
log.warn("[MethodArgumentNotValidException] cause: {}, message: {}", NestedExceptionUtils.getMostSpecificCause(e), e.getMessage());
Expand All @@ -75,7 +73,6 @@ public ResponseEntity<ErrorResponse> handleMethodArgumentNotValidException(Metho
.body(ErrorResponse.of(errorCode.getStatus(), errorCode.getMessage()));
}

//잘못된 포맷 요청 -> Json으로 안보내다던지
@ExceptionHandler(HttpMessageNotReadableException.class)
public ResponseEntity<ErrorResponse> handleHttpMessageNotReadableException(HttpMessageNotReadableException e){
log.warn("[HttpMessageNotReadableException] cause: {}, message: {}", NestedExceptionUtils.getMostSpecificCause(e), e.getMessage());
Expand All @@ -84,6 +81,7 @@ public ResponseEntity<ErrorResponse> handleHttpMessageNotReadableException(HttpM
.status(errorCode.getStatus())
.body(ErrorResponse.of(errorCode.getStatus(), errorCode.getMessage()));
}

@ExceptionHandler(HttpRequestMethodNotSupportedException.class)
public ResponseEntity<ErrorResponse> handleHttpMethodException(
HttpRequestMethodNotSupportedException e,
Expand All @@ -96,4 +94,3 @@ public ResponseEntity<ErrorResponse> handleHttpMethodException(
.body(ErrorResponse.of(errorCode.getStatus(), errorCode.getMessage()));
}
}

Original file line number Diff line number Diff line change
@@ -1,26 +1,37 @@
package org.terning.terningserver.common.security.jwt.filter;

import com.fasterxml.jackson.databind.ObjectMapper;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import lombok.RequiredArgsConstructor;
import org.springframework.http.MediaType;
import org.springframework.security.core.AuthenticationException;
import org.springframework.security.web.AuthenticationEntryPoint;
import org.springframework.stereotype.Component;
import org.terning.terningserver.common.exception.dto.ErrorResponse;
import org.terning.terningserver.common.security.jwt.exception.JwtErrorCode;
import org.terning.terningserver.common.security.jwt.exception.JwtException;

import java.io.IOException;

@Component
@RequiredArgsConstructor
public class CustomJwtAuthenticationEntryPoint implements AuthenticationEntryPoint {

private final ObjectMapper objectMapper;

@Override
public void commence(
HttpServletRequest request,
HttpServletResponse response,
AuthenticationException exception
AuthenticationException authException
) throws IOException {
throw new JwtException(JwtErrorCode.INVALID_JWT_TOKEN);
response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
response.setContentType(MediaType.APPLICATION_JSON_VALUE);
response.setCharacterEncoding("UTF-8");

JwtErrorCode errorCode = JwtErrorCode.INVALID_JWT_TOKEN;
ErrorResponse errorResponse = ErrorResponse.of(errorCode.getStatus().value(), errorCode.getMessage());

response.getWriter().write(objectMapper.writeValueAsString(errorResponse));
}
}
Original file line number Diff line number Diff line change
@@ -1,14 +1,21 @@
package org.terning.terningserver.common.security.jwt.filter;

import io.github.bucket4j.Bucket;
import io.github.bucket4j.ConsumptionProbe;
import jakarta.servlet.FilterChain;
import jakarta.servlet.ServletException;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.http.HttpStatus;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.stereotype.Component;
import org.springframework.web.filter.OncePerRequestFilter;
import org.terning.terningserver.common.security.jwt.auth.UserAuthentication;
import org.terning.terningserver.common.security.jwt.exception.JwtException;
import org.terning.terningserver.common.security.ratelimit.RateLimitingService;
import org.terning.terningserver.common.util.IpAddressUtil;

import java.io.IOException;
import java.util.Optional;
Expand All @@ -17,15 +24,40 @@

@Component
@RequiredArgsConstructor
@Slf4j
public class JwtAuthenticationFilter extends OncePerRequestFilter {

private final JwtTokenVerifier jwtTokenVerifier;
private final RateLimitingService rateLimitingService; // RateLimitingService 주입

@Override
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain) throws ServletException, IOException {
extractToken(request)
.flatMap(jwtTokenVerifier::validateAndExtractUserId)
.ifPresent(this::authenticateUser);
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain)
throws ServletException, IOException {

Optional<String> token = extractToken(request);

if (token.isPresent()) {
try {
Long userId = jwtTokenVerifier.validateAndExtractUserId(token.get())
.orElseThrow(() -> new JwtException(null));

Copilot AI Jun 15, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing null into JwtException will likely cause a null-pointer or unclear error. Consider using a specific JwtErrorCode or refactoring so that this orElseThrow is unreachable.

Suggested change
.orElseThrow(() -> new JwtException(null));
.orElseThrow(() -> new JwtException("Invalid JWT token: User ID could not be extracted."));

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋은 피드백 감사합니다. 👍

피드백대로, 예외 생성자에 null을 전달하는 것은 잠재적인 NPE를 유발하고 에러 추적을 어렵게 할 수 있어 수정이 필요한 것 같네요

명확한 에러 파악을 위해 null 대신 구체적인 한글 메시지를 사용하도록 코드를 개선하겠습니다.

authenticateUser(userId);

} catch (JwtException e) {
String clientIp = IpAddressUtil.getClientIp(request);
Bucket bucket = rateLimitingService.resolveBucket(clientIp);
ConsumptionProbe probe = bucket.tryConsumeAndReturnRemaining(1);

if (probe.isConsumed()) {
log.error("[ERROR] 유효하지 않은 JWT 토큰 요청. IP: {}. 남은 시도 횟수: {}", clientIp, probe.getRemainingTokens());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기는 아직 재시도를 할 수 있는 단계라 log.warn이 적합하다고 생각하는데 장순님은 어떻게 생각하시나요!?
요청이 차단되는 경우에는 저도 log.error가 적합하다고 생각해요!

말씀드린 이유는 로그레벨을 명확히 해야 나중에 에러 추적할 때도 편리할 것 같아서 여쭤봅니다!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

정윤님, 정말 좋은 의견 감사합니다! 덕분에 로그 레벨의 역할에 대해 다시 한번 생각하게 됐어요.

말씀해주신 것처럼, 아직 재시도가 가능한 유효하지 않은 토큰 요청은 log.warn으로 처리하고, 요청이 실제로 차단되는 과도한 시도는 log.error로 남기는 것이 로그의 의미를 훨씬 명확하게 만든다는 점에 전적으로 동의합니다. 이렇게 구분해두면 나중에 에러를 추적하거나 모니터링할 때 정말 편리할 것 같아요.

사실 정윤님께서 의견을 주셨을 때, '정말 좋은 생각인데 우리 팀에 명확한 로그 레벨 컨벤션이 있었나?' 하는 고민이 들었어요. 이번 기회를 발판 삼아, 정윤님 의견을 기준으로 간단하게라도 로그 컨벤션을 정하고 가면 앞으로의 개발 문화에도 좋은 영향을 줄 것 같다는 생각이 듭니다!

의견 주신 대로 해당 부분은 바로 log.warn으로 수정하겠습니다. 꼼꼼하게 리뷰해주셔서 정말 감사합니다! 😊

SecurityContextHolder.clearContext();

Copilot AI Jun 15, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After clearing the security context for an invalid token, the filter continues without sending a 401 response. It’s clearer to call response.sendError(HttpStatus.UNAUTHORIZED.value(), ...) and return immediately to prevent downstream handlers from treating the request as anonymous.

Suggested change
SecurityContextHolder.clearContext();
SecurityContextHolder.clearContext();
response.sendError(HttpStatus.UNAUTHORIZED.value(), "Invalid JWT token");
return;

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

피드백 감사합니다. 👍

피드백대로, 인증 실패 시 즉시 401 에러를 반환하고 필터 체인을 중단하는 것이 더 명확하고 안전하다고 판단하여 제안해주신 내용대로 수정하겠습니다!

} else {
long waitForRefillSeconds = probe.getNanosToWaitForRefill() / 1_000_000_000L;
log.error("[ERROR] 과도한 JWT 토큰 요청. IP: {}. 요청을 차단합니다. 대기 시간: {}초", clientIp, waitForRefillSeconds);
response.sendError(HttpStatus.TOO_MANY_REQUESTS.value(), "과도한 요청입니다. 잠시 후 다시 시도해주세요.");
return;
}
}
}

filterChain.doFilter(request, response);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import lombok.RequiredArgsConstructor;
import org.springframework.stereotype.Service;
import org.terning.terningserver.common.security.jwt.application.JwtUserIdExtractor;
import org.terning.terningserver.common.security.jwt.exception.JwtException;

import java.util.Optional;

Expand All @@ -12,11 +13,7 @@ public class JwtTokenVerifier {

private final JwtUserIdExtractor jwtUserIdExtractor;

public Optional<Long> validateAndExtractUserId(String token) {
try {
return Optional.of(jwtUserIdExtractor.extractUserId(token));
} catch (Exception e) {
return Optional.empty();
}
public Optional<Long> validateAndExtractUserId(String token) throws JwtException {
return Optional.of(jwtUserIdExtractor.extractUserId(token));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이거 왜 Optional로 감싸서 반환하는지 알 수 있을까요?! jwt에서 userId 파싱할 수 없을 때는 에러를 뱉어야 하지 않나 싶어서요!

+++ 지금 JwtTokenVerifier 클래스에 validateAndExtractUserId()만 존재하고, 해당 메서드도 jwtUserIdExtractor.extractUserId(token)를 호출하는 역할만 있는 걸로 보이는데 JwtTokenVerifier가 필요한 이유가 있나요??

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

정윤님, 정말 날카로운 피드백이네요!. 두 가지 질문 모두 타당하고, 덕분에 코드 구조를 다시 한번 깊게 점검하게 되었습니다.

1. Optional 반환 타입에 대하여

먼저 Optional로 감싸서 반환하는 부분에 대한 질문이 맞습니다. 결론부터 말씀드리면, Optional을 사용하는 것은 중복이며 잘못된 설계같네요!

정윤님께서 말씀하신 대로, 이 메서드의 역할은 토큰을 **검증(validate)**하고 userId를 **추출(extract)**하는 것입니다. 검증에 실패했을 경우, 예를 들어 토큰이 유효하지 않거나 userId를 파싱할 수 없을 때는 JwtException을 던져서 호출부(Filter)에서 명확하게 예외 상황을 처리하도록 하는 것이 올바른 책임 분리겠네요!.

현재 throws JwtException 시그니처가 이미 실패 케이스를 처리하고 있으므로, 성공 케이스의 결과(userId)를 Optional로 감쌀 이유가 전혀 없고, 오히려 Optional을 사용함으로써 호출하는 쪽에서 불필요한 .get() 이나 .orElseThrow() 같은 처리를 한 번 더 유발하게 될 것 같네요.

말씀해주신 대로 Optional을 제거하고 Long 타입을 직접 반환하도록 수정하겠습니다!

2. JwtTokenVerifier 클래스의 필요성에 대하여

두 번째로 지적해주신 JwtTokenVerifier 클래스의 필요성에 대해서도 전적으로 동감합니다.

초기 구상에서는 JwtTokenVerifier가 토큰의 유효성 검증(만료 시간, 서명 등)과 정보 추출을 모두 총괄하는 역할을 할 것으로 생각했습니다. 하지만 개발 과정에서 책임이 세분화되어 JwtUserIdExtractor 등으로 역할이 넘어가면서, 지금처럼 단순히 호출만 하는 래퍼(wrapper) 클래스로 남게 된 것 같네요.

현재 상태의 JwtTokenVerifier는 아무런 추가 로직 없이 단순히 JwtUserIdExtractor의 메서드를 호출만 하므로, 불필요한 추상화 계층을 만들고 코드의 복잡성을 높이는 것 같아요

결론 및 수정 계획

따라서 정윤님의 의견을 적극 수용하여 다음과 같이 리팩토링을 진행하겠습니다.

  1. JwtTokenVerifier 클래스를 제거하겠습니다.
  2. JwtAuthenticationFilterJwtUserIdExtractor를 직접 주입받아 사용하도록 수정하겠습니다.
  3. JwtUserIdExtractorextractUserId 메서드는 Long 타입을 직접 반환하고, 실패 시 JwtException을 던지는 책임을 명확히 하도록 하겠습니다.

꼼꼼한 피드백 감사합니다~!

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package org.terning.terningserver.common.security.ratelimit;

import io.github.bucket4j.Bandwidth;
import io.github.bucket4j.Bucket;
import io.github.bucket4j.Refill;
import org.springframework.stereotype.Service;

import java.time.Duration;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;

@Service
public class RateLimitingService {

private final Map<String, Bucket> cache = new ConcurrentHashMap<>();

Copilot AI Jun 15, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using an unbounded ConcurrentHashMap for IP buckets may lead to unbounded memory growth. Consider adding eviction (e.g., using Caffeine or a TTL) to prevent a memory leak for many distinct IP addresses.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

피드백 감사합니다. 👍

말씀해주신 대로, 실제 프로덕션 환경에서는 메모리 누수를 방지하기 위해 캐시 제거(Eviction) 정책이 반드시 필요하다는 점에 전적으로 동의합니다. ConcurrentHashMap만으로는 장기적인 서비스 안정성을 보장하기 어렵다는 것을 잘 알고 있습니다.

다만, 현재는 문제해결 초기 개발 단계로 실제 트래픽이 발생하기 전이라 동시 접속자 수가 많지 않을 것으로 예상하고 있습니다. 따라서, 우선은 ConcurrentHashMap을 이용한 최소한의 구현으로 핵심 기능을 완성하고, 정식 출시 또는 베타 테스트 이전에 제안해주신 Caffeine 캐시 도입을 통해 메모리 관리 로직을 반드시 보강하도록 하겠습니다.

잊지 않도록 이 내용은 별도의 기술 부채(Technical Debt) 이슈로 등록해서 반드시 추적하고 관리하겠습니다.

피드백 다시 한번 감사드립니다!


public Bucket resolveBucket(String ipAddress) {
return cache.computeIfAbsent(ipAddress, this::newBucket);
}

private Bucket newBucket(String ipAddress) {
Refill refill = Refill.intervally(10, Duration.ofMinutes(1));
Bandwidth limit = Bandwidth.classic(10, refill);
Comment on lines +22 to +23

Copilot AI Jun 15, 2025

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rate limit parameters (10 requests per minute) are hard-coded. Extract these values into configuration properties to allow tuning without a code change.

Suggested change
Refill refill = Refill.intervally(10, Duration.ofMinutes(1));
Bandwidth limit = Bandwidth.classic(10, refill);
Refill refill = Refill.intervally(requestsPerMinute, Duration.ofMinutes(durationInMinutes));
Bandwidth limit = Bandwidth.classic(requestsPerMinute, refill);

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋은 의견 감사합니다! 👍

말씀해주신 대로, Rate Limit 설정을 외부 설정 파일(application.yml)로 분리하는 것이 장기적인 유연성과 유지보수성 측면에서 훨씬 좋은 방향이라는 점에 전적으로 동의합니다.

다만, 현재 문제해결 초기 단계이고 Rate Limit 정책이 단기간에 변경될 가능성은 낮다고 판단했습니다. 우선은 핵심 기능 구현에 집중하고, 지금 당장 외부 설정으로 분리하는 것은 현재 단계에서는 오버엔지니어링이 될 수 있다고 생각했습니다.

현재 구현된 방식도 기능적으로는 문제가 없으므로, 일단은 이대로 진행하고 추후 Rate Limit 정책을 고도화하거나 환경별로 다른 설정을 적용해야 할 시점이 오면, 제안해주신 방식을 적극적으로 도입하여 리팩토링하도록 하겠습니다.

피드백 다시 한번 감사합니다!

return Bucket.builder()
.addLimit(limit)
.build();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package org.terning.terningserver.common.util;

import jakarta.servlet.http.HttpServletRequest;

public class IpAddressUtil {

private static final String[] IP_HEADER_CANDIDATES = {
"X-Forwarded-For",
"Proxy-Client-IP",
"WL-Proxy-Client-IP",
"HTTP_X_FORWARDED_FOR",
"HTTP_X_FORWARDED",
"HTTP_X_CLUSTER_CLIENT_IP",
"HTTP_CLIENT_IP",
"HTTP_FORWARDED_FOR",
"HTTP_FORWARDED",
"HTTP_VIA",
"REMOTE_ADDR"
};

public static String getClientIp(HttpServletRequest request) {
for (String header : IP_HEADER_CANDIDATES) {
String ip = request.getHeader(header);
if (ip != null && !ip.isEmpty() && !"unknown".equalsIgnoreCase(ip)) {
return ip.split(",")[0];
}
}
return request.getRemoteAddr();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import jakarta.persistence.Embeddable;
import jakarta.persistence.EnumType;
import jakarta.persistence.Enumerated;
import lombok.AllArgsConstructor;
import lombok.Getter;
import lombok.NoArgsConstructor;

Expand All @@ -13,6 +14,7 @@
@Embeddable
@Getter
@NoArgsConstructor(access = PROTECTED)
@AllArgsConstructor
public class Company {

@Column(nullable = false, length = 64)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.terning.terningserver.internshipAnnouncement.domain;

import jakarta.persistence.*;
import lombok.AllArgsConstructor;
import lombok.Getter;
import lombok.NoArgsConstructor;
import org.terning.terningserver.common.BaseTimeEntity;
Expand All @@ -15,6 +16,7 @@
@Entity
@Getter
@NoArgsConstructor(access = PROTECTED)
@AllArgsConstructor
Comment on lines 18 to +19

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AllArgsConstructor요거 왜 추가된건가요?!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

정윤님, 좋은 질문 감사합니다! 덕분에 코드에 대해 더 깊이 고민해볼 수 있었어요. 😊

말씀주신 @AllArgsConstructorScrapServiceTest.java에서 테스트용 객체를 생성할 때 발생하던 컴파일 에러를 해결하기 위해 일단 추가한 것이었습니다.

하지만 정윤님 말씀처럼 엔티티에 @AllArgsConstructor를 바로 사용하는 것은, 특히 저희 InternshipAnnouncement처럼 필드가 많을 때 다음과 같은 단점이 있다는 점에 저도 전적으로 동의합니다.

  • 가독성 저하: new Announcement(...) 형태로 코드를 작성하면 어떤 인자가 어떤 필드에 들어가는지 파악하기 어렵고.
  • 유지보수의 어려움: 추후 필드 순서가 바뀌거나 추가/삭제될 경우, 생성자를 사용하는 모든 코드가 깨질 위험이 있겠네요.

그래서 장기적으로는 @Builder 패턴을 적용해서 테스트 코드를 개선하는 것이 훨씬 좋은 방향이라고 생각합니다. 빌더를 사용하면 필요한 값만 명확하게 설정할 수 있어 가독성과 안정성을 모두 잡을 수 있으니까요.

제가 원본 테스트 작성자가 아니라 조심스럽긴 하지만, 괜찮으시다면 이 기회에 해당 부분을 @Builder로 리팩토링하는 건 어떨까요? 제가 직접 수정해서 다시 올릴 수도 있고, 더 좋은 의견이 있으시면 언제든 편하게 말씀해주세요!

꼼꼼하게 리뷰해주셔서 다시 한번 감사합니다!

public class InternshipAnnouncement extends BaseTimeEntity {

@Id
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package org.terning.terningserver.common.security.jwt.filter;

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.boot.test.mock.mockito.MockBean;
import org.springframework.http.HttpHeaders;
import org.springframework.test.web.servlet.MockMvc;
import org.springframework.test.web.servlet.ResultActions;
import org.terning.terningserver.common.security.jwt.exception.JwtErrorCode;
import org.terning.terningserver.common.security.jwt.exception.JwtException;

import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.when;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;

@SpringBootTest
@AutoConfigureMockMvc
class JwtAuthenticationFilterTest {

private static final Logger log = LoggerFactory.getLogger(JwtAuthenticationFilterTest.class);

@Autowired
private MockMvc mockMvc;

@MockBean
private JwtTokenVerifier jwtTokenVerifier;

@Test
@DisplayName("잘못된 JWT 토큰으로 반복 요청 시, Rate Limit에 따라 429 에러를 반환한다")
void when_repeated_requests_with_invalid_token_then_return_429_error() throws Exception {
// given
String invalidToken = "this-is-an-invalid-token";
when(jwtTokenVerifier.validateAndExtractUserId(anyString()))
.thenThrow(new JwtException(JwtErrorCode.INVALID_JWT_TOKEN));

// when & then
log.info("====== Rate Limit 허용 횟수 내 요청 테스트 시작 ======");
for (int i = 1; i <= 10; i++) {
log.info("{}번째 요청", i);
// when
ResultActions actions = mockMvc.perform(get("/api/v1/any-secured-endpoint")
.header(HttpHeaders.AUTHORIZATION, "Bearer " + invalidToken));
// then
actions.andExpect(status().isUnauthorized());
}
log.info("====== Rate Limit 허용 횟수 내 요청 테스트 통과 ======");


// when & then
log.info("\n====== Rate Limit 초과 요청 테스트 시작 ======");
log.info("11번째 요청");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요거 테스트코드에서 로그 찍는거는 테스트 완료하셨다면 혹시 머지 전에 삭제하는건 어떨가요?!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

정윤님, 좋은 의견 감사합니다!
말씀해주신 대로, 해당 로그들은 테스트를 처음 개발하면서 로직의 흐름을 눈으로 확인하기 위해 추가했던 부분입니다!

테스트가 안정적으로 동작하는 것을 확인했고, 성공한 테스트의 로그는 빌드 결과 확인 시 노이즈가 될 수 있다는 점에 공감합니다. 코드 리뷰의 핵심은 단언(assert)을 통한 검증에 있으니, 머지하기 전에 더 깔끔한 테스트 환경을 위해 해당 로그들은 모두 삭제하도록 하겠습니다!

꼼꼼하게 봐주셔서 감사합니다! 😊

// when
ResultActions finalAction = mockMvc.perform(get("/api/v1/any-secured-endpoint")
.header(HttpHeaders.AUTHORIZATION, "Bearer " + invalidToken));
// then
finalAction.andExpect(status().isTooManyRequests());
log.info("====== Rate Limit 초과 요청 테스트 통과 ======");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -211,4 +211,4 @@ public void setup() {
assertThat(savedAnnouncement.getScrapCount()).isEqualTo(100L);
}
}
}
}
Loading