[DDING-000] 컨트롤러 테스트 보안 환경 정리 #352
Conversation
Walkthrough테스트 지원 클래스를 단순화하고 구현체 기반 목(mock)과 WebMvcTest 설정을 제거해 테스트 스캐폴딩을 축소했으며, SecurityContextFactory의 권한 인자 전달 방식을 컬렉션으로 변경하고 개별 컨트롤러 테스트에서 서비스 목을 인터페이스 기반 Changes
Sequence Diagram(s)(생성 조건 미충족 — 변경은 주로 테스트 리팩토링 및 단순화로, 다수 컴포넌트 간 새로운 제어 흐름이 도입되지 않아 시퀀스 다이어그램을 생략합니다.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
Gopistol
changed the title
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/test/java/ddingdong/ddingdongBE/common/support/WebApiUnitTestSupport.javasrc/test/java/ddingdong/ddingdongBE/common/support/WithMockAuthenticatedUserSecurityContextFactory.javasrc/test/java/ddingdong/ddingdongBE/domain/documents/controller/AdminDocumentControllerUnitTest.javasrc/test/java/ddingdong/ddingdongBE/domain/documents/controller/DocumentControllerUnitTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_push
🔇 Additional comments (5)
src/test/java/ddingdong/ddingdongBE/common/support/WithMockAuthenticatedUserSecurityContextFactory.java (1)
25-26: 권한 주입 방식 수정이 올바릅니다!
TestingAuthenticationToken생성자는 세 번째 파라미터로Collection<? extends GrantedAuthority>를 기대하는데, 기존 코드는String.valueOf()로 문자열 표현을 전달하여 타입이 맞지 않았습니다. 이제principalDetails.getAuthorities()를 직접 전달하여 실제 권한 컬렉션을 정확히 주입하므로, 테스트에서 인가 검증이 올바르게 동작할 수 있습니다.src/test/java/ddingdong/ddingdongBE/domain/documents/controller/AdminDocumentControllerUnitTest.java (1)
34-34: 인터페이스 기반 목 사용으로 전환 - 적절합니다.구현체(
facadeAdminDocumentServiceImpl) 대신 인터페이스(facadeAdminDocumentService)를 사용하도록 일관되게 변경되었습니다. 의존성 역전 원칙에 부합하며 WebApiUnitTestSupport의 목 설정 변경과 올바르게 연동됩니다.Also applies to: 44-44, 59-59, 68-68, 83-83
src/test/java/ddingdong/ddingdongBE/domain/documents/controller/DocumentControllerUnitTest.java (1)
38-38: 인터페이스 기반 목 사용으로 전환 - 적절합니다.구현체(
facadeDocumentServiceImpl) 대신 인터페이스(facadeDocumentService)를 사용하도록 변경되었습니다. 테스트 인프라 변경 사항과 올바르게 정렬되어 있습니다.Also applies to: 63-63
src/test/java/ddingdong/ddingdongBE/common/support/WebApiUnitTestSupport.java (2)
34-34: SecurityConfig 통합 접근 방식 - 적절합니다.운영 SecurityConfig를 테스트 환경에 적용하고 필요한 최소한의 JWT 관련 빈(JwtAuthService, JwtConfig)만 목으로 주입하는 접근이 합리적입니다. MockMvc를 springSecurity()와 함께 재구성하여 보안 필터가 올바르게 적용되도록 보장합니다. PR 목표에 부합하며 과도한 목 사용을 피하면서도 테스트 컨텍스트 로딩을 보장합니다.
Also applies to: 48-51, 75-80
36-40: 인터페이스 기반 목 전환 및 배너 컨트롤러 지원 추가 - 적절합니다.구현체 대신 인터페이스 기반 목을 사용하도록 전환하여 테스트가 추상화에 의존하게 되었습니다. 배너 컨트롤러(AdminBannerController, UserBannerController)와 관련 서비스 목이 추가되어 테스트 인프라의 지원 범위가 확장되었습니다. 공유 테스트 지원 클래스의 일관성 유지를 위해 합리적인 접근입니다.
Also applies to: 53-66
src/test/java/ddingdong/ddingdongBE/common/support/WebApiUnitTestSupport.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/ddingdong/ddingdongBE/common/support/WebApiUnitTestSupport.java (1)
1-14: SecurityConfig와 JWT 관련 빈을 테스트 컨텍스트에 추가해야 함SecurityConfig, JwtAuthService, JwtConfig 클래스는 프로젝트에 존재하지만 WebApiUnitTestSupport에 import되거나 @MockitoBean으로 선언되지 않았습니다.
PR 목표에 따라 이들을 테스트 컨텍스트에 추가하려면:
ddingdong.ddingdongBE.common.config.SecurityConfigimport 추가ddingdong.ddingdongBE.auth.service.JwtAuthServiceimport 추가ddingdong.ddingdongBE.common.config.JwtConfigimport 추가- 해당 클래스들에 대한 @MockitoBean 필드 추가 필요
♻️ Duplicate comments (1)
src/test/java/ddingdong/ddingdongBE/common/support/WebApiUnitTestSupport.java (1)
11-12: 이전 리뷰 코멘트 확인: 미사용 import 제거이전 리뷰에서
FacadeAdminDocumentServiceImpl과FacadeDocumentServiceImplimport가 더 이상 사용되지 않으므로 제거해야 한다고 지적되었습니다. 인터페이스 기반 mock으로 전환되면서 구현체 import는 불필요합니다.현재 코드에서 해당 import들이 제거되었는지 확인이 필요합니다.
다음 스크립트로 Impl 클래스 import 존재 여부를 확인해주세요:
#!/bin/bash # Impl 클래스 import 확인 rg -n "FacadeAdminDocumentServiceImpl|FacadeDocumentServiceImpl" src/test/java/ddingdong/ddingdongBE/common/support/WebApiUnitTestSupport.java
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/test/java/ddingdong/ddingdongBE/common/support/WebApiUnitTestSupport.javasrc/test/java/ddingdong/ddingdongBE/domain/documents/controller/AdminDocumentControllerUnitTest.javasrc/test/java/ddingdong/ddingdongBE/domain/documents/controller/DocumentControllerUnitTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/ddingdong/ddingdongBE/domain/documents/controller/DocumentControllerUnitTest.java
- src/test/java/ddingdong/ddingdongBE/domain/documents/controller/AdminDocumentControllerUnitTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_push
🔇 Additional comments (1)
src/test/java/ddingdong/ddingdongBE/common/support/WebApiUnitTestSupport.java (1)
55-60: setUp 메서드의 security 적용 방식 양호
springSecurity()를 적용하여 MockMvc를 재구성하는 방식은 적절합니다. 이를 통해 테스트에서 실제 보안 설정이 적용되도록 보장할 수 있습니다.
src/test/java/ddingdong/ddingdongBE/common/support/WebApiUnitTestSupport.java
Outdated
Show resolved
Hide resolved
| @MockitoBean | ||
| protected FacadeDocumentService facadeDocumentService; | ||
| @MockitoBean | ||
| protected FacadeAdminDocumentService facadeAdminDocumentService; | ||
| @MockitoBean | ||
| protected FacadeAdminBannerService facadeAdminBannerService; | ||
| @MockitoBean | ||
| protected FacadeUserBannerService facadeUserBannerService; | ||
| @MockitoBean | ||
| protected FileService fileService; | ||
| @MockitoBean | ||
| protected FileInformationService fileInformationService; | ||
| @MockitoBean | ||
| protected ClubService clubService; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check usage of newly added mock beans in test files
cd "$(git rev-parse --show-toplevel)" 2>/dev/null || cd .
echo "=== Checking facadeAdminBannerService usage ==="
rg -l "facadeAdminBannerService" --type java 2>/dev/null | head -20
echo -e "\n=== Checking facadeUserBannerService usage ==="
rg -l "facadeUserBannerService" --type java 2>/dev/null | head -20
echo -e "\n=== Checking fileService usage ==="
rg -l "fileService" --type java 2>/dev/null | head -20
echo -e "\n=== Checking fileInformationService usage ==="
rg -l "fileInformationService" --type java 2>/dev/null | head -20
echo -e "\n=== Checking clubService usage ==="
rg -l "clubService" --type java 2>/dev/null | head -20
echo -e "\n=== Checking for JWT-related mocks ==="
rg -l "JwtAuthService|JwtConfig" --type java 2>/dev/null | head -20Repository: COW-dev/ddingdong-be
Length of output: 2699
🏁 Script executed:
# Check if the file WebApiUnitTestSupport exists and view its content
find . -name "WebApiUnitTestSupport.java" -type f 2>/dev/null | head -1 | xargs -r cat -nRepository: COW-dev/ddingdong-be
Length of output: 2844
🏁 Script executed:
# Check PR objectives or commit messages related to JWT
git log --oneline --all -20 2>/dev/null | grep -i "jwt\|auth" || echo "No JWT-related commits found in recent history"Repository: COW-dev/ddingdong-be
Length of output: 110
🏁 Script executed:
# Find test classes that extend WebApiUnitTestSupport
rg "extends WebApiUnitTestSupport" --type java -l
# Check if fileService is used in those test classes
echo -e "\n=== fileService usage in extending tests ==="
rg "extends WebApiUnitTestSupport" --type java -l | xargs -I {} sh -c 'echo "File: {}"; grep -n "fileService" "{}" || echo "Not used"'
# Check if fileInformationService is used in those test classes
echo -e "\n=== fileInformationService usage in extending tests ==="
rg "extends WebApiUnitTestSupport" --type java -l | xargs -I {} sh -c 'echo "File: {}"; grep -n "fileInformationService" "{}" || echo "Not used"'Repository: COW-dev/ddingdong-be
Length of output: 817
🏁 Script executed:
# Check JWT usage in test files to understand if JWT mocks should be in support
rg "JwtAuthService|JwtConfig" --type java -g "*Test.java" -l | head -10Repository: COW-dev/ddingdong-be
Length of output: 46
불필요한 mock 빈 제거 필요
fileService와 fileInformationService mock은 이 support 클래스를 상속하는 어떤 테스트 클래스에서도 사용되지 않습니다. 불필요한 의존성 mock을 제거하여 테스트 유지보수성을 개선하세요.
facadeAdminBannerService, facadeUserBannerService, clubService는 실제 테스트에서 사용되므로 유지하면 됩니다.
🤖 Prompt for AI Agents
In src/test/java/ddingdong/ddingdongBE/common/support/WebApiUnitTestSupport.java
around lines 33 to 46, remove the two unused mock beans fileService and
fileInformationService from this support class since no inheriting tests use
them; retain facadeAdminBannerService, facadeUserBannerService and clubService
as they are used. Update the class to delete the @MockitoBean fields for
FileService and FileInformationService and any unused imports or references that
become redundant after their removal.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/ddingdong/ddingdongBE/common/support/WebApiUnitTestSupport.java (1)
13-14: 주석 처리된 코드 제거 권장
WebApplicationContext필드가 주석 처리되어 있습니다. 주석 처리된 코드는 유지보수를 어렵게 만들고 코드베이스를 복잡하게 만들 수 있습니다. 필요 없다면 완전히 제거하는 것이 좋습니다. 버전 관리 시스템을 통해 언제든 과거 코드를 확인할 수 있습니다.🔎 제안하는 수정
-// @Autowired -// private WebApplicationContext context; @Autowired protected MockMvc mockMvc;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/test/java/ddingdong/ddingdongBE/common/support/WebApiUnitTestSupport.javasrc/test/java/ddingdong/ddingdongBE/domain/documents/controller/AdminDocumentControllerUnitTest.javasrc/test/java/ddingdong/ddingdongBE/domain/documents/controller/DocumentControllerUnitTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/ddingdong/ddingdongBE/domain/documents/controller/AdminDocumentControllerUnitTest.java
- src/test/java/ddingdong/ddingdongBE/domain/documents/controller/DocumentControllerUnitTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_push
🔇 Additional comments (2)
src/test/java/ddingdong/ddingdongBE/common/support/WebApiUnitTestSupport.java (2)
15-18: 테스트에 필요한 핵심 의존성 정의가 적절합니다
MockMvc와ObjectMapper를 protected로 선언하여 하위 테스트 클래스에서 접근 가능하도록 구성한 것이 적절합니다. 웹 API 단위 테스트에 필요한 최소한의 의존성만 유지하는 깔끔한 접근입니다.
20-22: 유용한 헬퍼 메서드입니다
toJson헬퍼 메서드는 테스트에서 객체를 JSON 문자열로 변환하는 데 유용하며, 구현이 간결하고 명확합니다.
| // @BeforeEach | ||
| // void setUp() { TODO: 추후 컨벤션 정의 후 적용 | ||
| // mockMvc = MockMvcBuilders.webAppContextSetup(context) | ||
| // .apply(springSecurity()) | ||
| // .build(); | ||
| // } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify test classes and their security configuration
echo "=== Test classes extending WebApiUnitTestSupport ==="
rg "extends WebApiUnitTestSupport" --type java -A 5
echo -e "\n=== Files containing WebApiUnitTestSupport ==="
rg "WebApiUnitTestSupport" --type java -l
echo -e "\n=== Check WebApiUnitTestSupport class content ==="
cat -n src/test/java/ddingdong/ddingdongBE/common/support/WebApiUnitTestSupport.javaRepository: COW-dev/ddingdong-be
Length of output: 3225
주석 처리된 코드 정리 필요
현재 테스트 클래스들(AdminDocumentControllerUnitTest, DocumentControllerUnitTest)이 @WithMockAuthenticatedUser 어노테이션을 통해 보안 설정을 적용하고 있어 테스트가 정상적으로 작동합니다. 주석 처리된 setUp() 메서드는 기존 패턴이며, 현재는 Spring의 자동 설정과 메서드 레벨 보안 어노테이션으로 충분히 작동하고 있습니다.
다만 주석 처리된 코드와 TODO 항목을 다음 중 하나로 정리해주세요:
- 향후 컨벤션 정의 시 적용할 예정이면 이슈/작업으로 등록하여 추적하고, 주석 코드는 제거
- 즉시 적용 불가능하면 명확한 설명과 함께 FIXME로 변경하여 의도를 명시
🤖 Prompt for AI Agents
In src/test/java/ddingdong/ddingdongBE/common/support/WebApiUnitTestSupport.java
around lines 24 to 29, remove the commented-out setUp() method and TODO since
tests rely on current Spring/autoconfig and method-level security; create a
tracked issue (e.g., in your project tracker) to define the future convention
and reference that issue ID in a single-line comment where the removed block was
(or in the PR description) so the intent is recorded.
🚀 작업 내용
@WebMvcTest 기반 컨트롤러 테스트에서 운영 SecurityConfig가 적용되지 않아 기본 보안이 켜지며 permitAll API도 401이 발생하던 문제를 수정했습니다.
@WithMockAuthenticatedUser 테스트 인증 컨텍스트에서 권한을 문자열로 넣던 부분을 실제 authorities 컬렉션으로 주입하도록 수정했습니다.
위 변경으로 영향받는 문서 컨트롤러 단위 테스트(AdminDocumentControllerUnitTest, DocumentControllerUnitTest)를 정상 동작하도록 최소 수정했습니다.
🤔 고민했던 내용
보안 설정을 포함시키면 JWT 관련 의존 빈이 필요하므로 컨텍스트 로딩을 위한 최소 mock 범위로 JwtAuthService/JwtConfig만 추가했습니다.
💬 리뷰 중점사항
WebApiUnitTestSupport에 SecurityConfig를 import하는 접근이 컨트롤러 단위 테스트 환경으로 적절한지
WithMockAuthenticatedUserSecurityContextFactory의 권한 주입 방식 수정이 팀 컨벤션과 맞는지
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.