-
Notifications
You must be signed in to change notification settings - Fork 51
Remember the UPN across a broker-install interruption to pre-fill login_hint, Fixes AB#3676213 #3195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Remember the UPN across a broker-install interruption to pre-fill login_hint, Fixes AB#3676213 #3195
Changes from all commits
7f5c420
7a70c0d
a3e64c0
4f933c7
8d285bd
a5638dd
ea754a3
1ad01b6
6c33201
d723ed0
12dc7af
7dadd0c
e2e0d13
f6cd95d
910bcd7
2af179a
7e4d693
da54f6d
01955a8
0893d56
c63dd95
f853d1a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,6 +47,7 @@ | |
| import androidx.lifecycle.ViewTreeLifecycleOwner; | ||
|
|
||
| import com.microsoft.identity.common.adal.internal.AuthenticationConstants; | ||
| import com.microsoft.identity.common.components.AndroidPlatformComponentsFactory; | ||
| import com.microsoft.identity.common.adal.internal.util.StringExtensions; | ||
| import com.microsoft.identity.common.internal.broker.BrokerData; | ||
| import com.microsoft.identity.common.internal.broker.BrokerValidator; | ||
|
|
@@ -86,6 +87,7 @@ | |
| import com.microsoft.identity.common.java.WarningType; | ||
| import com.microsoft.identity.common.java.exception.ClientException; | ||
| import com.microsoft.identity.common.java.exception.ErrorStrings; | ||
| import com.microsoft.identity.common.java.providers.MamUpnHintStore; | ||
| import com.microsoft.identity.common.java.providers.RawAuthorizationResult; | ||
| import static com.microsoft.identity.common.java.telemetry.OnboardingTelemetryConstants.STEP_AUTHENTICATOR_MFA_LINKING_STARTED; | ||
| import static com.microsoft.identity.common.java.telemetry.OnboardingTelemetryConstants.STEP_BROKER_INSTALL_PROMPTED; | ||
|
|
@@ -170,6 +172,20 @@ public class AzureActiveDirectoryWebViewClient extends OAuth2WebViewClient { | |
| private final SpanContext mSpanContext; | ||
| private final String mUtid; | ||
|
|
||
| /** | ||
| * Client id of the request being authorized, when known. Used to scope the MAM Conditional | ||
| * Access UPN hint to the app that was interrupted. | ||
| */ | ||
| @Nullable | ||
| private final String mClientId; | ||
|
|
||
| /** | ||
| * Host of the authority this WebView is talking to, so a UPN captured mid-flow can be bound to | ||
| * it. Null when it could not be determined from the authorization request url. | ||
| */ | ||
| @Nullable | ||
| private final String mAuthorityHost; | ||
|
|
||
| private String mPasskeyRegistrationScript; | ||
|
|
||
| /** | ||
|
|
@@ -195,6 +211,35 @@ public AzureActiveDirectoryWebViewClient(@NonNull final Activity activity, | |
| @Nullable final String utid, | ||
| final boolean isWebViewWebCpEnabledInBrokerlessCase, | ||
| @Nullable final IUrlLoadTracker urlLoadTracker) { | ||
| this(activity, completionCallback, pageLoadedCallback, redirectUrl, | ||
| switchBrowserProtocolCoordinator, utid, isWebViewWebCpEnabledInBrokerlessCase, | ||
| urlLoadTracker, null); | ||
| } | ||
|
|
||
| public AzureActiveDirectoryWebViewClient(@NonNull final Activity activity, | ||
| @NonNull final IAuthorizationCompletionCallback completionCallback, | ||
| @NonNull final OnPageLoadedCallback pageLoadedCallback, | ||
| @NonNull final String redirectUrl, | ||
| @NonNull final SwitchBrowserProtocolCoordinator switchBrowserProtocolCoordinator, | ||
| @Nullable final String utid, | ||
| final boolean isWebViewWebCpEnabledInBrokerlessCase, | ||
| @Nullable final IUrlLoadTracker urlLoadTracker, | ||
| @Nullable final String clientId) { | ||
| this(activity, completionCallback, pageLoadedCallback, redirectUrl, | ||
| switchBrowserProtocolCoordinator, utid, isWebViewWebCpEnabledInBrokerlessCase, | ||
| urlLoadTracker, clientId, null); | ||
| } | ||
|
|
||
| public AzureActiveDirectoryWebViewClient(@NonNull final Activity activity, | ||
| @NonNull final IAuthorizationCompletionCallback completionCallback, | ||
| @NonNull final OnPageLoadedCallback pageLoadedCallback, | ||
| @NonNull final String redirectUrl, | ||
| @NonNull final SwitchBrowserProtocolCoordinator switchBrowserProtocolCoordinator, | ||
| @Nullable final String utid, | ||
| final boolean isWebViewWebCpEnabledInBrokerlessCase, | ||
| @Nullable final IUrlLoadTracker urlLoadTracker, | ||
| @Nullable final String clientId, | ||
| @Nullable final String authorityHost) { | ||
| super(activity, completionCallback, pageLoadedCallback); | ||
| mRedirectUrl = redirectUrl; | ||
| mCertBasedAuthFactory = new CertBasedAuthFactory(activity); | ||
|
|
@@ -203,6 +248,8 @@ public AzureActiveDirectoryWebViewClient(@NonNull final Activity activity, | |
| mSpanContext = activity instanceof AuthorizationActivity ? ((AuthorizationActivity) getActivity()).getSpanContext() : null; | ||
| mIsWebViewWebCpEnabledInBrokerlessCase = isWebViewWebCpEnabledInBrokerlessCase; | ||
| mUrlLoadTracker = urlLoadTracker; | ||
| mClientId = clientId; | ||
| mAuthorityHost = authorityHost; | ||
| } | ||
|
|
||
| public AzureActiveDirectoryWebViewClient(@NonNull final Activity activity, | ||
|
|
@@ -1350,6 +1397,29 @@ private void processInstallRequest(@NonNull final WebView view, @NonNull final S | |
| final Map<String, String> parameters = StringExtensions.getUrlParameters(url); | ||
| final String appLink = parameters.get(APP_LINK_KEY); | ||
|
|
||
| // MAM Conditional Access onboarding: the service tells us which account is being onboarded | ||
| // on this redirect. Installing Company Portal usually kills this process, so remember the UPN | ||
| // now and pre-fill it on the request the user makes when they come back, instead of asking | ||
| // them to type their address again. No-op unless the flight is on and the redirect is marked | ||
| // as the MAM-CA path. | ||
| // | ||
| // Written inline on the UI thread, deliberately. This is the last moment the process is | ||
| // guaranteed to be alive: the completion callback below finishes the authorization activity | ||
| // and the Play Store install that follows very often kills us. Handing the write to a | ||
| // background thread would risk losing the hint in exactly the case it exists for, and would | ||
| // trade a bounded, best-effort write - a single encrypted key/value put, with every failure | ||
| // caught and swallowed inside the store - for a race it cannot win. | ||
| final Activity installRequestActivity = getActivity(); | ||
| if (installRequestActivity != null) { | ||
| MamUpnHintStore.saveUpnHintForMamCaInstall( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, UI thread - and kept that way deliberately. Added a comment in |
||
| AndroidPlatformComponentsFactory.createFromContext( | ||
| installRequestActivity.getApplicationContext()), | ||
| mClientId, | ||
| mAuthorityHost, | ||
| parameters | ||
| ); | ||
| } | ||
|
|
||
| Logger.info(methodTag,"Launching the link to app:" + appLink); | ||
| getCompletionCallback().onChallengeResponseReceived(result); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor doc accuracy nit: the "no-op unless (a) flight on, (b)
login_hintblank, (c) valid hint stored for this client and authority" list reads as sufficient, butapplyStoredUpnHintIfAbsentalso declines whenpromptisSELECT_ACCOUNTorCREATE(MamUpnHintStore.java:571-575), even with all three conditions holding. A reader following the comment could be surprised when a request withprompt=SELECT_ACCOUNTisn''t pre-filled despite the setup looking right.Suggested phrasing:
Same fix in
BrokerMsalControllerat line 357.Related (optional): the prompt-decline rule used to live in the class-level Javadoc of
MamUpnHintStoreand was removed in the "trim to intro + five bullets" cleanup. It''s now only visible by readingapplyStoredUpnHintIfAbsent''s body. Consider adding a short bullet or a line under the Authority-binding bullet noting the prompt exclusion, soapplyStoredUpnHintIfAbsent''s contract is discoverable from the class Javadoc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
CREATEas well.