-
Notifications
You must be signed in to change notification settings - Fork 0
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
✨ implement refresh token #113
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.
properties 가시성만 조금 손 본다면 모든게 좋습니다~~ 주말동안 고생허셨어요!!
@@ -7,6 +7,12 @@ jasypt: | |||
encryptor: | |||
password: ${JASYPT_PASSWORD} | |||
|
|||
jwt: | |||
access-token: | |||
expire-in-millis: 86400000 |
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.
Duration
클래스로 가독성을 높여 보는 것은 어떻까요? 86400000ms
가 1d
랑 같은 값인 것 같아요.
그리고 받을 때는 타입이 long
이 아닌 Duration
으로 받으면 될 것 같아요!
https://www.baeldung.com/configuration-properties-in-spring-boot#1-duration
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.
오 좋은데요! long을 쓰니 무조건 ms 단위로 썼는데 훨신 좋아보여요!
access-token: | ||
expire-in-millis: 86400000 | ||
refresh-token: | ||
expire-in-millis: 2419200000 |
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.
Duration
사용 마찬가지 입니다!
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.
해결했습니다!
1d, 30d로 변경했슴당
|
||
public JwtTokenManager( | ||
@Value("${jwt.secret}") String secret, | ||
@Value("${jwt.expire-in-millis}") long tokenExpirationMills | ||
@Value("${jwt.access-token.expire-in-millis}") long accessTokenExpirationMills, | ||
@Value("${jwt.refresh-token.expire-in-millis}") long refreshTokenExpirationMills |
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.
properties
에 코멘트 남겨놓은 것을 추가하면 여기에 타입으로 long
이 아닌 Duration
을 줄 수 있을 것 같아요!
@@ -48,7 +49,17 @@ void loginWithGoogleWithEmailAlreadyRegistered() throws FirebaseAuthException { | |||
when(firebaseToken.getEmail()).thenReturn(email); | |||
when(firebaseAuth.verifyIdToken(idToken)).thenReturn(firebaseToken); | |||
|
|||
GoogleLoginResponse actual = RestAssured.given().log().all() | |||
GoogleLoginResponse actual = RestAssured.given(spec).log().all() | |||
.filter(document(DEFAULT_RESTDOCS_PATH, |
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.
document
메서드의 2, 3번째 인자로 description
과 summary
가 있는데 API Docs 사용자인 안드로이드 측에서 사용하기 쉽도록 내용을 추가해주는건 어떨까요?!
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.
추가 했습니다!
다만 이슈가 조금 있습니다.
@Test
@DisplayName("이미 가입된 계정으로 로그인하면 이미 가입되었다고 알리고 access token과 refresh token을 반환한다.")
void loginWithGoogleWithEmailAlreadyRegistered() throws FirebaseAuthException {
String email = "[email protected]";
String idToken = "test.id.token";
FirebaseToken firebaseToken = mock(FirebaseToken.class);
when(firebaseToken.getEmail()).thenReturn(email);
when(firebaseAuth.verifyIdToken(idToken)).thenReturn(firebaseToken);
GoogleLoginResponse actual = RestAssured.given(spec).log().all()
.filter(document(DEFAULT_RESTDOCS_PATH,
"이미 가입된 계정으로 로그인하여 토큰을 반환합니다.",
"구글 로그인 API",
requestFields(
fieldWithPath("idToken").description("Google ID Token")
),
responseFields(
fieldWithPath("accessToken").description("JWT Access Token"),
fieldWithPath("refreshToken").description("JWT Refresh Token"),
fieldWithPath("registered").description("사용자 등록 여부")
)
))
.contentType(ContentType.JSON)
.body(new GoogleLoginRequest(idToken))
.when().post("/api/oauth/google/login")
.then().log().all()
.statusCode(200)
.extract()
.as(GoogleLoginResponse.class);
assertAll(
() -> assertThat(actual.accessToken()).isNotNull(),
() -> assertThat(actual.refreshToken()).isNotNull(),
() -> assertThat(actual.registered()).isTrue()
);
}
@Test
@DisplayName("처음 로그인하면 가입되어 있지 않음을 알리고 access token과 refresh token을 반환하지 않는다.")
void loginWithGoogleWithEmailNotRegistered() throws FirebaseAuthException {
String email = "[email protected]";
String idToken = "test.id.token";
FirebaseToken firebaseToken = mock(FirebaseToken.class);
when(firebaseToken.getEmail()).thenReturn(email);
when(firebaseAuth.verifyIdToken(idToken)).thenReturn(firebaseToken);
GoogleLoginResponse actual = RestAssured.given(spec).log().all()
.filter(document(DEFAULT_RESTDOCS_PATH,
"처음 로그인하여 등록되지 않은 계정을 알립니다.",
"구글 로그인 API",
requestFields(
fieldWithPath("idToken").description("Google ID Token")
),
responseFields(
fieldWithPath("accessToken").description("JWT Access Token").optional(),
fieldWithPath("refreshToken").description("JWT Refresh Token").optional(),
fieldWithPath("registered").description("사용자 등록 여부")
)
))
.contentType(ContentType.JSON)
.body(new GoogleLoginRequest(idToken))
.when().post("/api/oauth/google/login")
.then().log().all()
.statusCode(200)
.extract()
.as(GoogleLoginResponse.class);
assertAll(
() -> assertThat(actual.accessToken()).isNull(),
() -> assertThat(actual.refreshToken()).isNull(),
() -> assertThat(actual.registered()).isFalse()
);
}
이렇게 한 메서드에 두가지 테스트가 있을때 description이 달라지게 되는데 뒤에 실행되는 테스트의 description으로 덮어씌워지는것 같습니다.
이 경우에 두가지 디스크립션이 모두 필요하면 어떻게 해야할까요?
@@ -59,12 +70,13 @@ void loginWithGoogleWithEmailAlreadyRegistered() throws FirebaseAuthException { | |||
|
|||
assertAll( |
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.
토큰이 보통 해싱 기반으로 작동되는 것으로 아는데, 길이가 똑같지 않나요?
NotNull
체크 뿐만 아니라 길이 체크도 하면 테스트의 신뢰도가 훨씬 올라갈 것 같아요!
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.
제 생각에 페이로드에 담기는 정보의 양에 따라 토큰의 길이가 가변적일것 같습니다.
다만 말씀하신대로 테스트의 신뢰도를 위해서 토큰의 형식을 보여주는지 여부는 필요할것 같네요!
@@ -73,7 +85,17 @@ void loginWithGoogleWithEmailNotRegistered() throws FirebaseAuthException { | |||
when(firebaseToken.getEmail()).thenReturn(email); | |||
when(firebaseAuth.verifyIdToken(idToken)).thenReturn(firebaseToken); | |||
|
|||
GoogleLoginResponse actual = RestAssured.given().log().all() | |||
GoogleLoginResponse actual = RestAssured.given(spec).log().all() | |||
.filter(document(DEFAULT_RESTDOCS_PATH, |
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.
여기도 description
summary
>_<
@@ -84,6 +106,7 @@ void loginWithGoogleWithEmailNotRegistered() throws FirebaseAuthException { | |||
|
|||
assertAll( | |||
() -> assertThat(actual.accessToken()).isNull(), | |||
() -> assertThat(actual.refreshToken()).isNull(), | |||
() -> assertThat(actual.registered()).isFalse() |
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 org.springframework.web.context.request.NativeWebRequest; | ||
|
||
@SpringBootTest | ||
class LoginUserArgumentResolverTest { |
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.
ArgumentResolver
를 테스트할 수도 있겠군요! 👍
|
|
이제는 문서를 안줄 수가 없어 이 브랜치에 같이 올립니다.
|
이름이나 패키지 변경으로 file changed 수가 많습니다.
생각보다 변경된건 많지 않아요.
@LoginUser UserInfo userInfo
에 정보가 담김