Feat: #228 유저 인증을 위한 MSW 수정 및 페이지 라우트 인증 처리 #229
Conversation
Yoonyesol
changed the title
Walkthrough이 Pull Request는 사용자 인증을 위한 API 핸들러에서 액세스 토큰으로부터 사용자 ID를 추출하는 과정을 간소화하고, 로그인 후 및 로그인 전 라우트 컴포넌트에서 동적 인증 상태를 활용하도록 변경되었습니다. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
src/routes/BeforeLoginRoute.tsx (2)
6-6: 좋습니다: useStore 사용으로 동적 인증 상태 구현
useStore훅을 사용하여isAuthenticated상태를 가져오는 것은 좋은 변경사항입니다. 이는 인증 상태를 동적으로 관리할 수 있게 해주며, 라우트 인증 구현이라는 PR 목표에 부합합니다.타입 안전성을 위해 다음과 같이 구조 분해 할당에 타입을 명시적으로 지정하는 것을 고려해보세요:
const { isAuthenticated }: { isAuthenticated: boolean } = useStore();
Line range hint
8-10: 잘했습니다: 인증 상태에 따른 조건부 렌더링 구현인증된 사용자를 루트 경로로 리다이렉트하는 조건문 추가는 BeforeLoginRoute에 대한 인증 검사를 잘 구현했습니다. 이는 인증된 사용자가 접근해서는 안 되는 페이지에 접근하지 못하도록 보장합니다.
코드의 가독성을 높이기 위해 다음과 같이 early return 패턴을 사용하는 것을 고려해보세요:
if (isAuthenticated) { return <Navigate to="/" replace />; } return children || <Outlet />;이렇게 하면 조건문의 구조가 더 명확해지고 코드의 흐름을 이해하기 쉬워집니다.
src/mocks/services/userServiceHandler.ts (1)
54-54: 사용자 ID 추출 로직의 일관성 있는 적용링크 변경 API에서도 동일한 방식으로 사용자 ID 추출 로직이 개선되었습니다. 이는 코드베이스 전반에 걸쳐 일관성을 유지하는 좋은 접근 방식입니다.
추가적인 개선사항으로, 이 로직이 여러 곳에서 반복되는 것으로 보입니다. 향후에는 이 부분을 별도의 함수로 추출하여 재사용성을 높이는 것을 고려해 보시기 바랍니다. 예를 들어:
function getUserIdFromToken(request: Request): string | null { const accessToken = request.headers.get('Authorization'); if (!accessToken) return null; return convertTokenToUserId(accessToken); }이렇게 하면 각 API 핸들러에서 이 함수를 호출하여 코드 중복을 줄이고 유지보수성을 향상시킬 수 있습니다.
src/mocks/services/authServiceHandler.ts (4)
231-234: 액세스 토큰 갱신 로직이 개선되었습니다.토큰 갱신 프로세스가 간소화되어 더 명확하고 유지보수가 쉬워졌습니다. 더미 JWT 토큰 체크를 제거한 것은 PR 목표와 일치합니다.
오류 처리를 개선하기 위해 다음과 같이 수정하는 것을 고려해보세요:
- if (!userId) return new HttpResponse(null, { status: 401 }); + if (!userId) return HttpResponse.json({ message: '유효하지 않은 액세스 토큰입니다.' }, { status: 401 });이렇게 하면 클라이언트에 더 명확한 오류 메시지를 제공할 수 있습니다.
254-255: 사용자 정보 조회 로직이 개선되었습니다.사용자 ID 추출 프로세스가 간소화되어 더 일관성 있고 유지보수가 쉬워졌습니다. 더미 JWT 토큰 체크를 제거한 것은 PR 목표와 일치합니다.
오류 처리를 개선하기 위해 다음과 같이 수정하는 것을 고려해보세요:
- if (!userId) return new HttpResponse(null, { status: 401 }); + if (!userId) return HttpResponse.json({ message: '유효하지 않은 액세스 토큰입니다.' }, { status: 401 });이렇게 하면 클라이언트에 더 명확한 오류 메시지를 제공할 수 있습니다.
372-373: 비밀번호 변경 로직이 개선되었습니다.사용자 ID 추출 프로세스가 간소화되어 더 일관성 있고 유지보수가 쉬워졌습니다. 더미 JWT 토큰 체크를 제거한 것은 PR 목표와 일치합니다.
오류 처리를 개선하기 위해 다음과 같이 수정하는 것을 고려해보세요:
- if (!userId) return new HttpResponse(null, { status: 401 }); + if (!userId) return HttpResponse.json({ message: '유효하지 않은 액세스 토큰입니다.' }, { status: 401 });이렇게 하면 클라이언트에 더 명확한 오류 메시지를 제공할 수 있습니다.
Line range hint
1-385: 전반적인 인증 서비스 핸들러 개선이 파일의 변경사항들은 일관성 있게 토큰 처리 프로세스를 단순화하고 더미 토큰 체크를 제거하여 PR의 목표를 잘 달성했습니다. 이러한 변경으로 코드의 가독성과 유지보수성이 크게 향상되었습니다.
향후 개선사항으로 다음을 고려해보세요:
- 오류 메시지를 중앙에서 관리하는 상수 파일을 만들어 일관성을 유지하고 관리를 용이하게 할 수 있습니다.
- 토큰 처리와 관련된 공통 로직을 별도의 유틸리티 함수로 추출하여 코드 중복을 더욱 줄일 수 있습니다.
- 각 API 핸들러에 대한 단위 테스트를 작성하여 변경사항의 안정성을 보장할 수 있습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/mocks/services/authServiceHandler.ts (3 hunks)
- src/mocks/services/userServiceHandler.ts (2 hunks)
- src/routes/AfterLoginRoute.tsx (1 hunks)
- src/routes/BeforeLoginRoute.tsx (1 hunks)
🧰 Additional context used
🔇 Additional comments (6)
src/routes/BeforeLoginRoute.tsx (2)
1-1: LGTM: useStore 가져오기 추가
useStore훅을 가져오는 것은 인증 상태에 접근하기 위해 필요한 좋은 변경사항입니다. 별칭 경로를 사용하여 가져오는 것은 유지보수성을 높이는 좋은 방법입니다.
Line range hint
1-11: 전반적인 변경사항에 대한 요약이 PR의 변경사항들은 BeforeLoginRoute에 대한 인증 검사를 성공적으로 구현했습니다. 주요 개선 사항은 다음과 같습니다:
useStore훅을 사용하여 동적 인증 상태 관리- 인증된 사용자를 적절히 리다이렉트하는 로직 추가
- 비인증 사용자에 대한 기존 로직 유지
이러한 변경사항들은 PR의 목표인 "페이지 라우트 인증 처리"를 잘 달성했습니다. 코드는 이제 더 동적이고 유지보수가 용이해졌습니다.
src/routes/AfterLoginRoute.tsx (2)
1-1: useStore 가져오기 추가 승인useStore를 가져오는 것은 동적 인증 확인을 위해 필요한 좋은 변경사항입니다. 상대 경로를 사용하여 가져오는 것도 좋은 방식입니다.
Line range hint
1-11: 전체적인 변경 사항 요약 및 승인이 PR의 변경 사항은 AfterLoginRoute 컴포넌트의 인증 로직을 크게 개선했습니다:
- useStore를 사용하여 동적 인증 상태 확인을 구현했습니다.
- 하드코딩된 인증 상태를 제거하고 실제 사용자 상태를 반영하도록 했습니다.
이러한 변경은 컴포넌트의 유연성과 유지보수성을 향상시키며, PR의 목표인 라우트 인증 구현을 잘 달성했습니다. 또한, 이 변경으로 인한 보안 문제는 발견되지 않았습니다.
전반적으로 이 변경은 매우 긍정적이며, 코드의 품질을 향상시켰습니다.
src/mocks/services/userServiceHandler.ts (2)
20-21: 사용자 ID 추출 로직 개선액세스 토큰에서 사용자 ID를 추출하는 로직이 간소화되었습니다. 이전의 더미 JWT 토큰 체크를 제거하고
convertTokenToUserId함수를 일관되게 사용하도록 변경한 것은 좋은 개선입니다. 이로 인해 코드의 가독성이 향상되고 복잡성이 감소되었습니다.
Line range hint
1-138: 전체적인 코드 개선 사항 요약이번 변경사항들은 전반적으로 코드의 품질을 향상시켰습니다:
- 더미 JWT 토큰 관련 코드가 제거되어 실제 운영 환경에 더 적합한 코드가 되었습니다.
- 사용자 ID 추출 로직이 일관되게 적용되어 코드의 일관성과 가독성이 개선되었습니다.
convertTokenToUserId함수의 일관된 사용으로 인해 향후 인증 로직 변경 시 유지보수가 용이해졌습니다.이러한 변경사항들은 PR의 목표인 MSW 수정 및 사용자 인증 개선에 잘 부합합니다. 다만, 앞서 제안한 대로 반복되는 로직을 함수로 추출하여 재사용성을 높이는 추가적인 리팩토링을 고려해보시기 바랍니다.
|
@ice-bear98 승찬님 리뷰 감사합니다!😆 원래 버그문제는 PR 따로 올리려고 했는데 그냥 오늘 고쳐서 한번에 머지해야겠네요ㅋㅋㅋ 혹시 시간이 나신다면 나중에 한 번 더... 리뷰 해주시면 정말 감사하겠습니다🙇♂️ |
|
@Seok93 리뷰 감사합니다!😆 |
PR Type
What kind of change does this PR introduce?
Related Issues
What does this PR do?
View
Other information
새로고침 시 zustand 내부의 인증값이 휘발되면서 로그인 페이지로 이동하는 현상이 발생하고 있습니다.
다른 분들 작업을 위해서 일단 작업한 부분만 올리고, 문제가 되는 내용은 내일 다시 PR을 올려 시정하도록 하겠습니다.
추가 (수정)
새로고침 시
isAuthenticated가 초기화되는 문제가 발생하였습니다.로그인 후 영속적으로 유지되는 값인
userInfo정보의userId를 이용해 추가적인 체크를 하도록 하여 이를 수정했습니다.보안적으로 완벽한 방법이 아니라 임시방편이기는 하지만 우선 새로고침 후에도 로그인 화면으로 이동되는 문제는 해결이 되었습니다.
보안적인 문제도 그렇고 현재의 로직은
isAuthenticated가 아니라userInfo에 더 의존하고 있기 때문에 기존에 생각했던 로직과 달라진 점이 있습니다. 따라서 방법을 더 찾아보고 추가적인 수정이 필요할 것 같습니다.