Skip to content

Conversation

@s9hn
Copy link
Member

@s9hn s9hn commented May 8, 2025

📌𝘐𝘴𝘴𝘶𝘦𝘴

📎𝘞𝘰𝘳𝘬 𝘋𝘦𝘴𝘤𝘳𝘪𝘱𝘵𝘪𝘰𝘯

소셜로그인에 대한 토큰 관리로직을 구현합니다.(Authenticator, Interceptor)

파일 수정 사항

:app 모듈

  • 기존 네트워크 모듈 제거
    • BuildConfig의 URL 제거
    • Retrofit DI 수정
  • 여전히 API 인터페이스가 남아 있어 네트워크 의존성은 유지
  • WebsosoApp
    • AuthSession 만료 이벤트 구독
  • WebsosoNavigator
    • :core:common 모듈의 NavigatorProvider를 구현하고 DI 모듈에 등록
  • WebsosoAuthSessionManager
    • :core:auth 모듈의 AuthSessionManager를 구현하고 DI 모듈에 등록
    • 토큰 만료 시 전역 이벤트 전파
  • 로그인 화면 Compose로 마이그레이션
    • 관련 리소스 및 XML 코드 제거

:core:auth 모듈

  • 소셜 로그인용 AuthSessionManager 인터페이스 제공

:core:common 모듈

  • DispatcherModule
    • 전역에서 사용할 디스패처를 명시적으로 주입
  • ThrottleHelper
    • suspend 함수에 쓰로틀링을 적용하는 헬퍼 클래스
  • NavigatorProvider
    • Android 의존성 없이 네비게이션을 추상화한 인터페이스 제공
    • 새로운 엔트리 포인트 정의로 어느 위치에서도 주입 가능

:core:datastore 모듈

  • RepositoryLocalDataSource 구현
  • 간단한 데이터 저장을 위한 Preference DataStore 방식 사용
  • Qualifier를 통해 각 데이터스토어를 개별 관리
  • 네트워크 모듈과 동일한 디렉터리 구조 유지

:core:network 모듈

Authenticator

  • 401 응답 시 authenticate() 호출로 토큰 재발급 시도
  • /reissue API 호출
    • 성공 시: 새 토큰 부착
    • 실패 시: 로그인 화면으로 이동
  • shouldSkipCondition()을 통해 다음 방지
    • 엑세스 토큰 없음
    • 재갱신 API에서도 401 발생 시 무한 루프
  • 뮤텍스로 동시성 제어
  • 스로틀링으로 다음 방지
    • 병렬 재발급 요청
    • 병렬 로그인 화면 호출
    • 무한 재시도 루프

Interceptor

  • 필요한 헤더에 엑세스 토큰 자동 주입

NetworkModule

  • OkHttp의 maxRequestPerHost 조정으로 교착상태 방지

:data:account 모듈

  • LocalDataSource 인터페이스 제공
  • 토큰 재갱신 API 및 토큰 저장 로직 구현

💬𝘛𝘰 𝘙𝘦𝘷𝘪𝘦𝘸𝘦𝘳𝘴

