-
Notifications
You must be signed in to change notification settings - Fork 2
[✨ feat] 잘못된 JWT 토큰 요청에 대한 Rate Limit 적용 #249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 14 commits
0be5c3d
bc72e2c
b61c39e
322ccb3
79058ab
6b7feab
e6ec620
d94aaef
0b01fcf
dd54ab1
c4cc53a
e1dc0b0
d21b1da
e05ae06
019d77c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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; | ||||||||||
|
|
@@ -17,15 +24,43 @@ | |||||||||
|
|
||||||||||
| @Component | ||||||||||
| @RequiredArgsConstructor | ||||||||||
| @Slf4j | ||||||||||
| public class JwtAuthenticationFilter extends OncePerRequestFilter { | ||||||||||
|
|
||||||||||
| private final JwtTokenVerifier jwtTokenVerifier; | ||||||||||
| private final 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()).get(); | ||||||||||
| 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()); | ||||||||||
| SecurityContextHolder.clearContext(); | ||||||||||
|
||||||||||
| SecurityContextHolder.clearContext(); | |
| SecurityContextHolder.clearContext(); | |
| response.sendError(HttpStatus.UNAUTHORIZED.value(), "Invalid JWT token"); | |
| return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
피드백 감사합니다. 👍
피드백대로, 인증 실패 시 즉시 401 에러를 반환하고 필터 체인을 중단하는 것이 더 명확하고 안전하다고 판단하여 제안해주신 내용대로 수정하겠습니다!
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
||
|
|
@@ -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)); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이거 왜 Optional로 감싸서 반환하는지 알 수 있을까요?! jwt에서 userId 파싱할 수 없을 때는 에러를 뱉어야 하지 않나 싶어서요! +++ 지금
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 정윤님, 정말 날카로운 피드백이네요!. 두 가지 질문 모두 타당하고, 덕분에 코드 구조를 다시 한번 깊게 점검하게 되었습니다. 1.
|
||
| } | ||
| } | ||
| 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<>(); | ||||||||||
|
||||||||||
|
|
||||||||||
| 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
|
||||||||||
| 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); |
There was a problem hiding this comment.
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 정책을 고도화하거나 환경별로 다른 설정을 적용해야 할 시점이 오면, 제안해주신 방식을 적극적으로 도입하여 리팩토링하도록 하겠습니다.
피드백 다시 한번 감사합니다!
| 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 |
|---|---|---|
| @@ -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; | ||
|
|
@@ -15,6 +16,7 @@ | |
| @Entity | ||
| @Getter | ||
| @NoArgsConstructor(access = PROTECTED) | ||
| @AllArgsConstructor | ||
|
Comment on lines
18
to
+19
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 정윤님, 좋은 질문 감사합니다! 덕분에 코드에 대해 더 깊이 고민해볼 수 있었어요. 😊 말씀주신 하지만 정윤님 말씀처럼 엔티티에
그래서 장기적으로는 제가 원본 테스트 작성자가 아니라 조심스럽긴 하지만, 괜찮으시다면 이 기회에 해당 부분을 꼼꼼하게 리뷰해주셔서 다시 한번 감사합니다! |
||
| public class InternshipAnnouncement extends BaseTimeEntity { | ||
|
|
||
| @Id | ||
|
|
||
| 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번째 요청"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 요거 테스트코드에서 로그 찍는거는 테스트 완료하셨다면 혹시 머지 전에 삭제하는건 어떨가요?!
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
|---|---|---|
|
|
@@ -211,4 +211,4 @@ public void setup() { | |
| assertThat(savedAnnouncement.getScrapCount()).isEqualTo(100L); | ||
| } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기는 아직 재시도를 할 수 있는 단계라
log.warn이 적합하다고 생각하는데 장순님은 어떻게 생각하시나요!?요청이 차단되는 경우에는 저도
log.error가 적합하다고 생각해요!말씀드린 이유는 로그레벨을 명확히 해야 나중에 에러 추적할 때도 편리할 것 같아서 여쭤봅니다!
There was a problem hiding this comment.
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으로 수정하겠습니다. 꼼꼼하게 리뷰해주셔서 정말 감사합니다! 😊