Skip to content
Draft
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
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,9 @@ 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 the request still owns the current user (captured once), so a
// concurrent login/logout can't escalate this into an erroneous logout of another user
guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil
else {
if inBackground {
OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -278,8 +278,9 @@ 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 the request still owns the current user (captured once), so a
// concurrent login/logout can't escalate this into an erroneous logout of another user
guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil
else {
if inBackground {
OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -325,8 +325,9 @@ 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 the request still owns the current user (captured once), so a
// concurrent login/logout can't escalate this into an erroneous logout of another user
guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil
else {
if inBackground {
OSBackgroundTaskManager.endBackgroundTask(backgroundTaskIdentifier)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -412,8 +412,9 @@ 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 the request still owns the current user (captured once), so a
// concurrent login/logout can't escalate this into an erroneous logout of another user
guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModelToUpdate.modelId) != nil
else {
return
}
Expand Down Expand Up @@ -448,15 +449,15 @@ 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. Capture `_user` once and
// clear/hydrate that same instance, so a concurrent login/logout can't wipe or hydrate
// a different user's data.
OneSignalUserManagerImpl.sharedInstance.withCurrentUser(matching: request.identityModel.modelId) { user in
guard let response = response else {
return
}
// 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
Expand Down Expand Up @@ -485,8 +486,9 @@ 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 the request still owns the current user (captured once), so a
// concurrent login/logout can't escalate this into an erroneous logout of another user
guard OneSignalUserManagerImpl.sharedInstance.currentUser(matching: request.identityModel.modelId) != nil
else {
return
}
Expand Down Expand Up @@ -541,39 +543,40 @@ extension OSUserExecutor {
}
}

// Check if the current user is the same as the one in the request
// If user has changed, don't hydrate, except for push subscription above
guard OneSignalUserManagerImpl.sharedInstance.isCurrentUser(identityModel) else {
return
}

if let propertiesObject = parsePropertiesObjectResponse(response) {
OneSignalUserManagerImpl.sharedInstance._user?.propertiesModel.hydrate(propertiesObject)
}

// Now parse email and sms subscriptions
if let subscriptionObject = parseSubscriptionObjectResponse(response) {
let models = OneSignalUserManagerImpl.sharedInstance.subscriptionModelStore.getModels()
for subModel in subscriptionObject {
if let address = subModel["token"] as? String,
let rawType = subModel["type"] as? String,
rawType != "iOSPush",
let type = OSSubscriptionType(rawValue: rawType)
{
if let model = models[address] {
// This subscription exists in the store, hydrate
model.hydrate(subModel)
// Hydrate the properties and email/SMS subscriptions only if the request still owns the
// current user; if the user has changed, don't hydrate (except for the push subscription
// above). Capture `_user` once and hydrate that same instance, so a concurrent login/logout
// can't land this response's data on a different user. This is reached from both the fetch
// user and create user paths, so the guard must live here.
OneSignalUserManagerImpl.sharedInstance.withCurrentUser(matching: identityModel.modelId) { user in
if let propertiesObject = parsePropertiesObjectResponse(response) {
user.propertiesModel.hydrate(propertiesObject)
}

} else {
// This subscription does not exist in the store, add
OneSignalUserManagerImpl.sharedInstance.subscriptionModelStore.add(id: address, model: OSSubscriptionModel(
type: type,
address: address,
subscriptionId: subModel["id"] as? String,
reachable: true,
isDisabled: false,
changeNotifier: OSEventProducer()), hydrating: true
)
// Now parse email and sms subscriptions
if let subscriptionObject = parseSubscriptionObjectResponse(response) {
let models = OneSignalUserManagerImpl.sharedInstance.subscriptionModelStore.getModels()
for subModel in subscriptionObject {
if let address = subModel["token"] as? String,
let rawType = subModel["type"] as? String,
rawType != "iOSPush",
let type = OSSubscriptionType(rawValue: rawType)
{
if let model = models[address] {
// This subscription exists in the store, hydrate
model.hydrate(subModel)

} else {
// This subscription does not exist in the store, add
OneSignalUserManagerImpl.sharedInstance.subscriptionModelStore.add(id: address, model: OSSubscriptionModel(
type: type,
address: address,
subscriptionId: subModel["id"] as? String,
reachable: true,
isDisabled: false,
changeNotifier: OSEventProducer()), hydrating: true
)
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -427,13 +427,56 @@ public class OneSignalUserManagerImpl: NSObject, OneSignalUserManager {

return userInstance.identityModel.externalId == externalId
}

/**
Returns the current user only if it still owns `modelId`, otherwise `nil`.

`_user` is read exactly once, so callers operate on a single confirmed instance instead of
checking the current user and then separately re-reading `_user`. That check-then-deref is a
TOCTOU: a concurrent login/logout can swap `_user` in the window between the two, landing one
user's data or action on a different user.
*/
func currentUser(matching modelId: String) -> OSUserInternal? {
guard let user = _user, user.identityModel.modelId == modelId else {
return nil
}
return user
}

/**
Runs `body` with the current user, but only if it still owns `modelId`, capturing `_user` once.

The same captured instance is both checked and passed to `body`, so a concurrent login/logout
can't divert a network-response callback's mutation onto a different user. No lock is held
across `body`: the model mutations callers perform here (e.g. `hydrate`, `clearData`) fire
model-store listeners into the operation repo, so holding a lock across them could deadlock.
*/
func withCurrentUser(matching modelId: String, _ body: (OSUserInternal) -> Void) {
guard let user = currentUser(matching: modelId) else {
return
}
body(user)
}

/**
Clears the existing user's data in preparation for hydration via a fetch user call.
*/
func clearUserData() {
guard let user = _user else {
return
}
clearUserData(user)
}

/**
Clears the passed-in (already-confirmed) user's data in preparation for hydration via a fetch
user call. Operating on the captured `user` rather than re-reading `_user` avoids clearing a
different user's data if a concurrent login/logout swaps `_user`.
*/
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()
Expand Down
94 changes: 94 additions & 0 deletions iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -189,4 +189,98 @@ final class OneSignalUserTests: XCTestCase {
contains: expectedPayload)
)
}

// MARK: - Atomic current-user accessor (TOCTOU)

/**
The user executors guard a network-response callback with the current user and then separately
dereference `_user`. A concurrent login/logout can swap `_user` between the two, applying one
user's data or action to a different user. `withCurrentUser(matching:)` closes that window by
reading `_user` once and passing that same captured instance to the closure.

This is the safe path: when the current user still owns the model ID, the closure runs on the
captured current user.
*/
func testWithCurrentUser_runsClosureOnCapturedCurrentUser_whenModelIdMatches() throws {
/* Setup */
let user = OneSignalUserMocks.setUserManagerInternalUser(externalId: userA_EUID, onesignalId: userA_OSID)
let modelId = user.identityModel.modelId

/* When */
var runCount = 0
var capturedIsCurrentUser = false
OneSignalUserManagerImpl.sharedInstance.withCurrentUser(matching: modelId) { capturedUser in
runCount += 1
// The closure must receive the very instance that is currently `_user`.
capturedIsCurrentUser = capturedUser.identityModel === OneSignalUserManagerImpl.sharedInstance._user?.identityModel
}

/* Then */
XCTAssertEqual(runCount, 1)
XCTAssertTrue(capturedIsCurrentUser)
}

/**
Simulates the TOCTOU race: a concurrent login swaps `_user` to a different user before the
callback runs. The accessor must NOT run its closure for the now-stale model ID, so the response
can't be applied to the new current user (cross-user contamination).
*/
func testWithCurrentUser_doesNotRunClosure_whenUserSwappedToDifferentModelId() throws {
/* Setup */
let previousUser = OneSignalUserMocks.setUserManagerInternalUser(externalId: userA_EUID, onesignalId: userA_OSID)
let staleModelId = previousUser.identityModel.modelId

// A concurrent login swaps the current user out from under the in-flight request.
let currentUser = OneSignalUserMocks.setUserManagerInternalUser(externalId: userB_EUID, onesignalId: userB_OSID)
XCTAssertNotEqual(staleModelId, currentUser.identityModel.modelId)

/* When */
var runCount = 0
OneSignalUserManagerImpl.sharedInstance.withCurrentUser(matching: staleModelId) { _ in
runCount += 1
}

/* Then */
XCTAssertEqual(runCount, 0)
}

/**
When there is no current user (e.g. `_user == nil` during the logout window), the accessor must
not run its closure rather than dereferencing a nil or recreated user.
*/
func testWithCurrentUser_doesNotRunClosure_whenNoCurrentUser() throws {
/* Setup */
// `OneSignalUserMocks.reset()` (called in setUp) leaves `_user == nil`.
XCTAssertNil(OneSignalUserManagerImpl.sharedInstance._user)

/* When */
var runCount = 0
OneSignalUserManagerImpl.sharedInstance.withCurrentUser(matching: UUID().uuidString) { _ in
runCount += 1
}

/* Then */
XCTAssertEqual(runCount, 0)
}

/**
`currentUser(matching:)` is the non-closure variant used to gate `_logout()`. It must return the
current user only when the model ID matches, and `nil` when there is no user or the user was
swapped, so a stale failure callback can't log out a different user.
*/
func testCurrentUserMatching_returnsUserOnlyWhenModelIdMatches() throws {
/* No current user → nil */
XCTAssertNil(OneSignalUserManagerImpl.sharedInstance.currentUser(matching: UUID().uuidString))

/* Matching current user → the captured instance */
let user = OneSignalUserMocks.setUserManagerInternalUser(externalId: userA_EUID, onesignalId: userA_OSID)
let staleModelId = user.identityModel.modelId
let matched = OneSignalUserManagerImpl.sharedInstance.currentUser(matching: staleModelId)
XCTAssertNotNil(matched)
XCTAssertTrue(matched?.identityModel === user.identityModel)

/* Concurrent swap → the previous model ID no longer matches → nil */
_ = OneSignalUserMocks.setUserManagerInternalUser(externalId: userB_EUID, onesignalId: userB_OSID)
XCTAssertNil(OneSignalUserManagerImpl.sharedInstance.currentUser(matching: staleModelId))
}
}
Loading