Skip to content

Conversation

@jsoonworld
Copy link
Member

@jsoonworld jsoonworld commented Jun 21, 2025

๐Ÿ“„ Work Description

๋ฌธ์ œ ์ƒํ™ฉ

๊ธฐ์กด JwtAuthenticationFilter๋Š” SecurityConfig์˜ permitAll ์„ค์ •๊ณผ ๊ด€๊ณ„์—†์ด ๋ชจ๋“  ์š”์ฒญ ํ—ค๋”์— ์žˆ๋Š” ํ† ํฐ์˜ ์œ ํšจ์„ฑ์„ ๊ฒ€์‚ฌํ–ˆ์Šต๋‹ˆ๋‹ค. ์ด๋กœ ์ธํ•ด ํ† ํฐ ์žฌ๋ฐœ๊ธ‰, ์†Œ์…œ ๋กœ๊ทธ์ธ ๋“ฑ ๋งŒ๋ฃŒ๋œ ํ† ํฐ์„ ์‚ฌ์šฉํ•˜๊ฑฐ๋‚˜ ํ† ํฐ์ด ์—†๋Š” ์ƒํƒœ๋กœ ์ ‘๊ทผํ•ด์•ผ ํ•˜๋Š” ์ธ์ฆ ๊ด€๋ จ API ์š”์ฒญ์ด ํ•„ํ„ฐ ๋‹จ๊ณ„์—์„œ ๋ฏธ๋ฆฌ ์ฐจ๋‹จ๋˜๋Š” ๋ฌธ์ œ๊ฐ€ ๋ฐœ์ƒํ–ˆ์Šต๋‹ˆ๋‹ค.

ํ•ด๊ฒฐ ๋ฐฉ์•ˆ

OncePerRequestFilter๊ฐ€ ์ œ๊ณตํ•˜๋Š” shouldNotFilter ๋ฉ”์†Œ๋“œ๋ฅผ ์˜ค๋ฒ„๋ผ์ด๋“œํ•˜์—ฌ, SecurityConfig์— ์ •์˜๋œ AUTH_WHITELIST ๊ฒฝ๋กœ์— ๋Œ€ํ•ด์„œ๋Š” JwtAuthenticationFilter๊ฐ€ ๋™์ž‘ํ•˜์ง€ ์•Š๋„๋ก ์ˆ˜์ •ํ–ˆ์Šต๋‹ˆ๋‹ค.

์ด๋ฅผ ํ†ตํ•ด ์ธ์ฆ์ด ํ•„์š” ์—†๋Š” ๊ฒฝ๋กœ๋Š” ํ† ํฐ ์œ ํšจ์„ฑ ๊ฒ€์‚ฌ๋ฅผ ์•ˆ์ „ํ•˜๊ฒŒ ํ†ต๊ณผํ•˜์—ฌ, ์˜๋„ํ•œ ๋Œ€๋กœ ์ปจํŠธ๋กค๋Ÿฌ์—์„œ ์š”์ฒญ์„ ์ฒ˜๋ฆฌํ•  ์ˆ˜ ์žˆ๋„๋ก ๊ฐœ์„ ํ–ˆ์Šต๋‹ˆ๋‹ค.

์ฃผ์š” ๋ณ€๊ฒฝ ์‚ฌํ•ญ

@jsoonworld jsoonworld self-assigned this Jun 21, 2025
@jsoonworld jsoonworld added ๐Ÿ”จย fix ๋ฒ„๊ทธ, ์˜ค๋ฅ˜ ์ˆ˜์ • ๐ŸฆŠ์žฅ์ˆœ๐ŸฆŠ labels Jun 21, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the JWT filter to skip token validation on configured whitelist endpoints.

  • Added shouldNotFilter override in JwtAuthenticationFilter to bypass authentication for AUTH_WHITELIST paths
  • Made AUTH_WHITELIST in SecurityConfig public for filter access
  • Imported and initialized AntPathMatcher to match request URIs against whitelist

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
JwtAuthenticationFilter.java Implemented shouldNotFilter using AntPathMatcher and SecurityConfig.AUTH_WHITELIST
SecurityConfig.java Changed AUTH_WHITELIST visibility to public static final
Comments suppressed due to low confidence (1)

src/main/java/org/terning/terningserver/common/security/jwt/filter/JwtAuthenticationFilter.java:37

  • Add unit tests for shouldNotFilter to verify that requests matching AUTH_WHITELIST are correctly skipped and that others are filtered.
    @Override


private final JwtUserIdExtractor jwtUserIdExtractor;
private final RateLimitingService rateLimitingService;
private final AntPathMatcher antPathMatcher = new AntPathMatcher();
Copy link

Copilot AI Jun 21, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider making the AntPathMatcher instance static final or injecting a shared PathMatcher bean to avoid unnecessary instantiation and improve clarity.

Suggested change
private final AntPathMatcher antPathMatcher = new AntPathMatcher();
private static final AntPathMatcher antPathMatcher = new AntPathMatcher();

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

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

ํ”ผ๋“œ๋ฐฑ ๊ฐ์‚ฌํ•ฉ๋‹ˆ๋‹ค!

