Skip to content

Comments

Refactor, Feat: 회원 관련 추가 기능 구현 및 OAuth2 소셜로그인 리팩토링#1

Open
ldhapple wants to merge 19 commits intocode-zero-to-one:mainfrom
ldhapple:refactor-dohyun
Open

Refactor, Feat: 회원 관련 추가 기능 구현 및 OAuth2 소셜로그인 리팩토링#1
ldhapple wants to merge 19 commits intocode-zero-to-one:mainfrom
ldhapple:refactor-dohyun

Conversation

@ldhapple
Copy link

📌 PR 제목

  • [refactor] 소셜로그인 Code Grant 방식 리팩토링
  • [feat] 회원 탈퇴 구현
  • [feat] 회원 정지 (상태 변경) 구현
  • [feat] 회원 목록 조회 구현

📋 작업 내용

기존 소셜로그인 구현의 문제점 (개인적인 생각)

  • Controller에 과도한 책임이 있다. URI 핸들링부터 쿠키 세팅 등등의 기능이 전부 Controller에 있어 가독성이 좋지 않다.
  • 쿠키 생성 로직과 같은 부분이 중복되어 있다.
  • Spring Security를 제대로 활용하지 못하고 있다. handler나 filter 구현으로 할 수 있는 부분들이 직접 구현되어 있다.
  • api 경로에 따른 권한 설정이 불필요하게 복잡하게 되어 있다.
  • 토큰을 API로 반환하거나 쿼리스트링으로 전달하고 있는데, 이는 토큰이 자바스크립트 영역에 노출되어 보안상 좋지 않다.
  • OpaqueToken을 활용해 매번 유저 정보를 DB에서 조회해 가져오는데 이는 비효율적이다.
  • 테스트가 어렵다.
  • 깃허브 로그인, 네이버 로그인과 같은 새로운 소셜을 붙일 때 불필요한 작업량이 많을 수 있는 구조인 것 같다.

--> spring-boot-starter-oauth2-client를 활용해 리팩토링, JWT 토큰을 활용한 유저정보 조회 및 JwtFilter 도입
--> 쿠키 기반 토큰 전달

✅ 체크리스트

  • 테스트를 완료했나요?
  • 코드 컨벤션을 지켰나요?
  • 관련 문서를 업데이트 했나요?

🚨 주의사항

  • 소셜 로그인 후 가져오는 프로필 이미지는 따로 MemberProfile에 저장하지 않음.
  • OAuth2 시크릿 키는 해당 브랜치에는 푸시하지 않음.

Copy link
Collaborator

@rudeh1253 rudeh1253 left a comment

Choose a reason for hiding this comment

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

좋은 코드 감사합니다! 제가 사실 OAuth2 Client 라이브러리를 사용해 보지 않았습니다. 이거 공부한다고 생각만 하고 있었는데, 계속 다른 거 하느라 밀렸네요... 그래서 OAuth2 Client 관련된 부분은 사실 제가 잘 모르겠습니다... ㅎㅎ; 그래서 OAuth 2 Client 라이브러리 외의 부분에 대해서 리뷰 남겼습니다!
제가 실력이 부족하여 리뷰를 남긴 부분이 잘 납득이 되지 않으실 수 있습니다. 그래도 너그러운 마음으로 봐 주시면 감사하겠습니다! 혹은 리뷰에 답변을 달아 반박해 주신다면 제 성장에 도움이 되어 감사하겠습니다!
코드를 읽으면서 많은 공부 되었습니다 다시 한 번 감사드립니다 :)

}

Long memberId = jwtUtil.getMemberId(token);
String role = jwtUtil.getRole(token);
Copy link
Collaborator

Choose a reason for hiding this comment

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

JWT 토큰을 한 번 파싱한 결과를 객체에 담고 해당 객체를 재사용하는 방법과, JWT 토큰 자체를 재사용하는 방법 중 어떤 게 더 나을까요?
memberId, role 등 JWT에 담긴 데이터가 필요할 때마다 JWT 토큰을 파싱하여 Claim을 꺼내온다면 BASE64 디코딩, JWT 검증, 파싱 과정을 여러 번 거치게 되어 오버헤드가 발생할 수도 있을 것 같은데, 이 오버헤드는 무시할 만큼 작다고 볼 수 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

