refactor: 스프링 시큐리티 코드 리팩터링 by nayonsoso · Pull Request #154 · solid-connection/solid-connect-server

nayonsoso · 2025-01-14T20:09:03Z

특이 사항

위키에 정리해둔 스프링 시큐리티 관련 내용 읽고 리뷰해주세요!
https://github.com/solid-connection/solid-connect-server/wiki/Spring-Security-동작-원리

👇 전체적인 흐름은 아래와 같습니다

작업 내용

복잡하게 설정되어있던 스프링 시큐리티 코드를 리팩터링했습니다.
유지보수성, 객체의 책임을 생각하며 리팩터링 했습니다.
주요 변경 내용은 다음과 같습니다.

1/ uri 를 기준으로한 '인증 필수 여부' 분기 제거
SecurityConfiguration 과 JwtAuthenticationFilter 에 있었던, 인증 없이 통과되게 하는 uri 를 없앴습니다. c33466b
uri에 버전이 적용되면 uri를 관리하기 까다로워질 것입니다.
uri 관리 포인트를 최소화하기 위해서 uri 하드코딩을 지양할 필요가 있습니다.
그리고 'SecurityConfiguration에서 어떤 uri에 인증이 필요한지, 불필요한지 알고 있는게 맞나?🤨'라는 생각이 들더라고요.
uri 관련 내용은 컨트롤러에 두는게 더 응집도 있겠다고 생각했습니다.

2/ 스프링 시큐리티 필터를 활용
사실 이전 코드는 스프링 시큐리티에 대해서 이해를 많이 못하고 레퍼런스를 참고하며 코드를 작성했습니다😥
이번에는 제대로 공부했고! 스프링 시큐리티의 핵심인 '필터'를 적절히 활용해봤습니다.
우리는 쿠키가 아니라 토큰 + 헤더를 이용해 인증을 구현하고 있기 때문에, 로그아웃했으면 그 정보를 서버에 저장해야 합니다.
그렇지 않으면 로그아웃한 사용자가 같은 토큰으로 회원 기능을 사용하려 할 때, 마치 로그아웃 안한 것처럼 인증이 통과됩니다😱
이전에는 JwtAuthenticationFilter와 TokenValidator에 그 내용이 분산되어있었는데,
이를 SignOutCheckFilter로 옮겼습니다. b795eb8

3/ 토큰 인증 필터에서 DB 접근하는 부분 제거
기존에는 CustomUserDetailsService에서 '이 토큰의 subject에 해당하는 사용자가 정말로 있나?' 를 확인하기 위해서
ServiceRepository 로 DB에 접근했습니다.
하지만 이는 토큰의 장점을 살리지 못하는 구성이었습니다😔
물론 '이 토큰의 subject에 해당하는 사용자가 정말로 있나?'는 반드시 필요한 검증입니다.
하지만 이 검증이 들어가야 하는 부분이 jwt 검증 필터가 되어선 안된다고 생각합니다.
따라서 이 PR에서는 CustomUserDetailsService를 삭제했습니다.
사용자 검증은 서비스 단에서 이루어지게 해야 할 것 같은데 이 PR 변경 내용이 너무 많아져서.. 다음 PR에서 해보겠습니다!

- TokenService > TokenProvider - 비지니스 로직이 아니라 기능만 제공하는 것이라 Provider가 더 적절하다고 판단함

- 기존에 작성되지 않았던 것들도 작성함

- AS-IS: 액세스 토큰을 검증하면서 로그아웃했는지를 검증하고 있다. 이는 액세스 토큰의 검증부에 들어갈 것이 아니라, 더 이전 단계에서 처리되어야 한다. - TO-BE: 로그아웃 토큰을 필터에서 처리한다. 이전보다 더 빠르게 예외를 응답할 수 있다.

- 가독성 개선 - 다른 객체들에 책임 분산 - permitAllEndpoint에 대한 설정 제거

- 가독성 향상 - 필터 추가 - 시큐리티 단에서 관리하는 인증 필요없는 uri 제거

Gyuhyeok99 · 2025-01-16T08:16:25Z

전체적으로 스프링 시큐리티의 구조와 필터 체인 활용이 명확해져서 훨씬 깔끔하고 유지보수성이 좋아진 것 같습니다. 몇 가지 생각해본 점과 제안 드리고 싶은 부분이 있어 공유드립니다! 😊
1️⃣ URI 기반 인증 분기 제거
제가 이해한 바로는, SecurityConfiguration과 JwtAuthenticationFilter에서 인증 분기 처리를 제거하고, 이를 컨트롤러에서 @PreAuthorize를 활용해 관리하시려는 의도로 보입니다. 혹시 제가 제대로 이해한 걸까요?

