fix: [SDK-5137] user request queue and identity model follow-ups from the JWT review - #1740
Draft
nan-li wants to merge 6 commits into
Draft
fix: [SDK-5137] user request queue and identity model follow-ups from the JWT review#1740nan-li wants to merge 6 commits into
nan-li wants to merge 6 commits into
Conversation
…reate User `uncacheUserRequests` on 5.7-main kept a restored Identify User only when Identity Verification was on or the Request could prepare at once, and the Identify prepare needs the `onesignal_id` of the user it identifies. That dropped the archive an offline first launch with a `login` leaves behind, the anonymous Create User followed by the Identify User that logs A in, on relaunch whenever the requirement was off or unknown, so the login was lost. On main the Identify was kept whenever its identify model was already in the repo, which a restored Create User ahead of it guarantees, and it went out once that response supplied the id. PR6 (#1711) collapsed those cases into one check. Restore main's rule. Keep the Identify when its identify model is in the repo, or while the requirement is not known to be off so reshape can decide, otherwise only when it can be sent as is. Keep main's guard too. Once the requirement is known off, `reshapeInvalidRequests` drops an Identify whose user never received an `onesignal_id` and has nothing queued to supply one, because the FIFO loop stops at the first unexecutable Request and would otherwise retry it for the life of the install. Three tests in UserExecutorTests: the archive above with the requirement off, the same archive turning out to require auth, and a never-preparable Identify kept while unknown then dropped once auth is known to be off. The first two fail without the fix.
…its model `OSIdentityModel.hydrateModel` fired the user-state observer for whichever model it hydrated. Stepping over a parked Create User (#1711) lets a later login proceed, so that parked Request can now complete after another user is current, and its response reported the earlier user as signed in and persisted that pair, so nothing later re-reported the current user. Move the fire out of the model to the three executor sites that hydrate an identity model (Create User and Fetch User through `parseFetchUserResponse`, Identify User, and Fetch Identity By Subscription), through `OSUserStateSnapshot.fireUserStateChangedIfCurrent`, which reports only when the model still belongs to the current user. The hydration itself stays, since Requests queued behind that user need the `onesignal_id`. Test in a new UserStateReportingTests, since UserJwtLifecycleTests is at SwiftLint's type body limit: login(A) with no token, login(B) with one, then answer A's ask; the app hears nothing new and the persisted pair still names B. Fails without the fix.
…ting for a token `_executePendingRequests` steps over a Request the auth layer parked for a token, so a login for another user behind it is not stranded, and it schedules no retry for a wait only the app can end. It told a park apart from any other failed prepare with `awaitsToken`, which only asked whether the owner had a token. A Create User still inside the new-records cool-down on its push subscription fails to prepare before the auth layer runs, so with Identity Verification on and no token yet it was stepped over as if parked: nobody was asked, nothing retried, and the login sat in the queue until the next launch. Replace `awaitsToken` with `parkedForToken`, which answers whether the last authorization of that Request actually parked it. `OSRequestAuth` keeps the parked Requests in a weak table: `park` adds one, every authorization starts by forgetting it so the entry reflects the latest attempt, and the executor consumes the answer right after a failed prepare. A prepare that fails before authorizing therefore reads as not parked and gets the delayed retry. Tests: three unit tests on the mark's lifecycle in OSRequestAuthTests, and UserExecutorRetryTests, a new file since UserExecutorTests is at SwiftLint's type body limit, holding a Create User in the cool-down and asserting the app is asked once it lifts. Fails without the fix.
… hydrates `OSIdentityModel.clearData` blanked every alias in preparation for the Fetch User response that hydrates them, and `executeFetchUserRequest` runs that clear right before parsing the response, on the response thread. A properties or subscription Delta built in that gap on another queue read the current user's `externalId` as nil, and with Identity Verification on `OSOperationRepo.enqueueDelta` drops an anonymous Delta before it is ever persisted, so the change was lost for good. Keep `external_id` in `clearData`. The fetch that follows is by `onesignal_id`, so it cannot change who the user is, and its response overwrites the alias anyway. One behavior change comes with it: a fetch response that omits `external_id` no longer demotes the local user to anonymous. Only a server-side unlink produces one, and the next `login` corrects it, so that reads as the right trade against silently dropping an identified user's work. Test in OSIdentityModelTests: an identified model keeps `externalId` across `clearData` and loses every other alias. Fails without the fix. The ownership convention comment in OSUserRequest.swift no longer cites the blanked aliases as the reason for the owner stamp.
…tests `OSOperationRepoTestSupport.snapshotDeltaQueue()` reads `deltaQueue` on the repo's own queue, but it lived in OneSignalOSCoreTests, so DeltaOwnershipTests and UserJwtLifecycleTests read the array directly from the test thread while the repo appends to it on `dispatchQueue`. Move the helper into OneSignalOSCoreMocks, which both test targets link, and route the two reads through it. No remove or update coverage is added, per the decision on #1710: neither Delta's owner reaches the server.
…g it for a token
`OSRequestDeleteSubscription.prepareForExecution` ran the Request through
`authorize`, so under Identity Verification a delete whose owner had no
valid token was parked and the app was asked for one, and the executor's
failure handler treated a 401 as a rejected token. The route the SDK uses,
`DELETE /apps/{app}/subscriptions/{id}`, performs no JWT check: the server
registers it with auth skipped, only the by-alias variant validates
Identity Verification claims, and the OAuth layer in front treats the user
bearer as not applicable. Signing bought nothing, and parking cost an
unsubscribe: `removeEmail` or `removeSms` while the user had no token,
followed by a logout or a switch, left the delete parked, restored on every
launch, asking for a token the app could no longer supply, and the
subscription stayed on the server.
Drop the `authorize` call and the 401 branch. The owner stays on the
Request for the anonymous purge, which is what it was for. The ownership
convention in OSUserRequest.swift records the exception next to Update
Subscription, which sends unsigned for the same reason.
Test in ExecutorAnonymousPurgeTests: with Identity Verification on, a delete
owned by a user with no token goes out with no Authorization header and
nobody is asked. Fails without the fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
One Line Summary
Six follow-ups from the Identity Verification review in the user request queue, request authorization, and the identity model, tracked on SDK-5137.
Details
Motivation
Review feedback on the Identity Verification stack was collected on [SDK-5137] instead of amending the stacked branches. This PR takes the items that live in the user request queue and the identity model. One of them is a regression against
mainthat does not depend on Identity Verification.Scope
Each fix is its own commit with its own test, and each test was run against the previous commit to confirm it fails without the fix.
mainkept it. That rule is restored, along withmain's guard against a request that can never be sent blocking the queue.Not changed: the public API, the push subscription path, and in-app messaging.
Testing
Unit testing
One test per fix, next to the code it covers. Two new files,
UserStateReportingTestsandUserExecutorRetryTests, because the existing test classes are at SwiftLint's size limit.Manual testing
Not run on a device. The full unit test plan was run locally on an iPhone 17 Pro simulator, 474 tests, 0 failures.
Affected code checklist
Checklist
Overview
Testing
Final pass
🤖 Generated with Claude Code