파싱한 결과를 언제, 어떤 객체에 담아서 그걸 재활용하는 방식인지 알아야될 것 같습니다.
만약 객체에 저장하는 구조를 가져간다고 가정하면 첫 로그인 시점에 유저정보를 파싱하고 저장해야할 것 같은데, 객체에 저장하는 구조가 사실 불가능하다고 생각합니다. JWT 토큰의 사용 이유 중 하나가 Stateless를 유지하기 위함이기 때문에 객체에 저장해서 사용할 수 있더라도 피해야하는 방식이라고 생각합니다.
정보를 저장해서 활용하는 용도라면 Redis 정도를 활용해볼 수 있을 것 같은데 그 또한 오버헤드가 있고 JWT 토큰을 매 요청마다 파싱해서 사용하는 것 자체가 큰 오버헤드가 없는 것으로 알고 있어 현재 방식도 괜찮다고 생각합니다.

생각해보지 못한 부분이었는데 새로운 관점으로 볼 수 있어서 좋았습니다!

Copy link
Collaborator

@rudeh1253 rudeh1253 Jul 28, 2025

Choose a reason for hiding this comment

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

아 제가 말씀드린 재사용은 파싱한 결과를 저장소에 저장하는 게 아니라, 그냥 JWT 파싱한 결과를 객체에 담아서 반환하고, 이 메소드 안에서 재사용하는 것을 말씀드린 겁니다! 예를 들어, ParseResult parse(String token)라는 메소드가 있다고 한다면,

ParseResult parseResult = jwtUtil.parse(token);

Long memberId = parseResult.memberId();
String role = parseResult.role();

과 같은 형태로요! 이렇게 한다면 JWT 파싱 횟수를 줄일 수 있을 겁니다.
물론 구현해 주신 방식대로

Long memberId = jwtUtil.getMemberId(token);
String role = jwtUtil.getRole(token);

로 사용해도 성능상 크게 차이는 없을 것 같긴 합니다!
답변 감사합니다 :)

public Boolean isExpired(String token) {

return Jwts.parser().verifyWith(secretKey).build().parseSignedClaims(token).getPayload().getExpiration()
.before(new Date());
Copy link
Collaborator

@rudeh1253 rudeh1253 Jul 28, 2025

Choose a reason for hiding this comment

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

만약 JWT 토큰이 만료되었다면, JWT 토큰을 파싱하는 시도 자체가 예외를 발생시킬 수 있습니다.

실험 코드입니다.

import io.jsonwebtoken.Jwts;
import org.junit.jupiter.api.Test;

import javax.crypto.SecretKey;
import javax.crypto.spec.SecretKeySpec;
import java.nio.charset.StandardCharsets;
import java.util.Date;

public class Temp {

    @Test
    void test() throws InterruptedException {
        SecretKey secretKey = new SecretKeySpec("vjldksjfoiejfoaiejflskfjlsdkfjsoeifjdfkdfjlekdfjdkfjei".getBytes(StandardCharsets.UTF_8),
                Jwts.SIG.HS256.key().build().getAlgorithm());

        String jwt = Jwts.builder()
                .issuedAt(new Date(System.currentTimeMillis()))
                .expiration(new Date(System.currentTimeMillis() + 100L))
                .signWith(secretKey)
                .compact();

        Thread.sleep(1000L);

        boolean before = Jwts.parser().verifyWith(secretKey).build().parseSignedClaims(jwt).getPayload().getExpiration()
                .before(new Date());
        System.out.println(before);
    }
}

위 코드를 실행시키니 ExpiredJwtException이 발생했습니다.

JWT expired 1845 milliseconds ago at 2025-07-28T11:00:11.000Z. Current time: 2025-07-28T11:00:12.845Z. Allowed clock skew: 0 milliseconds.
io.jsonwebtoken.ExpiredJwtException: JWT expired 1845 milliseconds ago at 2025-07-28T11:00:11.000Z. Current time: 2025-07-28T11:00:12.845Z. Allowed clock skew: 0 milliseconds.
	at io.jsonwebtoken.impl.DefaultJwtParser.parse(DefaultJwtParser.java:682)
	at io.jsonwebtoken.impl.DefaultJwtParser.parse(DefaultJwtParser.java:362)
	at io.jsonwebtoken.impl.DefaultJwtParser.parse(DefaultJwtParser.java:94)
	at io.jsonwebtoken.impl.io.AbstractParser.parse(AbstractParser.java:36)
	at io.jsonwebtoken.impl.io.AbstractParser.parse(AbstractParser.java:29)
	at io.jsonwebtoken.impl.DefaultJwtParser.parseSignedClaims(DefaultJwtParser.java:821)
	at com.codezerotoone.mvp.Temp.test(Temp.java:26)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)

