-
Notifications
You must be signed in to change notification settings - Fork 8
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
이메일 회원가입, 로그인, 로그아웃 구현 #267
Conversation
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.
고생하셨습니다 제우스!!
몇 가지 간단한 리뷰와 질문사항 남겨두었습니다.
추가로 테스트 코드가 정말 깔끔한 것 같네요!!
추후에 다른 테스트 코드에도 FakeRepository를 적용해보도록 하겠습니다.
public class AuthWebConfiguration implements WebMvcConfigurer { | ||
|
||
private final AuthService authService; | ||
|
||
@Override | ||
public void addArgumentResolvers(List<HandlerMethodArgumentResolver> resolvers) { | ||
resolvers.add(new AuthArgumentResolver(authService)); | ||
} | ||
} |
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.
별 건 아니지만 궁금해서 질문 남깁니다!!
AuthArgumentResolver
를 component로 등록하지 않은 이유가 있나요? 등록한다면 아래와 같이 주입해서 사용할 수 있을 것 같아서요! component로 등록하면 단점이 생기는 건지 궁금합니다.
public class AuthWebConfiguration implements WebMvcConfigurer { | |
private final AuthService authService; | |
@Override | |
public void addArgumentResolvers(List<HandlerMethodArgumentResolver> resolvers) { | |
resolvers.add(new AuthArgumentResolver(authService)); | |
} | |
} | |
public class AuthWebConfiguration implements WebMvcConfigurer { | |
private final AuthArgumentResolver authArgumentResolver; | |
@Override | |
public void addArgumentResolvers(List<HandlerMethodArgumentResolver> resolvers) { | |
resolvers.add(authArgumentResolver); | |
} | |
} |
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.
레벨2에서 리뷰어가 참고하라고 주신 글인데요. 한 번 읽어보면 좋을 것 같아요.
현재 단계에서는 큰 차이가 발생하지 않지만, 프로젝트가 성장할수록 의존성 관리가 어려워지죠.
그래서 빈 등록이 주는 강력함도 있지만, 안 할 수 있다면 안 하는 편이 더 좋다고 생각합니다.
public record SignupRequest( | ||
@Schema(description = "이메일", example = "[email protected]") | ||
@Pattern(regexp = "^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\\.[a-zA-Z]{2,}$", message = "이메일 형식이 아닙니다.") | ||
@NotEmpty(message = "이메일이 입력되지 않았습니다.") |
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.
⚡
@NotBlank
를 사용하지 않고 @NotEmpty
를 사용하신 이유가 있나요?
@NotBlank
는 단순히 빈 문자열 뿐 아니라 공백까지 잡아줘서 @NotBlank
를 쓰면 더 좋지 않을까 해서 의견 남깁니다!
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.
제우스 수고하셨습니다 😊🍬🍭 코드가 전반적으로 깔끔하네요 !!
몇가지 부분만 코멘트 남겼습니다. 확인해주세요 😊
import lombok.Getter; | ||
import lombok.NoArgsConstructor; | ||
|
||
@Entity | ||
@Getter | ||
@NoArgsConstructor |
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.
⚡
@NoArgsConstructor | |
@NoArgsConstructor(access = AccessLevel.PROTECTED) |
@NoArgsConstructor의 기본 접근제어자를 찾아보니 public으로 설정 되어있네요. protected로 접근제어자를 좁히면 더 좋을 것 같습니다.
import codezap.member.domain.Member; | ||
|
||
@SuppressWarnings("unused") | ||
public interface MemberJpaRepository extends MemberRepository, JpaRepository<Member, Long> { |
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.
MemberJpaRepository 와 MemberRepository 두 개가 있는 이유가 있나요??
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[] credentials = decodeCredentials(authHeaderValue); | ||
String email = credentials[0]; | ||
String password = credentials[1]; |
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.
⚡ 배열이 2개의 String을 가지고 있음이 명확하지 않을 것 같아요.
email과 password를 배열에서 인덱스 정보로 꺼내기 전에 배열의 크기가 2가 맞는지 체크하는 로직이 있으면 좋을 것 같아요.
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.
어쩌면 decodeCredentials()
메서드가 일급 컬렉션, 또는 특정 책임을 가지는 객체를 반환해 주는 것도 방법이 될 수 있겠네요!
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.
⚡ 배열이 2개의 String을 가지고 있음이 명확하지 않을 것 같아요. email과 password를 배열에서 인덱스 정보로 꺼내기 전에 배열의 크기가 2가 맞는지 체크하는 로직이 있으면 좋을 것 같아요.
좋은 피드백 고마워요!
배열의 크기를 확인하는 대신, 인코딩 형식이 맞는지 확인하는 로직을 decodeCredentials
메서드에 추가했습니다.
이렇게 하면 배열의 크기가 2가 됨을 보장하면서도, Basic Auth credentials의 표현이 올바른지 동시에 검증할 수 있어요.
private String getAuthCookieValue(Cookie[] cookies) { | ||
return Arrays.stream(cookies) | ||
.filter(cookie -> Objects.equals(cookie.getName(), HttpHeaders.AUTHORIZATION)) | ||
.findFirst() | ||
.map(Cookie::getValue) | ||
.orElseThrow(this::throwUnauthorized); | ||
} | ||
|
||
private String[] decodeCredentials(String encodedCredentials) { | ||
byte[] decodedBytes = Base64.getDecoder().decode(encodedCredentials.getBytes(StandardCharsets.UTF_8)); | ||
String decodedString = new String(decodedBytes); | ||
return decodedString.split(":"); | ||
} | ||
|
||
private CodeZapException throwUnauthorized() { | ||
throw new CodeZapException(HttpStatus.UNAUTHORIZED, "인증에 실패했습니다."); | ||
} |
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.
이건 선호도의 문제인데
getAuthCookieValue
-> throwUnauthorized
-> decodeCredentials
순서로 메서드가 있으면 좋을 것 같아요.
getAuthCookieValue
에서 throwUnauthorized
을 사용하기 때문에 사용하는 메서드와 최대한 가깝게 메서드를 위치시키면 좋을 것 같아요.😊
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.
위에서 아래로 프로그램을 읽으면 함수 추상화 수준이 한 번에 한 단계씩 낮아진다. - 클린 코드
추상화 수준을 기준으로 메서드를 위치시킨 후 연관성을 고려했습니다!
|
||
boolean existsByUsername(String username); | ||
|
||
Optional<Member> findByEmail(String 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.
⚡ 다른 레파지토리들 처럼 fetchByEmail을 default 메서드로 만들어서 Optional 을 제거하면 더 좋을 것 같아요
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.
이해 완 ~
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.
간단한 수정사항 하나와 여러개의 의견들 담겨두었어요!
@GetMapping("/check-email") | ||
@ResponseStatus(HttpStatus.OK) | ||
public void checkUniqueEmail(@RequestParam String email) { | ||
memberService.assertUniqueEmail(email); | ||
} | ||
|
||
@GetMapping("/check-username") | ||
@ResponseStatus(HttpStatus.OK) | ||
public void checkUniqueUsername(@RequestParam String username) { | ||
memberService.assertUniqueUsername(username); | ||
} |
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.
@GetMapping("/check-email") | |
@ResponseStatus(HttpStatus.OK) | |
public void checkUniqueEmail(@RequestParam String email) { | |
memberService.assertUniqueEmail(email); | |
} | |
@GetMapping("/check-username") | |
@ResponseStatus(HttpStatus.OK) | |
public void checkUniqueUsername(@RequestParam String username) { | |
memberService.assertUniqueUsername(username); | |
} | |
@PostMapping("/assert-unique-email") | |
@ResponseStatus(HttpStatus.OK) | |
public void checkUniqueEmail(@RequestParam String email) { | |
memberService.assertUniqueEmail(email); | |
} | |
@PostMapping("/validate-unique-username") | |
@ResponseStatus(HttpStatus.OK) | |
public void checkUniqueUsername(@RequestParam String username) { | |
memberService.assertUniqueUsername(username); | |
} |
둘 다 중복되는 정보가 이미 존재하는지 확인하는 기능인데요.
check-email
/ check-username
라는 엔드포인트가 모호하지는 않은지 고민이 되네요 🤔
+) 두 요청 모두 자원을 받아오는 행위가 아니기도 하니 POST 요청을 사용해 보는 것은 어떨까요?
@ExampleObject(name = "이메일 입력 없음", value = """ | ||
{ | ||
"type": "about:blank", | ||
"title": "BAD_REQUEST", | ||
"status": 400, | ||
"detail": "이메일이 입력되지 않았습니다.", | ||
"instance": "/signup" | ||
} | ||
""" | ||
), |
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.
와, 제우스 엄청 귀찮았겠는데요...!
중복되는 부분들이 많은 것 같은데, 어떻게 중복을 제거할 수 있을지 같이 고민해볼게요!
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.
고생 많았어요 제우스 🔥
@ApiErrorResponse 중복 정의 가능하게 변경해두었으니, 이번 pr 머지 후 리팩토링 해보면 좋을 것 같아요.
⚡️ 당장 리팩토링 하지 않더라도 이슈로 만들어놓으면 좋을 것 같습니다 !
public boolean matchPassword(String other) { | ||
return Objects.equals(password, other); | ||
} |
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.
public boolean matchPassword(String other) { | |
return Objects.equals(password, other); | |
} | |
public boolean matchPassword(String password) { | |
return Objects.equals(this.password, password); | |
} |
⚡ 파라미터 명을 조금 더 명확하게 변경해 보았어요!
@Schema( | ||
description = "이메일", | ||
example = "[email protected]", | ||
pattern = "^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\\.[a-zA-Z]{2,}$\n" | ||
) | ||
@NotEmpty(message = "이메일이 입력되지 않았습니다.") | ||
@Size(max = 255, message = "이메일은 255자 이하로 입력해주세요.") | ||
String 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.
@Schema( | |
description = "이메일", | |
example = "[email protected]", | |
pattern = "^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\\.[a-zA-Z]{2,}$\n" | |
) | |
@NotEmpty(message = "이메일이 입력되지 않았습니다.") | |
@Size(max = 255, message = "이메일은 255자 이하로 입력해주세요.") | |
String email, | |
@Schema( | |
description = "이메일", | |
example = "[email protected]", | |
pattern = "^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\\.[a-zA-Z]{2,}$\n" | |
) | |
@NotEmpty(message = "이메일이 입력되지 않았습니다.") | |
@Size(max = 255, message = "이메일은 255자 이하로 입력해주세요.") | |
@Pattern(regexp = "^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\\.[a-zA-Z]{2,}$\n") | |
//@Email(regexp = "^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\\.[a-zA-Z]{2,}$\n") | |
String email, |
@Schema
에서 정규 표현식을 명시해 두었으니만큼, @Pattern
또는 @Email
을 검증에도 사용해 보는 것도 좋을 것 같아요.
String[] credentials = decodeCredentials(authHeaderValue); | ||
String email = credentials[0]; | ||
String password = credentials[1]; |
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.
어쩌면 decodeCredentials()
메서드가 일급 컬렉션, 또는 특정 책임을 가지는 객체를 반환해 주는 것도 방법이 될 수 있겠네요!
|
||
import codezap.member.domain.Member; | ||
|
||
public interface MemberRepository extends JpaRepository<Member, Long> { | ||
public interface MemberRepository { |
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.
아 이 인터페이스 너무 좋은데요 ⚡
import codezap.member.repository.FakeMemberRepository; | ||
import codezap.member.repository.MemberRepository; | ||
|
||
public class MemberServiceTest { |
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.
Fake 객체를 쓰니 @SpringBootTest
를 사용하지 않아도 되네요!
너무너무 마음에 듭니다 👍
혹시 지금 스웨거에서 회원가입이 되나요?? 제 로컬에서만 안되는지 궁금하네요... |
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.
멤버 정보가 가져와지지가 않아서 계속 확인해봤는데 어노테이션에 옵션들이 설정이 안 되어 있었네요!!
그리고 아래는 뭐 때문인지는 모르겠는데 뭔가 이상한 것 같아서 공유합니다. 확인 부탁드려요!!
요청시에 Header의 Cookie 부분에 다음과 같이 정보가 들어가있습니다.
token=eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiJhZG1pbkByb29tZXNjYXBlLmNvbSIsImlhdCI6MTcxODE3MjUwNiwiZXhwIjoxNzE4MTc2MTA2fQ.Tnj2aX5hU_FlZRhtzcxeDKBanhwQeHzocuaomdpLi1M; Authorization=dGVzdEBlbWFpbC5jb206cGFzc3dvcmQ=
여기서 token을 해석해보면
{"alg":"HS256"}{"sub":"[email protected]","iat":1718172506,"exp":1718176106}�f�?�VQ��·.j
이렇게 나오던데 뭐 때문에 이런건가요??
제 브라우저의 캐시 문제인가요..?
package codezap.member.configuration; | ||
|
||
public @interface BasicAuthentication { | ||
} |
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.
⚡
package codezap.member.configuration; | |
public @interface BasicAuthentication { | |
} | |
package codezap.member.configuration; | |
@Target(ElementType.PARAMETER) | |
@Retention(RetentionPolicy.RUNTIME) | |
public @interface BasicAuthentication { | |
} |
어노테이션에 이 친구들 붙여줘야 동작하는 것 같은데 확인 부탁드립니다!!
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.
안녕하세요 제우스!
수고하셨어요 리뷰 남겼으니 같이 얘기해보아요 🔥
@ExampleObject(name = "이메일 입력 없음", value = """ | ||
{ | ||
"type": "about:blank", | ||
"title": "BAD_REQUEST", | ||
"status": 400, | ||
"detail": "이메일이 입력되지 않았습니다.", | ||
"instance": "/signup" | ||
} | ||
""" | ||
), |
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.
고생 많았어요 제우스 🔥
@ApiErrorResponse 중복 정의 가능하게 변경해두었으니, 이번 pr 머지 후 리팩토링 해보면 좋을 것 같아요.
⚡️ 당장 리팩토링 하지 않더라도 이슈로 만들어놓으면 좋을 것 같습니다 !
public MemberDto authorizeByCookie(Cookie[] cookies) { | ||
String authHeaderValue = getAuthCookieValue(cookies); | ||
String[] credentials = decodeCredentials(authHeaderValue); | ||
String email = credentials[0]; | ||
String password = credentials[1]; | ||
return authorizeByEmailAndPassword(email, password); | ||
} | ||
|
||
private String getAuthCookieValue(Cookie[] cookies) { | ||
return Arrays.stream(cookies) | ||
.filter(cookie -> Objects.equals(cookie.getName(), HttpHeaders.AUTHORIZATION)) | ||
.findFirst() | ||
.map(Cookie::getValue) | ||
.orElseThrow(this::throwUnauthorized); | ||
} | ||
|
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.
지금은 쿠키이지만 Authorization Header로 인증 값을 주고 받는다면 서비스 전체가 변경될 것 같아요.
추상화를 시도해보면 어떨까요?
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.
좋은 생각이에요 :)
다만 저는 구현체가 하나 뿐인 인터페이스를 만드는 게 좋은 방법이라고 생각하지 않습니다. 그래서 인증 방식이 추가된다면 그때 추상화를 하면 좋겠어요!
@@ -0,0 +1,34 @@ | |||
package codezap.member.configuration; |
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.
AuthArgumentResolver 의 위치가 /member/configuration 인 것이 조금 모호한 것 같아요
오히려 /member/auth 패키지로 두는 것이 더 찾기 수월할 것 같은데 어떤가요?
@@ -0,0 +1,22 @@ | |||
package codezap.member.configuration; |
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.
⚡️ auth 패키지를 따로 분리하지 않고 member 패키지 아래에 인증 관련 것들을 둔 이유가 있을까용?
member 패키지 아래 파일들이 대부분 인증에 대한 것 같아서용
domain 패키지를 제외한 다른 파일들은 auth로 따로 있는 것이 적절할 것 같다는 생각이 들기도 하네용...
|
||
@RestController | ||
@RequiredArgsConstructor | ||
public class MemberController implements SpringDocMemberController { |
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.
⚡️ 이것도 비슷하게 AuthController
가 더 적절할 것 같습니당...
MemberController면 Member에 대한 CRUD 요청이 오고 갈 것 같은데 Auth 관련 로직만 있어서유
어드민 페이지가 생겨서 회원 정보를 조회해야 한다면? 그 친구들이 MemberController에 있는 것이 더 명확하지 않을까요?
@@ -0,0 +1,27 @@ | |||
package codezap.member.dto; |
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.
⚡️ 현재 dto가 request 만 있어서 /dto에 바로 위치시킨 것 같아요~
그런데 내 정보 조회 등 response body가 필요한 api 들이 다음 스프린트에서 추가될텐데 그때 패키지를 구분하게 되면 변경사항이 매우 많이 잡힐 것 같네용
미리 /dto/request 아래에 두는 것이 어떤가요?
|
||
import codezap.member.domain.Member; | ||
|
||
public record MemberDto( |
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.
Dto의 역할이 맞는데 뭔가 이름이 매우 불편쓰... 어카죠... 고민해봅시다...
그리고 Member 엔티티를 바로 조회해와도 좋을 것 같아요~ 같이 논의해보아요!!!!!
|
||
@Service | ||
@RequiredArgsConstructor | ||
public class MemberService { |
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.
이 부분도 �앞서 리뷰 남긴 것처럼 현재는 AuthService 가 적절한 것 같아요 :)
⚡️ 관련 이슈
close #197 #198 #199
📍주요 변경 사항
기능
MemberRepository
FakeMemberRepository
를 사용합니다.MemberRepository
인터페이스에 사용할 메서드만 정의했습니다.MemberJpaRepository
인터페이스가MemberRepository
와JpaRepository
를 상속받습니다.AuthArgumentResolver
Member
관련 정보를 사용할 수 있게 하기 위해 구현했습니다.@BasicAuthentication
을 사용해MemberDto
객체를 받아 사용할 수 있습니다.예제
🎸 추가 작업