영서님께서 말씀하신 대로 SecurityConfig와 JwtAuthenticationFilter 양쪽에서 인증/인가를 중복 관리하는 것은 비효율적이라고 생각합니다. 다만, 모든 인증/인가 처리를 컨트롤러에서 담당하게 되면 컨트롤러의 책임이 과도해질 수 있을 것 같습니다.

그래서 저는 다음과 같은 방향은 어떨까 제안드립니다:

getPermitAllEndpoints() 제거(이건 영서님이랑 의견이 같습니다!)
→ URI 기반 인증 예외 처리를 없애고, 중복 관리 방지
SecurityConfig에서 인증 예외 통합 관리
→ 전역적인 URI 접근 제어는 SecurityConfig에서 통합 관리하고,
→ JwtAuthenticationFilter는 토큰 검증과 인증 처리만 담당
예외적인 인증/인가만 컨트롤러에서 세부적으로 처리
→ 특정 API에서 예외적으로 인증이 필요 없는 경우에만 @PreAuthorize로 관리
SecurityConfiguration 예시

.authorizeHttpRequests(auth -> auth
    // 버전 상관없이 인증 없이 접근 가능
    .requestMatchers("/api/v*/auth/**", "/api/v*/public/**").permitAll() // 이건 추후 리스트로 빼서 관리해도 좋을 거 같긴 합니다.
    // 나머지는 인증 필요
    .antMatchers("/api/v*/**").authenticated()
)

이렇게 하면 컨트롤러의 책임은 최소화하면서도, 보안 정책은 일관성 있게 유지할 수 있을 것 같은데 어떻게 생각하시는지 궁금합니다!
추가로 현재 Admin에 대한 Role이 존재하지 않는데 추후 Admin 계정을 통한 유저 관리나 커뮤니티 관리 같은 기능은 따로 필요 없는 걸까요?

2️⃣ 스프링 시큐리티 필터를 활용
SignOutCheckFilter → JwtAuthenticationFilter 순서로 실행되도록 설정하신 부분부터 해서 보안 측면에서 적절하게 잘 구성되어 있다고 생각합니다.

그런데 한 가지 궁금한 점이 있어서 질문드립니다!
현재 구현 방식이 다음과 같은 흐름으로 이해했는데, 제가 제대로 이해한 게 맞을까요?

첫 로그인 시 AccessToken과 RefreshToken이 발급되고,
RefreshToken은 Redis에 "refresh:user@example.com" 값으로 저장.
로그아웃 시
Redis의 "refresh:user@example.com" 값을 "signOut"으로 변경.
다시 로그인 시
새로 발급된 RefreshToken이 Redis의 "refresh:user@example.com" 값에 덮어쓰기됨.

이러면 SignOutCheckFilter는 RefreshToken의 상태만 확인하므로, 기존 AccessToken으로 다시 접근이 가능한 상황인지 궁금합니다.
만약 위와 같은 흐름이 맞다면, 로그아웃 시 "refresh:user@example.com" 값을 "signOut"으로 변경하는 대신,
사용 중인 AccessToken을 블랙리스트에 등록하는 방식이 더 보안적으로 적합하지 않을까 생각했습니다.
물론 엑세스 토큰 유효 시간이 1시간이니 큰 문제가 될 거 같지는 않습니다!

제가 잘못이해했다면 알려주시면 감사하겠습니다 😂

public void signOut(String accessToken, long expirationTime) {
    redisTemplate.opsForValue().set(
        "BLACKLIST:" + accessToken,
        SIGN_OUT_VALUE,
        expirationTime,
        TimeUnit.MILLISECONDS
    );
}

3️⃣ 토큰 인증 필터에서 DB 접근하는 부분 제거
JWT의 큰 장점은 서버가 상태를 유지하지 않아도 인증이 가능하다는 점인데, DB를 통해 매번 사용자를 확인하는 것은 비효율적인 게 맞는 거 같습니다!
저도 JWT 인증 필터는 토큰 검증만 담당하고, 실제 유저 상태 확인(탈퇴, 정지 여부 등)은 비즈니스 로직에서 처리하도록 분리하면 단일책임원칙이 잘 지켜질 거 같습니다! 고생하셨습니다!

nayonsoso · 2025-01-20T16:49:10Z

결론만 이야기하는 것보다 고민의 과정을 다 공유하는게 좋을 것 같아 좀 길게 적어봅니다.. ㅎㅎ;

1️⃣

