feat: [JWT-3] feature flags, JWT config, and the Identity Verification gate - #1707
feat: [JWT-3] feature flags, JWT config, and the Identity Verification gate#1707nan-li wants to merge 3 commits into
Conversation
d0b8588 to
1b80146
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Multi-model adversarial review (interrogate)
Intent
Add the Identity Verification foundation without wiring consumers:
OSFeatureManager, three-stateOSUserJwtConfig(jwt_required), andOSIdentityVerificationService; hydrate from remote params; clear new cache keys on app-id change; retry failed params downloads so first-launch sessions are not stranded with an unknown requirement.
Reviewers
- A: claude-fable-5-thinking-xhigh — 10 findings
- B: gpt-5.6-sol-xhigh — 6 findings
- C: cursor-grok-4.5-high-fast — 5 findings
- D: claude-opus-5-thinking-high — 14 findings
Act On
- Stale
appIdin params retry (A/B/C/D) —scheduleDownloadIOSParamsRetryWithAppId:capturesappIdand only guards_downloadedParameters/_didCallDownloadParameters.handleAppIdChange:resets those flags, so a pending retry for app A can run after a switch to B and rehydrate IV + other remote params from the wrong app. - App-id change clears disk but not in-memory singletons (A/B/C/D) —
OSUD_USE_IDENTITY_VERIFICATION/OSUD_SDK_FEATURE_FLAGSare removed, butOSUserJwtConfig.shared/OSFeatureManager.sharedkeep the previous app’s values.refreshIfUnknown()cannot repair a non-unknown in-memory value. Add a productionreset()and call it fromhandleAppIdChange:. - No tests for the
OneSignal.mretry/hydrate path (A/B/C/D) — Swift unit tests cover the leaf types; the new retry state machine (four entry points, three statics,dispatch_after) is untested. That is where the highest-severity bugs live.
Consider
- Nested retry + indiscriminate 4xx (A/B/C/D) —
OneSignalClientalready retries 0/5xx up to 5 attempts; the outer loop multiplies that and also retries permanent 4xx / missing-app-id failures. - Empty-body 200 strands
unknown(A/B/D) — client can callsuccessBlock(nil); hydration is skipped but_downloadedParameters = truestill blocks further retries/session refetch. - Hydration handler double-delivery / stale last value (A/B/C/D) — register-then-read outside the lock races URLSession-queue hydrate. No production consumers yet, but this is the deferred-work signal.
- Orphan
requiresUserAuth(A/B/C/D) — writer removed;@objc publicproperty remains permanentlyfalse. PR text says removal is a later PR — prefer deleting in this wave if the stack allows. refreshIfUnknown/refreshIfEmptyunwired (D, noted by others) — prewarm helpers have no production caller;recoverIfDeferredalready exists inOneSignal.m.
Noted
- Unused JWT notification/token defines (
OS_ON_JWT_CONFIG_*, etc.) before posters exist - Feature-flag hydration deferred (scoped;
newCodePathsRun==ivBehaviorActiveuntil then) - Doc comments claim
OneSignalUserManagerImplreferences that do not exist yet - Hold lock across
UserDefaults synchronize; case-canonicalize onisEnabledlookup
Dismissed
- Collapsing feature manager + jwt config + gate into one type in this PR — stack intentionally lands the foundation before consumers; defer structural judo to the wiring PR unless reset/retry fixes force a reshape.
- “No consumers yet so races don’t matter” — foundation contract will be trusted by the next PRs; fix the retry/app-id issues now.
Agreement Map
All four models independently flagged (1) stale app-id retry, (2) disk-only app-id clear, and (3) missing OneSignal.m tests. Nested client/outer retry and observer race were near-unanimous. Lone-model pressure was mostly on collapsing types and resilient-storage mirroring — useful context, lower weight for this PR’s stated scope.
Lead verdict: Do not merge until Act On #1–#2 are fixed; add at least a focused test for the retry/app-id interaction before wiring consumers.
Sent by Cursor Automation: Untitled
nan-li
changed the title
nan-li
changed the title
1630188 to
e410174
Compare
e2a7d62 to
5ac1967
Compare
…gate The foundation the rest of Identity Verification is built on. Nothing consumes the gate yet, so this changes no behavior beyond the params handling below. OSUserJwtConfig holds whether the app requires a token as a three-state answer — unknown until remote params say — because "not yet known" and "not required" have to be told apart before anything can be sent. OSFeatureManager carries the flags remote params enable, and OSIdentityVerificationService is the single place that answers whether Identity Verification is on. Remote params now hydrate that requirement from jwt_required. A response that omits the key means Identity Verification is off for the app, while an empty response answers nothing and leaves the cached requirement standing. Because the operation repo will hold every queued operation until the requirement is known, a params request that failed was going to cost a first-launch app every tag, session count and event for the rest of the session: nothing retried it. It now retries with a backoff over five attempts and, failing that, tries again on the next session. Replaces the unread require_user_id_auth key with jwt_required. The requiresUserAuth property it wrote is removed in the next PR. Co-authored-by: Cursor <cursoragent@cursor.com>
Outer iOS-params retries now follow OSResponseStatusRetryable so classified 4xx are not re-armed for the session. Hydration handlers snapshot requirement under the same lock as registration. Co-authored-by: Cursor <cursoragent@cursor.com>
Rebasing onto main picked up SDK-5048 (#1723), which shipped a KMP-backed OSFeatureManager whose catalog already carries sdk_identity_verification. The standalone OSFeatureManager this branch added duplicated that role and collided with it, since two types of the same name cannot compile in one module. Drop the standalone manager, its tests, and the OSUD_SDK_FEATURE_FLAGS cache. OSIdentityVerificationService now reads the rollout flag from the shared manager by its catalog key, and its tests drive the flag through OSFeatureFlagsStore the way the existing feature-flag tests do. The app-id-change cleanup for the dropped cache goes too, because resetAndClearCachedFlags already covers the real flag cache. OSFeatureManagerProtocol and the OSFeatureFlag enum existed only to abstract the standalone manager, so they go as well.
e410174 to
b784b28
Compare
|
rebased on main, added a commit to reconcile the merged Feature Manager |
|
Potential issues:
|
|
Maybe also address flakey tests. |
abdulraqeeb33
left a comment
abdulraqeeb33
left a comment
There was a problem hiding this comment.
Thanks for splitting the gate out this cleanly. A couple of lifecycle holes around app-id change are worth closing before later PRs start trusting requirement.
Inline notes below, with a suggested fix for each.
| [sharedUserDefaults removeValueForKey:OSUD_RECEIVE_RECEIPTS_ENABLED]; | ||
| [sharedUserDefaults removeValueForKey:OS_PUSH_SUBSCRIPTION_MODEL_STORE_KEY]; | ||
|
|
||
| [sharedUserDefaults removeValueForKey:OSUD_USE_IDENTITY_VERIFICATION]; |
There was a problem hiding this comment.
Easy to miss because the disk key does get cleared.
handleAppIdChange removes OSUD_USE_IDENTITY_VERIFICATION and resets OSFeatureManager, but OSUserJwtConfig.shared keeps the previous app’s on/off. refreshIfUnknown only does anything while the value is still unknown, so it cannot repair this.
The bad direction is non-IV → IV: the SDK still thinks JWT is off and will create the new app’s user unsigned. Demo’s change-app-id path hits this.
I’d promote resetRequirementToUnknownForTests to a production reset (set _requirement to .unknown, clear the cache key) and call it next to resetAndClearCachedFlags.
A test that hydrates on, changes app id, then asserts requirement == .unknown would lock this in.
There was a problem hiding this comment.
Yeah, we have this app ID change stuff but it's really not supported after we shipped v5. I think something similar about app id changes was called out above and it was marked is a non-issue because the SDK doesn't actually support changing app ID.
We do have a lot of stale code in this method that we either should clean up or actually handle app ID changes (which we have a linear ticket about that discussion a little while back). The latest conversation was that Aug 11 slack thread asking for this feature
| if (_downloadedParameters || _didCallDownloadParameters) | ||
| return; | ||
|
|
||
| [self downloadIOSParamsWithAppId:appId]; |
There was a problem hiding this comment.
Same family as the in-memory reset above.
This timer captures appId. handleAppIdChange then flips _downloadedParameters / _didCallDownloadParameters back to false, so 5–40s later the old app’s params can hydrate the new process-wide jwt_required. The success handler at hydrateWithRequiresUserAuth has the same gap: it never checks that appId is still current.
In this dispatch_after and at the top of the params success handler, I’d bail when the captured appId is not OneSignalIdentifiers.currentAppId.
A test like “retry armed for A, switch to B, B’s policy must not become A’s” would cover both.
There was a problem hiding this comment.
same response, we never actually support app id change, and there's no real change to downloading remote params here except that the SDK will retry for a retriable error up to 4 times.
| _requirement = hydrated | ||
| // Written even when the value is unchanged, so a launch whose write was dropped by locked | ||
| // storage still ends up with the requirement on disk. | ||
| OneSignalUserDefaults.initShared().saveInteger(forKey: OSUD_USE_IDENTITY_VERIFICATION, withValue: hydrated.rawValue) |
There was a problem hiding this comment.
Smaller, same file family. hydrate keeps the NSLock across saveInteger, and that path calls synchronize. Readers of requirement are on the repo queue, IAM on main, and the executors, so they all wait on a disk flush.
You already kept the log and the handler out of the lock, which is the right instinct. I’d do the same for the write: update _requirement under the lock, then saveInteger after release. The “write even when unchanged” comment still holds.
3 participants
Description
One Line Summary
Add the Identity Verification foundation: feature flags, three-state JWT requirement config, the IV gate service, and remote-params hydration with retry — nothing consumes the gate yet.
Details
Motivation
Identity Verification needs a known answer to “does this app require a token?” before user-scoped work can be sent. That answer is three-state (unknown / on / off), comes from remote params (
jwt_required), and must survive a failed first-launch params download without stranding the session’s queued work forever.Scope
OSFeatureManager,OSUserJwtConfig,OSIdentityVerificationServiceOneSignal.mparams hydration fromjwt_required, retry/backoff, cache clears for the new keysrequire_user_id_authwithjwt_required(therequiresUserAuthproperty itself is removed in a later PR)Targets
5.6-main(already includes the project-file normalize and read-your-write consistency fixes).Testing
Unit testing
OSFeatureManagerTestsOSUserJwtConfigTestsOSIdentityVerificationServiceTestsManual testing
Built in sequence as part of the local JWT stack against an iOS Simulator.
Affected code checklist
Checklist
Overview
Testing
Final pass
Made with Cursor