Conversation
) develop <- feat/web/filter/#20
- User의 컬럼 목록 변경 - Database의 User 저장 로직 변경(sequential key 추가)
- Redirect Renderer/WebResponse 추가
- 세션 엔티티 정의 - 세션 스토리지 개발
- AuthenticationInfo 인터페이스 정의 - 비로그인 유저용 구현체 UnanimousAuthentication 개발 - 로그인 유저용 구현체 UnanimousAuthentication 개발 - HttpRequest 클래스의 멤버에 AuthenticationInfo 추가
- 세션 정보 로딩 필터 개발 - 권한별 접근 제어 필터 개발
- 로그인 핸들러 개발 - 로그인 뷰 버튼 수정
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
종합 리뷰
PR 요청사항 답변: 구조 설계
로그인 포인트를 핸들러 계층에 배치한 것은 적절한 선택입니다.
로그인은 순수 비즈니스 로직(사용자 검증, 세션 생성)이기 때문에 handlers에서 처리하는 것이 관심사의 분리 원칙에 부합합니다. 반면 필터는 요청/응답의 횡단 관심사(authentication 정보 로드, authorization 확인)에만 집중하므로 현재 구조가 명확하고 유지보수하기 좋습니다.
핵심 이슈 (우선순위순)
1. 보안: 평문 비밀번호 저장 및 비교 (심각)
- LoginWithPost.java의 직접 비교가 위험합니다. 데이터베이스 유출 시 모든 사용자 비밀번호 노출
- 필수: bcrypt, PBKDF2 등으로 해싱 구현 필요
2. 시간 계산 오류 (높음)
- VariableConfig의
30*60*100은 18초인데, 의도는 30분으로 보입니다 - 1000을 곱해야 올바른 밀리초 단위가 됩니다
3. 세션 관리 메모리 누수 (중간)
- 타임아웃된 세션만 삭제되고, 만료되지 않은 과거 세션은 영구 보관
- Scheduled task로 정기적인 정리 필요
4. Null Pointer Exception 위험 (중간)
- MemberAuthorizationFilter와 UnanimousAuthorizationFilter에서 authenticationInfo 체크 필수
5. 로그인 성능 이슈 (낮음)
- Database.findUserByEmail()의 stream 기반 순회는 O(n) 시간복잡도
- 사용자 수 증가 시 로그인 성능 저하
긍정적인 측면
✅ 필터-핸들러 분리가 명확하고 구조가 이해하기 쉬움
✅ ConcurrentHashMap 사용으로 thread-safe한 세션 관리
✅ Optional 사용으로 null 안전성 고려
✅ 인증/인가 필터 분리가 적절함
| "./src/main/resources/static"); | ||
|
|
||
| public static final long IDLE_MS = 30*60*100; | ||
| public static final long ABSOLUTE_MS = 180*60*100; |
There was a problem hiding this comment.
단위 오류: 30*60*100은 18,000ms(18초)이며, 180*60*100은 1,080,000ms(18분)입니다. 일반적으로 세션 타임아웃은 분 단위로 설정하는데, 계산식에서 1000을 곱해야 합니다.\n\n의도:\n- IDLE_MS: 30분 = 30*60*1000\n- ABSOLUTE_MS: 180분(3시간) = 180*60*1000"
| public static Optional<User> findUserByEmail(String email){ | ||
| return users.values().stream().filter(u -> u.getEmail().equals(email)).findAny(); |
There was a problem hiding this comment.
성능 경고: stream().filter().findAny()는 매번 전체 컬렉션을 순회합니다. 사용자 수가 증가하면 로그인 성능이 저하됩니다.\n\n개선안: 사용자를 이메일로 검색하려면 별도의 Map을 유지하거나 데이터베이스 인덱싱을 사용하세요.\njava\nprivate static Map<String, User> usersByEmail = new ConcurrentHashMap<>();\n\npublic static Optional<User> findUserByEmail(String email) {\n return Optional.ofNullable(usersByEmail.get(email));\n}\n"
|
|
||
| if (!user.getPassword().equals(password)) { | ||
| throw new ServiceException(ErrorCode.LOGIN_FAILED); |
There was a problem hiding this comment.
보안 위험: 평문 비밀번호를 데이터베이스에 저장하고 직접 비교하면 안 됩니다. 해킹으로 데이터베이스가 유출되면 모든 사용자의 비밀번호가 노출됩니다.\n\n필수 개선: 비밀번호를 해싱(예: bcrypt, PBKDF2)으로 저장하고, 로그인 시에는 입력 비밀번호를 동일한 해시 알고리즘으로 검증하세요."
| public User(String password, String nickname, String email, String userRole) { | ||
| this.password = password; | ||
| this.name = name; | ||
| this.nickname = nickname; | ||
| this.email = email; | ||
| this.userRole = userRole; |
There was a problem hiding this comment.
설계 문제: User 엔티티의 userId가 null로 초기화되고 나중에 setUserId()로 설정됩니다. 생성 후 즉시 사용하면 런타임 오류가 발생할 수 있습니다.\n\n개선안: 생성자에서 userId를 받거나, 필수 필드를 필드 선언 단계에서 초기화하세요."
| if(request.getAuthenticationInfo().getRole().equals(UserRole.MEMBER)){ | ||
| chain.doFilter(); | ||
| } else { | ||
| response.setStatus(HttpStatus.FOUND); | ||
| response.setHeader("Location", "/login"); | ||
| response.setHeader("Content-Length", "0"); | ||
| } |
There was a problem hiding this comment.
Null Pointer 위험: authenticationInfo가 null인 경우 NullPointerException이 발생합니다.\n\n개선안: null 체크를 추가하고, 권장되는 HTTP 상태 코드는 redirect(302)보다는 unauthorized(401) 또는 forbidden(403)이 더 적절합니다."
| public void runFilter(HttpRequest request, HttpResponse response, FilterChainContainer.FilterChainEngine chain) { | ||
| if(request.getAuthenticationInfo().getRole().equals(USER_ROLE)) { | ||
| chain.doFilter(); | ||
| } else { | ||
| response.setStatus(HttpStatus.FOUND); | ||
| response.setHeader("Location", "/"); | ||
| response.setHeader("Content-Length", "0"); |
There was a problem hiding this comment.
Null Pointer 위험: authenticationInfo가 null인 경우 NullPointerException이 발생합니다.\n\n개선안: null 체크를 추가하세요."
| if (now - s.getCreatedAt() > absoluteMs || now - s.getLastAccessAt() > idleMs) { | ||
| store.remove(sid); | ||
| return null; | ||
| } | ||
| s.touch(now); |
There was a problem hiding this comment.
메모리 누수 위험: 타임아웃된 세션을 명시적으로 삭제하지만, 만료되지 않은 세션이 요청되지 않으면 영구적으로 메모리에 남아있습니다(현재 remove() 호출이 getValid() 내부에만 있음).\n\n개선안: 별도의 background thread나 scheduled task를 통해 주기적으로 만료된 세션을 정리하세요."
| case PUBLIC -> List.of( | ||
| appConfig.authenticationFilter()); | ||
| case AUTHENTICATED -> List.of( | ||
| appConfig.authenticationFilter(), | ||
| appConfig.memberAuthorizationFilter()); | ||
| case RESTRICT -> List.of( | ||
| appConfig.restrictedFilter()); | ||
| case LOG_IN -> List.of( | ||
| appConfig.authenticationFilter(), | ||
| appConfig.unanimousAuthorizationFilter()); | ||
| }; | ||
| } |
There was a problem hiding this comment.
구조 설계 피드백 (PR 요청사항): 로그인 로직을 핸들러 계층에 배치한 것은 좋은 선택입니다. 이유:\n\n✅ 핸들러 계층 배치의 장점:\n- 로그인은 비즈니스 로직 (사용자 검증, 세션 생성)이므로 handlers에 속함\n- 필터는 요청/응답의 관심사 분리(authentication/authorization 처리)에만 집중\n- 필터-핸들러 분리가 명확하고 유지보수 용이\n\n✅ 구현 모델이 명확함: \n- AuthenticationFilter: 세션 정보 로드 및 사용자 정보 설정\n- MemberAuthorizationFilter/UnanimousAuthorizationFilter: 권한 확인\n- LoginWithPost: 실제 로그인 비즈니스 로직 처리\n\n다만 위의 보안 및 안정성 이슈(비밀번호 해싱, null 체크, 성능 등)를 개선하면 더 견고한 구조가 될 것입니다."
- 해당 커밋에 포함되지 않은 로그아웃 핸들러의 의존성을 제거
1 participant
💻 작업 내용
✨ 리뷰 포인트
로그인 포인트를 필터 체인단에 놓을까, 핸들러 계층에 놓을까 고민하다 핸들러 계층에 놓았습니다.
구조가 적절한지 피드백주세요.
🎯 관련 이슈
closed #22