토큰 로직이 워낙 많은 의존성을 필요로 하고, 모듈 간 의존성, 객체 지향에 욕심내다보니 수정 사항이 너무 많아졌습니다 죄송합니다.
베스트 케이스로 구현하고 싶었는데 아무래도 Auth 관련 로직이다보니 관련 레퍼런스가 많이 없네요 :(
토큰 관련해서 아래와 같은 시나리오로 테스트 진행했습니다.

테스트 시나리오

엑세스 실험

홈뷰 진입 → 피드 → 서재 → 마이페이지 → 홈 (정상작동 확인)

리프레시 실험

홈뷰 → 마이페이지 → 로그인뷰 → 홈뷰 (정상작동 확인)

마이페이지 → 홈뷰 → 로그인뷰 → 홈뷰 (정상작동 확인)

👊Trouble Shooting

순환 참조

순환 참조

Trouble

  • AuthorizationAuthenticatorOkHttpClient 생성 시 필요
  • OkHttpClientRetrofit에 주입되며, Retrofit은 다시 AccountRepository가 API 통신을 위해 필요
  • 따라서 순환 구조 형성
    AuthorizationAuthenticator
    → AccountRepository
    → API Service
    → Retrofit
    → OkHttpClient
    → AuthorizationAuthenticator

Used Solution

  • Retrofit 인스턴스를 두 개로 나누고, @Qualifier로 구분하여 주입

New Solution

  • HiltProvider<AccountRepository>를 통해 Lazy 주입
  • AuthorizationAuthenticator가 실제로 AccountRepository를 사용하는 시점에만 인스턴스를 생성함으로써 순환 참조를 회피

Code

    private val accountRepository: Provider<AccountRepository>
401 무한 루프

401 무한 루프

Trouble

  • 기존 로직에서는 어떤 API 요청이 401 응답을 받았을 때, 토큰을 갱신하기 위한 API (reissue) 호출 역시 401을 반환할 경우, 계속해서 authenticate() 함수가 반복 호출되는 무한 루프가 발생

Solution

  • 다음 조건 중 하나라도 만족하면 authenticate()에서 null을 반환하여 재시도를 방지
    1. Authorization 헤더가 없는 경우 (헤더가 필요 없는 API)
    2. Response.priorResponse를 기반으로 최대 재시도 횟수(MAX_ATTEMPT_COUNT = 2) 초과한 경우

Code

   private fun shouldSkipCondition(response: Response): Boolean =
       response.request.header("Authorization").isNullOrBlank() ||
       response.retryAttemptCount() >= MAX_ATTEMPT_COUNT
         
   private fun Response.retryAttemptCount(): Int =
       generateSequence(this) { it.priorResponse }.count()
병렬 API 호출에 대한 동시성 문제 해결 방법

병렬 API 호출에 대한 동시성 문제 해결 방법

갱신 성공 시 동시성 문제 해결

Trouble

  • 여러 API가 동시에 호출될 때, 엑세스 토큰이 만료되면 각각의 요청은 401 리스폰스를 받고 authenticate 함수가 실행
  • 이를 제어하지 않으면 여러 스레드가 병렬적으로 토큰을 갱신하려고 시도하여 재갱신 API가 중복 호출

Solution

  • Mutex를 사용하여 첫 번째로 authenticate 함수에 진입한 스레드만 재갱신 API를 호출하고, 나머지 스레드는 해당 Mutex가 unlock될 때까지 대기
  • 최초로 갱신된 토큰을 저장하고, 이후 다른 스레드들은 갱신된 토큰을 사용하여 API 호출 진행

Code
mutex.withLock을 사용하여, 첫 번째로 진입한 스레드만 토큰 갱신을 처리하고 나머지 스레드는 결과 수신 대기

    val renewedToken = runBlocking(dispatcher) {
        mutex.withLock {  // 최초 스레드만 재갱신
            when (response.isRefreshNeeded()) {
                true -> throttle { renewToken() }  
                false -> return@withLock accountRepository.get().accessToken()
            }
        }
    }

갱신 실패 시 동시성 문제

Trouble

  • 만약 토큰 갱신에 실패하면, 다른 스레드는 이전에 실패한 갱신 상태를 인지하지 못하고 계속해서 로그인 뷰를 여러 번 시작

Solution

  • ThrottleHelper 클래스를 활용하여, 최초로 재갱신을 시도한 스레드가 성공 여부와 상관없이 일정 시간 동안 다른 스레드의 재갱신 API 호출을 차단
  • 이를 통해 중복된 요청을 방지하고, 로그인 뷰 반복 생성 차단

Code

  • 쓰로틀링을 통해 일정 시간(기본값 1000ms) 동안 재갱신 API 호출 및 결과처리 로직을 차단
        val renewedToken = runBlocking(dispatcher) {
            mutex.withLock {  
                when (response.isRefreshNeeded()) {
                    true -> throttle { renewToken() }  // 최초 스레드만 재갱신
                    false -> return@withLock accountRepository.get().accessToken()
                }
            }
        }
재갱신 실패 시 로그인 뷰로 리다이렉션

재갱신 실패 시 로그인 뷰로 리다이렉션

Trouble

  • 리프레시 만료 이벤트는 앱 전역에서 수집해야 하며, 이를 BaseActivity에서 수집하도록 구현
  • 그러나 로그인 뷰가 BaseActivity의 영향을 받아, 로그인 뷰가 다시 로그인 뷰를 호출하는 무한 루프 발생

Solution

  • App 클래스에서 AuthManagerNavigator를 관리
  • 프로세스 생명주기를 추적하여 백그라운드 상태에서는 이벤트 수집을 중지하고, 포어그라운드 상태에서는 이벤트 수집

Code

    private fun subscribeSessionState() {
        sessionManager.sessionExpired.collectWithLifecycle(
        ProcessLifecycleOwner.get()
        ) {
        navigatorProvider.navigateToLoginActivity()
        }
    }
네트워크 큐 제한

네트워크 큐 제한

Trouble

  • 네트워크 큐 제한에 의해 동시 다발적으로 N개의 API를 호출할 때, 특히 홈 뷰와 마이페이지에서 재갱신 API 호출이 안 되는 현상 발생
  • OkHttpmaxRequestsPerHost가 기본값인 5로 설정되어 있기 때문에, 동시에 호출되는 5개 이상의 요청이 큐에서 막히는 현상 발생
  • Mutex를 사용하여 동시성 제어를 적용한 이후, 교착 상태(데드락)가 발생

Solution

  • OkHttpmaxRequestsPerHost 값을 늘려서 네트워크 큐의 용량을 확장
  • 5개 이상의 동시 요청이 가능하도록 maxRequestsPerHost를 20으로 조정

Code

        private val dispatcher: Dispatcher by lazy {
            Dispatcher().apply {
                maxRequestsPerHost = 20
            }
        }

@s9hn s9hn added the ❌[DO NOT MERGE] 머지를 하면 안됩니다. label May 8, 2025
@github-actions github-actions bot requested review from junseo511, m6z1 and yeonjeen May 8, 2025 08:29
@coderabbitai
Copy link

coderabbitai bot commented May 13, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This set of changes refactors the authentication and session management infrastructure, migrating from legacy Kakao-specific logic to a new modular architecture using core modules for authentication, network, and data storage. New interfaces and implementations for session management, navigation, and throttling are introduced, while legacy data models, interceptors, and authenticators are removed. Dependency injection bindings, data models, and API interfaces are updated to reflect the new architecture, and redundant or obsolete files are deleted.

Changes

File(s) / Group Change Summary
app/build.gradle.kts, core/network/build.gradle.kts, core/auth/build.gradle.kts, core/datastore/build.gradle.kts Updated module dependencies and plugins to include new core modules and coroutine support.
app/src/main/java/com/into/websoso/WebsosoApp.kt, app/src/main/java/com/into/websoso/ui/login/LoginActivity.kt Injected new session manager and navigator dependencies, added session expiration handling.
app/src/main/java/com/into/websoso/core/common/util/navigator/WebsosoNavigator.kt, core/common/src/main/java/com/into/websoso/core/common/navigator/NavigatorProvider.kt Introduced navigator interface and implementation for activity navigation, with Hilt module and entry point.
app/src/main/java/com/into/websoso/core/common/util/sessionManager/WebsosoAuthSessionManager.kt, core/auth/src/main/java/com/into/websoso/core/auth/AuthSessionManager.kt Added session manager interface and implementation using shared flow for session expiration events, with Hilt binding.
core/common/src/main/java/com/into/websoso/core/common/dispatchers/DispatchersModule.kt Added coroutine dispatcher module and qualifier for dependency injection.
core/common/src/main/java/com/into/websoso/core/common/extensions/ThrottleHelper.kt Added throttle helper class for rate-limiting coroutine execution.
core/datastore/src/main/java/com/into/websoso/core/datastore/di/DataStoreModule.kt, core/datastore/src/main/java/com/into/websoso/core/datastore/di/DataStoreQualifier.kt, core/datastore/src/main/java/com/into/websoso/core/datastore/datasource/account/DefaultAccountDataSource.kt Introduced local data source for account tokens and preferences using DataStore, with qualifier annotation and updated DI module.
data/account/src/main/java/com/into/websoso/data/account/AccountRepository.kt, data/account/src/main/java/com/into/websoso/data/account/datasource/AccountLocalDataSource.kt, data/account/src/main/java/com/into/websoso/data/account/datasource/AccountRemoteDataSource.kt, data/account/src/main/java/com/into/websoso/data/account/model/AccountEntity.kt, data/account/src/main/java/com/into/websoso/data/account/model/TokenEntity.kt Refactored account repository and data sources to use new local and remote interfaces and models for tokens and registration state.
core/network/src/main/java/com/into/websoso/core/network/authenticator/AuthorizationAuthenticator.kt, core/network/src/main/java/com/into/websoso/core/network/interceptor/AuthorizationInterceptor.kt Added new OkHttp authenticator and interceptor for handling token renewal and authorization headers, supporting throttling and session expiration.
core/network/src/main/java/com/into/websoso/core/network/datasource/account/AccountApi.kt, core/network/src/main/java/com/into/websoso/core/network/datasource/account/DefaultAccountDataSource.kt, core/network/src/main/java/com/into/websoso/core/network/datasource/account/model/KakaoLoginResponseDto.kt, core/network/src/main/java/com/into/websoso/core/network/datasource/account/model/TokenReissueRequestDto.kt, core/network/src/main/java/com/into/websoso/core/network/datasource/account/model/TokenReissueResponseDto.kt Updated or added DTOs and APIs for login and token reissue, refactored data mapping, and adjusted visibility.
core/network/src/main/java/com/into/websoso/core/network/datasource/account/AccountDataSourceModule.kt Deleted old Dagger module for account data source binding; replaced by new module in DefaultAccountDataSource.kt.
core/network/src/main/java/com/into/websoso/core/network/di/NetworkModule.kt Refactored network module for new authenticator/interceptor, updated timeouts, dispatcher, and visibility.
feature/signin/src/main/java/com/into/websoso/feature/signin/SignInScreen.kt, feature/signin/src/main/java/com/into/websoso/feature/signin/SignInViewModel.kt Updated sign-in screen and view model to use new navigation and token saving logic, added navigation events.
app/src/main/java/com/into/websoso/data/di/ApiModule.kt Removed secured/unsecured qualifiers from API provider functions.
app/src/main/java/com/into/websoso/ui/main/home/HomeFragment.kt, app/src/main/java/com/into/websoso/ui/onboarding/OnboardingActivity.kt Removed unused injected repository and intent extras for onboarding.
Deleted legacy Kakao/auth files:
app/src/main/java/com/into/websoso/data/authenticator/WebsosoAuthenticator.kt, app/src/main/java/com/into/websoso/data/di/NetworkModule.kt, app/src/main/java/com/into/websoso/data/di/OAuthModule.kt, app/src/main/java/com/into/websoso/data/interceptor/AuthInterceptor.kt, app/src/main/java/com/into/websoso/data/mapper/KakaoTokenMapper.kt, app/src/main/java/com/into/websoso/data/mapper/LoginMapper.kt, app/src/main/java/com/into/websoso/data/model/LoginEntity.kt, app/src/main/java/com/into/websoso/data/model/OAuthToken.kt, app/src/main/java/com/into/websoso/data/qualifier/Auth.kt, app/src/main/java/com/into/websoso/data/qualifier/Interceptor.kt, app/src/main/java/com/into/websoso/data/remote/api/KakaoAuthService.kt, app/src/main/java/com/into/websoso/data/remote/api/OAuthService.kt, app/src/main/java/com/into/websoso/data/remote/response/KakaoLoginResponseDto.kt, app/src/main/java/com/into/websoso/data/remote/response/KakaoTokenReissueResponseDto.kt, app/src/main/java/com/into/websoso/ui/login/LoginViewModel.kt, app/src/main/java/com/into/websoso/ui/login/model/LoginUiState.kt, data/account/src/main/java/com/into/websoso/data/account/AccountEntity.kt
Removed legacy Kakao authentication, token, and login data models, mappers, interceptors, authenticators, and related DI modules.
app/src/main/java/com/into/websoso/data/remote/api/AuthApi.kt, app/src/main/java/com/into/websoso/data/repository/AuthRepository.kt Removed Kakao login and token reissue API endpoints and repository methods.
app/src/main/res/layout/activity_login.xml, app/src/main/res/drawable/bg_login_white_radius_14dp_stroke_primary100_1dp.xml Removed all content from login activity layout and deleted unused drawable resource.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant App
    participant AuthSessionManager
    participant NavigatorProvider
    participant AccountRepository
    participant AuthorizationAuthenticator

    User->>App: Uses app (session may expire)
    App->>AuthSessionManager: Observes sessionExpired flow
    AuthSessionManager-->>App: Emits sessionExpired event
    App->>NavigatorProvider: navigateToLoginActivity()
    NavigatorProvider->>App: Starts LoginActivity

    Note over AuthorizationAuthenticator, AccountRepository: On HTTP 401 response
    AuthorizationAuthenticator->>AccountRepository: renewToken()
    AccountRepository->>AccountRepository: Refreshes tokens
    AuthorizationAuthenticator->>AuthSessionManager: onSessionExpired() (if renewal fails)
    AuthSessionManager-->>App: Emits sessionExpired event
Loading

Poem

🐇
A hop, a leap, a bounding stride—
Old tokens gone, new flows supplied!
With sessions watched and throttles set,
Navigation’s safe, no need to fret.
From network core to storage deep,
The rabbit’s code now guards your sleep.
Hoppily onward, bugs retreat!


🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@s9hn
Copy link
Member Author

s9hn commented May 13, 2025

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented May 13, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@s9hn s9hn added 🍯 [FEAT] 새로운 기능을 개발합니다. ♻️ [REFACTOR] 기존 코드를 리팩토링합니다. ⚖️ 재상 세훈 전지적 세훈 시점 and removed ❌[DO NOT MERGE] 머지를 하면 안됩니다. labels May 13, 2025
@s9hn s9hn changed the title Feat/663 refactor: 소셜로그인 재구현 May 13, 2025
@s9hn s9hn changed the title refactor: 소셜로그인 재구현 refactor: 토큰 Interceptor 및 Authenticator 구현 May 13, 2025
@coderabbitai
Copy link

coderabbitai bot commented May 13, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🧹 Nitpick comments (14)
core/common/src/main/java/com/into/websoso/core/common/dispatchers/DispatchersModule.kt (1)

25-27: Consider adding additional dispatcher types for future extensibility

Currently, the enum only defines IO, but you might want to consider adding other dispatcher types (like Main, Default, etc.) to support different coroutine contexts throughout the application.

enum class WebsosoDispatchers {
    IO,
+   Main,
+   Default,
+   Unconfined,
}
app/src/main/java/com/into/websoso/WebsosoApp.kt (1)

30-34: Excellent implementation of session expiration handling

The use of ProcessLifecycleOwner ensures session expiration is handled regardless of which activity is in the foreground. This approach correctly handles authentication across the entire application lifecycle.

Consider whether you need to clear activity stack when navigating to login (not critical):

private fun subscribeSessionState() {
    sessionManager.sessionExpired.collectWithLifecycle(ProcessLifecycleOwner.get()) {
-        navigatorProvider.navigateToLoginActivity()
+        // Consider if flags need to be set to clear the back stack
+        navigatorProvider.navigateToLoginActivity()
    }
}
core/network/src/main/java/com/into/websoso/core/network/datasource/account/AccountApi.kt (1)

22-25: Well-defined token reissue endpoint

The postReissueToken endpoint is properly defined with appropriate annotations. This enables token refresh functionality critical for maintaining authenticated sessions.

Consider adding KDoc to document the endpoint's purpose and expected behavior:

/**
 * Requests a token reissue when the current token is expired.
 * @param tokenReissueRequestDto Contains the refresh token used to obtain new tokens
 * @return New access and refresh tokens if the refresh token is valid
 */
@POST("reissue")
suspend fun postReissueToken(
    @Body tokenReissueRequestDto: TokenReissueRequestDto,
): TokenReissueResponseDto
app/src/main/java/com/into/websoso/core/common/util/navigator/WebsosoNavigator.kt (1)

16-35: Well-implemented navigator with clear navigation methods

The WebsosoNavigator class properly implements the NavigatorProvider interface with clear, concise navigation methods. Each method follows a consistent pattern of creating an intent via the activity's static getIntent method.

Consider adding flags for certain navigation scenarios:

override fun navigateToLoginActivity() {
    val intent = LoginActivity.getIntent(context)
+   intent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK)
    context.startActivity(intent)
}

