feat(iv): attach JWT in HTTP client and operation executors (4/6)#2626
Open
nan-li wants to merge 2 commits intofeat/iv-queue-runtime-03from
Open
feat(iv): attach JWT in HTTP client and operation executors (4/6)#2626nan-li wants to merge 2 commits intofeat/iv-queue-runtime-03from
nan-li wants to merge 2 commits intofeat/iv-queue-runtime-03from
Conversation
HTTP layer: - OptionalHeaders.jwt; HttpClient sets Authorization: Bearer header when non-null. Defensive redaction of Authorization in logHTTPSent. Backend services (unconditional jwt: String? = null plumbing): - IIdentityBackendService.setAlias / deleteAlias - IUserBackendService.createUser / updateUser / getUser - ISubscriptionBackendService.createSubscription / updateSubscription / deleteSubscription / transferSubscription - ICustomEventBackendService.sendCustomEvent - getIdentityFromSubscription deliberately excluded: endpoint is not allowed when jwt_required=true; LoginUserFromSubscription executor already returns FAIL_NORETRY under IV. Executor gating via extension pattern (matches PR 2625 precedent): - New file ExecutorsIvExtensions.kt exposes resolveIvBackendParams, resolveIvJwt, shouldFailLoginUserFromSubscription. Outer dispatch at each base executor checks IdentityVerificationGates.newCodePathsRun; inner checks ivBehaviorActive to keep Phase 3 users (new code path on, IV behavior off) on legacy alias/jwt values byte-for-byte. - 7 executors dispatch around alias + JWT resolution: LoginUser, Identity, Subscription (Create/Update/Delete/Transfer), UpdateUser, RefreshUser, LoginUserFromSubscription, CustomEvent. - LoginUserFromSubscription short-circuits FAIL_NORETRY when IV active. DI wires automatically via constructor reflection; no module changes. Tests: - ExecutorsIvExtensionsTests: 9 focused tests covering all three phases. - IdentityOperationExecutor: IV-active and Phase 3 integration tests. - HttpClient: Authorization header presence/absence. - Backend service test mocks updated for the new three-arg http calls. - ExecutorMocks gained getJwtTokenStore() helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…UTHORIZED Three bugs surfaced by bot review on PR 2626: 1. HttpClient: set all optional headers (cacheKey, rywToken, retryCount, sessionDuration, jwt) BEFORE the body write. On real Android, `getOutputStream()` triggers `connect()`, after which `setRequestProperty` throws or is silently dropped. Pre-PR this was latent because OptionalHeaders was only passed on GET. This PR wires it through POST/PATCH/DELETE, activating the bug for every authenticated call. Matches the fix from old IV branch commit 1dab8e0. MockHttpURLConnection now simulates the real contract: `connect`, `getOutputStream`, and `getResponseCode` flip `headersCommitted=true`, and `setRequestProperty` throws after that. A new HttpClientTests case (POST + JWT) now regression-guards the ordering fix. 2. LoginUserOperationExecutor: under `ivBehaviorActive`, skip the optimistic inline SetAliasOperation and go straight to createUser. The inline op identifies the target by `onesignal_id = existingOnesignalId`, but IV's alias resolution rewrites the call to `external_id = newExternalId` — targeting a user that doesn't exist yet. 404 -> FAIL_NORETRY -> fallthrough to createUser orphans the anonymous user; or a 200 idempotent-PATCH against a different pre-existing user corrupts local identity. createUser's upsert semantics handle the merge correctly via the identities map. 3. SubscriptionOperationExecutor: add UNAUTHORIZED -> FAIL_UNAUTHORIZED to update/delete/transfer catch blocks. `createSubscription` handles it correctly; the other three fall through to `else -> FAIL_NORETRY` which swallows 401s without invalidating the stored JWT or firing `jwtInvalidatedHandler`. Pre-existing asymmetry that this PR activates by wiring JWT into these endpoints. Tests: - HttpClientTests: POST + JWT doesn't throw (ordering enforced by mock). - LoginUserOperationExecutorTests: IV active + existingOnesignalId + externalId -> createUser; IdentityOperationExecutor not invoked. - SubscriptionOperationExecutorTests: update/delete/transfer each return FAIL_UNAUTHORIZED on 401 under IV. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@claude review |
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.
Fourth of six PRs re-implementing Identity Verification (IV) on the
feat/identity_verification_feature_flagged_major_releaseintegration branch.Stack: #2623 → #2624 → #2625 → this → (5/6) → (6/6)
What this PR does
Plumbs the JWT all the way from storage to the HTTP wire, and wraps all seven user-facing operation executors in the new-code-path extension pattern established in #2625.
HTTP layer (unconditional plumbing)
OptionalHeaders.jwt: String?field.HttpClientsetsAuthorization: Bearer <jwt>when non-null.logHTTPSentredactsAuthorizationdefensively (the log call currently happens before the header is set, but protecting against future reordering).Backend service signatures
jwt: String? = nulldefault param added to 10 methods across 4 services:IIdentityBackendService.setAlias/deleteAliasIUserBackendService.createUser/updateUser/getUserISubscriptionBackendService.createSubscription/updateSubscription/deleteSubscription/transferSubscriptionICustomEventBackendService.sendCustomEventjwtintoOptionalHeaders. Existing callers unchanged.getIdentityFromSubscription. That endpoint is not allowed whenjwt_required=true;LoginUserFromSubscriptionOperationExecutoralready returnsFAIL_NORETRYunder IV, so the call never reaches the backend service on the IV path. KDoc added to document this.Executor gating — extension pattern
New file
ExecutorsIvExtensions.ktexposes:IvBackendParams(aliasLabel, aliasValue, jwt)withlegacyFor(onesignalId)factory.resolveIvBackendParams(op, onesignalId, jwtTokenStore)— alias + JWT for backend calls.resolveIvJwt(op, jwtTokenStore)— JWT-only for calls that don't take alias label/value.shouldFailLoginUserFromSubscription()— IV-active predicate.Each helper short-circuits on
IdentityVerificationGates.ivBehaviorActive == false. Base-class dispatch sites gate onIdentityVerificationGates.newCodePathsRun, so:newCodePathsRun=false): extensions never called; executors byte-for-byte identical to today.newCodePathsRun=true,ivBehaviorActive=false): extensions run and return legacy values; new code path exercised without IV behavior — validates structural changes.external_id, JWT attaches.Seven executors wired:
LoginUserOperationExecutorresolveIvJwt(createUser uses identities map, no alias)IdentityOperationExecutorresolveIvBackendParams(setAlias / deleteAlias)SubscriptionOperationExecutor.createSubscriptionresolveIvBackendParamsSubscriptionOperationExecutor.updateSubscription/deleteSubscriptionresolveIvJwtSubscriptionOperationExecutor.transferSubscriptionresolveIvBackendParamsUpdateUserOperationExecutorresolveIvBackendParams(externalId pulled fromoperations.first(); grouped ops share user)RefreshUserOperationExecutorresolveIvBackendParamsLoginUserFromSubscriptionOperationExecutorshouldFailLoginUserFromSubscription→FAIL_NORETRYCustomEventOperationExecutorresolveIvJwtDI auto-wires
JwtTokenStoreinto executors through constructor reflection — no module changes needed (store was registered in #2623).Tests
ExecutorsIvExtensionsTests— 9 focused unit tests across Phase 1 / Phase 3 / IV-active for bothresolveIvBackendParamsandresolveIvJwt, plusshouldFailLoginUserFromSubscription.HttpClientTestscases — Authorization header set iffOptionalHeaders.jwt != null.IdentityOperationExecutorTestscases — IV-active and Phase 3 end-to-end verifies that the correct alias + JWT reach the backend mock.ExecutorMocks.getJwtTokenStore()helper (real emptyJwtTokenStorebacked byMockPreferencesService).Test plan
./gradlew :OneSignal:core:testDebugUnitTest— 814 pass / 2 fail (both pre-existingSDKInitTestsfailures, unrelated; reproduced on baseline).:OneSignal:in-app-messages,:OneSignal:notifications,:OneSignal:locationall compile clean.Authorization: Bearer <jwt>is present on network requests when a customer hasjwt_required=true(will be testable once PR 5 wires the publiclogin(externalId, jwt)API).Out of scope (deferred to PR 5)
InAppBackendServiceJWT integrationCreateUserResponse/JSONConverterIJwtUpdateListenerJwtTokenStore.pruneToExternalIdscaller (logout path)🤖 Generated with Claude Code