Conversation
4509a8f to
457b745
Compare
...DK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt
Show resolved
Hide resolved
OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/LogoutHelper.kt
Show resolved
Hide resolved
OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/identity/JwtTokenStore.kt
Show resolved
Hide resolved
...DK/onesignal/core/src/main/java/com/onesignal/core/internal/operations/impl/OperationRepo.kt
Show resolved
Hide resolved
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
examples/demo/app/src/main/java/com/onesignal/sdktest/ui/main/MainViewModel.kt
Show resolved
Hide resolved
examples/demo/app/src/main/java/com/onesignal/sdktest/ui/main/MainViewModel.kt
Show resolved
Hide resolved
| <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.
...al/core/src/main/java/com/onesignal/core/internal/config/impl/IdentityVerificationService.kt
Show resolved
Hide resolved
...DK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserOperation.kt
Show resolved
Hide resolved
OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/LogoutHelper.kt
Show resolved
Hide resolved
OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/UserManager.kt
Outdated
Show resolved
Hide resolved
OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/UserModule.kt
Outdated
Show resolved
Hide resolved
| * @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
…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
…ion Bearer header to HttpClient Identity verification: plumb JWT through the HTTP and backend layer. - HttpClient: set Authorization: Bearer header when OptionalHeaders.jwt is non-null - IIdentityBackendService + impl: add jwt param to setAlias, deleteAlias - ISubscriptionBackendService + impl: add jwt param to createSubscription, updateSubscription, deleteSubscription, transferSubscription, getIdentityFromSubscription - IUserBackendService + impl: add jwt param to createUser, updateUser, getUser - All jwt params default to null so existing callers are unaffected
…D handling to OperationRepo Identity verification: OperationRepo becomes JWT-aware. - Add JwtTokenStore and IdentityModelStore as constructor dependencies - Centralized externalId stamping: internalEnqueue() auto-sets op.externalId from the current identity model for new operations (not loaded from persistence, not already set by the operation's constructor) - IV gating in getNextOps(): when IV=null (unknown), hold ALL operations; when IV=true, skip operations without a valid JWT; when IV=false, proceed normally - FAIL_UNAUTHORIZED: invalidate the per-user JWT in JwtTokenStore and re-queue operations to front (held by JWT gating until a new JWT is provided) - Cold-start cleanup: prune JwtTokenStore to externalIds from pending operations plus the current identity model's externalId - New removeOperationsWithoutExternalId() method on IOperationRepo for IdentityVerificationService to purge anonymous operations when IV is enabled
…tity verification - Add resolveIdentityAlias() helper to dynamically choose external_id vs onesignal_id for API paths - Each executor now looks up JWT from JwtTokenStore using operation's externalId and passes it to backend calls - LoginUserFromSubscriptionOperationExecutor returns FAIL_NORETRY when IV is enabled (v4 migration safety net) Made-with: Cursor
- Add jwt param to IInAppBackendService.listInAppMessages and pass through OptionalHeaders - Skip IAM fetch when identity verification is enabled and user is anonymous - Look up JWT from JwtTokenStore for authenticated IAM requests Made-with: Cursor
…subscriptions/:subscription_id/iams Made-with: Cursor
… and subscription listeners - LoginHelper: store JWT unconditionally on login, handle same-externalId re-login (store + forceExecute), set existingOneSignalId to null when IV=ON - LogoutHelper: IV=ON branch opts out push before user switch so backend is notified, then creates local-only anonymous user with suppressBackendOperation - UserManager: add jwtInvalidatedNotifier EventProducer for JWT callbacks - OneSignalImp: wire updateUserJwt, addUserJwtInvalidatedListener, removeUserJwtInvalidatedListener; pass JwtTokenStore/SubscriptionModelStore to LoginHelper/LogoutHelper - SubscriptionModelStoreListener, IdentityModelStoreListener, PropertiesModelStoreListener: suppress ops for anonymous users when IV=ON Made-with: Cursor
- New IdentityVerificationService: listens for config HYDRATE events, purges anonymous operations when IV=true, wakes OperationRepo when IV resolves from null, fires UserJwtInvalidatedEvent for beta migration (externalId present but no JWT) - CoreModule: register JwtTokenStore and IdentityVerificationService - ParamsBackendService: remove leftover TODO comments Made-with: Cursor
Register for JWT invalidation events, log a warning and show a toast when triggered.
Optional headers (ETag, RYW-Token, Retry-Count, Session-Duration, Authorization) were set after the body write, which opens the connection. This caused IllegalStateException on POST requests with a JWT. Move all setRequestProperty calls before outputStream write to prevent the error. Made-with: Cursor
Cache the JWT token on login and updateUserJwt calls. When Identity Verification is enabled, include the Authorization Bearer header in the demo app's fetch user request.
When Identity Verification is ON and logout is called, the SDK now sets isDisabledInternally=true instead of optedIn=false. This preserves the real opt-in preference while telling the backend the subscription is disabled. On the next login, UserSwitcher creates a fresh SubscriptionModel that defaults isDisabledInternally=false, restoring the real state automatically. Made-with: Cursor
Add addJwtInvalidatedListener, removeJwtInvalidatedListener, and fireJwtInvalidated methods to UserManager. Callers no longer need to cast IUserManager to UserManager to access the notifier directly. Register UserManager as a concrete DI service so IdentityVerificationService can depend on it directly. Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
When OperationRepo handled FAIL_UNAUTHORIZED it invalidated the JWT and re-queued operations but never fired IUserJwtInvalidatedListener, leaving the queue permanently stuck. Wire a callback from OperationRepo through IdentityVerificationService to UserManager.fireJwtInvalidated() so the developer is notified and can supply a fresh token. Made-with: Cursor
Follow-up operations returned by executors (e.g. RefreshUserOperation) were inserted directly into the queue without externalId, causing them to be permanently blocked when identity verification is enabled. Made-with: Cursor
The externalId was being stamped inside internalEnqueue which runs on the OperationRepo coroutine thread. When LogoutHelper set isDisabledInternally=true (triggering an UpdateSubscriptionOperation) then immediately called createAndSwitchToNewUser(), the identity model's externalId could be cleared before the coroutine ran, leaving the operation with externalId=null and permanently blocked. Move stamping to enqueue()/enqueueAndWait() on the caller's thread so the externalId is captured at the moment the operation is created. Made-with: Cursor
Wrap JSONObject parsing in ensureLoaded() with try-catch so that corrupted persisted data (e.g. from a process kill mid-write) does not permanently break JWT lookups and stall the operation queue. Made-with: Cursor
When the server omits jwt_required from the params response, safeBool() returned null, leaving useIdentityVerification unset. getNextOps() treated null as "not yet known" and returned null permanently, silently blocking every SDK operation for the session. Default to false when the field is absent so non-IV apps are unaffected by the identity verification gating logic. Made-with: Cursor
The field is written from the main thread in IdentityVerificationService.start() and read on the OperationRepo coroutine thread. @volatile guarantees cross-thread visibility. Made-with: Cursor
When IV is enabled and the JWT has already been invalidated (e.g. by a prior 401), the UpdateSubscriptionOperation to disable the push subscription would be permanently blocked by hasValidJwtIfRequired. Since the backend call would fail with 401 anyway, skip it entirely and just switch to the new anonymous user. Made-with: Cursor Revert "Skip push subscription disable on logout when JWT is already expired" This reverts commit 5ce284257b6e03b8c862745ff58fcf4293299577. Exempt UpdateSubscriptionOperation from JWT gating The subscription update endpoint does not require a JWT on the backend. Add Operation.requiresJwt (default true) and override it to false in UpdateSubscriptionOperation so these operations are not blocked by hasValidJwtIfRequired when the JWT is expired or missing. This fixes the edge case where logging out with an expired JWT would permanently block the push subscription disable operation. Made-with: Cursor
Made-with: Cursor
…ationRepo Pass externalId alongside onesignalId in every operation constructor, eliminating the fragile stampExternalId workaround and parentExternalId propagation from OperationRepo. Every creation site (model store listeners, session listener, executors, etc.) now explicitly provides the externalId at construction time. Made-with: Cursor
Add suspend variant of updateUserJwt for API consistency with login/loginSuspend. Also add waitForInit to the non-suspend updateUserJwt so it safely handles calls before SDK initialization.
b9b637d to
16ce7f6
Compare
- Baseline 2 new UseCheckOrError entries for OneSignalImp.kt for updateUserJwt/updateUserJwtSuspend init guards that follow the existing login/logout pattern. - Baseline ReturnCount for hasValidJwtIfRequired — the guard-clause style is clearer than collapsing into a single boolean expression for this JWT gating logic. - Narrow JwtTokenStore catch from Exception to JSONException to satisfy detekt's TooGenericExceptionCaught rule, no baseline added.
…r params All new _prefix constructor parameters (e.g. _jwtTokenStore, _identityModelStore, _configModelStore) follow the existing codebase convention — 80+ identical entries already exist in the baseline. These are DI-injected dependencies across IdentityVerificationService, OperationRepo, JwtTokenStore, and various executors/listeners.
- Update 4 existing baseline entries (added jwt/_jwtTokenStore to signatures) - Baseline 4 new LongParameterList entries for IV constructors: CreateSubscriptionOperation, UpdateSubscriptionOperation, RefreshUserOperationExecutor, and UpdateUserOperationExecutor crossed the constructor threshold (8) after adding externalId and _jwtTokenStore params. This follows existing patterns.
1de75fa to
cdc61b8
Compare
OperationRepo: on FAIL_UNAUTHORIZED, invalidate JWT and re-queue before notifying the app; invoke setJwtInvalidatedHandler synchronously with try/catch so listener failures do not hit executeOperations outer catch. UserManager.fireJwtInvalidated performs the single async hop (SupervisorJob + Dispatchers.Default) with Otel-style launch try/catch and per-listener isolation. IdentityVerificationService: call forceExecuteOperations before fireJwtInvalidated on HYDRATE. Documentation: IOperationRepo handler runs on the op-repo thread and must return quickly; public API documents background-thread delivery for IUserJwtInvalidatedListener, addUserJwtInvalidatedListener, and UserJwtInvalidatedEvent (LiveData postValue guidance). Tests: FAIL_UNAUTHORIZED JWT handler coverage; no artificial delay after sync repo handler.
ca64cdf to
258c8e6
Compare
…2600) Co-authored-by: AR Abdul Azeez <abdul@onesignal.com>
abdulraqeeb33
left a comment
There was a problem hiding this comment.
Lets try and do a bug bash on this today/tomorrow?
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