diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift index 1a3c3e839..516ca0c02 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSIdentityOperationExecutor.swift @@ -223,8 +223,8 @@ class OSIdentityOperationExecutor: OSOperationExecutor { // Remove from cache and queue self.addRequestQueue.removeAll(where: { $0 == request}) OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_IDENTITY_EXECUTOR_ADD_REQUEST_QUEUE_KEY, withValue: self.addRequestQueue) - // Logout if the user in the SDK is the same - guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel) + // Logout only if this request's user is still current, so a concurrent login can't log out the wrong user. + guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil else { if inBackground { OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSPropertyOperationExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSPropertyOperationExecutor.swift index 107254fd2..88f29af73 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSPropertyOperationExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSPropertyOperationExecutor.swift @@ -257,11 +257,11 @@ class OSPropertyOperationExecutor: OSOperationExecutor { // Re-assert the tags the server just confirmed by merging them back into the local model, // to remedy a concurrent FetchUser whose response is missing the just-written tags - if OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel), + if let user = OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId), let properties = response?["properties"] as? [String: Any], let confirmedTags = properties["tags"] as? [String: String], !confirmedTags.isEmpty { - OneSignalUserManagerImpl.sharedInstance._user?.propertiesModel.mergeConfirmedTags(confirmedTags) + user.propertiesModel.mergeConfirmedTags(confirmedTags) } if let onesignalId = request.identityModel.onesignalId { @@ -287,8 +287,8 @@ class OSPropertyOperationExecutor: OSOperationExecutor { // remove from cache and queue self.updateRequestQueue.removeAll(where: { $0 == request}) OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_PROPERTIES_EXECUTOR_UPDATE_REQUEST_QUEUE_KEY, withValue: self.updateRequestQueue) - // Logout if the user in the SDK is the same - guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel) + // Logout only if this request's user is still current, so a concurrent login can't log out the wrong user. + guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil else { if inBackground { OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSSubscriptionOperationExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSSubscriptionOperationExecutor.swift index 3f823038c..f100bbfd9 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSSubscriptionOperationExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSSubscriptionOperationExecutor.swift @@ -327,8 +327,8 @@ class OSSubscriptionOperationExecutor: OSOperationExecutor { if responseType == .missing { self.addRequestQueue.removeAll(where: { $0 == request}) OneSignalUserDefaults.initShared().saveCodeableData(forKey: OS_SUBSCRIPTION_EXECUTOR_ADD_REQUEST_QUEUE_KEY, withValue: self.addRequestQueue) - // Logout if the user in the SDK is the same - guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel) + // Logout only if this request's user is still current, so a concurrent login can't log out the wrong user. + guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil else { if inBackground { OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift index 4fb6094a4..1b8d0e5b3 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift @@ -120,7 +120,7 @@ class OSUserExecutor { // Translate the last request into a Create User request, if the current user is the same if let request = transferSubscriptionRequestQueue.last, let userInstance = OneSignalUserManagerImpl.sharedInstance._user, - OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.aliasId) { + userInstance.identityModel.externalId == request.aliasId { createUser(userInstance) } } @@ -251,7 +251,7 @@ extension OSUserExecutor { // If this user already exists and we logged into an external_id, fetch the user data // Fetch the user only if its the current user and non-anonymous - if OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel), + if OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil, let identity = request.parameters?["identity"] as? [String: String], let onesignalId = request.identityModel.onesignalId, identity[OS_EXTERNAL_ID] != nil { @@ -320,7 +320,7 @@ extension OSUserExecutor { request.identityModel.hydrate(identityObject) // Fetch this user's data if it is the current user - guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel) + guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil else { self.executePendingRequests() return @@ -382,7 +382,7 @@ extension OSUserExecutor { request.identityModelToUpdate.hydrate(aliases) // the anonymous user has been identified, still need to Fetch User as we cleared local data - if OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModelToUpdate) { + if OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModelToUpdate.modelId) != nil { // Add onesignal ID to new records because an immediate fetch may not return the newly-applied external ID self.newRecordsState.add(onesignalId, true) self.fetchUser(aliasLabel: OS_ONESIGNAL_ID, aliasId: onesignalId, identityModel: request.identityModelToUpdate) @@ -397,8 +397,7 @@ extension OSUserExecutor { self.removeFromQueue(request) - if let userInstance = OneSignalUserManagerImpl.sharedInstance._user, - OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModelToUpdate) { + if let userInstance = OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModelToUpdate.modelId) { // Generate a Create User request, if it's still the current user self.createUser(userInstance) } else { @@ -412,8 +411,8 @@ extension OSUserExecutor { } else if responseType == .missing { self.removeFromQueue(request) self.executePendingRequests() - // Logout if the user in the SDK is the same - guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModelToUpdate) + // Logout only if this request's user is still current, so a concurrent login can't log out the wrong user. + guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModelToUpdate.modelId) != nil else { return } @@ -448,15 +447,12 @@ extension OSUserExecutor { OneSignalCoreImpl.sharedClient().execute(request) { response in self.removeFromQueue(request) - // A fetch for a user that is no longer current is stale - guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel) else { - self.executePendingRequests() - return - } - - if let response = response { + // A fetch for a user that is no longer current is stale. A login can land while this + // response is in flight, so the clear must apply to the user the response is for. + if let user = OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId), + let response = response { // Clear local data in preparation for hydration - OneSignalUserManagerImpl.sharedInstance.clearUserData() + OneSignalUserManagerImpl.sharedInstance.clearUserData(user) self.parseFetchUserResponse(response: response, identityModel: request.identityModel, originalPushToken: OneSignalUserManagerImpl.sharedInstance.pushSubscriptionImpl.token) // If this is a on-new-session's fetch user call, check that the subscription still exists @@ -485,8 +481,8 @@ extension OSUserExecutor { let responseType = OSNetworkingUtils.getResponseStatusType(error.code) if responseType == .missing { self.removeFromQueue(request) - // Logout if the user in the SDK is the same - guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(request.identityModel) + // Logout only if this request's user is still current, so a concurrent login can't log out the wrong user. + guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil else { return } @@ -541,14 +537,14 @@ extension OSUserExecutor { } } - // Check if the current user is the same as the one in the request + // Hydrate onto the user this response is for // If user has changed, don't hydrate, except for push subscription above - guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(identityModel) else { + guard let user = OneSignalUserManagerImpl.sharedInstance.currentUser(matching: identityModel.modelId) else { return } if let propertiesObject = parsePropertiesObjectResponse(response) { - OneSignalUserManagerImpl.sharedInstance._user?.propertiesModel.hydrate(propertiesObject) + user.propertiesModel.hydrate(propertiesObject) } // Now parse email and sms subscriptions diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift index 5b9e0ce35..eb7608b2d 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift @@ -149,7 +149,16 @@ public class OneSignalUserManagerImpl: NSObject, OneSignalUserManager { return createNewUser(externalId: nil, token: nil) } - var _user: OSUserInternal? + /// Guards `_user`. Held only across a single read or write; holding it while callers mutate + /// models would re-enter the model stores and operation repo, and could deadlock. + private let userLock = NSLock() + + private var _userStorage: OSUserInternal? + + var _user: OSUserInternal? { + get { userLock.withLock { _userStorage } } + set { userLock.withLock { _userStorage = newValue } } + } // This is a user instance to operate on when there is no app_id and/or privacy consent yet, effectively no-op. // The models are not added to any model stores. @@ -413,27 +422,26 @@ public class OneSignalUserManagerImpl: NSObject, OneSignalUserManager { } /** - Returns if the OSIdentityModel passed in belongs to the current user. This method is used in deciding whether or not to hydrate via a server response, for example. + Act on the instance returned: the current user is read once here, so a concurrent + `login()`/`logout()` can't land between the check and the use. Its identity and properties + models are safe to mutate; the shared model stores are not scoped to a user. */ - func isCurrentUser(_ identityModel: OSIdentityModel) -> Bool { - return self.identityModelStore.getModel(modelId: identityModel.modelId) != nil - } - - func isCurrentUser(_ externalId: String) -> Bool { - guard let userInstance = _user, !externalId.isEmpty else { - OneSignalLog.onesignalLog(.LL_ERROR, message: "isCurrentUser called with empty externalId or no user instance") - return false + func currentUser(matching modelId: String) -> OSUserInternal? { + guard let user = _user, user.identityModel.modelId == modelId else { + return nil } - - return userInstance.identityModel.externalId == externalId + return user } + /** - Clears the existing user's data in preparation for hydration via a fetch user call. + Clears the passed-in user's data in preparation for hydration via a fetch user call. + + Operates on the given user so a concurrent login can't redirect the clear onto a different one. */ - func clearUserData() { + func clearUserData(_ user: OSUserInternal) { // Identity and property models should still be the same instances, but with data cleared - _user?.identityModel.clearData() - _user?.propertiesModel.clearData() + user.identityModel.clearData() + user.propertiesModel.clearData() // Subscription model store should be cleared completely OneSignalUserManagerImpl.sharedInstance.subscriptionModelStore.clearModelsFromStore() diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/Executors/UserExecutorTests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserTests/Executors/UserExecutorTests.swift index f1eb5bbb3..703079453 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/Executors/UserExecutorTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/Executors/UserExecutorTests.swift @@ -222,8 +222,8 @@ final class UserExecutorTests: XCTestCase { } /** - The normal new-session Fetch User for the *current* user must still clear stale local data before hydrating - from the response, so the `isCurrentUser` guard added for the race above does not regress the common path. + A Fetch User for the *current* user must still clear stale local data before hydrating from the + response, so guarding against the race above does not regress the common path. */ func testFetchUser_forCurrentUser_stillClearsStaleData() { /* Setup */ diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift index 9c55ec928..bfe416b46 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift @@ -248,4 +248,65 @@ final class OneSignalUserTests: XCTestCase { // The confirmed tags from the 202 response are merged back into the local model XCTAssertEqual(OneSignalUserManagerImpl.sharedInstance.getTags(), tags) } + + // MARK: - Atomic current user access + + /** + A callback that acts on the current user must keep acting on the user it checked, even if a + `login()` makes a different user current right afterwards. Swapping the user between the check + and the mutation reproduces that interleaving. + */ + func testCurrentUser_matching_isTheUserMutated_whenTheUserChangesRightAfterTheCheck() throws { + /* Setup */ + OneSignalCoreImpl.setSharedClient(MockOneSignalClient()) + let manager = OneSignalUserManagerImpl.sharedInstance + let userA = OneSignalUserMocks.setUserManagerInternalUser(externalId: userA_EUID, onesignalId: userA_OSID) + + /* When */ + let checkedUser = manager.currentUser(matching: userA.identityModel.modelId) + // A concurrent login switches the current user before the response is applied + let userB = OneSignalUserMocks.setUserManagerInternalUser(externalId: userB_EUID, onesignalId: userB_OSID) + checkedUser?.propertiesModel.hydrate(["language": "language-for-user-a"]) + + /* Then */ + // The response's data went to the user it was for, and the new current user is untouched + XCTAssertEqual(userA.propertiesModel.language, "language-for-user-a") + XCTAssertNil(userB.propertiesModel.language) + XCTAssertEqual(manager._user?.identityModel.externalId, userB_EUID) + } + + /// The common path: the request's user is still current, so it is returned to be mutated. + func testCurrentUser_matching_returnsTheCurrentUser() throws { + /* Setup */ + OneSignalCoreImpl.setSharedClient(MockOneSignalClient()) + let manager = OneSignalUserManagerImpl.sharedInstance + let user = OneSignalUserMocks.setUserManagerInternalUser(externalId: userA_EUID, onesignalId: userA_OSID) + + /* Then */ + XCTAssertEqual(manager.currentUser(matching: user.identityModel.modelId)?.identityModel.externalId, userA_EUID) + } + + /// A response for a user that is no longer current must not be applied at all. + func testCurrentUser_matching_isNilWhenTheUserIsNoLongerCurrent() throws { + /* Setup */ + OneSignalCoreImpl.setSharedClient(MockOneSignalClient()) + let manager = OneSignalUserManagerImpl.sharedInstance + let userA = OneSignalUserMocks.setUserManagerInternalUser(externalId: userA_EUID, onesignalId: userA_OSID) + let userB = OneSignalUserMocks.setUserManagerInternalUser(externalId: userB_EUID, onesignalId: userB_OSID) + + /* Then */ + XCTAssertNil(manager.currentUser(matching: userA.identityModel.modelId)) + XCTAssertNotNil(manager.currentUser(matching: userB.identityModel.modelId)) + } + + /// With no current user, there is nothing for a late response to act on. + func testCurrentUser_matching_isNilWhenThereIsNoUser() throws { + /* Setup */ + let manager = OneSignalUserManagerImpl.sharedInstance + let identityModel = OSIdentityModel(aliases: nil, changeNotifier: OSEventProducer()) + + /* Then */ + XCTAssertNil(manager._user) + XCTAssertNil(manager.currentUser(matching: identityModel.modelId)) + } }