계정(user)관련 기능 리팩토링 + 테스트 코드 추가 #113
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
❌ 빌드 테스트가 실패했습니다. 자세한 내용은 Github Actions 탭에서 확인해주세요 |
rodom1018
force-pushed
the
refactor-account
branch
from
21a1128 to
cf87fb3
Compare
seop-h
reviewed
Jan 11, 2025
Comment on lines
+33
to
+49
| String ip = request.getHeader("X-Forwarded-For"); | ||
| if (ip == null || ip.length() == 0 || "unknown".equalsIgnoreCase(ip)) { | ||
| ip = request.getHeader("Proxy-Client-IP"); | ||
| } | ||
| if (ip == null || ip.length() == 0 || "unknown".equalsIgnoreCase(ip)) { | ||
| ip = request.getHeader("WL-Proxy-Client-IP"); | ||
| } | ||
| if (ip == null || ip.length() == 0 || "unknown".equalsIgnoreCase(ip)) { | ||
| ip = request.getHeader("HTTP_CLIENT_IP"); | ||
| } | ||
| if (ip == null || ip.length() == 0 || "unknown".equalsIgnoreCase(ip)) { | ||
| ip = request.getHeader("HTTP_X_FORWARDED_FOR"); | ||
| } | ||
| if (ip == null || ip.length() == 0 || "unknown".equalsIgnoreCase(ip)) { | ||
| ip = request.getRemoteAddr(); | ||
| } | ||
| return ip; |
Contributor
There was a problem hiding this comment.
if문 조건이 전부다 동일한 것 같은데 이 부분 로직이 정확히 어떻게 되는 건가요?
Contributor
Author
There was a problem hiding this comment.
프록시 같은게 있을 때 client ip 가 바로 나오지 않는다고 하네요
아직 배포는 안해봐서 겪어보지는 못한 이슈입니다
Nginx가 프록시니까 넣어두었어요
참고 : https://stackoverflow.com/questions/4678797/how-do-i-get-the-remote-address-of-a-client-in-servlet
| logging: | ||
| config: "classpath:./logback-release.xml" | ||
|
|
||
| allow-ips: "${allow-ips}" |
Contributor
There was a problem hiding this comment.
별건 아니지만 allow-ips 를 allow_ips 로 바꿔주실 수 있나요? 지금 QA 서버의 모든 환경 변수 값을 이렇게 스네이크 케이스로 관리하고 있어서요
rodom1018
force-pushed
the
refactor-account
branch
from
ea0a328 to
193069a
Compare
seop-h
approved these changes
Jan 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
구현한 것
(1) 관리자 (admin) 이 접속하는 페이지는 ip 검사 하여 특정 ip 만 접속하게 함(jwt 토큰 검사하여 admin 계정인지도 체크함)
해당 기능을 위해서 security filter를 2개로 구분했습니다.
(2) 테스트 코드 작성 - 단위 테스트 / 통합 테스트 구분
api 접속할 때 유효한 토큰인지 검사하는 JWTFilter 와 같은 여러 필터에 대해(JWTFilterUnitTest, IPCheckFilterUnitTest) 단위 테스트, 회원가입 에 대한 단위 테스트 작성
로그인, 회원가입에 대한 통합 테스트(mockMvc) 작성
*소셜 로그인은 통합 테스트를 하려면 oauth2 관련 리소스 서버, 인증 서버를 Mock 화 해서 만들어야 해서(3rd party 서버에 직접 call 하는 방식의 통합 테스트는 비추천 되고 있음) 이는 너무 작업이 복잡하다고 판단하여 소셜로그인은 통합로그인 제작하지 않음.
대신, google/ kakao/ naver 마다 json 반환 값이 달라서 이를 변환해주는 저의 로직이 있는데 이 로직이 틀렸는지를 검사하기 위해(추후 리팩토링 할 때 빠른 검사 위함.) 이 부분에 한정해 단위테스트는 작성 하였음
(3) 테스트 코드 중복을 덜기위한 유틸리티 클래스 제공
매번 테스트 마다 유저는 필수적이라 (특히 통합테스트에 ) 여러 유저를 반복적으로 작성해야 하는 번거로움이 보였습니다.
그래서 빌더 패턴을 살짝 응용해서 임의의 n명의 유저를 생성하는 UserEntityBuilder 유틸리티 클래스를 제공합니다.
실제 application 부분에는 allow-ips: "${allow-ips}" 로 작성하였음 .