Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/Projects/BKData/Sources/API/UserAPI.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import Foundation
enum UserAPI {
case me
case termsAgreement(termsAgreed: Bool)
case upsertFCMToken(fcmToken: String)
case upsertFCMToken(fcmToken: String, deviceId: String)
case upsertNotificationSettings(notificationEnabled: Bool)
}

Expand Down Expand Up @@ -51,8 +51,8 @@ extension UserAPI: RequestTarget {
switch self {
case .termsAgreement(let termsAgreed):
return TermsAgreementRequestDTO(termsAgreed: termsAgreed)
case .upsertFCMToken(let fcmToken):
return UpsertFCMTokenRequestDTO(fcmToken: fcmToken)
case .upsertFCMToken(let fcmToken, let deviceId):
return UpsertFCMTokenRequestDTO(fcmToken: fcmToken, deviceId: deviceId)
case .upsertNotificationSettings(let notificationEnabled):
return NotificationStatusRequestDTO(notificationEnabled: notificationEnabled)
case .me:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@ import Foundation

struct UpsertFCMTokenRequestDTO: Encodable {
let fcmToken: String
let deviceId: String
}
6 changes: 5 additions & 1 deletion src/Projects/BKData/Sources/DataAssembly.swift
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,11 @@ public struct DataAssembly: Assembly {
type: NotificationRepository.self
) { _ in
@Autowired(name: "OAuth") var networkProvider: NetworkProvider
return DefaultNotificationRepository(networkProvider: networkProvider)
@Autowired var deviceIDProvider: DeviceIDProvider
return DefaultNotificationRepository(
networkProvider: networkProvider,
deviceIDProvider: deviceIDProvider
)
}

container.register(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Copyright © 2025 Booket. All rights reserved

public protocol DeviceIDProvider {
var deviceID: String? { get }
func clearCache()
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Copyright © 2025 Booket. All rights reserved

import BKDomain
import Combine

public protocol DeviceIDStore {
func getOrCreate() -> AnyPublisher<String, TokenError>
func clear() -> AnyPublisher<Void, TokenError>
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,27 @@ import Combine

public struct DefaultNotificationRepository: NotificationRepository {
private let networkProvider: NetworkProvider

public init(networkProvider: NetworkProvider) {
private let deviceIDProvider: DeviceIDProvider

public init(
networkProvider: NetworkProvider,
deviceIDProvider: DeviceIDProvider
) {
self.networkProvider = networkProvider
self.deviceIDProvider = deviceIDProvider
}

public func upsertFCMToken(
fcmToken: String
) -> AnyPublisher<Void, DomainError> {
networkProvider.request(
target: UserAPI.upsertFCMToken(fcmToken: fcmToken),
guard let deviceID = deviceIDProvider.deviceID else {
Log.error("Device ID not available", logger: AppLogger.storage)
return Fail(error: DomainError.unknown)
.eraseToAnyPublisher()
}

return networkProvider.request(
target: UserAPI.upsertFCMToken(fcmToken: fcmToken, deviceId: deviceID),
type: UserProfileResponseDTO.self
)
.debugError(logger: AppLogger.network)
Expand Down
1 change: 1 addition & 0 deletions src/Projects/BKStorage/Sources/Constant/StorageKeys.swift
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,5 @@ public enum StorageKeys {
public static let refreshTokenKey = "refreshToken"
public static let fcmTokenKey = "fcmToken"
public static let isSyncNeededKey = "isSyncNeeded"
public static let deviceIDKey = "deviceID"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Copyright © 2025 Booket. All rights reserved

import BKCore
import BKData
import OSLog

public final class KeychainDeviceIDProvider: DeviceIDProvider {
private let storage: KeyValueStorage
private var cachedDeviceID: String?

public init(storage: KeyValueStorage) {
self.storage = storage
}

public var deviceID: String? {
if let cachedDeviceID {
return cachedDeviceID
}
do {
let id: String = try storage.load(for: StorageKeys.deviceIDKey)
self.cachedDeviceID = id
return id
} catch {
Log.error("Failed to load deviceID: \(error)", logger: AppLogger.storage)
return nil
}
}
Comment on lines +7 to +27
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

스레드 안전성 문제: 캐시 접근이 동기화되지 않음

cachedDeviceID는 여러 스레드에서 동시에 접근될 수 있지만 동기화 메커니즘이 없습니다. 특히 deviceID 프로퍼티에서 캐시를 읽고(line 16-18), 저장소에서 로드한 후 캐시에 쓰는(line 21) 작업이 원자적이지 않아 레이스 컨디션이 발생할 수 있습니다.

다음 중 하나의 방법으로 해결하세요:

방법 1: DispatchQueue를 사용한 동기화

 public final class KeychainDeviceIDProvider: DeviceIDProvider {
     private let storage: KeyValueStorage
     private var cachedDeviceID: String?
+    private let queue = DispatchQueue(label: "com.booket.deviceIDProvider")
 
     public init(storage: KeyValueStorage) {
         self.storage = storage
     }
     
     public var deviceID: String? {
-        if let cachedDeviceID {
-            return cachedDeviceID
-        }
-        do {
-            let id: String = try storage.load(for: StorageKeys.deviceIDKey)
-            self.cachedDeviceID = id
-            return id
-        } catch {
-            Log.error("Failed to load deviceID: \(error)", logger: AppLogger.storage)
-            return nil
+        return queue.sync {
+            if let cachedDeviceID {
+                return cachedDeviceID
+            }
+            do {
+                let id: String = try storage.load(for: StorageKeys.deviceIDKey)
+                self.cachedDeviceID = id
+                return id
+            } catch {
+                Log.error("Failed to load deviceID: \(error)", logger: AppLogger.storage)
+                return nil
+            }
         }
     }
     
     public func clearCache() {
-        cachedDeviceID = nil
+        queue.sync {
+            cachedDeviceID = nil
+        }
     }

방법 2: actor 사용 (iOS 13+ 지원 시)

actor를 사용하면 자동으로 스레드 안전성이 보장되지만, deviceID가 동기 프로퍼티에서 비동기로 변경되어야 합니다.

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


public func clearCache() {
cachedDeviceID = nil
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Copyright © 2025 Booket. All rights reserved

import BKData
import BKDomain
import Combine
import Foundation

public struct KeychainDeviceIDStore: DeviceIDStore {
private let storage: KeyValueStorage

/// AppDelegate나 다른 곳에서 DI 없이 사용할 수 있도록 shared instance 제공
public static let shared = KeychainDeviceIDStore(storage: KeychainKeyValueStorage())

public init(storage: KeyValueStorage) {
self.storage = storage
}

/// 디바이스 ID를 가져오거나, 없으면 새로 생성하여 저장
/// 키체인에 저장되므로 앱 삭제 후 재설치해도 유지됨
Copy link

@coderabbitai coderabbitai bot Nov 8, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

부정확한 주석: 키체인의 앱 재설치 시 동작

주석에서 "앱 삭제 후 재설치해도 유지됨"이라고 명시되어 있으나, 기본 키체인 항목은 앱 삭제 시 함께 삭제됩니다. 키체인 데이터를 유지하려면 별도의 접근 제어 설정(예: kSecAttrAccessGroup)과 앱 그룹 구성이 필요합니다.

주석을 수정하여 실제 동작을 정확히 반영하세요:

-    /// 디바이스 ID를 가져오거나, 없으면 새로 생성하여 저장
-    /// 키체인에 저장되므로 앱 삭제 후 재설치해도 유지됨
+    /// 디바이스 ID를 가져오거나, 없으면 새로 생성하여 저장
+    /// 키체인에 저장되므로 앱 업데이트나 재시작 시에도 유지됨 (앱 삭제 시에는 제거됨)

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

🤖 Prompt for AI Agents
In src/Projects/BKStorage/Sources/DeviceStorage/KeychainDeviceIDStore.swift
around line 19, the comment stating "앱 삭제 후 재설치해도 유지됨" is inaccurate; update the
comment to explain that by default keychain items may be removed when an app is
uninstalled and that retaining keychain data across uninstall/reinstall requires
explicit configuration (e.g., using kSecAttrAccessGroup and configuring an App
Group/Keychain Access Group or other secure backup strategies). Replace the
current statement with a concise, accurate note about default behavior and the
steps/requirements needed to preserve keychain entries across app
reinstallations.

Copy link
Member Author

Choose a reason for hiding this comment

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

kSecAttrAccessGroup는 다른 이야기를 하고 있는거같은데. 이건 공유를 위해서 쓰는 키고.
실제로 키체인은 앱 재설치 이후에도 남아있는걸로 알고 있는데 ? 다시 확인해볼 필요가 있을 듯.

Copy link

Choose a reason for hiding this comment

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

🧩 Analysis chain

🌐 Web query:

iOS keychain persistence after app uninstall reinstall

💡 Result:

Short answer: By default, Keychain items generally survive an app uninstall and are available after reinstall — because keychain storage is kept by the OS, separate from the app sandbox. They are only removed if the device/keychain is wiped or if the items are explicitly deleted (or the keychain is reset). [1][2][3][4]

Important caveats:

  • The app must use the same keychain access group / code‑signing team/entitlements; otherwise the new install won’t be able to read the old items. [2][3]
  • iCloud Keychain sync or backups can affect which items persist/are restored. [3]
  • If you need “fresh” installs to start clean, detect first-launch (NSUserDefaults cleared on uninstall) and explicitly remove keychain items on first run, or revoke/remove server tokens before uninstall. [2][4]

Sources: Apple Keychain docs and community/engineering reports. [1][2][3][4]


@clxxrlove 말씀하신 것이 맞습니다. 죄송합니다. 제가 잘못된 정보를 드렸네요.

확인 결과, iOS 키체인 아이템은 기본적으로 앱 삭제 후 재설치해도 유지됩니다. 키체인 스토리지는 앱 샌드박스가 아닌 OS 레벨에서 별도로 관리되기 때문입니다.

말씀하신 대로 kSecAttrAccessGroup은 앱 간 키체인 공유를 위한 것이고, 재설치 후 지속성과는 별개의 개념입니다.

따라서 현재 코드의 주석 **"키체인에 저장되므로 앱 삭제 후 재설치해도 유지됨"**은 정확합니다. 제 리뷰 코멘트가 부정확했습니다.

참고사항:

  • 재설치 후 동일한 코드 서명/엔타이틀먼트를 사용해야 기존 키체인 아이템에 접근 가능합니다
  • 디바이스를 초기화하거나 명시적으로 삭제하지 않는 한 키체인 데이터는 유지됩니다

지적해주셔서 감사합니다! 🙏


🧠 Learnings used
Learnt from: doyeonk429
Repo: YAPP-Github/Reed-iOS PR: 68
File: src/Projects/BKDesign/PreviewApp/Sources/View/BKButtonTestViewController.swift:0-0
Timestamp: 2025-07-10T08:21:44.204Z
Learning: doyeonk429는 BKDesign PreviewApp의 테스트 파일에서 주석 처리된 코드를 의도적으로 남겨두어 쉽게 테스트 구성을 전환할 수 있도록 구현한다. 이는 개발 편의성을 위한 것으로, 테스트 파일에서는 일반적인 "주석 처리된 코드 제거" 규칙을 적용하지 않는다.

Learnt from: doyeonk429
Repo: YAPP-Github/Reed-iOS PR: 163
File: src/Projects/BKData/Sources/DTO/Request/UserBookRegisterRequestDTO.swift:7-9
Timestamp: 2025-08-08T16:13:31.712Z
Learning: Reed-iOS에서 서버 스펙 변경으로 DTO 필드명이 바뀔 때, doyeonk429는 코드 전반의 파라미터/시그니처 리네이밍까지 확장하지 않고, 실제 요청 직렬화 키가 맞는지(예: isbn13)만 보장하면 충분하다는 범위 최소화 방식을 선호한다. (PR: YAPP-Github/Reed-iOS#163, 파일: BKData/Sources/DTO/Request/UserBookRegisterRequestDTO.swift)

Learnt from: doyeonk429
Repo: YAPP-Github/Reed-iOS PR: 163
File: src/Projects/BKData/Sources/DTO/Response/UserBookResponseDTO.swift:0-0
Timestamp: 2025-08-08T16:14:35.659Z
Learning: Reed-iOS: 서버/DTO 응답 키는 isbn13로 정규화되었음(사용자 doyeonk429 확인). Domain은 현재 BookInfo가 isbn을 사용 중이라 혼재 상태이며, 향후 isbn13으로 통일 리팩터 필요 가능성이 있음.

public func getOrCreate() -> AnyPublisher<String, TokenError> {
do {
if let existingID: String = try? storage.load(for: StorageKeys.deviceIDKey) {
return Just(existingID)
.setFailureType(to: TokenError.self)
.eraseToAnyPublisher()
}

let newDeviceID = UUID().uuidString
try storage.save(newDeviceID, for: StorageKeys.deviceIDKey)

Comment on lines +28 to +30
Copy link
Member

Choose a reason for hiding this comment

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

실제 디바이스 아이디 UIDevice.current.identifierForVendor!.uuidString 이런식으로 구할 수 있는걸로 알고 있긴 한데, 이거랑 UUID랑 크게 역할이 다르지 않을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

애초에 FCM 토큰 외에 deviceId를 별도로 두는 이유는 FCM은 앱 재설치 등의 변수를 통해 변할 수 있기 떄문입니다. 이러면 deviceId를 딱히 쓰는 의미는 없다고 생각하구요. (생명 주기가 같으므로)
말씀해주신 IDFV는 실제 디바이스 아이디는 아니구요, Team ID 단위로 부여되는 Identifier인데, FCM이랑 마찬가지로 앱 재설치하는 경우 변합니다.
UUID가 이런걸 대체해주기보다는, 정확히는 키체인을 믿고 설계한겁니다. 키체인은 앱 재설치/기기 초기화 등 변수에서도 남아있을 수 있어요. 초기화의 경우에는 리스토어를 해줘야 돌아오긴 하지만요.

추가로, 실제 디바이스 ID에 해당하는 IMEI나 MAC 같은 정말 PK가 될 수 있는 값들은 정책상 접근 금지이므로 이런 꼼수를 통해 deviceId를 공유한거에요.

나름 조금 알아보고 했는데, 혹시라도 제 의견이 틀렸거나 다른 의견이 있으시면 말씀해주세요. 개선 방향이 있는지 더 알아볼게요.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

아 네 저도 일단 IDFV 생각하고 말씀드린거긴 한데, 이게 앱 재설치 했을 때 값이 변할 수 있다는 사실을 제가 몰랐었네요! 지금 방식도 충분한데, 왜 안쓰셨는지 궁금해서 여쭤봤습니다! 👍

return Just(newDeviceID)
.setFailureType(to: TokenError.self)
.eraseToAnyPublisher()
} catch {
return Fail(error: TokenError.saveFailed(underlying: error))
.eraseToAnyPublisher()
}
}
Comment on lines +20 to +38
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

레이스 컨디션: getOrCreate()의 check-then-act 패턴이 원자적이지 않음

getOrCreate() 메서드에서 저장소 확인(line 22)과 생성/저장(lines 28-29)이 별도의 작업으로 분리되어 있어, 여러 스레드나 여러 호출이 동시에 실행되면 중복된 UUID가 생성될 수 있습니다. 특히 앱 초기화 시 여러 컴포넌트에서 동시에 호출될 가능성이 있습니다.

저장소 레이어에 동기화 메커니즘을 추가하거나, 이 메서드 자체를 동기화 처리하세요:

+import Foundation
+
 public struct KeychainDeviceIDStore: DeviceIDStore {
     private let storage: KeyValueStorage
+    private static let lock = NSLock()
     
     /// AppDelegate나 다른 곳에서 DI 없이 사용할 수 있도록 shared instance 제공
     public static let shared = KeychainDeviceIDStore(storage: KeychainKeyValueStorage())

     public init(storage: KeyValueStorage) {
         self.storage = storage
     }
     
     /// 디바이스 ID를 가져오거나, 없으면 새로 생성하여 저장
     /// 키체인에 저장되므로 앱 삭제 후 재설치해도 유지됨
     public func getOrCreate() -> AnyPublisher<String, TokenError> {
+        Self.lock.lock()
+        defer { Self.lock.unlock() }
+        
         do {
             if let existingID: String = try? storage.load(for: StorageKeys.deviceIDKey) {
                 return Just(existingID)
                     .setFailureType(to: TokenError.self)
                     .eraseToAnyPublisher()
             }
             
             let newDeviceID = UUID().uuidString
             try storage.save(newDeviceID, for: StorageKeys.deviceIDKey)

             return Just(newDeviceID)
                 .setFailureType(to: TokenError.self)
                 .eraseToAnyPublisher()
         } catch {
             return Fail(error: TokenError.saveFailed(underlying: error))
                 .eraseToAnyPublisher()
         }
     }

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

🤖 Prompt for AI Agents
In src/Projects/BKStorage/Sources/DeviceStorage/KeychainDeviceIDStore.swift
around lines 20 to 38, the getOrCreate() method uses a non-atomic check-then-act
sequence which can produce duplicate UUIDs under concurrent calls; make the
operation atomic by serializing access or using a concurrency primitive: either
protect the entire get-or-create sequence with a dedicated serial DispatchQueue
or lock (or convert to a Swift actor) so only one caller can check and create at
a time, or alternatively attempt to write the new UUID and on write-failure
re-load the stored value and return it; update error handling to reflect the
chosen approach and ensure the returned publisher always contains the single
canonical device ID.


public func clear() -> AnyPublisher<Void, TokenError> {
do {
try storage.delete(for: StorageKeys.deviceIDKey)
return Just(())
.setFailureType(to: TokenError.self)
.eraseToAnyPublisher()
} catch {
return Fail(error: TokenError.clearFailed(underlying: error))
.eraseToAnyPublisher()
}
}
}
19 changes: 19 additions & 0 deletions src/Projects/BKStorage/Sources/StorageAssembly.swift
Original file line number Diff line number Diff line change
Expand Up @@ -59,5 +59,24 @@ public struct StorageAssembly: Assembly {
storage: keyValueStorage
)
}

container.register(
type: DeviceIDProvider.self,
scope: .singleton
) { _ in
@Autowired(name: "Keychain") var keyValueStorage: KeyValueStorage
return KeychainDeviceIDProvider(
storage: keyValueStorage
)
}

container.register(
type: DeviceIDStore.self
) { _ in
@Autowired(name: "Keychain") var keyValueStorage: KeyValueStorage
return KeychainDeviceIDStore(
storage: keyValueStorage
)
}
}
}
15 changes: 15 additions & 0 deletions src/Projects/Booket/Sources/AppDelegate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,21 @@ final class AppDelegate: UIResponder, UIApplicationDelegate {
UNUserNotificationCenter.current().delegate = self
Messaging.messaging().delegate = self

// 앱 실행 시 DeviceID 생성 (없으면)
// 키체인에 저장되므로 앱 삭제 후 재설치해도 유지됨
KeychainDeviceIDStore.shared.getOrCreate()
.sink(
receiveCompletion: { completion in
if case .failure(let error) = completion {
AppLogger.auth.error("Failed to get or create device ID: \(error)")
}
},
receiveValue: { deviceID in
AppLogger.auth.info("Device ID ready: \(deviceID)")
}
)
.store(in: &cancellables)

return true
}

Expand Down