Conversation
4509a8f to
457b745
Compare
113e96b to
00d3799
Compare
|
@claude re-review |
|
Claude finished @nan-li's task in 3m 55s —— View job Re-review: feat/identity_verification_5.8
All four critical issues flagged in the previous review have been addressed. Here's a status breakdown: Previously Critical — Now Fixed ✅1. Runtime 401 not firing Fixed correctly. _operationRepo.setJwtInvalidatedHandler { externalId ->
_userManager.fireJwtInvalidated(externalId)
}And in the 2. Logout race condition — Fixed correctly. 3. Fixed correctly. The JSON parsing is wrapped in 4. Fixed correctly. The field now defaults to useIdentityVerification = responseJson.safeBool("jwt_required") ?: falseNon-IV apps get Remaining Observations (non-blocking)A. private var _jwtInvalidatedHandler: ((String) -> Unit)? = nullThis field is written from B. Expired JWT at logout time blocks the unsubscribe operation ( When IV=true and a 401 has already invalidated the JWT before the user calls This is an edge case (JWT expired + user logs out before providing a refresh) and the overall security posture of the IV feature prevents worse outcomes, but it may be worth documenting or handling explicitly — for example, by skipping the JWT gate for the unsubscribe-on-logout operation. Not blocking the PR. The implementation is solid. All critical issues are resolved and the approach is well-structured. The |
|
@claude re-review |
|
Claude finished @nan-li's task in 2m 39s —— View job Re-review: feat/identity_verification_5.8
Both remaining observations from the previous re-review have been addressed. The PR is clean. Previously Non-blocking — Now Fixed ✅A. Fixed in commit @Volatile
private var _jwtInvalidatedHandler: ((String) -> Unit)? = nullB. Expired JWT at logout blocks the push subscription disable ( Fixed in commit // Operation.kt:69
open val requiresJwt: Boolean get() = true
// UpdateSubscriptionOperation.kt:89
override val requiresJwt: Boolean get() = false
Minor Observation (no action needed)The The implementation is solid. All six issues identified across both review passes are resolved. The architecture is clean: |
972eace to
f6e4227
Compare
| <CurrentIssues> | ||
| <ID>ComplexCondition:InAppMessagesManager.kt$InAppMessagesManager$!message.isTriggerChanged && isMessageDisplayed && (isTriggerOnMessage || isNewTriggerAdded && isOnlyDynamicTriggers)</ID> | ||
| <ID>ComplexMethod:TriggerController.kt$TriggerController$private fun evaluateTrigger(trigger: Trigger): Boolean</ID> | ||
| <ID>ComplexMethod:InAppMessagesManager.kt$InAppMessagesManager$private suspend fun fetchMessages(rywData: RywData)</ID> |
There was a problem hiding this comment.
we should break it down if its a complex method.
There was a problem hiding this comment.
Yes, I undid the detekt changes and then added them back in the last few commits with reasoning. I actually didn't end up making changes to in-app-messages in detekt.
| * @param externalId The external ID of the user whose token is being updated. | ||
| * @param token The new JWT bearer token. | ||
| */ | ||
| fun updateUserJwt( |
There was a problem hiding this comment.
like i mentioned elsewhere, we need suspend methods for these
ca64cdf to
258c8e6
Compare
abdulraqeeb33
left a comment
There was a problem hiding this comment.
Lets try and do a bug bash on this today/tomorrow?
|
@claude review |
b445a01 to
17fc13b
Compare
There was a problem hiding this comment.
The two bugs I flagged in the last pass (IAM rate-limit poisoning and JWT exposure in login() logs) have been fixed in the latest commit — thanks for addressing them. Given the security-sensitive nature of this feature (JWT-based identity gating across 86 files), a final human look before merge is warranted.
Extended reasoning...
Overview
This PR adds Identity Verification (JWT-based authentication gating) to the Android SDK, spanning 86 files including new public API, a new JwtTokenStore, OperationRepo changes, IAM fetch gating, and HttpClient JWT header injection.
Prior Review Rounds
- Rounds 1–2 (late March): 6 bugs found and fixed.
- Round 3 (2026-04-14): 4 new bugs filed as inline comments. The latest commit (82678a5 "Address Claude review bot feedback on IV PR") addresses all of them:
- ✅ IAM rate-limit poisoning: Guard now checks both and , returning before is set.
- ✅ JWT logging in / : Both now use .
- ✅ FAIL_UNAUTHORIZED loop when IV=OFF: Gated on .
- ✅ JWT in HttpClient logs: Authorization header filtered from logged request properties.
- 403/401 conflation and stale ops at logout explicitly deferred by author (out of scope / by design).
Security Risks
The PR touches auth-critical paths: JWT storage/retrieval, JWT injection into HTTP headers, FAIL_UNAUTHORIZED error routing, and the public-facing / APIs. No new vulnerabilities found in this pass, but the breadth of the change means a human reviewer should give final sign-off.
Level of Scrutiny
This is production-facing security code that gates API calls behind developer-issued JWTs. The implementation looks correct across the key paths reviewed, but the sheer scope (86 files, new public API surface, threading concerns) means automated review alone is insufficient.
Other Factors
- The LoginHelper pre-HYDRATE logic is sound: the null case falls through to IV=OFF behavior, and IdentityVerificationService cleans up on HYDRATE.
- intentionally omits UNAUTHORIZED handling (requiresJwt=false by design; JWT is passed opportunistically).
- Test infrastructure () correctly gates test suites from the IV hold path.
16f9650 to
fbc53f2
Compare
…ication nullable, OptionalHeaders.jwt Foundational models and infrastructure for identity verification: - Create JwtTokenStore: persistent Map<externalId, JWT> backed by SharedPreferences, supporting multi-user JWT storage with getJwt/putJwt/invalidateJwt/pruneToExternalIds - Add var externalId to Operation base class so OperationRepo can stamp and gate operations per-user; remove redundant externalId from LoginUserOperation and TrackCustomEventOperation (same Model data-map key, no migration needed) - Change ConfigModel.useIdentityVerification from Boolean to Boolean? (null = unknown, false = off, true = on) to eliminate race between operation processing and remote params - Add jwt field to OptionalHeaders for passing Bearer tokens through HTTP layer - Add PREFS_OS_JWT_TOKENS key to PreferenceOneSignalKeys Made-with: Cursor
Same-user-id login with a fresh JWT (the documented post-401 refresh path) was storing the new token but never calling forceExecuteOperations, so ops parked by hasValidJwtIfRequired stayed deferred until something else woke the queue. updateUserJwt already does this correctly; reference #2599 LoginHelper does too. Match that design — drop redundant if (jwtBearerToken != null) guards (putJwt no-ops on null) and call forceExecuteOperations unconditionally on the same-id branch so the queue drains as soon as the developer supplies a fresh token. Extend the existing same-id+JWT test to verify forceExecuteOperations fires. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three review-driven fixes on the IV public-API surface: 1. OneSignal.kt: add @JvmStatic wrappers for updateUserJwt, addUserJwtInvalidatedListener, removeUserJwtInvalidatedListener, and updateUserJwtSuspend. Without these, the four new IOneSignal methods were unreachable from app code (the OneSignal object is the documented Java/Kotlin entry point and "implements IOneSignal in spirit"). Mirrors the convention from reference branch #2599. 2. OneSignalImp.kt: mask jwtBearerToken in login()/loginSuspend() DEBUG logs (...${jwtBearerToken?.takeLast(8)}). Pre-PR the parameter was a no-op (LoginHelper had a TODO) so the leak was theoretical; this PR wires the token through to JwtTokenStore.putJwt, so a live bearer credential now flows through Logging.log at DEBUG. updateUserJwt already masks; mirror that. 3. UserManager.kt: restore Logging.warn(msg, ex) form on the two new runCatching.onFailure handlers in fireJwtInvalidated and addJwtInvalidatedListener. Interpolating ${ex.message} drops the stack trace; Logging.warn already accepts a Throwable second arg that propagates through to log listeners and Otel — same pattern restored in JwtTokenStore via e76fb60. Also refreshes detekt baseline for the new MagicNumber (takeLast(8)), ConstructorParameterNaming (_jwtTokenStore on UserManager), TooManyFunctions (UserManager now also implements IJwtUpdateListener), and UseCheckOrError on the 4 new IllegalStateException throws. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When jwt_required hydrates true and removeOperationsWithoutExternalId drops the anon ops, surviving LoginUserOperations may still carry existingOnesignalId pointing at the just-dropped anon user (the merge-anon-into-identified link set by LoginHelper). Under IV that link is permanently unresolvable — anon user creation needs a JWT-less call the backend rejects — so canStartExecute=false sticks forever and deadlocks the queue (no other op can dispatch since they all wait on the login to resolve the local onesignal_id). Clear existingOnesignalId on every surviving LoginUserOperation so the executor takes the createUser (upsert) path. Matches the same fix in reference branches #2599 and #2613. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Revert the order swap from dc93995 — that commit reordered switchUserIv to switch users first and then set isDisabledInternally on the new push sub with NO_PROPOGATE, on the bot review's claim that firing an UpdateSubscriptionOperation against the OLD user was a bug. It is not a bug — it is the intentional behavior in reference branches #2599 and #2613: setting the flag on the CURRENT push sub with the default NORMAL tag fires an UpdateSubscriptionOperation that tells the backend "this device is unsubscribing as the user logs out", dispatched with the OLD user's still-valid JWT before the switch. Without this, logout under IV silently leaves the just-departed user's push sub subscribed server-side. The new (anonymous) user's model not carrying isDisabledInternally is fine: anon ops are filtered by hasValidJwtIfRequired (externalId=null under IV-active), so they accumulate but never dispatch. Update the test to verify the corrected order. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LoginHelper.switchUser was setting existingOnesignalId = currentOneSignalId when the prior user was anonymous (currentExternalId == null). Under IV-required that anon user was never created server-side — no JWT — so its onesignalId stays a local id forever. Carrying it as existingOnesignalId on the new LoginUserOperation makes canStartExecute=false stick, deadlocking the queue across logout→login cycles. Skip the merge-link entirely when useIdentityVerification == REQUIRED so the executor takes the createUser (upsert) path. Matches reference branches #2599 and #2613. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add shouldSuppressAnonymousOp from reference branch #2599: any non- LoginUserOperation enqueued without an externalId is dropped at the enqueue boundary when useIdentityVerification == REQUIRED, since it can't authenticate and would otherwise sit in the queue forever blocked by hasValidJwtIfRequired. LoginUserOperation is exempt — it's enqueued intentionally during logout and purged later by removeOperationsWithoutExternalId if needed. Outer-gated on _identityVerificationService.newCodePathsRun so Phase 1 customers stay byte-for-byte on the legacy enqueue path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously the loginUser block left the loading overlay on under the expectation that onUserStateChange → fetchUserDataFromApi() would dismiss it. Under IV, login can fail (bad JWT, anon-purge, etc.) and onUserStateChange may never fire, leaving the overlay up indefinitely. Match #2599 commit 0f1ad82 — dismiss the loader as soon as the SDK call returns. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…s iOS The buffer-and-consume design (pendingJwtInvalidatedExternalId + jwtInvalidatedLock + consume-on-first-subscribe replay) was added to handle cold-start 401s when no listener was subscribed yet. iOS doesn't do this — it fires only to currently-subscribed listeners, late subscribers miss earlier events. Match iOS: drop the buffer entirely. Reverts the structure introduced by d137481 ("align jwt-invalidated replay with #2613 buffer-and-consume"). The simpler design here matches reference branch #2599. Side effects: - onModelReplaced becomes a no-op (no buffer to clear). Kept the override since ISingletonModelStoreChangeHandler requires it. - fireJwtInvalidated now uses OneSignalDispatchers.launchOnDefault per maintainer request (#3184062053) instead of a custom CoroutineScope(SupervisorJob() + Dispatchers.Default). - KDoc on IOneSignal.addUserJwtInvalidatedListener and the OneSignal.kt facade now spell out the pure-pub/sub semantics: "Subscribe early (e.g. in Application.onCreate) to avoid missing cold-start 401s." Test changes: drop the 4 buffer/replay-specific tests (listener-replay-buffered-event, consume-on-first-subscribe, fire-with-subscribers-no-buffer, onModelReplaced-clears-buffer). Replace with one test that confirms late subscribers don't receive earlier events.
…Store
JwtTokenStore is the source of truth for invalidation events (its
invalidateJwt method is what fires both internal and developer-facing
notifications). It already implements IEventNotifier<IJwtUpdateListener>
and dispatches to internal listeners. Adding a second EventProducer for
the developer-facing IUserJwtInvalidatedListener is consistent with the
class's existing notifier shape — just a different audience.
This eliminates the bridge that lived in UserManager:
- UserManager loses IJwtUpdateListener, _jwtTokenStore ctor param,
jwtInvalidatedNotifier, addJwtInvalidatedListener,
removeJwtInvalidatedListener, fireJwtInvalidated, onJwtInvalidated.
- OneSignalImp.{add,remove}UserJwtInvalidatedListener now route directly
to JwtTokenStore.
Side benefits:
- No eager-construction problem: JwtTokenStore is already pulled in by
OperationRepo (an IStartableService), so the bridge is live before
any 401 can dispatch — no IBootstrapService registration needed for
any class.
- UserManager goes back to being purely about user state.
- Improves on reference branches #2599 / #2613 which kept the bridge in
UserManager and accepted an eager-construction-via-IAM dependency.
Tests:
- Move the 3 JWT-listener tests from UserManagerTests to
JwtTokenStoreTests (invalidation fires listener, late subscriber
doesn't get replay, removeListener stops notifications).
- Drop _jwtTokenStore param from UserManager construction sites.
When jwt_required hydrates true and removeOperationsWithoutExternalId drops the anon ops, surviving LoginUserOperations may still carry existingOnesignalId pointing at the just-dropped anon user (the merge-anon-into-identified link set by LoginHelper). Under IV that link is permanently unresolvable — anon user creation needs a JWT-less call the backend rejects — so canStartExecute=false sticks forever and deadlocks the queue (no other op can dispatch since they all wait on the login to resolve the local onesignal_id). Clear existingOnesignalId on every surviving LoginUserOperation so the executor takes the createUser (upsert) path. Matches the same fix in reference branches #2599 and #2613. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Revert the order swap from dc93995 — that commit reordered switchUserIv to switch users first and then set isDisabledInternally on the new push sub with NO_PROPOGATE, on the bot review's claim that firing an UpdateSubscriptionOperation against the OLD user was a bug. It is not a bug — it is the intentional behavior in reference branches #2599 and #2613: setting the flag on the CURRENT push sub with the default NORMAL tag fires an UpdateSubscriptionOperation that tells the backend "this device is unsubscribing as the user logs out", dispatched with the OLD user's still-valid JWT before the switch. Without this, logout under IV silently leaves the just-departed user's push sub subscribed server-side. The new (anonymous) user's model not carrying isDisabledInternally is fine: anon ops are filtered by hasValidJwtIfRequired (externalId=null under IV-active), so they accumulate but never dispatch. Update the test to verify the corrected order. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LoginHelper.switchUser was setting existingOnesignalId = currentOneSignalId when the prior user was anonymous (currentExternalId == null). Under IV-required that anon user was never created server-side — no JWT — so its onesignalId stays a local id forever. Carrying it as existingOnesignalId on the new LoginUserOperation makes canStartExecute=false stick, deadlocking the queue across logout→login cycles. Skip the merge-link entirely when useIdentityVerification == REQUIRED so the executor takes the createUser (upsert) path. Matches reference branches #2599 and #2613. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add shouldSuppressAnonymousOp from reference branch #2599: any non- LoginUserOperation enqueued without an externalId is dropped at the enqueue boundary when useIdentityVerification == REQUIRED, since it can't authenticate and would otherwise sit in the queue forever blocked by hasValidJwtIfRequired. LoginUserOperation is exempt — it's enqueued intentionally during logout and purged later by removeOperationsWithoutExternalId if needed. Outer-gated on _identityVerificationService.newCodePathsRun so Phase 1 customers stay byte-for-byte on the legacy enqueue path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously the loginUser block left the loading overlay on under the expectation that onUserStateChange → fetchUserDataFromApi() would dismiss it. Under IV, login can fail (bad JWT, anon-purge, etc.) and onUserStateChange may never fire, leaving the overlay up indefinitely. Match #2599 commit 0f1ad82 — dismiss the loader as soon as the SDK call returns. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Store
JwtTokenStore is the source of truth for invalidation events (its
invalidateJwt method is what fires both internal and developer-facing
notifications). It already implements IEventNotifier<IJwtUpdateListener>
and dispatches to internal listeners. Adding a second EventProducer for
the developer-facing IUserJwtInvalidatedListener is consistent with the
class's existing notifier shape — just a different audience.
This eliminates the bridge that lived in UserManager:
- UserManager loses IJwtUpdateListener, _jwtTokenStore ctor param,
jwtInvalidatedNotifier, addJwtInvalidatedListener,
removeJwtInvalidatedListener, fireJwtInvalidated, onJwtInvalidated.
- OneSignalImp.{add,remove}UserJwtInvalidatedListener now route directly
to JwtTokenStore.
Side benefits:
- No eager-construction problem: JwtTokenStore is already pulled in by
OperationRepo (an IStartableService), so the bridge is live before
any 401 can dispatch — no IBootstrapService registration needed for
any class.
- UserManager goes back to being purely about user state.
- Improves on reference branches #2599 / #2613 which kept the bridge in
UserManager and accepted an eager-construction-via-IAM dependency.
Tests:
- Move the 3 JWT-listener tests from UserManagerTests to
JwtTokenStoreTests (invalidation fires listener, late subscriber
doesn't get replay, removeListener stops notifications).
- Drop _jwtTokenStore param from UserManager construction sites.
When jwt_required hydrates true and removeOperationsWithoutExternalId drops the anon ops, surviving LoginUserOperations may still carry existingOnesignalId pointing at the just-dropped anon user (the merge-anon-into-identified link set by LoginHelper). Under IV that link is permanently unresolvable — anon user creation needs a JWT-less call the backend rejects — so canStartExecute=false sticks forever and deadlocks the queue (no other op can dispatch since they all wait on the login to resolve the local onesignal_id). Clear existingOnesignalId on every surviving LoginUserOperation so the executor takes the createUser (upsert) path. Matches the same fix in reference branches #2599 and #2613. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Revert the order swap from dc93995 — that commit reordered switchUserIv to switch users first and then set isDisabledInternally on the new push sub with NO_PROPOGATE, on the bot review's claim that firing an UpdateSubscriptionOperation against the OLD user was a bug. It is not a bug — it is the intentional behavior in reference branches #2599 and #2613: setting the flag on the CURRENT push sub with the default NORMAL tag fires an UpdateSubscriptionOperation that tells the backend "this device is unsubscribing as the user logs out", dispatched with the OLD user's still-valid JWT before the switch. Without this, logout under IV silently leaves the just-departed user's push sub subscribed server-side. The new (anonymous) user's model not carrying isDisabledInternally is fine: anon ops are filtered by hasValidJwtIfRequired (externalId=null under IV-active), so they accumulate but never dispatch. Update the test to verify the corrected order. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LoginHelper.switchUser was setting existingOnesignalId = currentOneSignalId when the prior user was anonymous (currentExternalId == null). Under IV-required that anon user was never created server-side — no JWT — so its onesignalId stays a local id forever. Carrying it as existingOnesignalId on the new LoginUserOperation makes canStartExecute=false stick, deadlocking the queue across logout→login cycles. Skip the merge-link entirely when useIdentityVerification == REQUIRED so the executor takes the createUser (upsert) path. Matches reference branches #2599 and #2613. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add shouldSuppressAnonymousOp from reference branch #2599: any non- LoginUserOperation enqueued without an externalId is dropped at the enqueue boundary when useIdentityVerification == REQUIRED, since it can't authenticate and would otherwise sit in the queue forever blocked by hasValidJwtIfRequired. LoginUserOperation is exempt — it's enqueued intentionally during logout and purged later by removeOperationsWithoutExternalId if needed. Outer-gated on _identityVerificationService.newCodePathsRun so Phase 1 customers stay byte-for-byte on the legacy enqueue path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously the loginUser block left the loading overlay on under the expectation that onUserStateChange → fetchUserDataFromApi() would dismiss it. Under IV, login can fail (bad JWT, anon-purge, etc.) and onUserStateChange may never fire, leaving the overlay up indefinitely. Match #2599 commit 0f1ad82 — dismiss the loader as soon as the SDK call returns. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Rewrite the listener replay to match the reference design from #2613 commit 89cca43: - Lock-protected pendingJwtInvalidatedExternalId (consume-on-first-subscribe) replaces the @volatile lastJwtInvalidatedExternalId (always-replay) - fireJwtInvalidated buffers ONLY when no subscribers exist; otherwise schedules an async fire. Closes the double-fire race. - onModelReplaced clears the buffer on IdentityModel replacement (login or logout switch via UserSwitcher.createAndSwitchToNewUser → replace). - Drop the explicit clearLastJwtInvalidated() method and its calls in OneSignalImp.logout() / logoutSuspend() — clear is now automatic. Match #2599 conventions: - Drop the isInitialized throw in addUserJwtInvalidatedListener / removeUserJwtInvalidatedListener. - Add updateUserJwtSuspend. Update IUserJwtInvalidatedListener docstring to clarify replay is synchronous on the caller's thread; regular fire is async. Tests rewritten for the new semantics: first-subscriber-replay, consume-on-first, fire-with-subscribers-doesn't-buffer, onModelReplaced clears.
Same-user-id login with a fresh JWT (the documented post-401 refresh path) was storing the new token but never calling forceExecuteOperations, so ops parked by hasValidJwtIfRequired stayed deferred until something else woke the queue. updateUserJwt already does this correctly; reference #2599 LoginHelper does too. Match that design — drop redundant if (jwtBearerToken != null) guards (putJwt no-ops on null) and call forceExecuteOperations unconditionally on the same-id branch so the queue drains as soon as the developer supplies a fresh token. Extend the existing same-id+JWT test to verify forceExecuteOperations fires. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three review-driven fixes on the IV public-API surface: 1. OneSignal.kt: add @JvmStatic wrappers for updateUserJwt, addUserJwtInvalidatedListener, removeUserJwtInvalidatedListener, and updateUserJwtSuspend. Without these, the four new IOneSignal methods were unreachable from app code (the OneSignal object is the documented Java/Kotlin entry point and "implements IOneSignal in spirit"). Mirrors the convention from reference branch #2599. 2. OneSignalImp.kt: mask jwtBearerToken in login()/loginSuspend() DEBUG logs (...${jwtBearerToken?.takeLast(8)}). Pre-PR the parameter was a no-op (LoginHelper had a TODO) so the leak was theoretical; this PR wires the token through to JwtTokenStore.putJwt, so a live bearer credential now flows through Logging.log at DEBUG. updateUserJwt already masks; mirror that. 3. UserManager.kt: restore Logging.warn(msg, ex) form on the two new runCatching.onFailure handlers in fireJwtInvalidated and addJwtInvalidatedListener. Interpolating ${ex.message} drops the stack trace; Logging.warn already accepts a Throwable second arg that propagates through to log listeners and Otel — same pattern restored in JwtTokenStore via e76fb60. Also refreshes detekt baseline for the new MagicNumber (takeLast(8)), ConstructorParameterNaming (_jwtTokenStore on UserManager), TooManyFunctions (UserManager now also implements IJwtUpdateListener), and UseCheckOrError on the 4 new IllegalStateException throws. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…s iOS The buffer-and-consume design (pendingJwtInvalidatedExternalId + jwtInvalidatedLock + consume-on-first-subscribe replay) was added to handle cold-start 401s when no listener was subscribed yet. iOS doesn't do this — it fires only to currently-subscribed listeners, late subscribers miss earlier events. Match iOS: drop the buffer entirely. Reverts the structure introduced by d137481 ("align jwt-invalidated replay with #2613 buffer-and-consume"). The simpler design here matches reference branch #2599. Side effects: - onModelReplaced becomes a no-op (no buffer to clear). Kept the override since ISingletonModelStoreChangeHandler requires it. - fireJwtInvalidated now uses OneSignalDispatchers.launchOnDefault per maintainer request (#3184062053) instead of a custom CoroutineScope(SupervisorJob() + Dispatchers.Default). - KDoc on IOneSignal.addUserJwtInvalidatedListener and the OneSignal.kt facade now spell out the pure-pub/sub semantics: "Subscribe early (e.g. in Application.onCreate) to avoid missing cold-start 401s." Test changes: drop the 4 buffer/replay-specific tests (listener-replay-buffered-event, consume-on-first-subscribe, fire-with-subscribers-no-buffer, onModelReplaced-clears-buffer). Replace with one test that confirms late subscribers don't receive earlier events.
…Store
JwtTokenStore is the source of truth for invalidation events (its
invalidateJwt method is what fires both internal and developer-facing
notifications). It already implements IEventNotifier<IJwtUpdateListener>
and dispatches to internal listeners. Adding a second EventProducer for
the developer-facing IUserJwtInvalidatedListener is consistent with the
class's existing notifier shape — just a different audience.
This eliminates the bridge that lived in UserManager:
- UserManager loses IJwtUpdateListener, _jwtTokenStore ctor param,
jwtInvalidatedNotifier, addJwtInvalidatedListener,
removeJwtInvalidatedListener, fireJwtInvalidated, onJwtInvalidated.
- OneSignalImp.{add,remove}UserJwtInvalidatedListener now route directly
to JwtTokenStore.
Side benefits:
- No eager-construction problem: JwtTokenStore is already pulled in by
OperationRepo (an IStartableService), so the bridge is live before
any 401 can dispatch — no IBootstrapService registration needed for
any class.
- UserManager goes back to being purely about user state.
- Improves on reference branches #2599 / #2613 which kept the bridge in
UserManager and accepted an eager-construction-via-IAM dependency.
Tests:
- Move the 3 JWT-listener tests from UserManagerTests to
JwtTokenStoreTests (invalidation fires listener, late subscriber
doesn't get replay, removeListener stops notifications).
- Drop _jwtTokenStore param from UserManager construction sites.
When jwt_required hydrates true and removeOperationsWithoutExternalId drops the anon ops, surviving LoginUserOperations may still carry existingOnesignalId pointing at the just-dropped anon user (the merge-anon-into-identified link set by LoginHelper). Under IV that link is permanently unresolvable — anon user creation needs a JWT-less call the backend rejects — so canStartExecute=false sticks forever and deadlocks the queue (no other op can dispatch since they all wait on the login to resolve the local onesignal_id). Clear existingOnesignalId on every surviving LoginUserOperation so the executor takes the createUser (upsert) path. Matches the same fix in reference branches #2599 and #2613. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Revert the order swap from dc93995 — that commit reordered switchUserIv to switch users first and then set isDisabledInternally on the new push sub with NO_PROPOGATE, on the bot review's claim that firing an UpdateSubscriptionOperation against the OLD user was a bug. It is not a bug — it is the intentional behavior in reference branches #2599 and #2613: setting the flag on the CURRENT push sub with the default NORMAL tag fires an UpdateSubscriptionOperation that tells the backend "this device is unsubscribing as the user logs out", dispatched with the OLD user's still-valid JWT before the switch. Without this, logout under IV silently leaves the just-departed user's push sub subscribed server-side. The new (anonymous) user's model not carrying isDisabledInternally is fine: anon ops are filtered by hasValidJwtIfRequired (externalId=null under IV-active), so they accumulate but never dispatch. Update the test to verify the corrected order. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LoginHelper.switchUser was setting existingOnesignalId = currentOneSignalId when the prior user was anonymous (currentExternalId == null). Under IV-required that anon user was never created server-side — no JWT — so its onesignalId stays a local id forever. Carrying it as existingOnesignalId on the new LoginUserOperation makes canStartExecute=false stick, deadlocking the queue across logout→login cycles. Skip the merge-link entirely when useIdentityVerification == REQUIRED so the executor takes the createUser (upsert) path. Matches reference branches #2599 and #2613. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add shouldSuppressAnonymousOp from reference branch #2599: any non- LoginUserOperation enqueued without an externalId is dropped at the enqueue boundary when useIdentityVerification == REQUIRED, since it can't authenticate and would otherwise sit in the queue forever blocked by hasValidJwtIfRequired. LoginUserOperation is exempt — it's enqueued intentionally during logout and purged later by removeOperationsWithoutExternalId if needed. Outer-gated on _identityVerificationService.newCodePathsRun so Phase 1 customers stay byte-for-byte on the legacy enqueue path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously the loginUser block left the loading overlay on under the expectation that onUserStateChange → fetchUserDataFromApi() would dismiss it. Under IV, login can fail (bad JWT, anon-purge, etc.) and onUserStateChange may never fire, leaving the overlay up indefinitely. Match #2599 commit 0f1ad82 — dismiss the loader as soon as the SDK call returns. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Description
One Line Summary
Add Identity Verification (JWT-based) support to the Android SDK, gating API operations behind server-issued JWTs when enabled.
Details
Motivation
Identity Verification allows app developers to authenticate users with JWTs before the SDK sends operations to the OneSignal backend. This prevents unauthorized API calls by requiring a valid JWT for all user-scoped operations when the feature is enabled server-side via the
jwt_requiredremote param.Scope
Public API additions:
OneSignal.login(externalId, jwtBearerToken)— accepts an optional JWT on loginOneSignal.updateUserJwt(externalId, token)— supply a fresh JWT when the current one expiresOneSignal.addUserJwtInvalidatedListener(listener)/removeUserJwtInvalidatedListener(listener)— get notified when a JWT is invalidated (e.g. 401 from backend) so the app can provide a new oneIUserJwtInvalidatedListener/UserJwtInvalidatedEvent— listener interface and event classInternal changes:
JwtTokenStore— persistent store mapping externalId → JWT, backed by SharedPreferencesIdentityVerificationService— reacts to thejwt_requiredremote param arriving via config HYDRATE; purges anonymous operations when IV is enabled; wires JWT invalidation callbacksOperationRepo— gates operation execution on valid JWT when IV is enabled (hasValidJwtIfRequired); handlesFAIL_UNAUTHORIZEDby invalidating the JWT, re-queuing operations, and notifying the developer; stampsexternalIdsynchronously at enqueue time to avoid race conditionsLogoutHelper— when IV is enabled, disables the push subscription internally and suppresses the backend login operation (the device-scoped user is local-only)ConfigModelStoreListener/ParamsBackendService— readsjwt_requiredfrom remote params, defaults tofalsewhen absentAuthorization: Bearer <jwt>header to requests when IV is enabledexternalIdto result operations and attach JWT to backend requestsWhat is NOT affected:
jwt_requireddefaults tofalseand no JWT gating occursTesting
Unit testing
WIP Adding more testing
OperationRepoTests— added tests forFAIL_UNAUTHORIZEDhandling (identified user fires handler + retries; anonymous user drops operations), and synchronousexternalIdstamping before async dispatchLoginUserOperationExecutorTests— updated for identity verification parametersLogoutHelperTests— covers IV-enabled logout flowLoginHelperTests— covers IV-enabled login flow with JWTManual testing
Tested with the demo app on an Android emulator (Pixel 7, API 35):
IUserJwtInvalidatedListener, callupdateUserJwt→ operations resumeAffected code checklist
Checklist
Overview
Testing
Final pass