Skip to content

feat(iv): foundation: gates singleton, JWT token store, feature flag entry (1/6)#2623

Open
nan-li wants to merge 5 commits intofeat/identity_verification_feature_flagged_major_releasefrom
feat/iv-foundation-01
Open

feat(iv): foundation: gates singleton, JWT token store, feature flag entry (1/6)#2623
nan-li wants to merge 5 commits intofeat/identity_verification_feature_flagged_major_releasefrom
feat/iv-foundation-01

Conversation

@nan-li
Copy link
Copy Markdown
Contributor

@nan-li nan-li commented Apr 24, 2026

Description

One Line Summary

First of five PRs against the Identity Verification integration branch — lands foundation primitives (IdentityVerificationGates singleton, JwtTokenStore, FeatureFlag.IDENTITY_VERIFICATION entry) with no consumer wiring or public API yet.

Details

Motivation

IV is gated behind two nested conditions:

  • SDK-side rollout switchFeatureFlag.IDENTITY_VERIFICATION controlled via Turbine.
  • Customer configjwt_required remote param from the backend.

Post-release rollout strategy: ship with feature flag OFF; customers with jwt_required=true get IV via the customer-config override; progressively flip the feature flag ON for remaining customers to validate structural changes independent of IV activation.

Scope

All additions are inert at runtime — nothing consumes the gates or the JWT store yet. Subsequent PRs wire them in:

  • PR 2 - 3OperationRepo IV gating, pre-HYDRATE deferral, anon-op purge race fix.
  • PR 4 — HTTP Authorization header + executor JWT integration.
  • PR 5 — Public API (login(externalId, jwt), updateUserJwt, listeners) + replay.
  • PR 6 — Logout + IAM integration + RYW plumbing.

What lands in this PR:

New files:

  • IdentityVerificationGates.kt — singleton object mirroring ThreadingMode. Volatile newCodePathsRun (= featureFlag || jwtRequired == REQUIRED) and ivBehaviorActive (= jwtRequired == REQUIRED). The two gates differ on purpose: customer config must be honored even when the feature flag is off, otherwise a kill-switch flip would break customers who've already enabled IV server-side.
  • JwtRequirement.kt — tri-state enum (UNKNOWN / NOT_REQUIRED / REQUIRED) for ConfigModel.useIdentityVerification, replacing the original Boolean?. Explicit about pre-HYDRATE; visually distinct from FeatureFlag.IDENTITY_VERIFICATION.
  • JwtTokenStore.ktSharedPreferences-backed externalId → JWT map. Multi-user so ops queued under a previous user can still resolve their JWT at execution time. Storage is unconditional; usage is gated on IdentityVerificationGates.ivBehaviorActive.
  • IJwtUpdateListener.kt — pure wake-up notification (onJwtUpdated(externalId) with no payload). Subscribers must re-read getJwt(externalId); matches the ModelStore convention and avoids stale-ordering hazards when multiple threads write.

Modified files:

  • FeatureFlag.kt — added IDENTITY_VERIFICATION("identity_verification", IMMEDIATE). IMMEDIATE because a Turbine kill-switch should take effect within a foreground-poll cycle rather than wait for a cold start, and because the jwt_required side of the gate is already live-updating via HYDRATE — keeping both inputs symmetric.
  • FeatureManager.ktapplySideEffects now has an IDENTITY_VERIFICATION branch pushing both into IdentityVerificationGates.
  • ConfigModel.ktuseIdentityVerification type changed to JwtRequirement (internal property because the enum is internal). Backing storage is unchanged (still a nullable boolean), so no cache migration needed.
  • IPreferencesService.kt — added PREFS_OS_JWT_TOKENS key.
  • CoreModule.kt — registered JwtTokenStore in DI.

Gate-access pattern: single-tier. Consumers read IdentityVerificationGates.newCodePathsRun / .ivBehaviorActive directly. Pre-init the volatile fields default to false (safe fallback). Post-init they're populated by FeatureManager.init → refreshEnabledFeatures → applySideEffects, and refreshed on every HYDRATE because IDENTITY_VERIFICATION is IMMEDIATE.

IdentityVerificationService is NOT in this PR — moves to PR 2 where its HYDRATE handler has real work (anon-op purge, queue wake). The IMMEDIATE activation mode means FeatureManager.applySideEffects refreshes the gates on every HYDRATE without needing a separate listener in PR 1.

Testing

Unit testing

  • IdentityVerificationGatesTests — 9 tests covering the full gate matrix including the ERROR STATE row (feature flag off, jwt_required=REQUIRED → customer config wins).
  • JwtTokenStoreTests — 17 tests: put/get/invalidate/prune, persistence round-trip, listener notifications (fire on change, no-op on no-change, fire on invalidation/prune), malformed JSON recovery.
  • FeatureManagerTests — added IV coverage: flag-off baseline, flag-on newCodePathsRun, ERROR STATE row, full IV, HYDRATE propagation, IMMEDIATE mid-session flip.
  • FeatureFlagTests — key and activation-mode assertions for IDENTITY_VERIFICATION.
  • stubConfigModel helper updated to stub useIdentityVerification with JwtRequirement.UNKNOWN by default.