Also consider adding KDoc to document the class and its methods.

core/network/src/main/java/com/into/websoso/core/network/datasource/account/model/TokenReissueResponseDto.kt (1)

14-19: Clean data transformation with toData method

The toData() method provides a clean conversion from the DTO to the domain entity TokenEntity, following good practices for separating network and domain models.

Consider adding a nullability check or validation for the authorization and refreshToken fields:

internal fun toData(): TokenEntity =
    TokenEntity(
-       accessToken = authorization,
-       refreshToken = refreshToken,
+       accessToken = authorization.takeIf { it.isNotBlank() } ?: throw IllegalStateException("Authorization token cannot be empty"),
+       refreshToken = refreshToken.takeIf { it.isNotBlank() } ?: throw IllegalStateException("Refresh token cannot be empty"),
    )
data/account/src/main/java/com/into/websoso/data/account/datasource/AccountLocalDataSource.kt (1)

1-15: Clear and concise data source interface

The AccountLocalDataSource interface provides a clean contract for accessing and storing account-related data locally. The separation of token-related operations into distinct functions follows the single responsibility principle and allows for more granular control.

Consider adding a function to clear all account data during logout for better completeness of the interface.

 interface AccountLocalDataSource {
     suspend fun accessToken(): String

     suspend fun refreshToken(): String

     suspend fun isAutoLogin(): Boolean

     suspend fun saveAccessToken(accessToken: String)

     suspend fun saveRefreshToken(refreshToken: String)

     suspend fun saveIsAutoLogin(isAutoLogin: Boolean)
+
+    suspend fun clearAccountData()
 }
core/datastore/src/main/java/com/into/websoso/core/datastore/datasource/account/DefaultAccountDataSource.kt (2)

19-23: Add error handling for DataStore operations

The DataStore read operations could throw exceptions but don't have any error handling.

Add try-catch blocks to handle potential exceptions during DataStore operations:

 override suspend fun accessToken(): String =
+    try {
         accountDataStore.data
             .map { preferences ->
                 preferences[ACCESS_TOKEN].orEmpty()
             }.first()
+    } catch (e: Exception) {
+        // Log error
+        ""
+    }

Also applies to: 25-29, 31-35


55-59: Consider adding documentation for preference keys

The preference keys are well-defined in the companion object, but adding documentation would improve maintainability.