이를 컨트롤러에서 @PreAuthorize 를 활용해 관리하시려는 의도로 보입니다. 혹시 제가 제대로 이해한 걸까요?

@PreAuthorize를 고려한 것은 아닙니다😓
제가 디스크립션에 모호하게 적은 부분 때문에 헷갈리신 것 같아요.
위에서 말한 “uri 관련 내용은 컨트롤러에 두는게 더 응집도 있겠다” 라는 것은,
uri 의 위치가 다른 곳에 분산되지 않고, 컨트롤러에만 있으면 좋겠다는 의미였습니다.
인증의 처리를 컨트롤러에서 하자는 뜻은 아니었습니다.

그리고 SecurityConfig에서 중앙 관리하는 것에 대해서.. 정말 생각을 많이 해봤는데요😵‍💫
SecurityConfig에서 하지 않을 이유가 더 많다고 생각해서, 그 이유를 정리해봤습니다.

첫째로, uri 와 보안이 함께 묶이는 것이 좋지 않다 생각합니다.
저는 보안은 비지니스 로직이고, uri 는 그 비지니스 로직을 실행하기 위한 정해진 규약이라 생각합니다.
예를 들어 [/my-page/profile 로 GET 요청을 하면, 로그인한 사용자인 경우에는 프로필을 내려준다] 라는 요구사항이 있다고 합시다.
제 관점에서의 핵심은 “인증된 사용자인 경우에는 프로필을 내려준다” 이고,
/my-page/profile GET 는 이를 실행하기 위한 프로토콜입니다.
따라서 이 둘을 SecurityConfig에서 함께 관리하는 것은 적절하지 않다고 생각합니다.
비지니스 로직과, 그게 상관 없는 것이 엮여있다는 생각이 들었습니다.
마치 컨트롤러에 서비스 코드가 위치하는 느낌이랄까요..

그리고 SecurityConfig에서 통합으로 관리하는 방법에 대해 회의적이었던 또 다른 이유가 있는데요,
보안에 대한 코드가 여러곳에 분산됨으로써 실수를 유발할 수 있기 때문입니다.

