Skip to content

Conversation

@nayonsoso
Copy link
Collaborator

관련 이슈

작업 내용

이메일로 인증하는 것을 구현했습니다.
지금은 아주 엄청난 날림코드입니다... 🌪️
그래도 동작합니다...

특이 사항

제가 처음에 헷갈렸던 부분이 있었는데,

  • auth/email/sign-in 이 이메일, 비밀번호를 받고 엑세스 코드와 리프레시 코드를 주는 api
  • auth/email/sign-up 이 이메일, 비밀번호를 받고 signUpToken 을 주는 api 입니다.

그런데 마치 auth/email/sign-up가 이메일로 회원가입한다는 의미라고 오해하기 쉬운 것 같습니다.
api 들에 대해서 여러분들은 어떻게 생각하는지 리뷰로 듣고 싶습니다!!

리뷰 요구사항 (선택)

@nayonsoso nayonsoso self-assigned this Feb 7, 2025
Comment on lines +1 to +2
ALTER TABLE site_user
ADD COLUMN password VARCHAR(255) NULL;
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_deleted 스크립트 머지 된 후를 기준으로 생각해서
버전을 V5 로 했습니다~

Copy link
Member

@wibaek wibaek left a comment

Choose a reason for hiding this comment

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

SignUpService 템플릿 메소드 패턴을 이용한 추상화 훌륭합니다~👍

영서님 PR은 보기가 편한 것 같네요. lgtm.