Copy link
Author

Choose a reason for hiding this comment

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

사실 이 테스트 자체에서 만료된 토큰을 파싱시도했을 때 예외가 터지는 것은 문제가 없다고 생각은하지만, 반환형이 Boolean인데 false가 아닌 만료 예외를 터뜨리고 있어 설계 의도에는 부합하지 않다고 생각합니다.

그래서 try-catch문으로 예외를 잡아 Boolean으로 명확하게 반환하는 형태를 가지는게 좋을 것 같습니다.
좋은 의견 감사합니다 :)

@Override
public void handle(HttpServletRequest request, HttpServletResponse response,
AccessDeniedException accessDeniedException) throws IOException, ServletException {
response.setStatus(HttpServletResponse.SC_FORBIDDEN);
Copy link
Collaborator

Choose a reason for hiding this comment

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

공통 에러 응답을 여기서 반환하면 403 에러가 왜 발생했는지 클라이언트에서 구체적으로 알 수 있을 거라고 생각합니다

return;
}

response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
Copy link
Collaborator

Choose a reason for hiding this comment

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

공통 에러 응답을 여기서 반환하면 401 에러가 왜 발생했는지 클라이언트에서 구체적으로 알 수 있을 거라고 생각합니다. 그에 따라 클라이언트에서도 상황에 맞게 대처할 수 있을 거고요

if (this.deletedAt == null) {
this.memberStatus = MemberStatus.QUIT;
this.deletedAt = LocalDateTime.now();
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

deletedAt에 대해 null 체크를 안 하면 이전에 삭제된 회원의 deletedAt 값이 의도와 맞지 않게 업데이트된다는 것을 제가 놓치고 있었네요
감사합니다

m.getCreatedAt(),
memberProfileData.getTel(),
memberProfileData.getBirthDate().toString(),
preferredStudySubject.getStudySubjectName()
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 부분에서 N+1 문제가 발생할 것 같은데 맞을까요?
N+1 문제가 발생한다면 fetch join을 사용하여 문제를 해결할 수 있을 것 같습니다

Copy link
Author

Choose a reason for hiding this comment

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

맞습니다! OneToOne, ManyToOne 구조밖에 없어서 페이징이어도 Fetch Join으로 간단히 해결할 수 있을 것 같습니다.

return ResponseCookie.from(key, value)
.path("/")
.maxAge(60 * 60 * 24 * 30)
.httpOnly(true)
Copy link
Collaborator

@rudeh1253 rudeh1253 Jul 28, 2025

Choose a reason for hiding this comment

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

쿠키를 생성할 때 HTTP-only가 아닌 쿠키도 생성하는 경우도 있을 거라고 생각합니다

Copy link
Author

Choose a reason for hiding this comment

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

그런 경우가 필요하다면 필요에 알맞은 새로운 메서드를 만들면 될 것 같습니다!
이 메서드는 이름을 변경하는게 좋을 것 같네요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

boolean 타입 파라미터를 추가하는 대신, HTTP-only 쿠키를 반환하는 메소드와 HTTP-only가 아닌 메소드를 따로 만들자는 말씀이시군요
그렇게 한다면 메소드의 응집도가 더욱 커질 수 있을 것 같습니다. 의견 감사합니다!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants