-
Notifications
You must be signed in to change notification settings - Fork 2
[feat/#135] google login 세팅 #141
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
sonms
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.
고생하셨습니다~! 홧팅홧팅
| // 토큰 관리를 위한 키들 | ||
| private val ACCESS_TOKEN_KEY = stringPreferencesKey("access_token") | ||
| private val REFRESH_TOKEN_KEY = stringPreferencesKey("refresh_token") | ||
|
|
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.
access token과 refresh token은 평문으로 저장할 경우 보안에 문제가 생길 수 있으니 EncryptedSharedPreferences 을 사용해서 암호화해서 저장해보는 것은 어떨까 싶습니다!
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.
아 더 찾아보니 Tink라는 구글의 암호화 라이브러리를 사용하는 것이 더 좋아보이네요! 현재 EncryptedSharedPreferences는 보안 취약점이 있기도 했고, 개발도 중단 된 거 같네요.. Tink 관련해서는 Medium에서 확인해보시면 좋을 것 같습니다! 래퍼런스가 많이 없긴한데 편하신 걸로 구현하시면 될 것 같습니다!
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.
현재 EncryptedSharedPreferences 로 작업하고 있긴했는데, Tink 로 한 번 적용해서 구현해보겠습니다. 자료감사합니당
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.
취소.. 참조할만한 자료가 마땅치않네용 나중에 마이그레이션하던지 합시당
| compileOptions { | ||
| sourceCompatibility = JavaVersion.VERSION_11 | ||
| targetCompatibility = JavaVersion.VERSION_11 | ||
| isCoreLibraryDesugaringEnabled = true |
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.
👍
gradle/libs.versions.toml
Outdated
| v2All = "2.20.1" | ||
|
|
||
| credentials = "1.6.0-alpha03" |
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.
alpha 버전 보단 원체 credential이 오류가 많은 친구라 안정화 버전을 씁시당 1.5.0으로 변경해주세요!
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 AuthRemoteDataSourceImpl @Inject constructor( |
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.
google은 google sdk 관련 책임과 역할로, AuthRemoteDataSourceImpl는 우리 백엔드와 관련된 책임과 역할이기 때문에 분리해주시면 더 좋을 것 같습니다! google sdk에서 우리 백엔드 연결 방식을 전혀 몰라야겠지요?
| interface GoogleAuthDataSource { | ||
| suspend fun signIn(context: Context): Result<GoogleIdTokenCredential> | ||
| } | ||
|
|
||
| interface AuthRemoteDataSource { | ||
| suspend fun login(providerToken: String, provider: String): BaseResponse<LoginResponseDto> | ||
| } No newline at end of file |
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.
여기도 책임과 역할에 맞게 각자 분리~
|
|
||
|
|
||
| interface LoginService { | ||
| @POST("api/v1/auth/login") |
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.
저희 아마 api/v1까지라서 auth/login 까지만 사용하시면 됩니다!
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.
주말에 백엔드분들께 전달받으면 바로 수정하겠습니다!
|
|
||
| interface AuthRepository { | ||
| suspend fun signInWithGoogle(context: Context): Result<String> | ||
| suspend fun login(providerToken: String, provider: String): Result<LoginResponseDto> |
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는 여기서 사용하는게 아니죵? 다시 한 번 확인해보고 presentation -> domain -> data의 의존성을 확인하고 domain의 repository를 통해 presentation의 viewmodel에 의존성을 주입 받을 때를 생각해봅시다!
| @Composable | ||
| fun TrackingCard( | ||
| onClick: () -> Unit, | ||
| modifier: Modifier = Modifier |
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.
상위에서 modifier를 받았는데 아래에 최상위 컴포저블에 modifier는 여전히 Modifier를 사용하는 것 같은데 확인 부탁드려요~
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.
이거 pull 받았을때 오류나서 일단 수정해놓긴했는데, 한번더 확인해보겠습니다
| try { | ||
| val result = regionCurrentRepository.RegionCurrent(userId.first()) | ||
| val result = regionCurrentRepository.regionCurrent(userId.first()) |
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.
Result를 사용해서 구현했으니 try-catch문을 제거하고 중복된 코드를 제거해도 좋을 것 같아요!
| val state: StateFlow<LoginContract.LoginState> | ||
| get() = _state.asStateFlow() | ||
|
|
||
| private val _sideEffect = MutableStateFlow<LoginContract.LoginSideEffect?>(null) | ||
| val sideEffect : StateFlow<LoginContract.LoginSideEffect?> | ||
| val sideEffect: StateFlow<LoginContract.LoginSideEffect?> |
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.
아직 LoginContract가 {}로 묶여있는 거 같은데 여유있으실 때 없애주시면 좋을 것 같아요~!
JiWoo1261
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.
수고햇더염
ISSUE
❗ WORK DESCRIPTION
📸 SCREENSHOT
📢 TO REVIEWERS