이용일 sprint4#82
Hidden character warning
Conversation
hagyutae
left a comment
hagyutae
left a comment
There was a problem hiding this comment.
전체 프로젝트를 새로 구성한 PR로, 도메인별 컨트롤러 분리, DTO 정의, 전역 예외 처리, 정적 리소스 서빙 등 대부분의 요구사항을 시도했습니다. 하지만 @RequestMapping만 사용하라는 핵심 요구사항을 다수 컨트롤러에서 위반했고, User 엔티티를 직접 반환하여 비밀번호가 API 응답에 노출되는 보안 이슈, JCFUserStatusRepository.deleteByUserId의 무한 재귀 버그 등 치명적인 문제들이 있습니다.
다시 한 번 살펴보며 보완해보세요~!
추가 리뷰
요구사항에 "Postman API 테스트 결과를 export하여 PR에 첨부해주세요"라고 명시되어 있으나, PR에 Postman collection이나 test run 결과가 첨부되어 있지 않습니다.
미션 요구사항에 "@RequestMapping만 사용해 구현해보세요"라고 명시되어 있으나, 다음 컨트롤러에서 shortcut annotation을 사용하고 있습니다:
BinaryContentController.java:@PostMapping,@GetMapping,@DeleteMapping사용MessageController.java:@PostMapping,@PatchMapping,@DeleteMapping,@GetMapping사용ReadStatusController.java:@PostMapping,@PatchMapping,@GetMapping사용
반면ChannelController,UserController,AuthController,UpdateUserStatusController는@RequestMapping을 사용하고 있어 일관성도 떨어집니다. 모든 컨트롤러에서 되도록이면@RequestMapping(method = RequestMethod.XXX)형태로 통일해야 합니다.
| value = "/login", | ||
| method = RequestMethod.POST | ||
| ) | ||
| public ResponseEntity<User> login( |
There was a problem hiding this comment.
User 엔티티 직접 반환 - 비밀번호 노출 (보안 이슈)
User 엔티티를 그대로 반환합니다. User 엔티티에는 password 필드가 있으므로 비밀번호가 JSON 응답에 그대로 포함됩니다. 이는 심각한 보안 취약점입니다.
UserDto처럼 password를 제외한 응답 DTO를 만들어 사용해야 합니다.
UserController.updateUser()에도 동일한 문제가 있으니 같이 해결해보세요.
There was a problem hiding this comment.
엔티티 반환을 DTO를 반환하도록 수정했습니다.
| @Override | ||
| public void deleteByUserId(UUID userId) { | ||
| this.findByUserId(userId) | ||
| .ifPresent(userStatus -> this.deleteByUserId(userStatus.getId())); |
There was a problem hiding this comment.
이상한 재귀 호출입니다.
이 함수의 동작이 어떻게 되는지 머릿속에 그려보고 수정해보세요.
| ) { | ||
| User user = authService.login(request); | ||
|
|
||
| userStatusService.updateByUserId( |
There was a problem hiding this comment.
컨트롤러에 비즈니스 로직 포함
로그인 성공 시 UserStatus를 업데이트하는 로직이 컨트롤러에 위치합니다. 이 로직은 AuthService.login() 내부에서 처리되어야 합니다. 컨트롤러는 HTTP 요청/응답 처리만 담당하고, 비즈니스 로직은 서비스 레이어에 위임하는 것이 레이어드 아키텍처의 원칙입니다.
|
|
||
| import java.time.Instant; | ||
|
|
||
| @Controller |
There was a problem hiding this comment.
AuthController와 UpdateUserStatusController에서 @Controller를 사용하고 있습니다. ResponseEntity를 반환하기 때문에 기능적으로는 동작하지만, REST API 컨트롤러에서는 @RestController를 사용하는 것이 Spring 컨벤션에 맞습니다. 다른 컨트롤러들은 @RestController를 사용하므로 일관성도 떨어집니다.
src/main/resources/application.yaml
Outdated
| spring: | ||
| application: | ||
| name: discodeit | ||
| server:port: 8080 No newline at end of file |
There was a problem hiding this comment.
올바른 YAML이 아닙니다. 올바른 형태는 다음과 같습니다:
server:
port: 8080There was a problem hiding this comment.
더 이상 사용되지 않는 파일은 삭제해주세요~!
There was a problem hiding this comment.
넵 혹여나 쓸일있을줄알고 남겨놨는데 죄송합니다
| public ResponseEntity<String> handleException(Exception e){ | ||
| return ResponseEntity | ||
| .status(HttpStatus.INTERNAL_SERVER_ERROR) | ||
| .body(e.getMessage()); |
There was a problem hiding this comment.
예외의 getMessage()를 클라이언트에 직접 반환합니다. 내부 구현 세부사항(스택트레이스, DB 정보 등)이 노출될 수 있어 보안 위험입니다. 위의 예외 핸들링에도 마찬가지로 적용되는 내용입니다.
특히, 500 에러에서는 "Internal server error"와 같은 일반적인 메시지를 반환하고, 원본 에러는 서버 로그에만 기록해야 합니다.
There was a problem hiding this comment.
요구사항에 "바이너리 파일을 1개 또는 여러 개 조회할 수 있다"라고 명시되어 있습니다. BinaryContentService.findAllByIdIn()은 구현되어 있지만, 이를 호출하는 컨트롤러 엔드포인트가 없습니다. 단일 조회(/api/binaryContent/find)만 존재하며, 다중 ID로 조회하는 API가 누락되었습니다.
lyi980403-arch
requested review from
rlarudgkswkd
and removed request for
rlarudgkswkd
hagyutae
left a comment
hagyutae
left a comment
There was a problem hiding this comment.
베이스 브랜치가 main으로 되어있어서 이용일 브랜치로 변경했구요,
그랬더니 충돌 감지가 되어서, 이 충돌에 대한 해결 진행해주셔야 합니다~!
앞으로 PR 작성할 때 베이스 브랜치는 항상 이용일 브랜치로 설정해주세요~!
2 participants
요구사항
기본
[O] DiscodeitApplication의 테스트 로직은 삭제하세요.
[O] 지금까지 구현한 서비스 로직을 활용해 웹 API를 구현하세요.
이때 @RequestMapping만 사용해 구현해보세요.
사용자 관리
[O] 사용자를 등록할 수 있다.
[O] 사용자 정보를 수정할 수 있다.
[O] 사용자를 삭제할 수 있다.
[O] 모든 사용자를 조회할 수 있다.
[O] 사용자의 온라인 상태를 업데이트할 수 있다.
[O] 웹 API의 예외를 전역으로 처리하세요.
[O] Postman을 활용해 컨트롤러를 테스트 하세요
권한 관리
[O] 사용자는 로그인할 수 있다.
채널 관리
[O] 공개 채널을 생성할 수 있다.
[O] 비공개 채널을 생성할 수 있다.
[O] 공개 채널의 정보를 수정할 수 있다.
[O] 채널을 삭제할 수 있다.
[O] 특정 사용자가 볼 수 있는 모든 채널 목록을 조회할 수 있다.
메시지 관리
[O] 메시지를 보낼 수 있다.
[O] 메시지를 수정할 수 있다.
[O] 메시지를 삭제할 수 있다.
[O] 특정 채널의 메시지 목록을 조회할 수 있다.
메시지 수신 정보 관리
[O] 특정 채널의 메시지 수신 정보를 생성할 수 있다.
[O] 특정 채널의 메시지 수신 정보를 수정할 수 있다.
[O] 특정 사용자의 메시지 수신 정보를 조회할 수 있다.
바이너리 파일 다운로드
[O] 바이너리 파일을 1개 또는 여러 개 조회할 수 있다.
심화
-[O] 사용자 목록 조회, BinaryContent 파일 조회 API를 다음의 조건을 만족하도록 수정하세요.
[O] 사용자 목록 조회
url: /api/user/findAll
요청
파라미터, 바디 없음
응답
ResponseEntity<List>
url: /api/binaryContent/find
요청
파라미터: binaryContentId
바디 없음
응답: ResponseEntity
주요 변경사항
스크린샷
멘토에게