Conversation
…#3688631
Add a dedicated, non-mutating `log_telemetry` action to AuthUxJavaScriptInterface so
Auth UX page JS can push an opaque server error code to the native host over the existing
postMessageToBroker bridge, without touching the number-match / write_data device-store
path (H3).
Implemented against the Auth UX design-doc wire format:
{ "correlationID": ..., "action_name": "log_telemetry", "action_component": "host",
"params": { "v": 1, "sessionID": ..., "errorCode": 530003, "pageId": ..., "trackingId": ... } }
- AuthUxJavaScriptInterface: dispatch by top-level action_name == "log_telemetry" (the action
carries no params.operation) via a new ActionNames.LOG_TELEMETRY; add a minimal
AuthUxTelemetrySink seam and an optional (@jvmoverloads) telemetrySink constructor param that
keeps existing no-arg construction source-compatible. The branch reads errorCode and forwards
it to the sink; absent/empty errorCode is a safe no-op. number_matching still dispatches off
params.operation, unchanged.
- AuthUxJsonPayload: model the full params contract with @SerializedName — v (version), sessionID,
errorCode (opaque; captured as a string even when sent as a JSON number), pageId, trackingId;
all fields nullable.
- Forward-compat: new top-level and params key/value pairs do not break parsing or dispatch
(custom deserializer reads only known top-level keys; Gson ignores unknown params fields).
- Tests: exact design-doc payload; errorCode present/absent; unknown top-level + params fields
tolerated; sink invoked exactly once; no-op on missing errorCode; number_matching regression-safe
and never routed to the sink.
The onboarding-blob append (blocking-errors list + non-blocking exclusion) and consumption of
pageId/trackingId/sessionID/v are handled downstream in AB#3688632.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…s), AB#3688632 Wire the log_telemetry bridge (AB#3688631) to the onboarding telemetry recorder so a valid Auth UX log_telemetry event appends the server error code to the onboarding blob's blocking-errors list, unless the code is in the non-blocking exclusion list (parity with iOS nonBlockingOnboardingErrorCodes). - AzureActiveDirectoryWebViewClient.initializeAuthUxJavaScriptApi: thread the recorder into the bridge at registration via a sink (this::recordAuthUxServerErrorCode). New @VisibleForTesting recordAuthUxServerErrorCode reads mOnboardingTelemetryRecorder lazily (works whether the recorder is attached before or after registration), applies the exclusion, and appends via addBlockingError. Append-only / non-mutating, best-effort (never throws), null recorder = no-op. - OnboardingBlockingErrorParser: expose isNonBlockingOnboardingErrorCode(code) reusing the existing NON_ONBOARDING_AADSTS_CODES set, so the bridge path shares one source of truth with the x-ms-clitelem / authorization-error-code parsers (no drift). - AuthUxJavaScriptInterface: KDoc/comment refinement (sink now wired by the host). - IOnboardingTelemetryRecorder / OnboardingTelemetryRecorder: reconcile addBlockingError KDoc — the value may be a symbolic constant OR a numeric server/STS code (as already produced by the parser and now the bridge), recorded verbatim as an opaque string. - Tests: WebViewClient (Robolectric, real recorder + finalizeBlob) append/exclusion/null-no-op; parser isNonBlockingOnboardingErrorCode cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Work item link check complete. Description contains link AB#3688632 to an Azure Boards work item. |
github-actions
Bot
changed the title
added 13 commits
August 3, 2026 09:26
…8632-onboarding-blob-append
…8632-onboarding-blob-append
…ry sink survives navigation, AB#3688632 onPageStarted re-evaluates and re-registers the Auth UX JS bridge on every navigation, and addJavascriptInterface replaces whatever object was bound to the same name. It constructed a bare AuthUxJavaScriptInterface, so on the very first page load it replaced the sink-carrying instance registered by initializeAuthUxJavaScriptApi -- silently turning the whole log_telemetry path into a no-op in production. Build the bridge through a protected createAuthUxJavaScriptInterface() factory that AzureActiveDirectoryWebViewClient overrides to attach the sink, so both registration sites produce an identically configured instance. Also drop the mAuthUxJavaScriptInterfaceAdded field that shadowed the base class's protected field: the subclass tracked the initial registration while the base class tracked the per-navigation add/remove, so the shim in onPageFinished could be injected for a page whose interface the base class had already removed. The existing tests called recordAuthUxServerErrorCode() directly and so could not observe this; add a regression test that drives receiveAuthUxMessage() on the instance the client actually binds. It fails without this change (expected:<1> but was:<0>).
… context, AB#3688631
Blocking fixes:
- Dispatch on action_name BEFORE params.operation. Previously a log_telemetry
message that smuggled params.operation=number_matching took the number-match
branch and mutated the device store, contradicting the H3 invariant the code
comment claimed. Regression test fails without the reordering.
- params.v retyped Int? -> String?. Gson throws on a type mismatch, so a version
of "1.0" or "v2" dropped the ENTIRE message including errorCode. The one field
designed to change over time must never be a parse cliff.
Hardening:
- Validate errorCode against ^[A-Za-z0-9_-]{1,32}$ BEFORE it reaches any log
line, so a crafted value cannot inject newlines into logs or push an unbounded
string into the uploaded onboarding blob. Mirrors the NumberMatchHelper
precedent on this same bridge.
- Dedupe and cap forwarded codes per bridge instance (max 10) so a looping page
cannot bloat the blob.
- Bound page-supplied action/operation strings echoed into logs.
Observability:
- Log the forwarded / rejected / duplicate / capped / no-sink-wired outcomes
distinguishably, each with the correlation id inline.
- Record the code on a new authux_js_error_code span attribute so the path is
queryable in android_spans without waiting for the blob to land.
- Give the sink invocation its own try/catch so a throwing sink is no longer
misreported as "Unknown error occurred while processing the payload".
API:
- Sink now takes an AuthUxTelemetryEvent (correlation id, error code, session /
page / tracking ids, version) instead of a bare error code. correlationId is
the Kusto/DRI join key and was being discarded at the seam; passing the
context now avoids a source-breaking SAM change in a follow-up PBI.
- Document the JavaBridge calling thread and the thread-safety requirement on
AuthUxTelemetrySink.
Also: document the log_telemetry wire format on receiveAuthUxMessage (the KDoc
described only number-match), use the unused SerializedNames.PARAMS constant,
and add the reviewer's missing test cases (28 bridge + 10 payload, all passing).
…, AB#3688632 The bridge sink now receives an AuthUxTelemetryEvent (correlation id, error code, session / page / tracking ids, contract version) instead of a bare error code, so recordAuthUxServerErrorCode takes the event and reads getErrorCode() from it. The correlation id is now available at this seam and is attached to both the non-blocking-skip and the failure log lines. Conflict resolution: kept the reordered action_name-first dispatch and the single handleLogTelemetry path from AB#3688631, dropping this branch's older inline log_telemetry block; kept this branch's createAuthUxJavaScriptInterface reference in the sink KDoc alongside the new threading contract.
…8632-onboarding-blob-append
…tize correlationId, AB#3688631 A (blocking) record-before-forward: a code was added to forwardedErrorCodes before the sink was null-checked or invoked, so a code that nothing consumed was suppressed as a duplicate on retry and permanently lost -- while the bridge had already logged "Forwarded ...". The sink now returns whether it consumed the event; the code is recorded only on true, never on the no-sink path. A sink that throws counts as consumed (a throwing sink is a host defect, retrying cannot fix it, and treating it as retryable lets a looping page re-invoke it without bound). B (blocking) dedupe/cap scope: the bridge is rebuilt on every onPageStarted, so the per-instance state resets each navigation while the consumer lives for the whole request. Confirmed by our own device E2E, whose blob contained blocking_errors ["530003","530003"] from two page loads -- previously written off as a simulation artifact. The KDoc now states the per-page scope and its consequence plainly, a test pins the behaviour across two registrations, and request-wide de-duplication moves to the consumer, which owns the blob. C: the span attribute was set before the dedupe/cap checks, so it reported codes that were never forwarded. Moved after the forward decision, and the last-forwarded-wins overwrite semantics are documented. D: correlationID is page-controlled and was logged raw, then passed as the correlationID argument of five Logger calls -- common4j formats it verbatim, so the log-forging vector this PR closes for errorCode was still open here. Sanitized once immediately after parsing. E: the sink is no longer invoked while holding the internal lock, and the threading contract now states the guarantee the bridge actually provides (serialized per instance, on the JavaBridge thread) instead of asserting a requirement it cannot enforce. Host-owned state is made thread-safe in #3201. F: the version KDoc no longer claims "no cliff" -- a structured value still drops the message; documented as out of contract. Tests: 28 -> 34. The A fix is pinned by two tests that fail without it (expected:<[530003]> but was:<[]>).
…8632-onboarding-blob-append
…ecorder, AB#3688632 recordAuthUxServerErrorCode now returns whether it consumed the event: false when no recorder is attached yet (so the bridge keeps the code eligible for retry instead of suppressing it as already-forwarded), true when the code was recorded or deliberately dropped as non-blocking. Move request-wide de-duplication to the recorder, which owns the blob and outlives the bridge. The JS bridge is re-registered on every WebView navigation and only de-duplicates within one page load, so a redirect-heavy flow recorded the same server error repeatedly -- our own device E2E produced blocking_errors ["530003","530003"] for this reason. Make the recorder's steps/blocking-errors collections thread-safe and snapshot them under their monitor before serializing. Collections.synchronizedList makes single operations atomic but not iteration, and these lists are written from the WebView JavaBridge thread while finalizeBlob serializes on the caller's thread -- previously a lost-write / ConcurrentModificationException risk at exactly the moment the data is finalized for upload. This is what makes the bridge's threading contract satisfied by construction rather than by convention.
added 7 commits
August 4, 2026 23:02
…8631 N-B (regression from the last round): moving add() after consumption meant the distinct-code cap only ever counted CONSUMED codes, so the not-consumed and no-sink paths were unbounded -- a code that is never consumed is never recorded, so neither contains() nor the size check can ever fire for it. A page looping the same code while the recorder was unattached re-invoked the sink and re-logged without limit, and today every production message takes one of those two paths. The previous test hid this: it posted 15 codes against a cap of 10 and asserted only that none were consumed, never that the sink stopped being called. Added telemetryAttempts, counted before the outcome is known, so every path past validation is bounded (MAX_TELEMETRY_ATTEMPTS = 25, above the distinct-code cap so legitimate retries still get through). The test now asserts sink invocations are capped and fails without the guard (expected:<25> but was:<40>). N-A: a throw set consumed = true, which drove three decisions when it should drive one. Suppressing retry is right; also setting authux_js_error_code and logging "Forwarded ... to onboarding telemetry" was not -- nothing reached downstream telemetry, and the comment above setAttribute asserted exactly that could not happen. Replaced the boolean with a SinkOutcome tri-state (CONSUMED / NOT_CONSUMED / THREW): THREW suppresses retry only. N-C: the KDoc credited per-instance serialization to this class, but receiveAuthUxMessage takes no lock -- that is a WebView platform property (one JavaBridge thread per WebView). It matters because the dedupe/cap bookkeeping is now check-then-act across two critical sections with the sink call between them. Documented the gap, attributed the serialization correctly, and noted the state stays guarded so an off-thread caller could at worst cause a bounded duplicate forward rather than a corrupted set. Also renamed forwardedErrorCodes -> handledErrorCodes: it now holds codes that will not be offered again, which includes the throw case.
…8632-onboarding-blob-append
…join key, AB#3688631
The attempt cap was checked AFTER the duplicate and distinct-code checks, and both
of those log and return -- so a page looping one already-handled code, or posting
new codes once the distinct-code cap was reached, never reached the counter and
could spam the log without bound. Same failure mode the counter was added to
prevent, one level up. Now checked and incremented first, so every path past
validation is bounded, and the cap message itself is logged once on the
transition rather than on every subsequent message.
correlationID was run through sanitizeForLog, which truncates at 64 characters.
It is also the telemetry join key forwarded to the sink, so truncating a longer
ID would silently break correlation. Split out sanitizeCorrelationId: strips
control characters (the log-forging concern) without changing length semantics,
cutting only past 128 characters, which no real correlation ID reaches. A GUID
now survives verbatim, pinned by a test.
Mirrored authux_js_error_code into broker4j's AttributeName enum, which already
carries authux_js_action_name / _action_component / _operation -- cross-repo
telemetry keys must stay aligned. Switched the JavaDoc to {@code} in both, since
Markdown backticks do not render in JavaDoc.
Made the log_telemetry test fixtures strict JSON (quoted keys). They are
described as matching the design-doc wire format, so they should be what a real
JSON.stringify sender emits rather than something only a lenient parser accepts.
Pre-existing number-match fixtures left alone -- not this PR's code.
Tests 37 -> 39. The ordering fix is pinned by a test that fails without it
(expected:<1> but was:<2>).
…8632-onboarding-blob-append
…8632-onboarding-blob-append
Repeated merges of an edited changelog line left four duplicate #3197 entries and split this PR across three separate lines. Collapsed to a single entry each.
added 3 commits
August 5, 2026 11:34
Both sanitize helpers transformed the whole page-supplied string and only then truncated it. sanitizeCorrelationId was the worse of the two: String.map produces a List<Char>, so it boxed every character of the input before joining. Replaced both with one sanitizeBounded(value, limit) that walks at most limit characters into a pre-sized StringBuilder. Work is now proportional to the output bound rather than to the size of the untrusted input. Note this bounds OUR processing, not the attacker's allocation -- the value is already fully materialized by the time the bridge is called (WebView marshals the payload across JNI and Gson parses it before we see it), so the DoS framing is weaker than it looks. It is still strictly better and trivially cheap. Unifying on the shared helper also means sanitizeForLog now strips all control characters instead of only CR/LF, so an ESC or NEL can no longer reach a log line either.
…8632-onboarding-blob-append
…e KDoc, AB#3688631 Three Logger calls in receiveAuthUxMessage used the two-arg overload while the comment right above them claimed the correlation ID is passed as the dedicated argument of every call on this path. It was true of handleLogTelemetry but not of these, so a DRI could not reliably filter or group the earlier lines. All three now pass correlationId; the comment matches the code again. The "Correlation ID during JavaScript Call: [id]" line no longer embeds the ID in the message, since common4j now prefixes it -- it would have appeared twice. Reworded to say what the line actually marks (payload parsed). sanitizeBounded REPLACES control characters with spaces, it does not remove them, but three KDoc blocks and two test names said "strip". Corrected the wording and documented why replacing is the right choice: the result keeps the same length as the inspected prefix, so a value's shape stays recognisable in a log line instead of silently closing up around the removed characters. Strengthened the newline test to assert the exact replacement output rather than just the absence of CR/LF, so the documented behaviour is pinned.
… reference Review feedback on #3201. Ordering (blocking): the all-or-nothing guarantee the Auth UX sink's retraction depends on was not airtight. persistSessionCorrelation catches Exception but deliberately lets Error through, and it ran AFTER the append - so an Error out of the persist (OOM while touching SharedPreferences) left the code in blocking_errors while the caller saw a throw. The sink retracts its de-duplication claim on ANY throw, so the next navigation would re-offer the same code, add() would pass again, and it would land in the blob twice - the exact duplicate the retraction exists to prevent, reached through Error instead of Exception. Fixed by running the persist first, as the reviewer suggested. It costs nothing: the persist does not depend on the append, and persisting for a block that is then never recorded is harmless (the session-correlation write is idempotent). The alternative - catching Throwable in the persist - was rejected because swallowing an OutOfMemoryError to protect a telemetry write is the wrong trade. Docs corrected in all three places that asserted the old guarantee: the addBlockingError KDoc no longer scopes it to "with respect to exceptions", the persistSessionCorrelation KDoc now explains that the ordering is what makes the guarantee hold, and the sink's inline comment no longer claims the persist "cannot throw". Forward reference: the mForwardedAuthUxErrorCodes KDoc cited OnboardingRecorderRegistry, which does not exist on this branch - it arrives in AB#3708195. The known-limitation paragraph also read as if a rebuilt client always duplicates, when on this PR the host sets the recorder directly, so a rebuilt client normally gets a fresh recorder and there is nothing to duplicate into. Rewritten to describe this PR's behaviour and name the follow-up as the point where it becomes reachable. New test testAddBlockingError_ErrorFromPersist_RecordsNothing drives a real recorder over a Context whose getSharedPreferences throws OutOfMemoryError. Revert-tested: restoring the append-first order fails it with "a throw must mean nothing was recorded, or the sink's retraction produces duplicates". 176 tests green: WebViewClient 98, bridge 45, recorder 19, payload 10, store 4.
wzhipan
pushed a commit
that referenced
this pull request
Aug 8, 2026
…ed), AB#3708195 The broker builds an onboarding telemetry recorder per request from the seed, but the WebView that renders the interactive auth / remediation pages lives in a separate AuthorizationActivity created by the OS from an Intent. A live recorder cannot ride an Intent, so the WebView-side onboarding hooks were inert for brokered flows: steps observed in the WebView (MDM enrollment, Company Portal launch, broker install), the last loaded domain, and the Auth UX log_telemetry error code never reached the blob the broker finalizes and returns. Both components run in the broker :auth process, so this adds an in-process handoff keyed by the request correlationId. OnboardingRecorderRegistry (new) - Rejects DiagnosticContext.UNSET_CORRELATION_ID as a key. Every thread whose request context was never set carries that sentinel, and the authorization Intent extra is populated by reading the request-context map directly rather than through getThreadCorrelationId(), so the raw sentinel can reach the registry. It is shared by definition: accepting it would let two unrelated requests resolve to the same recorder and merge one flow's blocking errors into the other's uploaded blob. Rejecting it makes the feature inert (and logs a warning) instead of silently mis-attributing telemetry. - Bounded, access-ordered map (cap 16). Entries must be unregistered on terminal outcome, and because this is process-static state in a process that lives as long as the device is up, a missed unregister would otherwise leak a recorder permanently - the recorder object and its collected steps / blocking errors, not an Activity, since OnboardingTelemetryRecorder holds only the application context. On overflow it evicts the least-recently-used entry and logs a warning, bounding the leak while surfacing the underlying bug. AuthorizationFragment - Captures the correlation id in extractState and round-trips it through onSaveInstanceState. The read was already a base-class responsibility, so the save belongs next to it: putting it in one subclass left the other two (BrowserAuthorizationFragment, CurrentTaskBrowserAuthorizationFragment) still blanking their diagnostic context after activity recreation, and invited the pair to drift apart again. All three subclasses already call super, so the parent implementation fixes all of them with no duplication. - This also repairs a pre-existing bug on that path: extractState previously fed setDiagnosticContextForNewThread(null) after a recreation, so every subsequent log line for the request lost its correlation id. WebViewAuthorizationFragment - Resolves the recorder in onCreateView from the inherited mCorrelationId and attaches it to the WebView client before initializeAuthUxJavaScriptApi(...). No-op when the request seeded no recorder, which is the MSAL-client path. DiagnosticContext - UNSET_CORRELATION_ID is now public, and the internal duplicate string literal in getThreadCorrelationId() references it. Callers that use the correlation id as a KEY rather than for logging have to reject the sentinel explicitly, and a copied "UNSET" literal in the registry would silently stop matching if this ever changed - reintroducing cross-request contamination with no signal. Logger - Notes that its own UNSET literal is a display placeholder, deliberately independent of the DiagnosticContext sentinel: it also stands in for a missing thread id and is never used as a key, so it must not be collapsed into the new constant. Note this PR alone changes no behaviour for the recorder handoff: register/unregister have no caller in this repo (both live in the paired broker PR). Merge order: #3197 -> #3201 -> #3204 -> broker PR. Tests - OnboardingRecorderRegistryTest (15): keying, the UNSET sentinel (including the concrete two-request cross-wiring it prevents), removal / lifecycle, absent-key behaviour, null and empty inputs, idempotent unregister, and the leak bound. - AuthorizationFragmentCorrelationIdTest (6, new): the fragment half of the handoff - extractState captures the id, the captured id resolves the registered recorder, it survives save/restore, and the no-recorder / missing-id / sentinel paths resolve to null rather than throwing. Without these, a regression in the correlation-id plumbing would leave every hook inert while the registry tests still passed. Revert-tested: removing the sentinel guard fails with "the sentinel must never become a key expected:<0> but was:<1>"; disabling the cap fails with "expected:<16> but was:<100>"; switching eviction to insertion order evicts a live in-use recorder; dropping the onSaveInstanceState round-trip fails with "the correlation id must round-trip through the saved bundle expected:<...> but was:<null>". Verified end-to-end on device: built brokerHost against this change and drove a brokered MAM/Conditional-Access flow; the finalized blob carried the WebView- observed steps, last_loaded_domain, and blocking_errors ["530003"] from the Auth UX bridge. 246 tests green across the onboarding / Auth UX / authorization-fragment suites.
added 3 commits
August 7, 2026 18:41
…, AB#3688631 Doc only. The rationale said the regex is permissive enough for the symbolic blocking-error constants, which reads as if a page posting one of them is a supported use. It is not: those constants are what the broker writes for blocks it detected itself, so a page posting DEVICE_REGISTRATION_NEEDED would be indistinguishable from a real device-registration block in the onboarding blob. The bridge cannot make that call - it is a generic seam and does not know where a code ends up - so the narrowing belongs to the sink, and the onboarding sink now accepts only numeric server codes (#3201). Recording that split here so the next reader does not take the permissive shape as a licence.
Two review findings, both about the sink being the trust boundary for the only
untrusted caller of addBlockingError.
Symbolic-code collision. The bridge shape-checks [A-Za-z0-9_-]{1,32}, which every
one of broker4j's symbolic blocking-error constants also matches -
DEVICE_REGISTRATION_NEEDED and INSUFFICIENT_DEVICE_REGISTRATION are the exact two
it writes on a real device-registration block. A page could post either, we would
append it to the same blocking_errors list and set last_blocking_error to it, and
nothing downstream - the onboarding dashboards included - could tell it from a
block the broker detected itself.
Fixed at the sink rather than at the bridge. The bridge is a generic seam and
cannot know where a code ends up; this sink knows its destination is a list shared
with broker4j, so the narrowing is its policy to make. Server-reported codes are
numeric, so ^[0-9]{1,32}$ closes the collision without rejecting anything a page
can legitimately report. (#3197 carries a matching doc note so the permissive
shape there is not read as a licence.)
Unbounded per-request growth. The bridge's caps (10 forwarded, 25 attempts) are
per INSTANCE and onPageStarted rebuilds it on every navigation, so they reset each
page load - this client's de-duplication set was the only thing spanning the
request, and nothing bounded it. A page cycling DISTINCT codes across navigations
could grow the uploaded blob without limit.
Capped at 10 distinct codes per request. Duplicates never counted anyway, and a
real flow reports one or two, so reaching this needs a page deliberately cycling
values. Checked AFTER the atomic add() rather than before, so the check-and-claim
stays one operation and the earlier check-then-act gap is not reintroduced; the
code is removed again when it does not fit. Returns consumed, so a capped code
does not burn the bridge's retry budget on something that can never be recorded.
The cap warning is logged once, on the transition, so it cannot become the flood
it exists to prevent.
Revert-tested: disabling either guard fails its test -
testRecordAuthUxServerErrorCode_SymbolicCodeIsRejected and
testRecordAuthUxServerErrorCode_PerRequestCapBoundsTheBlob.
179 tests green: WebViewClient 101, bridge 45, recorder 19, payload 10, store 4.
wzhipan
pushed a commit
that referenced
this pull request
Aug 8, 2026
…ed), AB#3708195 The broker builds an onboarding telemetry recorder per request from the seed, but the WebView that renders the interactive auth / remediation pages lives in a separate AuthorizationActivity created by the OS from an Intent. A live recorder cannot ride an Intent, so the WebView-side onboarding hooks were inert for brokered flows: steps observed in the WebView (MDM enrollment, Company Portal launch, broker install), the last loaded domain, and the Auth UX log_telemetry error code never reached the blob the broker finalizes and returns. Both components run in the broker :auth process, so this adds an in-process handoff keyed by the request correlationId. OnboardingRecorderRegistry (new) - Rejects DiagnosticContext.UNSET_CORRELATION_ID as a key. Every thread whose request context was never set carries that sentinel, and the authorization Intent extra is populated by reading the request-context map directly rather than through getThreadCorrelationId(), so the raw sentinel can reach the registry. It is shared by definition: accepting it would let two unrelated requests resolve to the same recorder and merge one flow's blocking errors into the other's uploaded blob. Rejecting it makes the feature inert (and logs a warning) instead of silently mis-attributing telemetry. - Bounded, access-ordered map (cap 16). Entries must be unregistered on terminal outcome, and because this is process-static state in a process that lives as long as the device is up, a missed unregister would otherwise leak a recorder permanently - the recorder object and its collected steps / blocking errors, not an Activity, since OnboardingTelemetryRecorder holds only the application context. On overflow it evicts the least-recently-used entry and logs a warning, bounding the leak while surfacing the underlying bug. AuthorizationFragment - Captures the correlation id in extractState and round-trips it through onSaveInstanceState. The read was already a base-class responsibility, so the save belongs next to it: putting it in one subclass left the other two (BrowserAuthorizationFragment, CurrentTaskBrowserAuthorizationFragment) still blanking their diagnostic context after activity recreation, and invited the pair to drift apart again. All three subclasses already call super, so the parent implementation fixes all of them with no duplication. - This also repairs a pre-existing bug on that path: extractState previously fed setDiagnosticContextForNewThread(null) after a recreation, so every subsequent log line for the request lost its correlation id. WebViewAuthorizationFragment - Resolves the recorder in onCreateView from the inherited mCorrelationId and attaches it to the WebView client before initializeAuthUxJavaScriptApi(...). No-op when the request seeded no recorder, which is the MSAL-client path. DiagnosticContext - UNSET_CORRELATION_ID is now public, and the internal duplicate string literal in getThreadCorrelationId() references it. Callers that use the correlation id as a KEY rather than for logging have to reject the sentinel explicitly, and a copied "UNSET" literal in the registry would silently stop matching if this ever changed - reintroducing cross-request contamination with no signal. Logger - Notes that its own UNSET literal is a display placeholder, deliberately independent of the DiagnosticContext sentinel: it also stands in for a missing thread id and is never used as a key, so it must not be collapsed into the new constant. Note this PR alone changes no behaviour for the recorder handoff: register/unregister have no caller in this repo (both live in the paired broker PR). Merge order: #3197 -> #3201 -> #3204 -> broker PR. Tests - OnboardingRecorderRegistryTest (15): keying, the UNSET sentinel (including the concrete two-request cross-wiring it prevents), removal / lifecycle, absent-key behaviour, null and empty inputs, idempotent unregister, and the leak bound. - AuthorizationFragmentCorrelationIdTest (6, new): the fragment half of the handoff - extractState captures the id, the captured id resolves the registered recorder, it survives save/restore, and the no-recorder / missing-id / sentinel paths resolve to null rather than throwing. Without these, a regression in the correlation-id plumbing would leave every hook inert while the registry tests still passed. Revert-tested: removing the sentinel guard fails with "the sentinel must never become a key expected:<0> but was:<1>"; disabling the cap fails with "expected:<16> but was:<100>"; switching eviction to insertion order evicts a live in-use recorder; dropping the onSaveInstanceState round-trip fails with "the correlation id must round-trip through the saved bundle expected:<...> but was:<null>". Verified end-to-end on device: built brokerHost against this change and drove a brokered MAM/Conditional-Access flow; the finalized blob carried the WebView- observed steps, last_loaded_domain, and blocking_errors ["530003"] from the Auth UX bridge. 246 tests green across the onboarding / Auth UX / authorization-fragment suites.
wzhipan
force-pushed
the
copilot/pbi-3688631-log-telemetry
branch
from
August 8, 2026 01:57
3e0a810 to
cb19149
Compare
wzhipan
pushed a commit
that referenced
this pull request
Aug 8, 2026
…s, AB#3688630 Non-brokered (OneAuth) flows never reach the Auth UX JavaScript bridge: it is gated on ProcessUtil.isRunningOnAuthService(), which is permanently false outside the broker's isolated :auth process. Those flows therefore cannot report the onboarding error codes the log_telemetry action exists to carry, even though the OneAuth core already builds the onboarding seed for them. Rather than widening the existing gate -- which would hand every MSAL client the full bridge, including the number-match device store -- this adds a second, narrower surface: - AuthUxJavaScriptInterface takes a telemetryOnly capability. When set it serves log_telemetry and refuses every other action, so the only effect a page can have is appending to the onboarding telemetry blob. Enforced at dispatch rather than only at the registration gate, so a future caller that constructs the bridge for a brokerless host cannot re-expose the mutating path by forgetting a check at its own call site. - shouldExposeJavaScriptInterface now selects the flight by host: the broker's :auth process keeps ENABLE_JS_API_FOR_AUTHUX, and any other host is gated by the new ENABLE_BROKERLESS_TELEMETRY_JS_API_FOR_AUTHUX (default off). Keeping the flights independent means the brokerless surface can be turned off without also disabling number-matching for the broker. The host allow-list (H1) still applies to both. - The capability is resolved in createAuthUxJavaScriptInterface(), because onPageStarted rebuilds the bridge on every navigation; a capability passed at a single call site would be silently dropped on the next page load, which is exactly how the telemetry sink was lost before #3201. Behaviour inside the broker is unchanged: the constructor default is false and the :auth branch of the gate is the same predicate it was. Tests (+8): number_matching inert in telemetry-only mode and its :auth-process control (so "the store stayed empty" cannot pass for the wrong reason); log_telemetry still served; a refused message does not abort the page; the constructor default; and the four gate combinations. Revert-tested -- dropping the dispatch guard fails "a telemetry-only bridge must never write to the number-match store", and collapsing the two flights fails "the broker's flight must not expose a bridge outside the :auth process". Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
wzhipan
added a commit
that referenced
this pull request
Aug 9, 2026
…xes AB#3688631 (#3197) ## Summary Implements [AB#3688631](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3688631) (child of Feature [AB#3688629](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3688629) — *Auth UX Telemetry Native Host: Broker / common-core (AndroidCommon)*). Adds a dedicated, **non-mutating** `log_telemetry` action to the existing `AuthUxJavaScriptInterface` JS bridge so Auth UX page JS can push an **opaque** server error code (e.g. an STS code such as `530003`) to the native host over the already-shipped `window.broker.postMessageToBroker(...)` → `receiveAuthUxMessage` path — **without** creating a new bridge, WebView interface, or telemetry island. ## Wire format Implemented against the Auth UX design-doc shape: ```json { "correlationID": "$correlationId", "action_name": "log_telemetry", "action_component": "host", "params": { "v": 1, "sessionID": "$sessionID", "errorCode": 530003, "pageId": "ConvergedTFA", "trackingId": "$trackingId" } } ``` Adding new key/value pairs — at the **top level** and inside **`params`** — does not break parsing or dispatch: the custom deserializer reads only known top-level keys, and Gson ignores unknown `params` fields. Covered by explicit tests. ## Changes **Dispatch** - Telemetry is matched on the top-level **`action_name == "log_telemetry"` first**, *before* `params.operation`. This ordering is load-bearing: a `log_telemetry` message that smuggles `params.operation = "number_matching"` must never reach the number-match device store. Pinned by a test that fails if the branches are reordered. - `number_matching` continues to dispatch off `params.operation`, unchanged. **Sink seam** - `AuthUxTelemetrySink` receives an **`AuthUxTelemetryEvent`** — `correlationId`, `errorCode`, `sessionId`, `pageId`, `trackingId`, `version` — rather than a bare error code. `correlationId` is the Kusto / DRI join key; passing the whole context now avoids a source-breaking change to a published SAM interface when a downstream consumer wants those fields. - The sink **returns whether it consumed the event**. A sink may legitimately accept a call and record nothing (the [AB#3688632](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3688632) implementation does exactly this while its recorder is not yet attached). Returning `false` leaves the code eligible for a later retry instead of it being suppressed as already-forwarded and lost. - Optional (`@JvmOverloads`) `telemetrySink` constructor param, so the existing no-arg construction stays source- and binary-compatible. **Validation and bounding** — `errorCode` is page-controlled and (via [AB#3688632](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3688632)) ends up in an uploaded blob, so it is: - shape/length checked against `^[A-Za-z0-9_-]{1,32}$` — permissive enough for both numeric STS codes and the symbolic constants `addBlockingError` accepts; - validated **before** it reaches any log line, so an embedded CR/LF cannot forge log entries; - de-duplicated and capped per bridge instance — 10 distinct codes handed to a sink, plus a separate bound of 25 total post-validation attempts so the paths that deliberately do *not* record a handled code (no sink wired, sink declines) are bounded too; - and every other page-supplied value (`action_name`, `operation`, `correlationID`) is sanitized before logging too. **Observability** — distinct log lines for forwarded / malformed / duplicate / capped / not-consumed / no-sink-wired, each carrying the correlation ID; new `authux_js_error_code` span attribute, set only for codes a sink actually took (so the span cannot disagree with downstream telemetry). A throwing sink gets its own `try/catch` and a sink-specific message rather than being misreported as a payload parsing failure, and a throw suppresses retry **only** — it does not set the span attribute or claim the code was forwarded, because nothing reached downstream telemetry. **Model** — `params` is modelled with `@SerializedName`; all fields nullable. Note `v` is deliberately typed **`String?`, not `Int?`**: Gson throws on a type mismatch, so an `Int?` version meant a `"1.0"` or `"v2"` value would drop the *entire* message including `errorCode`. Please do not "clean this up" back to `Int`. ## Scope of the dedupe/cap The cap is **per bridge instance**, and the WebView host rebuilds the bridge on every navigation — so it is a per-page flood guard, not a request-wide guarantee. Request-wide de-duplication belongs to the consumer that owns the blob and is implemented in [AB#3688632](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3688632). This is stated plainly in the KDoc and pinned by a test. ## Security (H3 / H6) The new branch is on a **separate code path** from `write_data` / `number_matching` / `storeNumberMatch` — telemetry-only, append-oriented, and it **never** mutates device or credential state. The client takes **no action** on the code (H6); `errorCode` is an opaque telemetry value. ## Threading `@JavascriptInterface` methods are dispatched on the WebView's private JavaBridge thread, not the UI thread. The bridge serializes its own calls per instance and never invokes a sink while holding an internal lock. It cannot make host-owned state safe, so the recorder's collections are made thread-safe in [AB#3688632](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3688632) rather than relying on a documentation-only requirement. ## Merge order — please read The host wiring for this seam lives in the stacked child PR #3201. If this PR lands on `dev` alone, `dev` carries a `log_telemetry` path that no host wires: every message will parse, validate, and then log *"no telemetry sink is wired; dropping"*. **Land them together**, or expect that silence. Both flows also remain gated on Auth UX actually emitting `log_telemetry` server-side ([AB#3696811](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3696811)). ## Scope note — brokered flow only (by design) Bridge exposure is gated by `shouldExposeJavaScriptInterface(...)` → `ProcessUtil.isRunningOnAuthService(...)` (plus the origin allow-list and the `ENABLE_JS_API_FOR_AUTHUX` flight), so this action is exercised in the **brokered** flow only. Non-brokered (OneAuth brokerless WebView) + iOS enablement is tracked under Feature [AB#3688630](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3688630). ## Out of scope (follow-up PBIs) - [AB#3688632](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3688632) — append the code to the onboarding blob's blocking-errors list + non-blocking exclusion (wires the concrete sink). - [AB#3688633](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3688633) — full page→native security controls (H1, H4–H6). - [AB#3688634](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3688634) — additional unit-test matrix. ## Testing ``` ./gradlew :common:testLocalDebugUnitTest \ --tests "*AuthUxJavaScriptInterfaceTest" --tests "*AuthUxJsonPayloadTest" ``` **BUILD SUCCESSFUL** — `AuthUxJavaScriptInterfaceTest` 37/37, `AuthUxJsonPayloadTest` 10/10, 0 failures. Two behaviours are pinned by revert-tests (they fail if the fix is undone): - dispatch order — *"log_telemetry must never write to the number-match store"*; - record-only-when-consumed — *"expected:`<[530003]>` but was:`<[]>`"*, i.e. the code would otherwise be permanently lost; - the attempt bound — *"expected:`<25>` but was:`<40>`"*, i.e. an unconsumed code would otherwise re-invoke the sink without limit. ## Known, accepted limitations - `errorCode: true` is coerced by Gson to the string `"true"`, which is alphanumeric and so indistinguishable from a symbolic code by shape. Validation bounds charset and length, **not** semantics — judging what is a meaningful error code is the server contract's job. Recorded as a named test rather than left undiscovered. - A structured `v` (`{major: 2}` / `[2]`) still throws and drops the message. That shape is not part of the contract. - `errorCode: 530003.0` is captured as `"530003.0"` and then rejected by validation — the recorded value depends on how the page serializes the number. --------- Co-authored-by: Zhipan Wang <zhipanwang@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…3197) was squash-merged # Conflicts: # common/src/main/java/com/microsoft/identity/common/internal/broker/AuthUxJavaScriptInterface.kt # common/src/test/java/com/microsoft/identity/common/internal/broker/AuthUxJavaScriptInterfaceTest.kt
Contributor
|
Could we consolidate the four |
Contributor
|
Could we rename this method to |
Contributor
|
Could we add a bridge-level test for codes deliberately consumed but not recorded, such as non-numeric or over-cap codes? Since returning |
Praveen Kumar (Prvnkmr337)
approved these changes
Aug 10, 2026
…gelog Review round 4 on AB#3688632 (shahzaibj, Prvnkmr337). 1. eSTS's "0" no-error sentinel could become a blocking error. All six extraction paths filter `!= "0"`, but the public single-code policy check did not -- while its KDoc claimed callers "apply the SAME policy as the header/response parsers". The Auth UX sink reaches that check directly and the numeric shape guard accepts "0", so a page reporting errorCode 0 recorded a blocking error the server never reported, and it won last_blocking_error. Folded "0" into the shared check behind a named NO_ERROR_SENTINEL constant and routed the six existing sites through it so they cannot drift again. 2. "Consumed" was reported as "recorded". For any consumed-but-not-recorded code the bridge logged "Forwarded ... to onboarding telemetry" immediately after the sink logged "rejecting ..." -- a directly contradictory pair in logcat, breaking this method's own documented promise that every exit path is distinguishable. Note this pre-dated the numeric check: the non-blocking 50058 drop produced the same contradiction. The log now states what CONSUMED means and defers to the sink's own line. Renamed recordAuthUxServerErrorCode -> tryConsumeAuthUxServerErrorCode so the name matches the contract, and rewrote the KDoc around "is this terminal here?" rather than "was it stored?". The span attribute keeps carrying a dropped code, deliberately. authux_js_error_code is namespaced to the bridge, so every value on it is page-supplied by construction -- unlike blocking_errors, which is SHARED with broker4j's own symbolic constants and therefore loses provenance. A symbolic value there is unambiguously "a page sent this", which is signal worth keeping. Now stated in the comment and pinned by tests rather than left implicit. 3. Changelog: five #3201 entries -> three. One feature entry absorbs the review hardening; the two genuinely independent fixes stay separate because they affect pre-existing paths (number-matching, and broker's own addBlockingError call sites). Tests +7: four for the sentinel (including one pinning the direct path against the header parser, and one asserting "00" is still blocking) and three stating the consumed-but-not-recorded span behaviour explicitly. Revert-tested: restoring the old policy line fails 2 sentinel tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Thanks Praveen — all three taken, in Changelog. Five entries → three. One feature entry now absorbs the review hardening (numeric restriction, caps, exclusions). I kept two separate because they aren't part of this feature's story: the bridge-availability fix also affects the pre-existing number-matching path, and the persistence fix also affects broker's own Rename. Done — The log. Also fixed — it claimed Tests. Added On whether the span behaviour is intended — yes, and I've documented why. |
Three #3201 entries -> one. The repo convention is one entry per PR: of 90 entries across the released versions, 89 are single-entry (one outlier), and the median length is ~101 characters -- these were 568 / 241 / 330. Kept the bridge-availability fix in the line because it also affects the pre-existing number-matching path, so a reader hitting that needs to find it. The session-correlation persistence fix is internal robustness with no visible behaviour change and is covered by the PR itself. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
wzhipan
pushed a commit
that referenced
this pull request
Aug 11, 2026
Three #3204 entries -> one, matching the repo's one-entry-per-PR convention (89 of 90 released entries are single-entry; median ~101 chars, these were 358 / 189 / 318). Kept the correlation-id round-trip fix in the line because it affects all three authorization fragments and every log line after a recreation, so it is worth finding from the changelog. The UNSET_CORRELATION_ID exposure exists to support this feature and is covered by the PR. Also de-duplicated the #3201 block: merging the consolidation up appended the new line instead of replacing the old three, which is the changelog merge hazard this stack has hit before. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
wzhipan
pushed a commit
that referenced
this pull request
Aug 11, 2026
One entry per PR across the stack, matching the repo convention (89 of 90 released entries are single-entry; median ~101 chars). The #3209 line drops from 562 to 224 characters and now leads with what a reader needs -- non-brokered flows get a telemetry-only bridge, gated on the onboarding seed -- leaving the rationale to the PR description. Also de-duplicated #3201 and #3204: merging each consolidation up the stack APPENDED the new line rather than replacing the old ones, so the block had to be rebuilt canonically. Worth remembering -- a changelog edit merged upward is not idempotent, and the count has to be re-censused after every merge. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…boarding-blob-append
Contributor
|
Copilot resolve the merge conflicts in this pull request |
…boarding-blob-append # Conflicts: # common/src/main/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java Co-authored-by: Prvnkmr337 <198076477+Prvnkmr337@users.noreply.github.com>
Contributor
Resolved by merging the current |
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
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.
Summary
Implements AB#3688632 (child of Feature AB#3688629 — Auth UX Telemetry Native Host: Broker / common-core (AndroidCommon)).
Wires the
log_telemetrybridge (added in AB#3688631) to the onboarding telemetry recorder: on a valid Auth UXlog_telemetryevent, the server error code is appended to the onboarding blob's blocking-errors list — unless it's in the non-blocking exclusion list (parity with iOSnonBlockingOnboardingErrorCodes).Changes
Production
AzureActiveDirectoryWebViewClient.javacreateAuthUxJavaScriptInterface()to attachthis::recordAuthUxServerErrorCodeas theAuthUxTelemetrySink. Using the factory (rather than passing the sink at one call site) is load-bearing:OAuth2WebViewClient.onPageStartedre-registers the bridge on every navigation, andaddJavascriptInterfacereplaces whatever was bound to the same name — so a bare instance registered there would silently drop the sink and make the whole path a no-op.@VisibleForTestingrecordAuthUxServerErrorCode(AuthUxTelemetryEvent): applies the non-blocking exclusion, then readsmOnboardingTelemetryRecorderlazily (works whether the recorder is attached before or after registration) and appends viarecorder.addBlockingError(...). The policy check runs first because it has no dependency on the recorder — a non-blocking code arriving early is terminal rather than retried against a budget it can never spend.falsewhen no recorder is attached yet, so the bridge keeps the code eligible for a later retry instead of suppressing it as already-handled;truewhen recorded, deliberately dropped as non-blocking, or already forwarded by this client. Nevertruefor a code that failed to reach the recorder — a throwing recorder propagates into the bridge'sTHREWhandling, which suppresses retry without setting the span attribute or logging a false "Forwarded" line. The de-dup claim is staked before the append and retracted in afinallyblock if the append does not succeed, so a throw cannot leave a code marked forwarded and make a later navigation's offer short-circuit to "already forwarded".["530003","530003"]without this. The de-dup lives here rather than in the recorder deliberately: this client is constructed once per request and outlives every navigation (the bridge does not), while the recorder'sblocking_errorslist is shared with broker4j and thex-ms-clitelemparsers and is contractually append-only and chronological. De-duplicating there would have changedlast_blocking_errorfrom "the last block observed" to "the last distinct block first observed" for every caller — mis-attributing an A → B → A remediation to B. Known limitation:AuthorizationActivity'sconfigChangesdoes not listuiMode, so toggling dark mode mid-flow rebuilds the client (and its de-dup set) while the recorder survives in the process-static registry — one duplicate entry, accepted and documented on the field rather than fixed by weakening the shared append-only contract.mOnboardingTelemetryRecorderis nowvolatile: this PR is what makes it cross-thread (UI-thread write, JavaBridge-thread read), and without the visibility guarantee the sink could readnullindefinitely and burn the bridge's retry budget against a recorder that was in fact attached.mAuthUxJavaScriptInterfaceAddedfield that shadowed the base class's, which had desynchronised interface add/remove tracking fromOAuth2WebViewClient. This also fixes bridge availability for the shipping number-matching path, not just telemetry: previously an initial non-allow-listed URL left the shim uninjected even after a later navigation added the interface, and once set the flag never cleared, so the shim could be injected over an undefinedwindow.broker.OnboardingBlockingErrorParser.kt(common4j) — exposeisNonBlockingOnboardingErrorCode(code)(@JvmStatic) reusing the existingNON_ONBOARDING_AADSTS_CODESset, so the bridge path applies the same exclusion policy as thex-ms-clitelem/ authorization-error-code parsers (single source of truth, no drift).OnboardingTelemetryRecorder.kt— makes the steps / blocking-errors / ux-flow collections thread-safe and snapshots them under their own monitor before serializing, and markslastLoadedDomain/profilevolatile.Collections.synchronizedListmakes single operations atomic but not iteration, andblockingErrorsis written from the WebView JavaBridge thread whilefinalizeBlobserializes on the caller's thread — previously a lost-write /ConcurrentModificationExceptionrisk at exactly the moment the data is finalized for upload. The list itself stays append-only and chronological; see the de-dup note above for why. Also makesaddBlockingErrorall-or-nothing with respect to exceptions: its best-effortpersistSessionCorrelationstep now catchesExceptionrather than onlyJSONException, so a failure there (e.g.getSharedPreferencesthrowing on credential-encrypted storage before first unlock) can no longer propagate after the code was already appended. The sink's retraction depends on a throw meaning "not recorded", and this also stops a best-effort persistence failure from propagating into broker4j's blocking-error call sites.Erroris deliberately still allowed to propagate.IOnboardingTelemetryRecorder.java— reconciled theaddBlockingErrorKDoc: the value may be a symbolic constant or a numeric server/STS code (as already produced byOnboardingBlockingErrorParserand now the Auth UX bridge), recorded verbatim as an opaque string.Tests
AzureActiveDirectoryWebViewClientTest(Robolectric, real recorder +finalizeBlob()): valid code → appears inblocking_errors[]+last_blocking_error; excluded code (50058) → not appended but still reported as consumed (a policy drop, not "try again later"), including when no recorder is attached; null recorder → no-op, reported as not consumed; the same code offered twice → recorded once; a throwing recorder propagates rather than being reported as forwarded, and a code whose append threw is retried successfully on a later offer rather than being suppressed as already-forwarded; and the bridge instance the WebView actually binds carries the sink (the regression test for the factory issue above — it fails without the override).OnboardingBlockingErrorParserTest:isNonBlockingOnboardingErrorCodereturns true for50058/50097/50126, false for real CA blocks (530003/53003/65001), false for null/blank.OnboardingTelemetryRecorderTest: the recorder'sblocking_errorslist stays append-only and chronological for A → B → A withlast_blocking_error == A. This pins the contract relied on by broker4j and thex-ms-clitelemparsers, and is why de-duplication lives at the WebView client rather than in the recorder. Also: a persistence failure (aContextwhosegetSharedPreferencesthrows) must not failaddBlockingError— that all-or-nothing property is what makes the sink's retraction safe.Design note — exclusion policy reuse
The team already ships
OnboardingBlockingErrorParser.NON_ONBOARDING_AADSTS_CODES={50058, 50097, 50126}(used by thex-ms-clitelem/ redirecterror_codespaths). Rather than introduce a second list for the bridge, this PR exposes that same set viaisNonBlockingOnboardingErrorCode(...). That keeps iOS parity in one place and avoids drift.Merge order — please read
This PR and #3197 must land together. #3197 alone puts a
log_telemetrypath ondevthat no host wires (every message would validate, then log "no telemetry sink is wired; dropping"); this PR alone doesn't compile, since the bridge it wires into is introduced there.The base branch will be retargeted to
devonce #3197 merges — happy to request a re-approval at that point if the diff changes.Both remain gated on Auth UX actually emitting
log_telemetryserver-side (AB#3696811); until then no real error codes flow. The path was verified end-to-end on a device using a simulated emission — the returned blob carried a real CA block code through the full OneAuth → Broker → OneAuth round trip.Acceptance criteria
AuthUxJavaScriptInterfaceat every registration site, surviving per-navigation re-registration.log_telemetrycode appended to the blob's blocking-errors list and surfaces in the emitted blob (matches theblocking_errors/last_blocking_errorschema).blocking_errorssemantics for other callers.Testing
BUILD SUCCESSFUL —
AzureActiveDirectoryWebViewClientTest93/93,AuthUxJavaScriptInterfaceTest41/41,AuthUxJsonPayloadTest10/10,OnboardingTelemetryRecorderTest18/18. 0 failures.