Add KDoc comments explaining each preference key's purpose:

 companion object {
+    /** Key for storing the user's access token */
     private val ACCESS_TOKEN = stringPreferencesKey("ACCESS_TOKEN")
+    /** Key for storing the user's refresh token */
     private val REFRESH_TOKEN = stringPreferencesKey("REFRESH_TOKEN")
+    /** Key for storing whether auto-login is enabled */
     private val IS_LOGIN = booleanPreferencesKey("IS_LOGIN")
 }
core/network/src/main/java/com/into/websoso/core/network/di/NetworkModule.kt (1)

22-26: Document timeout constant values

The timeout constants are well-defined, but adding documentation would help explain the rationale behind these specific values.

Add comments explaining why these specific timeout values were chosen:

-    private const val CONNECT_TIME_LIMIT = 60L
-    private const val READ_TIME_LIMIT = 30L
-    private const val WRITE_TIME_LIMIT = 15L
+    /** Connection timeout in seconds - longer to account for poor network conditions */
+    private const val CONNECT_TIME_LIMIT = 60L
+    /** Read timeout in seconds - for larger responses */
+    private const val READ_TIME_LIMIT = 30L
+    /** Write timeout in seconds - for request body transmission */
+    private const val WRITE_TIME_LIMIT = 15L
core/network/src/main/java/com/into/websoso/core/network/interceptor/AuthorizationInterceptor.kt (1)

36-39: Improve path exclusion logic

The current implementation might have false positives if a path contains one of the excluded strings as a substring.

Use more precise path matching:

 private fun shouldSkipCondition(request: Request): Boolean =
-    EXCLUDED_PATHS.any { path ->
-        request.url.encodedPath.contains(path)
-    }
+    EXCLUDED_PATHS.any { path ->
+        request.url.encodedPath.endsWith(path) || 
+        request.url.encodedPath.contains("/$path/")
+    }
data/account/src/main/java/com/into/websoso/data/account/AccountRepository.kt (2)

11-12: Remove TODO comment before merging

This TODO comment should be resolved before merging.

Either implement the referenced singleton pattern or remove the TODO comment.


42-49: Justify or remove arbitrary delay in token renewal

There's a hardcoded 100ms delay in the token renewal process without explanation.

Either remove the arbitrary delay or add a comment explaining its purpose:

 accountLocalDataSource.saveAccessToken(tokens.accessToken)
 accountLocalDataSource.saveRefreshToken(tokens.refreshToken)
-delay(100)
+// Allow time for DataStore to complete the write operation
+delay(100)
 return tokens.accessToken

If this delay is needed for DataStore operations to complete, consider a more robust approach like awaiting the completion of the write operations.

core/network/src/main/java/com/into/websoso/core/network/authenticator/AuthorizationAuthenticator.kt (1)

37-50: Blocking runBlocking is acceptable but keep eye on thread-starvation

Authenticator.authenticate() is already invoked on an OkHttp thread; using
runBlocking(IO) is fine, yet the mutex + throttling means this call will
block that thread for the whole refresh round-trip.
Consider off-loading heavy I/O (network) to a dedicated dispatcher or using
await() style non-blocking refresh if refresh times grow.

app/src/main/java/com/into/websoso/data/di/ApiModule.kt (1)

23-61: Reduce boilerplate with a generic API factory

All provider functions share the identical body. A small helper keeps the file
concise and avoids copy-paste errors when adding new APIs.

+private inline fun <reified T> Retrofit.createApi(): T = create(T::class.java)
+
 @Provides
 @Singleton
-fun provideAuthApi(retrofit: Retrofit): AuthApi = retrofit.create(AuthApi::class.java)
+fun provideAuthApi(retrofit: Retrofit): AuthApi = retrofit.createApi()
...
-fun provideVersionApi(retrofit: Retrofit): VersionApi = retrofit.create(VersionApi::class.java)
+fun provideVersionApi(retrofit: Retrofit): VersionApi = retrofit.createApi()
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7f608e7 and 4a920ba.

📒 Files selected for processing (55)
  • app/build.gradle.kts (1 hunks)
  • app/src/main/java/com/into/websoso/WebsosoApp.kt (1 hunks)
  • app/src/main/java/com/into/websoso/core/common/util/navigator/WebsosoNavigator.kt (1 hunks)
  • app/src/main/java/com/into/websoso/core/common/util/sessionManager/WebsosoAuthSessionManager.kt (1 hunks)
  • app/src/main/java/com/into/websoso/data/authenticator/WebsosoAuthenticator.kt (0 hunks)
  • app/src/main/java/com/into/websoso/data/di/ApiModule.kt (1 hunks)
  • app/src/main/java/com/into/websoso/data/di/NetworkModule.kt (0 hunks)
  • app/src/main/java/com/into/websoso/data/di/OAuthModule.kt (0 hunks)
  • app/src/main/java/com/into/websoso/data/interceptor/AuthInterceptor.kt (0 hunks)
  • app/src/main/java/com/into/websoso/data/mapper/KakaoTokenMapper.kt (0 hunks)
  • app/src/main/java/com/into/websoso/data/mapper/LoginMapper.kt (0 hunks)
  • app/src/main/java/com/into/websoso/data/model/LoginEntity.kt (0 hunks)
  • app/src/main/java/com/into/websoso/data/model/OAuthToken.kt (0 hunks)
  • app/src/main/java/com/into/websoso/data/qualifier/Auth.kt (0 hunks)
  • app/src/main/java/com/into/websoso/data/qualifier/Interceptor.kt (0 hunks)
  • app/src/main/java/com/into/websoso/data/remote/api/AuthApi.kt (0 hunks)
  • app/src/main/java/com/into/websoso/data/remote/api/KakaoAuthService.kt (0 hunks)
  • app/src/main/java/com/into/websoso/data/remote/api/OAuthService.kt (0 hunks)
  • app/src/main/java/com/into/websoso/data/remote/response/KakaoLoginResponseDto.kt (0 hunks)
  • app/src/main/java/com/into/websoso/data/remote/response/KakaoTokenReissueResponseDto.kt (0 hunks)
  • app/src/main/java/com/into/websoso/data/repository/AuthRepository.kt (0 hunks)
  • app/src/main/java/com/into/websoso/ui/login/LoginActivity.kt (3 hunks)
  • app/src/main/java/com/into/websoso/ui/login/LoginViewModel.kt (0 hunks)
  • app/src/main/java/com/into/websoso/ui/login/model/LoginUiState.kt (0 hunks)
  • app/src/main/java/com/into/websoso/ui/main/home/HomeFragment.kt (0 hunks)
  • app/src/main/java/com/into/websoso/ui/onboarding/OnboardingActivity.kt (0 hunks)
  • app/src/main/res/drawable/bg_login_white_radius_14dp_stroke_primary100_1dp.xml (0 hunks)
  • app/src/main/res/layout/activity_login.xml (0 hunks)
  • core/auth/build.gradle.kts (1 hunks)
  • core/auth/src/main/java/com/into/websoso/core/auth/AuthSessionManager.kt (1 hunks)
  • core/common/src/main/java/com/into/websoso/core/common/dispatchers/DispatchersModule.kt (1 hunks)
  • core/common/src/main/java/com/into/websoso/core/common/extensions/ThrottleHelper.kt (1 hunks)
  • core/common/src/main/java/com/into/websoso/core/common/navigator/NavigatorProvider.kt (1 hunks)
  • core/datastore/build.gradle.kts (1 hunks)
  • core/datastore/src/main/java/com/into/websoso/core/datastore/datasource/account/DefaultAccountDataSource.kt (1 hunks)
  • core/datastore/src/main/java/com/into/websoso/core/datastore/di/DataStoreModule.kt (1 hunks)
  • core/datastore/src/main/java/com/into/websoso/core/datastore/di/DataStoreQualifier.kt (1 hunks)
  • core/network/build.gradle.kts (1 hunks)
  • core/network/src/main/java/com/into/websoso/core/network/authenticator/AuthorizationAuthenticator.kt (1 hunks)
  • core/network/src/main/java/com/into/websoso/core/network/datasource/account/AccountApi.kt (2 hunks)
  • core/network/src/main/java/com/into/websoso/core/network/datasource/account/AccountDataSourceModule.kt (0 hunks)
  • core/network/src/main/java/com/into/websoso/core/network/datasource/account/DefaultAccountDataSource.kt (2 hunks)
  • core/network/src/main/java/com/into/websoso/core/network/datasource/account/model/KakaoLoginResponseDto.kt (2 hunks)
  • core/network/src/main/java/com/into/websoso/core/network/datasource/account/model/TokenReissueRequestDto.kt (1 hunks)
  • core/network/src/main/java/com/into/websoso/core/network/datasource/account/model/TokenReissueResponseDto.kt (1 hunks)
  • core/network/src/main/java/com/into/websoso/core/network/di/NetworkModule.kt (2 hunks)
  • core/network/src/main/java/com/into/websoso/core/network/interceptor/AuthorizationInterceptor.kt (1 hunks)
  • data/account/src/main/java/com/into/websoso/data/account/AccountEntity.kt (0 hunks)
  • data/account/src/main/java/com/into/websoso/data/account/AccountRepository.kt (1 hunks)
  • data/account/src/main/java/com/into/websoso/data/account/datasource/AccountLocalDataSource.kt (1 hunks)
  • data/account/src/main/java/com/into/websoso/data/account/datasource/AccountRemoteDataSource.kt (1 hunks)
  • data/account/src/main/java/com/into/websoso/data/account/model/AccountEntity.kt (1 hunks)
  • data/account/src/main/java/com/into/websoso/data/account/model/TokenEntity.kt (1 hunks)
  • feature/signin/src/main/java/com/into/websoso/feature/signin/SignInScreen.kt (2 hunks)
  • feature/signin/src/main/java/com/into/websoso/feature/signin/SignInViewModel.kt (2 hunks)
💤 Files with no reviewable changes (24)
  • app/src/main/java/com/into/websoso/data/qualifier/Interceptor.kt
  • app/src/main/java/com/into/websoso/data/interceptor/AuthInterceptor.kt
  • app/src/main/java/com/into/websoso/data/qualifier/Auth.kt
  • app/src/main/java/com/into/websoso/data/remote/response/KakaoTokenReissueResponseDto.kt
  • app/src/main/java/com/into/websoso/data/mapper/LoginMapper.kt
  • app/src/main/res/drawable/bg_login_white_radius_14dp_stroke_primary100_1dp.xml
  • app/src/main/java/com/into/websoso/ui/onboarding/OnboardingActivity.kt
  • data/account/src/main/java/com/into/websoso/data/account/AccountEntity.kt
  • app/src/main/java/com/into/websoso/data/remote/api/OAuthService.kt
  • app/src/main/res/layout/activity_login.xml
  • app/src/main/java/com/into/websoso/data/di/OAuthModule.kt
  • app/src/main/java/com/into/websoso/data/repository/AuthRepository.kt
  • app/src/main/java/com/into/websoso/data/mapper/KakaoTokenMapper.kt
  • app/src/main/java/com/into/websoso/data/remote/response/KakaoLoginResponseDto.kt
  • app/src/main/java/com/into/websoso/data/authenticator/WebsosoAuthenticator.kt
  • app/src/main/java/com/into/websoso/data/model/OAuthToken.kt
  • app/src/main/java/com/into/websoso/ui/main/home/HomeFragment.kt
  • app/src/main/java/com/into/websoso/data/model/LoginEntity.kt
  • app/src/main/java/com/into/websoso/ui/login/model/LoginUiState.kt
  • app/src/main/java/com/into/websoso/ui/login/LoginViewModel.kt
  • app/src/main/java/com/into/websoso/data/remote/api/KakaoAuthService.kt
  • core/network/src/main/java/com/into/websoso/core/network/datasource/account/AccountDataSourceModule.kt
  • app/src/main/java/com/into/websoso/data/remote/api/AuthApi.kt
  • app/src/main/java/com/into/websoso/data/di/NetworkModule.kt
🧰 Additional context used
🧬 Code Graph Analysis (3)
core/network/build.gradle.kts (1)
build-logic/src/main/kotlin/com/into/websoso/WebsosoDependenciesExtensions.kt (1)
  • implementation (70-76)
core/datastore/build.gradle.kts (1)
build-logic/src/main/kotlin/com/into/websoso/WebsosoDependenciesExtensions.kt (1)
  • implementation (70-76)
data/account/src/main/java/com/into/websoso/data/account/AccountRepository.kt (1)
core/network/src/main/java/com/into/websoso/core/network/datasource/account/model/TokenReissueRequestDto.kt (1)
  • refreshToken (6-10)
🔇 Additional comments (45)
core/common/src/main/java/com/into/websoso/core/common/extensions/ThrottleHelper.kt (1)

11-42: Well-structured throttling mechanism with proper thread safety

This ThrottleHelper class provides a clean, thread-safe implementation for operation throttling using coroutines. The use of Mutex ensures proper synchronization when checking and updating the execution timestamp.

A few thoughts:

  • The invoke operator function provides a convenient syntax for callers
  • Using TimeSource.Monotonic is appropriate for measuring elapsed time accurately
  • The null return when throttled requires careful handling by callers
core/common/src/main/java/com/into/websoso/core/common/dispatchers/DispatchersModule.kt (2)

11-17: Clean Dagger Hilt module implementation for dispatchers

The module correctly provides the IO dispatcher as a singleton with proper Hilt annotations.


19-23: Well-designed custom qualifier for type-safe dispatcher injection

The @Dispatcher qualifier with the enum parameter allows for type-safe injection of different dispatcher types.

core/auth/build.gradle.kts (1)

1-5: Added coroutines support to auth module

Adding the websoso.kotlin.coroutines plugin is appropriate for enabling coroutine support in the auth module, which is necessary for the new authentication mechanism implementation.

core/network/build.gradle.kts (1)

34-35: Added dependency on core.common module

This dependency is required to use the newly created utilities like ThrottleHelper and dispatcher modules in the network implementation. This change aligns with the architectural refactoring being done for authentication.

data/account/src/main/java/com/into/websoso/data/account/model/AccountEntity.kt (1)

3-6: Good encapsulation of account data with token entity

The data class effectively encapsulates account information by grouping the token data into a separate entity. However, consider adding KDoc documentation to clarify the purpose of the isRegister property - does it indicate if a user is registered or if registration is needed?

core/datastore/build.gradle.kts (1)

12-15:

Details

✅ Verification successful

Improved dependency organization with clear categorization

The added comments improve readability by clearly categorizing dependencies, making the build file more maintainable. Good practice to separate data layer dependencies from library dependencies.


🏁 Script executed:

#!/bin/bash
# Verify there's no circular dependency between data.account and core.datastore
echo "Checking if data.account depends on core.datastore..."
fd -p "data/account/build\.gradle\.kts$" | xargs cat | grep -i "datastore"

Length of output: 191


No circular dependency found between data.account and core.datastore

The added comments in core/datastore/build.gradle.kts improve readability by clearly categorizing dependencies, making the build file more maintainable. It’s a good practice to separate data-layer dependencies from library dependencies.

– Confirmed that data/account/build.gradle.kts contains no references to datastore, so there’s no circular dependency.

app/src/main/java/com/into/websoso/ui/login/LoginActivity.kt (2)

32-33: Proper injection of navigation provider

The navigator is correctly injected using Hilt's @Inject annotation, adhering to dependency inversion by using the interface type.


45-45: Correctly passing navigator to composable

The injected navigator is properly passed to the SignInScreen, enabling centralized navigation handling.

core/network/src/main/java/com/into/websoso/core/network/datasource/account/model/TokenReissueRequestDto.kt (1)

1-7: Improved encapsulation with internal visibility modifier

Changing the DTO visibility to internal is a good practice as it:

  1. Restricts usage to within the network module
  2. Prevents leaking network-specific data models to other layers
  3. Enforces clear architectural boundaries

The package relocation also better organizes the code by placing the DTO in a domain-specific location.

data/account/src/main/java/com/into/websoso/data/account/model/TokenEntity.kt (1)

3-6: Well-structured data class for token management.

This data class provides a clean encapsulation of token data with immutable properties, following proper Kotlin practices. It supports the overall refactoring from legacy authentication to a more modular approach.

core/datastore/src/main/java/com/into/websoso/core/datastore/di/DataStoreQualifier.kt (1)

5-7: Appropriate use of Dagger qualifier.

The @AccountDataStore qualifier is properly defined with the correct annotations and visibility modifiers. Making it internal properly restricts its usage to within the module, following good encapsulation practices.

app/build.gradle.kts (2)

79-79: Proper dependency addition for new architecture.

Adding the core.common module dependency aligns with the architectural refactoring to improve navigation and provide foundational utilities across the application.


83-83: Good integration of datastore module.

This dependency addition properly integrates the new datastore module which handles token persistence, supporting the new authentication and session management approach.

core/auth/src/main/java/com/into/websoso/core/auth/AuthSessionManager.kt (1)

5-9: Well-designed authentication session manager interface.

This interface follows good design principles:

  • Uses SharedFlow for broadcasting session expiration events to multiple observers
  • Provides a suspending function for handling expiration asynchronously
  • Maintains a focused responsibility on session management

The design enables proper separation of concerns between authentication state management and UI navigation.

core/datastore/src/main/java/com/into/websoso/core/datastore/di/DataStoreModule.kt (1)

21-23: LGTM - Adding the AccountDataStore qualifier aligns with the modular token management architecture

The addition of the @AccountDataStore qualifier and changing the function visibility to internal are well-aligned with the broader refactoring that modularizes account token management. This provides more explicit dependency injection control and better encapsulation.

feature/signin/src/main/java/com/into/websoso/feature/signin/SignInViewModel.kt (2)

48-60: Well-implemented error handling with clear navigation outcomes

The updated implementation properly handles success and failure cases when saving tokens. Using runCatching provides a clean way to propagate errors while ensuring the appropriate navigation path is triggered based on registration status.


89-91: Good addition of navigation UI effects

These new UI effects appropriately support the navigation architecture refactoring, allowing for clear separation between home and onboarding navigation paths based on user registration status.

data/account/src/main/java/com/into/websoso/data/account/datasource/AccountRemoteDataSource.kt (2)

5-6: LGTM - Clean import of the new entity models

The imports of the new AccountEntity and TokenEntity models are appropriate for the updated interface methods.


12-14: Well-designed interface for token management

The explicit return type AccountEntity for postLogin and the addition of postReissue method provide a clean contract for token issuance and renewal. This change supports the new token management architecture effectively.

feature/signin/src/main/java/com/into/websoso/feature/signin/SignInScreen.kt (3)

21-26: LGTM - Appropriate imports for the new navigation system

The import of NavigatorProvider and direct imports of UI effect objects simplify the code and align with the new navigation architecture.


36-36: Good integration of the NavigatorProvider

Adding the websosoNavigator parameter enables proper dependency injection of navigation capabilities, aligning with the refactored architecture.


44-57: Clean implementation of navigation event handling

The event handling is concise and directly invokes the appropriate navigator methods. The updated syntax using direct object matching rather than is keyword checks is more elegant.

app/src/main/java/com/into/websoso/WebsosoApp.kt (3)

5-9: Good imports organization for Lifecycle and Auth components

The addition of ProcessLifecycleOwner, AuthSessionManager and NavigatorProvider imports indicate a proper separation of concerns for authentication and navigation.


16-20: Well-structured dependency injection

The use of constructor injection for SessionManager and NavigatorProvider follows best practices for Hilt dependency injection in Android applications.


26-26: Good initialization of session management

Calling subscribeSessionState() during onCreate() ensures session management is properly initialized during application startup.

core/network/src/main/java/com/into/websoso/core/network/datasource/account/AccountApi.kt (2)

3-5: Appropriate imports for token reissue functionality

The imports for TokenReissueRequestDto and TokenReissueResponseDto are correctly added to support the new token reissue endpoint.


33-33: Good encapsulation with internal visibility modifier

Changing provideAccountApi to internal visibility improves encapsulation by restricting access to the module, following the principle of least privilege.

app/src/main/java/com/into/websoso/core/common/util/navigator/WebsosoNavigator.kt (2)

3-15: Appropriate imports for navigation and dependency injection

The imports cover all necessary dependencies for navigation (LoginActivity, MainActivity, OnboardingActivity) and Hilt dependency injection annotations.


37-43: Properly configured Hilt module for navigation

The NavigatorModule correctly binds the WebsosoNavigator implementation to the NavigatorProvider interface as a singleton, ensuring consistent navigation throughout the app.

core/network/src/main/java/com/into/websoso/core/network/datasource/account/model/TokenReissueResponseDto.kt (2)

3-6: Appropriate imports for serialization and domain model conversion

The imports correctly include the necessary Kotlin serialization annotations and the TokenEntity for conversion.


7-13: Well-structured DTO with proper serialization annotations

The TokenReissueResponseDto is properly annotated with @serializable and each field has the correct @SerialName annotation to match the API response format.

core/network/src/main/java/com/into/websoso/core/network/datasource/account/model/KakaoLoginResponseDto.kt (2)

1-1: Package and import paths properly updated

The package and import paths have been updated to reflect the new organization structure, moving models to dedicated model packages. This change improves code organization and follows proper separation of concerns.

Also applies to: 3-4


17-22: Improved token encapsulation with TokenEntity

The refactoring properly encapsulates authentication tokens within a dedicated TokenEntity rather than storing them as separate fields in AccountEntity. This change supports better separation of concerns and aligns with the broader authentication refactoring.

core/common/src/main/java/com/into/websoso/core/common/navigator/NavigatorProvider.kt (2)

1-13: Well-designed navigation interface

The NavigatorProvider interface clearly defines the navigation responsibilities with well-named methods for each destination. This abstraction will allow for better testing and separation of concerns by decoupling navigation logic from UI components.


15-19: Proper Hilt EntryPoint implementation

The NavigatorEntryPoint is correctly set up as a Hilt EntryPoint installed in the SingletonComponent, providing a way to obtain the NavigatorProvider in contexts where field injection isn't possible (like in non-Hilt classes).

app/src/main/java/com/into/websoso/core/common/util/sessionManager/WebsosoAuthSessionManager.kt (3)

1-13: Appropriate imports and package structure

The imports and package structure follow good practices, importing only what's needed and organizing the code in an appropriate package hierarchy.


14-23: Well-implemented session expiration handling

The WebsosoAuthSessionManager effectively implements the AuthSessionManager interface using a SharedFlow to broadcast session expiration events. The use of extraBufferCapacity=1 ensures that emissions aren't missed when there are no active collectors, which is important for session expiration events.


25-31: Proper Hilt module and binding

The WebsosoAuthSessionManagerModule correctly uses @BINDS to provide the implementation of AuthSessionManager, and the @singleton scope ensures only one instance exists throughout the app lifetime, which is appropriate for session management.

core/datastore/src/main/java/com/into/websoso/core/datastore/datasource/account/DefaultAccountDataSource.kt (1)

37-41: LGTM - DataStore write operations implementation

The DataStore write operations are implemented correctly using the edit function.

Also applies to: 43-47, 49-53

core/network/src/main/java/com/into/websoso/core/network/di/NetworkModule.kt (2)

28-37: LGTM - Efficient usage of lazy initialization

Good use of lazy initialization for HttpLoggingInterceptor and Dispatcher to avoid unnecessary instantiation until needed.


58-71: LGTM - Well-structured OkHttpClient configuration

The OkHttpClient configuration is well-structured with appropriate timeout settings, interceptors, and authenticator.

core/network/src/main/java/com/into/websoso/core/network/interceptor/AuthorizationInterceptor.kt (1)

19-21: Good use of Provider for AccountRepository

Using Provider helps avoid potential circular dependencies in the dependency injection graph.

data/account/src/main/java/com/into/websoso/data/account/AccountRepository.kt (2)

19-21: LGTM - Token accessor methods

These methods provide a clean way to access tokens from the local data source.


32-39: LGTM - Token storage logic

The token storage logic is well-implemented, saving both tokens and handling the auto-login state correctly.

Comment on lines +14 to +20
internal class DefaultAccountDataSource
@Inject
constructor(
@AccountDataStore private val accountDataStore: DataStore<Preferences>,
) : AccountLocalDataSource {
override suspend fun accessToken(): String =
accountDataStore.data
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Local token storage implementation needs security enhancements

While DataStore is a good choice for storing preferences, sensitive authentication tokens should be stored more securely.

Consider using the Android Keystore System or an encryption library like Tink to encrypt the tokens before storing them in DataStore. This would protect the tokens from being extracted if the device is compromised.

Comment on lines +42 to +46
private val EXCLUDED_PATHS = listOf(
"auth/login/kakao",
"reissue",
"minimum-version",
)
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Handle empty tokens in Authorization header

The interceptor doesn't check if the token is empty before adding it to the header.

Add null/empty token check to avoid sending invalid Authorization headers:

 val token = runBlocking(dispatcher) { accountRepository.get().accessToken() }
+
+if (token.isBlank()) {
+    return chain.proceed(request)
+}
+
 val newRequest = request
     .newBuilder()
     .addHeader("Authorization", "Bearer $token")
     .build()

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines 27 to 34
val token = runBlocking(dispatcher) { accountRepository.get().accessToken() }
val newRequest = request
.newBuilder()
.addHeader("Authorization", "Bearer $token")
.build()

return chain.proceed(newRequest)
}
Copy link

@coderabbitai coderabbitai bot May 13, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid using runBlocking in interceptors

Using runBlocking in an interceptor can block the OkHttp thread, potentially causing performance issues or ANRs if token retrieval is slow.

Consider refactoring to avoid blocking operations in the interceptor. Possible approaches:

  1. Use a synchronous method in AccountRepository specifically for the interceptor
  2. Implement a token cache that's updated asynchronously but read synchronously
  3. Pre-fetch tokens before making network requests
-            val token = runBlocking(dispatcher) { accountRepository.get().accessToken() }
+            // Option 1: Use a synchronous method
+            val token = accountRepository.get().getAccessTokenSync()
+            
+            // Option 2: Use a cached token (implement caching in repository)
+            val token = accountRepository.get().getCachedAccessToken()

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Member Author

Choose a reason for hiding this comment

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

@coderabbitai
but accountRepository.get().refreshToken() is suspend function from Datastore, so it can be called in suspend block and also we have to controll the concurrency problem using 'Mutex'.

Copy link

Choose a reason for hiding this comment

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

@s9hn You're right that accessToken() and refreshToken() are suspend functions from DataStore that need to be called from a coroutine context, and concurrency control is important.

The concern with runBlocking isn't about correctness but performance - it blocks the OkHttp thread pool while waiting for DataStore operations to complete, which could impact network request throughput.

A better approach might be:

  1. Implement in-memory caching of the token with background refresh:
class TokenManager @Inject constructor(
    private val accountRepository: AccountRepository,
    @ApplicationScope private val scope: CoroutineScope
) {
    private val _token = MutableStateFlow<String?>(null)
    val token: StateFlow<String?> = _token

    init {
        scope.launch {
            accountRepository.accessToken()
                .also { _token.value = it }
        }
    }
    
    // Non-blocking function for interceptors
    fun getToken(): String? = token.value
}
  1. Then in your interceptor:
val token = tokenManager.getToken() ?: return chain.proceed(request)

This keeps the token in memory while updating it asynchronously, avoiding OkHttp thread blocking.


✏️ Learnings added
Learnt from: s9hn
PR: Team-WSS/WSS-Android#676
File: core/network/src/main/java/com/into/websoso/core/network/interceptor/AuthorizationInterceptor.kt:27-34
Timestamp: 2025-05-14T07:41:05.683Z
Learning: Accessing DataStore via `runBlocking` in OkHttp interceptors can block network threads and cause performance issues. Consider using in-memory caching with background updates instead to achieve thread safety without blocking OkHttp threads.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Copy link
Member Author

Choose a reason for hiding this comment

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

Still Mutex can be called in Coroutine context.

Copy link

Choose a reason for hiding this comment

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

@s9hn You're correct that Mutex is designed to be used within coroutine contexts for concurrency control. However, the primary concern with your current implementation isn't about using Mutex, but about using runBlocking in the interceptor.

Even with proper concurrency control using Mutex inside the suspended function, when you call it with runBlocking, you're still blocking OkHttp's thread pool while waiting for DataStore operations to complete.

If you want to keep using Mutex for concurrency control, you could still implement a non-blocking approach:

class TokenManager @Inject constructor(
    private val accountRepository: AccountRepository,
    @ApplicationScope private val scope: CoroutineScope
) {
    private val _token = MutableStateFlow<String?>(null)
    val token: StateFlow<String?> = _token
    private val mutex = Mutex()
    
    init {
        scope.launch {
            refreshToken() // Initial load
        }
    }
    
    // This runs in background, protected by Mutex
    private suspend fun refreshToken() {
        mutex.withLock {
            accountRepository.accessToken()
                .also { _token.value = it }
        }
    }
    
    // Non-blocking function for interceptors
    fun getToken(): String? = _token.value
}

This way, you get the thread safety benefits of Mutex while still avoiding OkHttp thread blocking.

Comment on lines 42 to 49
suspend fun renewToken(): String {
val tokens = accountRemoteDataSource.postReissue(refreshToken = refreshToken())

accountLocalDataSource.saveAccessToken(tokens.accessToken)
accountLocalDataSource.saveRefreshToken(tokens.refreshToken)
delay(100)
return tokens.accessToken
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling for token renewal

The renewToken function doesn't handle potential errors during the token renewal process.

Add try-catch block to handle network or storage failures:

 suspend fun renewToken(): String {
+    try {
         val tokens = accountRemoteDataSource.postReissue(refreshToken = refreshToken())
         
         accountLocalDataSource.saveAccessToken(tokens.accessToken)
         accountLocalDataSource.saveRefreshToken(tokens.refreshToken)
         delay(100)
         return tokens.accessToken
+    } catch (e: Exception) {
+        // Log error
+        throw TokenRenewalException("Failed to renew token", e)
+    }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
suspend fun renewToken(): String {
val tokens = accountRemoteDataSource.postReissue(refreshToken = refreshToken())
accountLocalDataSource.saveAccessToken(tokens.accessToken)
accountLocalDataSource.saveRefreshToken(tokens.refreshToken)
delay(100)
return tokens.accessToken
}
suspend fun renewToken(): String {
try {
val tokens = accountRemoteDataSource.postReissue(refreshToken = refreshToken())
accountLocalDataSource.saveAccessToken(tokens.accessToken)
accountLocalDataSource.saveRefreshToken(tokens.refreshToken)
delay(100)
return tokens.accessToken
} catch (e: Exception) {
// Log error
throw TokenRenewalException("Failed to renew token", e)
}
}

Comment on lines +24 to +31
): AccountEntity =
when (platform) {
AuthPlatform.KAKAO ->
accountApi
.postLoginWithKakao(
accessToken = authToken.accessToken,
).toData()
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Non-exhaustive when causes compilation failure & hides unsupported platforms

AuthPlatform is an enum/sealed type but only one branch (KAKAO) is handled and there is no else.
Kotlin requires exhaustive handling; this will not compile as soon as another value (e.g. APPLE) exists.
In addition, silently ignoring unsupported platforms would be dangerous – fail fast instead.

-        ): AccountEntity =
-            when (platform) {
-                AuthPlatform.KAKAO ->
-                    accountApi
-                        .postLoginWithKakao(
-                            accessToken = authToken.accessToken,
-                        ).toData()
-            }
+        ): AccountEntity = when (platform) {
+            AuthPlatform.KAKAO -> accountApi
+                .postLoginWithKakao(
+                    accessToken = authToken.accessToken,
+                ).toData()
+            // TODO: add proper implementations
+            else -> throw IllegalArgumentException("Unsupported auth platform: $platform")
+        }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
): AccountEntity =
when (platform) {
AuthPlatform.KAKAO ->
accountApi
.postLoginWithKakao(
accessToken = authToken.accessToken,
).toData()
}
): AccountEntity = when (platform) {
AuthPlatform.KAKAO -> accountApi
.postLoginWithKakao(
accessToken = authToken.accessToken,
).toData()
// TODO: add proper implementations
else -> throw IllegalArgumentException("Unsupported auth platform: $platform")
}

Comment on lines 52 to 57
return renewedToken.let { token ->
response.request
.newBuilder()
.header("Authorization", "Bearer $token")
.build()
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Null token can still be injected ⇒ infinite 401 loop

renewedToken may be null (no refresh token, refresh failed …), yet it is interpolated
into the header "Bearer $token", producing "Bearer null".
That will provoke another 401 and may trap clients in a retry loop.

-            return renewedToken.let { token ->
-                response.request
-                    .newBuilder()
-                    .header("Authorization", "Bearer $token")
-                    .build()
-            }
+            val token = renewedToken ?: return null      // give OkHttp a chance to propagate 401
+
+            return response.request
+                .newBuilder()
+                .header("Authorization", "Bearer $token")
+                .build()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return renewedToken.let { token ->
response.request
.newBuilder()
.header("Authorization", "Bearer $token")
.build()
}
val token = renewedToken ?: return null // give OkHttp a chance to propagate 401
return response.request
.newBuilder()
.header("Authorization", "Bearer $token")
.build()

Copy link
Member

@junseo511 junseo511 left a comment

Choose a reason for hiding this comment

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

리뷰가 조금 늦었습니다! 몇가지 코멘트만 확인해주시면 감사하겠습니다 👍
작업이,,, 오래 걸릴 만 했네요,,, 고생 많으셨어요 👏

Comment on lines +17 to +22
private val _sessionExpired = MutableSharedFlow<Unit>(replay = 1, extraBufferCapacity = 1)
override val sessionExpired: SharedFlow<Unit> get() = _sessionExpired.asSharedFlow()

override suspend fun onSessionExpired() {
_sessionExpired.emit(Unit)
}
Copy link
Member

Choose a reason for hiding this comment

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

c: Unit을 가진 SharedFlow가 있는 이유가 있으실까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

AuthSessionManager는 단발성 트리거로 '세션이 만료되었음' 만을 전파합니다.
해당 이벤트는 단 1회만 수신해야하며, 별 다른 상태를 가질 필요가 없습니다!
추후 해당 이벤트 분기가 추가된다면, 이벤트 상태 객체가 추가될테지만 현재로썬 달리 추가될 기능이 없어보입니다!


@Module
@InstallIn(SingletonComponent::class)
internal interface AccountDataSourceModule {
Copy link
Member

@junseo511 junseo511 May 14, 2025

Choose a reason for hiding this comment

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

c: 모든 모듈을 주입받는 객체가 있는 파일에 두시는 이유가 있으실까요? 혹시 이 파일을 포함해 다른 파일에서도 bind할 항목이 늘어난다면 어떻게 처리하실건지 궁금합니다

Copy link
Member Author

Choose a reason for hiding this comment

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

해당 객체의 인스턴스 제공을 해당 객체가 담긴 파일 내부에서 하는 것 뿐입니다!
이는 마치 A액티비티의 인스턴스를 A액티비티가 제공하는 것과도 같지 않나요?
bind할 항목이 늘어난다는 것은 해당 인터페이스의 구현체가 본 구현체와 다르다는 것이고, 해당 구현체는 해당 구현체 파일내부에서 인스턴스를 제공하는게 좀 더 자연스럽다고 생각하는데 어떻게 생각하시나요!

private val throttle: ThrottleHelper,
@Dispatcher(WebsosoDispatchers.IO) private val dispatcher: CoroutineDispatcher,
) : Authenticator {
private val mutex: Mutex = Mutex()
Copy link
Member

@junseo511 junseo511 May 14, 2025

Choose a reason for hiding this comment

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

c: 이친구는 처음보는 것 같은데 혹시 간략하게 역할을 알려주실 수 있나요 ?.?

Copy link
Member Author

Choose a reason for hiding this comment

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

진~짜 간략하게, 뮤텍스는 1인 화장실에 비유하곤 합니다.
하나의 스레드가 화장실을 사용(lock)하면, 다른 스레드는 대기큐에서 대기하게 됩니다.
사용중인 스레드가 화장실을 나오면(unlock)하면, 그 다음 순번의 스레드가 입장합니다!
동시 접근을 막는 가장 기본적인 방법 중 하나입니다!

.build()
}

private fun shouldSkipCondition(response: Response): Boolean =
Copy link
Member

Choose a reason for hiding this comment

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

c: shouldSkipCondition -> isSkipRequired 는 어떠신가요? Condition이 Boolean값인지 조금 모호할 수 있을 것 같아요

Copy link
Member

@junseo511 junseo511 May 14, 2025

Choose a reason for hiding this comment

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

c: val이 아닌 fun으로 두신 이유가 있으실까요? 이정도는 val이 더 가독성이 좋아 보이기도 합니다...!

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 +19 to +21
suspend fun accessToken(): String = accountLocalDataSource.accessToken()

suspend fun refreshToken(): String = accountLocalDataSource.refreshToken()
Copy link
Member

Choose a reason for hiding this comment

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

r: 함수명에 적합하지 않는 것 같아요! access는 접근하다 refresh는 새로고침하다인데 String을 반환하기에 적합한 함수명일까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

동사 + 명사가 아닌 '명사'만을 나타내긴 했습니다!
datastore는 알다시피 중단함수가 필요하기에, 단순히 엑세스토큰, 리프레시토큰에 대한 값만을 가져오고 싶은데 함수 형태가 강제되어 저렇게 네이밍했습니다.
이런건 안봐주나요?


accountLocalDataSource.saveAccessToken(tokens.accessToken)
accountLocalDataSource.saveRefreshToken(tokens.refreshToken)
delay(100)
Copy link
Member

@junseo511 junseo511 May 14, 2025

Choose a reason for hiding this comment

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

r: 100의 딜레이를 주신 이유가 있을까요? 앞 작업이 끝남을 기다리기 위해? 🤔 혹시 저장 작업이 제대로 이루어지지 않으면 어떤 결과가 일어날까요?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🍯 [FEAT] 새로운 기능을 개발합니다. ♻️ [REFACTOR] 기존 코드를 리팩토링합니다. ⚖️ 재상 세훈 전지적 세훈 시점

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants