-
Notifications
You must be signed in to change notification settings - Fork 8
refactor: 요청의 인증 정보를 컨트롤러에서 원하는 형태로 받을 수 있도록 #171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- 클래스 자체는 많아졌지만, 이 코드를 처음 보는 개발자도 쉽게 이해할 수 있도록, 스프링 시큐리티의 표준 스펙에 맞게 개발하였음
- 설정(configuration)인 것들과 아닌 것들 분리
- 서비스 코드가 더이상 어떤 정보가 token 의 subject인지 몰라도 되도록 결합도를 낮춤
|
영서님이 전에 보여주신 그림을 보면서 보니 코드가 완전 잘읽히네요! 시큐리티 구조가 이번에 아주 잘 이해된 거 같습니다 😃
테스트코드 작성하면서 이부분이 늘 걸렸었는데 엄청 고생하셨을 거 같습니다... 저는 처음에 JwtAuthentication에서 만료여부 ture/false로만 해도 될 거 같다고 생각해서 JwtAuthentication와 ExpiredTokenAuthentication로 나누어진 게 궁금해서 고민을 하며 전부 구현한 코드를 살펴보았는데 @ExpirationIgnored | 만료된 토큰도 허용 | 로그아웃, 토큰 재발급 이렇게 안하면 추가 로직을 구현하거나 토큰이 만료되면 로그아웃을 못하는 일이 생기기도 했겠네요 😰 |
Gyuhyeok99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다!
...ava/com/example/solidconnection/custom/security/provider/SiteUserAuthenticationProvider.java
Outdated
Show resolved
Hide resolved
...java/com/example/solidconnection/custom/security/userdetails/SiteUserDetailsServiceTest.java
Outdated
Show resolved
Hide resolved
.../java/com/example/solidconnection/custom/security/authentication/SiteUserAuthentication.java
Outdated
Show resolved
Hide resolved
|
규혁님에 위에 "탈퇴"라는 말을 언급해주셔서.. |
|
ExpiredToken에 대한 인증 처리자를 따로 구현해서 활용한것이 흥미롭네요. Spring Security를 잘 알지 못했는데 공부가 많이 되네요. siteUserDetailsService.loadUserByUsername(username) 에서 username이 nickname을 말하는 것인줄알아 순간 혼동이 있었는데, userId를 의미하는 것이였군요. userIdentifier이나 userId대신 username이라는 명명을 사용하시게 된 이유나 참고자료등이 있을까요? |
커스텀 UserDetails 는 UserDetails 인터페이스를 구현해야하고, |
- 이전에는 siteUser.email로 조회를 해와서 DB에 있는 내용이 바로 반영된 SiteUser를 대상으로 했기 때문에 siteUser.getGpaScoreList 나 siteUser.getLanguageTestScoreList를 했을 때 조회가 되었었다. - 하지만 SiteUser 객체 자체를 넘기도록 테스트가 바뀌었고, 테스트 코드에서 Transactional 사용하는 것을 지양하기 위해서 양방향을 명시적으로 걸어주도록 코드를 수정했다.
| * - 관심 국가와 지역은 site_user_id를 참조하므로, 사용자 저장 후 저장한다. | ||
| * - 바로 로그인하도록 액세스 토큰과 리프레시 토큰을 발급한다. | ||
| * */ | ||
| // todo: 여러가지 가입 방법 적용해야 함 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 PR에서는 SignUpService 나 SignInService 같은 코드들을 가입의 종류가 다양하도록 바꾸진 않았어요.
그래서 일단 카카오 인증만 다루면서 함수 이름이 signUp인 등.. 만족스럽지 않은 부분들이 있긴 한데,
이 부분은 다른 가입 방식을 추가하며 구현해보겠습니다.
| Optional<SiteUser> findByEmailAndAuthType(String email, AuthType authType); | ||
|
|
||
| boolean existsByEmail(String email); | ||
| boolean existsByEmailAndAuthType(String email, AuthType authType); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이제 email 로만 식별하는 함수들은 다 없앴습니다😊
wibaek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
|
||
| public String generateToken(String email, TokenType tokenType) { | ||
| Claims claims = Jwts.claims().setSubject(email); | ||
| public String generateToken(SiteUser siteUser, TokenType tokenType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오버로딩을 하신건 아직 email을 사용하는 로직이 남아있어서 그런건가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아뇨 가입 시 발급하는 토큰을 만들 때, String을 인자로 받아서 그랬습니다. 이 부분 헷갈릴 수 있으니 분리하는게 좋겠네요! 짚어주셔서 감사합니다😊
작업 내용
슬랙으로 대화나눈 내용을 구현했습니다.
1/ SprintSecurity 규격을 지키며 개발
전에는 AuthenticationManager 나 AuthenticationProvider 를 생략했었는데요,
처음 보시는 개발자도 스프링 시큐리티의 흐름을 따라서 코드를 이해할 수 있도록 SprintSecurity의 표준 규격을 따라 개발했습니다.
전보다 클래스는 늘어났지만, 이편이 장점이 많다 생각합니다.
책임이 잘 나뉘어졌고, 테스트를 하기도 더 쉬워졌습니다 👍
2/ jwt 에 어떤 정보가 담기며, 어떻게 사용자를 식별하는지를 응집도 있게 변경
'가입 방식이 다르면 같은 이메일로도 가입할 수 있다'는 정책 변경으로 인해,
email 을 통해서 사용자를 식별하는 코드들을 모두 바꾸게 되었습니다.
이 변경이 공포스러운 이유는 소스코드 전체에 걸쳐서 이메일로 사용자를 식별하는 코드들이 있기 때문입니다🙀
이 PR에서는 다시 정책이 변경되더라도 코드 변경의 최소화될 수 있게 구현해봤습니다.
jwt 토큰의 subject에 있는 값이 siteUserId 라는 것을 아는 곳을 아래 두곳으로 한정했습니다.
그리고 argumenrResolver 를 사용해서, 컨트롤러에서는 이미 인증된 사용자만을 받아오도록 구현했습니다.
이제 컨트롤러에서 [Principal 에 저장된 name 이 사용자의 email 이다] 라는 보안과 관련된 내용이 사라지게 될 것입니다!!🚀