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] 구글 자동 로그인을 구현 한다. #129

Merged
merged 26 commits into from
Nov 12, 2024

Conversation

no1msh
Copy link
Contributor

@no1msh no1msh commented Nov 6, 2024

resolved #126

AS-IS

수동 로그인이었던 구글 로그인을

TO-BE

자동 로그인 한다.

KEY-POINT

  • 현재 병일님이 스프링 시큐리티 정책 설정 중이라 조금 변경 되거나 작동이 안될 순 있는데 리뷰를 위하여 먼저 올려놓습니다!

SCREENSHOT (Optional)

Nov-06-2024 14-33-15

@no1msh no1msh requested a review from librarywon November 6, 2024 05:35
@no1msh no1msh self-assigned this Nov 6, 2024
Copy link
Contributor

@librarywon librarywon left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 간단한 리뷰사항만 반영하거나 답변을 주시면 감사드리겠습니다

추가적으로 로그아웃 -> 재로그인 -> 앱종료 -> 앱시작 시에 앱이 아래와 오류와 함께 터지는 현상을 확인하였습니다!

java.lang.IllegalArgumentException: Cannot have an empty start destination route

import okhttp3.Route
import javax.inject.Inject

class DefaultAuthenticator
Copy link
Contributor

Choose a reason for hiding this comment

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

Default 보다 저희 프로젝트에 맞게 커스텀 했다는 의미를 볼 수 있게 HY2Authenticator로 명명하시는 것은 어떻게 생각하시나요?

@@ -0,0 +1,7 @@
package com.benenfeldt.remote.token

enum class TokenRole {
Copy link
Contributor

Choose a reason for hiding this comment

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

카톡으로 이야기를 나누었지만 USER, GUEST, ADMIN의 의미가 혼동이 올 수 있을꺼 같았는데 주석을 작성해두는 것은 어떨까요

Copy link
Contributor

Choose a reason for hiding this comment

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

GUESTUNREGISTERED_USER로만 변경해도 혼동사항은 없을꺼 같기도 합니다!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GUEST는 서버에서 사용한 네이밍임으로 통일성을 위해 일단은 그냥 두겠습니다
추후 게스트로그인 등을 구현시 처리하겠습니다!

@@ -24,6 +29,8 @@ class InitialViewModel
@Inject
constructor(
private val webViewRepository: WebViewRepository,
private val jwtManager: JwtManager,
Copy link
Contributor

Choose a reason for hiding this comment

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

이전 PR에서 다루지 못해서 죄송하지만 JwtManager가 현재 GoogleIdToken까지 관리를 하고 있는데 그러면 JwtManager이라는 이름의 역할을 넘어서는 일을 하는 것 같습니다. TokenManager 정도로 변경하는 것은 어떤가요? 분리하기엔 너무 관리 주체가 나눠지는거 같아서

when (tokenInformation.role) {
TokenRole.USER -> setStartDestination(Home.ROUTE)
TokenRole.GUEST -> setStartDestination(SignUp.ROUTE)
TokenRole.ADMIN -> resetToken()
Copy link
Contributor

Choose a reason for hiding this comment

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

관리자도 USER와 동일하게 HOME으로 보내주지 않고 있는데 따로 이유가 있을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

관리자는 앱상에서 로그인 할 케이스가 없기 때문에 비정상적 로그인으로 판단하고 토큰을 리셋합니다.

Comment on lines +15 to +19
val intent =
Intent(context, MainActivity::class.java).apply {
flags = Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK
}
context.startActivity(intent)
Copy link
Contributor

Choose a reason for hiding this comment

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

컴포즈에서 Intent를 쓰니까 새롭군요 ㅋㅋㅋㅋ

Copy link
Contributor

@librarywon librarywon left a comment

Choose a reason for hiding this comment

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

QA 수고하셨습니다!

@librarywon librarywon merged commit 9924872 into develop Nov 12, 2024
1 check passed
@librarywon librarywon deleted the Feature/#126-auto-login branch November 16, 2024 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] 구글 로그인 자동로그인을 구현합니다.
2 participants