public String generateAndSaveSignUpToken(String email, String password) {
String encodedPassword = passwordEncoder.encode(password);
Map<String, Object> passwordClaims = new HashMap<>(Map.of(PASSWORD_CLAIM_KEY, encodedPassword));
Copy link
Member

Choose a reason for hiding this comment

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

encodedPassword 를 토큰에 넣는 것은 비록 해싱되어 있다고 해도 위험할 것 같습니다.

oauth용의 SignUpTokenProvider에서는

Map<String, Object> authTypeClaim = new HashMap<>(Map.of(AUTH_TYPE_CLAIM_KEY, authType));

로 authType만을 담아내고 있는 것으로 확인했습니다.

Copy link
Member

Choose a reason for hiding this comment

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

아 비밀번호 정보를 서버에 저장하지 않고 토큰에 저장해둔 후 최종가입시에 사용하는 거군요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

네 저도 이 부분이 조금 애매하더라고요.. 😥
그래서 이메일 회원가입은 아예 다른 로직을 타게 해야하나? 라는 생각도 들었어요.
signUpToken 없이 이메일과 비밀번호를 프로필 정보와 함께 입력하는 식으로요.
어떻게 생각하시나요?

Copy link
Member

@wibaek wibaek Feb 8, 2025

Choose a reason for hiding this comment

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

프론트에선 코드를 공유할 수 있게 하나로 통합하는게 좋긴 합니다. 레디스등에 임시로 저장하는건 구현이 어려울까요?

Copy link
Member

Choose a reason for hiding this comment

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

추가적으로 말씀드리자면 저는 장기적으로 이메일/비밀번호 저장, OAuth 리소스 서버에서 정보 받아오기 등의 인증 절차는 최초 토큰 발급시에 실행하고, 이때 토큰 발급과 동시에 임시계정을 DB에 생성하는쪽으로 변경하는게 좋을 것 같습니다. 그리고 현재의 최종가입 절차는 닉네임/프사/선호국가 등록등의 절차로만 남겨 인증과 관련된 요소들은 제거하는 것입니다.

이러면 비밀번호 문제를 쉽게 해결할 수 있습니다. 또한 OAuth 서버에서 최초로 가져오는 정보들을 손실없이 저장할 수 있습니다.

예를들어 애플 로그인의 경우에는 '최초 인증시에만 들어오는 정보'가 있습니다. 이런 정보들을 저희는 임시토큰에 넣어서 최종가입시에 사용할텐데요, 이럴경우 첫번째에 인증만하고 최종가입을 진행하지 않는다면 문제가 생길 수 있습니다. 두번째로 인증을 하고 최종가입을 한다면 이 '최초가입시의 정보'를 받을 수 없을테니까요.

Copy link
Collaborator Author

@nayonsoso nayonsoso Feb 8, 2025

Choose a reason for hiding this comment

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

오 위백님은 더 하고 싶은 말씀이 있으신가보군요!
좋습니다! 디스커션

Copy link
Member

@wibaek wibaek Feb 8, 2025

Choose a reason for hiding this comment

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

앗 아뇨 곰곰히 생각해보니 현재 임시 토큰 방식도 괜찮을 것 같습니다. 생각했던 내용은 디스커션에 남겨두겠습니다.

그래서 제 생각에는 SignUpToken는 순수하게 “임시 토큰”으로 사용하고,
SignUpToken과 인증 정보(email, pwd)를 매핑해서 우리 서버에 저장하는게 더 좋은 방법 같습니다.

그러면 해당 방식으로 해싱된 비밀번호를 토큰에 포함하지 않고 redis에 저장해서 사용하는게 가능할까요? 해싱만 믿고 비밀번호가 노출되어 있는건 문제가 된다 생각해서요.

이 경우에 토큰의 stateless한 특성을 다소 포기하는 느낌이라 아쉽긴하네요. 하지만 이전에 토큰에 email을 저장하고 토큰을 파싱해 인증을 하려 했던 것에서 -> 매번 디비에 접근해서 인증을 하는 것으로 바꾸었듯이 타협가능한 정도의 트레이드오프라고 생각합니다.

Copy link
Member

Choose a reason for hiding this comment

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

아니면 jjwt 라이브러리에도 jwe 구현이 있으니 암호화하는 방식을 사용할 수도 있을것 같네요!

https://github.com/jwtk/jjwt?tab=readme-ov-file#jwe-example

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그러면 해당 방식으로 해싱된 비밀번호를 토큰에 포함하지 않고 redis에 저장해서 사용하는게 가능할까요? 해싱만 믿고 비밀번호가 노출되어 있는건 문제가 된다 생각해서요.

네 가능합니다!
jwe 구현도 좋은데 민감한 정보를 토큰에 넣는 다는 것 자체가 조금 위험해 보여서
"SignUpToken는 순수하게 “임시 토큰”으로 사용하고,
SignUpToken과 인증 정보(email, pwd)를 매핑해서 우리 서버에 저장" 하는 방식으로 구현해볼게요~

Copy link
Member

Choose a reason for hiding this comment

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

네 감사합니다! 🙇


public record EmailSignUpTokenRequest(

@NotBlank(message = "이메일을 입력해주세요.")
Copy link
Member

Choose a reason for hiding this comment

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

@Email 나 regex로 이메일 여부 검증해줘도 좋을 것 같습니다.

Copy link
Collaborator Author

@nayonsoso nayonsoso Feb 8, 2025

Choose a reason for hiding this comment

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

반영했습니다😊

36575a3

import static com.example.solidconnection.custom.exception.ErrorCode.USER_NOT_FOUND;

/*
* 보안을 위해 이메일과 비밀번호 중 무엇이 틀렸는지 구체적으로 응답하지 않는다.
Copy link
Member

Choose a reason for hiding this comment

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

해킹 방지를 위해 해당 계정이 존재하는지 알리지 않는 것도 좋은 것 같네요. 다만 회원가입시에 중복가입이 불가능한 걸 알려주기에 다소 복잡하지만 해커가 계정 존재여부를 체크할 수 있을 것 같습니다.

반대로 이메일이 틀렸는지 비밀번호가 틀렸는지 알려주면 일반 사용자에게는 더 좋은 UX로 다가올 수 있을 것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

반대로 이메일이 틀렸는지 비밀번호가 틀렸는지 알려주면 일반 사용자에게는 더 좋은 UX로 다가올 수 있을 것 같습니다.

이 말씀은 "에러 메세지에서 다르게 응답하면 좋겠다는 말씀이 맞나요?"

Copy link
Member

Choose a reason for hiding this comment

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

네 그렇습니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

저는 보안상 에러메시지를 통일하는 게 좋다고 생각합니다!

네이버 카카오 구글은 어떻게 되어있나 보았는데 네이버, 카카오는 응답메시지를 통일해서 주고, 구글의 경우는 이메일을 먼저 입력하는데 존재하는 이메일을 입력해야지만 비밀번호 입력으로 넘어가네요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이메일 X 비밀번호 X → 이메일과 비밀번호를 확인해주세요
이메일 X 비밀번호 O → 이메일과 비밀번호를 확인해주세요
이메일 O 비밀번호 X → 비밀번호를 확인해주세요

이렇게는 어떤가요?
지금 비밀번호 찾기 기능이 없으니, 이메일만 맞았을 경우에 "거기까지는 맞았다~" 힌트를 주는 것도 괜찮을 것 같아요!

Copy link
Member

Choose a reason for hiding this comment

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

음 저도 이메일 비밀번호 틀린요소에 관계없이 같은 메시지를 받는 것이 익숙하고 이것이 좋은 패턴이라 생각하는데요, 다만 현재 구조에서는 회원가입 부분이 뚫려있기에 해커 입장에서 어짜피 계정 유무를 판별할 수 있다고 판단해서 이런 건의를 드렸습니다.

다만 이후에 회원가입에 이메일 발송 인증이 들어가면 이 부분은 안전하게 되고, 동일한 메시지가 반환되는 것이 좋다고 생각하기에 해당부분은 수정하지 않아도 괜찮을 것 같습니다!

return ResponseEntity.ok(signInResponse);
String signUpToken = emailSignUpTokenProvider.generateAndSaveSignUpToken(signUpRequest);
return ResponseEntity.ok(new EmailSignUpTokenResponse(signUpToken));
}
Copy link
Member

Choose a reason for hiding this comment

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

현재 구조로는 만약 이미 가입된 유저가 다시 가입을 시도 했을 때, 닉네임/프사/선호지역/등등... 을 모두 선택하고 가입하려는 순간! 이메일 중복 오류가 발생해서 허무하게 될 것 같은데요, 첫 이메일 토큰 발급시에 이메일 중복여부를 체크해서 알려주는 건 어떻게 생각하시나요?

Copy link
Collaborator Author

@nayonsoso nayonsoso Feb 8, 2025

Choose a reason for hiding this comment

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

ㅋㅋㅋㅋ 그렇겠네요! 반영했습니다~

8e800b9

@nayonsoso
Copy link
Collaborator Author

nayonsoso commented Feb 8, 2025

✍️ api docs 변경사항

https://github.com/solid-connection/api-docs/pull/3

이 PR 과 함께 머지할 예정입니다!

Copy link
Contributor

@Gyuhyeok99 Gyuhyeok99 left a comment

Choose a reason for hiding this comment

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

지난번부터 느꼈는데 추상화를 정말 잘 활용하시네요 👍

Comment on lines +51 to +52
validateUserNotDuplicated(signUpRequest);
validateNicknameDuplicated(signUpRequest.nickname());
Copy link
Contributor

Choose a reason for hiding this comment

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

나중에 이 부분도 DTO에서 어노테이션으로 검증하도록 해도 되겠네요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이 부분은 레포지토리와의 상호작용이 있어야 검증되는거라 서비스 코드가 맞다 생각해요!
dto 검증은 형식에 해당하는 것만 검증하도록 제한해야 할 것 같습니다~

Comment on lines +11 to +12
@NotBlank(message = "비밀번호를 입력해주세요.")
String password
Copy link
Contributor

Choose a reason for hiding this comment

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

나중에 비밀번호 조건 검증도 추가하면 좋겠네요!

@nayonsoso nayonsoso merged commit 0adbb4c into main Feb 8, 2025
@nayonsoso nayonsoso deleted the feat/140-email-auth branch February 15, 2025 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

이메일 로그인 추가

4 participants