AntPathMatcher๋Š” thread-safeํ•˜๊ณ  ์ƒํƒœ๊ฐ€ ์—†์œผ๋ฏ€๋กœ, ํ•„ํ„ฐ ์ธ์Šคํ„ด์Šค๋งˆ๋‹ค ์ƒˆ๋กœ ์ƒ์„ฑํ•˜๋Š” ๊ฒƒ์€ ๋‚ญ๋น„๊ฐ™๋„ค์š”!.
์ œ์•ˆํ•ด์ฃผ์‹  ๋Œ€๋กœ static final๋กœ ๋ณ€๊ฒฝํ•˜์—ฌ ํด๋ž˜์Šค ๋ ˆ๋ฒจ์˜ ๋‹จ์ผ ์ธ์Šคํ„ด์Šค๋ฅผ ์‚ฌ์šฉํ•˜๋„๋ก ์ˆ˜์ •ํ•˜๊ฒ ์Šต๋‹ˆ!

@Override
protected boolean shouldNotFilter(HttpServletRequest request) throws ServletException {
String requestURI = request.getRequestURI();
for (String pattern : SecurityConfig.AUTH_WHITELIST) {
Copy link

Copilot AI Jun 21, 2025

Choose a reason for hiding this comment

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

[nitpick] Referencing SecurityConfig.AUTH_WHITELIST statically couples the filter to the config class. Inject the whitelist via constructor or @value to improve testability and reduce tight coupling.

Copilot uses AI. Check for mistakes.
Copy link
Member Author

@jsoonworld jsoonworld Jun 21, 2025

Choose a reason for hiding this comment

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

์ด ๋ถ€๋ถ„์€ ํŠนํžˆ ์ค‘์š”ํ•œ ํ”ผ๋“œ๋ฐฑ์ด๋ผ๊ณ  ์ƒ๊ฐํ•ฉ๋‹ˆ๋‹ค.

ํ˜„์žฌ ํ•„ํ„ฐ๊ฐ€ ์„ค์ • ํด๋ž˜์Šค์— ์ง์ ‘ ์˜์กดํ•˜๊ณ  ์žˆ์–ด ํ…Œ์ŠคํŠธ๊ฐ€ ์–ด๋ ต๊ณ  ์œ ์—ฐ์„ฑ์ด ๋–จ์–ด์ง€๋Š” ๊ตฌ์กฐ๋„ค์š”!

๋ง์”€ํ•ด์ฃผ์‹  ๋Œ€๋กœ SecurityConfig์—์„œ AUTH_WHITELIST๋ฅผ JwtAuthenticationFilter์˜ ์ƒ์„ฑ์ž๋ฅผ ํ†ตํ•ด ์ฃผ์ž…ํ•˜๋Š” ๋ฐฉ์‹์œผ๋กœ ๋ฆฌํŒฉํ† ๋งํ•˜์—ฌ, ๋‘ ํด๋ž˜์Šค ๊ฐ„์˜ ๊ฒฐํ•ฉ๋„๋ฅผ ๋‚ฎ์ถ”๊ณ  ํ…Œ์ŠคํŠธ ์šฉ์ด์„ฑ์„ ๋†’์—ฌ๋ณผ๊ฒŒ์š”!

private final CustomJwtAuthenticationEntryPoint customJwtAuthenticationEntryPoint;
private static final String[] AUTH_WHITELIST = {

public static final String[] AUTH_WHITELIST = {
Copy link

Copilot AI Jun 21, 2025

Choose a reason for hiding this comment

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

Static mutable arrays can be modified at runtime. Consider using an unmodifiable List or an immutable collection to prevent accidental modifications.

Suggested change
public static final String[] AUTH_WHITELIST = {
public static final List<String> AUTH_WHITELIST = List.of(

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

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

๋งž์Šต๋‹ˆ๋‹ค!

public static final ๋ฐฐ์—ด์€ ๋Ÿฐํƒ€์ž„์— ๋‚ด๋ถ€ ๊ฐ’์ด ๋ณ€๊ฒฝ๋  ์ˆ˜ ์žˆ์–ด ์ž ์žฌ์ ์ธ ์œ„ํ—˜์ด ์กด์žฌํ•˜์ฃ !
์ œ์•ˆํ•ด์ฃผ์‹  ๋Œ€๋กœ List.of()๋ฅผ ์‚ฌ์šฉํ•˜์—ฌ ๋ˆ„๊ตฌ๋„ ์ˆ˜์ •ํ•  ์ˆ˜ ์—†๋Š” ๋ถˆ๋ณ€ ์ปฌ๋ ‰์…˜์œผ๋กœ ๋งŒ๋“ค์–ด ์ฝ”๋“œ์˜ ์•ˆ์ •์„ฑ์„ ๊ฐ•ํ™”ํ•ด๋ณด๊ฒ ์Šต๋‹ˆ๋‹ค!

@pull-request-size pull-request-size bot added size/M and removed size/S labels Jun 21, 2025
@jsoonworld jsoonworld merged commit 75f4c32 into develop Jun 21, 2025
1 check passed
@thingineeer thingineeer deleted the fix/#252 branch June 21, 2025 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

๐Ÿ”จย fix ๋ฒ„๊ทธ, ์˜ค๋ฅ˜ ์ˆ˜์ • size/M ๐ŸฆŠ์žฅ์ˆœ๐ŸฆŠ

Projects

None yet

2 participants