Fix Login for Admin and Welcome Discovery incompatibility#2933
Fix Login for Admin and Welcome Discovery incompatibility#2933brandonpage wants to merge 2 commits into
Conversation
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| salesforceWelcomeDiscoveryHostAndPathUrl: Uri | ||
| ) = salesforceWelcomeDiscoveryHostAndPathUrl.buildUpon() | ||
| .appendQueryParameter( | ||
| LoginActivity.SALESFORCE_WELCOME_DISCOVERY_MOBILE_URL_QUERY_PARAMETER_KEY_CLIENT_ID, |
There was a problem hiding this comment.
| This method should only be accessed from tests or within private scope |
| oAuthConfig.consumerKey, | ||
| ) | ||
| .appendQueryParameter( | ||
| LoginActivity.SALESFORCE_WELCOME_DISCOVERY_MOBILE_URL_QUERY_PARAMETER_KEY_CLIENT_VERSION, |
There was a problem hiding this comment.
| This method should only be accessed from tests or within private scope |
| URLEncoder.encode(SalesforceSDKManager.getInstance().appVersion, "utf8") | ||
| ) | ||
| .appendQueryParameter( | ||
| LoginActivity.SALESFORCE_WELCOME_DISCOVERY_MOBILE_URL_QUERY_PARAMETER_KEY_CALLBACK_URL, |
There was a problem hiding this comment.
| This method should only be accessed from tests or within private scope |
Generated by 🚫 Danger |
The guards read viewModel.selectedServer.value on a relaxed mockk, which returns a String proxy that fails the .toUri() CHECKCAST with "Object_1_Proxy cannot be cast to String" in CI. The guards were documentation-only; the underlying tests still exercise the non-Welcome path because the LoginServerManager default in the instrumentation environment is not a Welcome Discovery URL. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| // applySalesforceWelcomeLoginHintAndHost → applyPendingServer. | ||
| val welcomeUrl = selectedLoginServerUri.toString() | ||
| if (selectedServer.value != welcomeUrl) { | ||
| selectedServer.value = welcomeUrl |
There was a problem hiding this comment.
The ordering of loginUrl.value = (line 350) before selectedServer.value = (line 360) is load-bearing: LoginUrlSource's same-host short-circuit relies on seeing the new loginUrl host before selectedServer changes, so it suppresses a spurious auth-URL regen. This invariant is currently documented only in the large block comment above — consider adding a short inline comment at each of the two assignment lines (e.g. // Must precede selectedServer reset — see LoginUrlSource same-host short-circuit) so a future refactor doesn't accidentally reorder them or extract one without the other.
| @Deprecated(message = "This function will be replaced by a permanent solution in 14.0.") | ||
| fun launchLoginForAdminsAction() { | ||
| val selectedServer = SalesforceSDKManager.getInstance().loginServerManager.selectedLoginServer?.url?.toUri() | ||
| if (selectedServer?.let { isSalesforceWelcomeDiscoveryUrlPath(it) } == true) { |
There was a problem hiding this comment.
The Compose-layer visibility gate (isWelcomeDiscoveryServer in LoginView.kt) is keyed off viewModel.selectedServer, while this programmatic guard is keyed off LoginServerManager.selectedLoginServer. These are intentionally different state sources (documented well in reloadWebView), but they create a subtle window: in phase 2 the Compose layer correctly shows the LFA item (VM's selectedServer is the My Domain), yet if this guard fires anyway (e.g. in a race or from a direct external call), the action silently no-ops while the user sees the item as active. A brief inline comment here noting the dual-source-of-truth intent would help future readers understand why the two guards differ rather than assuming one is stale.
| } | ||
|
|
||
| val isWelcomeDiscoveryServer = selectedServer.value | ||
| ?.let { LoginActivity.isSalesforceWelcomeDiscoveryUrlPath(it.toUri()) } == true |
There was a problem hiding this comment.
isWelcomeDiscoveryServer is computed from viewModel.selectedServer, which in phase 2 holds the resolved My Domain — so this correctly evaluates to false and shows the LFA item. However, the LFA guard in LoginActivity.launchLoginForAdminsAction() uses LoginServerManager.selectedLoginServer instead. These are consistent for all described scenarios, but a short comment here (e.g. // During phase 2, selectedServer is the My Domain, so this is false and LFA is shown — see LoginActivity guard for the programmatic defense-in-depth check) would make the dual-source-of-truth explicit to a future reader rather than leaving it implicit.
2 participants
Summary
Fixes the interaction between Login for Admin (LFA) and the Welcome Discovery 2-phase login flow. Previously the LFA menu item was offered even on the Welcome Discovery Phase 1 page (welcome.salesforce.com/discovery), where the registered OAuth callback (
sfdc://discocallback) is not app-unique — any app on the device that registers thesfdc://scheme could intercept the response. This change hides LFA in Phase 1, keeps it available in Phase 2 (the discovered My Domain), guardsLoginActivity.launchLoginForAdminsActionso a programmatic invocation is also a no-op for Welcome Discovery, and fixes a related reload bug.Behavior changes
1. LFA hidden on Welcome Discovery Phase 1
LoginView(Compose) now passesonLoginForAdmins = nulltoDefaultTopAppBarwhen the VM-levelselectedServerLiveData matchesLoginActivity.isSalesforceWelcomeDiscoveryUrlPath.DefaultTopAppBaralready conditionally renders the menu item whenonLoginForAdmins != null— the test wrapper signature was widened to nullable to mirror that contract.2. LFA action is also a no-op for Welcome Discovery (defense-in-depth)
LoginActivity.launchLoginForAdminsAction()now short-circuits with a singlew(TAG, ...)log whenLoginServerManager.selectedLoginServer.urlmatches the discovery path. Even if a host app calls the (deprecated) public method directly, it cannot launch a Custom Tab against welcome.salesforce.com.@Deprecatedannotation, KDoc, and visibility are unchanged. KDoc gains one line noting the no-op.3. Reload / Clear Cookies / Clear Cache / re-select Welcome → returns the WebView to Phase 1
LoginViewModel.reloadWebView()checksLoginServerManager.selectedLoginServer(the source of truth — distinct fromviewModel.selectedServer, which during Phase 2 holds the discovered My Domain). When it is Welcome Discovery, the function:dynamicBackgroundColorto white synchronously (so the top app bar background flips immediately, not afteronPageFinished),generateSalesforceWelcomeDiscoveryMobileUrl,viewModel.selectedServer.valueback to the Welcome URL so the title and the menu gating recompose,PickerBottomSheetadds an equality-branch handler: re-selecting Welcome Discovery while it is already the selected server now callsviewModel.reloadWebView()(otherwise the picker would dismiss with no observable change).4. Top app bar title is now reactive
LoginViewpreviously readviewModel.defaultTitleText(a plain Kotlin getter that callsLiveData.getValue()), which is invisible to Compose's snapshot system, so the title never recomposed whenselectedServerchanged. The composable nowobserveAsState()sselectedServerandloginUrland reads them directly inside the title computation, so the bar updates in lock-step with reload/re-select.5. Structural-only relocation
generateSalesforceWelcomeDiscoveryMobileUrlmoved from aprivate funonLoginActivityto aninternal funonLoginViewModelso both the activity (intent dispatch) and the VM (reload) can share one implementation. Bytes-identical Phase-1 URL — verified by S19 in the manual test plan.SALESFORCE_WELCOME_DISCOVERY_MOBILE_CALLBACK_URLwidened fromprivatetointernalto support the relocation. No public API surface change.Test plan — manual verification checklist
A full manual test plan with 20 numbered scenarios was authored alongside this work (kept locally; not committed). Reviewers should walk through the following scenarios on at least Android API 31 and API 36, running the AuthFlowTester sample app unless noted otherwise.
Menu gating
Reload bug fix (the WebView returns to Phase 1 on each of these)
https://welcome.salesforce.com/discovery?client_id=...&client_version=...&callback_url=sfdc%3A%2F%2Fdiscocallback.Phase-1 → Phase-2 transition (regression guard)
LFA still works in Phase 2
welcome.salesforce.comin theloadLoginPageInCustomTabpath.Multi-user
Configuration changes
FrontDoor bridge interaction
isUsingFrontDoorBridgeshort-circuit.Hybrid parity
MobileSyncExplorerHybrid→ re-run S1, S2, S4 → identical observable results.Backward compatibility (apps without Welcome Discovery)
RestExplorer(no Welcome inservers.xml) → menu list unchanged from prior release; Reload, Clear Cookies, Clear cache, Login for Admin all behave identically to the pre-change build.URL-builder relocation regression guard
dev-baseline capture: same scheme/host/path, same query parameters in the same order (client_id,client_version,callback_url),client_versionUTF-8 encoded with%2Ffor slashes,callback_url=sfdc%3A%2F%2Fdiscocallback.Cross-platform parity
Sign-off (CLAUDE.md code-review checklist)
:libs:SalesforceSDK:compileDebugKotlin).LoginViewModel.reloadWebViewKDoc reflects the Welcome branch;LoginActivity.launchLoginForAdminsActionKDoc notes the no-op for Welcome Discovery; deprecation message atLoginActivity.kt:771is still accurate.