-
Notifications
You must be signed in to change notification settings - Fork 0
[Feat] 401 토큰 만료 감지 로직 TokenAuthenticator으로 마이그레이션 #288
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
Conversation
…, refreshToken 보내는 형식 수정 (Bearer)
|
file chages 보기 불편하면 주석 지우고 봐주셔두 댐요 |
HI-JIN2
left a comment
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.
혹시 RetrofitImpl.kt 없앨 순 없을까요? 이게 레거시 레트로핏이라서... 혹시 아직 의존성이 있어서 못지운건가요? (제가 안지운 이유는 의존성이 있어서 못지운거라서)
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.
아뇨! 얘도 지워도 되냐고 물어보려고 했는데 까먹었었어요
지우겠씁니다!
| @Named("NoToken") | ||
| fun provideNoAuthOkHttpClient(): OkHttpClient { | ||
| val builder = OkHttpClient.Builder() | ||
| if (BuildConfig.DEBUG) { | ||
| builder.addInterceptor(HttpLoggingInterceptor().apply { | ||
| level = HttpLoggingInterceptor.Level.BODY | ||
| }) | ||
| } | ||
| return builder.build() | ||
| } |
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.
OkHttpClient는 하나만 있어도 되지 않나요? Retrofit만 두개 있으면 되는거 아닌가용?
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.
Interceptor 로 헤더에 accessToken을 넣는 로직이 불필요해서 구분했습니다!
| // 토큰 없는 retrofit | ||
| @Singleton | ||
| @Provides | ||
| @Named("NoToken") |
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.
|
|
||
| import android.content.Context | ||
| import android.content.Intent | ||
| import android.widget.Toast |
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.
도메인인 usecase에서 toast 호출을 하는건 적합하지 않은 것 같아요오! 도메인 계층이 할 일에서 벗어난다고 생각합니다 (클린아키텍쳐 기반의 모듈화를 생각하고 있어서요!)
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.
맞습니다 ㅜㅜ 흠......이건 좀 더 고민해볼게요
| class ShowToastSafely @Inject constructor( | ||
| @ApplicationContext private val context: Context, | ||
| ) { | ||
| suspend fun showToast(message: String) { | ||
| withContext(Dispatchers.Main) { | ||
| Toast.makeText(context, message, Toast.LENGTH_SHORT).show() | ||
| } | ||
| } |
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.
usecase가 ui 작업을 하는게 클린아키텍쳐에 위반하지 않나..? 싶습니다! 구현 의도가 있는지 궁금해욥!!
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.
사실 util 처럼 쓰고자 만든거긴 한데
맞는 말씀같아요 이것도 로그인 usecase랑 더 고민해볼게욤
| oauthService.getNewToken( | ||
| refreshToken = "Bearer $refreshToken" | ||
| ) |
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.
아 여기서 토큰 가져오는건 usecase로 안했군요. 제 기억상 토큰 재발급 api 호출을 usecase로 하니까 hilt 순환참조? 가 일어났던걸로 기억해요
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.
어,, usecase 쓸 생각을 못했어서 이렇게 한거긴 한데 흠.... 순환참조가 안 날 거 같은데 해보겠습니다!
| @Provides | ||
| @Singleton | ||
| fun provideTokenAuthenticator( | ||
| getRefreshTokenUseCase: GetRefreshTokenUseCase, | ||
| setAccessTokenUseCase: SetAccessTokenUseCase, | ||
| setRefreshTokenUseCase: SetRefreshTokenUseCase, | ||
| logoutUseCase: LogoutUseCase, | ||
| showToastSafely: ShowToastSafely, | ||
| @Named("NoToken") noTokenRetrofit: Retrofit | ||
| ): TokenAuthenticator { | ||
| return TokenAuthenticator( | ||
| getRefreshTokenUseCase, | ||
| setAccessTokenUseCase, | ||
| setRefreshTokenUseCase, | ||
| logoutUseCase, | ||
| showToastSafely, | ||
| noTokenRetrofit.create(OauthService::class.java) | ||
| ) | ||
| } |
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.
토큰 관련 로직은 앱 전체에서 하나만 필요하다고 생각돼서 싱글톤으로 했고 의존성이 많아 힐트로 주입하도록하려고 했습니다~
| response.request.newBuilder() | ||
| .header("Authorization", "Bearer ${newToken.accessToken}") | ||
| .build() |
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.
아 이구문이 아래와 같은 구문이군요!
val newRequest = originalRequest.newAuthBuilder().build()
return chain.proceed(newRequest)
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.
네 맞아요!!
HI-JIN2
left a comment
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.
PR 다시 보구 몇가지 코멘트 남겼습니다!
앱을 사용하지 않는 경우에 만료된 경우는 TokenInterceptor가 백그라운드 작업이므로 로그아웃이 이미 되어 앱 실행 시 로그인 화면으로 갑니다.
혹시 요기는 어떻게 찾을 수 있을까요? 앱이 실행하지 않을때에도 실행이 되는 백그라운드 코드는 아닌 것 같아서욥..! TokenInterceptor는 토큰이 필요한 api 호출에 한해서 호출 전에 Intercept해서 토큰을 헤더에 붙이는 작업을 하는건데 이게 어떻게 되죠..? (TokenAuthenticator로 바꿔서 생각해봐도 동일한 생각이 들어서 질문합니다!)
| val intent = Intent(context, LoginActivity::class.java).apply { | ||
| flags = Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK | ||
| } | ||
| context.startActivity(intent) | ||
| } |
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.
이거 혹시 로그인 화면에서 이 Intent가 호출되면 로그인 화면 이전은 없는데.. 앱이 종료되지 않겠쬬?
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.
FLAG_ACTIVITY_NEW_TASK : 새로운 task(작업 스택) 만들어 액티비티 실행. 보통 백그라운드에서 사용
FLAG_ACTIVITY_CLEAR_TASK : task 에 남은 기존 액티비티 스택 모두 제거. 새 인텐트의 액티비티만 남김
이기 때문에 로그인 -> 로그인이어도 모든 액티비티 지워지고 새로운 로그인 액티비티를 만들기 때문에 문제 없을 것 같아요!
| } catch (e: Exception) { | ||
| // refreshToken이 만료된 경우 | ||
| Timber.e(e, "토큰 재발급 중 예외 발생") | ||
| showToastSafely.showToast("토큰이 만료되어 로그아웃 됩니다.") | ||
| logoutUseCase() | ||
|
|
||
| null |
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.
여기서 null을 반환하면 앱이 터지지는 않겠구만요 (=강제종료)
| } catch (e: Exception) { | ||
| // refreshToken이 만료된 경우 | ||
| Timber.e(e, "토큰 재발급 중 예외 발생") | ||
| showToastSafely.showToast("토큰이 만료되어 로그아웃 됩니다.") |
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.
보통 저는 세션이 만료되었습니다..? 문구 종종 봤긴 했는데
이건 슬랙에 함 물어볼게융
3이 젤 친근하긴 하네요..
|
|
||
| val intent = Intent(context, LoginActivity::class.java).apply { | ||
| flags = Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK | ||
| } | ||
| context.startActivity(intent) |
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.
흠..... 로그아웃 Usecase에서 intent 처리까지하면 이거 하나만 호출하면 되어서 깔끔하고 좋긴한데,, 도메인 단에서 화면 이동까지 컨트롤하는게 옳은가? 생각이 쵸콤 들어욥..
kangyuri1114
left a comment
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.
아뇨! 얘도 지워도 되냐고 물어보려고 했는데 까먹었었어요
지우겠씁니다!
| // 토큰 없는 retrofit | ||
| @Singleton | ||
| @Provides | ||
| @Named("NoToken") |
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.
| @Named("NoToken") | ||
| fun provideNoAuthOkHttpClient(): OkHttpClient { | ||
| val builder = OkHttpClient.Builder() | ||
| if (BuildConfig.DEBUG) { | ||
| builder.addInterceptor(HttpLoggingInterceptor().apply { | ||
| level = HttpLoggingInterceptor.Level.BODY | ||
| }) | ||
| } | ||
| return builder.build() | ||
| } |
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.
Interceptor 로 헤더에 accessToken을 넣는 로직이 불필요해서 구분했습니다!
| @Provides | ||
| @Singleton | ||
| fun provideTokenAuthenticator( | ||
| getRefreshTokenUseCase: GetRefreshTokenUseCase, | ||
| setAccessTokenUseCase: SetAccessTokenUseCase, | ||
| setRefreshTokenUseCase: SetRefreshTokenUseCase, | ||
| logoutUseCase: LogoutUseCase, | ||
| showToastSafely: ShowToastSafely, | ||
| @Named("NoToken") noTokenRetrofit: Retrofit | ||
| ): TokenAuthenticator { | ||
| return TokenAuthenticator( | ||
| getRefreshTokenUseCase, | ||
| setAccessTokenUseCase, | ||
| setRefreshTokenUseCase, | ||
| logoutUseCase, | ||
| showToastSafely, | ||
| noTokenRetrofit.create(OauthService::class.java) | ||
| ) | ||
| } |
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.
토큰 관련 로직은 앱 전체에서 하나만 필요하다고 생각돼서 싱글톤으로 했고 의존성이 많아 힐트로 주입하도록하려고 했습니다~
| response.request.newBuilder() | ||
| .header("Authorization", "Bearer ${newToken.accessToken}") | ||
| .build() |
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.
네 맞아요!!
| oauthService.getNewToken( | ||
| refreshToken = "Bearer $refreshToken" | ||
| ) |
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.
어,, usecase 쓸 생각을 못했어서 이렇게 한거긴 한데 흠.... 순환참조가 안 날 거 같은데 해보겠습니다!
| } catch (e: Exception) { | ||
| // refreshToken이 만료된 경우 | ||
| Timber.e(e, "토큰 재발급 중 예외 발생") | ||
| showToastSafely.showToast("토큰이 만료되어 로그아웃 됩니다.") |
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.
보통 저는 세션이 만료되었습니다..? 문구 종종 봤긴 했는데
이건 슬랙에 함 물어볼게융
3이 젤 친근하긴 하네요..
|
|
||
| import android.content.Context | ||
| import android.content.Intent | ||
| import android.widget.Toast |
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.
맞습니다 ㅜㅜ 흠......이건 좀 더 고민해볼게요
| class ShowToastSafely @Inject constructor( | ||
| @ApplicationContext private val context: Context, | ||
| ) { | ||
| suspend fun showToast(message: String) { | ||
| withContext(Dispatchers.Main) { | ||
| Toast.makeText(context, message, Toast.LENGTH_SHORT).show() | ||
| } | ||
| } |
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.
사실 util 처럼 쓰고자 만든거긴 한데
맞는 말씀같아요 이것도 로그인 usecase랑 더 고민해볼게욤
@HI-JIN2 |
HI-JIN2
left a comment
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.
왕따봉 드립니다! 👍 거창하게 리뷰할건 딱히 없어 보입니다. 정말 정말 수고 많았어요!
앱 전역의 큰 코루틴을 쓴 것과 이벤트버스라는 직관적인 네이밍이 인상적이에요.
해당 업데이트 올리기 전에 한번 기기로 QA 돌려야할 것 같아요! 월요일까지 테스트 해보고 화요일에 올리는거 어떠실까요?
Summary
토큰 리프레쉬 로직 리팩토링을 통해 기존 앱 실행 시 강종 문제 해결
Describe your changes
Issue
To reviewers
앱 실행 중 리프레쉬 만료 시는 영상과 같습니다.
앱을 사용하지 않는 경우에 만료된 경우는
TokenInterceptor가 백그라운드 작업이므로 로그아웃이 이미 되어앱 실행 시 intro화면에서 기존 로직으로 있는 토큰 유효성 검사로 인해서 앱 클릭하여 들어간 경우 로그인 화면으로 갑니다.default.mp4
기존 로직은 우선 주석 처리 했는데, 확인하시고 지워도 문제 없는 것 확인되면 주석 지우고 pr 업뎃할게요
2025.04.26 update
위 과정으로 수정되어 관심사 분리 및 안드로이드 아키텍처 구조로 수정되었습니다.