fix: [SDK-4818] resolve TOCTOU race on the current user

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)

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)

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)

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

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()

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 */

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))
+    }
 }