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

Feature/#39 add logout #40

Merged
merged 12 commits into from
Nov 4, 2022
Merged

Feature/#39 add logout #40

merged 12 commits into from
Nov 4, 2022

Conversation

ur2e
Copy link
Member

@ur2e ur2e commented Oct 29, 2022

  • 로그인 성공 후 answer 조회
    image

  • 토큰과 함께 로그아웃 요청
    image

  • 로그아웃 시 사용한 토큰으로 answer 조회
    image

@ur2e ur2e linked an issue Oct 29, 2022 that may be closed by this pull request
@ur2e ur2e self-assigned this Oct 29, 2022
Comment on lines 71 to 77
public Long getExpiration(String token) {
Date expiration = Jwts.parser().setSigningKey(Base64.getEncoder().encodeToString(secretKey.getBytes()))
.parseClaimsJws(token).getBody().getExpiration();

Long now = new Date().getTime();
return (expiration.getTime() - now);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

함수 이름을 보면 expiration을 줄 것 같지만 실제 구현은 그렇지 않네요, 메서드 이름을 변경하거나 반환하는 로직을 변경해야 할 것 같아요.

Copy link
Member Author

@ur2e ur2e Nov 1, 2022

Choose a reason for hiding this comment

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

image

해당 함수가 반환하는 값이 토큰의 남은 유효기간이라 getExpiration 으로 하였습니다 ! 혹시 getExpirationTime이 더 적합할까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

만료 기간 그대로 반환하는것이 아니고 계산해서 반환해주니 후자 getExpirationTime이 좋을 것 같아요

Copy link
Member Author

Choose a reason for hiding this comment

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

수정했습니다 ! 😄 반환 값에 맞는 함수명이 맞는지 더 신경쓰면서 코드 작성하겠습니답 ~!

Comment on lines +79 to +83
public boolean isTokenLogout(String token) {
String isLogout = (String) redisTemplate.opsForValue().get(token);
if (StringUtils.hasText(isLogout)) throw new LoggedOutTokenException();
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public boolean isTokenLogout(String token) {
String isLogout = (String) redisTemplate.opsForValue().get(token);
if (StringUtils.hasText(isLogout)) throw new LoggedOutTokenException();
return true;
}
public boolean isTokenLogout(String token) {
String isLogout = (String) redisTemplate.opsForValue().get(token);
return !StringUtils.hasText(isLogout);
}

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
Member Author

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.

요건 만약에 진행하신다면 별도 이슈를 만들어서 하시죠~

디버깅용 코드 삭제
@ur2e ur2e requested a review from level120 November 1, 2022 13:28
Comment on lines +79 to +83
public boolean isTokenLogout(String token) {
String isLogout = (String) redisTemplate.opsForValue().get(token);
if (StringUtils.hasText(isLogout)) throw new LoggedOutTokenException();
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

요건 그대로인데 어떻게 하기로 결정하셨나요?

403: 권한 없음, 401: 인증 실패
@ur2e ur2e requested a review from level120 November 1, 2022 13:57
@leedonggyu1848 leedonggyu1848 merged commit 4851283 into main Nov 4, 2022
@leedonggyu1848 leedonggyu1848 deleted the feature/#39-add-logout branch November 5, 2022 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

logout 기능 구현
3 participants