Manual testing

Not applicable — no runtime behavior change in this PR. All new code is unreferenced by consumers.

Affected code checklist

  • Notifications
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing — lands IV foundation primitives only
  • Any Public API changes are explained in the PR details and conform to existing APIs (none in this PR)

Testing

  • I have included test coverage for these changes
  • All automated tests pass locally (pre-existing SDKInitTests failures on the integration branch are unrelated)
  • Manual testing is not applicable — no runtime behavior change

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

…entry

First of set of PRs against the IV integration branch. Lands only the foundation
primitives; no consumer wiring or public API yet.

- Add `FeatureFlag.IDENTITY_VERIFICATION` (IMMEDIATE) so a Turbine kill-switch
  takes effect without a cold start.
- Add `IdentityVerificationGates` singleton (mirrors `ThreadingMode`) holding
  `newCodePathsRun` (featureFlag || jwt_required) and `ivBehaviorActive`
  (jwt_required alone). `FeatureManager.applySideEffects` pushes both values
  in on every refresh, reading from the passed `model` parameter to stay in
  sync with the HYDRATE snapshot.
- Add `JwtTokenStore` (SharedPreferences-backed externalId -> JWT) with an
  `EventProducer<IJwtUpdateListener>` for PR 5's IAM retry to subscribe to.
- Change `ConfigModel.useIdentityVerification` from `Boolean` to `Boolean?`;
  `null` means pre-HYDRATE so PR 2's `getNextOps` deferral can distinguish
  it from an explicit `false`.
- Add `PREFS_OS_JWT_TOKENS` key.
- Register `JwtTokenStore` in DI. `IdentityVerificationService` is deferred
  to PR 2 where its HYDRATE handler has real work.

Tests cover the gate matrix (including the ERROR STATE row where
jwt_required=true overrides a false feature flag), JWT store put/get/
invalidate/prune + listener broadcast, and FeatureManager propagation to
the gates on init and on HYDRATE.
@nan-li nan-li force-pushed the feat/iv-foundation-01 branch from 8d2cd44 to 44bc2f4 Compare April 24, 2026 04:03
@nan-li
Copy link
Copy Markdown
Contributor Author

nan-li commented Apr 24, 2026

@claude review

Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior race-condition concern was addressed (payload dropped from IJwtUpdateListener); only a nit remains, but deferring since this lands persistent JWT storage and the IV gate primitives that subsequent PRs will build on.

Extended reasoning...

Overview

This is a foundation PR for the Identity Verification (IV) initiative across the OneSignal Android SDK. It adds: FeatureFlag.IDENTITY_VERIFICATION (IMMEDIATE activation), an IdentityVerificationGates singleton tracking newCodePathsRun/ivBehaviorActive, a SharedPreferences-backed JwtTokenStore (externalId → JWT) with an IJwtUpdateListener event interface, a JwtRequirement enum that replaces ConfigModel.useIdentityVerification: Boolean so callers can distinguish pre-HYDRATE from explicit false, and DI wiring in CoreModule. There is no consumer wiring or public API change in this PR — those are deferred to follow-on PRs.

Security risks

The PR introduces persistent storage of JWT tokens in unencrypted SharedPreferences (PREFS_OS_JWT_TOKENS). On most modern Android devices file-based encryption protects this at rest, but tokens are still readable by any process running as the app's UID (e.g. via root or a backup). This is consistent with how the SDK persists other identifiers, but is worth flagging for human review since this is the first JWT-bearing material the SDK persists. No injection / auth-bypass / TOCTOU concerns in the storage logic itself; ensureLoaded recovers gracefully from corrupt JSON and tests cover that path.

Level of scrutiny

Higher than a typical config tweak. The PR is foundation code for an auth-adjacent feature (JWT signing of SDK requests), introduces a new persistence surface, and changes the type of an existing ConfigModel property that is read across the codebase. Even though it lands no consumer wiring, the contracts here (gate semantics, event interface, persistence format) constrain the design of the four follow-on PRs in the series. That makes it worth a human pass before merge.

Other factors

Test coverage is thorough: the gate-state matrix (including the documented ERROR STATE row where jwt_required overrides a false feature flag), JWT store put/get/invalidate/prune, listener broadcast, and FeatureManager propagation on init and HYDRATE are all covered. The author addressed my prior race-condition concern by taking option 1 from the suggestion (commit ae498ac dropped the JWT payload from IJwtUpdateListener.onJwtUpdated so subscribers must re-query getJwt, and added a doc comment to that effect on the interface) and additionally refactored Boolean? to a three-state JwtRequirement enum (commit b02645d) for clearer pre-HYDRATE handling. The only new finding from this round is a nit about preserving the Throwable stack trace in JwtTokenStore.ensureLoaded — non-blocking. Deferring to nan-li for the human pass they requested.

