-
Notifications
You must be signed in to change notification settings - Fork 1
[Refactor] 내부 자동화를 위한 관리자용 로그인 API 추가 (장기 refresh 토큰 기반) #161
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
base: develop
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Test Results397 tests 397 ✅ 56s ⏱️ Results for commit ecf97e3. |
📊 JaCoCo Coverage
|
JangIkhwan
left a comment
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.
코멘트가 늦어서 죄송합니다
| @Mock | ||
| RedisRefreshTokenRepository redisRefreshTokenRepository; | ||
|
|
||
| @DisplayName("관리자 로그인 성공 시 access/refresh 토큰을 반환하고 refresh는 관리자 TTL로 Redis 저장한다") |
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.
"Redis 저장한다" 오타인 것 같아요...!
| String refreshToken = jwtUtil.createRefreshJwt(user.getId(), adminRefreshTtlMs); | ||
|
|
||
| // 관리자 계정만 TTL 1년으로 저장 | ||
| redisRefreshTokenRepository.save(user.getId(), refreshToken, adminRefreshTtlMs); |
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.
리프레시 토큰 TTL은 어느정도로 생각하시나요?
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.
현재는 PM님이 요청하신대로 1년으로 설정했습니다
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.
리프레시 토큰이 1년이어야 할 이유를 아직 잘 이해를 못하겠습니다... 리프레시 토큰을 API에서 내보낸다는 건 외부 서버가 리프레시 토큰 로테이션을 할 수 있다는 거 아닌가요? 근데 그러면 리프레시 토큰의 만료 기간이 1년이어야하나요...?
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.
음.. 저는 PM님의 요청이었던 'ADMIN'에게 유효기간을 1년 동안 준다' 라는 것에 초점을 맞추고, 1년간 무조건 재로그인 API 호출이 필요 없도록 구현하려고 했습니다.
근데 TTL을 더 짧게 잡고, 그 기간 내에 클라이언트측에서 한 번 이상 access 토큰 만료될 때마다 reissue 호출, 그때마다 refresh를 갱신해 저장하는게 보장돼있다면 더 짧게 해도 괜찮을 것 같아요. 어느 정도의 기간이 적당하다 생각하시나요 ?
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.
저는 길어봐야 3달이라고 생각하긴 해요. 근데 홍보용 서버가 어떤 상황인지는 모르겠네요..
| .orElseThrow(() -> new CustomException(USER_NOT_FOUND)); | ||
|
|
||
| String accessToken = jwtUtil.createAccessJwt(user.getId(), user.getRole()); | ||
| String refreshToken = jwtUtil.createRefreshJwt(user.getId(), adminRefreshTtlMs); |
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.
리프레시 토큰이 필요한가요?
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.
Access token을 길게(1년) 주면 만일 한 번 유출되는 경우 1년 동안 뚫린 상태가 되니, Access token은 기존대로 짧게(현재는 1시간), refresh로 자동 갱신을 해서 만일 유출되더라도 바로 끊을 수 있도록 해야한다고 생각했습니다. 또, 운영 자동화 측면에서도 필요하다 생각했습니다.
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.
그럼 accessToken 이 만료되면 관리자의 경우에도 refreshToken 을 통해서 reissue 를 해야하는건가요?
제가 알기로는 이 인스타 포스트 자동화 기능을 사용할 때는 일반적인 안드로이드 스튜디오를 활용하는 것이 아니라, 별도의 프로그램을 사용하는 것으로 알아서 PM 님과 이야기 해보아야할 것 같습니다. 아니면 혹시 이 부분까지 이미 토의가 된 것일까여?
추가적으로 만약에 ADMIN ROLE 이 추가된다고 가정한다면, 기존의 reissue API 의 경우 USER ROLE 을 담아 반환하다보니 추가적인 API 를 만들어서 ADMIN ROLE 을 담은 토큰을 재발급해주도록 하거나, 혹은 reissue API 를 수정해서 전달받은 refreshToken 의 ROLE 을 기반으로 분기 처리를 해야되지 않을까 생각됩니다
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.
상균이형 말대로 토큰 재발급에서 어떤 역할의 토큰을 발급하는지 분기 처리가 필요하겠네요. 리프레시에도 ROLE을 넣도록 JwtUtil을 수정해야할 수 있겠네요.
| User user = userRepository.findById(adminUserId) | ||
| .orElseThrow(() -> new CustomException(USER_NOT_FOUND)); | ||
|
|
||
| String accessToken = jwtUtil.createAccessJwt(user.getId(), user.getRole()); |
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.
엑세스 토큰은 어떤 권한을 갖고 있나요? 저희 자동화 목적에 맞는 권한만 갖고 있는 게 맞나요?
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.
음 지금은 ROLE을 USER로 갖게끔 되어 있고, 특정한 KEY로 로그인 요청을 보내다보니 상관 없지 않을까 생각했어요.
근데 아예 ROLE에 'ADMIN'이라는 걸 추가하고, 해당 ROLE에게는 특정 API만 호출 가능하도록 수정하는 것이 나을 것 같다는 생각이 듭니다.
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.
동의합니다
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.
저도 이 부분 동의합니다.
현재 구현은 사실상 그냥 kakaoLogin 과 다를 것이 없어보이고, 그냥 DB 에 관리자 유저를 하나 만들어두고 그 유저의 kakaoId 만 알려주면 지금 구현한 adminLogin 기능을 동일하게 대체할 수 있을 것 같아요.
그래서 언급하신 것처럼 ADMIN 이라는 ROLE 을 추가하고, 인스타 포스트 자동화 API 를 ADMIN ROLE 을 가진 유저만 접근할 수 있도록 하는 것이 더 적합한 구현인 것 같슴다
| ) | ||
| @PostMapping("/login/admin") | ||
| public BaseResponse<AdminLoginResponse> adminLogin( | ||
| @RequestHeader(value = "X-ADMIN-KEY", required = false) String adminKey |
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.
서버 간에 인증을 위해서 client credentials라는 방법을 사용하기도 하던데 이렇게 구현하신 이유가 있나요?
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.
외부 다수 클라이언트가 붙는 구조가 아니라 내부 자동화 주체 1개만 인증하면 되는 요구사항이라, 인증 인프라를 과도하게 키우기보다는 로그인 단계에서만 API Key를 사용하고, 이후에는 짧은 만료의 access token으로 요청을 처리해 보안 수준은 유지하면서 구현과 운영 복잡도를 낮추는 방식이 적절하다고 생각했습니다.
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.
보안 관점에서는 아쉽지만 그렇게 생각하신다면 알겠습니다
Related issue 🛠
Work Description 📝
Screenshot 📸
Uncompleted Tasks 😅
To Reviewers 📢