Feat/#14/6th assignment - #15
Conversation
5eoyng
left a comment
There was a problem hiding this comment.
심화과제까지 하시느라 고생 많으셨어요! 주석도 꼼꼼하게 달아주신거 같아요!!👍👍 남은 세미나도 화이팅!
| // 카카오 로그인 화면으로 이동 | ||
| String kakaoLoginUrl = KAKAO_AUTHORIZE_URI | ||
| + "?response_type=" + RESPONSE_TYPE_CODE | ||
| + "&client_id=" + kakaoRestApiKey | ||
| + "&redirect_uri=" + kakaoRedirectUri; |
There was a problem hiding this comment.
URL 문자열을 +로 조합하고 있는데 나중에 쿼리 파라미터가 늘어나거나 URL 포맷이 바뀌면 가독성이 떨어지고, 혹시 공백이나 특수문자가 들어갔을 때 URL 인코딩 문제가 발생할 수 있어 UriComponentsBuilder을 사용하는 방법도 좋을거 같습니다..!
| private TokenPair generateTokenPair(User user) { | ||
| String accessToken = jwtService.generateAccessToken(user.getId(), user.getEmail()); | ||
| String refreshToken = jwtService.generateRefreshToken(user.getId()); | ||
| return new TokenPair(accessToken, refreshToken); | ||
| } | ||
|
|
||
| // Access Token과 Refresh Token을 묶어서 전달 | ||
| private record TokenPair( | ||
| String accessToken, | ||
| String refreshToken | ||
| ){ | ||
| } |
There was a problem hiding this comment.
Access token과 Refresh token을 같이 발급하고 있는데, 이걸 내부 레코드로 묶어서 login, kakaoLogin, reissue 모두 generateTokenPair()로 처리하기 때문에 중복없이 깔끔해서 좋은거 같아요!
| .requestMatchers(HttpMethod.POST, "/api/v1/auth/signup").permitAll() | ||
| .requestMatchers(HttpMethod.POST, "/api/v1/auth/login").permitAll() | ||
| .requestMatchers(HttpMethod.POST, "/api/v1/auth/reissue").permitAll() | ||
| .requestMatchers(HttpMethod.GET, "/api/v1/oauth2/kakao").permitAll() | ||
| .requestMatchers(HttpMethod.GET, "/api/v1/oauth2/kakao/callback").permitAll() | ||
|
|
||
| // 로그아웃 API는 Access Token이 있는 사용자만 접근 가능 | ||
| .requestMatchers(HttpMethod.POST, "/api/v1/auth/logout").authenticated() | ||
|
|
||
| // 게시글 조회 API는 누구나 접근 가능 | ||
| .requestMatchers(HttpMethod.GET, "/api/v1/posts").permitAll() | ||
| .requestMatchers(HttpMethod.GET, "/api/v1/posts/*").permitAll() | ||
|
|
||
| // 게시글 작성, 수정, 삭제 API는 인증이 필요 | ||
| .requestMatchers(HttpMethod.POST, "/api/v1/posts").authenticated() | ||
| .requestMatchers(HttpMethod.PUT, "/api/v1/posts/*").authenticated() | ||
| .requestMatchers(HttpMethod.DELETE, "/api/v1/posts/*").authenticated() | ||
|
|
||
| // 좋아요 추가, 취소 API는 인증이 필요 | ||
| .requestMatchers(HttpMethod.POST, "/api/v1/posts/*/likes").authenticated() | ||
| .requestMatchers(HttpMethod.DELETE, "/api/v1/posts/*/likes").authenticated() | ||
|
|
||
| // Swagger 문서는 인증 없이 접근 가능 | ||
| .requestMatchers( | ||
| "/swagger-ui/**", | ||
| "/v3/api-docs/**", | ||
| "/swagger-ui.html" | ||
| ).permitAll() |
There was a problem hiding this comment.
인증이 필요하지 않은 요청을 처리하기 위해 경로뿐만 아니라 요청 메서드까지 함께 관리하는 방법은 좋은 것 같아요.
다만 가독성과 편의성을 위해 인증 없이 처리하려는 요청들에 대한 정보를 별도의 enum으로 관리하도록 분리하는 방법을 고려해보시면 좋을 것 같습니다!
| // 그 외 요청 우선 허용 | ||
| .anyRequest().permitAll() |
There was a problem hiding this comment.
현재 RequestMatcher를 기반으로 경로와 메서드를 통해 인증 정보가 없는 상황에서의 접근을 허용했지만 마지막에 모든 경로의 접근을 허용하여 접근 제한이 필요한 경우에도 접근이 가능하도록 되어있는 것 같아요.
접근 제한을 설정하여 인증이 필요한 요청의 접근을 제한하는 방식을 고려해보시면 좋을 것 같습니다!
| session.sessionCreationPolicy(SessionCreationPolicy.STATELESS) | ||
| ) | ||
|
|
||
| .authorizeHttpRequests(auth -> auth |
There was a problem hiding this comment.
현재 authorizeHttpRequests에서 모든 요청을 허용하고 있지만 인증에 따른 요청을 제한하게 된다면 예외가 발생합니다. 이 부분에서 예외가 발생하는 경우 Security에서 제공하는 기본 예외 응답이 반환되어 응답의 일관성을 해치게 될 수 있습니다.
| public String generateAccessToken(Long userId, String email) { | ||
| Instant now = Instant.now(); | ||
| return JWT.create() | ||
| .withSubject(String.valueOf(userId)) | ||
| .withClaim("email", email) | ||
| .withIssuedAt(Date.from(now)) | ||
| .withExpiresAt(Date.from(now.plusSeconds(accessTokenExpiresInSeconds))) | ||
| .sign(algorithm); | ||
| } | ||
|
|
||
| // Refresh Token 생성 | ||
| public String generateRefreshToken(Long userId) { | ||
| Instant now = Instant.now(); | ||
| return JWT.create() | ||
| .withSubject(String.valueOf(userId)) | ||
| .withIssuedAt(Date.from(now)) | ||
| .withExpiresAt(Date.from(now.plusSeconds(refreshTokenExpiresInSeconds))) | ||
| .sign(algorithm); | ||
| } |
There was a problem hiding this comment.
현재 jwt의 발급 과정에서 코드의 중복이 많이 발생하고 있는 것 같습니다.
jwt의 발급 로직을 하나의 공통 메서드로 분리하고 파라미터를 통해 토큰의 종류에 따른 발급을 진행하는 방법에 대해 고민해보시면 좋을 것 같습니다!
There was a problem hiding this comment.
현재 JwtService가 jwt의 발급, 검증, 파싱 등의 책임을 가지고 있는 것 같습니다. jwt 자체는 서비스 내에서 도메인에 해당되지 않기 때문에, service 계층에서 인증 과정에 필요한 모든 메서드를 관리하기 보다는 필요한 역할 별로 나누어 관리하는 방법에 대해 고려해보시면 좋을 것 같습니다!
| // 요청 정보를 인증 객체에 함께 담음 | ||
| authentication.setDetails( | ||
| new WebAuthenticationDetailsSource().buildDetails(request) | ||
| ); |
There was a problem hiding this comment.
인증 정보를 생성하는 과정에서 details까지 설정해주는 것은 굉장히 좋은 방법인 것 같아요! 다만 추후에 details의 활용성을 높이기 위해 컨트롤러에서 AuthenticationPrincipal로 사용자의 식별자만 전달받기 보다는 Authentication객체를 전달받도록 하는 방법도 함께 고려해보시면 좋을 것 같습니다!
| if (!hasBearerToken(authorizationHeader)) { | ||
| filterChain.doFilter(request, response); | ||
| return; | ||
| } |
There was a problem hiding this comment.
토큰 유무에 대한 분기처리를 외부로 넘기고 Filter에서는 인증 객체 생성만을 담당하도록 책임을 분리한 것은 좋은 것 같아요. 다만 SecurityConfig에서 RequestMatcher를 통한 접근제어에 따른 예외에 대한 처리를 고민해보시면 좋을 것 같습니다!
There was a problem hiding this comment.
ResponseBody로 토큰을 반환하는 방법도 좋지만 브라우저와 통신하는 상황에서 보안성을 높이기 위해 헤더나 쿠키를 이용하여 토큰을 전달하는 방법에 대해서도 생각해보시면 좋을 것 같아요!
| // 이메일로 로그인할 유저 조회 | ||
| User user = userRepository.findByEmail(request.email()) | ||
| .orElseThrow(() -> new BusinessException(ErrorCode.USER_NOT_FOUND)); | ||
|
|
||
| // 요청으로 받은 비밀번호가 맞는지 확인 | ||
| if(!passwordEncoder.matches(request.password(), user.getPassword())){ | ||
| throw new BusinessException(ErrorCode.INVALID_PASSWORD); | ||
| } | ||
|
|
||
| // 로그인에 성공하면 access token과 refresh token을 새로 발급 | ||
| TokenPair tokenPair = generateTokenPair(user); | ||
|
|
||
| // 기존 refresh token 삭제 | ||
| refreshTokenRepository.deleteByUserId(user.getId()); | ||
|
|
||
| // 새 refresh token을 DB에 저장 | ||
| RefreshToken savedRefreshToken = RefreshToken.of( | ||
| user.getId(), | ||
| tokenPair.refreshToken(), | ||
| jwtService.getRefreshTokenExpiresInSeconds() | ||
| ); | ||
|
|
||
| refreshTokenRepository.save(savedRefreshToken); | ||
|
|
||
| return new LoginResponse(tokenPair.accessToken(), tokenPair.refreshToken()); |
There was a problem hiding this comment.
현재 JwtAuthenticationFilter에서 BEARER_PREFIX를 이용한 검증과 문자열 슬라이싱을 사용하고 있지만 토큰의 발급 과정에서는 prefix를 누락한 상태로 전달하고 있는 것 같습니다.
클라이언트의 요청으로부터 BEARER_PREFIX의 누락으로 인한 오류를 방지하는 방법에 대해 고민해보시면 좋을 것 같습니다!
Kyoung-M1N
left a comment
There was a problem hiding this comment.
심화과제까지 정말 열심히 해주신 것 같아요!
고생하셨습니다!👍
Jy000n
left a comment
There was a problem hiding this comment.
과제 고생 많으셨습니다 :)
주석까지 깔끔하게 작성해주셔서 코드를 더 쉽게 잘 이해할 수 있었던 것 같습니다..!! 많이 배울 수 있는 코드였습니다 😊
There was a problem hiding this comment.
이건 그냥 코드들을 보다가 궁금했던건데 전체적으로 롬복을 안 쓰셨는데 이유가 따로 있으신 걸까욥
| @PathVariable("postId") Long postId, | ||
| @Parameter(description = "좋아요를 누르는 유저 ID", example = "1", required = true) | ||
| @RequestParam("userId") Long userId | ||
| @AuthenticationPrincipal Long userId |
There was a problem hiding this comment.
저는 Authentication authentication을 사용했는데 @AuthenticationPrincipal을 사용하면 principal 값을 바로 주입받는 방법도 있군요-!! 이 방법으로 userId만 바로 사용하는 경우에는 authentication.getName() 같이 직접 꺼내지 않아도 돼서 가독성 측면에서도 좋은 것 같아요🙂
|
|
||
| security: | ||
| jwt: | ||
| secret: seopsopt-fighting |
There was a problem hiding this comment.
헛 이거 이렇게 올라와도 괜찮은 건가요?
과제라 괜찮긴 하겠지만 ${JWT_SECRET} 같이 환경변수로 분리하는 게 좋을 것 같습니다!
| // 이메일로 로그인할 유저 조회 | ||
| User user = userRepository.findByEmail(request.email()) | ||
| .orElseThrow(() -> new BusinessException(ErrorCode.USER_NOT_FOUND)); | ||
|
|
||
| // 요청으로 받은 비밀번호가 맞는지 확인 | ||
| if(!passwordEncoder.matches(request.password(), user.getPassword())){ | ||
| throw new BusinessException(ErrorCode.INVALID_PASSWORD); | ||
| } |
There was a problem hiding this comment.
이메일이 없는 경우와 비밀번호가 틀린 경우를 다른 에러로 던지면 공격자가 해당 이메일의 가입 여부를 알 수 있어요.
두 경우 모두 동일한 에러 메시지로 통일하는 게 보안상 안전합니다!
|
|
||
| import java.time.LocalDateTime; | ||
|
|
||
| // redis로 확장할 수 있게 interface 적용 |
There was a problem hiding this comment.
나중에 Redis로 교체할 때 RedisBlacklistTokenStore 구현체만 새로 만들어서 교체하면 되니까 기존 코드 수정 없이 확장이 가능한 구조라 좋은 것 같습니다! 저는 고려해보지 못한 부분이라 배워가요!
| // 기존 refresh token 삭제 | ||
| refreshTokenRepository.deleteByUserId(user.getId()); | ||
|
|
||
| // 새 refresh token을 DB에 저장 | ||
| RefreshToken savedRefreshToken = RefreshToken.of( | ||
| user.getId(), | ||
| tokenPair.refreshToken(), | ||
| jwtService.getRefreshTokenExpiresInSeconds() | ||
| ); | ||
|
|
||
| refreshTokenRepository.save(savedRefreshToken); |
There was a problem hiding this comment.
요 로직 아래 kakaoLogin 함수에도 똑같이 있던데 따로 메서드로 추출해도 좋을 것 같아요!
🔥Pull requests
👷 과제 구현
필수과제
선택과제
구현한 내용에 대해서 설명해주세요
구현하며 고민했던 내용을 적어주세요 (사소한 것도 좋아요)
Access Token 블랙리스트를 DB로 관리할지 Redis 같은 캐시로 관리할지 고민했습니다.
실제 서비스에서는 TTL 관리가 쉬운 Redis가 더 적합하다고 생각하지만, 현재 프로젝트에서는 DB 기반으로 먼저 구현하고, 추후 Redis로 교체할 수 있도록 'BlacklistTokenStore' 인터페이스로 분리했습니다.
키워드 과제 정리내용
https://github.com/38-lets-sopt-server/yongmin.lee/blob/feat/%2314/6th_hw/keywords/hw_6.md
🚨 참고 사항