nan-li and others added 2 commits April 24, 2026 08:45
…dering

JwtTokenStore fires update events after releasing the synchronized block
(matching the codebase's ModelStore pattern). With two concurrent writers
on the same externalId, event delivery order isn't guaranteed to match
mutation order — so a subscriber reading the event payload could see a
stale token.

Changed [IJwtUpdateListener] to a pure wake-up (`onJwtUpdated(externalId)`);
subscribers must call [JwtTokenStore.getJwt] to read the current value.
This matches the codebase's "events as wake-ups" convention (e.g. ModelStore
handlers re-read after notification) and removes the attractive nuisance
before PR 5's IAM retry is written against it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`ConfigModel.useIdentityVerification` was `Boolean?` where `null` silently
meant "pre-HYDRATE, not yet known" — easy to miss and easy to confuse with
the SDK-side `FeatureFlag.IDENTITY_VERIFICATION`. Replace with a tri-state
`JwtRequirement` enum (UNKNOWN / NOT_REQUIRED / REQUIRED) that names the
customer-config side explicitly and ties to the backend param (`jwt_required`).

- New `JwtRequirement` with a `fromBoolean(Boolean?)` helper so
  `ConfigModelStoreListener` can map the backend's nullable boolean.
- `ConfigModel.useIdentityVerification` is now `internal var … : JwtRequirement`
  (internal because the type is internal-only). Backing storage is unchanged —
  still a nullable boolean under the hood, so no cache migration needed.
- `IdentityVerificationGates.update` takes `jwtRequirement: JwtRequirement`;
  `required` is derived locally as `== REQUIRED`.
- Tests updated to use enum values.
@nan-li nan-li force-pushed the feat/iv-foundation-01 branch from b02645d to 474f1b3 Compare April 24, 2026 15:46
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous structural concern about event payload ordering was addressed by dropping the payload from IJwtUpdateListener; remaining items are nits.

Extended reasoning...

Overview

Foundation PR (1 of 5) for Identity Verification: adds IdentityVerificationGates singleton, JwtTokenStore, JwtRequirement enum, IJwtUpdateListener, a new IDENTITY_VERIFICATION FeatureFlag entry, and threads ConfigModel through FeatureManager.applySideEffects. ConfigModel.useIdentityVerification changes from Boolean to the new enum but backs the same nullable-boolean storage (no cache migration). Test coverage is thorough (gate matrix including the ERROR STATE row, JwtTokenStore put/get/invalidate/prune/persistence/listener behaviors, FeatureManager IV wiring including HYDRATE propagation and the mid-session IMMEDIATE flip).

Security risks

Low for this PR in isolation — nothing reads the gates or the JWT store yet, so there is no runtime behavior change. The JWT store persists tokens in SharedPreferences under a new key; this matches how other sensitive config is stored in the SDK and the actual auth/HTTP integration is deferred to PR 3. No injection or auth-bypass surface introduced here.

Level of scrutiny

Moderate. It is foundation code for a security feature, but it is intentionally inert — the real scrutiny load lands in PRs 2–5 when consumers wire in. The one design decision worth calling out (payload-less event with a re-query contract) was already surfaced in my prior review and the author adopted it, which is visible in the current IJwtUpdateListener signature and its documentation comment.

Other factors

  • My earlier concern about event-ordering in JwtTokenStore was addressed by dropping the JWT payload from onJwtUpdated and explicitly documenting the re-query contract. That was the substantive issue.
  • Outstanding items are nits: (a) ensureLoaded logs e.message rather than forwarding the throwable to Logging.warn; (b) the two @Volatile writes in IdentityVerificationGates.update are sequential so a reader can transiently observe (newCodePathsRun=false, ivBehaviorActive=true) during a (true,true)→(false,false) transition, violating the file-level invariant. Neither affects runtime today since there are no consumers, and both are trivially fixable before PR 2 lands.
  • Test coverage is strong and the PR scope is tight.

@nan-li nan-li changed the title feat(iv): foundation: gates singleton, JWT token store, feature flag … feat(iv): foundation: gates singleton, JWT token store, feature flag entry (1/6) Apr 24, 2026
`IdentityVerificationGates.update` previously wrote the two `@Volatile` fields
sequentially. On a `(true, true) → (false, false)` downgrade, a reader could
observe `(newCodePathsRun=false, ivBehaviorActive=true)` between the writes,
violating the documented invariant that `ivBehaviorActive=true` implies
`newCodePathsRun=true`.

Drop `newCodePathsRun` as a stored `@Volatile` field and compute it on read
from the two stored inputs (`_featureFlagOn` and `ivBehaviorActive`). The
invariant holds algebraically at every observation — `X || Y >= Y` — so
consumers can read either field (or both, in any order) without observing
an inconsistent state. Writer still performs two volatile stores but their
ordering no longer matters for the documented invariant.

Per-field access stays identical for consumers; `newCodePathsRun` is now
a computed property (two volatile reads + a boolean OR) but the perf delta
is negligible.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant