Skip to content

MAM onboarding Phase 1: install-referrer redirect to the calling app (flight-gated), Fixes AB#3676213 - #3193

Merged
wzhipan merged 20 commits into
devfrom
copilot/mam-referrer-phase1
Aug 7, 2026
Merged

MAM onboarding Phase 1: install-referrer redirect to the calling app (flight-gated), Fixes AB#3676213#3193
wzhipan merged 20 commits into
devfrom
copilot/mam-referrer-phase1

Conversation

@wzhipan

@wzhipan wzhipan commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Summary

Phase 1 of "Improve MAM onboarding on Android" (PBI AB#3686094, Feature AB#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_RESUMEENABLE_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, 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 · Feature AB#3676213

…(flight-gated)

Adds the Company Portal install-referrer decoration so that, when an interactive
auth request is interrupted by a Conditional-Access "install broker" (Company
Portal) response, the Play Store install link is tagged with the calling app's
package as the install referrer. Company Portal reads this referrer and redirects
the user back to the originating app after install. This is product Phase 1
(redirect only) - no park / auto-resume, which ships separately as Phase 2.

Scope (all in Common, all gated on CommonFlight.ENABLE_BROKER_INSTALL_RESUME,
default off):
- common4j: new MamInstallReferrerBuilder (+ unit tests) providing the
  CP-compatible bare-origin form decorateAppLinkWithOriginReferrer(appLink,
  originPkg) => "...&referrer=<originPkg>", allowlist-preserving and null-safe.
- common4j: add the ENABLE_BROKER_INSTALL_RESUME flight constant.
- common: decorate the install launch at the three broker-install call sites -
  BrowserAuthorizationFragment, CurrentTaskBrowserAuthorizationFragment,
  AzureActiveDirectoryWebViewClient. originPkg is sourced locally from the
  Android context (getPackageName / getActivity().getPackageName()); no
  cross-repo / OneAuth plumbing is required on the build side.

With the flight off the install link is launched exactly as before. End-to-end
redirect-back requires the Company Portal read-side (separate CP feature); until
then the decoration is a harmless flight-gated no-op.

Verified: common4j MamInstallReferrerBuilderTest = 10 tests, 0 failures;
:common:compileLocalDebugJavaWithJavac BUILD SUCCESSFUL.

Feature AB#3676213 (Broker-Install Request Resume - Common Engine)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown

✅ Work item link check complete. Description contains link AB#3676213 to an Azure Boards work item.

@github-actions

Copy link
Copy Markdown

❌ Invalid work item number: AB#3676213):. Work item number must be a valid integer.

Click here to learn more.

@github-actions github-actions Bot changed the title MAM onboarding Phase 1: install-referrer redirect to the calling app (flight-gated) MAM onboarding Phase 1: install-referrer redirect to the calling app (flight-gated), Fixes AB#3676213 Jul 25, 2026
…thod

The broker-install "Get the app" launch is triggered from three independent
paths (BrowserAuthorizationFragment, CurrentTaskBrowserAuthorizationFragment,
and AzureActiveDirectoryWebViewClient), and each duplicated the same
flight-check + null-package guard + referrer decoration.

Extract that policy into MamInstallReferrerBuilder.decorateAppLinkWithOriginReferrerIfEnabled
(common4j) so the ENABLE_BROKER_INSTALL_RESUME gate is evaluated in exactly one
place and is unit-testable as pure Java. The two browser fragments' identical
adapter collapses into a shared protected decorateInstallLinkWithReferrer helper
on their common AuthorizationFragment base; the WebViewClient keeps a 2-line
adapter (it uses getActivity() and posts on a Handler) but no longer re-implements
the flight logic. Behavior is unchanged (flight default off).

Add unit tests for the gated method (flight on decorates identically to the
ungated form; flight off / missing package / no flights manager all return the
link unchanged).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown

✅ Work item link check complete. Description contains link AB#3686094 to an Azure Boards work item.

@wzhipan
wzhipan marked this pull request as ready for review July 25, 2026 02:52
@wzhipan
wzhipan requested a review from a team as a code owner July 25, 2026 02:52
Copilot AI review requested due to automatic review settings July 25, 2026 02:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces Phase 1 of improved MAM onboarding by (flight-gated) tagging the Company Portal Play Store install link with the calling app’s package as the Play install referrer, enabling Company Portal to redirect the user back to the originating app after install.

Changes:

  • Added MamInstallReferrerBuilder (and unit tests) to decorate Play Store install links with a CP-compatible referrer=<originPkg> form behind CommonFlight.ENABLE_BROKER_INSTALL_RESUME.
  • Added the ENABLE_BROKER_INSTALL_RESUME flight constant to CommonFlight.
  • Wired the decoration into broker-install launches across the browser fragments and embedded WebView flow.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
common4j/src/main/com/microsoft/identity/common/java/providers/MamInstallReferrerBuilder.java Adds referrer-building and flight-gated decoration helpers for Company Portal redirect-back.
common4j/src/test/com/microsoft/identity/common/java/providers/MamInstallReferrerBuilderTest.java Adds unit coverage for decoration/idempotency/allowlist/round-trip parsing behavior.
common4j/src/main/com/microsoft/identity/common/java/flighting/CommonFlight.java Introduces the ENABLE_BROKER_INSTALL_RESUME flight toggle.
common/src/main/java/com/microsoft/identity/common/internal/ui/webview/AzureActiveDirectoryWebViewClient.java Decorates the broker install link launched from the embedded WebView flow.
common/src/main/java/com/microsoft/identity/common/internal/providers/oauth2/AuthorizationFragment.java Centralizes install-link decoration for AuthorizationFragment subclasses.
common/src/main/java/com/microsoft/identity/common/internal/providers/oauth2/BrowserAuthorizationFragment.java Uses the shared decoration helper before launching the broker install link.
common/src/main/java/com/microsoft/identity/common/internal/providers/oauth2/CurrentTaskBrowserAuthorizationFragment.java Uses the shared decoration helper before launching the broker install link.

Comment thread common4j/src/main/com/microsoft/identity/common/java/flighting/CommonFlight.java Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@wzhipan
wzhipan requested a review from a team as a code owner July 25, 2026 02:55
- decode(): also catch IllegalArgumentException so parseReferrer stays
  defensive against malformed percent-encoding (e.g. a lone '%' or '%zz')
  instead of crashing; added a regression test.
- MamInstallReferrerBuilder Javadoc: drop the individual name/date and
  reference Feature 3676213 instead ("server-provided" app_link).
- ENABLE_BROKER_INSTALL_RESUME: reword as an explicit multi-phase gate so
  the description matches what Phase 1 actually gates (referrer/redirect-only)
  vs. later park-and-replay phases.
- AzureActiveDirectoryWebViewClient.processInstallRequest: cache the Activity
  once inside the postDelayed runnable and bail out (with a warn) if the
  Activity or app_link is gone, so the delayed launch never NPEs on a
  detached fragment.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
wzhipan and others added 2 commits July 28, 2026 09:50
…on, drop Phase-2 surface

Brings the install-referrer change in line with the phased MAM-CA design:

- Scope the decoration to the MAM Conditional Access install path. The same
  msauth://wpj redirect also drives ordinary device-registration installs,
  which must keep behaving as they do today, so decoration now requires the
  server's intuneAppProtection=1 marker. New MamCaRedirect reads the markers
  (and logs the redirect's parameter names only - the redirect carries the
  user's UPN, so the URL itself is never logged).
- Add ENABLE_MAM_CA_INSTALL_WITHOUT_MARKER (default off) so the client
  behavior can be validated and rolled out before the server marker ships.
- Rename the gate to ENABLE_MAM_CA_INSTALL_REFERRER. The old name described a
  park-and-resume design that the phased plan replaces; the flight is default
  off and unshipped, so the rename is free.
- Drop the Phase-2 park/replay surface from MamInstallReferrerBuilder
  (buildReferrerValue, decorateAppLinkWithReferrer, buildMarketFallbackUri,
  parseReferrer and their constants). Phase 1 is referrer-only.

Tests: MamInstallReferrerBuilderTest 12/12, new MamCaRedirectTest 7/7.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…marker

The server now appends intuneAppProtection=1 as a top-level query parameter on
the broker-install redirect when Conditional Access blocks the request pending a
Company Portal install, so the client no longer needs an escape hatch for
running ahead of that marker.

- Drop the ENABLE_MAM_CA_INSTALL_WITHOUT_MARKER flight and its fallback.
- Collapse hasIntuneAppProtectionMarker into isMamCaInstall, now a pure
  predicate; logRedirectParameterNames remains the single diagnostic.
- Document that the marker is a top-level parameter on the redirect, appended
  after app_link, and not nested inside the app_link value.
- Replace the flight-dependent tests with strict marker tests, including one
  that pins the exact install-link shape the server produces.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few review comments — thanks for the clean phased scoping and the marker-based gating.

Comment thread common4j/src/main/com/microsoft/identity/common/java/providers/MamCaRedirect.java Outdated
wzhipan and others added 4 commits July 28, 2026 18:46
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>
New classes are expected to be Kotlin, so MamCaRedirect and
MamInstallReferrerBuilder and their tests move over. Behaviour is unchanged
by the conversion itself; the existing assertions are the safety net.

Review fixes on top of that:

- The parameter-name log fired on every broker-install redirect, ahead of
  the flight check, so turning the flight off did not turn the feature off.
  It now sits behind the gate, inside decorateAppLinkForMamCaInstall, which
  also extends it to the browser and custom-tab flows that never had it.
- A referrer the server already put on the install link is now kept rather
  than overwritten. The server names the calling app; this code can only see
  the package of the process hosting the sign-in UI, which is the broker's
  when a broker is hosting the flow. Where they disagree the server is right.
- The "tagged the install launch" log fired even when the link was returned
  unchanged, so the rollout signal could report success for a no-op. It now
  lives on the path that actually decorated the link.
- Corrected the comment on the install-launch guard: it does not make the
  Runnable detach-safe, and it deliberately does not check isFinishing(),
  because the result callback finishes the Activity before this fires.

Tests: MamCaRedirect 7, MamInstallReferrerBuilder 15. Two assertions the
review called weak are now absolute, and the no-UPN-on-the-log property is
pinned at the public call site rather than only on its helper.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The referrer builder itself is well covered in common4j, but the Android glue
that reaches it - threading the redirect parameters from the WebView client and
the authorization fragments into the install launch - had no tests, which is
what dragged the common module's coverage down.

Adds Robolectric coverage for both call sites: the flight gate, the MAM-CA
marker gate, the calling-app package being named as the Play install referrer,
and a referrer the server already set being preserved rather than overridden.

Also pins that a non-https app_link is rejected by the broker-install allowlist
before the launch is ever reached, which is worth stating explicitly because it
makes the browser:// rewrite further down unreachable for app_links.

Test-only change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Follow-up on the latest changes.

@Prvnkmr337

Copy link
Copy Markdown
Contributor

Should this emit a metric while the flight is on?

The only observability in this PR is Logger.info lines — no span attributes or events. Once we ramp the flight we won't be able to answer basic rollout questions from Kusto:

  • Is the server actually marking redirects with intuneAppProtection=1 yet?
  • Of marked redirects, how many got decorated vs. bailed out (missing originPkg, server already set a referrer, parse failure)?

processInstallRequest already calls recordOnboardingStep(STEP_BROKER_INSTALL_PROMPTED), so the plumbing is right there. A single attribute on that span (e.g. is_mam_ca_install_referrer_decorated, plus an outcome enum) would give us the ramp-safety signal.

Add one attribute in this PR, or defer to a follow-up that lands before we ramp past 0%?

… 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>
@wzhipan

wzhipan commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Good call - done in 810bf9c rather than deferred, because it turned out to need no schema change.

OnboardingTelemetryRecorder.addUxFlowUsed(...) already exists for exactly this ("a phased rollout cohort like MobileOnboardingPhase1", per its KDoc) and had no production callers yet. Reusing it means the data lands in the onboarding blob that is already emitted, surfacing as mo_ux_flow_used.

MamInstallReferrerBuilder now returns an outcome alongside the link, and the WebView client reports it:

outcome meaning
FLIGHT_OFF not reported at all
MamCaInstallReferrer_NotMamCa broker install, but no intuneAppProtection=1 marker
MamCaInstallReferrer_NoOriginPackage marked, but we could not determine the calling package
MamCaInstallReferrer_NoAppLink marked, but no app_link to decorate
MamCaInstallReferrer_ServerReferrer the server already named a referrer, so we left it alone
MamCaInstallReferrer_LinkUnparseable app_link failed to parse
MamCaInstallReferrer_Decorated we added the referrer

That answers both ramp questions directly: Decorated vs ServerReferrer shows how much of the population the client-side fallback is actually carrying, and NotMamCa vs the rest shows how often the marker is arriving at all - which matters while the eSTS side is still ramping.

Two things worth flagging:

FLIGHT_OFF reports nothing, deliberately. finalizeBlob() omits ux_flow_used entirely when the list is empty, so with the flight off the feature still leaves zero trace - consistent with what we agreed earlier about logging, now extended to telemetry. There is a test that fails if that ever changes.

Only the embedded-WebView path has a recorder. AuthorizationFragment.decorateInstallLinkWithReferrer, used by the two browser fragments, has no OnboardingTelemetryRecorder attached, so those paths stay log-only. Acceptable for now because MAM-CA onboarding runs through the WebView (that is where STEP_BROKER_INSTALL_PROMPTED is recorded too), but it is a genuine gap rather than full coverage, so calling it out.

One behavioural note: the marker check now runs before the origin-package check. Both branches return the link unchanged, so this is reporting-only - it stops a missing package from being misreported as "not MAM-CA".

The outcome was reported from inside the 1s-delayed install Runnable, but the
completion callback fires first: it sends the result and finishes the Activity,
and the onboarding blob is finalized off that same result (broker side,
AccountChooserActivity#finalizeOnboardingBlobForFailure -> finalizeBlob). Anything
added to the recorder a second later would be written into a blob that had already
shipped, so mo_ux_flow_used would never have carried the ramp signal it exists for.

Decoration and reporting now happen before the callback; the Runnable keeps only
the launch. getActivity() is a final field, so hoisting it cannot change whether
the launch happens - the same null check simply moves earlier. This mirrors
STEP_BROKER_INSTALL_PROMPTED, which is already recorded at method entry.

The existing telemetry tests could not catch this because they finalize the blob
themselves at the end of the test, observing a tag production would never emit, so
a test that snapshots the blob from inside the completion callback is added.
Negative-tested: recording only from the Runnable fails it (plus the three outcome
tests), and nothing else moves.

AzureActiveDirectoryWebViewClientTest 94/94.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ux_flow_used records which UX-variant cohort a user was in - the
"MobileOnboardingPhase0/1/2" family - not per-attempt result codes. This
was writing six of them into it, five of which mean "the feature did not
apply", onto a shared append-only array that other onboarding features
also append to.

It never reached MATS in any case. The only way to attach a recorder to
AzureActiveDirectoryWebViewClient is setOnboardingTelemetryRecorder,
which has no production caller anywhere in common, broker, msal, adal or
oneauth - only tests. The hosts that build a recorder build their own and
never hand it over, so recordMamCaReferrerOutcome always hit its
null-recorder early return: the onboarding blob captured from a device
run of this feature carries no ux_flow_used key at all, despite that run
producing an outcome whose tag is non-null.

This also settles the review point about the browser/custom-tab path
decorating without reporting. With no telemetry on either launch site,
the embedded WebView and the two browser fragments now make the same
call and emit the same MamInstallReferrerBuilder log lines - which is
what the ramp is read off, and what the E2E runs used.

Outcome and the WithOutcome overload stay: six of the seven outcomes
return the app_link unchanged, so without them the flight gate, the
MAM-CA gate and the server-referrer branch are indistinguishable in
tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Prvnkmr337

Copy link
Copy Markdown
Contributor

Clarification on Outcome / Decoration after ce46c615

All three production call sites use the non-WithOutcome overload and drop the outcome; only tests read it. Three quick questions:

  1. Is decorateAppLinkForMamCaInstallWithOutcome intended to have a production consumer later, or stay test-only?
  2. Four of seven outcomes (FLIGHT_OFF, NOT_MAM_CA, NO_ORIGIN_PKG, NO_APP_LINK) emit no log, and the three that do don't carry the enum value. Intentional, or worth one uniform Logger.info per branch?
  3. Are these outcomes expected to surface as OneAuth metrics (via the OneAuth telemetry pipeline consuming common's signals)?

Only three of the seven outcomes left a log line, so a capture could not say
whether a link went untagged because the marker was missing, because the host
could not name itself, or because there was no app_link at all.

Each of those three now logs, and the three that already logged carry the
outcome value too. FLIGHT_OFF stays silent by design: with the flight off this
feature is meant to be indistinguishable from its absence, log lines included.

The new test reads the expected value off the enum rather than hard-coding
strings, so a rename cannot leave the log and the enum disagreeing.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@wzhipan

wzhipan commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

1. Test-only, by design. Six of the seven outcomes return the app_link unchanged, so without the overload the flight gate, the MAM-CA gate and the server-referrer branch are indistinguishable in tests. No production consumer planned for Phase 1.

2. Partly intentional. FLIGHT_OFF is deliberate and documented on the enum: with the flight off this feature should be indistinguishable from its absence, log lines included. The other three had no such reason — each now logs, and the three that already logged carry the outcome value too. Added a test that reads the expected value off the enum, so the log and the enum cannot drift.

3. Not from these PRs. Mobile-onboarding telemetry is being handled as a separate follow-up project, and MAM-CA onboarding will be wired there. It is also not a small add here: at the OneAuth boundary a MAM-CA install currently arrives as plain BROKER_INSTALLATION_TRIGGERED, indistinguishable from a regular broker install, so it needs a common-side change first.

@Prvnkmr337

Copy link
Copy Markdown
Contributor

LGTM

…ay Store

The MAM-CA marker says the redirect is a MAM Conditional Access install, but it
does not say which app the redirect installs. A broker-install redirect's app_link
is allowed to name any of three destinations, and only one of them can act on a
Play install referrer:

  - the Company Portal Play listing - reads the referrer to skip its own sign-in
    UX and redirect back to the caller, which is the whole point of this change;
  - the Authenticator Play listing - honours no such contract;
  - the China Company Portal link - a go.microsoft.com fwlink rather than a Play
    Store link, so a referrer on it is never delivered.

Decoration was package-agnostic, so a marked redirect carrying either of the last
two got tagged and reported as DECORATED. Appending an inert query parameter is
harmless in itself, but rollout is read off these log lines, so it would have
overstated how far the feature had actually reached - and the China link is the
more likely of the two to be hit.

Constrain decoration to the Company Portal Play listing and report the rest as a
distinct NOT_COMPANY_PORTAL outcome, so "skipped, wrong install target" stays
separable from "tagged". Only the package id is matched: BrokerInstallLinkValidator
has already restricted app_link to its allowlist, unflighted, on the path that
produces BROKER_INSTALLATION_TRIGGERED in the first place.

The check sits inside the existing parse rather than ahead of it, so it reuses the
one CommonURIBuilder and an unparseable app_link is still reported as
LINK_UNPARSEABLE instead of being mislabelled as the wrong target.

Behaviour is unchanged for every link the feature is meant to tag, and unchanged
for unmarked redirects: ordinary device-registration and MDM installs share this
redirect and are still identified solely by the absence of the marker.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@wzhipan

wzhipan commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Pushed one more scoping change (18a19849b) after re-auditing how we decide to tag the install link.

The gap. The intuneAppProtection=1 marker tells us the redirect is a MAM-CA install, but not which app it installs. app_link is allowed to name any of the three destinations BrokerInstallLinkValidator permits, and only one of them can act on a Play install referrer:

app_link before
Company Portal Play listing tagged — intended
Authenticator Play listing tagged — Authenticator honours no referrer contract
China CP go.microsoft.com fwlink tagged — not a Play link, so the referrer is never delivered

Appending an inert parameter is harmless in itself, but rollout is read off these log lines, so both cases would have been reported as DECORATED and overstated how far the feature had actually reached. The China link is the likelier of the two.

The change. Decoration is now constrained to the Company Portal Play listing; anything else reports a new NOT_COMPANY_PORTAL outcome, keeping "skipped, wrong install target" separable from "tagged". Only the package id is matched — the allowlist upstream has already constrained the host, unflighted, on the path that produces BROKER_INSTALLATION_TRIGGERED. The check sits inside the existing parse rather than ahead of it, so an unparseable link is still reported as LINK_UNPARSEABLE rather than mislabelled.

MDM/device registration is unaffected — those share this redirect and are still identified solely by the absence of the marker, which this does not touch.

3 tests added, plus the new outcome wired into outcome_everyEvaluatedOutcomeNamesItselfInTheLog. Negative-tested both ways: removing the guard fails exactly the 3 tests that cover it; moving it ahead of the parse fails exactly the 3 that depend on LINK_UNPARSEABLE. MamInstallReferrerBuilder 23/0, MamCaRedirect 7/0, AzureActiveDirectoryWebViewClientTest 87/0, AuthorizationFragmentInstallReferrerTest 6/0.

Praveen Kumar (@Prvnkmr337) sorry — this invalidates your approval from earlier, would you mind taking another look?

Reverts 18a1984. That change narrowed decoration to app_links naming the Company
Portal Play listing, on the reasoning that the MAM-CA marker says the redirect is a
MAM-CA install but not which app it installs. Both premises behind it were wrong:

  - the server sets intuneAppProtection=1 only on MAM-CA flows, so the marker alone
    already establishes the flow, and the referrer should always be appended once it
    is present and the flight is on;
  - the China `go.microsoft.com/fwlink/?linkid=2134649` target is not a different app.
    It is Company Portal by another distribution channel, and BrokerInstallLinkValidator
    documents that the server itself may already set a `referrer` on it - so the claim
    that a referrer there is inert was speculation, and contradicted.

The gate therefore did not tighten anything. It dropped the referrer on real MAM-CA
installs in China, which is the one thing this class exists to add.

Keep a test and a note pinning the decision, because re-adding the gate looks like a
safe tightening and is not - that is exactly the mistake being reverted here. The
class KDoc now says the marker is the whole of the scope and why, and
gated_markedInstall_isDecoratedWhateverTheInstallTargetIs asserts all three allowed
destinations are tagged. Re-introducing the gate fails that test and nothing else.

Net effect against the last reviewed state is a comment and a test; production
behaviour is byte-for-byte what was approved.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@wzhipan

wzhipan commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Correction to my previous comment — that scoping change is reverted (1ac98e603). Please disregard it.

Confirmed with Veena: the server sets intuneAppProtection=1 only on MAM-CA flows, and the referrer should always be appended once the marker is present and the flight is on. Both premises I gated on were wrong:

  • the marker does establish the flow on its own, so there was nothing left to narrow;
  • the China go.microsoft.com/fwlink/?linkid=2134649 target is not a different app — it is Company Portal by another distribution channel, and BrokerInstallLinkValidator's own docs note the server may already set a referrer on it. So my "a referrer there is inert" claim was speculation, and contradicted.

The gate didn't tighten anything; it would have dropped the referrer on real MAM-CA installs in China.

Net effect against the state you last reviewed is a comment and a test — production behaviour is byte-for-byte identical to 2e996cee2. I kept a note on the class KDoc and gated_markedInstall_isDecoratedWhateverTheInstallTargetIs, which asserts all three allowed destinations are tagged, since re-adding this gate looks like a safe tightening and isn't. Re-introducing it fails that test and nothing else.

MamInstallReferrerBuilder 21/0, MamCaRedirect 7/0, AzureActiveDirectoryWebViewClientTest 87/0, AuthorizationFragmentInstallReferrerTest 6/0.

Praveen Kumar (@Prvnkmr337) apologies for the churn — your approval was never dismissed and still stands, so no action needed from you.

@shahzaibj
Shahzaib (shahzaibj) requested a balanced review from Copilot August 6, 2026 23:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

common4j/src/main/com/microsoft/identity/common/java/providers/MamInstallReferrerBuilder.kt:264

  • The success log message is overly specific to “Company Portal”, but the implementation explicitly decorates any install target on a marked redirect (including non-CP targets like Authenticator / fwlinks, per tests). Recommend making this message target-agnostic (e.g., “Tagged the broker-install launch…” or “Tagged the install link…”) to avoid misleading field diagnostics.
                Logger.info(
                    methodTag,
                    "Tagged the Company Portal install launch with the calling app as the install " +
                        "referrer. [outcome=${Outcome.DECORATED}]"
                )

common4j/src/main/com/microsoft/identity/common/java/providers/MamInstallReferrerBuilder.kt:231

  • decorateWithOutcome(...) always uses the decorateAppLinkWithOriginReferrer method tag even when invoked from the gated MAM-CA entry point. This makes log correlation harder because some outcomes for the MAM-CA path will appear under a different method tag than others. Consider passing the caller’s method tag into decorateWithOutcome (or computing the tag at the call site) so logs consistently identify the entry point being evaluated.
    private fun decorateWithOutcome(appLink: String?, originPkg: String): Decoration {
        val methodTag = "$TAG:decorateAppLinkWithOriginReferrer"

The two browser fragments hand app_link straight to Uri.parse, and it is not
obvious from the launch sites themselves that it cannot be null: the presence
check upstream is containsKey, which passes for a bare &app_link or an empty
&app_link=. What actually rules those out is the broker-install allowlist,
which rejects a blank link before the result code is ever set.

Pin that with a test and note it at both call sites, so the guarantee is
enforced rather than inferred.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (1)

common/src/main/java/com/microsoft/identity/common/internal/providers/oauth2/AuthorizationFragment.java:242

  • The new helper is intentionally null-tolerant (tests pass null appLink / redirectParameters and expect a null / unchanged result), and it delegates to a Kotlin API that returns a nullable String. Without @Nullable annotations here, Java/Kotlin callers lose the actual contract and static analysis can miss real null flows.

Recommendation: annotate the return value and both parameters as @Nullable to reflect the true behavior and improve interop.

    protected String decorateInstallLinkWithReferrer(final String appLink,
                                                     final Map<String, String> redirectParameters) {
        final Context context = getContext();
        return MamInstallReferrerBuilder.decorateAppLinkForMamCaInstall(
                appLink, context == null ? null : context.getPackageName(), redirectParameters);

@wzhipan
wzhipan enabled auto-merge (squash) August 7, 2026 06:56
@wzhipan
wzhipan merged commit 52bb6ec into dev Aug 7, 2026
30 checks passed
wzhipan pushed a commit that referenced this pull request Aug 7, 2026
Conflict in AzureActiveDirectoryWebViewClientTest: both sides appended tests at
the end of the class. Kept both - this PR's 11 Auth UX onboarding / shim tests
and dev's 5 new MAM install-referrer tests (#3193). 93 + 5 = 98 @test, all green.
Shahzaib (shahzaibj) added a commit that referenced this pull request Aug 11, 2026
…B#3706623)

Resolve the sole CommonFlight.java enum conflict by keeping both new flights
(ENABLE_MAM_CA_INSTALL_REFERRER from dev PR #3193 and
ENABLE_PKEYAUTH_SUBMIT_URL_ORIGIN_VALIDATION), with the PKeyAuth flight last.

Review findings addressed in the same commit:
- Finding 1 (log-forging): drop the warnPII that logged the raw, attacker-shaped
  authority slice in the backslash guard; the existing non-PII Logger.warn plus
  the thrown ClientException carry all diagnostic signal. Added authorityLength
  (int only) to that warn for triage.
- Finding 2 (coverage): add onPageStarted origin-tracking tests — https redirect
  target becomes the challenging origin; a non-https onPageStarted does not
  replace a recorded https origin; flight-off is a no-op.
- Finding 3 (stale doc): reword the CHALLENGING_ORIGIN fixture comment to state
  the same-origin check compares scheme + host + normalized port (fixture differs
  only in path/query), not host-only.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
wzhipan pushed a commit that referenced this pull request Aug 12, 2026
Drops the #3193/#3212 narrative and keeps only the load-bearing fact:
these resolve to their compile-time defaults outside the broker process.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
wzhipan added a commit that referenced this pull request Aug 13, 2026
…eferrer, Fixes AB#3686094 (#3212)

## Summary

Follow-up to #3193. That PR gated the MAM-CA install referrer on
`CommonFlight.ENABLE_MAM_CA_INSTALL_REFERRER`. That gate cannot be read
in the app process that needs it, and in the one process where it *can*
be read it would have produced the wrong answer. This moves the decision
to a boolean supplied by the host SDK.

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).

## Why the flight was the wrong mechanism

**In the app process it could never be turned on.** The only production
initialization of `CommonFlightsManager` in the tree is
`AndroidBrokerPlatformComponentsFactory` →
`AndroidBrokerFlightsManager.initializeFlightsManager`
(`AndroidBrokerFlightsManager.kt:92`), which runs in the **broker**.
OneAuth and MSAL have zero references to `CommonFlightsManager`,
`CommonFlight` or `IFlightsManager`, so in an app process the flight
always falls back to its compile-time default of `false`, and no ECS
entry could change that.

That matters because the referrer is only ever added on the
**brokerless** path: the decoration uses the package of the process
hosting the sign-in UI, which — when no broker is installed, the only
case where the Company Portal install interstitial appears in-app — is
the calling app.

**In the broker process, where it could be turned on, it would have been
wrong.** The broker also hosts an interactive WebView
(`AbstractBrokerController` → `AndroidAuthorizationStrategyFactory` →
`EmbeddedWebViewAuthorizationStrategy`), and "Authenticator installed,
Company Portal not installed" is an ordinary enterprise state. There,
`activity.getPackageName()` is the *broker's* package. So ramping the
ECS flight would have tagged the Company Portal install with the broker,
sending the user back to Authenticator after install instead of to the
app they were signing in to — worse than not decorating at all.

So the flight was unreadable where it was needed and actively harmful
where it was readable. Removing it closes both by construction.

## What changed

The gate is now an input from the host SDK, following the same path
`isWebViewWebCpEnabled` and `enableSwitchBrowser` already take:

```
AuthorizationActivityParameters.enableMamCaInstallReferrer
  -> intent extra
  -> AuthorizationFragment
  -> AzureActiveDirectoryWebViewClient
  -> MamInstallReferrerBuilder
```

- `ENABLE_MAM_CA_INSTALL_REFERRER` is removed from `CommonFlight`.
- `MamInstallReferrerBuilder`'s two entry points take a leading
`enabled: Boolean`, and `Outcome.FLIGHT_OFF` becomes
`Outcome.NOT_ENABLED`. The "nothing is logged while off" invariant is
unchanged.
- `AzureActiveDirectoryWebViewClient` gains a 9-arg constructor; the
existing 8- and 7-arg ones delegate with `false` and are now test-only,
so no caller breaks.
- `AuthorizationFragment` saves and restores the flag alongside the
request state, so it survives process death mid-flow.
- `WebViewAuthorizationFragment` builds its client in a new
`@VisibleForTesting createAADWebViewClient`, extracted from
`onCreateView`. Behaviour-preserving - same call order and construction,
the caller still assigns the field - so the fragment-to-client hop can
be tested without inflating a layout, which this module's unit tests
cannot do.
- `CommonFlight`'s class doc now states that flights only resolve in the
broker process. That warning is what would have caught this in review
the first time.

**Defaults to `false` everywhere, so MSAL and the broker are
unchanged.** That matches the precedent: Common's own
`BrowserAuthorizationStrategy` and
`EmbeddedWebViewAuthorizationStrategy` do not set
`isWebViewWebCpEnabled` either — OneAuth is its only supplier.

## Companion change and kill switch

The OneAuth side supplies the boolean from the **`MamCaInstallReferrer`
ChangeGate**, served by the **`OneAuth-MSAL` ECS project** (not the
broker ECS portal), so the switch is server-flippable without an app
update. It is up for review in parallel and cannot compile until this
publishes — confirmed by building it against both this branch (`BUILD
SUCCESSFUL`) and the currently pinned `common:24.5.0` (fails, as
expected).

Note that Common no longer owns a kill switch for this path. Two caveats
are being tracked on the OneAuth PR: a pulled gate needs two app
restarts to take effect, and gate fetching depends on
`OneAuthFlight::ECS_FETCH`, which is not on by default for every host.

## Known gaps

- **Brokered MAM-CA onboarding is not covered.** In the broker-hosted
case above the referrer is simply not added (`false`). Doing it
correctly there means using the caller's package
(`InteractiveTokenCommandParameters.getCallerPackageName()`) rather than
`Context#getPackageName()` — a design change, not a flag flip. Worth a
Phase-2 PBI.
- **No tenant scoping.** Neither mechanism available to the host is
tenant-aware — OneAuth ECS config filters on `ApplicationID` / `OS` /
`OneAuthVersion`, and Flights are per-app — so per-tenant rollout or
exclusion is not achievable through this boolean.

## Testing

| suite | result |
| --- | --- |
| `MamInstallReferrerBuilderTest` (common4j) | 20 / 0 |
| `MamCaRedirectTest` (common4j) | 7 / 0 |
| `AzureActiveDirectoryWebViewClientTest` | 87 / 0 |
| `AuthorizationFragmentInstallReferrerTest` | 8 / 0 |
| `AuthorizationActivityFactoryTest` | 12 / 0 |
| `WebViewAuthorizationFragmentInstallReferrerTest` | 3 / 0 |

All three new seams were verified by mutation rather than by passing
alone:

- Replacing the opt-in argument the fragment hands the client with a
literal `false` - silently disabling the feature on the embedded-WebView
path - left **every pre-existing suite green**, including the 87-test
`AzureActiveDirectoryWebViewClientTest`, which builds the client
directly and so pins the decoration rules but never the wiring. Only
`WebViewAuthorizationFragmentInstallReferrerTest` catches it.
- Deleting the `putExtra` in the factory failed exactly one test, and
nothing else moved.
- Deleting either the `extractState` read or the `onSaveInstanceState`
write in `AuthorizationFragment` originally left the **entire suite
green**. `AuthorizationFragmentInstallReferrerTest` now covers that hop;
each deletion fails it independently.

Each of the three new WebView tests was also checked to fail for a
distinct, realistic defect: an inverted default (`getBoolean(key,
true)`) fails only the absent-key test, and a presence-based read
(`containsKey`) fails only the explicit-opt-out test.

The coverage check reports a 0.01 pp line-coverage drop with `No
per-class coverage regressions detected`. That is the arithmetic of
deleting covered lines (the removed enum constant and its flight reads
were all covered), not a regression, hence the `skip-coverage-check`
label.

---------

Co-authored-by: sim <sim@local>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants