Skip to content
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

#1 회원가입 기능 구현 시큐리티설정 작업 #8

Merged
merged 10 commits into from
Jan 14, 2025

Conversation

rlawltjd8547
Copy link
Collaborator

@rlawltjd8547 rlawltjd8547 commented Nov 19, 2024

기본적인 회원가입/로그인 동작 코드 만들기

  1. 기본적인 형태로만 만들어보자는 생각으로 진행하였고 예외,검증,테스트코드 등의 세부로직은 구현하지않은 상태
  • 회원가입
    회원가입에서는 기본적인 데이터를 받은뒤 비밀번호 암호화 한 이후 DB에 저장되는 형식으로 구현 목표

  • 로그인
    로그인은 시큐리티작업을 통하여 진행하였으나 폼데이터방식을 사용하지 않고 JSON데이터를 받게끔 설정을 바꿔야함


비밀번호의 암호화는 Bcrypt방식으로 진행

Copy link
Collaborator

@f-lab-maverick f-lab-maverick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

관련내용은 멘토링 스크립트를 참조해주세요. 고생하셨습니다.

Copy link
Collaborator Author

@rlawltjd8547 rlawltjd8547 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

예전에 진행한 기본적인 test코드 작성해보았을때랑은
테스트코드의 작성이 조금 힘들어서 진행중에 시간이더 걸릴것같아 그냥 푸쉬를 해놓은상태입니다
확실히 조금 고민하면서 진핸해보니
명확하지는않지만 느낌이 아 왜 validation은 따로 유틸로 묶고
왜 exception을 따로 나누어 클래스로 관리하는지 알것같은기분이 듭니다

무엇보다 진행하다보니 아 왜 이슈를 깔끔하게 정리를 해야하는지도 생각이 드네요

지금은 다시 재정리를 해야할시간이 온것같습니다 차주 목요일 멘토링전까지
제 프로젝트를 전체적으로 좀 정리도 먼저 진행해야할것같습니다
다시 돌아보니 지금상태는 너무 빵꾸가 많은것같아요

  1. 테스트코드 작성을 어떻게해야할것인가
  • 전에는 그냥 기본적인 기능을 테스트코드를 만들었다면 이번에는 하나의 흐름으로 되어진
    모듈이라고할까요? 그러다보니 service를 바라보고 테스트를 해야할지 controller-service-repo 의 전체적인 테스트를 진행해야하는지
    또 회원가입의 경우 실행로직인 service를 진행할때 그러면 비교군인 repo를통해 값을 가져와야하는데 흠.. 이런 고민들이 많았습니다 이부분은 좀더 고민하면서 작성해야할듯싶습니다
  1. validator는 따로 만드는게 맞다고 생각이 듭니다
    생각보다 시스템에서 요구하는 조건들이 많다보니 이걸 관리하지않으면 어느순간 제가 컨트롤할수없을것같다는 생각이 들어서 미리 만들어야한다는 생각이 들었습니다
    이부분은 제가 요구사항을 제대로 정리하지않은 업보 가 여기서 느껴졌네요
    아부분은 요구사항을 좀더 깊게 고민하면서 같이 작성해보도록하겠습니다

  2. Exception은 따로 관리하여야하는가?
    이부분도 validation부분과 마찬가지로 솔직히 아직 크게 느껴지지않고 지금은 그냥 따로 관리해도될것같다는 생각이 큰데
    validation을 고민하는도중에 아 이부분도 지금은 아니지만 나중에 후회할거같은데..라는생각이 들어서..ㅋㅋㅋ
    근데 여기도 제가 요구사항정리가 너무 빈약한 부분의 업보인것 같습니다
    이것 역시 요구사항 즉 이슈부분 정리를 대대적으로 하면서 보완을 해야할것같습니다

그냥 지금은 시큐리티 부분만 봐주시면 좋을것같습니다
하면서 느끼네요 부족한 부분들이 허허....

Copy link
Collaborator

@f-lab-maverick f-lab-maverick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spring Security의 옵션 하나하나는 정말 큰 영향을 미치기때문에, 정확하게 알고 사용하셨는지 질문을 남겼습니다.
추가적으로 코드 컨벤션관련 리뷰를 해두었으니 확인 후 수정해주세요. 고생하셨습니다.

Comment on lines +13 to +17
private final Member member;

public CustomUserDetails(Member member) {
this.member = member;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

불변선언 좋습니다 👍

@Bean
public SecurityFilterChain filterChain(HttpSecurity http) throws Exception { //final HttpSecurity http 이점이있는가?
http
.csrf((auth) -> auth.disable()) // csrf 비활성화 (REST API등 비상태 통신에서는 CSRF토큰이 필요하지 않을수있다)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSRF 토큰은 어떤 역할을 할까요?
REST API등 비상태 통신에서는 이걸 disabled하는 이유는 어떤게 있을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSRF는 공격자가 원하지않는 요청을 실행하는 취약점입니다
즉 사용자가 로그인된상태(인증된 세션)에서 공격자가 사용자 몰래 특정요청을 보내는 공격입니다
여기서 CSRF토큰의 역할은 "서버가 클라이언트에게 발급하는 일회성 난수값"으로
CSRF공격을 방지하기위해 요청에 함께전송되어집니다
그래서 서버는 해당 토큰이 유효한지 확인함으로 CSRF공격을 방지하는 역할을 합니다
여기서 REST API (Stateless)방식에서 CSRF를 비활성화 하는 이유는
REST API의 경우는 서버가 클라이언트의 상태를 유지하지않고
CSRF는 주로 세션기반 인증에서 발생하는 공격이기때문에
REST API는 주로 JWT나 OAuth토큰등 쿠키를 사용하지 않는 인증 방식을 사용하기에 dissable을 합니다

진행하고 있는 프로젝트가 만약 실제 클라이언트를 위한 서비스로 진행한다면
CSRF는 dissable하지 말아야할것이라 생각합니다

public SecurityFilterChain filterChain(HttpSecurity http) throws Exception { //final HttpSecurity http 이점이있는가?
http
.csrf((auth) -> auth.disable()) // csrf 비활성화 (REST API등 비상태 통신에서는 CSRF토큰이 필요하지 않을수있다)
.httpBasic((auth) -> auth.disable()) // httpBasic 비활성화
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 옵션을 끈 이유가 있을까요?
Spring Security의 httpBasic 옵션은 어떤 역할을 하나요?

아래 문서를 참고해서 답변해보세요~

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

httpBasic옵션은
웹사이트에 로그인 할때 아이디/비밀번호를 매번 입력한다고 가정하고
서버에 요청을 보낼때마다 아이디와 비밀번호가 따라가는 방식입니다
여기서문제는
아이디와 비밀번호에서 비밀번호가 그저 암호화가아닌 base64로 바꾼형태로 노출이된다는게 큰 문제입니다
에초에 요청에 아이디와 비밀번호 정보가 매번 담기는게 위험하기때문에
보안을 위해 Http Basic인증을 끄고
세션이나 JWT(토큰)방식으로 로그인한후 세션이나 토큰값만 보내는 방식을 사용하기위함입니다

사용자가 로그인하거나 인증할 때 기존 세션을 폐기하고 새로운 세션을 생성합니다.
이로 인해 기존 세션 ID가 무효화되며, 세션 고정 공격을 방지합니다.
*/
.sessionFixation(SessionManagementConfigurer.SessionFixationConfigurer::newSession)//세션고정공격방지
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

세션 고정공격에 대한 방지는 기본값인 migrateSession 으로도 동작합니다.
newSession 옵션으로 설정하면 기존 세션을 무효화하는데요, 이 설정은 어떤 장/단점이 있을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

우선 세션고정 공격이라는것은
로그인시 발급받은 세션ID가 로그인 전/후 모두 동일하게 사용되어 악의적인 사용자가 피해자의 세션을 하이제킹하여 정상적인 사용자로 위장하여 접근하는 행위입니다
(처음에는 사용자가 로그인하여 발급받은 세션아이디를 공격자가 탈취하여 사용하는 방식인줄알았으나)
세션고정 공격의 요점은 공격자가 먼저 사이트에 접속하여 세션아이디를 받은후
해당 세션아이디를 피해자에세 심는것이 요점입니다

1.migrateSession은 인증후에는 새로운 세션 ID를 발급하지만 기존 세션에 담긴 데이터는 유지되는것이 특징입니다 (ex:장바구니, 폼입력데이터)
세션ID가 인증후 변경되기때문에 세션고정공격에 방지가 되지만
데이터가 남아있을 위험이 존재합니다

  1. newSession의 경우는 인증후 완전히 새로운 세션을 생성하고 기존 데이터를 폐기합니다 즉 세션ID뿐만 아니라 세션자체가 새롭게 초기화됩니다
    그러면 보안성이 높아지는데 단점으로는 데이터가초기화되기때문에 사용자가 불편함을 느낄가능성이있습니다
    이런경우는 보통 금융권이나 보안성이 높은 요구사항인 경우에 사용해야합니다

현재 프로젝트는 기본적인 게시판 즉 폼데이터를 사용할것이라 예측되기때문에
newSesion보다는 migrateSession이 조금더 적합해 보인다고생각합니다
예를들어 글쓰기 작성중에 내용을 저장해야할수도 있을 가능성이 있기때문입니다

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 내용 [newSesion보다는 migrateSession이 조금더 적합] 이 아직 적용안된것으로 보입니다. 적용해주세요~

Comment on lines 11 to 24
@Service
@RequiredArgsConstructor
public class UserDetailService implements UserDetailsService {

private final MemberRepository memberRepository;

//로그인시 유저를 찾고 유저가 있으면 CustomUserDetails를 반환
@Override
public CustomUserDetails loadUserByUsername(String userId) throws UsernameNotFoundException {
Member member = memberRepository.findMemberById(userId)
.orElseThrow(() -> new UsernameNotFoundException("해당하는 사용자가 없습니다."));
return new CustomUserDetails(member);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

깔끔하게 잘 구현하셨네요 👍

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 클래스는 '패스워드 인코더', 'Bcrypt 인코딩` 두가지 역할을 동시에 수행하고있습니다.
하나의 클래스는 하나의 역할만 하도록 만들어야 결합도를 낮추고 추후 변경사항을 유연하게 적용시킬 수 있습니다.

예를 들어, 현재 구조에서 특정일까지 가입한 유저는 모두 SHA암호화를 쓰며 / 이후에는 Bcrypt 암호화를 써야한다는 요구사항이 발생할경우 반영하기 어렵습니다.

두가지 역할의 클래스를 분리해주세요.

Comment on lines 27 to 37
@Override
public String encode(CharSequence rawPassword) {
return org.springframework.security.crypto.bcrypt.BCrypt.hashpw(rawPassword.toString(),
org.springframework.security.crypto.bcrypt.BCrypt.gensalt(WORK_FACTOR));
}

//PasswordEncoder를 상속받아서 구현해야하는 메소드
@Override
public boolean matches(CharSequence rawPassword, String encodedPassword) {
return org.springframework.security.crypto.bcrypt.BCrypt.checkpw(rawPassword.toString(), encodedPassword);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

열심히 만들어둔 encrypt / isMatch 메서드를 사용하지 않고 별도 유틸을 사용하고있네요~

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

주석처리를 하지 말고 파일을 삭제해주세요. 파일 기록이 git상에 남기 때문에 복구가 필요하다면 git에서 복구하시면 되며, 주석으로 남겨두어도 실제 복구하는 경우가 많지 않기 때문에 정리해주시는게 좋습니다.

Comment on lines 27 to 42
@Test
@DisplayName("회원가입 테스트")
void signUp() {
// given
String rawPwd = "1234";
String encodingPwd = bcryptEncoding.encrypt(rawPwd);
SignupRequest signupRequest = new SignupRequest("test", "1234", "김현성", LocalDate.now());

// when
memberService.signUp(signupRequest);
String actual = String.valueOf(memberRepository.findMemberById("test"));
// then
assertEquals(encodingPwd, bcryptEncoding.encrypt(rawPwd));
assertThat(actual.getId()).isEqualTo(signupRequest);

}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 테스트는 어떤것을 의도하신건가요?

  1. 테스트 DisplayName에 명확하게 어떤 조건에서 어떤걸 테스트하는지 드러나지 않습니다
  2. 테스트 대상이 명확하지 않고, Mock이라는 테스트더블을 사용했기 때문에 기대하신 값이 나오지 않을거에요.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

비어있는 테스트파일은 git commit시 제외시켜주시거나 삭제해주세요~

public interface MemberRepository {
void save(Member member);
Optional<Member> findMemberById(String user_id); // DetailsService에서 orElseThrow때문에 Optional사용
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional을 처음 써보아서 어색합니다만 제가 아는 정보는 Nullpoint 안전성을 보장하기위한 용도로 사용한다고 합니다만
제가 이걸 쓴 큰이유는 주석과 마찬가지로 orElseThrow때문에 어쩔수없이 사용을 한것입니다
그래서 이걸 security구현시에는 Optional이 필수인가? 라는 의문이들어 코멘트 남겨봅니다!

Copy link
Collaborator

@f-lab-maverick f-lab-maverick Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional은 사용하지 않으셔도 됩니다. 함수형 프로그래밍 또는 소스코드를 좀 더 깔끔하게 만들고 싶을 때 사용할 수 있는 옵션이라고 생각해주셔도 좋습니다.

다음 두 코드는 같은 로직입니다. Optional을 사용할 경우 변수 선언을 한번이라도 덜 할 수 있으며, 메서드 체이닝을 통해 좀 더 깔끔하게 짤 수 있기 때문에 코드 정리를 할 때는 유용하게 쓸 수 있습니다.

1- Optional 사용 안함

String getMemberName() {
  Member member = memberRepository.findById("test");
  if (member == null) {
    return "AnonymousUser"
  }
  return member.getId();
}

2- Optional 사용함

String getMemberName() {
  return memberRepository.findById("test") // Optional Return
    .map(Member::getId)
    .orElse("AnonymousUser");
}

@Bean
public SecurityFilterChain filterChain(HttpSecurity http) throws Exception { //final HttpSecurity http 이점이있는가?
http
.csrf((auth) -> auth.disable()) // csrf 비활성화 (REST API등 비상태 통신에서는 CSRF토큰이 필요하지 않을수있다)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSRF는 공격자가 원하지않는 요청을 실행하는 취약점입니다
즉 사용자가 로그인된상태(인증된 세션)에서 공격자가 사용자 몰래 특정요청을 보내는 공격입니다
여기서 CSRF토큰의 역할은 "서버가 클라이언트에게 발급하는 일회성 난수값"으로
CSRF공격을 방지하기위해 요청에 함께전송되어집니다
그래서 서버는 해당 토큰이 유효한지 확인함으로 CSRF공격을 방지하는 역할을 합니다
여기서 REST API (Stateless)방식에서 CSRF를 비활성화 하는 이유는
REST API의 경우는 서버가 클라이언트의 상태를 유지하지않고
CSRF는 주로 세션기반 인증에서 발생하는 공격이기때문에
REST API는 주로 JWT나 OAuth토큰등 쿠키를 사용하지 않는 인증 방식을 사용하기에 dissable을 합니다

진행하고 있는 프로젝트가 만약 실제 클라이언트를 위한 서비스로 진행한다면
CSRF는 dissable하지 말아야할것이라 생각합니다

public SecurityFilterChain filterChain(HttpSecurity http) throws Exception { //final HttpSecurity http 이점이있는가?
http
.csrf((auth) -> auth.disable()) // csrf 비활성화 (REST API등 비상태 통신에서는 CSRF토큰이 필요하지 않을수있다)
.httpBasic((auth) -> auth.disable()) // httpBasic 비활성화
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

httpBasic옵션은
웹사이트에 로그인 할때 아이디/비밀번호를 매번 입력한다고 가정하고
서버에 요청을 보낼때마다 아이디와 비밀번호가 따라가는 방식입니다
여기서문제는
아이디와 비밀번호에서 비밀번호가 그저 암호화가아닌 base64로 바꾼형태로 노출이된다는게 큰 문제입니다
에초에 요청에 아이디와 비밀번호 정보가 매번 담기는게 위험하기때문에
보안을 위해 Http Basic인증을 끄고
세션이나 JWT(토큰)방식으로 로그인한후 세션이나 토큰값만 보내는 방식을 사용하기위함입니다

사용자가 로그인하거나 인증할 때 기존 세션을 폐기하고 새로운 세션을 생성합니다.
이로 인해 기존 세션 ID가 무효화되며, 세션 고정 공격을 방지합니다.
*/
.sessionFixation(SessionManagementConfigurer.SessionFixationConfigurer::newSession)//세션고정공격방지
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

우선 세션고정 공격이라는것은
로그인시 발급받은 세션ID가 로그인 전/후 모두 동일하게 사용되어 악의적인 사용자가 피해자의 세션을 하이제킹하여 정상적인 사용자로 위장하여 접근하는 행위입니다
(처음에는 사용자가 로그인하여 발급받은 세션아이디를 공격자가 탈취하여 사용하는 방식인줄알았으나)
세션고정 공격의 요점은 공격자가 먼저 사이트에 접속하여 세션아이디를 받은후
해당 세션아이디를 피해자에세 심는것이 요점입니다

1.migrateSession은 인증후에는 새로운 세션 ID를 발급하지만 기존 세션에 담긴 데이터는 유지되는것이 특징입니다 (ex:장바구니, 폼입력데이터)
세션ID가 인증후 변경되기때문에 세션고정공격에 방지가 되지만
데이터가 남아있을 위험이 존재합니다

  1. newSession의 경우는 인증후 완전히 새로운 세션을 생성하고 기존 데이터를 폐기합니다 즉 세션ID뿐만 아니라 세션자체가 새롭게 초기화됩니다
    그러면 보안성이 높아지는데 단점으로는 데이터가초기화되기때문에 사용자가 불편함을 느낄가능성이있습니다
    이런경우는 보통 금융권이나 보안성이 높은 요구사항인 경우에 사용해야합니다

현재 프로젝트는 기본적인 게시판 즉 폼데이터를 사용할것이라 예측되기때문에
newSesion보다는 migrateSession이 조금더 적합해 보인다고생각합니다
예를들어 글쓰기 작성중에 내용을 저장해야할수도 있을 가능성이 있기때문입니다

Comment on lines 6 to 11
@Getter
@Setter
public class LoginRequest {
private String username;
private String password;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rawPassword 변수명 작성 하도록하겠습니다!

3. SPRING_SECURITY_CONTEXT는 SpringSecurity에서 사용하는 세션의 기본키이다
*/
SecurityContextHolder.getContext().setAuthentication(authentication);
session.setAttribute("SPRING_SECURITY_CONTEXT", SecurityContextHolder.getContext());
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

인증이 성고한뒤 해당 정보를 SecurityContextHolder에 저장하고난뒤
당연히 세션을 통한 로그인이기때문에 개발자가 session영역에도 저장을 해야된다는 생각에 직접 설정하였습니다

Spring Security가 기본적으로 컨텍스트에 저장한뒤 세션에 저장해주는 일이 존재하는걸 해당 코멘트를 보고나서 찾아보니 알게되었습니다....

중복작업이기때문에 해당 구문은 삭제하도록하겠습니다!

Copy link
Collaborator

@f-lab-maverick f-lab-maverick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이해도가 조금씩 올라가는게 보입니다.
아직 수정해야할 부분들이 많이 보입니다. 노력하시는만큼 코드 퀄리티와 이해도가 높아질 것입니다.
수정하신 후 다시 리뷰요청 해주세요. 고생 많으셨습니다.

사용자가 로그인하거나 인증할 때 기존 세션을 폐기하고 새로운 세션을 생성합니다.
이로 인해 기존 세션 ID가 무효화되며, 세션 고정 공격을 방지합니다.
*/
.sessionFixation(SessionManagementConfigurer.SessionFixationConfigurer::newSession)//세션고정공격방지
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 내용 [newSesion보다는 migrateSession이 조금더 적합] 이 아직 적용안된것으로 보입니다. 적용해주세요~

이로 인해 기존 세션 ID가 무효화되며, 세션 고정 공격을 방지합니다.
*/
.sessionFixation(SessionManagementConfigurer.SessionFixationConfigurer::newSession)//세션고정공격방지
.maximumSessions(1) // 동시세션수 제한 (하나의 사용자계정이 유지할수있는 세션의 수를 제한)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

동시세션수를 제한할 필요가 없다면, 해당 설정은 뺴는게 좋습니다.
유저가 여러 기기로 접속하는것도 막아버리거든요

Comment on lines 25 to 29
@PostMapping("/login")
public ResponseEntity<?> login(@Valid @RequestBody LoginRequest loginRequest) {
loginService.login(loginRequest);
return ResponseEntity.ok("Login successful");
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

로그인 서비스를 직접 구현하실 필요가 없습니다.
UsernamePasswordAuthenticationFilter를 커스텀하게 만들어서 등록해주면 직접 login 관련 설정을 진행하지 않고도 필터체인을 통해 인증처리가 가능합니다.

Custom UsernamePasswordAuthenticationFilter를 키워드로 검색해보시고 적용해주세요.

Comment on lines 31 to 36
@PostMapping("/logout")
public ResponseEntity<?> logout(HttpSession session) {
session.invalidate(); //로그아웃시에 세션을 무효화하여 저장된 모든 데이터를 삭제한다 "SPRING_SECURITY_CONTEXT"해당 키 삭제
SecurityContextHolder.clearContext(); //SecurityContextHolder의 인증정보를 삭제한다
return ResponseEntity.ok().build(); // build()는 바디없이 빈 응답을 생성한다
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

로그아웃또한 직접적으로 구현하지 않아도 됩니다.
이런 요청들은 모두 Spring Security의 설정을 통해 가능합니다. 시큐리티 설정에서 logout URL을 지정할 수 있으니, 설정을 찾아 적용해보세요.

@ExceptionHandler 어노테이션은 특정 예외가 발생했을 때 메소드가 처리하도록 하는 어노테이션이다
여기서는 BadCredentialsException 예외가 발생했을 때 handleBadCredentialsException 메소드가 처리하도록 한다
ProblemDetail은 Spring(6부터)에서 제공하는 클래스로 예외처리시 상태코드와 상세정보를 담아서 반환할 수 있다
기존방식으로 처리하게되면 ResponseEntity<Map<String, Object>> 형식으로 직접 관리를 해야하지만 ProblemDetail을 사용하면 편리하게 처리할 수 있다
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

첨언을 하자면 Map으로 Response를 관리하는것은 좋지 않은 코드작성 방식입니다.
Map이 아닌 DTO로 반환해야 API 스펙이 명시적으로 변하며, 동적으로 스펙이 변경되지 않습니다.

즉, ResponseEntity<DTO> 의 형태로 리턴해주는것이 더 적절합니다.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ProblemDetail을 사용해도 됩니다~ Map만 피해주세요.

Comment on lines 29 to 33
@ExceptionHandler(BadCredentialsException.class)
public ProblemDetail handleBadCredentialsException(BadCredentialsException e) {
return ProblemDetail.forStatusAndDetail(HttpStatus.UNAUTHORIZED, "아이디or비밀번호가 일치하지 않습니다");
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spring Security의 필터체인방식으로 로그인을 처리하게되면 이 핸들러는 더이상 동작하지 않게 됩니다. 필터체인이 Spring context 외부에 있기 때문이에요. 이 경우 적절한 Handler를 등록해주어서 response를 조작할 수 있습니다.

로그인 실패는 AuthenticationFailureHandler를 키워드로 찾아서 적용해보세요.

Comment on lines 17 to 40
public void login(LoginRequest loginRequest) {
/*
인증 요청
토큰이라고 해서 토큰방식을 사용하는것이 아니고
Spring Security에서 토큰 기반의 인증을 수행하는 객체이다
Spring Secutiry 내부 에서 사용자 인증 정보를 담기위한 객체이다
*/
UsernamePasswordAuthenticationToken token = new UsernamePasswordAuthenticationToken(loginRequest.getEmail(), loginRequest.getRawPassword()); //토큰생성
/*
1. authenticationManager가 Provider에게 인증을 위임
2. config에 설정한 내용대로 UserDetailsService를 통해 유저정보를 가져온다
3. 입력된비밀번호와 가져온정보의 비밀번호를 비교하여 인증에 성공하면 Authentication 객체를 생성하여 리턴
*/
Authentication authentication = authenticationManager.authenticate(token); // AuthenticationManager를 통해 인증을 시도한다
// 세션에 인증 정보 저장
/*
SecurityContextHolder는 SpringSecurity의 인증 정보를 저장하고 조회하는 컨텍스트
1. 인증이 성공하면 Authentication 객체를 SecurityContextHolder에 저장
2. 이후 클라인언트의 요청은 사용자 인증상태를 유지하게끔 한다
3. SPRING_SECURITY_CONTEXT는 SpringSecurity에서 사용하는 세션의 기본키이다
*/
SecurityContextHolder.getContext().setAuthentication(authentication);

}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 작업을 여기서하는게 아니라 Security설정에서 진행해주셔야 합니다. 위에 적어두었듯 커스텀 UsernamePasswordAuthenticationFilter 로 구현해주세요~

Comment on lines 9 to 20
@Getter //Jackson 라이브러리는 객체를 JSON으로 변환할 때 기본적으로 getter 메서드를 사용
public class SignUpResponse {
private final String email;
private final LocalDateTime createAt;
private final String message;

public SignUpResponse(Member member) {
this.email = member.getEmail();
this.createAt = LocalDateTime.now();
this.message = "회원가입이 완료되었습니다.";
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

회원가입이 완료되었을 때 굳이 response DTO가 필요하지는 않습니다.
성공은 200 OK 또는 201 응답으로 처리하면 되며, 실패에 대해서는 400번대 에러를 주면 됩니다.
꼭 응답 DTO가 Body를 통해 내려갈 필요는 없으니, 꼭 필요한 경우가 아니라면 HTTP Status로 처리해주세요.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

모든 클래스마다 이렇게 핸들러를 등록하기보다는, 예외에 대한 공통 Exception Handler를 두고 거기서 관리하시는게 더 낫지 않을까요? 기본 예외를 패키지마다 재활용하지말고, 필요하다면 세부적인 예외를 직접 만드는게 더 명시적입니다.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여전히 BcryptEncoderPassWordEncoder의 역할이 겹치고있습니다.
가능하다면 PassWordEncoder 하나만 사용해주시고, 동일한 역할의 인코더는 여러개 두지 않는게 혼동이나 코드중복을 줄일 수 있습니다.

Copy link
Collaborator

@f-lab-maverick f-lab-maverick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생많으셨습니다.
머지해주셔도 좋습니다.

@rlawltjd8547 rlawltjd8547 merged commit 52b3dd7 into main Jan 14, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants