-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feat/#462 닉네임 변경 API 구현 #470
base: main
Are you sure you want to change the base?
Changes from 15 commits
9ef006e
95bd4c7
ee8bc8a
3d14050
ded41a5
d52d13c
9bfb040
621a625
4685592
5a64990
2847c8d
05fdcf1
514c475
f0e54f9
04af872
3d46182
72ea86a
d9115bf
21358a7
8ca6f07
9c6fc11
21f4d60
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
package shook.shook.auth.application; | ||
|
||
import io.jsonwebtoken.Claims; | ||
import lombok.RequiredArgsConstructor; | ||
import org.springframework.stereotype.Service; | ||
import org.springframework.transaction.annotation.Transactional; | ||
import shook.shook.auth.application.dto.ReissueAccessTokenResponse; | ||
import shook.shook.auth.exception.AuthorizationException; | ||
import shook.shook.auth.repository.InMemoryTokenPairRepository; | ||
|
||
@Transactional(readOnly = true) | ||
@RequiredArgsConstructor | ||
@Service | ||
public class TokenService { | ||
|
||
public static final String EMPTY_REFRESH_TOKEN = "none"; | ||
public static final String TOKEN_PREFIX = "Bearer "; | ||
|
||
private final TokenProvider tokenProvider; | ||
private final InMemoryTokenPairRepository inMemoryTokenPairRepository; | ||
|
||
public void validateRefreshToken(final String refreshToken) { | ||
if (refreshToken.equals(EMPTY_REFRESH_TOKEN)) { | ||
throw new AuthorizationException.RefreshTokenNotFoundException(); | ||
} | ||
} | ||
|
||
public String extractAccessToken(final String authorization) { | ||
return authorization.substring(TOKEN_PREFIX.length()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 그리고 만약 authorization 문자열이 |
||
} | ||
|
||
public ReissueAccessTokenResponse reissueAccessTokenByRefreshToken(final String refreshToken, | ||
final String accessToken) { | ||
final Claims claims = tokenProvider.parseClaims(refreshToken); | ||
final Long memberId = claims.get("memberId", Long.class); | ||
final String nickname = claims.get("nickname", String.class); | ||
|
||
inMemoryTokenPairRepository.validateTokenPair(refreshToken, accessToken); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
현재 만약 refreshToken이 만료되었는데, 이를 캐치하지 못하고 먼저
그러면 프론트엔드에서는 이렇게 되면 결과적으로 "accessToken이 만료되어서 재발급 받으려 하는데, refreshToken도 이미 만료가 된 상황"에 대한 처리가 제대로 이루어지지 않을 것 같다는 생각이 들어요. 베로는 어떻게 생각하시나요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 날카로운 지적입니다. |
||
final String reissuedAccessToken = tokenProvider.createAccessToken(memberId, nickname); | ||
inMemoryTokenPairRepository.addOrUpdateTokenPair(refreshToken, reissuedAccessToken); | ||
|
||
return new ReissueAccessTokenResponse(reissuedAccessToken); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,7 +45,8 @@ private HandlerInterceptor tokenInterceptor() { | |
.includePathPattern("/songs/*/parts/*/likes", PathMethod.PUT) | ||
.includePathPattern("/voting-songs/*/parts", PathMethod.POST) | ||
.includePathPattern("/songs/*/parts/*/comments", PathMethod.POST) | ||
.includePathPattern("/members/*", PathMethod.DELETE); | ||
.includePathPattern("/members/*", PathMethod.DELETE) | ||
.includePathPattern("/members/*/nickname", PathMethod.PATCH); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍👍 |
||
} | ||
|
||
@Override | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,9 +7,8 @@ | |
import org.springframework.web.bind.annotation.PostMapping; | ||
import org.springframework.web.bind.annotation.RequestHeader; | ||
import org.springframework.web.bind.annotation.RestController; | ||
import shook.shook.auth.application.AuthService; | ||
import shook.shook.auth.application.TokenService; | ||
import shook.shook.auth.application.dto.ReissueAccessTokenResponse; | ||
import shook.shook.auth.exception.AuthorizationException; | ||
import shook.shook.auth.ui.openapi.AccessTokenReissueApi; | ||
|
||
@RequiredArgsConstructor | ||
|
@@ -18,21 +17,18 @@ public class AccessTokenReissueController implements AccessTokenReissueApi { | |
|
||
private static final String EMPTY_REFRESH_TOKEN = "none"; | ||
private static final String REFRESH_TOKEN_KEY = "refreshToken"; | ||
private static final String TOKEN_PREFIX = "Bearer "; | ||
|
||
private final AuthService authService; | ||
private final TokenService tokenService; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
|
||
@PostMapping("/reissue") | ||
public ResponseEntity<ReissueAccessTokenResponse> reissueAccessToken( | ||
@CookieValue(value = REFRESH_TOKEN_KEY, defaultValue = EMPTY_REFRESH_TOKEN) final String refreshToken, | ||
@RequestHeader(HttpHeaders.AUTHORIZATION) final String authorization | ||
) { | ||
if (refreshToken.equals(EMPTY_REFRESH_TOKEN)) { | ||
throw new AuthorizationException.RefreshTokenNotFoundException(); | ||
} | ||
final String accessToken = authorization.split(TOKEN_PREFIX)[1]; | ||
final ReissueAccessTokenResponse response = | ||
authService.reissueAccessTokenByRefreshToken(refreshToken, accessToken); | ||
tokenService.validateRefreshToken(refreshToken); | ||
final String accessToken = tokenService.extractAccessToken(authorization); | ||
final ReissueAccessTokenResponse response = tokenService.reissueAccessTokenByRefreshToken(refreshToken, | ||
accessToken); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
reissue 메서드 내에서 validate, extract 를 함께 하도록 하지 않고 validate와 extract를 public으로 두고 controller 에서 따로 호출하도록 하신 이유가 있을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 지금은 사라졌지만 닉네임 변경 시 리프레시 토큰을 받도록 하면 다른 곳에서도 검증이 필요해서 public 메서드로 만들어두었습니다! |
||
|
||
return ResponseEntity.ok(response); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,8 +6,11 @@ | |
import lombok.RequiredArgsConstructor; | ||
import org.springframework.stereotype.Service; | ||
import org.springframework.transaction.annotation.Transactional; | ||
import shook.shook.auth.application.TokenProvider; | ||
import shook.shook.auth.exception.AuthorizationException; | ||
import shook.shook.auth.repository.InMemoryTokenPairRepository; | ||
import shook.shook.auth.ui.argumentresolver.MemberInfo; | ||
import shook.shook.member.application.dto.NicknameUpdateRequest; | ||
import shook.shook.member.domain.Email; | ||
import shook.shook.member.domain.Member; | ||
import shook.shook.member.domain.Nickname; | ||
|
@@ -27,6 +30,8 @@ public class MemberService { | |
private final MemberRepository memberRepository; | ||
private final KillingPartCommentRepository commentRepository; | ||
private final KillingPartLikeRepository likeRepository; | ||
private final TokenProvider tokenProvider; | ||
private final InMemoryTokenPairRepository inMemoryTokenPairRepository; | ||
|
||
@Transactional | ||
public Member register(final String email) { | ||
|
@@ -37,6 +42,7 @@ public Member register(final String email) { | |
final Member newMember = new Member(email, BASIC_NICKNAME); | ||
final Member savedMember = memberRepository.save(newMember); | ||
savedMember.updateNickname(savedMember.getNickname() + savedMember.getId()); | ||
|
||
return savedMember; | ||
} | ||
|
||
|
@@ -55,30 +61,24 @@ public Member findByIdAndNicknameThrowIfNotExist(final Long id, final Nickname n | |
|
||
@Transactional | ||
public void deleteById(final Long id, final MemberInfo memberInfo) { | ||
final long requestMemberId = memberInfo.getMemberId(); | ||
final Member requestMember = findById(requestMemberId); | ||
final Member targetMember = findById(id); | ||
validateMemberAuthentication(requestMember, targetMember); | ||
final Member member = getMemberIfValidRequest(id, memberInfo.getMemberId()); | ||
|
||
final List<KillingPartLike> membersExistLikes = likeRepository.findAllByMemberAndIsDeleted( | ||
targetMember, | ||
false | ||
); | ||
final List<KillingPartLike> membersExistLikes = likeRepository.findAllByMemberAndIsDeleted(member, false); | ||
|
||
membersExistLikes.forEach(KillingPartLike::updateDeletion); | ||
commentRepository.deleteAllByMember(targetMember); | ||
memberRepository.delete(targetMember); | ||
commentRepository.deleteAllByMember(member); | ||
memberRepository.delete(member); | ||
} | ||
|
||
private Member findById(final Long id) { | ||
return memberRepository.findById(id) | ||
.orElseThrow(() -> new MemberException.MemberNotExistException( | ||
Map.of("Id", String.valueOf(id)) | ||
)); | ||
private Member getMemberIfValidRequest(final Long memberId, final Long requestMemberId) { | ||
final Member requestMember = findById(requestMemberId); | ||
final Member targetMember = findById(memberId); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 부분에서 현재 request의 Path로 넘어온 id와 arguemntResolver로 넘어온 id로 member를 DB에서 불러온 뒤에 같은지 비교를 하고 있는데 현재 member의 경우 equal&hash가 id로만 재정의 되어 있습니다. 따라서 db에서 member를 불러오기에 앞서서 memberId와 requestMemberId를 비교하고 그 다음에 실제 존재하는 member인지 검증하기 위해 DB로 요청을 보내면 db로 보내는 요청횟수를 줄일 수 있을 것 같습니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 오 굉장히 합리적인 방법이네요 |
||
validateMemberAuthentication(requestMember, targetMember); | ||
|
||
return targetMember; | ||
} | ||
|
||
private void validateMemberAuthentication(final Member requestMember, | ||
final Member targetMember) { | ||
private void validateMemberAuthentication(final Member requestMember, final Member targetMember) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 메서드 순서 변경이 필요해 보입니다! 이 메서드를 위로 올리는 것은 어떨까요? |
||
if (!requestMember.equals(targetMember)) { | ||
throw new AuthorizationException.UnauthenticatedException( | ||
Map.of( | ||
|
@@ -88,4 +88,37 @@ private void validateMemberAuthentication(final Member requestMember, | |
); | ||
} | ||
} | ||
|
||
private Member findById(final Long id) { | ||
return memberRepository.findById(id) | ||
.orElseThrow(() -> new MemberException.MemberNotExistException( | ||
Map.of("Id", String.valueOf(id)) | ||
)); | ||
} | ||
|
||
@Transactional | ||
public boolean updateNickname(final Long memberId, final Long requestMemberId, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 현재 memberInfo 의 id 만 쓰이고 있는 상태라 id 만 넘겨주는 걸로 통일했습니다 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
final NicknameUpdateRequest request) { | ||
final Member member = getMemberIfValidRequest(memberId, requestMemberId); | ||
final Nickname nickname = new Nickname(request.getNickname()); | ||
|
||
if (member.hasSameNickname(nickname)) { | ||
return false; | ||
} | ||
|
||
validateDuplicateNickname(nickname); | ||
member.updateNickname(nickname.getValue()); | ||
memberRepository.save(member); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 부분 jpa 더티채킹을 통해서 값이 수정되어서 memberRespository.save()를 하지 않아도 될 것 같습니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 좋은 지적입니다 👍🏻 |
||
return true; | ||
} | ||
|
||
private void validateDuplicateNickname(final Nickname nickname) { | ||
final boolean isDuplicated = memberRepository.existsMemberByNickname(nickname); | ||
if (isDuplicated) { | ||
throw new MemberException.ExistNicknameException( | ||
Map.of("Nickname", nickname.getValue()) | ||
); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
package shook.shook.member.application.dto; | ||
|
||
import io.swagger.v3.oas.annotations.media.Schema; | ||
import jakarta.validation.constraints.NotBlank; | ||
import lombok.AllArgsConstructor; | ||
import lombok.Getter; | ||
import lombok.NoArgsConstructor; | ||
|
||
@Schema(description = "닉네임 변경 요청") | ||
@NoArgsConstructor(access = lombok.AccessLevel.PRIVATE) | ||
@AllArgsConstructor | ||
@Getter | ||
public class NicknameUpdateRequest { | ||
|
||
@Schema(description = "닉네임", example = "shookshook") | ||
@NotBlank | ||
private String nickname; | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. 매의 눈... 이거 왜 남아 있었을까요 🤦🏻♀️ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
package shook.shook.member.application.dto; | ||
|
||
import io.swagger.v3.oas.annotations.media.Schema; | ||
import lombok.AccessLevel; | ||
import lombok.AllArgsConstructor; | ||
import lombok.Getter; | ||
import lombok.NoArgsConstructor; | ||
|
||
@Schema(description = "닉네임 변경 응답") | ||
@AllArgsConstructor | ||
@NoArgsConstructor(access = AccessLevel.PRIVATE) | ||
@Getter | ||
public class NicknameUpdateResponse { | ||
|
||
@Schema(description = "액세스 토큰", example = "lfahrg;aoiehflksejflakwjeglk") | ||
private String accessToken; | ||
|
||
@Schema(description = "변경된 닉네임", example = "shook") | ||
private String nickname; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,6 +49,14 @@ public void updateNickname(final String newNickName) { | |
this.nickname = new Nickname(newNickName); | ||
} | ||
|
||
public void updateNickname(final Nickname newNickname) { | ||
this.nickname = newNickname; | ||
} | ||
Comment on lines
51
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
public boolean hasSameNickname(final Nickname nickname) { | ||
return nickname.equals(this.nickname); | ||
} | ||
|
||
public String getEmail() { | ||
return email.getValue(); | ||
} | ||
|
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.
현재 TokenService 내부에
TokenProvider
와InMemoryTokenPairRepository
를 가지고 구성이 되어 있는데 MemberService와 AuthService는 TokenService를 의존하는 것이 아닌TokenProvider
와InMemoryTokenPairRepository
의존하고 있는데 이 부분이 전에 말했던 순환참조 때문이었나요? 그렇지 않으면 TokenService를 의존하는 것이 좋을 것 같은데 어떻게 생각하시나요?