예를 들어 SecurityConfig에서 /my-page/** 패턴에 대해, authenticated() 로 설정했다 합시다.
마이페이지에 대해서는 대부분 인증이 필요하므로 매우 합리적인 설정처럼 보입니다.

그런데 시간이 흘러 흘러..
어떤 개발자가 /my-page/visit 라는 방문자 수를 나타내는 새로운 api 를 만들게 되었습니다.
이 api 는 인증이 되지 않은 사용자도 호출할 수 있으므로
그 개발자는 인증을 검증하는 코드를 작성하지 않았고, 성공적으로 구현을 마쳤습니다.
그러나!! 인증이 안된 사태에서 api 를 호출하니 403이 반환되는 상황을 마주하게 됩니다 😧😧

이 실수를 하지 않기 위해서는 SecurityConfig에서 /my-page/** 패턴이 authenticated()로 설정되어있으니,
이 패턴에 해당하는 uri 중에 허용이 필요한 곳은 별도로 permitAll 를 해줘야 한다는 사전 지식이 필요합니다.
이처럼 새로운 기능을 만들 때, 기존 코드에 대한 사전 지식이 필요한 것은 바람직하지 못한 상황입니다.
이는 응집도가 낮아서 발생한 문제라고 할 수 있습니다.
그래서 SecurityConfig에서 중앙 관리하는게 실수를 유발할 수 있다고 생각합니다.

보안은 횡단 관심사이기 때문에, SecurityConfig에서 중앙관리를 하면 이점이 있는 상황도 있다고 생각합니다.
api 별로 보안이 잘 나뉘어진 상황에서는 오히려 이 방식이 실수를 예방할 것입니다.
규혁님이 예시로 들어주신 "/api/v*/public/**" 와 같은 패턴의 api 가 많다면 말이죠.
관리자 페이지도 만들 계획이 있는데, 그때는 /admin/**에 대해서 hasRole(ADMIN) 을 하는것은 괜찮은 것 같습니다.
명백하게 예외가 없으니깐요.

하지만 지금 우리 api 들은 통일되지 않은 패턴을 갖는 경우가 더 많습니다.
예를 들어서 /auth/sign-out 는 인증 필요, /auth/kakao 는 인증 불필요
/university/{{university-id}}/like 는 인증 필요, /university/detail/{id} 는 인증 불필요
/university/recommends 는 인증이 있으면 그 정보를 활용, 인증이 없어도 예외를 발생시키지 않고 일반 추천 대학을 반환하는 등..
패턴으로 통일되어있지 않기 때문에, 중앙에서 설정하는게 큰 의미가 없을 것 같습니다.

그리고 횡단 관심사라면 차라리 컨트롤러의 ArgumentResolver 나, aop 로 어노테이션을 만들어서 사용하는게 적절치 않을까 생각합니다.
인증 처리와 관련된 코드는 한 곳에 있으면서, 개발자가 인증이 필요한 곳에서 어노테이션을 다는 방식이요.
물론 이것도 “개발자 개인이 어노테이션을 누락하는 실수를 할 수 있지 않나?” 생각할 수도 있지만,
이정도는 @RequestBody와 같은 기본 어노테이션 관리와 동일한 수준의 책임이라 생각합니다.

정리하자면,

보안도 비지니스 로직이니 config 파일에 두지 말자
SecurityConfig에서 통합 관리한다면, 예외 상황에 대해서 처리하기 위해 SecurityConfig에 대한 사전 지식이 필요하다.
(응집도 ↓, 실수 유발 ↑)
uri 가 권한별로 통일되어있다면 적용해도 된다 생각하지만, 우리 api 들은 통일되지 않은 부분들이 많다
보안 관련 코드를 관심사로 분리해서 ArgumentResolver or aop를 적용하는게 적합해보인다.

는 생각입니다.

nayonsoso · 2025-01-20T16:51:04Z

2️⃣
네 규혁님이 이해하신게 맞습니다.
사실 저도 로그아웃 처리 방식을 바꿀 필요가 있다 생각해서.. 남몰래 디스커션도 파뒀답니다 🤫
그런데 이 PR의 영역은 아닌 것 같아서 이번에 건들이진 않았어요.
언제 말씀드릴까 했었는데 이렇게 먼저 말씀해주셔서 감사해요.
저도 엑세스 토큰을 사용한 블랙리스트 방식이 더 좋아보입니다.
다음 PR에서 반영하겠습니다!

nayonsoso · 2025-01-20T19:00:42Z

추가 커밋으로

중복된 cors 설정 제거
TokenType 패키지 이동 : config.token → auth.domain
JwtProvider와 JwtValidator 패키지 이동 : config.token → auth.service
JwtProvider 안의 유틸성 코드를 JwtUtils 클래스로 분리

했습니다! 😊

Gyuhyeok99 · 2025-01-22T03:00:31Z

api 별로 보안이 잘 나뉘어진 상황에서는 오히려 이 방식이 실수를 예방할 것입니다.
규혁님이 예시로 들어주신 "/api/v*/public/**" 와 같은 패턴의 api 가 많다면 말이죠.

저는 그동안 이렇게만 생각해왔어서 이게 적절하다고 생각하고 있었는데 이 사고 과정에서는 현재 저희 코드가 어떻게 구현되어있는가에 대한 생각이 빠져있던 거 같습니다 😅

하지만 지금 우리 api 들은 통일되지 않은 패턴을 갖는 경우가 더 많습니다.
예를 들어서 /auth/sign-out 는 인증 필요, /auth/kakao 는 인증 불필요
/university/{{university-id}}/like 는 인증 필요, /university/detail/{id} 는 인증 불필요

이부분 읽으면서 다시 코드를 생각해보니 영서님이 말씀하신 대로 컨트롤러에서 구분하는 게 더 실수도 적을 거 같네요! 좋은 거 같습니다!

Gyuhyeok99

고생하셨습니다!

Gyuhyeok99 · 2025-01-22T05:42:41Z

src/main/java/com/example/solidconnection/auth/service/TokenProvider.java

+package com.example.solidconnection.auth.service;
+


TokenProvider는 Auth의 책임이니 적절하게 이동한 거 같습니다! 다만 앞으로 애플 등의 인증 방식이 추가될 수 있으므로, 확장성을 위해 인터페이스 도입을 고려하면 좋을 것 같습니다. 현재는 구조를 유지하면서, 새로운 인증 방식 추가 시점에 인터페이스화를 논의해도 좋을 거 같습니다!

Gyuhyeok99 · 2025-01-22T05:48:08Z

src/main/java/com/example/solidconnection/config/security/JwtAuthenticationFilter.java

    private static final String REISSUE_METHOD = "post";

-    private final TokenProvider tokenProvider;
+    private final JwtProperties jwtProperties;


JWT 관련 설정값들을 한 곳(JwtProperties)에서 관리하니 더 깔끔해진 거 같습니다! 추후 재사용하기도 편하고 확장시킬 때도 편하겠네요!

Gyuhyeok99 · 2025-01-22T05:54:53Z

src/main/java/com/example/solidconnection/util/JwtUtils.java

+public class JwtUtils {
+


JWT 관련 유틸리티 코드가 한곳에 깔끔하게 모였네요 👍 그런데 이 유틸 함수는 static으로만 사용하니 실수로 인스턴스화를 하는 것을 방지하기 위해 private 생성자를 추가하는 건 어떨까요?

너~~무 좋아요! 제가 놓치고 있었네요😓
d9c965b

Gyuhyeok99 · 2025-01-22T06:06:40Z

src/test/java/com/example/solidconnection/util/JwtUtilsTest.java

+        }
+
+        @Test
+        void 유효하지_않은_토큰의_subject_를_추출하면_예외가_발생한다() {


저희 ~면_예외_응답을_반환한다로 통일할까요?

짚어주셔서 감사합니다🙇🏻‍♀️
605f24a

Gyuhyeok99 · 2025-01-22T06:15:51Z

src/test/java/com/example/solidconnection/util/JwtUtilsTest.java

+import static org.junit.jupiter.api.Assertions.assertAll;
+
+@DisplayName("JwtUtils 테스트")
+class JwtUtilsTest {


테스트코드 가독성이 아주 좋네요 👍

nayonsoso added 12 commits January 14, 2025 22:19

refactor: 토큰 기능 제공 클래스 이름 변경

210ae52

- TokenService > TokenProvider - 비지니스 로직이 아니라 기능만 제공하는 것이라 Provider가 더 적절하다고 판단함

refactor: 토큰 접두사 추가 함수 이름 변경, static import 적용

ee49cfe

feat: subject 추출 함수 추가

7227179

test: TokenProvider 테스트 작성

e8632fc

- 기존에 작성되지 않았던 것들도 작성함

refactor: 예외 응답 함수 추출

feb828f

test: 로그아웃 체크 필터 테스트 작성

54141e1

refactor: 중복 코드 함수로 추출

e89dc76

refactor: JWT 인증 필터 수정

ff902d2

- 가독성 개선 - 다른 객체들에 책임 분산 - permitAllEndpoint에 대한 설정 제거

refactor: 사용하지 않는 코드,클래스 제거

d0e0929

test: JWT 인증 필터 테스트 작성

53299c9

refactor: 스프링 시큐리티 설정 클래스 수정

c33466b

- 가독성 향상 - 필터 추가 - 시큐리티 단에서 관리하는 인증 필요없는 uri 제거

nayonsoso added the 리팩터링 label Jan 14, 2025

nayonsoso self-assigned this Jan 14, 2025

nayonsoso added 6 commits January 21, 2025 03:41

refactor: 중복 선언된 cors 설정 제거

aaca84f

refactor: cors 관련 설정 ConfigurationProperties로 변경

1d37ff4

refactor: TokenType 패키지 이동

b607988

refactor: TokenProvider, TokenValidator 패키지 이동

995ab18

refactor: JwtProperties 분리

505b746

refactor: JwtUtils 분리

a4b0db4

refactor: ConfigurationPropertiesScan 적용

2019370

Gyuhyeok99 approved these changes Jan 22, 2025

View reviewed changes

nayonsoso added 2 commits January 23, 2025 02:07

refactor: 인스턴스화 방지

d9c965b

test: 테스트 메서드 이름에 컨벤션 적용

605f24a

nayonsoso merged commit f7dd92d into main Jan 22, 2025

nayonsoso deleted the refactor/153-spring-security branch February 15, 2025 19:16

		package com.example.solidconnection.auth.service;

Conversation

nayonsoso commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

관련 이슈

특이 사항

작업 내용

Uh oh!

Gyuhyeok99 commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nayonsoso commented Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nayonsoso commented Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nayonsoso commented Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gyuhyeok99 commented Jan 22, 2025

Uh oh!

Gyuhyeok99 left a comment

Choose a reason for hiding this comment

Uh oh!

Gyuhyeok99 Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

Gyuhyeok99 Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

Gyuhyeok99 Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

nayonsoso Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gyuhyeok99 Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

nayonsoso Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gyuhyeok99 Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nayonsoso commented Jan 14, 2025 •

edited

Loading

Gyuhyeok99 commented Jan 16, 2025 •

edited

Loading

nayonsoso commented Jan 20, 2025 •

edited

Loading

nayonsoso commented Jan 20, 2025 •

edited

Loading

nayonsoso commented Jan 20, 2025 •

edited

Loading

nayonsoso Jan 22, 2025 •

edited

Loading

nayonsoso Jan 22, 2025 •

edited

Loading