Remember the UPN across a broker-install interruption to pre-fill login_hint, Fixes AB#3676213 - #3195
Remember the UPN across a broker-install interruption to pre-fill login_hint, Fixes AB#3676213#3195wzhipan wants to merge 22 commits into
Conversation
…in_hint, Fixes AB#3676213 When Conditional Access blocks an interactive request until Company Portal is installed, the server already returns the UPN on the broker-install redirect (msauth://wpj/?username=<upn>&app_link=...), and AuthorizationResultFactory already parses it onto AuthorizationErrorResponse#getUpnToWpj(). It was never used. Installing Company Portal usually kills the calling app, so when the user comes back they are asked to type their address a second time. Add MamUpnHintStore: a small, self-contained store that remembers that UPN in the platform's encrypted (app-private) name-value store so it survives the process death, and pre-fills it as login_hint on the next interactive request. - MamUpnHintStore (common4j) - save / TTL-validated read / clear, plus applyStoredUpnHintIfAbsent() which only fills login_hint when the caller left it blank. Every record carries an absolute expiry (7 minutes, matching the broker-install park TTL); an expired or half-written record is never returned and is deleted in place, so the UPN is not kept at rest longer than the flow needs it. All operations are best-effort - a storage failure is logged and swallowed, never allowed to fail authentication. - LocalMSALController - store the UPN only when authorization actually failed with broker_needs_to_be_installed. - InteractiveTokenCommand - pre-fill login_hint on the way in, and drop the hint once the user is signed in. - Gated by the new CommonFlight ENABLE_BROKER_INSTALL_UPN_HINT (default off): with the flight off nothing is stored, nothing is returned, and no login_hint is ever added, so behavior is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
❌ Work item link check failed. Description does not contain AB#{ID}. Click here to Learn more. |
The first cut hooked LocalMSALController (capture) and InteractiveTokenCommand (pre-fill). OneAuth reaches neither: it parses the authorization result itself in EmbeddedBrowser instead of going through AuthorizationResultFactory, and it calls BrokerMsalController.acquireToken directly rather than dispatching a command. The feature would therefore have been a no-op for the 1P apps it is meant to help. Move both hooks down to code every caller shares: - Capture in AzureActiveDirectoryWebViewClient.processInstallRequest, reading the UPN straight off the msauth://wpj redirect. This runs in the calling app's process for MSAL and OneAuth alike, and does not depend on who parses the result. - Pre-fill (and clear on success) in BrokerMsalController.acquireToken and LocalMSALController.acquireToken, the entry points both stacks share. InteractiveTokenCommand is left alone now that the controllers underneath it are covered. The LocalMSALController capture stays as the MSAL browser/CCT path, which does populate upnToWpj on the error response. Fixes AB#3676213 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reworks the store to the contract in the phased design doc: - Records are keyed by client id, so two clients hosted in the same app cannot read each other's hint. - Reads are single-use: a hint that is handed out is deleted in the same call, so it can never be replayed onto a later, unrelated request. - Each record stores when it was written and validity is decided at read time against the new MamCaUpnHintTtlSeconds flight (180s), so changing the flight also governs records already at rest. Every read sweeps out all expired or half-written records, for every client id. - Storage is gated on the intuneAppProtection marker, so an ordinary device-registration broker install stores nothing. To let the browser/custom-tab path make that same distinction without re-parsing the redirect, AuthorizationErrorResponse now carries whether the install was MAM-CA triggered, populated where the username already is. Also fixes MockFlightsProvider, whose value getters ignored the flights put into it and returned 0/false/null instead of the configured or default value - which would have silently zeroed the TTL under test. Tests: MamUpnHintStoreTest 37, MamCaRedirectTest 7, plus a 358-test sweep of the flighting/providers/commands/controllers/cache/net packages, all passing; :common:compileLocalDebugJavaWithJavac succeeds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
An end-to-end run on a physical device showed the 3-minute default cannot survive its own scenario. Measured timeline: hint stored 10:33:26 (expires 10:36:26) Company Portal installed 10:35:34 (2m08s of download + install alone) user back in the app 10:38:53 -> hint already swept as expired The window has to cover a ~93 MB Play Store download and install, Company Portal's first launch and redirect back, the calling app restarting, and the user reaching their "add account" screen. On a fast Wi-Fi connection the install alone consumed 2m08s of the 3-minute budget; on a metered or slow connection - the common case for onboarding - it would exceed it outright. 15 minutes leaves real headroom while staying bounded. Hints remain single-use and are cleared as soon as they are read, so a hint rarely lives near the TTL. The value stays flight-configurable via MamCaUpnHintTtlSeconds.
github-actions
Bot
changed the title
The service appends intuneAppProtection=1 to the broker-install link when it fails a request with AADSTS50127 for MAM, so the client can rely on the marker being there and no longer needs an escape hatch for redirects that lack it. - Drop ENABLE_MAM_CA_INSTALL_WITHOUT_MARKER. MamCaRedirect.isMamCaInstall is now a pure read of what the service sent, so an ordinary device-registration broker install keeps its existing behavior with no way to opt into MAM-CA behavior by mistake. - Collapse hasIntuneAppProtectionMarker into isMamCaInstall; with the flight gone the two were the same check, and AuthorizationResultFactory now calls the one that remains. - saveUpnHintForMamCaInstall(AuthorizationErrorResponse) drops its flight fallback and simply requires isMamCaInstall(). - Document that the marker is a top-level query parameter appended after app_link rather than something nested inside the app_link value, and cover that shape in MamCaRedirectTest. MamCaRedirectTest 5, MamUpnHintStoreTest 36, all passing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Installing the broker does not reliably kill the calling app. When the process survives, the user returns to an already-created screen, so a hint read wired to process or view creation never runs and the field is left empty. Reading on every presentation is safe because the hint is single-use and self-clearing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Handling the broker-install redirect finishes the authorization activity before the store listing is launched, which briefly resumes the calling app's own account screen while the flow is still in progress. A host SDK that reads the hint when that screen is shown - which is exactly what the API tells it to do, because installing the broker does not reliably kill the caller - therefore read it tens of milliseconds after it was written. The read spent the hint, so it was destroyed long before the app restart it exists to survive. The failure was invisible on the surviving-process path (the field kept the text in memory) and showed up only when the app really was killed, where the field came back empty. Reading no longer spends the hint. It is retired where it is genuinely consumed - applyStoredUpnHintIfAbsent, once it has been attached to a request - and bounded by its TTL either way, so a pre-fill read is now safe to repeat on every presentation of the account screen. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reads stopped being destructive when the hint was made to survive the Company Portal install, but two pieces of prose still described the old behaviour: the class javadoc claimed a hint is deleted in the same call that hands it out, and every write logged that the hint was usable "only once". Both now describe what the code actually does - the hint is spent when it is carried into a request. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The 15-minute default was sized off a manual run that expired a 3-minute hint, but that run is not representative: most of the elapsed time was a human stepping through the UI and stopping to read logs, not the flow. The machine-paced parts - Play capturing the referrer, the install finishing, Company Portal redirecting back, the app restarting - fit inside three minutes, which is also what the design calls for. Erring short is the cheaper mistake: a missed hint just means the user types their address, which is today's behaviour, while erring long leaves an address on disk for no benefit. The value stays flighted, so it can be raised through ECS without shipping code if telemetry disagrees. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review pass over PR #3195. TTL was only enforced as a side effect of the expiry sweep: the read swept, then returned whatever `get` produced without checking that record's own timestamp. That is correct only for as long as the sweep can enumerate every client id via `getAll()`. Against a storage implementation with a partial or empty listing an expired UPN would have been handed out with no TTL check at all. The read now validates the record it is about to return, so the method is correct in isolation rather than by cooperation. `read_whenTheStoreCannotEnumerateItself_stillEnforcesTheTtl` pins this against a store whose `getAll()` returns nothing; it fails against the previous code, returning the expired address. Also corrects two comments left stale by the earlier switch away from destructive reads, and shortens the changelog entry, which was the longest in the section by some margin. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Behavioral fixes: - Do not inject login_hint when the caller asked for the account picker or to create an account. BaseController suppresses the picker whenever a hint is present, and on the broker path the hint becomes the account-resolution field, so injecting a remembered address silently answered a question the caller explicitly wanted to put to the user. - Sweep expired records before consulting the flight rather than after. Expiry is read-driven, so gating the sweep meant turning the flight off - the kill switch - stranded every UPN already on disk permanently. - Stop spending the hint when it is applied. It was retired at acquireToken entry, before parameter validation or the network check could throw, so a flaky network right after a ~100 MB install destroyed the hint in exactly the scenario the feature exists for. It is now retired on a successful sign-in (LocalMSALController gains the clear that BrokerMsalController already had), by an explicit clear, or by its TTL. Robustness: - Write the UPN before the timestamp. A tear between the two writes now pairs the new UPN with the old timestamp (expires early) rather than the old UPN with a fresh one (wrong address, clock restarted). - Reject a negative written_at, which would overflow the subtraction and read as "written moments ago" forever. - Do not store under the placeholder client id; no reader ever asks for it. - Swallow storage failures on write and clear, as the read path already did. - Narrow the three-arg saveUpnHint to package-private; it has no external callers. Logging: - Filter redirect parameter names through a name-shaped pattern before logging them. The query parser turns a trailing token with no '=' into a key, so a malformed redirect could put a UPN on the non-PII channel. Tests: 43 in MamUpnHintStoreTest, 6 in MamCaRedirectTest, 269 across the providers/controllers/commands/flighting packages. The three new guards were negative-tested - each fails against the pre-fix code, and nothing else does. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The query parser turns a trailing token with no '=' into a key with a null value, so a malformed broker-install redirect ending in a bare '?user@contoso.com' would put the UPN itself on the non-PII log channel. Only keys matching an ordinary parameter-name shape are printed now; the rest are counted. Identical to the change on copilot/mam-upn-hint-store (#3195), which carries the same file, so the two branches stay conflict-free. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keying records by client id stays: "one process, one client id" does not hold. A broker hosts the authorization WebView in its own process (BrokerAuthorizationActivity, process :auth) on behalf of every calling app, so that one store sees every client id on the device; and the credential cache already keys by client id for the same reason. The placeholder key, though, became unreachable when the previous commit made the write path refuse a missing client id. Nothing can be stored under it, so reading or clearing through it only ever addressed an empty bucket. Reads and clears without a client id now return early instead, which says the same thing more directly and removes the shared-key concept - a shared key is exactly the cross-client bleed the keying exists to prevent. Replaces save_nullClientId_isKeyedConsistently, which pinned the removed behavior, with readAndClear_withoutAClientId_areSafeNoOps. Negative-tested: it fails if a missing client id is treated as "all records". Tests: 43 MamUpnHintStoreTest, 6 MamCaRedirectTest, 269 across providers/controllers/commands/flighting. :common compiles. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Review feedback: new classes should be Kotlin. Converted MamCaRedirect and both test classes. No behavior change - the assertions are carried over verbatim, so the tests still pin the same contract. MamUpnHintStore stays Java. In common4j compileKotlin runs before compileJava, so Lombok has not generated its accessors yet and Kotlin cannot see them: Unresolved reference: isMamCaInstall / upnToWpj / toBuilder Cannot access 'loginHint': it is invisible (private in a supertype) Cannot access 'prompt' / 'platformComponents' / 'clientId' Reading those generated members of InteractiveTokenCommandParameters and AuthorizationErrorResponse is precisely what applyStoredUpnHintIfAbsent and the error-response overload exist to do, so it cannot be factored out. Unblocking it needs org.jetbrains.kotlin.plugin.lombok added to common4j, which is a module-wide build change and belongs in its own PR. The reason is recorded above the class so nobody re-attempts it blind. The tests convert cleanly because compileTestKotlin runs after main's Java, so by then the Lombok accessors are real methods on the jar. Note MamCaRedirect.kt is kept byte-identical with the copy on copilot/mam-referrer-phase1 so the two PRs still merge without conflict. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A stored UPN is only known to the authority that returned it on the broker-install redirect. Without a binding, any interactive request made by the same app inside the TTL would carry that address as login_hint -- including a request to a different sovereign cloud, which would disclose a user's address to a service that never had it. Records now carry the authority host they were captured against, and applyStoredUpnHintIfAbsent only attaches the hint when the request's authority host matches. Binding is on the host rather than the whole url, because the capture happens against /common and the request that follows is against the resolved tenant -- url-level binding would break the exact flow this feature ships for. A mismatch declines the hint but does not delete it: an unrelated request to another authority during the install detour must not wipe the hint the user is coming back for. The record is now written as one JSON value under a single key. Three fields only mean anything together, and a torn write could otherwise pair an old UPN with a new host -- the precise leak this prevents. The sweep drops anything that is not a readable record, so residue from the earlier two-key format is cleaned up on sight. The user-visible pre-fill (getValidUpnHint) is deliberately left unbound: it fills a local text box the user can edit, at a point in the flow where there is no authority to check against yet. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
getValidUpnHint(storage, clientId, now, ttl) had no production callers - it existed only so tests could inject a clock, which getValidRecord already does. Two seams for one thing is surface a reviewer has to reason about for no benefit, so the tests now read through the same method production does. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a flight-gated mechanism in Common/Common4j to persist a MAM Conditional Access broker-install redirect’s UPN across process death and automatically reapply it as login_hint (with TTL + authority-host binding) to pre-fill the next interactive sign-in.
Changes:
- Introduces
MamUpnHintStore(encrypted storage, per-client keying, TTL enforcement, authority-host binding) and integrates capture/apply/clear across WebView and controllers. - Adds
MamCaRedirectand propagates the “MAM-CA install” marker intoAuthorizationErrorResponsefor the browser/CCT path. - Adds extensive unit test coverage and a vNext changelog entry; extends test
MockFlightsProviderto return configured values.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| common4j/src/main/com/microsoft/identity/common/java/providers/MamUpnHintStore.java | New persisted UPN-hint store with TTL, authority-host binding, and request-parameter injection. |
| common4j/src/main/com/microsoft/identity/common/java/providers/MamCaRedirect.kt | New helper to detect MAM-CA redirects and safely log parameter names only. |
| common4j/src/main/com/microsoft/identity/common/java/providers/oauth2/AuthorizationResultFactory.java | Populates mamCaInstall on AuthorizationErrorResponse based on redirect params. |
| common4j/src/main/com/microsoft/identity/common/java/providers/oauth2/AuthorizationErrorResponse.java | Adds mMamCaInstall flag (Lombok accessors) for MAM-CA install detection. |
| common4j/src/main/com/microsoft/identity/common/java/flighting/CommonFlight.java | Adds ENABLE_MAM_CA_UPN_HINT and MAM_CA_UPN_HINT_TTL_SECONDS flights. |
| common/src/main/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java | Captures and persists UPN hint on broker-install redirect handling. |
| common/src/main/java/com/microsoft/identity/common/internal/providers/oauth2/WebViewAuthorizationFragment.java | Extracts client_id and authority host from request URL and passes to WebView client. |
| common/src/main/java/com/microsoft/identity/common/internal/controllers/LocalMSALController.java | Applies stored hint to interactive params and clears on successful interactive sign-in. |
| common/src/main/java/com/microsoft/identity/common/internal/controllers/BrokerMsalController.java | Applies stored hint to interactive params and clears on successful interactive sign-in. |
| common4j/src/test/com/microsoft/identity/common/java/providers/MamUpnHintStoreTest.kt | New comprehensive unit tests for persistence/TTL/sweep/client scoping/authority binding/injection. |
| common4j/src/test/com/microsoft/identity/common/java/providers/MamCaRedirectTest.kt | New unit tests for marker parsing and PII-safe logging behavior. |
| common4j/src/test/com/microsoft/identity/common/java/flighting/MockFlightsProvider.java | Updates mock to return stored/default values for boolean/int/double/string flight reads. |
| changelog.txt | Adds vNext entry documenting the new flight-gated behavior. |
Comments suppressed due to low confidence (1)
common4j/src/main/com/microsoft/identity/common/java/providers/MamUpnHintStore.java:293
- Javadoc says applyStoredUpnHintIfAbsent "clears" the hint once attached to a request, but the implementation is deliberately non-destructive (clearing happens on success / explicit clear / TTL). This documentation inconsistency makes the lifetime semantics easy to misinterpret.
* The hint is retired instead when it is actually used: {@link #applyStoredUpnHintIfAbsent}
* clears it once it has been attached to a request, and its TTL bounds it either way. Callers
* that pre-fill their own UI may call {@link #clearUpnHint} once the user commits.
The `Compare Code Coverage PR VS. Dev` check flags this PR because it adds Android-module glue with no unit tests, while the well-tested Kotlin store lives in common4j (a separate coverage report). This adds tests for that glue. `AzureActiveDirectoryWebViewClientTest` (+4): pins that `processInstallRequest` offers every broker-install redirect to `MamUpnHintStore`, passing the client id, the authority host and the whole redirect parameter map, and that a non-broker-install redirect never reaches the store at all. These assert on the delegation rather than on stored state, because the encrypted name-value store does not round-trip under Robolectric in this module: `getEncryptedNameValueStore(...)` returns a store, but a `put` followed by a `get` reads back null. `MamUpnHintStore` swallows storage failures by design, so that is silent. The store's own behaviour -- the flight gate, the marker gate, per-client-id scoping, TTL and authority binding -- is covered by the 51 `MamUpnHintStoreTest` cases in common4j, which run against an in-memory storage. Splitting it this way keeps each layer tested where it can be. New `WebViewAuthorizationFragmentUrlHelpersTest` (10): the two request-URL readers, including a sovereign-cloud host, a missing parameter, a URL with no query, and an opaque URI -- the last one covers the `UnsupportedOperationException` that `Uri.getQueryParameter` throws on a non-hierarchical URI, which is the reason those catch blocks exist. To make those two readers testable they are now `@VisibleForTesting static` and take the request URL as a parameter instead of reading the field. No behaviour change; the two call sites pass `mAuthorizationRequestUrl`. Each new test was negative-tested: removing the save call fails exactly the three tests that verify it and nothing else; moving it above the `BROKER_INSTALLATION_TRIGGERED` early return fails exactly the test that asserts a non-broker-install is left alone; making both URL readers return null fails exactly the three positive assertions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two review comments on #3195 pointed out that the documentation claims the hint is consumed when it is applied to a request. That is no longer true: the clear used to run inside applyStoredUpnHintIfAbsent at acquireToken entry, which meant a request that failed validation or a network check destroyed the hint in exactly the flow the feature exists for (right after a ~100 MB Company Portal install). It was moved onto the successful sign-in path, but these three spots were not updated with it. The file already contradicted itself - the class javadoc and the javadoc on applyStoredUpnHintIfAbsent both describe the current behaviour correctly. This aligns the remaining three with them. - MamUpnHintStore: the save log said "until it is carried into a request"; it now says "or until sign-in succeeds". - MamUpnHintStore: the getValidUpnHint javadoc said applyStoredUpnHintIfAbsent clears the hint. It now states the three real retirement paths and says explicitly that applying does not delete, because the request can fail. - BrokerMsalController: dropped the false parenthetical claiming the hint was already retired when applied; the comment now says this is the retirement point. - LocalMSALController: not flagged by the reviewer, but it is the other retirement site and carried a terser version of the same comment. Aligned the two so they read identically. Documentation and one log string only - no behaviour change. MamUpnHintStoreTest 51/51; :common4j:compileJava and :common:compileLocalDebugJavaWithJavac both successful. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| import lombok.NonNull; | ||
|
|
||
| /** | ||
| * Remembers the UPN across a MAM Conditional-Access "install Company Portal" interruption, so that |
There was a problem hiding this comment.
nit: The java doc is verbose. Can be simplified capturing only essential details
There was a problem hiding this comment.
Fair - trimmed in 0893d56ad to a short intro plus bullets (lifetime / sweeping / keying / authority binding / flighting / telemetry). Rationale kept, prose dropped.
| @Nullable final String authorityHost, | ||
| @Nullable final Map<String, String> redirectParameters) { | ||
| if (!isEnabled() || !MamCaRedirect.isMamCaInstall(redirectParameters)) { | ||
| return; |
There was a problem hiding this comment.
The two skip paths return silently. During rollout/triage, "flight off", "not a MAM-CA redirect", and "never called" look identical in logs. Add a verbose log per branch (verbose, not info — this fires on every non-MAM-CA broker install):
final String methodTag = TAG + ":saveUpnHintForMamCaInstall";
if (!isEnabled()) {
Logger.verbose(methodTag, "Skipped: flight disabled.");
return;
}
if (!MamCaRedirect.isMamCaInstall(redirectParameters)) {
Logger.verbose(methodTag, "Skipped: redirect is not MAM-CA.");
return;
}Same treatment for the AuthorizationErrorResponse overload below.
There was a problem hiding this comment.
Added in 0893d56ad - each branch now logs which it was, so "flight off" and "not the MAM-CA path" are distinguishable in a capture.
| || !MicrosoftAuthorizationErrorResponse.BROKER_NEEDS_TO_BE_INSTALLED | ||
| .equals(errorResponse.getError())) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Same as the map overload above: the three skip paths (flight off, non-broker-install error, non-MAM-CA install) all return silently, which makes rollout/triage harder. Add a verbose log per branch:
final String methodTag = TAG + ":saveUpnHintForMamCaInstall";
if (!isEnabled()) {
Logger.verbose(methodTag, "Skipped: flight disabled.");
return;
}
if (errorResponse == null
|| !MicrosoftAuthorizationErrorResponse.BROKER_NEEDS_TO_BE_INSTALLED
.equals(errorResponse.getError())) {
Logger.verbose(methodTag, "Skipped: error response is not a broker-install.");
return;
}
if (!errorResponse.isMamCaInstall()) {
Logger.verbose(methodTag, "Skipped: broker install is not MAM-CA.");
return;
}(This overload doesn't currently check isEnabled() — worth adding for consistency and to match the map overload; saveUpnHint already gates on it, so behavior is unchanged.)
There was a problem hiding this comment.
Same fix in 0893d56ad. This one also surfaced a missing isEnabled() gate on the overload - behaviour was already correct since saveUpnHint gates too, but the two overloads now read alike.
| } | ||
|
|
||
| if (!errorResponse.isMamCaInstall()) { | ||
| // An ordinary device-registration broker install; nothing to remember. |
There was a problem hiding this comment.
Add a verbose log before this return so an ordinary device-registration broker install is distinguishable from "never entered this path" in logs:
Logger.verbose(TAG + ":saveUpnHintForMamCaInstall",
"Skipped: broker install is not MAM-CA (ordinary device registration).");
return;There was a problem hiding this comment.
Added in 0893d56ad - an ordinary device-registration broker install now says so explicitly.
… outcome Two review comments on #3193. 1. Comment on the "link already names a referrer" guard. Added the requested comment, but with a corrected rationale. The concern raised was that addParameterIfAbsent may be case-sensitive; it is not. CommonURIBuilder.addParameterIfAbsent delegates to containsParam, which compares with equalsIgnoreCase, so both paths already agree on a mixed-case "Referrer". Writing the suggested reason down would have baked a falsehood into the source. What the explicit branch actually earns is that we hand back the caller's original string instead of a re-serialised build(), and that we log "left it alone" rather than "tagged it". The comment says that. 2. Should the feature emit a metric while the flight is on? Yes - implemented here rather than deferred, because it needs no schema change. MamInstallReferrerBuilder now reports one of seven outcomes (FLIGHT_OFF, NOT_MAM_CA, NO_ORIGIN_PKG, NO_APP_LINK, SERVER_REFERRER, LINK_UNPARSEABLE, DECORATED) and AzureActiveDirectoryWebViewClient forwards the reportable ones to the existing OnboardingTelemetryRecorder.addUxFlowUsed, which surfaces as mo_ux_flow_used. That answers both ramp questions from Kusto: how often a marked redirect reaches the decoration, and whether the server already supplied a referrer. FLIGHT_OFF carries a null tag so nothing is reported at all when the flight is off. That keeps the earlier commitment that a disabled flight leaves no trace, now extended to telemetry, and it is enforced by a test rather than by convention. The marker check now runs before the origin-package check. Both branches return the link unchanged, so this is reporting-only: a missing package on a marked redirect is now visible as such instead of being masked as "not MAM-CA". Only the embedded-WebView path has a telemetry recorder. The browser fragments stay log-only, which is acceptable because MAM-CA onboarding runs through the WebView, but it is a real gap worth naming. Tests: MamInstallReferrerBuilderTest 20/20 (was 15), MamCaRedirectTest 7/7, AzureActiveDirectoryWebViewClientTest 92/92 (was 87), AuthorizationFragmentInstallReferrerTest 6/6. Negative-tested by giving FLIGHT_OFF a non-null tag: the three tests that pin the silent-when-off guarantee failed as intended, and nothing else moved. Public API unchanged - decorateAppLinkForMamCaInstall keeps its signature and delegates, so AuthorizationFragment and both browser fragments are untouched. MamCaRedirect.kt is deliberately not modified; it stays byte-identical with #3195 so the two PRs merge in either order. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| * @param upn the UPN to remember. | ||
| * @param authorityHost host of the authority the hint came from, if it could be determined. | ||
| */ | ||
| static void saveUpnHint(@Nullable final IPlatformComponents components, |
There was a problem hiding this comment.
saveUpnHint is package-private so MamUpnHintStoreTest.kt (lines 868, 875) can call it directly. That's fine, but nothing marks the intent — a future caller in the same package could bypass the two public saveUpnHintForMamCaInstall entry points and skip the MAM-CA redirect check.
Annotate with @VisibleForTesting to make the contract explicit:
@VisibleForTesting
static void saveUpnHint(@Nullable final IPlatformComponents components, ...Same for the INameValueStorage overload at line 265.
There was a problem hiding this comment.
Added in 0893d56ad as //@VisibleForTesting. The annotation is not importable in common4j (no androidx/guava dependency) - every occurrence in src/main is commented out, so this follows the module's existing form.
| * @param authorityHost host of the authority the hint came from, already normalized. | ||
| * @param nowMillis the current time in epoch millis. | ||
| */ | ||
| static void saveUpnHint(@NonNull final INameValueStorage<String> storage, |
There was a problem hiding this comment.
Nit (optional): both saveUpnHint overloads share a name, which is fine for overloading but slightly hides intent — the 4-arg is the gated entry point, this 5-arg is the raw atomic write with an injected clock and storage. Consider renaming this one to writeRecord (or putRecord) so the atomic-write invariant documented above is easier to find from call sites, and the two responsibilities are visually distinct.
Not a blocker — the file already uses the same overload shape for clearUpnHint, so staying consistent is defensible.
There was a problem hiding this comment.
Leaving as-is. The name is symmetric with clearUpnHint, which has the same two-overload shape, and I would rather not break that pairing for the storage-level variant alone.
| // Read first, so that the sweep still runs even when the hint turns out not to be applicable. | ||
| final Record record = readValidRecord(parameters.getPlatformComponents(), parameters.getClientId()); | ||
| if (record == null || StringUtil.isNullOrEmpty(record.getUpn())) { | ||
| return parameters; |
There was a problem hiding this comment.
Every other skip branch in applyStoredUpnHintIfAbsent logs (prompt gate, missing host, host mismatch) — this null-record branch is the only silent one. During triage of "why didn't the pre-fill happen?" that's a blind spot.
This path fires on every interactive acquireToken without a stored hint, so info-level would be spam. Add a verbose log:
if (record == null || StringUtil.isNullOrEmpty(record.getUpn())) {
Logger.verbose(methodTag, "Not pre-filling login_hint: no usable UPN hint stored.");
return parameters;
}Distinguishing why readValidRecord returned null (flight off vs. no record vs. expired vs. storage unavailable) would need per-branch logs inside readValidRecord; not worth it here, since the overwhelmingly common case is just "no hint stored".
There was a problem hiding this comment.
Skipping this one per your follow-up below - added the log in getValidRecord instead.
| // Re-check this record rather than trusting the sweep to have removed it: the sweep depends | ||
| // on the store being able to enumerate itself, and an expired UPN must never be handed out | ||
| // just because a listing came back short. | ||
| if (!isUsable(record, nowMillis, ttlMillis)) { |
There was a problem hiding this comment.
This return null is the only silent decline in the read/apply chain — every other branch (applyStoredUpnHintIfAbsent prompt gate, missing host, host mismatch) logs. During triage of "why didn't the pre-fill happen?" this is a blind spot: no signal to distinguish an empty store from an expired/corrupt record.
Add a single verbose log — no per-reason breakdown, since isUsable should stay a pure predicate and the aggregate sweep at line 417 already reports discards:
if (!isUsable(record, nowMillis, ttlMillis)) {
Logger.verbose(TAG + ":getValidRecord", "No usable stored record for this client id.");
return null;
}If you take this, you can drop my earlier suggestion at line 547 — the log belongs here, and a duplicate at the caller would be noise. Verbose (not info) because this fires on every acquireToken in apps with no stored hint, which is the vast majority.
There was a problem hiding this comment.
Added in 0893d56ad - getValidRecord now logs the miss, so "no record" and "record present but unusable" are distinguishable.
| final long ttlMillis) { | ||
| // Sweep first, so a hint that is itself stale is dropped rather than returned below. This | ||
| // runs before the client-id check because it is not scoped to one client. | ||
| sweepUnusableRecords(storage, nowMillis, ttlMillis); |
There was a problem hiding this comment.
Duplicate sweep on the production read path. readValidRecord at line 340 already calls sweepUnusableRecords(...) before delegating here, so on every applyStoredUpnHintIfAbsent / getValidUpnHint call the store is enumerated and deserialized twice back-to-back with the same clock/TTL. Not a correctness bug — sweeping twice is idempotent — but wasteful, and non-obvious to future readers.
The tension is that getValidRecord is package-private and called directly by tests, so it can't assume a caller swept first. Two ways to close this without breaking the seam:
Option A (minimal): add a short comment here noting the production path double-sweeps by design, so the next reader doesn't try to "fix" one of the calls.
Option B (cleaner): drop the sweep from getValidRecord, keep the one in readValidRecord, and have tests call sweepUnusableRecords explicitly when they need it. Storage-level helpers stay pure; production stays single-sweep.
Leaning toward B — the sweep isn't logically part of "get a valid record for this client id", it's store hygiene. But A is fine if you'd rather not touch the tests.
There was a problem hiding this comment.
Agreed - same as the note above, fixed in 0893d56ad. Single sweep in readValidRecord; getValidRecord is now a pure read. New test getValidRecord_doesNotSweep.
| // out. Converting this file needs the org.jetbrains.kotlin.plugin.lombok compiler plugin added to | ||
| // common4j first; that is a build change for the whole module, not for a feature PR. The tests are | ||
| // Kotlin because compileTestKotlin runs after main's Java and therefore does see those accessors. | ||
| public final class MamUpnHintStore { |
There was a problem hiding this comment.
Severity: Medium – No telemetry for a flight-gated feature.
Issue: The store is instrumented entirely via Logger. No counters or attributes on any span.
Impact: Log grep doesn't roll up into a dashboard. Ramp decisions for a flight-gated feature need fleet-level counts of apply / decline / save-failure outcomes, and the counterfactual on the flight-off population — none of which are available today.
Recommendation: Attach a bounded-cardinality outcome attribute to the existing command-execution span from applyStoredUpnHintIfAbsent and from the controllers'' clearUpnHint call sites. Emit on the flight-off path too, so the counterfactual is measurable.
Follow-up (per §6.5 / §14.4): any new AttributeName must also be mirrored in the Broker repo''s AttributeName.java — Broker''s exporter drops unknown attribute names. Separate PR.
There was a problem hiding this comment.
Added in 0893d56ad. applyStoredUpnHintIfAbsent now tags the request's span with mam_ca_upn_hint_outcome, one of five fixed values: applied, none_stored, declined_prompt, declined_authority_mismatch, declined_no_request_host.
Two deliberate omissions, both documented at reportOutcome:
- Nothing is reported when the caller supplied its own
login_hint- that is the ordinary case for most callers and would drown the signal. - The flight-off counterfactual is not obtainable. Writes are gated by the same flight, so with it off no hint is ever stored and there is nothing to have counted; making it measurable would mean persisting UPNs for users outside the experiment.
Mirroring the new AttributeName constant into Broker is a separate change, as you noted.
| public AcquireTokenResult acquireToken(final @NonNull InteractiveTokenCommandParameters parameters) | ||
| public AcquireTokenResult acquireToken(final @NonNull InteractiveTokenCommandParameters requestParameters) | ||
| throws BaseException, InterruptedException, ExecutionException { | ||
| // MAM Conditional Access onboarding: this is the request the user makes after installing the |
There was a problem hiding this comment.
is this comment accurate? the same API can be invoked during when Broker already installed right?
There was a problem hiding this comment.
You are right - corrected in 0893d56ad. The comment now says this runs on every interactive request, and is a no-op unless the flight is on, login_hint is blank, and a hint stored for this client and authority is still within its TTL.
| throws ExecutionException, InterruptedException, ClientException, IOException, ArgumentException { | ||
| final String methodTag = TAG + ":acquireToken"; | ||
|
|
||
| // MAM broker-install onboarding: if this request was interrupted earlier by a Conditional |
There was a problem hiding this comment.
Same correction in 0893d56ad.
| ResultUtil.logResult(TAG, result); | ||
|
|
||
| // MAM Conditional Access onboarding: if Conditional Access blocked this request until | ||
| // Company Portal is installed, remember the UPN the server sent back. Installing the broker |
There was a problem hiding this comment.
"Installing the broker" - is this only for CP or all brokers?
There was a problem hiding this comment.
Only Company Portal - corrected in 0893d56ad. The save is gated on the MAM-CA install specifically; an ordinary device-registration broker install stores nothing.
| final long ttlMillis) { | ||
| // Sweep first, so a hint that is itself stale is dropped rather than returned below. This | ||
| // runs before the client-id check because it is not scoped to one client. | ||
| sweepUnusableRecords(storage, nowMillis, ttlMillis); |
There was a problem hiding this comment.
readValidRecord already sweeps on 340 right before it gets here, so with the flight on every read enumerates and decrypts the whole store twice back to back. getValidRecord sweeping makes sense for the tests that call it directly, but on the real read path everything comes through readValidRecord. Could we drop the sweep here and let readValidRecord own it?
There was a problem hiding this comment.
Right - fixed in 0893d56ad. The sweep now happens once, in readValidRecord, which also reads the clock and TTL once and passes both down; getValidRecord no longer sweeps.
It still re-checks the record it finds, since the sweep depends on the store being able to enumerate itself and an expired UPN must not leak out on a short listing. Pinned by getValidRecord_doesNotSweep.
| // as the MAM-CA path. | ||
| final Activity installRequestActivity = getActivity(); | ||
| if (installRequestActivity != null) { | ||
| MamUpnHintStore.saveUpnHintForMamCaInstall( |
There was a problem hiding this comment.
This is on the UI thread right? shouldOverrideUrlLoading -> handleUrl -> processInstallRequest. Opening the encrypted store and the put both hit disk here, inline as we handle the redirect. Probably fine since the activity's about to be torn down for the Play Store hop, but did we think about doing the store write off the main thread?
There was a problem hiding this comment.
Yes, UI thread - and kept that way deliberately. Added a comment in 0893d56ad explaining why: this is the last moment the process is guaranteed alive (the Company Portal install usually kills us), and it is a single encrypted put with every failure caught inside the store. Handing it to a background thread would risk losing the hint in exactly the case it exists for.
Reviewer comments on #3195, taken in full except where noted below. Sweep once per read, not twice. readValidRecord swept the store and then called getValidRecord, which swept it again - so every read enumerated and deserialized the whole store twice. The sweep now happens only in readValidRecord, which also reads the clock and the TTL once and passes both down. getValidRecord still judges the record it finds on its own terms rather than trusting the sweep, because the sweep depends on the store being able to enumerate itself and an expired UPN must never be handed out just because a listing came back short. Pinned by getValidRecord_doesNotSweep. Log why a save was skipped. Both saveUpnHintForMamCaInstall overloads returned silently, so an on-device capture could not tell a flight that was off from a redirect that was not the MAM-CA path. Each branch now says which it was. This also surfaced a missing isEnabled() gate on the AuthorizationErrorResponse overload - behaviour was already correct, since saveUpnHint gates too, but the overloads now read the same way. Log when a read finds nothing, so the store's two silent outcomes - no record and an unusable one - are distinguishable in a capture. Mark the package-private seams //@VisibleForTesting. The annotation itself is not importable in common4j - the module has no androidx or guava dependency, and every existing occurrence in src/main is commented out - so this follows the module's established form. Trim the class javadoc to an intro plus five bullets. All of the rationale is still there, just no longer in prose. Report the outcome to telemetry. applyStoredUpnHintIfAbsent now tags the request's span with mam_ca_upn_hint_outcome, one of five fixed values, so the pre-fill's reach can be measured as the flight ramps instead of inferred from logs. Two deliberate omissions, both documented at reportOutcome: nothing is reported when the caller supplied its own login_hint, which is the ordinary case and would drown the signal; and the flight-off counterfactual is not attempted, because writes are gated by the same flight, so with it off no hint is ever stored and there is nothing to have counted - making it observable would mean persisting UPNs for users who are not in the experiment. Mirroring the new AttributeName constant into Broker is a separate change. Correct three comments that overstated what the code does: the controller pre-fill runs on every interactive request, not only the one after an install, and the LocalMSALController save is specific to the MAM-CA install rather than to broker installs generally. Record why the WebView store write is inline on the UI thread: it is the last moment the process is guaranteed alive, and the write is a single encrypted put with every failure caught inside the store. Not taken: renaming saveUpnHint to writeRecord. The reviewer marked it optional and gave the counter-argument, and the current name is symmetric with clearUpnHint's identical overload shape. MamUpnHintStoreTest 54, MamCaRedirectTest 7, AzureActiveDirectoryWebViewClientTest 86, all passing. Both new assertions negative-tested: colliding two outcome values fails only outcomeValues_areDistinctAndNonBlank, and splicing the sweep back into getValidRecord fails only getValidRecord_doesNotSweep. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
|
||
| // MAM broker-install onboarding: pre-fill the UPN the user gave us before a Conditional | ||
| // Access "install Company Portal" block interrupted them. This runs on every interactive | ||
| // request, not only the one that follows an install - it is a no-op unless the flight is on, |
There was a problem hiding this comment.
Minor doc accuracy nit: the "no-op unless (a) flight on, (b) login_hint blank, (c) valid hint stored for this client and authority" list reads as sufficient, but applyStoredUpnHintIfAbsent also declines when prompt is SELECT_ACCOUNT or CREATE (MamUpnHintStore.java:571-575), even with all three conditions holding. A reader following the comment could be surprised when a request with prompt=SELECT_ACCOUNT isn''t pre-filled despite the setup looking right.
Suggested phrasing:
"... unless the flight is on, the caller left
login_hintblank, the caller did not ask the user to pick or create an account, and a hint stored for this client and authority is still within its TTL."
Same fix in BrokerMsalController at line 357.
Related (optional): the prompt-decline rule used to live in the class-level Javadoc of MamUpnHintStore and was removed in the "trim to intro + five bullets" cleanup. It''s now only visible by reading applyStoredUpnHintIfAbsent''s body. Consider adding a short bullet or a line under the Authority-binding bullet noting the prompt exclusion, so applyStoredUpnHintIfAbsent''s contract is discoverable from the class Javadoc.
There was a problem hiding this comment.
Agreed on all three. Taking your phrasing for both controllers, and restoring the prompt rule to the class Javadoc.
One addition: the line that was trimmed only mentioned "choose", so the restored bullet will cover CREATE as well.
| * Name of the (encrypted) name-value store backing the hint. Deliberately its own store so the | ||
| * whole feature can be reasoned about - and removed - independently of the token cache. | ||
| */ | ||
| static final String STORE_NAME = "com.microsoft.identity.common.broker_install_upn_hint"; |
There was a problem hiding this comment.
Nit for consistency: STORE_NAME is package-private because MamUpnHintStoreTest reads it directly (e.g. MamUpnHintStoreTest.kt:444, :963) to seed the underlying store with expired/corrupt records. That''s the same reason the follow-up commit added //@VisibleForTesting on the other seams in this file - this constant is the one seam that was skipped. Consider adding the marker here too so the widened visibility is self-documenting.
There was a problem hiding this comment.
Agreed - adding it.
KEY_PREFIX_RECORD just below is the same case (tests read it the same way), so marking both rather than leaving the next one inconsistent.
| final String methodTag = TAG + ":saveUpnHint"; | ||
|
|
||
| if (!isEnabled() || StringUtil.isNullOrEmpty(upn)) { | ||
| return; |
There was a problem hiding this comment.
Two remaining silent skip paths worth logging, to match the follow-up commit''s "every skip explains itself" pass:
-
saveUpnHintline 251 - whenupnis null/blank, we return silently. Both production callers reach this line only after they''ve confirmed the server marked the redirect as MAM-CA, so a blank UPN here means the marker was present without an accompanying UPN - a server-contract oddity worth a verbose log. (The!isEnabled()half of the same condition is fine to stay silent; the outer overloads already logged it.) -
getStorageline 676 - thecomponents == nullbranch returns null silently. The exception branch below already warns; only this branch is quiet. A single verbose log here coverssaveUpnHint,readValidRecord, andclearUpnHintin one place, without duplicating logs at each caller.
There was a problem hiding this comment.
Both agreed.
Your read of the blank-UPN case is right: the public overloads log the flight and the MAM-CA gates before this line, so reaching it with a blank UPN means the marker arrived without a UPN - worth recording. Keeping the !isEnabled() half silent as you suggest, and putting the getStorage log in the one place rather than at each caller.
|
Overall LGTM - follow-up commit addressed all earlier feedback cleanly. Invariants hold end-to-end; |
- The prompt exclusion was documented on the class but had been dropped from the bullet list; restored as its own bullet covering select_account and create, and named in the two controller comments that summarise when the pre-fill is a no-op. - STORE_NAME and KEY_PREFIX_RECORD are reached from tests only; marked with the module's commented //@VisibleForTesting idiom like their nine siblings. - saveUpnHint returned silently on a blank UPN. The callers have already established the redirect was marked MAM-CA at that point, so a missing UPN is a server-contract oddity worth a line. The flight-off half stays silent - the public overloads already log it. - getStorage warned when opening the store threw but said nothing when it was handed no platform components; both now leave a trace. Documentation and logging only; no behaviour change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…(flight-gated), Fixes AB#3676213 (#3193) ## Summary **Phase 1** of "Improve MAM onboarding on Android" (PBI [AB#3686094](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3686094), Feature [AB#3676213](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3676213)). When an interactive request is interrupted by a Conditional-Access **"install Company Portal"** response, tag the Play Store install link with the calling app's package as the Play **install referrer**. Company Portal reads it and, after install, sends the user **back to the app they started in** instead of stranding them in Company Portal. Redirect-only. No park / auto-resume — that is Phase 2. ## What changed Everything is in Common and gated on `CommonFlight.ENABLE_MAM_CA_INSTALL_REFERRER` (**default off**). **common4j** - New `MamCaRedirect` — the single place that decides whether a broker-install redirect is the MAM-CA variety. Looks for the `intuneAppProtection` marker on the redirect, exposes `getUsername()`, and provides a PII-safe `logRedirectParameterNames()` — names only, never values, and only names that are actually shaped like parameter names (the query parser turns a trailing token with no `=` into a key, so a malformed redirect could otherwise put a UPN on the log). - New `MamInstallReferrerBuilder` — two entry points: - `decorateAppLinkForMamCaInstall(appLink, originPkg, redirectParameters)` — flight-gated and **scoped to MAM-CA redirects only**, so a plain WPJ/device-registration install is untouched. - `decorateAppLinkWithOriginReferrer(appLink, originPkg)` — the ungated primitive. Allowlist-preserving, null-safe, idempotent (exactly one `referrer` param); on any parse problem it returns the original link so the existing install flow can never be broken. - **A referrer already on the link wins.** The server derives the referrer from the package hosting the sign-in UI, which is the calling app in the no-broker case this targets but is the *broker* when one is installed and hosting the flow. Where the two disagree the server is right, so this is a fallback for links that arrive without a referrer, not an override. - Flight: `ENABLE_MAM_CA_INSTALL_REFERRER`. **common** - The redirect's query parameters are now threaded to the point where the install link is launched (`AuthorizationFragment`, both browser fragments, and `AzureActiveDirectoryWebViewClient#processInstallRequest`), which is what lets the decoration be scoped to MAM-CA. `originPkg` is still sourced locally from the Android context — no cross-repo plumbing. With the flight off the link is launched exactly as before. ## Design alignment Reworked to match the current phased design doc: - Decoration is gated on the MAM-CA marker rather than applied to every broker-install redirect. - Flight renamed `ENABLE_BROKER_INSTALL_RESUME` → `ENABLE_MAM_CA_INSTALL_REFERRER`. Safe: default-off, never merged, no ECS entry. - Phase-2 surface removed from this PR. ## Server contract The gate is the `intuneAppProtection=1` marker the server puts on the broker-install redirect ([ESTS-Main PR 16454630](https://msazure.visualstudio.com/One/_git/ESTS-Main/pullrequest/16454630), server-side flighted, not yet merged). Shape: ``` https://<redirect_uri>?wpj=1&username=<upn>&app_link=<encoded Play link>&intuneAppProtection=1 ``` It is a **top-level parameter on the redirect, appended after `app_link`** — not nested inside the `app_link` value — so `StringExtensions.getUrlParameters(url)` already surfaces it. Only `intuneAppProtection=1` counts; anything else is treated as an ordinary broker install and left alone. Both sides are default-off, so the order they ramp in doesn't matter: until the server marker is live the client is a no-op, and until the client flight is on the marker is simply ignored. ## Testing - `MamInstallReferrerBuilderTest` — 15 tests, 0 failures. - `MamCaRedirectTest` — 7 tests, 0 failures. - Regression sweep across `providers` / `flighting` / `controllers` / `commands` — 242 tests, 0 failures. - `:common:compileLocalDebugJavaWithJavac` — BUILD SUCCESSFUL. - **End-to-end on a physical device** (OneAuthTestApp, MAM-CA account, no broker installed): sign-in → CA "install Company Portal" interstitial → "Get the app" → **genuine Play Store install** → Company Portal **relaunched the calling app**. - `MamInstallReferrerBuilder:decorateAppLinkForMamCaInstall: Tagged the Company Portal install launch with the calling app as the install referrer.` - `Finsky: Capture referrer for com.microsoft.windowsintune.companyportal` - `START … cmp=…/OneAuthTestActivity from uid … (com.microsoft.windowsintune.companyportal)` - The server marker was simulated client-side for this run, since the server side has not shipped yet. ## Note for reviewers `MamCaRedirect.kt` is added identically by #3195, and the two copies are kept **byte-identical on purpose** so the PRs can merge in either order. Whichever merges second will conflict in two purely additive spots — `CommonFlight` (adjacent enum constants) and `AzureActiveDirectoryWebViewClient` (imports + `processInstallRequest`). Both resolve by keeping both sides. Verified with `git merge-tree`: `MamCaRedirect.kt` itself is *not* in the conflict set. All new files here are Kotlin, with `@JvmStatic` on everything the existing Java call sites use. ## Rollout Redirect-back also needs the Company Portal read-side (separate CP feature). Until that lands this is a flight-gated no-op and safe to merge. ## Work item PBI [AB#3686094](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3686094) · Feature [AB#3676213](https://identitydivision.visualstudio.com/fac9d424-53d2-45c0-91b5-ef6ba7a6bf26/_workitems/edit/3676213) --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: sim <sim@local>
Summary
Phase 1 of "Improve MAM onboarding on Android" (Feature AB#3676213), companion to #3193.
Installing Company Portal can kill the calling app. When the user comes back, they land on a blank "Add account" screen and have to type the address they just typed. This PR persists the UPN from the CA install redirect so the host app can pre-fill it.
Independent of #3193 — either can merge first.
What changed
Gated on
CommonFlight.ENABLE_MAM_CA_UPN_HINT(default off).common4j
MamUpnHintStore:saveUpnHintForMamCaInstall(...)— writes on a MAM-CA broker-install redirect.getValidUpnHint(components, clientId)— returns the hint while it is within its TTL. Reading does not spend it (see below).clearUpnHint(...)— deliberately not flight-gated, so cleanup always works.applyStoredUpnHintIfAbsent(parameters)— injects it aslogin_hintwhen the caller didn't supply one, and when the caller has not asked the user to pick or create an account.MamCaRedirect(shared with MAM onboarding Phase 1: install-referrer redirect to the calling app (flight-gated), Fixes AB#3676213 #3193) decides what counts as a MAM-CA redirect.ENABLE_MAM_CA_UPN_HINT,MAM_CA_UPN_HINT_TTL_SECONDS.common
AzureActiveDirectoryWebViewClient#processInstallRequest— the one point every interactive caller passes through, including OneAuth (see below).AuthorizationErrorResponse#isMamCaInstall()is populated for the MSAL browser/CCT path.BrokerMsalController/LocalMSALControllerapply the hint and clear it on success.When the hint is spent — reads are deliberately non-destructive
The read started out single-use, matching the design. On device that turns out to be unimplementable as written, and it produced a bug that only shows up on the path this feature exists for.
Handling the install redirect finishes
AuthorizationActivitybefore the store listing is launched, which briefly resumes the calling app's own account screen while the flow is still in progress. A host that reads the hint when that screen is shown — which is exactly what the API asks for, because the install does not reliably kill the caller — read it ~34 ms after it was written, a full second before Play Store even opened:The hint was destroyed long before the restart it exists to survive. Worse, the failure was invisible when the process happened to survive — the text field kept the string in memory and looked correct — and only surfaced as an empty field when the app really was killed.
So the deletion moved off the read entirely:
BrokerMsalControllerandLocalMSALController).clearUpnHint(...)clears it, for hosts that render their own account UI.applyStoredUpnHintIfAbsentdeliberately does not clear it. It runs atacquireTokenentry, before parameter validation and the network check can throw — so clearing there meant a flaky network right after a ~100 MB install destroyed the hint in exactly the scenario this feature exists for.A pre-fill read is now safe to repeat every time the account screen is shown.
read_whileStillInTheInstallHandoff_leavesTheHintForTheReadAfterTheRestartpins this so it cannot regress.The hint is not applied when the caller asked the user to choose
Setting
login_hintis not cosmetic.BaseControllerclears the prompt when a hint is present, which suppresses the account-picker page, andMsalBrokerRequestAdaptermaps it to the broker's account-resolution field. A remembered address injected there would silently answer a question the caller explicitly wanted to put to the user — a real risk on a shared or multi-account device, since within the TTL the same hint can be offered to more than one request.So
applyStoredUpnHintIfAbsentdeclines when the prompt isSELECT_ACCOUNTorCREATE.LOGINis not excluded: it is about credential freshness, not identity ambiguity, and does not suppress any UI.The hint is bound to the authority it came from
A stored UPN is only known to the authority that returned it. Without a binding, any interactive request the same app makes inside the TTL would carry that address as
login_hint— including a request to a different sovereign cloud, handing a user's address to a service that never had it. That is a privacy and correctness problem rather than an escalation one (login_hintis not a credential), but it is avoidable.Each record therefore carries the authority host it was captured against, and the hint is only attached when the request's authority host matches.
Why the host, not the full url or the tenant. The capture happens against
/commonand the request that follows is against the resolved tenant, so url-level binding would kill the pre-fill in exactly the flow this ships for. Tenant-level binding is worse still:/72f988bf-…and/contoso.onmicrosoft.comare the same tenant with different strings, so a textual comparison gives unpredictable false negatives. The host is precisely the cloud boundary that carries the weight here, and it is stable across the/common→/{tenant}transition.A mismatch declines the hint but does not delete it. An unrelated request to another authority in the middle of the install detour must not wipe the hint the user is coming back for. The TTL already bounds how long it lives.
Unknown hosts fail closed. A record with no host — and a request whose authority cannot be parsed, which
Authority#getAuthorityUrisignals by throwing — can never satisfy the check, so it is never put on the wire.The user-visible pre-fill is deliberately left unbound.
getValidUpnHint(components, clientId)fills a local text box the user can see and edit, at a point in the flow where no authority has been chosen yet — there is nothing to bind against. Enforcement belongs at the point of transmission, which is where it is.Where the read happens — a deliberate deviation
The design assigns "read + inject" to OneAuth Android. On-device tracing shows the read has to be in both places:
BrokerMsalController#acquireTokendirectly (BrokerClient.java:525), bypassingInteractiveTokenCommand, and parses results itself inEmbeddedBrowser.completeInteractive. SoAuthorizationResultFactorynever runs on the OneAuth path — the WebView capture point is the only one that sees it.OneAuthNavigationActivity, which Common cannot reach. So the host SDK / 1P app must read the hint for its own UI.This PR therefore keeps the controller-level injection (so MSAL apps get it for free) and exposes
getValidUpnHint()for OneAuth to render. Additive, not a substitute.TTL
Default is 180s (3 minutes), as the design specifies, governed by
MamCaUpnHintTtlSeconds.An earlier manual run did expire a 180s hint, which briefly argued for a longer default. That run is not representative: much of the elapsed time was a human driving the UI step by step and pausing to read logs between steps, not the flow itself. The parts that are actually machine-paced - Play capturing the referrer, the install completing, Company Portal redirecting back, and the app restarting - fit inside 3 minutes comfortably.
That matches the end-to-end run recorded under Testing below: the hint was stored at
11:21:18and pre-filled after a genuine Play install, a process kill and a cold start at11:23:11— 1m53s, comfortably inside a 3-minute window.Erring short is also the cheaper mistake here: a missed hint just means the user types their address, which is exactly today's behaviour, whereas erring long leaves an address sitting on disk for no benefit. If real-world telemetry disagrees - slow cellular, or a much larger Company Portal build - this is a flight, so it can be raised through ECS without shipping code.
Server contract
Capture is gated on the
intuneAppProtection=1marker the server puts on the broker-install redirect (ESTS-Main PR 16454630, server-side flighted, not yet merged) — same gate as #3193, via the sharedMamCaRedirect. It is a top-level parameter appended afterapp_link, sogetUrlParameters(url)already surfaces it. OnlyintuneAppProtection=1counts; an ordinary broker install stores nothing.Both sides are default-off, so ramp order doesn't matter.
Testing
MamUpnHintStoreTest— 51 tests, 0 failures.MamCaRedirectTest— 7, 0 failures.AzureActiveDirectoryWebViewClientTest(fromdev) — 82 tests, 0 failures.:common:compileLocalDebugJavaWithJavac— BUILD SUCCESSFUL.The authority-binding tests were checked in both directions, since a binding can fail by being too loose or too strict. Removing the host check fails exactly the five "must not apply" tests and nothing else; widening the binding from the host to the whole authority url fails
apply_sameHostDifferentTenant_isStillUsed— the/common→/{tenant}case this feature actually ships for.End-to-end on a physical device, the point being to prove the hint survives process death:
11:19:04 pid 16311 MamUpnHintPrefill: No usable MAM-CA UPN hint11:21:18 MamUpnHintStore:saveUpnHint: Stored a UPN hint …Finsky: Capture referrer for …companyportalSTART …OneAuthTestActivity from uid … (…companyportal)11:23:11 MamUpnHintPrefill: Pre-filled the account hint from the MAM-CA UPN hint.idlabmamca@msidlab4.onmicrosoft.com✅The install was also observed not killing the caller on other runs, which is what motivated the lifetime change above. Both return paths — process killed and process survived — now pre-fill from the same stored hint.
Known limitation
The write lands in whichever process hosts the auth WebView. With no broker installed — the MAM onboarding case this targets — that is the calling app, so the pre-fill reads it back. If a broker were already installed and hosting the WebView, the write would land in the broker's storage and be invisible to the caller. Not an issue for the target scenario, but real.
Note for reviewers
MamCaRedirect.ktis added identically by #3193, and the two copies are kept byte-identical on purpose so the PRs can merge in either order. Whichever merges second conflicts in two purely additive spots —CommonFlightandAzureActiveDirectoryWebViewClient— both resolved by keeping both sides. Verified withgit merge-tree:MamCaRedirect.ktitself is not in the conflict set.Why
MamUpnHintStoreis JavaPer review feedback new classes should be Kotlin, and
MamCaRedirectplus both test classes are.MamUpnHintStorecannot be, yet. In common4jcompileKotlinruns beforecompileJava, so Lombok has not generated its accessors at the point Kotlin compiles and cannot resolve them:Reading those generated members of
InteractiveTokenCommandParametersandAuthorizationErrorResponseis exactly whatapplyStoredUpnHintIfAbsentand the error-response overload exist to do, so it can't be factored behind a seam. Unblocking it needsorg.jetbrains.kotlin.plugin.lombokadded to common4j — a module-wide build change affecting every consumer, which belongs in its own PR with build-owner sign-off rather than inside a feature PR. The reason is recorded in a comment above the class so nobody re-attempts it blind.The tests are Kotlin:
compileTestKotlinruns after main's Java, so by then the Lombok accessors are real methods on the jar.Work item
PBI AB#3705267 —
[Phase 1] 2. UPN hint storeFeature AB#3676213