임혜민 sprint11 - #240
Hidden character warning
임혜민 sprint11#240hyemin-L wants to merge 20 commits into
Conversation
There was a problem hiding this comment.
Sprint11 리뷰 요약
Sprint11 미션(테스트 코드, S3 스토리지, CI/CD, Docker 배포)을 담은 대규모 PR입니다. @Retryable+@Recover 실패 복구, presigned URL, MDC 로깅 인터셉터 등 구조 설계는 좋습니다. 다만 아래 두 가지 P1을 먼저 해결해 주세요.
P1-1. 다수 핵심 클래스 미커밋 → 컴파일 불가
PR head 트리를 직접 조회한 결과, 코드가 참조하는 다음 클래스/패키지가 저장소에 존재하지 않습니다.
security/패키지 전체 (JwtTokenProvider,JwtRegistry,DiscodeitUserDetails)event/BinaryContentCreatedEvent,service/NotificationServiceentity/Role,entity/BinaryContentStatusdto/data/JwtInformation,dto/request/RoleUpdateRequest
User.java는 Role/Role.USER를, BasicAuthService/S3BinaryContentStorage/BasicBinaryContentService는 위 클래스들을 직접 사용하므로 빌드가 깨집니다. git add 누락 또는 .gitignore 과다 제외로 보입니다. 누락 파일을 모두 푸시해 주세요.
P1-2. 빌드 산출물·OS 파일 커밋 (P2)
build/**/*.class, .gradle/**, .DS_Store가 다수 추적되고 있습니다(약 170여 개). .gitignore에 패턴은 있으나 이미 추적된 파일은 제외되지 않으니 git rm -r --cached build .gradle 후 정리해 주세요.
나머지 라인별 코멘트(배포 워크플로우 버그, 인가 우회, 민감정보 로깅, S3Client 재생성 등)를 함께 확인 부탁드립니다. 전반적인 설계 방향은 좋으니 위 항목만 보완되면 좋을 것 같습니다.
| # 3️⃣ 새 이미지로 Task Definition 수정 | ||
| - name: Update task definition image | ||
| run: | | ||
| $REPO_URI=${{ vars.ECR_REPOSITORY_URI }} |
There was a problem hiding this comment.
P1. bash 변수 할당은 $ 없이 작성해야 합니다.
| $REPO_URI=${{ vars.ECR_REPOSITORY_URI }} | |
| REPO_URI=${{ vars.ECR_REPOSITORY_URI }} |
현재 $REPO_URI=...는 "command not found" 에러를 발생시켜 deploy 잡이 실패합니다.
| echo "REPO_URI=${{ vars.ECR_REPOSITORY_URI }}" >> $GITHUB_ENV | ||
|
|
||
| docker tag discodeit:latest $REPO_URI:latest | ||
| docker tag discodeit:latest $REPO_URI:$COMMIT_HASH |
There was a problem hiding this comment.
P1. 바로 위에서 echo "REPO_URI=..." >> $GITHUB_ENV로 설정한 변수를 같은 스텝에서 $REPO_URI로 사용하고 있습니다. $GITHUB_ENV에 기록한 값은 다음 스텝부터 반영되므로 이 스텝에서는 빈 값입니다. 또한 IMAGE_TAG를 정의했지만 태그에는 미정의 변수 $COMMIT_HASH를 쓰고 있습니다. 같은 스텝 안에서 쓰려면 일반 셸 변수로 정의해서 사용하세요.
| docker tag discodeit:latest $REPO_URI:$COMMIT_HASH | |
| REPO_URI=${{ vars.ECR_REPOSITORY_URI }} | |
| COMMIT_HASH=${GITHUB_SHA::7} | |
| docker tag discodeit:latest $REPO_URI:latest | |
| docker tag discodeit:latest $REPO_URI:$COMMIT_HASH |
참고로 build-and-push 잡과 deploy 잡은 별도 러너라 env가 공유되지 않으니, deploy 잡에서도 변수를 다시 정의해야 합니다.
|
|
||
| public UUID put(UUID binaryContentId, byte[] bytes) { | ||
| try { | ||
| Thread.sleep(3000); |
There was a problem hiding this comment.
P2. 운영 코드의 업로드 경로에 3초 인위적 지연(Thread.sleep(3000))이 그대로 남아 있습니다. 비동기/재시도 동작 테스트용이라면 제거하거나 테스트 전용 코드로 분리해 주세요. 실제 업로드마다 3초가 추가됩니다.
|
|
||
| @Transactional | ||
| @Override | ||
| public UserDto updateRoleInternal(RoleUpdateRequest request) { |
There was a problem hiding this comment.
P2. 인가 우회 가능. updateRole은 @PreAuthorize("hasRole('ADMIN')")로 보호되지만, 실제 로직인 updateRoleInternal이 public + AuthService 인터페이스에 노출되어 있고 권한 애너테이션이 없습니다. 다른 빈에서 updateRoleInternal을 직접 호출하면 권한 검사를 건너뛰고 임의 사용자의 role을 변경할 수 있습니다. internal 메서드는 private/패키지 스코프로 숨기거나 인터페이스에서 제외해 주세요. (같은 빈 내부 호출은 프록시를 거치지 않아 @PreAuthorize도 적용되지 않습니다.)
| } | ||
| } | ||
|
|
||
| private S3Client getS3Client() { |
There was a problem hiding this comment.
P3. getS3Client()/getS3Presigner()가 put·get·download 호출마다 새 클라이언트를 생성합니다. S3Client는 thread-safe하고 생성 비용이 큰 무거운 객체이며, get/put 경로에서는 close()도 되지 않아 리소스 누수가 발생합니다. @Bean singleton으로 등록해 주입받는 형태를 권장합니다.
| String key = metaData.id().toString(); | ||
| String presignedUrl = generatePresignedUrl(key, metaData.contentType()); | ||
|
|
||
| log.info("생성된 Presigned URL: {}", presignedUrl); |
There was a problem hiding this comment.
P3. 민감정보 로깅. presigned URL은 임시 접근 권한이 담긴 민감 정보입니다. info 레벨로 운영 로그에 남기면 로그 접근자가 그대로 객체에 접근할 수 있습니다. debug로 강등하거나 제거해 주세요.
| Exception e | ||
| ) { | ||
|
|
||
| userRepository.findAll() |
There was a problem hiding this comment.
P3. userRepository.findAll().stream().filter(role==ADMIN).findFirst()는 전체 사용자를 메모리에 적재한 뒤 필터링합니다. 사용자가 늘어나면 부담이 커집니다. findFirstByRole(Role.ADMIN) 같은 쿼리 메서드로 DB에서 직접 조회하세요.
| // Validate refresh token | ||
| if (!tokenProvider.validateRefreshToken(refreshToken) | ||
| || !jwtRegistry.hasActiveJwtInformationByRefreshToken(refreshToken)) { | ||
| log.error("Invalid or expired refresh token: {}", refreshToken); |
There was a problem hiding this comment.
P3. 민감정보 로깅. refresh token 원문을 로그에 남기면 안 됩니다. 토큰 값 대신 사유/사용자 식별자 정도만 기록하세요.
| datasource: | ||
| url: jdbc:postgresql://localhost:5432/discodeit | ||
| username: discodeit_user | ||
| password: discodeit1234 |
There was a problem hiding this comment.
P3. dev DB 비밀번호가 하드코딩되어 있습니다(discodeit1234). 로컬 전용이라도 환경변수/.env로 분리하는 편이 안전합니다. 더불어 application.yaml의 JWT 기본 시크릿 fallback(your-access-token-secret-key-...)도 prod에서 우연히 사용되지 않도록 fallback을 제거하는 것을 권장합니다.
| try { | ||
| Files.createDirectories(root); | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
P4. 예외를 e.printStackTrace() 대신 SLF4J 로거(log.error(...))로 남겨 주세요. (73번 라인에도 동일.) @Retryable+@Recover 실패 복구 패턴은 S3 쪽에 잘 적용되어 있습니다 👍
2 participants
요구사항
기본
기본 항목은 전부 완료했습니다.
심화
멘토에게