From c3dd2f308a927cfc74e645b31def2e3fe7100cae Mon Sep 17 00:00:00 2001 From: Nan Date: Wed, 26 Aug 2026 19:13:58 -0700 Subject: [PATCH 1/9] fix: respect REST API-disabled push subscriptions A push subscription disabled through the REST API (notification_types -31) was re-enabled by the SDK: fetch responses never hydrated the disable onto an existing push subscription, and every Create User and update payload recomputed enabled from device state. Mirror the server's disable code on the subscription model when a response reports it, and echo it back in Create User and update payloads instead of the device-derived values. The mirror clears when the server reports any other state, when the subscription ID resets because the server record is gone, and on an explicit optIn(), whose clear outranks stale in-flight hydration until the server confirms. Both payload builders share one snapshot-based body so concurrent changes cannot tear enabled away from notification_types. --- .../Source/Executors/OSUserExecutor.swift | 61 +++-- .../Source/OSSubscriptionModel.swift | 217 +++++++++++++++--- .../Source/OneSignalUserManagerImpl.swift | 4 +- .../OSRequestUpdateSubscription.swift | 12 +- .../OneSignalUserTests.swift | 175 ++++++++++++++ 5 files changed, 402 insertions(+), 67 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift index 1b8d0e5b3..da6a6c6c1 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift @@ -514,28 +514,7 @@ extension OSUserExecutor { } } - // TODO: Determine how to hydrate the push subscription, which is still faulty. - // Hydrate by token if sub_id exists? - // Problem: a user can have multiple iOS push subscription, and perhaps missing token - // Ideally we only get push subscription for this device in the response, not others - - // Hydrate the push subscription if we don't already have a subscription ID AND token matches the original request - if OneSignalUserManagerImpl.sharedInstance.pushSubscriptionModel?.subscriptionId == nil, - let subscriptionObject = parseSubscriptionObjectResponse(response) - { - for subModel in subscriptionObject { - if subModel["type"] as? String == "iOSPush", - // response may have "" token or no token - areTokensEqual(tokenA: originalPushToken, tokenB: subModel["token"] as? String) - { - OneSignalUserManagerImpl.sharedInstance.pushSubscriptionModel?.hydrate(subModel) - if addNewRecords, let subId = subModel["id"] as? String { - newRecordsState.add(subId) - } - break - } - } - } + hydratePushSubscription(response: response, originalPushToken: originalPushToken, addNewRecords: addNewRecords) // Hydrate onto the user this response is for // If user has changed, don't hydrate, except for push subscription above @@ -576,6 +555,44 @@ extension OSUserExecutor { } } + /// Hydrates the push subscription from a fetch or create response: the whole object before a + /// subscription ID exists, only the server's REST API disable state once one does. + // TODO: Determine how to hydrate the push subscription, which is still faulty. + // Hydrate by token if sub_id exists? + // Problem: a user can have multiple iOS push subscription, and perhaps missing token + // Ideally we only get push subscription for this device in the response, not others + private func hydratePushSubscription(response: [AnyHashable: Any], originalPushToken: String?, addNewRecords: Bool) { + guard let subscriptionObject = parseSubscriptionObjectResponse(response) else { + return + } + // The response's subscription ID is recorded as a new record even when the model is absent. + let pushSubscriptionModel = OneSignalUserManagerImpl.sharedInstance.pushSubscriptionModel + + // Hydrate the push subscription if we don't already have a subscription ID AND token matches the original request + guard let subscriptionId = pushSubscriptionModel?.subscriptionId else { + for subModel in subscriptionObject { + if subModel["type"] as? String == "iOSPush", + // response may have "" token or no token + areTokensEqual(tokenA: originalPushToken, tokenB: subModel["token"] as? String) + { + pushSubscriptionModel?.hydrate(subModel) + if addNewRecords, let subId = subModel["id"] as? String { + newRecordsState.add(subId) + } + break + } + } + return + } + + // Only the REST API disable state hydrates onto an existing push subscription; the device + // owns the rest. Skipping it lets the next subscription payload re-enable a suppressed device. + for subModel in subscriptionObject where subModel["id"] as? String == subscriptionId { + pushSubscriptionModel?.hydrateRestApiDisabledState(from: subModel) + break + } + } + /** Returns if 2 tokens are equal. This is needed as a nil token is equal to the empty string "". */ diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift index cc9dd2211..1055623b8 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift @@ -114,6 +114,10 @@ class OSSubscriptionModel: OSModel { var deviceModel: String? var appVersion: String? var netType: Int? + var restApiDisabledReason: Int? + // Not persisted; an optIn() clear outranks stale hydration until the server reports + // the subscription in another state. + var restApiDisableClearedByUser = false } /** @@ -182,6 +186,11 @@ class OSSubscriptionModel: OSModel { return } + // The disable code describes a specific server record; the record is gone when the ID resets. + if newValue == nil { + restApiDisabledReason = nil + } + // Cache the subscriptionId to UserDefaults for routine reads, and the OSResilientStorage mirror OneSignalUserDefaults.initShared().saveString(forKey: OSUD_PUSH_SUBSCRIPTION_ID, withValue: newValue) OSResilientStorage.setString(newValue ?? "", forKey: OSResilientStorage.keySubscriptionId) @@ -194,7 +203,12 @@ class OSSubscriptionModel: OSModel { var enabled: Bool { // Does not consider subscription_id in the calculation get { let state = snapshot() - return calculateIsEnabled(address: state.address, reachable: state.reachable, isDisabled: state.isDisabled) + return calculateIsEnabled( + address: state.address, + reachable: state.reachable, + isDisabled: state.isDisabled, + restApiDisabledReason: state.restApiDisabledReason + ) } } @@ -261,6 +275,28 @@ class OSSubscriptionModel: OSModel { } } + /// The notification_types value for a REST API disable, the only server-owned code; other + /// negative codes are device or delivery errors the device recovers by re-asserting its state. + static let restApiDisabledNotificationType = -31 + + /** + The server's REST API disable code, or nil when the server has not disabled this subscription. + Hydrated from responses, never derived from device state, and echoed back in payloads so routine + updates and logins don't re-enable a suppressed subscription. Cleared when a response reports + any other state, when the subscription ID resets, or by `optIn()`. + */ + var restApiDisabledReason: Int? { + get { stateLock.withLock { state.restApiDisabledReason } } + set { + let oldValue = swapValue(\.restApiDisabledReason, to: newValue) + guard newValue != oldValue else { + return + } + // Mirrors server state rather than a local change, so persist without generating a delta. + self.set(property: "restApiDisabledReason", newValue: newValue, preventServerUpdate: true) + } + } + // Properties for push subscription var testType: Int? { get { stateLock.withLock { state.testType } } @@ -371,7 +407,9 @@ class OSSubscriptionModel: OSModel { sdk: ONESIGNAL_VERSION, deviceModel: OSDeviceUtils.getDeviceVariant(), appVersion: Bundle.main.infoDictionary?["CFBundleShortVersionString"] as? String, - netType: OSNetworkingUtils.getNetType() as? Int + netType: OSNetworkingUtils.getNetType() as? Int, + restApiDisabledReason: nil, + restApiDisableClearedByUser: false ) super.init(changeNotifier: changeNotifier) @@ -393,6 +431,7 @@ class OSSubscriptionModel: OSModel { coder.encode(state.deviceModel, forKey: "deviceModel") coder.encode(state.appVersion, forKey: "appVersion") coder.encode(state.netType, forKey: "netType") + coder.encode(state.restApiDisabledReason, forKey: "restApiDisabledReason") } required init?(coder: NSCoder) { @@ -415,7 +454,9 @@ class OSSubscriptionModel: OSModel { sdk: coder.decodeObject(forKey: "sdk") as? String ?? ONESIGNAL_VERSION, deviceModel: coder.decodeObject(forKey: "deviceModel") as? String, appVersion: coder.decodeObject(forKey: "appVersion") as? String, - netType: coder.decodeObject(forKey: "netType") as? Int + netType: coder.decodeObject(forKey: "netType") as? Int, + restApiDisabledReason: coder.decodeObject(forKey: "restApiDisabledReason") as? Int, + restApiDisableClearedByUser: false ) super.init(coder: coder) @@ -436,13 +477,11 @@ class OSSubscriptionModel: OSModel { // self.address = property.value as? String case "enabled": if let enabled = property.value as? Bool { - if self.enabled != enabled { // TODO: Is this right? - _isDisabled = !enabled - } + hydrateEnabled(enabled, response: response) } case "notification_types": if let notificationTypes = property.value as? Int { - self.notificationTypes = notificationTypes + hydrateNotificationTypes(notificationTypes) } default: OneSignalLog.onesignalLog(.LL_DEBUG, message: "Unused property on subscription model") @@ -450,6 +489,33 @@ class OSSubscriptionModel: OSModel { } } + /// Applies a hydrated `enabled`. A REST API disable is server-owned, not a user opt-out, + /// so it must not flip `_isDisabled`; the notification_types hydration records it instead. + private func hydrateEnabled(_ enabled: Bool, response: [String: Any]) { + guard !isRestApiDisable(response) else { + return + } + if self.enabled != enabled { // TODO: Is this right? + _isDisabled = !enabled + } + } + + /// Routes a hydrated notification_types: -31 records the server's disable; any other value + /// clears it and becomes the device value. + private func hydrateNotificationTypes(_ value: Int) { + if value == Self.restApiDisabledNotificationType { + recordRestApiDisable(value) + } else { + acceptServerNonDisabledState() + self.notificationTypes = value + } + } + + /// True when the response's notification_types carries a REST API disable code. + private func isRestApiDisable(_ response: [String: Any]) -> Bool { + return response["notification_types"] as? Int == Self.restApiDisabledNotificationType + } + // Using snake_case so we can use this in request bodies public func jsonRepresentation() -> [String: Any] { let state = snapshot() @@ -457,19 +523,22 @@ class OSSubscriptionModel: OSModel { json["id"] = state.subscriptionId json["type"] = state.type.rawValue json["token"] = state.address - json["enabled"] = calculateIsEnabled(address: state.address, reachable: state.reachable, isDisabled: state.isDisabled) + json["enabled"] = calculateIsEnabled( + address: state.address, + reachable: state.reachable, + isDisabled: state.isDisabled, + restApiDisabledReason: state.restApiDisabledReason + ) json["test_type"] = state.testType json["device_os"] = state.deviceOs json["sdk"] = state.sdk json["device_model"] = state.deviceModel json["app_version"] = state.appVersion json["net_type"] = state.netType - // notificationTypes defaults to -1 instead of nil, don't send if it's -1 - if state.notificationTypes != -1 { - json["notification_types"] = state.notificationTypes - } + json["notification_types"] = outboundNotificationTypes(state) return json } + } // Push Subscription related @@ -491,14 +560,87 @@ extension OSSubscriptionModel { // Calculates if push notifications are enabled on the device. // Does not consider the existence of the subscription_id, as we send this in the request to create a push subscription. - func calculateIsEnabled(address: String?, reachable: Bool, isDisabled: Bool) -> Bool { - return address != nil && reachable && !isDisabled + func calculateIsEnabled(address: String?, reachable: Bool, isDisabled: Bool, restApiDisabledReason: Int?) -> Bool { + return address != nil && reachable && !isDisabled && restApiDisabledReason == nil } func updateNotificationTypes() { notificationTypes = Int(OSNotificationsManager.getNotificationTypes(_isDisabled)) } + /// Records the server's disable code unless `optIn()` cleared one and the server has not yet + /// reported the subscription in another state; the user's explicit intent wins that race. + private func recordRestApiDisable(_ code: Int) { + let clearedByUser = stateLock.withLock { state.restApiDisableClearedByUser } + guard !clearedByUser else { + return + } + restApiDisabledReason = code + } + + /// Clears `restApiDisabledReason` and re-arms recording once the server reports a non-disabled state. + private func acceptServerNonDisabledState() { + restApiDisabledReason = nil + stateLock.withLock { state.restApiDisableClearedByUser = false } + } + /// notification_types for outgoing payloads: the recorded disable code (the positive device + /// value would re-enable it), else the device value, nil for the -1 default. + private func outboundNotificationTypes(_ state: State) -> Int? { + if let restApiDisabledReason = state.restApiDisabledReason { + return restApiDisabledReason + } + return state.notificationTypes != -1 ? state.notificationTypes : nil + } + + /// The PATCH body for a subscription update, built from one snapshot so a concurrent hydration + /// or `optIn()` can't tear `enabled` away from `notification_types`. + func updateParams() -> [String: Any] { + let state = snapshot() + var params: [String: Any] = [:] + params["token"] = state.address + params["device_os"] = state.deviceOs + params["sdk"] = state.sdk + params["app_version"] = state.appVersion + params["notification_types"] = outboundNotificationTypes(state) + params["enabled"] = calculateIsEnabled( + address: state.address, + reachable: state.reachable, + isDisabled: state.isDisabled, + restApiDisabledReason: state.restApiDisabledReason + ) + return params + } + + /** + Mirrors the server's REST API disable state from a fetched subscription object: -31 records it, + any other reported value clears it. The device stays the source of truth for the rest of an + existing subscription's state, so nothing else is read. + */ + func hydrateRestApiDisabledState(from serverSubscription: [String: Any]) { + guard type == .push, let serverTypes = serverSubscription["notification_types"] as? Int else { + return + } + if serverTypes == Self.restApiDisabledNotificationType { + recordRestApiDisable(serverTypes) + } else { + acceptServerNonDisabledState() + } + } + + /** + Clears a REST API disable and enqueues an enabled-change delta so the server re-enables the + subscription. Called from `optIn()`, where a deliberate user action overrides the suppression. + */ + func clearRestApiDisable() { + let oldValue = swapValue(\.restApiDisabledReason, to: nil) + guard oldValue != nil else { + return + } + stateLock.withLock { state.restApiDisableClearedByUser = true } + self.set(property: "restApiDisabledReason", newValue: nil as Int?, preventServerUpdate: true) + firePushSubscriptionChanged(.restApiDisabledReason(oldValue)) + } + func updateTestType() { let releaseMode: OSUIApplicationReleaseMode = OneSignalMobileProvision.releaseMode() // Workaround to unsure how to extract the Int value in 1 step... @@ -532,38 +674,47 @@ extension OSSubscriptionModel { case reachable(Bool) case isDisabled(Bool) case address(String?) + case restApiDisabledReason(Int?) } func firePushSubscriptionChanged(_ changedProperty: OSPushPropertyChanged) { - var prevIsOptedIn = true - var prevIsEnabled = true - var prevSubscriptionState = OSPushSubscriptionState(id: "", token: "", optedIn: true) + // The previous state is the current state with only the changed property's old value substituted. + var prevId = subscriptionId + var prevAddress = address + var prevReachable = _reachable + var prevIsDisabled = _isDisabled + var prevRestApiDisabledReason = restApiDisabledReason switch changedProperty { case .subscriptionId(let oldValue): - prevIsEnabled = calculateIsEnabled(address: address, reachable: _reachable, isDisabled: _isDisabled) - prevIsOptedIn = calculateIsOptedIn(reachable: _reachable, isDisabled: _isDisabled) - prevSubscriptionState = OSPushSubscriptionState(id: oldValue, token: address, optedIn: prevIsOptedIn) - + prevId = oldValue case .reachable(let oldValue): - prevIsEnabled = calculateIsEnabled(address: address, reachable: oldValue, isDisabled: _isDisabled) - prevIsOptedIn = calculateIsOptedIn(reachable: oldValue, isDisabled: _isDisabled) - prevSubscriptionState = OSPushSubscriptionState(id: subscriptionId, token: address, optedIn: prevIsOptedIn) - + prevReachable = oldValue case .isDisabled(let oldValue): - prevIsEnabled = calculateIsEnabled(address: address, reachable: _reachable, isDisabled: oldValue) - prevIsOptedIn = calculateIsOptedIn(reachable: _reachable, isDisabled: oldValue) - prevSubscriptionState = OSPushSubscriptionState(id: subscriptionId, token: address, optedIn: prevIsOptedIn) - + prevIsDisabled = oldValue case .address(let oldValue): - prevIsEnabled = calculateIsEnabled(address: oldValue, reachable: _reachable, isDisabled: _isDisabled) - prevIsOptedIn = calculateIsOptedIn(reachable: _reachable, isDisabled: _isDisabled) - prevSubscriptionState = OSPushSubscriptionState(id: subscriptionId, token: oldValue, optedIn: prevIsOptedIn) + prevAddress = oldValue + case .restApiDisabledReason(let oldValue): + prevRestApiDisabledReason = oldValue } + let prevIsEnabled = calculateIsEnabled( + address: prevAddress, + reachable: prevReachable, + isDisabled: prevIsDisabled, + restApiDisabledReason: prevRestApiDisabledReason + ) + let prevIsOptedIn = calculateIsOptedIn(reachable: prevReachable, isDisabled: prevIsDisabled) + let prevSubscriptionState = OSPushSubscriptionState(id: prevId, token: prevAddress, optedIn: prevIsOptedIn) + let newIsOptedIn = calculateIsOptedIn(reachable: _reachable, isDisabled: _isDisabled) - let newIsEnabled = calculateIsEnabled(address: address, reachable: _reachable, isDisabled: _isDisabled) + let newIsEnabled = calculateIsEnabled( + address: address, + reachable: _reachable, + isDisabled: _isDisabled, + restApiDisabledReason: restApiDisabledReason + ) if prevIsEnabled != newIsEnabled { self.set(property: "enabled", newValue: newIsEnabled) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift index d9850691f..a62b36f29 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift @@ -932,7 +932,9 @@ extension OneSignalUserManagerImpl { guard !OneSignalConfig.shouldAwaitAppIdAndLogMissingPrivacyConsent(forMethod: "pushSubscription.optIn") else { return } - pushSubscriptionModelStore.getModel(key: OS_PUSH_SUBSCRIPTION_MODEL_KEY)?._isDisabled = false + let model = pushSubscriptionModelStore.getModel(key: OS_PUSH_SUBSCRIPTION_MODEL_KEY) + model?._isDisabled = false + model?.clearRestApiDisable() OSNotificationsManager.requestPermission(nil, fallbackToSettings: true) } diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestUpdateSubscription.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestUpdateSubscription.swift index 3f211fad5..bce0be310 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestUpdateSubscription.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Requests/OSRequestUpdateSubscription.swift @@ -57,17 +57,7 @@ class OSRequestUpdateSubscription: OneSignalRequest, OSUserRequest { /// Rebuild the PATCH body from the current subscription model. func refreshParametersFromLiveModel() { - var subscriptionParams: [String: Any] = [:] - subscriptionParams["token"] = subscriptionModel.address - subscriptionParams["device_os"] = subscriptionModel.deviceOs - subscriptionParams["sdk"] = subscriptionModel.sdk - subscriptionParams["app_version"] = subscriptionModel.appVersion - // notificationTypes defaults to -1 instead of nil, don't send if it's -1 - if subscriptionModel.notificationTypes != -1 { - subscriptionParams["notification_types"] = subscriptionModel.notificationTypes - } - subscriptionParams["enabled"] = subscriptionModel.enabled - self.parameters = ["subscription": subscriptionParams] + self.parameters = ["subscription": subscriptionModel.updateParams()] } init(subscriptionModel: OSSubscriptionModel) { diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift index e130af24b..4f6647660 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift @@ -324,4 +324,179 @@ final class OneSignalUserTests: XCTestCase { XCTAssertNil(manager._user) XCTAssertNil(manager.currentUser(matching: identityModel.modelId)) } + + // MARK: - REST API disabled push subscriptions + + /// A push model hydrated with the server's REST API disable state (notification_types -31). + private func pushModelWithRestApiDisable() -> OSSubscriptionModel { + let model = OSSubscriptionModel( + type: .push, + address: "test-token", + subscriptionId: "test-sub-id", + reachable: true, + isDisabled: false, + changeNotifier: OSEventProducer() + ) + model.hydrateRestApiDisabledState(from: ["id": "test-sub-id", "enabled": false, "notification_types": -31]) + return model + } + + func testRestApiDisable_overridesOutgoingSubscriptionPayloads() { + let model = pushModelWithRestApiDisable() + XCTAssertEqual(model.restApiDisabledReason, -31) + + let json = model.jsonRepresentation() + XCTAssertEqual(json["enabled"] as? Bool, false) + XCTAssertEqual(json["notification_types"] as? Int, -31) + + let updateRequest = OSRequestUpdateSubscription(subscriptionModel: model) + let params = updateRequest.parameters?["subscription"] as? [String: Any] + XCTAssertEqual(params?["enabled"] as? Bool, false) + XCTAssertEqual(params?["notification_types"] as? Int, -31) + } + + func testRestApiDisable_survivesDeviceStateRefresh() { + let model = pushModelWithRestApiDisable() + + // Device-driven recomputes must not clear server-owned disable state + model.updateNotificationTypes() + model.update() + + XCTAssertEqual(model.restApiDisabledReason, -31) + XCTAssertEqual(model.jsonRepresentation()["enabled"] as? Bool, false) + } + + func testRestApiDisable_survivesArchiving() throws { + let model = pushModelWithRestApiDisable() + + let data = try NSKeyedArchiver.archivedData(withRootObject: model, requiringSecureCoding: false) + let unarchiver = try NSKeyedUnarchiver(forReadingFrom: data) + unarchiver.requiresSecureCoding = false + let restored = try XCTUnwrap(unarchiver.decodeObject(forKey: NSKeyedArchiveRootObjectKey) as? OSSubscriptionModel) + + XCTAssertEqual(restored.restApiDisabledReason, -31) + XCTAssertEqual(restored.jsonRepresentation()["enabled"] as? Bool, false) + } + + func testRestApiDisable_clearedByOptIn() { + let model = pushModelWithRestApiDisable() + + model.clearRestApiDisable() + + XCTAssertNil(model.restApiDisabledReason) + let json = model.jsonRepresentation() + XCTAssertEqual(json["enabled"] as? Bool, true) + XCTAssertNotEqual(json["notification_types"] as? Int, -31) + } + + func testRestApiDisable_mirrorsServerField() { + // Only -31 is ever recorded; any other reported value is not, and clears an existing disable. + let model = OSSubscriptionModel( + type: .push, + address: "test-token", + subscriptionId: "test-sub-id", + reachable: true, + isDisabled: false, + changeNotifier: OSEventProducer() + ) + model.hydrateRestApiDisabledState(from: ["id": "test-sub-id", "enabled": false, "notification_types": -2]) + XCTAssertNil(model.restApiDisabledReason) + + model.hydrateRestApiDisabledState(from: ["id": "test-sub-id", "enabled": false, "notification_types": -31]) + XCTAssertEqual(model.restApiDisabledReason, -31) + + model.hydrateRestApiDisabledState(from: ["id": "test-sub-id", "enabled": false, "notification_types": -2]) + XCTAssertNil(model.restApiDisabledReason) + XCTAssertEqual(model.jsonRepresentation()["enabled"] as? Bool, true) + } + + func testRestApiDisable_clearedWhenSubscriptionIdResets() { + // The disable code describes a specific server record; it must die with the record. + let model = pushModelWithRestApiDisable() + + model.subscriptionId = nil + + XCTAssertNil(model.restApiDisabledReason) + XCTAssertEqual(model.jsonRepresentation()["enabled"] as? Bool, true) + } + + func testRestApiDisable_optInOutranksStaleHydration() { + let model = pushModelWithRestApiDisable() + + // A stale fetch response landing after optIn() cleared the disable must not re-record it + model.clearRestApiDisable() + model.hydrateRestApiDisabledState(from: ["id": "test-sub-id", "enabled": false, "notification_types": -31]) + XCTAssertNil(model.restApiDisabledReason) + + // Once the server reports another state, recording re-arms for a later operator disable + model.hydrateRestApiDisabledState(from: ["id": "test-sub-id", "enabled": true, "notification_types": 1]) + model.hydrateRestApiDisabledState(from: ["id": "test-sub-id", "enabled": false, "notification_types": -31]) + XCTAssertEqual(model.restApiDisabledReason, -31) + } + + func testRestApiDisable_clearedWhenServerReportsEnabled() { + let model = pushModelWithRestApiDisable() + + model.hydrateRestApiDisabledState(from: ["id": "test-sub-id", "enabled": true, "notification_types": 1]) + + XCTAssertNil(model.restApiDisabledReason) + } + + /** + A push subscription disabled through the REST API (server notification_types -31) must stay + disabled across a login to a different external ID. The fetch hydrates the server's disable + state onto the existing subscription, and the login's Create User payload echoes it back. + */ + func testLoginToDifferentUser_afterRestApiDisable_sendsDisabledPushSubscription() throws { + /* Setup */ + let client = MockOneSignalClient() + MockUserRequests.setDefaultCreateAnonUserResponses(with: client) + MockUserRequests.setDefaultIdentifyUserResponses(with: client, externalId: userA_EUID) + MockUserRequests.setDefaultCreateUserResponses(with: client, externalId: userB_EUID) + + // Fetching user A reports the push subscription disabled through the REST API + var disabledResponse = MockUserRequests.testDefaultFullCreateUserResponse( + onesignalId: anonUserOSID, + externalId: userA_EUID, + subscriptionId: testPushSubId + ) + let disabledSub = MockUserRequests.testDefaultPushSubPayload(id: testPushSubId) + .merging(["enabled": false, "notification_types": -31]) { _, new in new } + disabledResponse["subscriptions"] = [disabledSub] + client.setMockResponseForRequest( + request: "", + response: disabledResponse + ) + OneSignalCoreImpl.setSharedClient(client) + + // 1. Start with an anonymous user and log in to user A; the post-identify fetch + // hydrates the REST API disable onto the existing push subscription + OneSignalUserManagerImpl.sharedInstance.start() + OneSignalUserManagerImpl.sharedInstance.login(externalId: userA_EUID, token: nil) + + OneSignalCoreMocks.waitUntil("Fetch did not hydrate the REST API disable") { + OneSignalUserManagerImpl.sharedInstance.user.identityModel.externalId == userA_EUID && + OneSignalUserManagerImpl.sharedInstance.pushSubscriptionModel?.restApiDisabledReason == -31 + } + + /* When */ + + // 2. Log in to user B, which sends a Create User carrying the push subscription + OneSignalUserManagerImpl.sharedInstance.login(externalId: userB_EUID, token: nil) + + func createUserBRequest() -> OSRequestCreateUser? { + return client.executedRequests.compactMap { $0 as? OSRequestCreateUser }.first { + ($0.parameters?["identity"] as? [String: String])?[OS_EXTERNAL_ID] == userB_EUID + } + } + OneSignalCoreMocks.waitUntil("Create User for user B was not sent") { + createUserBRequest() != nil + } + + /* Then */ + + let subscriptions = try XCTUnwrap(createUserBRequest()?.parameters?["subscriptions"] as? [[String: Any]]) + XCTAssertEqual(subscriptions.first?["enabled"] as? Bool, false) + XCTAssertEqual(subscriptions.first?["notification_types"] as? Int, -31) + } } From 0de021e0f4d2a5602bd0b199d17cc076383a720a Mon Sep 17 00:00:00 2001 From: Nan Date: Wed, 2 Sep 2026 09:34:16 -0700 Subject: [PATCH 2/9] fix: make REST API disable bookkeeping atomic Recording, clearing, and accepting the server's disable state each read the opt-in guard and wrote the reason under separate lock acquisitions, so a hydrate racing an optIn() could leave the guard armed with the reason re-recorded. Each transition now does its guard and write in one critical section and fires events after release. OSPushSubscription.optedIn documents that it reflects the user's preference and OS permission, not a server-side disable. --- .../Source/OSSubscriptionModel.swift | 36 +++++++++++++++---- .../Source/OneSignalUserManagerImpl.swift | 2 ++ 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift index 1055623b8..d1ef50361 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift @@ -571,17 +571,33 @@ extension OSSubscriptionModel { /// Records the server's disable code unless `optIn()` cleared one and the server has not yet /// reported the subscription in another state; the user's explicit intent wins that race. private func recordRestApiDisable(_ code: Int) { - let clearedByUser = stateLock.withLock { state.restApiDisableClearedByUser } - guard !clearedByUser else { + let changed: Bool = stateLock.withLock { + guard !state.restApiDisableClearedByUser, state.restApiDisabledReason != code else { + return false + } + state.restApiDisabledReason = code + return true + } + guard changed else { return } - restApiDisabledReason = code + self.set(property: "restApiDisabledReason", newValue: code, preventServerUpdate: true) } /// Clears `restApiDisabledReason` and re-arms recording once the server reports a non-disabled state. private func acceptServerNonDisabledState() { - restApiDisabledReason = nil - stateLock.withLock { state.restApiDisableClearedByUser = false } + let changed: Bool = stateLock.withLock { + state.restApiDisableClearedByUser = false + guard state.restApiDisabledReason != nil else { + return false + } + state.restApiDisabledReason = nil + return true + } + guard changed else { + return + } + self.set(property: "restApiDisabledReason", newValue: nil as Int?, preventServerUpdate: true) } /// notification_types for outgoing payloads: the recorded disable code (the positive device /// value would re-enable it), else the device value, nil for the -1 default. @@ -632,11 +648,17 @@ extension OSSubscriptionModel { subscription. Called from `optIn()`, where a deliberate user action overrides the suppression. */ func clearRestApiDisable() { - let oldValue = swapValue(\.restApiDisabledReason, to: nil) + let oldValue: Int? = stateLock.withLock { + guard let recorded = state.restApiDisabledReason else { + return nil + } + state.restApiDisabledReason = nil + state.restApiDisableClearedByUser = true + return recorded + } guard oldValue != nil else { return } - stateLock.withLock { state.restApiDisableClearedByUser = true } self.set(property: "restApiDisabledReason", newValue: nil as Int?, preventServerUpdate: true) firePushSubscriptionChanged(.restApiDisabledReason(oldValue)) } diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift index a62b36f29..4e78b02e1 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift @@ -96,6 +96,8 @@ import OneSignalNotifications @objc public protocol OSPushSubscription { var id: String? { get } var token: String? { get } + /// The user's preference combined with OS permission; a subscription the app owner disabled + /// through the REST API still reports true here. var optedIn: Bool { get } func optIn() From 80f7cd40b1745df9e4b86f654e68d12054f12bbc Mon Sep 17 00:00:00 2001 From: Nan Date: Wed, 9 Sep 2026 12:01:06 -0700 Subject: [PATCH 3/9] feat: treat a dashboard unsubscribe (-22) as a remote disable The server reports notification_types -22 when someone turns a subscription off by hand from the dashboard. That means the same thing as -31, disabled through the REST API, so both codes now suppress outgoing subscription payloads the same way and neither is derived from device state. The two codes stay distinct. remoteDisabledReason records whichever one the server sent, and hydration writes it verbatim, so a payload echoes back the code the server actually reported. A second disable arriving under the other code replaces the recorded one instead of being ignored as already disabled. Renamed the restApiDisable members to remoteDisable, since "REST API" no longer describes the concept, and moved the codes themselves into a new OSRemoteDisable namespace at file scope. Keeping them inside the class pushed its body past swiftlint's type_body_length error threshold, and they describe the wire protocol rather than any one model instance. The NSCoding key changed with the property name, but no release has written the old key. --- .../Source/Executors/OSUserExecutor.swift | 6 +- .../Source/OSSubscriptionModel.swift | 156 +++++++------ .../Source/OneSignalUserManagerImpl.swift | 4 +- .../OneSignalUserTests.swift | 209 +++++++++++------- 4 files changed, 230 insertions(+), 145 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift index da6a6c6c1..afef173c7 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/Executors/OSUserExecutor.swift @@ -556,7 +556,7 @@ extension OSUserExecutor { } /// Hydrates the push subscription from a fetch or create response: the whole object before a - /// subscription ID exists, only the server's REST API disable state once one does. + /// subscription ID exists, only the server's remote disable state once one does. // TODO: Determine how to hydrate the push subscription, which is still faulty. // Hydrate by token if sub_id exists? // Problem: a user can have multiple iOS push subscription, and perhaps missing token @@ -585,10 +585,10 @@ extension OSUserExecutor { return } - // Only the REST API disable state hydrates onto an existing push subscription; the device + // Only the remote disable state hydrates onto an existing push subscription; the device // owns the rest. Skipping it lets the next subscription payload re-enable a suppressed device. for subModel in subscriptionObject where subModel["id"] as? String == subscriptionId { - pushSubscriptionModel?.hydrateRestApiDisabledState(from: subModel) + pushSubscriptionModel?.hydrateRemoteDisableState(from: subModel) break } } diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift index d1ef50361..d4083769c 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift @@ -94,6 +94,30 @@ enum OSSubscriptionType: String { case sms = "SMS" } +/** + The `notification_types` codes the app owner sets remotely, meaning the server turned the + subscription off rather than the device failing to register. The SDK treats them identically but + records them separately, so outgoing payloads echo back the code the server sent. Every other + negative code is a device or delivery error the device recovers from by re-asserting its own state. + */ +enum OSRemoteDisable { + /// Unsubscribed by hand from the dashboard. + static let manuallyUnsubscribed = -22 + + /// Disabled through the REST API. + static let restApiDisabled = -31 + + private static let allCodes: Set = [manuallyUnsubscribed, restApiDisabled] + + /// True when `notificationTypes` is one of the codes the app owner sets remotely. + static func matches(_ notificationTypes: Int?) -> Bool { + guard let notificationTypes else { + return false + } + return allCodes.contains(notificationTypes) + } +} + /** Internal subscription model. */ @@ -114,10 +138,10 @@ class OSSubscriptionModel: OSModel { var deviceModel: String? var appVersion: String? var netType: Int? - var restApiDisabledReason: Int? + var remoteDisabledReason: Int? // Not persisted; an optIn() clear outranks stale hydration until the server reports // the subscription in another state. - var restApiDisableClearedByUser = false + var remoteDisableClearedByUser = false } /** @@ -188,7 +212,7 @@ class OSSubscriptionModel: OSModel { // The disable code describes a specific server record; the record is gone when the ID resets. if newValue == nil { - restApiDisabledReason = nil + remoteDisabledReason = nil } // Cache the subscriptionId to UserDefaults for routine reads, and the OSResilientStorage mirror @@ -207,7 +231,7 @@ class OSSubscriptionModel: OSModel { address: state.address, reachable: state.reachable, isDisabled: state.isDisabled, - restApiDisabledReason: state.restApiDisabledReason + remoteDisabledReason: state.remoteDisabledReason ) } } @@ -275,25 +299,23 @@ class OSSubscriptionModel: OSModel { } } - /// The notification_types value for a REST API disable, the only server-owned code; other - /// negative codes are device or delivery errors the device recovers by re-asserting its state. - static let restApiDisabledNotificationType = -31 - /** - The server's REST API disable code, or nil when the server has not disabled this subscription. - Hydrated from responses, never derived from device state, and echoed back in payloads so routine - updates and logins don't re-enable a suppressed subscription. Cleared when a response reports - any other state, when the subscription ID resets, or by `optIn()`. + The server's remote disable code, either -22 (unsubscribed by hand from the dashboard) or -31 + (disabled through the REST API), or nil when the server has not disabled this subscription. The + two codes are kept apart so payloads echo back the one the server sent rather than collapsing + them. Hydrated from responses, never derived from device state, and echoed back in payloads so + routine updates and logins don't re-enable a suppressed subscription. Cleared when a response + reports any other state, when the subscription ID resets, or by `optIn()`. */ - var restApiDisabledReason: Int? { - get { stateLock.withLock { state.restApiDisabledReason } } + var remoteDisabledReason: Int? { + get { stateLock.withLock { state.remoteDisabledReason } } set { - let oldValue = swapValue(\.restApiDisabledReason, to: newValue) + let oldValue = swapValue(\.remoteDisabledReason, to: newValue) guard newValue != oldValue else { return } // Mirrors server state rather than a local change, so persist without generating a delta. - self.set(property: "restApiDisabledReason", newValue: newValue, preventServerUpdate: true) + self.set(property: "remoteDisabledReason", newValue: newValue, preventServerUpdate: true) } } @@ -408,8 +430,8 @@ class OSSubscriptionModel: OSModel { deviceModel: OSDeviceUtils.getDeviceVariant(), appVersion: Bundle.main.infoDictionary?["CFBundleShortVersionString"] as? String, netType: OSNetworkingUtils.getNetType() as? Int, - restApiDisabledReason: nil, - restApiDisableClearedByUser: false + remoteDisabledReason: nil, + remoteDisableClearedByUser: false ) super.init(changeNotifier: changeNotifier) @@ -431,7 +453,7 @@ class OSSubscriptionModel: OSModel { coder.encode(state.deviceModel, forKey: "deviceModel") coder.encode(state.appVersion, forKey: "appVersion") coder.encode(state.netType, forKey: "netType") - coder.encode(state.restApiDisabledReason, forKey: "restApiDisabledReason") + coder.encode(state.remoteDisabledReason, forKey: "remoteDisabledReason") } required init?(coder: NSCoder) { @@ -455,8 +477,8 @@ class OSSubscriptionModel: OSModel { deviceModel: coder.decodeObject(forKey: "deviceModel") as? String, appVersion: coder.decodeObject(forKey: "appVersion") as? String, netType: coder.decodeObject(forKey: "netType") as? Int, - restApiDisabledReason: coder.decodeObject(forKey: "restApiDisabledReason") as? Int, - restApiDisableClearedByUser: false + remoteDisabledReason: coder.decodeObject(forKey: "remoteDisabledReason") as? Int, + remoteDisableClearedByUser: false ) super.init(coder: coder) @@ -489,10 +511,10 @@ class OSSubscriptionModel: OSModel { } } - /// Applies a hydrated `enabled`. A REST API disable is server-owned, not a user opt-out, + /// Applies a hydrated `enabled`. A remote disable is server-owned, not a user opt-out, /// so it must not flip `_isDisabled`; the notification_types hydration records it instead. private func hydrateEnabled(_ enabled: Bool, response: [String: Any]) { - guard !isRestApiDisable(response) else { + guard !isRemoteDisable(response) else { return } if self.enabled != enabled { // TODO: Is this right? @@ -500,20 +522,20 @@ class OSSubscriptionModel: OSModel { } } - /// Routes a hydrated notification_types: -31 records the server's disable; any other value - /// clears it and becomes the device value. + /// Routes a hydrated notification_types: -22 and -31 record the server's disable verbatim, so + /// the recorded code stays distinguishable; any other value clears it and becomes the device value. private func hydrateNotificationTypes(_ value: Int) { - if value == Self.restApiDisabledNotificationType { - recordRestApiDisable(value) + if OSRemoteDisable.matches(value) { + recordRemoteDisable(value) } else { acceptServerNonDisabledState() self.notificationTypes = value } } - /// True when the response's notification_types carries a REST API disable code. - private func isRestApiDisable(_ response: [String: Any]) -> Bool { - return response["notification_types"] as? Int == Self.restApiDisabledNotificationType + /// True when the response's notification_types carries a remote disable code. + private func isRemoteDisable(_ response: [String: Any]) -> Bool { + return OSRemoteDisable.matches(response["notification_types"] as? Int) } // Using snake_case so we can use this in request bodies @@ -527,7 +549,7 @@ class OSSubscriptionModel: OSModel { address: state.address, reachable: state.reachable, isDisabled: state.isDisabled, - restApiDisabledReason: state.restApiDisabledReason + remoteDisabledReason: state.remoteDisabledReason ) json["test_type"] = state.testType json["device_os"] = state.deviceOs @@ -560,50 +582,52 @@ extension OSSubscriptionModel { // Calculates if push notifications are enabled on the device. // Does not consider the existence of the subscription_id, as we send this in the request to create a push subscription. - func calculateIsEnabled(address: String?, reachable: Bool, isDisabled: Bool, restApiDisabledReason: Int?) -> Bool { - return address != nil && reachable && !isDisabled && restApiDisabledReason == nil + func calculateIsEnabled(address: String?, reachable: Bool, isDisabled: Bool, remoteDisabledReason: Int?) -> Bool { + return address != nil && reachable && !isDisabled && remoteDisabledReason == nil } func updateNotificationTypes() { notificationTypes = Int(OSNotificationsManager.getNotificationTypes(_isDisabled)) } - /// Records the server's disable code unless `optIn()` cleared one and the server has not yet - /// reported the subscription in another state; the user's explicit intent wins that race. - private func recordRestApiDisable(_ code: Int) { + /// Records the server's disable code verbatim, so -22 and -31 stay distinguishable, unless + /// `optIn()` cleared one and the server has not yet reported the subscription in another state; + /// the user's explicit intent wins that race. + private func recordRemoteDisable(_ code: Int) { let changed: Bool = stateLock.withLock { - guard !state.restApiDisableClearedByUser, state.restApiDisabledReason != code else { + guard !state.remoteDisableClearedByUser, state.remoteDisabledReason != code else { return false } - state.restApiDisabledReason = code + state.remoteDisabledReason = code return true } guard changed else { return } - self.set(property: "restApiDisabledReason", newValue: code, preventServerUpdate: true) + self.set(property: "remoteDisabledReason", newValue: code, preventServerUpdate: true) } - /// Clears `restApiDisabledReason` and re-arms recording once the server reports a non-disabled state. + /// Clears `remoteDisabledReason` and re-arms recording once the server reports a non-disabled state. private func acceptServerNonDisabledState() { let changed: Bool = stateLock.withLock { - state.restApiDisableClearedByUser = false - guard state.restApiDisabledReason != nil else { + state.remoteDisableClearedByUser = false + guard state.remoteDisabledReason != nil else { return false } - state.restApiDisabledReason = nil + state.remoteDisabledReason = nil return true } guard changed else { return } - self.set(property: "restApiDisabledReason", newValue: nil as Int?, preventServerUpdate: true) + self.set(property: "remoteDisabledReason", newValue: nil as Int?, preventServerUpdate: true) } + /// notification_types for outgoing payloads: the recorded disable code (the positive device /// value would re-enable it), else the device value, nil for the -1 default. private func outboundNotificationTypes(_ state: State) -> Int? { - if let restApiDisabledReason = state.restApiDisabledReason { - return restApiDisabledReason + if let remoteDisabledReason = state.remoteDisabledReason { + return remoteDisabledReason } return state.notificationTypes != -1 ? state.notificationTypes : nil } @@ -622,45 +646,45 @@ extension OSSubscriptionModel { address: state.address, reachable: state.reachable, isDisabled: state.isDisabled, - restApiDisabledReason: state.restApiDisabledReason + remoteDisabledReason: state.remoteDisabledReason ) return params } /** - Mirrors the server's REST API disable state from a fetched subscription object: -31 records it, - any other reported value clears it. The device stays the source of truth for the rest of an - existing subscription's state, so nothing else is read. + Mirrors the server's remote disable state from a fetched subscription object: -22 and -31 are + recorded verbatim, any other reported value clears it. The device stays the source of truth for + the rest of an existing subscription's state, so nothing else is read. */ - func hydrateRestApiDisabledState(from serverSubscription: [String: Any]) { + func hydrateRemoteDisableState(from serverSubscription: [String: Any]) { guard type == .push, let serverTypes = serverSubscription["notification_types"] as? Int else { return } - if serverTypes == Self.restApiDisabledNotificationType { - recordRestApiDisable(serverTypes) + if OSRemoteDisable.matches(serverTypes) { + recordRemoteDisable(serverTypes) } else { acceptServerNonDisabledState() } } /** - Clears a REST API disable and enqueues an enabled-change delta so the server re-enables the + Clears a remote disable and enqueues an enabled-change delta so the server re-enables the subscription. Called from `optIn()`, where a deliberate user action overrides the suppression. */ - func clearRestApiDisable() { + func clearRemoteDisable() { let oldValue: Int? = stateLock.withLock { - guard let recorded = state.restApiDisabledReason else { + guard let recorded = state.remoteDisabledReason else { return nil } - state.restApiDisabledReason = nil - state.restApiDisableClearedByUser = true + state.remoteDisabledReason = nil + state.remoteDisableClearedByUser = true return recorded } guard oldValue != nil else { return } - self.set(property: "restApiDisabledReason", newValue: nil as Int?, preventServerUpdate: true) - firePushSubscriptionChanged(.restApiDisabledReason(oldValue)) + self.set(property: "remoteDisabledReason", newValue: nil as Int?, preventServerUpdate: true) + firePushSubscriptionChanged(.remoteDisabledReason(oldValue)) } func updateTestType() { @@ -696,7 +720,7 @@ extension OSSubscriptionModel { case reachable(Bool) case isDisabled(Bool) case address(String?) - case restApiDisabledReason(Int?) + case remoteDisabledReason(Int?) } func firePushSubscriptionChanged(_ changedProperty: OSPushPropertyChanged) { @@ -705,7 +729,7 @@ extension OSSubscriptionModel { var prevAddress = address var prevReachable = _reachable var prevIsDisabled = _isDisabled - var prevRestApiDisabledReason = restApiDisabledReason + var prevRemoteDisabledReason = remoteDisabledReason switch changedProperty { case .subscriptionId(let oldValue): @@ -716,15 +740,15 @@ extension OSSubscriptionModel { prevIsDisabled = oldValue case .address(let oldValue): prevAddress = oldValue - case .restApiDisabledReason(let oldValue): - prevRestApiDisabledReason = oldValue + case .remoteDisabledReason(let oldValue): + prevRemoteDisabledReason = oldValue } let prevIsEnabled = calculateIsEnabled( address: prevAddress, reachable: prevReachable, isDisabled: prevIsDisabled, - restApiDisabledReason: prevRestApiDisabledReason + remoteDisabledReason: prevRemoteDisabledReason ) let prevIsOptedIn = calculateIsOptedIn(reachable: prevReachable, isDisabled: prevIsDisabled) let prevSubscriptionState = OSPushSubscriptionState(id: prevId, token: prevAddress, optedIn: prevIsOptedIn) @@ -735,7 +759,7 @@ extension OSSubscriptionModel { address: address, reachable: _reachable, isDisabled: _isDisabled, - restApiDisabledReason: restApiDisabledReason + remoteDisabledReason: remoteDisabledReason ) if prevIsEnabled != newIsEnabled { diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift index 4e78b02e1..3bff446c1 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift @@ -97,7 +97,7 @@ import OneSignalNotifications var id: String? { get } var token: String? { get } /// The user's preference combined with OS permission; a subscription the app owner disabled - /// through the REST API still reports true here. + /// remotely, from the dashboard or the REST API, still reports true here. var optedIn: Bool { get } func optIn() @@ -936,7 +936,7 @@ extension OneSignalUserManagerImpl { } let model = pushSubscriptionModelStore.getModel(key: OS_PUSH_SUBSCRIPTION_MODEL_KEY) model?._isDisabled = false - model?.clearRestApiDisable() + model?.clearRemoteDisable() OSNotificationsManager.requestPermission(nil, fallbackToSettings: true) } diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift index 4f6647660..2c919e729 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift @@ -325,10 +325,16 @@ final class OneSignalUserTests: XCTestCase { XCTAssertNil(manager.currentUser(matching: identityModel.modelId)) } - // MARK: - REST API disabled push subscriptions + // MARK: - Remotely disabled push subscriptions - /// A push model hydrated with the server's REST API disable state (notification_types -31). - private func pushModelWithRestApiDisable() -> OSSubscriptionModel { + /// The notification_types codes the app owner sets remotely: -22 unsubscribes the subscription + /// by hand from the dashboard, -31 disables it through the REST API. Both mean "the server + /// turned this off" and are treated identically, but each is recorded as itself so outgoing + /// payloads echo back the code the server actually sent. + private static let remoteDisableCodes = [-22, -31] + + /// A push model hydrated with the server's remote disable state for `code`. + private func pushModelWithRemoteDisable(_ code: Int = -31) -> OSSubscriptionModel { let model = OSSubscriptionModel( type: .push, address: "test-token", @@ -337,60 +343,91 @@ final class OneSignalUserTests: XCTestCase { isDisabled: false, changeNotifier: OSEventProducer() ) - model.hydrateRestApiDisabledState(from: ["id": "test-sub-id", "enabled": false, "notification_types": -31]) + model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "enabled": false, "notification_types": code]) return model } - func testRestApiDisable_overridesOutgoingSubscriptionPayloads() { - let model = pushModelWithRestApiDisable() - XCTAssertEqual(model.restApiDisabledReason, -31) + func testRemoteDisable_recognizesOnlyTheTwoServerOwnedCodes() { + for code in Self.remoteDisableCodes { + XCTAssertTrue( + OSRemoteDisable.matches(code), + "\(code) is a code the app owner sets remotely and must suppress outgoing payloads" + ) + } + // Every other code describes device or delivery state the device recovers from by + // re-asserting its own truth. Treating one as a remote disable would permanently suppress + // a subscription the device could have re-enabled, so the boundary matters more than the + // list: -21/-23/-24 sit inside the range reserved for other platforms, and -30/-32 + // bracket the REST API code. + for code in [1, 0, -2, -3, -13, -21, -23, -24, -25, -30, -32] { + XCTAssertFalse( + OSRemoteDisable.matches(code), + "\(code) is device-recoverable and must not be recorded as a remote disable" + ) + } + XCTAssertFalse(OSRemoteDisable.matches(nil)) + } + + func testRemoteDisable_overridesOutgoingSubscriptionPayloads() { + for code in Self.remoteDisableCodes { + let model = pushModelWithRemoteDisable(code) + XCTAssertEqual(model.remoteDisabledReason, code) - let json = model.jsonRepresentation() - XCTAssertEqual(json["enabled"] as? Bool, false) - XCTAssertEqual(json["notification_types"] as? Int, -31) + // The recorded code round-trips rather than collapsing to the other one. + let json = model.jsonRepresentation() + XCTAssertEqual(json["enabled"] as? Bool, false) + XCTAssertEqual(json["notification_types"] as? Int, code) - let updateRequest = OSRequestUpdateSubscription(subscriptionModel: model) - let params = updateRequest.parameters?["subscription"] as? [String: Any] - XCTAssertEqual(params?["enabled"] as? Bool, false) - XCTAssertEqual(params?["notification_types"] as? Int, -31) + let updateRequest = OSRequestUpdateSubscription(subscriptionModel: model) + let params = updateRequest.parameters?["subscription"] as? [String: Any] + XCTAssertEqual(params?["enabled"] as? Bool, false) + XCTAssertEqual(params?["notification_types"] as? Int, code) + } } - func testRestApiDisable_survivesDeviceStateRefresh() { - let model = pushModelWithRestApiDisable() + func testRemoteDisable_survivesDeviceStateRefresh() { + for code in Self.remoteDisableCodes { + let model = pushModelWithRemoteDisable(code) - // Device-driven recomputes must not clear server-owned disable state - model.updateNotificationTypes() - model.update() + // Device-driven recomputes must not clear server-owned disable state + model.updateNotificationTypes() + model.update() - XCTAssertEqual(model.restApiDisabledReason, -31) - XCTAssertEqual(model.jsonRepresentation()["enabled"] as? Bool, false) + XCTAssertEqual(model.remoteDisabledReason, code) + XCTAssertEqual(model.jsonRepresentation()["enabled"] as? Bool, false) + } } - func testRestApiDisable_survivesArchiving() throws { - let model = pushModelWithRestApiDisable() + func testRemoteDisable_survivesArchiving() throws { + for code in Self.remoteDisableCodes { + let model = pushModelWithRemoteDisable(code) - let data = try NSKeyedArchiver.archivedData(withRootObject: model, requiringSecureCoding: false) - let unarchiver = try NSKeyedUnarchiver(forReadingFrom: data) - unarchiver.requiresSecureCoding = false - let restored = try XCTUnwrap(unarchiver.decodeObject(forKey: NSKeyedArchiveRootObjectKey) as? OSSubscriptionModel) + let data = try NSKeyedArchiver.archivedData(withRootObject: model, requiringSecureCoding: false) + let unarchiver = try NSKeyedUnarchiver(forReadingFrom: data) + unarchiver.requiresSecureCoding = false + let restored = try XCTUnwrap(unarchiver.decodeObject(forKey: NSKeyedArchiveRootObjectKey) as? OSSubscriptionModel) - XCTAssertEqual(restored.restApiDisabledReason, -31) - XCTAssertEqual(restored.jsonRepresentation()["enabled"] as? Bool, false) + XCTAssertEqual(restored.remoteDisabledReason, code) + XCTAssertEqual(restored.jsonRepresentation()["enabled"] as? Bool, false) + } } - func testRestApiDisable_clearedByOptIn() { - let model = pushModelWithRestApiDisable() + func testRemoteDisable_clearedByOptIn() { + for code in Self.remoteDisableCodes { + let model = pushModelWithRemoteDisable(code) - model.clearRestApiDisable() + model.clearRemoteDisable() - XCTAssertNil(model.restApiDisabledReason) - let json = model.jsonRepresentation() - XCTAssertEqual(json["enabled"] as? Bool, true) - XCTAssertNotEqual(json["notification_types"] as? Int, -31) + XCTAssertNil(model.remoteDisabledReason) + let json = model.jsonRepresentation() + XCTAssertEqual(json["enabled"] as? Bool, true) + XCTAssertNotEqual(json["notification_types"] as? Int, code) + } } - func testRestApiDisable_mirrorsServerField() { - // Only -31 is ever recorded; any other reported value is not, and clears an existing disable. + func testRemoteDisable_mirrorsServerField() { + // Only -22 and -31 are ever recorded; any other reported value is not, and clears an + // existing disable. Each code is recorded as itself, including replacing the other one. let model = OSSubscriptionModel( type: .push, address: "test-token", @@ -399,69 +436,93 @@ final class OneSignalUserTests: XCTestCase { isDisabled: false, changeNotifier: OSEventProducer() ) - model.hydrateRestApiDisabledState(from: ["id": "test-sub-id", "enabled": false, "notification_types": -2]) - XCTAssertNil(model.restApiDisabledReason) + model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "enabled": false, "notification_types": -2]) + XCTAssertNil(model.remoteDisabledReason) - model.hydrateRestApiDisabledState(from: ["id": "test-sub-id", "enabled": false, "notification_types": -31]) - XCTAssertEqual(model.restApiDisabledReason, -31) + for code in Self.remoteDisableCodes { + model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "enabled": false, "notification_types": code]) + XCTAssertEqual(model.remoteDisabledReason, code) - model.hydrateRestApiDisabledState(from: ["id": "test-sub-id", "enabled": false, "notification_types": -2]) - XCTAssertNil(model.restApiDisabledReason) - XCTAssertEqual(model.jsonRepresentation()["enabled"] as? Bool, true) + model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "enabled": false, "notification_types": -2]) + XCTAssertNil(model.remoteDisabledReason) + XCTAssertEqual(model.jsonRepresentation()["enabled"] as? Bool, true) + } + } + + func testRemoteDisable_switchingBetweenTheTwoCodesRecordsTheLatest() { + // The dashboard and the REST API can both act on the same subscription, so a second + // disable arriving under the other code must replace the recorded one rather than be + // ignored as "already disabled". + let model = pushModelWithRemoteDisable(-22) + XCTAssertEqual(model.remoteDisabledReason, -22) + + model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "enabled": false, "notification_types": -31]) + + XCTAssertEqual(model.remoteDisabledReason, -31) + XCTAssertEqual(model.jsonRepresentation()["notification_types"] as? Int, -31) } - func testRestApiDisable_clearedWhenSubscriptionIdResets() { + func testRemoteDisable_clearedWhenSubscriptionIdResets() { // The disable code describes a specific server record; it must die with the record. - let model = pushModelWithRestApiDisable() + for code in Self.remoteDisableCodes { + let model = pushModelWithRemoteDisable(code) - model.subscriptionId = nil + model.subscriptionId = nil - XCTAssertNil(model.restApiDisabledReason) - XCTAssertEqual(model.jsonRepresentation()["enabled"] as? Bool, true) + XCTAssertNil(model.remoteDisabledReason) + XCTAssertEqual(model.jsonRepresentation()["enabled"] as? Bool, true) + } } - func testRestApiDisable_optInOutranksStaleHydration() { - let model = pushModelWithRestApiDisable() + func testRemoteDisable_optInOutranksStaleHydration() { + for code in Self.remoteDisableCodes { + let model = pushModelWithRemoteDisable(code) - // A stale fetch response landing after optIn() cleared the disable must not re-record it - model.clearRestApiDisable() - model.hydrateRestApiDisabledState(from: ["id": "test-sub-id", "enabled": false, "notification_types": -31]) - XCTAssertNil(model.restApiDisabledReason) + // A stale fetch response landing after optIn() cleared the disable must not re-record it + model.clearRemoteDisable() + model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "enabled": false, "notification_types": code]) + XCTAssertNil(model.remoteDisabledReason) - // Once the server reports another state, recording re-arms for a later operator disable - model.hydrateRestApiDisabledState(from: ["id": "test-sub-id", "enabled": true, "notification_types": 1]) - model.hydrateRestApiDisabledState(from: ["id": "test-sub-id", "enabled": false, "notification_types": -31]) - XCTAssertEqual(model.restApiDisabledReason, -31) + // Once the server reports another state, recording re-arms for a later operator disable + model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "enabled": true, "notification_types": 1]) + model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "enabled": false, "notification_types": code]) + XCTAssertEqual(model.remoteDisabledReason, code) + } } - func testRestApiDisable_clearedWhenServerReportsEnabled() { - let model = pushModelWithRestApiDisable() + func testRemoteDisable_clearedWhenServerReportsEnabled() { + for code in Self.remoteDisableCodes { + let model = pushModelWithRemoteDisable(code) - model.hydrateRestApiDisabledState(from: ["id": "test-sub-id", "enabled": true, "notification_types": 1]) + model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "enabled": true, "notification_types": 1]) - XCTAssertNil(model.restApiDisabledReason) + XCTAssertNil(model.remoteDisabledReason) + } } /** - A push subscription disabled through the REST API (server notification_types -31) must stay - disabled across a login to a different external ID. The fetch hydrates the server's disable - state onto the existing subscription, and the login's Create User payload echoes it back. + A push subscription the app owner turned off remotely must stay disabled across a login to a + different external ID. The fetch hydrates the server's disable state onto the existing + subscription, and the login's Create User payload echoes it back. + + Uses -22 (unsubscribed by hand from the dashboard) rather than -31 so this end-to-end path also + proves the exact recorded code survives, instead of every remote disable reporting as -31. */ - func testLoginToDifferentUser_afterRestApiDisable_sendsDisabledPushSubscription() throws { + func testLoginToDifferentUser_afterRemoteDisable_sendsDisabledPushSubscription() throws { /* Setup */ let client = MockOneSignalClient() MockUserRequests.setDefaultCreateAnonUserResponses(with: client) MockUserRequests.setDefaultIdentifyUserResponses(with: client, externalId: userA_EUID) MockUserRequests.setDefaultCreateUserResponses(with: client, externalId: userB_EUID) - // Fetching user A reports the push subscription disabled through the REST API + // Fetching user A reports the push subscription unsubscribed from the dashboard var disabledResponse = MockUserRequests.testDefaultFullCreateUserResponse( onesignalId: anonUserOSID, externalId: userA_EUID, subscriptionId: testPushSubId ) let disabledSub = MockUserRequests.testDefaultPushSubPayload(id: testPushSubId) - .merging(["enabled": false, "notification_types": -31]) { _, new in new } + .merging(["enabled": false, "notification_types": -22]) { _, new in new } disabledResponse["subscriptions"] = [disabledSub] client.setMockResponseForRequest( request: "", @@ -470,13 +531,13 @@ final class OneSignalUserTests: XCTestCase { OneSignalCoreImpl.setSharedClient(client) // 1. Start with an anonymous user and log in to user A; the post-identify fetch - // hydrates the REST API disable onto the existing push subscription + // hydrates the remote disable onto the existing push subscription OneSignalUserManagerImpl.sharedInstance.start() OneSignalUserManagerImpl.sharedInstance.login(externalId: userA_EUID, token: nil) - OneSignalCoreMocks.waitUntil("Fetch did not hydrate the REST API disable") { + OneSignalCoreMocks.waitUntil("Fetch did not hydrate the remote disable") { OneSignalUserManagerImpl.sharedInstance.user.identityModel.externalId == userA_EUID && - OneSignalUserManagerImpl.sharedInstance.pushSubscriptionModel?.restApiDisabledReason == -31 + OneSignalUserManagerImpl.sharedInstance.pushSubscriptionModel?.remoteDisabledReason == -22 } /* When */ @@ -497,6 +558,6 @@ final class OneSignalUserTests: XCTestCase { let subscriptions = try XCTUnwrap(createUserBRequest()?.parameters?["subscriptions"] as? [[String: Any]]) XCTAssertEqual(subscriptions.first?["enabled"] as? Bool, false) - XCTAssertEqual(subscriptions.first?["notification_types"] as? Int, -31) + XCTAssertEqual(subscriptions.first?["notification_types"] as? Int, -22) } } From 04d09b882f43ec495d73673b2f8df236d0bfb6bf Mon Sep 17 00:00:00 2001 From: Nan Date: Wed, 9 Sep 2026 12:19:13 -0700 Subject: [PATCH 4/9] fix: log when a remote disable code is recorded or cleared recordRemoteDisable and acceptServerNonDisabledState changed the model silently, so parsing -22 or -31 off the wire left no trace at any log level short of the raw HTTP body. Add a DEBUG line at the point each one actually changes the value, matching the Android side. Reading the previous reason for that message also made the guard inside acceptServerNonDisabledState's lock redundant, since clearing a reason that is already nil is a no-op on a struct field. The outer guard already gates both the log and the persist. --- .../Source/OSSubscriptionModel.swift | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift index d4083769c..baa6963c5 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift @@ -604,22 +604,28 @@ extension OSSubscriptionModel { guard changed else { return } + OneSignalLog.onesignalLog( + .LL_DEBUG, + message: "OSSubscriptionModel: recording remote disable \(code) for push subscription \(subscriptionId ?? "nil")" + ) self.set(property: "remoteDisabledReason", newValue: code, preventServerUpdate: true) } /// Clears `remoteDisabledReason` and re-arms recording once the server reports a non-disabled state. private func acceptServerNonDisabledState() { - let changed: Bool = stateLock.withLock { + let clearedReason: Int? = stateLock.withLock { state.remoteDisableClearedByUser = false - guard state.remoteDisabledReason != nil else { - return false - } + let previous = state.remoteDisabledReason state.remoteDisabledReason = nil - return true + return previous } - guard changed else { + guard let clearedReason else { return } + OneSignalLog.onesignalLog( + .LL_DEBUG, + message: "OSSubscriptionModel: clearing remote disable \(clearedReason) for push subscription \(subscriptionId ?? "nil")" + ) self.set(property: "remoteDisabledReason", newValue: nil as Int?, preventServerUpdate: true) } From c7b061bcd70f98fdf3e2cf695e71442ce138be59 Mon Sep 17 00:00:00 2001 From: Nan Date: Wed, 9 Sep 2026 12:29:16 -0700 Subject: [PATCH 5/9] fix: arm the opt-in guard even when no remote disable was recorded clearRemoteDisable() returned before setting remoteDisableClearedByUser when nothing was recorded locally, which is exactly the state the race starts in. The customer disables the subscription, a fetch goes out that will report the code, and optIn() runs before that response lands. The guard never armed, so the fetch recorded the disable and the next update re-sent it, silently undoing the opt-in. Arm the flag on every opt-in and keep the persist and change event gated on there having been a recorded reason. Android already did this, in "keep an opt-in over a stale REST API disable report"; this brings iOS in line and adds the regression test that platform has. --- .../Source/OSSubscriptionModel.swift | 11 ++++---- .../OneSignalUserTests.swift | 27 +++++++++++++++++++ 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift index baa6963c5..a0b5d7310 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift @@ -679,14 +679,15 @@ extension OSSubscriptionModel { */ func clearRemoteDisable() { let oldValue: Int? = stateLock.withLock { - guard let recorded = state.remoteDisabledReason else { - return nil - } - state.remoteDisabledReason = nil + // The flag arms on every opt-in, not only when a disable was already recorded, because + // a fetch issued before this opt-in can still land the customer's first disable and + // undo it. Nothing is recorded locally on that first cycle, which is the common case. state.remoteDisableClearedByUser = true + let recorded = state.remoteDisabledReason + state.remoteDisabledReason = nil return recorded } - guard oldValue != nil else { + guard let oldValue else { return } self.set(property: "remoteDisabledReason", newValue: nil as Int?, preventServerUpdate: true) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift index 2c919e729..46dcad35a 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift @@ -490,6 +490,33 @@ final class OneSignalUserTests: XCTestCase { } } + func testRemoteDisable_optInOutranksStaleHydrationWithNothingRecorded() { + // The customer's first disable is the common case for this race, and it is the one a + // "only arm when a disable was already recorded" flag would miss. The customer disables + // the subscription, a fetch goes out that will report it, and the user calls optIn() + // before that response lands. Nothing is recorded locally at that point, so the opt-in + // has to arm the guard anyway or the fetch re-suppresses the subscription the user just + // opted into, and the next update re-sends the code. Android pins the same behavior in + // "optIn takes precedence over a pending fetch even when no remote disable was recorded". + let model = OSSubscriptionModel( + type: .push, + address: "test-token", + subscriptionId: "test-sub-id", + reachable: true, + isDisabled: false, + changeNotifier: OSEventProducer() + ) + XCTAssertNil(model.remoteDisabledReason) + + model.clearRemoteDisable() + + for code in Self.remoteDisableCodes { + model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "enabled": false, "notification_types": code]) + XCTAssertNil(model.remoteDisabledReason, "a fetch predating optIn() must not record \(code)") + XCTAssertEqual(model.jsonRepresentation()["enabled"] as? Bool, true) + } + } + func testRemoteDisable_clearedWhenServerReportsEnabled() { for code in Self.remoteDisableCodes { let model = pushModelWithRemoteDisable(code) From 05528ddf14d591e27baacc4dc07371f09f3c3bee Mon Sep 17 00:00:00 2001 From: Nan Date: Wed, 9 Sep 2026 13:36:40 -0700 Subject: [PATCH 6/9] incorporate a remote disable through optedIn optedIn was the user's preference combined with OS permission, which stopped describing "will push reach this device" once the SDK started respecting a remote disable. The disable now sticks instead of being flipped back on by the next routine update, and nothing else in the public API reveals it, so an app syncing preferences through the REST API would read opted in forever on a device receiving nothing. calculateIsOptedIn takes the recorded reason alongside reachable and isDisabled, so the value, the observer payload built by currentPushSubscriptionState, and the previous/current pair inside firePushSubscriptionChanged all agree. The two hydration paths now fire so the transition reaches observers, and they pass generateEnabledDelta: false because the state came from the server and an enabled delta would tell it what it just told us. An optIn() clear keeps the delta, which is how the server re-enables the subscription. --- .../Source/OSSubscriptionModel.swift | 54 +++++++-- .../Source/OneSignalUserManagerImpl.swift | 5 +- .../OneSignalUserTests.swift | 111 ++++++++++++++++++ 3 files changed, 155 insertions(+), 15 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift index a0b5d7310..345c7d310 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift @@ -237,10 +237,14 @@ class OSSubscriptionModel: OSModel { } var optedIn: Bool { - // optedIn = permission + userPreference + // optedIn = permission + userPreference + not suppressed by the app owner get { let state = snapshot() - return calculateIsOptedIn(reachable: state.reachable, isDisabled: state.isDisabled) + return calculateIsOptedIn( + reachable: state.reachable, + isDisabled: state.isDisabled, + remoteDisabledReason: state.remoteDisabledReason + ) } } @@ -570,14 +574,20 @@ extension OSSubscriptionModel { let state = snapshot() return OSPushSubscriptionState(id: state.subscriptionId, token: state.address, - optedIn: calculateIsOptedIn(reachable: state.reachable, isDisabled: state.isDisabled) + optedIn: calculateIsOptedIn( + reachable: state.reachable, + isDisabled: state.isDisabled, + remoteDisabledReason: state.remoteDisabledReason + ) ) } // Calculates if the device is opted in to push notification. - // Must have permission and not be opted out. - func calculateIsOptedIn(reachable: Bool, isDisabled: Bool) -> Bool { - return reachable && !isDisabled + // Must have permission, not be opted out, and not be disabled remotely by the app owner. A + // remote disable suppresses delivery just as surely as a missing permission or an opt-out, and + // it is the only one of the three the app cannot see any other way. + func calculateIsOptedIn(reachable: Bool, isDisabled: Bool, remoteDisabledReason: Int?) -> Bool { + return reachable && !isDisabled && remoteDisabledReason == nil } // Calculates if push notifications are enabled on the device. @@ -594,12 +604,13 @@ extension OSSubscriptionModel { /// `optIn()` cleared one and the server has not yet reported the subscription in another state; /// the user's explicit intent wins that race. private func recordRemoteDisable(_ code: Int) { - let changed: Bool = stateLock.withLock { + let (changed, previousReason) = stateLock.withLock { () -> (Bool, Int?) in guard !state.remoteDisableClearedByUser, state.remoteDisabledReason != code else { - return false + return (false, nil) } + let previousReason = state.remoteDisabledReason state.remoteDisabledReason = code - return true + return (true, previousReason) } guard changed else { return @@ -609,6 +620,8 @@ extension OSSubscriptionModel { message: "OSSubscriptionModel: recording remote disable \(code) for push subscription \(subscriptionId ?? "nil")" ) self.set(property: "remoteDisabledReason", newValue: code, preventServerUpdate: true) + // The disable takes `optedIn` to false, which observers need to hear about. + firePushSubscriptionChanged(.remoteDisabledReason(previousReason), generateEnabledDelta: false) } /// Clears `remoteDisabledReason` and re-arms recording once the server reports a non-disabled state. @@ -627,6 +640,8 @@ extension OSSubscriptionModel { message: "OSSubscriptionModel: clearing remote disable \(clearedReason) for push subscription \(subscriptionId ?? "nil")" ) self.set(property: "remoteDisabledReason", newValue: nil as Int?, preventServerUpdate: true) + // Clearing the disable takes `optedIn` back to true, which observers need to hear about. + firePushSubscriptionChanged(.remoteDisabledReason(clearedReason), generateEnabledDelta: false) } /// notification_types for outgoing payloads: the recorded disable code (the positive device @@ -730,7 +745,12 @@ extension OSSubscriptionModel { case remoteDisabledReason(Int?) } - func firePushSubscriptionChanged(_ changedProperty: OSPushPropertyChanged) { + /// Notifies push subscription observers of the state after `changedProperty` changed. + /// + /// Pass `generateEnabledDelta: false` when the change came from a server response: the server + /// already holds that state, so enqueueing an `enabled` delta for it would send a request that + /// tells the server what it just told us. + func firePushSubscriptionChanged(_ changedProperty: OSPushPropertyChanged, generateEnabledDelta: Bool = true) { // The previous state is the current state with only the changed property's old value substituted. var prevId = subscriptionId var prevAddress = address @@ -757,10 +777,18 @@ extension OSSubscriptionModel { isDisabled: prevIsDisabled, remoteDisabledReason: prevRemoteDisabledReason ) - let prevIsOptedIn = calculateIsOptedIn(reachable: prevReachable, isDisabled: prevIsDisabled) + let prevIsOptedIn = calculateIsOptedIn( + reachable: prevReachable, + isDisabled: prevIsDisabled, + remoteDisabledReason: prevRemoteDisabledReason + ) let prevSubscriptionState = OSPushSubscriptionState(id: prevId, token: prevAddress, optedIn: prevIsOptedIn) - let newIsOptedIn = calculateIsOptedIn(reachable: _reachable, isDisabled: _isDisabled) + let newIsOptedIn = calculateIsOptedIn( + reachable: _reachable, + isDisabled: _isDisabled, + remoteDisabledReason: remoteDisabledReason + ) let newIsEnabled = calculateIsEnabled( address: address, @@ -769,7 +797,7 @@ extension OSSubscriptionModel { remoteDisabledReason: remoteDisabledReason ) - if prevIsEnabled != newIsEnabled { + if generateEnabledDelta && prevIsEnabled != newIsEnabled { self.set(property: "enabled", newValue: newIsEnabled) } diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift index 3bff446c1..8385981a3 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift @@ -96,8 +96,9 @@ import OneSignalNotifications @objc public protocol OSPushSubscription { var id: String? { get } var token: String? { get } - /// The user's preference combined with OS permission; a subscription the app owner disabled - /// remotely, from the dashboard or the REST API, still reports true here. + /// The user's preference combined with OS permission. This is false while the app owner has the + /// subscription disabled remotely, from the dashboard or the REST API; `optIn()` clears that + /// suppression. var optedIn: Bool { get } func optIn() diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift index 46dcad35a..d901bd2b5 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift @@ -588,3 +588,114 @@ final class OneSignalUserTests: XCTestCase { XCTAssertEqual(subscriptions.first?["notification_types"] as? Int, -22) } } + + +/** + `optedIn` is the only signal the public API gives an app for "will push reach this device", so a + subscription the app owner turned off remotely has to report false there. Kept in its own class + because these build bare models and never touch the user manager singleton. + */ +final class RemoteDisableOptedInTests: XCTestCase { + + override func setUpWithError() throws { + OneSignalCoreMocks.clearUserDefaults() + // Firing a push subscription change reaches the user manager singleton and can enqueue a + // delta, so reset it between tests the way the rest of this suite does. + OneSignalUserMocks.reset() + OneSignalIdentifiers.currentAppId = "test-app-id" + } + + /// The notification_types codes the app owner sets remotely: -22 from the dashboard, -31 through + /// the REST API. Treated identically, recorded separately. + private static let remoteDisableCodes = [-22, -31] + + private func makePushModel() -> OSSubscriptionModel { + return OSSubscriptionModel( + type: .push, + address: "test-token", + subscriptionId: "test-sub-id", + reachable: true, + isDisabled: false, + changeNotifier: OSEventProducer() + ) + } + + private func pushModelWithRemoteDisable(_ code: Int) -> OSSubscriptionModel { + let model = makePushModel() + model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "enabled": false, "notification_types": code]) + return model + } + + func testRemoteDisable_reportsOptedInFalse() { + // A remote disable suppresses delivery, so the property clients read to decide whether push + // works must say so. Before this, a preference center showed "subscribed" on a device the + // app owner had turned off, and nothing in the public API revealed why. + for code in Self.remoteDisableCodes { + let model = pushModelWithRemoteDisable(code) + + XCTAssertFalse(model.optedIn, "a \(code) disable must report optedIn false") + // currentPushSubscriptionState builds the observer payload, so it has to agree. + XCTAssertFalse(model.currentPushSubscriptionState.optedIn) + + // The toggle a client drives off is not a dead end: opting in reports true again. + model.clearRemoteDisable() + XCTAssertTrue(model.optedIn) + XCTAssertTrue(model.currentPushSubscriptionState.optedIn) + } + } + + func testRemoteDisable_optedInIgnoresDeviceRecoverableCodes() { + // Only the two server-owned codes reach optedIn, and they arrive through + // remoteDisabledReason rather than notificationTypes. A device-side delivery error is + // recoverable by re-asserting local state, so it must not read as an opt-out to the app. + let model = makePushModel() + + for code in [-2, -13, -25, -30, -32] { + model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "notification_types": code]) + XCTAssertTrue(model.optedIn, "\(code) is device-recoverable and must not clear optedIn") + } + } + + func testRemoteDisable_hydrationDoesNotEnqueueAnEnabledDelta() { + // Hydrating the disable flips optedIn, which observers need to hear about, but the server + // is where the state came from. Enqueueing an `enabled` delta for it would send a request + // telling the server what it just told us, so the observer fires without one. + for code in Self.remoteDisableCodes { + let model = makePushModel() + let spy = SpyModelChangedHandler() + model.changeNotifier.subscribe(spy) + + model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "notification_types": code]) + XCTAssertEqual(model.remoteDisabledReason, code) + XCTAssertTrue(spy.serverUpdates.isEmpty, "hydrating \(code) must not enqueue a delta") + // The reason was still written, so the assertion above is not passing vacuously. + XCTAssertTrue(spy.hydratedUpdates.contains("remoteDisabledReason")) + + // Clearing it from the server side is the same story. + model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "notification_types": -2]) + XCTAssertNil(model.remoteDisabledReason) + XCTAssertFalse(spy.serverUpdates.contains("enabled")) + + // An optIn() clear is the opposite case: the delta is how the server re-enables it. + model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "notification_types": code]) + spy.serverUpdates.removeAll() + model.clearRemoteDisable() + XCTAssertTrue(spy.serverUpdates.contains("enabled"), "optIn() must re-enable on the server") + } + } +} + +/// Records the properties a model reported as changed, split by whether the change was meant to +/// reach the server. `hydrating` is true for writes that only mirror state the server already has. +private class SpyModelChangedHandler: OSModelChangedHandler { + var serverUpdates: [String] = [] + var hydratedUpdates: [String] = [] + + func onModelUpdated(args: OSModelChangedArgs, hydrating: Bool) { + if hydrating { + hydratedUpdates.append(args.property) + } else { + serverUpdates.append(args.property) + } + } +} From b31ea63c0f20afc3fc59e61c9e459fc55c432e31 Mon Sep 17 00:00:00 2001 From: Nan Date: Fri, 11 Sep 2026 13:13:45 -0700 Subject: [PATCH 7/9] fix: push-only remote disables, opt-in guard on real opt-ins, reset event state Three review findings on the remote disable work. Email and SMS models hydrate through the same path as push, and the server writes the same codes on their rows when the app owner disables them. The notification_types hydration recorded the code on those models and fired the push subscription observer with an email address or phone number as the token. Remote disable state is push-only now; email and SMS keep the plain enabled mapping they had before. The opt-in guard armed on every optIn(), including calls that changed nothing. Those send nothing, so the guard had no write to protect, and it stayed armed until a fetch reported a non-disabled state. Any disable that landed in the meantime was ignored for the life of the process, and the next routine PATCH re-enabled the subscription. Apps that call optIn() on every launch re-armed it before each session's fetch returned and never recorded a disable at all. The guard now arms only when the opt-in cleared a recorded disable or lifted the user's own opt-out. When the subscription ID resets, the recorded code was cleared before the observer event was built, so a disabled subscription reported its previous state as opted in. The old code now travels with the event, and the reset no longer generates an enabled delta, since the new record's state goes out with the create request. --- .../Source/OSSubscriptionModel.swift | 92 ++++++++----- .../Source/OneSignalUserManagerImpl.swift | 3 +- .../OneSignalUserTests.swift | 126 +++++++++++++++--- 3 files changed, 164 insertions(+), 57 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift index 345c7d310..8fb6c25e5 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift @@ -210,7 +210,9 @@ class OSSubscriptionModel: OSModel { return } - // The disable code describes a specific server record; the record is gone when the ID resets. + // The disable code describes a specific server record; the record is gone when the ID + // resets. The old code rides along on the event so the previous state still reads as disabled. + let previousRemoteDisabledReason = remoteDisabledReason if newValue == nil { remoteDisabledReason = nil } @@ -219,7 +221,11 @@ class OSSubscriptionModel: OSModel { OneSignalUserDefaults.initShared().saveString(forKey: OSUD_PUSH_SUBSCRIPTION_ID, withValue: newValue) OSResilientStorage.setString(newValue ?? "", forKey: OSResilientStorage.keySubscriptionId) - firePushSubscriptionChanged(.subscriptionId(oldValue)) + // The new record's state goes out with the create request, so no enabled delta here. + firePushSubscriptionChanged( + .subscriptionId(oldValue, remoteDisabledReason: previousRemoteDisabledReason), + generateEnabledDelta: false + ) } } @@ -515,33 +521,6 @@ class OSSubscriptionModel: OSModel { } } - /// Applies a hydrated `enabled`. A remote disable is server-owned, not a user opt-out, - /// so it must not flip `_isDisabled`; the notification_types hydration records it instead. - private func hydrateEnabled(_ enabled: Bool, response: [String: Any]) { - guard !isRemoteDisable(response) else { - return - } - if self.enabled != enabled { // TODO: Is this right? - _isDisabled = !enabled - } - } - - /// Routes a hydrated notification_types: -22 and -31 record the server's disable verbatim, so - /// the recorded code stays distinguishable; any other value clears it and becomes the device value. - private func hydrateNotificationTypes(_ value: Int) { - if OSRemoteDisable.matches(value) { - recordRemoteDisable(value) - } else { - acceptServerNonDisabledState() - self.notificationTypes = value - } - } - - /// True when the response's notification_types carries a remote disable code. - private func isRemoteDisable(_ response: [String: Any]) -> Bool { - return OSRemoteDisable.matches(response["notification_types"] as? Int) - } - // Using snake_case so we can use this in request bodies public func jsonRepresentation() -> [String: Any] { let state = snapshot() @@ -600,6 +579,41 @@ extension OSSubscriptionModel { notificationTypes = Int(OSNotificationsManager.getNotificationTypes(_isDisabled)) } + /// Applies a hydrated `enabled`. On a push subscription a remote disable is server-owned, not a + /// user opt-out, so it must not flip `_isDisabled`; the notification_types hydration records it + /// instead. Email and SMS have no remote disable state and keep the plain mapping. + private func hydrateEnabled(_ enabled: Bool, response: [String: Any]) { + guard type != .push || !isRemoteDisable(response) else { + return + } + if self.enabled != enabled { // TODO: Is this right? + _isDisabled = !enabled + } + } + + /// Routes a hydrated notification_types. On a push subscription -22 and -31 record the server's + /// disable verbatim, so the recorded code stays distinguishable, and any other value clears it and + /// becomes the device value. Email and SMS rows carry the same codes when the app owner disables + /// them and hydrate through here too, but the remote disable state and the push observer it + /// drives are push-only. + private func hydrateNotificationTypes(_ value: Int) { + guard type == .push else { + self.notificationTypes = value + return + } + if OSRemoteDisable.matches(value) { + recordRemoteDisable(value) + } else { + acceptServerNonDisabledState() + self.notificationTypes = value + } + } + + /// True when the response's notification_types carries a remote disable code. + private func isRemoteDisable(_ response: [String: Any]) -> Bool { + return OSRemoteDisable.matches(response["notification_types"] as? Int) + } + /// Records the server's disable code verbatim, so -22 and -31 stay distinguishable, unless /// `optIn()` cleared one and the server has not yet reported the subscription in another state; /// the user's explicit intent wins that race. @@ -691,14 +705,19 @@ extension OSSubscriptionModel { /** Clears a remote disable and enqueues an enabled-change delta so the server re-enables the subscription. Called from `optIn()`, where a deliberate user action overrides the suppression. + + The guard against a stale fetch arms only when the opt-in changed something, either a disable + was recorded here or the caller lifted the user's own opt-out, because only then is a re-enable + on its way that an earlier fetch can still contradict. An opt-in that changed nothing sends + nothing, and arming for it would blind the SDK to a disable that lands afterward for as long as + the process lives. */ - func clearRemoteDisable() { + func clearRemoteDisable(userWasOptedOut: Bool = false) { let oldValue: Int? = stateLock.withLock { - // The flag arms on every opt-in, not only when a disable was already recorded, because - // a fetch issued before this opt-in can still land the customer's first disable and - // undo it. Nothing is recorded locally on that first cycle, which is the common case. - state.remoteDisableClearedByUser = true let recorded = state.remoteDisabledReason + if recorded != nil || userWasOptedOut { + state.remoteDisableClearedByUser = true + } state.remoteDisabledReason = nil return recorded } @@ -738,7 +757,7 @@ extension OSSubscriptionModel { } enum OSPushPropertyChanged { - case subscriptionId(String?) + case subscriptionId(String?, remoteDisabledReason: Int?) case reachable(Bool) case isDisabled(Bool) case address(String?) @@ -759,8 +778,9 @@ extension OSSubscriptionModel { var prevRemoteDisabledReason = remoteDisabledReason switch changedProperty { - case .subscriptionId(let oldValue): + case .subscriptionId(let oldValue, let oldRemoteDisabledReason): prevId = oldValue + prevRemoteDisabledReason = oldRemoteDisabledReason case .reachable(let oldValue): prevReachable = oldValue case .isDisabled(let oldValue): diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift index 8385981a3..478e75113 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift @@ -936,8 +936,9 @@ extension OneSignalUserManagerImpl { return } let model = pushSubscriptionModelStore.getModel(key: OS_PUSH_SUBSCRIPTION_MODEL_KEY) + // Clear first so the guard is armed before the opt-out flips and its delta goes out. + model?.clearRemoteDisable(userWasOptedOut: model?._isDisabled == true) model?._isDisabled = false - model?.clearRemoteDisable() OSNotificationsManager.requestPermission(nil, fallbackToSettings: true) } diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift index d901bd2b5..f8564afde 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift @@ -490,30 +490,48 @@ final class OneSignalUserTests: XCTestCase { } } - func testRemoteDisable_optInOutranksStaleHydrationWithNothingRecorded() { - // The customer's first disable is the common case for this race, and it is the one a - // "only arm when a disable was already recorded" flag would miss. The customer disables - // the subscription, a fetch goes out that will report it, and the user calls optIn() - // before that response lands. Nothing is recorded locally at that point, so the opt-in - // has to arm the guard anyway or the fetch re-suppresses the subscription the user just - // opted into, and the next update re-sends the code. Android pins the same behavior in - // "optIn takes precedence over a pending fetch even when no remote disable was recorded". - let model = OSSubscriptionModel( - type: .push, - address: "test-token", - subscriptionId: "test-sub-id", - reachable: true, - isDisabled: false, - changeNotifier: OSEventProducer() - ) - XCTAssertNil(model.remoteDisabledReason) + func testRemoteDisable_optInFromOptedOutOutranksStaleHydration() { + // The customer's first disable is the common shape of this race. The customer disables the + // subscription, a fetch goes out that will report it, and an opted-out user opts in before + // that response lands. Nothing is recorded locally yet, but the opt-in changed the device + // state and a re-enable is on its way, so the stale response must not record over it. + for code in Self.remoteDisableCodes { + let model = OSSubscriptionModel( + type: .push, + address: "test-token", + subscriptionId: "test-sub-id", + reachable: true, + isDisabled: true, + changeNotifier: OSEventProducer() + ) - model.clearRemoteDisable() + model.clearRemoteDisable(userWasOptedOut: true) - for code in Self.remoteDisableCodes { model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "enabled": false, "notification_types": code]) XCTAssertNil(model.remoteDisabledReason, "a fetch predating optIn() must not record \(code)") - XCTAssertEqual(model.jsonRepresentation()["enabled"] as? Bool, true) + } + } + + func testRemoteDisable_optInThatChangesNothingDoesNotOutrankALaterDisable() { + // Many apps call optIn() on every launch. A call that finds the user already opted in with + // nothing recorded sends nothing, so it has no intent to protect. Arming the guard for it + // would make every fetch that reports a disable get ignored until the process died, and the + // next routine update would re-enable the subscription, which is the bug this branch fixes. + for code in Self.remoteDisableCodes { + let model = OSSubscriptionModel( + type: .push, + address: "test-token", + subscriptionId: "test-sub-id", + reachable: true, + isDisabled: false, + changeNotifier: OSEventProducer() + ) + + model.clearRemoteDisable(userWasOptedOut: false) + + model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "enabled": false, "notification_types": code]) + XCTAssertEqual(model.remoteDisabledReason, code, "a disable landing after a no-op optIn() must be recorded") + XCTAssertEqual(model.jsonRepresentation()["enabled"] as? Bool, false) } } @@ -683,10 +701,78 @@ final class RemoteDisableOptedInTests: XCTestCase { XCTAssertTrue(spy.serverUpdates.contains("enabled"), "optIn() must re-enable on the server") } } + + func testRemoteDisable_emailAndSmsHydrateWithoutRecordingOrNotifyingPush() { + // The server writes the same codes on email and SMS rows the app owner disables, and those + // models hydrate through the same path. The remote disable state and the push observer it + // drives are push-only, so an email row must not record one or reach push observers with an + // email address as the token. + let observer = SpyPushSubscriptionObserver() + OneSignalUserManagerImpl.sharedInstance.pushSubscriptionImpl.addObserver(observer) + defer { OneSignalUserManagerImpl.sharedInstance.pushSubscriptionImpl.removeObserver(observer) } + + for (type, address) in [(OSSubscriptionType.email, "person@example.com"), (.sms, "+15555550100")] { + for code in Self.remoteDisableCodes { + let model = OSSubscriptionModel( + type: type, + address: address, + subscriptionId: "test-other-sub-id", + reachable: true, + isDisabled: false, + changeNotifier: OSEventProducer() + ) + + model.hydrate([ + "id": "test-other-sub-id", "type": type.rawValue, "token": address, "enabled": false, "notification_types": code + ]) + + XCTAssertNil(model.remoteDisabledReason, "\(type.rawValue) must not record \(code)") + XCTAssertEqual(model.notificationTypes, code) + XCTAssertTrue(model._isDisabled, "\(type.rawValue) keeps the plain enabled mapping") + } + } + + // Observer callbacks hop to the main queue, so let anything queued arrive before asserting nothing did. + RunLoop.current.run(until: Date().addingTimeInterval(0.1)) + XCTAssertTrue(observer.changes.isEmpty, "email and SMS hydration must not reach push observers") + } + + func testRemoteDisable_resetEventReportsThePreviousStateAsDisabled() throws { + // Observers see the reset as one change: the disabled record with its ID, then no record and + // no disable. Clearing the code before building the event made the previous state read as + // opted in, so the app never saw the disable end. + for code in Self.remoteDisableCodes { + let model = pushModelWithRemoteDisable(code) + let observer = SpyPushSubscriptionObserver() + OneSignalUserManagerImpl.sharedInstance.pushSubscriptionImpl.addObserver(observer) + defer { OneSignalUserManagerImpl.sharedInstance.pushSubscriptionImpl.removeObserver(observer) } + let spy = SpyModelChangedHandler() + model.changeNotifier.subscribe(spy) + + model.subscriptionId = nil + + OneSignalCoreMocks.waitUntil("the reset did not reach push observers") { !observer.changes.isEmpty } + let change = try XCTUnwrap(observer.changes.last) + XCTAssertEqual(change.previous.id, "test-sub-id") + XCTAssertFalse(change.previous.optedIn, "the record was disabled by \(code) until it went away") + XCTAssertNil(change.current.id) + XCTAssertTrue(change.current.optedIn) + // The new record's state travels with the create request, not as a delta against the dead ID. + XCTAssertFalse(spy.serverUpdates.contains("enabled")) + } + } } /// Records the properties a model reported as changed, split by whether the change was meant to /// reach the server. `hydrating` is true for writes that only mirror state the server already has. +private class SpyPushSubscriptionObserver: NSObject, OSPushSubscriptionObserver { + var changes: [OSPushSubscriptionChangedState] = [] + + func onPushSubscriptionDidChange(state: OSPushSubscriptionChangedState) { + changes.append(state) + } +} + private class SpyModelChangedHandler: OSModelChangedHandler { var serverUpdates: [String] = [] var hydratedUpdates: [String] = [] From 5294dd3a61845f6104e62849a6c507549517f26c Mon Sep 17 00:00:00 2001 From: Nan Date: Fri, 11 Sep 2026 13:21:35 -0700 Subject: [PATCH 8/9] style: tighten the remote disable comments --- .../Source/OSSubscriptionModel.swift | 26 +++++++------------ .../Source/OneSignalUserManagerImpl.swift | 2 +- 2 files changed, 11 insertions(+), 17 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift index 8fb6c25e5..018b58144 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift @@ -210,8 +210,7 @@ class OSSubscriptionModel: OSModel { return } - // The disable code describes a specific server record; the record is gone when the ID - // resets. The old code rides along on the event so the previous state still reads as disabled. + // A reset drops the record's disable; the change event still needs the old code for its previous state. let previousRemoteDisabledReason = remoteDisabledReason if newValue == nil { remoteDisabledReason = nil @@ -221,7 +220,7 @@ class OSSubscriptionModel: OSModel { OneSignalUserDefaults.initShared().saveString(forKey: OSUD_PUSH_SUBSCRIPTION_ID, withValue: newValue) OSResilientStorage.setString(newValue ?? "", forKey: OSResilientStorage.keySubscriptionId) - // The new record's state goes out with the create request, so no enabled delta here. + // The create request carries the new record's state, so no enabled delta here. firePushSubscriptionChanged( .subscriptionId(oldValue, remoteDisabledReason: previousRemoteDisabledReason), generateEnabledDelta: false @@ -579,9 +578,8 @@ extension OSSubscriptionModel { notificationTypes = Int(OSNotificationsManager.getNotificationTypes(_isDisabled)) } - /// Applies a hydrated `enabled`. On a push subscription a remote disable is server-owned, not a - /// user opt-out, so it must not flip `_isDisabled`; the notification_types hydration records it - /// instead. Email and SMS have no remote disable state and keep the plain mapping. + /// Applies a hydrated `enabled`. A push remote disable is not a user opt-out, so it must not flip + /// `_isDisabled`; `hydrateNotificationTypes` records it instead. private func hydrateEnabled(_ enabled: Bool, response: [String: Any]) { guard type != .push || !isRemoteDisable(response) else { return @@ -591,11 +589,8 @@ extension OSSubscriptionModel { } } - /// Routes a hydrated notification_types. On a push subscription -22 and -31 record the server's - /// disable verbatim, so the recorded code stays distinguishable, and any other value clears it and - /// becomes the device value. Email and SMS rows carry the same codes when the app owner disables - /// them and hydrate through here too, but the remote disable state and the push observer it - /// drives are push-only. + /// Applies a hydrated notification_types. For push, -22 and -31 are recorded verbatim and any other + /// value clears the record. Email and SMS carry the same codes but have no remote disable state. private func hydrateNotificationTypes(_ value: Int) { guard type == .push else { self.notificationTypes = value @@ -706,11 +701,10 @@ extension OSSubscriptionModel { Clears a remote disable and enqueues an enabled-change delta so the server re-enables the subscription. Called from `optIn()`, where a deliberate user action overrides the suppression. - The guard against a stale fetch arms only when the opt-in changed something, either a disable - was recorded here or the caller lifted the user's own opt-out, because only then is a re-enable - on its way that an earlier fetch can still contradict. An opt-in that changed nothing sends - nothing, and arming for it would blind the SDK to a disable that lands afterward for as long as - the process lives. + `remoteDisableClearedByUser` is set only when the opt-in changed something, a disable recorded + here or the user's own opt-out per `userWasOptedOut`, since only then is a re-enable on its way + that an earlier fetch can contradict. An opt-in that changed nothing sends nothing, and setting + the flag for it would ignore every later disable until the process died. */ func clearRemoteDisable(userWasOptedOut: Bool = false) { let oldValue: Int? = stateLock.withLock { diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift index 478e75113..8eb593df2 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift @@ -936,7 +936,7 @@ extension OneSignalUserManagerImpl { return } let model = pushSubscriptionModelStore.getModel(key: OS_PUSH_SUBSCRIPTION_MODEL_KEY) - // Clear first so the guard is armed before the opt-out flips and its delta goes out. + // Clear first so `remoteDisableClearedByUser` is set before the opt-out flips and its delta goes out. model?.clearRemoteDisable(userWasOptedOut: model?._isDisabled == true) model?._isDisabled = false OSNotificationsManager.requestPermission(nil, fallbackToSettings: true) From 85764c4f08f83511f3c8371398d294a50b481b58 Mon Sep 17 00:00:00 2001 From: Nan Date: Fri, 11 Sep 2026 13:53:16 -0700 Subject: [PATCH 9/9] fix: count a missing permission as a real opt-in optIn() prompts for permission when the device has none, and the grant is the write that re-enables the subscription. That opt-in set nothing, so a fetch already in flight could record the operator's first disable before the grant landed, and the grant's update then carried the disable instead of enabling the subscription the user asked for. clearRemoteDisable() now reads the model itself, before optIn() flips the opt-out, and sets the flag when a disable was recorded, the user had opted out, or permission is missing. --- .../Source/OSSubscriptionModel.swift | 15 +-- .../Source/OneSignalUserManagerImpl.swift | 2 +- .../OneSignalUserTests.swift | 113 +++++++++++------- 3 files changed, 77 insertions(+), 53 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift index 018b58144..d6fa848c7 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OSSubscriptionModel.swift @@ -699,17 +699,18 @@ extension OSSubscriptionModel { /** Clears a remote disable and enqueues an enabled-change delta so the server re-enables the - subscription. Called from `optIn()`, where a deliberate user action overrides the suppression. + subscription. Called from `optIn()` before `_isDisabled` flips, so it still sees the state the + opt-in is changing. - `remoteDisableClearedByUser` is set only when the opt-in changed something, a disable recorded - here or the user's own opt-out per `userWasOptedOut`, since only then is a re-enable on its way - that an earlier fetch can contradict. An opt-in that changed nothing sends nothing, and setting - the flag for it would ignore every later disable until the process died. + `remoteDisableClearedByUser` is set only when the opt-in leads to a re-enable: a disable was + recorded here, the user had opted out, or permission is missing and `optIn()` is about to prompt + for it. An opt-in that changes nothing sends nothing, and setting the flag for it would ignore + every later disable until the process died. */ - func clearRemoteDisable(userWasOptedOut: Bool = false) { + func clearRemoteDisable() { let oldValue: Int? = stateLock.withLock { let recorded = state.remoteDisabledReason - if recorded != nil || userWasOptedOut { + if recorded != nil || state.isDisabled || !state.reachable { state.remoteDisableClearedByUser = true } state.remoteDisabledReason = nil diff --git a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift index 8eb593df2..c84a03b77 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUser/Source/OneSignalUserManagerImpl.swift @@ -937,7 +937,7 @@ extension OneSignalUserManagerImpl { } let model = pushSubscriptionModelStore.getModel(key: OS_PUSH_SUBSCRIPTION_MODEL_KEY) // Clear first so `remoteDisableClearedByUser` is set before the opt-out flips and its delta goes out. - model?.clearRemoteDisable(userWasOptedOut: model?._isDisabled == true) + model?.clearRemoteDisable() model?._isDisabled = false OSNotificationsManager.requestPermission(nil, fallbackToSettings: true) } diff --git a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift index f8564afde..cbf52d686 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalUserTests/OneSignalUserTests.swift @@ -490,51 +490,6 @@ final class OneSignalUserTests: XCTestCase { } } - func testRemoteDisable_optInFromOptedOutOutranksStaleHydration() { - // The customer's first disable is the common shape of this race. The customer disables the - // subscription, a fetch goes out that will report it, and an opted-out user opts in before - // that response lands. Nothing is recorded locally yet, but the opt-in changed the device - // state and a re-enable is on its way, so the stale response must not record over it. - for code in Self.remoteDisableCodes { - let model = OSSubscriptionModel( - type: .push, - address: "test-token", - subscriptionId: "test-sub-id", - reachable: true, - isDisabled: true, - changeNotifier: OSEventProducer() - ) - - model.clearRemoteDisable(userWasOptedOut: true) - - model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "enabled": false, "notification_types": code]) - XCTAssertNil(model.remoteDisabledReason, "a fetch predating optIn() must not record \(code)") - } - } - - func testRemoteDisable_optInThatChangesNothingDoesNotOutrankALaterDisable() { - // Many apps call optIn() on every launch. A call that finds the user already opted in with - // nothing recorded sends nothing, so it has no intent to protect. Arming the guard for it - // would make every fetch that reports a disable get ignored until the process died, and the - // next routine update would re-enable the subscription, which is the bug this branch fixes. - for code in Self.remoteDisableCodes { - let model = OSSubscriptionModel( - type: .push, - address: "test-token", - subscriptionId: "test-sub-id", - reachable: true, - isDisabled: false, - changeNotifier: OSEventProducer() - ) - - model.clearRemoteDisable(userWasOptedOut: false) - - model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "enabled": false, "notification_types": code]) - XCTAssertEqual(model.remoteDisabledReason, code, "a disable landing after a no-op optIn() must be recorded") - XCTAssertEqual(model.jsonRepresentation()["enabled"] as? Bool, false) - } - } - func testRemoteDisable_clearedWhenServerReportsEnabled() { for code in Self.remoteDisableCodes { let model = pushModelWithRemoteDisable(code) @@ -761,6 +716,74 @@ final class RemoteDisableOptedInTests: XCTestCase { XCTAssertFalse(spy.serverUpdates.contains("enabled")) } } + + func testRemoteDisable_optInFromOptedOutOutranksStaleHydration() { + // The customer's first disable is the common shape of this race. The customer disables the + // subscription, a fetch goes out that will report it, and an opted-out user opts in before + // that response lands. Nothing is recorded locally yet, but the opt-in changed the device + // state and a re-enable is on its way, so the stale response must not record over it. + for code in Self.remoteDisableCodes { + let model = OSSubscriptionModel( + type: .push, + address: "test-token", + subscriptionId: "test-sub-id", + reachable: true, + isDisabled: true, + changeNotifier: OSEventProducer() + ) + + model.clearRemoteDisable() + + model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "enabled": false, "notification_types": code]) + XCTAssertNil(model.remoteDisabledReason, "a fetch predating optIn() must not record \(code)") + } + } + + func testRemoteDisable_optInWithoutPermissionOutranksStaleHydration() { + // A user who never granted permission taps opt-in while a fetch that will report the + // customer's first disable is in flight. Nothing changes locally yet, but optIn() is about + // to prompt for permission, and the grant is the write that re-enables the subscription. + // Without the flag the stale response would record the disable first, and the grant's + // update would carry it instead of enabling the subscription the user just asked for. + for code in Self.remoteDisableCodes { + let model = OSSubscriptionModel( + type: .push, + address: "test-token", + subscriptionId: "test-sub-id", + reachable: false, + isDisabled: false, + changeNotifier: OSEventProducer() + ) + + model.clearRemoteDisable() + + model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "enabled": false, "notification_types": code]) + XCTAssertNil(model.remoteDisabledReason, "a fetch predating optIn() must not record \(code)") + } + } + + func testRemoteDisable_optInThatChangesNothingDoesNotOutrankALaterDisable() { + // Many apps call optIn() on every launch. A call that finds the user already opted in with + // nothing recorded sends nothing, so it has no intent to protect. Arming the guard for it + // would make every fetch that reports a disable get ignored until the process died, and the + // next routine update would re-enable the subscription, which is the bug this branch fixes. + for code in Self.remoteDisableCodes { + let model = OSSubscriptionModel( + type: .push, + address: "test-token", + subscriptionId: "test-sub-id", + reachable: true, + isDisabled: false, + changeNotifier: OSEventProducer() + ) + + model.clearRemoteDisable() + + model.hydrateRemoteDisableState(from: ["id": "test-sub-id", "enabled": false, "notification_types": code]) + XCTAssertEqual(model.remoteDisabledReason, code, "a disable landing after a no-op optIn() must be recorded") + XCTAssertEqual(model.jsonRepresentation()["enabled"] as? Bool, false) + } + } } /// Records the properties a model reported as changed, split by whether the change was meant to