-
Notifications
You must be signed in to change notification settings - Fork 378
fix(jwt): race conditions and IAM reliability issues with identity verification enabled #2609
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
b6a57f4
7ce86bb
77a2d3a
2784adf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,7 +68,7 @@ | |
| <ID>LongMethod:InAppRepository.kt$InAppRepository$override suspend fun cleanCachedInAppMessages()</ID> | ||
| <ID>LongParameterList:IInAppBackendService.kt$IInAppBackendService$( appId: String, subscriptionId: String, variantId: String?, messageId: String, clickId: String?, isFirstClick: Boolean, )</ID> | ||
| <ID>LongParameterList:InAppDisplayer.kt$InAppDisplayer$( private val _applicationService: IApplicationService, private val _lifecycle: IInAppLifecycleService, private val _promptFactory: IInAppMessagePromptFactory, private val _backend: IInAppBackendService, private val _influenceManager: IInfluenceManager, private val _configModelStore: ConfigModelStore, private val _languageContext: ILanguageContext, private val _time: ITime, )</ID> | ||
| <ID>LongParameterList:InAppMessagesManager.kt$InAppMessagesManager$( private val _applicationService: IApplicationService, private val _sessionService: ISessionService, private val _influenceManager: IInfluenceManager, private val _configModelStore: ConfigModelStore, private val _userManager: IUserManager, private val _identityModelStore: IdentityModelStore, private val _subscriptionManager: ISubscriptionManager, private val _outcomeEventsController: IOutcomeEventsController, private val _state: InAppStateService, private val _prefs: IInAppPreferencesController, private val _repository: IInAppRepository, private val _backend: IInAppBackendService, private val _triggerController: ITriggerController, private val _triggerModelStore: TriggerModelStore, private val _displayer: IInAppDisplayer, private val _lifecycle: IInAppLifecycleService, private val _languageContext: ILanguageContext, private val _time: ITime, private val _consistencyManager: IConsistencyManager, )</ID> | ||
| <ID>LongParameterList:InAppMessagesManager.kt$InAppMessagesManager$( private val _applicationService: IApplicationService, private val _sessionService: ISessionService, private val _influenceManager: IInfluenceManager, private val _configModelStore: ConfigModelStore, private val _userManager: IUserManager, private val _identityModelStore: IdentityModelStore, private val _subscriptionManager: ISubscriptionManager, private val _outcomeEventsController: IOutcomeEventsController, private val _state: InAppStateService, private val _prefs: IInAppPreferencesController, private val _repository: IInAppRepository, private val _backend: IInAppBackendService, private val _triggerController: ITriggerController, private val _triggerModelStore: TriggerModelStore, private val _displayer: IInAppDisplayer, private val _lifecycle: IInAppLifecycleService, private val _languageContext: ILanguageContext, private val _time: ITime, private val _consistencyManager: IConsistencyManager, private val _jwtTokenStore: JwtTokenStore, )</ID> | ||
| <ID>LongParameterList:OneSignalAnimate.kt$OneSignalAnimate$( view: View, deltaFromY: Float, deltaToY: Float, duration: Int, interpolator: Interpolator?, animCallback: Animation.AnimationListener?, )</ID> | ||
| <ID>MagicNumber:DraggableRelativeLayout.kt$DraggableRelativeLayout$3</ID> | ||
| <ID>MagicNumber:DraggableRelativeLayout.kt$DraggableRelativeLayout$3000</ID> | ||
|
Comment on lines
68
to
74
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The PR adds _jwtTokenStore: JwtTokenStore as a new constructor parameter to InAppMessagesManager and correctly updates the LongParameterList baseline entry, but omits the corresponding ConstructorParameterNaming baseline entry. All 19 other underscore-prefixed constructor parameters have their own ConstructorParameterNaming suppression entries; _jwtTokenStore is the sole omission. Add the missing entry: ConstructorParameterNaming:InAppMessagesManager.kt$InAppMessagesManager$private val _jwtTokenStore: JwtTokenStore. Extended reasoning...What the bug is The PR introduces The specific code path that triggers it When detekt runs in CI, it scans Why existing code does not prevent it The PR author correctly updated the What the impact would be The detekt CI check will fail when the PR is merged, blocking the release build. No functional behavior is affected; this is purely a build-tooling issue. How to fix it Add the following entry to the Step-by-step proof
|
||
|
|
@@ -103,12 +103,12 @@ | |
| <ID>ReturnCount:DraggableRelativeLayout.kt$DraggableRelativeLayout.<no name provided>$override fun clampViewPositionVertical( child: View, top: Int, dy: Int, ): Int</ID> | ||
| <ID>ReturnCount:DynamicTriggerController.kt$DynamicTriggerController$fun dynamicTriggerShouldFire(trigger: Trigger): Boolean</ID> | ||
| <ID>ReturnCount:InAppBackendService.kt$InAppBackendService$override suspend fun getIAMData( appId: String, messageId: String, variantId: String?, ): GetIAMDataResponse</ID> | ||
| <ID>ReturnCount:InAppBackendService.kt$InAppBackendService$private suspend fun attemptFetchWithRetries( baseUrl: String, rywData: RywData, sessionDurationProvider: () -> Long, ): List<InAppMessage>?</ID> | ||
| <ID>ReturnCount:InAppBackendService.kt$InAppBackendService$private suspend fun attemptFetchWithRetries( baseUrl: String, rywData: RywData, sessionDurationProvider: () -> Long, jwt: String? = null, ): List<InAppMessage>?</ID> | ||
| <ID>ReturnCount:InAppHydrator.kt$InAppHydrator$fun hydrateIAMMessageContent(jsonObject: JSONObject): InAppMessageContent?</ID> | ||
| <ID>ReturnCount:InAppMessage.kt$InAppMessage$private fun parseEndTimeJson(json: JSONObject): Date?</ID> | ||
| <ID>ReturnCount:InAppMessagePreviewHandler.kt$InAppMessagePreviewHandler$private fun inAppPreviewPushUUID(payload: JSONObject): String?</ID> | ||
| <ID>ReturnCount:InAppMessagesManager.kt$InAppMessagesManager$override fun onMessageWasDisplayed(message: InAppMessage)</ID> | ||
| <ID>ReturnCount:InAppMessagesManager.kt$InAppMessagesManager$private suspend fun fetchMessages(rywData: RywData)</ID> | ||
| <ID>ReturnCount:InAppMessagesManager.kt$InAppMessagesManager$private suspend fun fetchMessages(rywData: RywData?)</ID> | ||
| <ID>ReturnCount:TriggerController.kt$TriggerController$override fun evaluateMessageTriggers(message: InAppMessage): Boolean</ID> | ||
| <ID>ReturnCount:TriggerController.kt$TriggerController$override fun isTriggerOnMessage( message: InAppMessage, triggersKeys: Collection<String>, ): Boolean</ID> | ||
| <ID>ReturnCount:TriggerController.kt$TriggerController$override fun messageHasOnlyDynamicTriggers(message: InAppMessage): Boolean</ID> | ||
|
|
@@ -124,7 +124,7 @@ | |
| <ID>TooManyFunctions:InAppBackendService.kt$InAppBackendService : IInAppBackendService</ID> | ||
| <ID>TooManyFunctions:InAppMessage.kt$InAppMessage : IInAppMessage</ID> | ||
| <ID>TooManyFunctions:InAppMessageView.kt$InAppMessageView</ID> | ||
| <ID>TooManyFunctions:InAppMessagesManager.kt$InAppMessagesManager : IInAppMessagesManagerIStartableServiceISubscriptionChangedHandlerISingletonModelStoreChangeHandlerIInAppLifecycleEventHandlerITriggerHandlerISessionLifecycleHandlerIApplicationLifecycleHandler</ID> | ||
| <ID>TooManyFunctions:InAppMessagesManager.kt$InAppMessagesManager : IInAppMessagesManagerIStartableServiceISubscriptionChangedHandlerISingletonModelStoreChangeHandlerIInAppLifecycleEventHandlerITriggerHandlerISessionLifecycleHandlerIApplicationLifecycleHandlerIJwtUpdateListener</ID> | ||
| <ID>TooManyFunctions:TriggerController.kt$TriggerController : ITriggerControllerIModelStoreChangeHandler</ID> | ||
| <ID>TooManyFunctions:WebViewManager.kt$WebViewManager : IActivityLifecycleHandler</ID> | ||
| <ID>UndocumentedPublicClass:TriggerModel.kt$TriggerModel : Model</ID> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ package com.onesignal.core.internal.config.impl | |
| import com.onesignal.common.modeling.ISingletonModelStoreChangeHandler | ||
| import com.onesignal.common.modeling.ModelChangeTags | ||
| import com.onesignal.common.modeling.ModelChangedArgs | ||
| import com.onesignal.common.threading.suspendifyOnIO | ||
| import com.onesignal.core.internal.config.ConfigModel | ||
| import com.onesignal.core.internal.config.ConfigModelStore | ||
| import com.onesignal.core.internal.operations.IOperationRepo | ||
|
|
@@ -42,21 +43,25 @@ internal class IdentityVerificationService( | |
|
|
||
| val useIV = model.useIdentityVerification | ||
|
|
||
| var jwtInvalidatedExternalId: String? = null | ||
| if (useIV == true) { | ||
| Logging.debug("IdentityVerificationService: IV enabled, purging anonymous operations") | ||
| _operationRepo.removeOperationsWithoutExternalId() | ||
| suspendifyOnIO { | ||
| _operationRepo.awaitInitialized() | ||
|
|
||
| val externalId = _identityModelStore.model.externalId | ||
| if (externalId != null && _jwtTokenStore.getJwt(externalId) == null) { | ||
| Logging.debug("IdentityVerificationService: IV enabled but no JWT for $externalId, will fire invalidated event after queue wake") | ||
| jwtInvalidatedExternalId = externalId | ||
| var jwtInvalidatedExternalId: String? = null | ||
| if (useIV == true) { | ||
| Logging.debug("IdentityVerificationService: IV enabled, purging anonymous operations") | ||
| _operationRepo.removeOperationsWithoutExternalId() | ||
|
|
||
| val externalId = _identityModelStore.model.externalId | ||
| if (externalId != null && _jwtTokenStore.getJwt(externalId) == null) { | ||
| Logging.debug("IdentityVerificationService: IV enabled but no JWT for $externalId, will fire invalidated event after queue wake") | ||
| jwtInvalidatedExternalId = externalId | ||
| } | ||
| } | ||
| } | ||
|
|
||
| _operationRepo.forceExecuteOperations() | ||
| _operationRepo.forceExecuteOperations() | ||
|
|
||
| jwtInvalidatedExternalId?.let { _userManager.fireJwtInvalidated(it) } | ||
| jwtInvalidatedExternalId?.let { _userManager.fireJwtInvalidated(it) } | ||
| } | ||
|
Comment on lines
44
to
+64
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 In the PR's refactored Extended reasoning...What the bug is and how it manifests The PR wraps the entire HYDRATE handler body in The specific code path that triggers it
Why existing code does not prevent it Capturing What the impact would be Two failure modes: (1) If userB has a valid JWT, How to fix it Capture val useIV = model.useIdentityVerification
val externalId = _identityModelStore.model.externalId // capture before coroutine
suspendifyOnIO {
_operationRepo.awaitInitialized()
if (useIV == true) {
_operationRepo.removeOperationsWithoutExternalId()
if (externalId \!= null && _jwtTokenStore.getJwt(externalId) == null) {
jwtInvalidatedExternalId = externalId
}
}
...
}Step-by-step proof
|
||
| } | ||
|
|
||
| override fun onModelUpdated( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 The PR correctly updates the
LongParameterListbaseline entry forLoginUserOperationExecutorto include_consistencyManager: IConsistencyManager, but omits the requiredConstructorParameterNamingsuppression entry for that parameter. All 10 other underscore-prefixed constructor parameters inLoginUserOperationExecutorhave individualConstructorParameterNamingentries, and both peer executors (SubscriptionOperationExecutor,UpdateUserOperationExecutor) have the analogous entry for their own_consistencyManagerparameters. Add the missing baseline entry to prevent the detekt CI check from failing.Extended reasoning...
What the bug is and how it manifests
The detekt static-analysis tool enforces a
ConstructorParameterNamingrule that flags underscore-prefixed constructor parameters (e.g.private val _consistencyManager). These violations are suppressed by adding per-parameter entries to the detekt baseline XML file. This PR addsprivate val _consistencyManager: IConsistencyManageras a new constructor parameter toLoginUserOperationExecutorand correctly updates theLongParameterListbaseline entry to reflect the new parameter, but does not add the correspondingConstructorParameterNamingentry. When detekt runs in CI baseline-check mode, any violation not present in the baseline XML is treated as a new unbaselined finding and fails the build.The specific code path that triggers it
When the CI pipeline runs
./gradlew detekt(or the equivalent baseline-check task) againstLoginUserOperationExecutor.kt, detekt scans the constructor and findsprivate val _consistencyManager: IConsistencyManager. It raises aConstructorParameterNamingviolation. The baseline checker compares this againstdetekt-baseline-core.xmland finds no matching suppression entry, so it reports one new unbaselined violation and returns a non-zero exit code.Why existing code does not prevent it
The
LongParameterListandConstructorParameterNamingrules are tracked independently in the XML, under separate<ID>entries. Updating theLongParameterListentry (which lists all parameters in a single string) has no effect on theConstructorParameterNamingrule, which requires one entry per parameter. The two-rule pattern is well-established in this codebase: every other underscore-prefixed parameter inLoginUserOperationExecutor(_application,_configModelStore,_deviceService,_identityModelStore,_identityOperationExecutor,_jwtTokenStore,_languageContext,_propertiesModelStore,_subscriptionsModelStore,_userBackend) has its ownConstructorParameterNamingentry. The omission of_consistencyManageris an oversight.What the impact would be
The detekt CI check will fail after this PR is merged, blocking the release build pipeline. There is no functional impact on SDK behavior; this is purely a build-tooling issue. The build failure is deterministic and will reproduce on every CI run until the entry is added.
How to fix it
Add the following entry to the
<CurrentIssues>section ofOneSignalSDK/detekt/detekt-baseline-core.xml, in alphabetical order among the otherConstructorParameterNaming:LoginUserOperationExecutorentries:Step-by-step proof
private val _consistencyManager: IConsistencyManagertoLoginUserOperationExecutor's constructor.LongParameterListbaseline entry is updated to include_consistencyManager— this is correct.ConstructorParameterNamingon the constructor; all 10 existing underscore-prefixed parameters are suppressed via baseline entries._consistencyManagerwith no matchingConstructorParameterNamingbaseline entry.SubscriptionOperationExecutorandUpdateUserOperationExecutorboth already haveConstructorParameterNamingentries for their_consistencyManagerparameters, confirming the expected pattern.