@W-24296713: Add Android ephemeral auth sessions - #3052
Conversation
Generated by 🚫 Danger |
wmathurin
left a comment
wmathurin
left a comment
There was a problem hiding this comment.
Review against the W-24296713 spec/plan (Workspace PR #117). The implementation matches the spec closely and CI is green — 4 non-blocking follow-ups below (2 test-coverage gaps that the plan explicitly promised, 1 spec-wording divergence, 1 cosmetic).
|
|
||
| // endregion | ||
|
|
||
| // region Ephemeral Custom Tabs |
There was a problem hiding this comment.
Missing test: prompt=login independence. Plan §1 (bullet 6), the spec acceptance criterion ("sharedBrowserSession continues to control prompt=login independently"), and the test-plan "URL policy" row all call for extending an existing sharedBrowserSession test to assert prompt=login stays independent of the new ephemeral option. This region adds the ephemeral tests but that independence assertion is absent. Low risk (the new flag never enters buildCustomTabAuthorizeUrl, so independence is structural) but it was an explicitly promised test.
There was a problem hiding this comment.
Added buildCustomTabAuthorizeUrl_promptLoginDependsOnlyOnSharedBrowserSession, which exercises both ephemeral values and verifies that only sharedBrowserSession controls whether prompt=login is present.
| setOpenInBrowserButtonState(OPEN_IN_BROWSER_STATE_OFF) | ||
| setInstantAppsEnabled(false) | ||
| setBackgroundInteractionEnabled(false) | ||
| configureEphemeralBrowsing( |
There was a problem hiding this comment.
Integration wiring is untested. The three configureEphemeralBrowsing_* tests exercise the extension in isolation (mocked LoginActivity + real CustomTabsIntent.Builder via callOriginal()). Nothing verifies that loadLoginPageInCustomTab actually calls it here with enabled = sdkManager.useEphemeralSessionForAdvancedAuth and the resolved provider. So reading the SDK flag and the takeIf provider resolution have no coverage — the test-plan’s "Custom Tab enabled/disabled" rows are satisfied only at the helper level, not at the launch path. Suggest one test asserting the wiring.
There was a problem hiding this comment.
Added an ActivityScenario integration test that invokes the real loadLoginPageInCustomTab method twice, captures the launched AndroidX intents, and verifies the current manager setting plus configured browser package. The test bypasses only callback registration through a narrow injected check, so no test AndroidManifest change is required.
| setBackgroundInteractionEnabled(false) | ||
| configureEphemeralBrowsing( | ||
| enabled = sdkManager.useEphemeralSessionForAdvancedAuth, | ||
| provider = customTabBrowser.takeIf { customTabBrowserExists }, |
There was a problem hiding this comment.
Capability check skips the default-browser fallback. Spec (Custom Tab Launch, step 2) says resolve the provider using the configured-browser and default-browser fallback. Here provider is null whenever the configured browser isn’t installed, so isEphemeralBrowsingSupported() is skipped and no diagnostic warning fires even if the OS default browser lacks ephemeral support. Behavior is still correct (ephemeral is still requested, launch proceeds) — only the best-effort warning is missing for that path. For true parity with the spec, resolve via CustomTabsClient.getPackageName(this, null) when the configured package is absent; otherwise tighten the spec to say the warning is best-effort for the configured browser only.
There was a problem hiding this comment.
Good catch. I removed this from the code to include as few behavior/code changes as possible but did not update the spec.
There was a problem hiding this comment.
Updated the workspace spec, plan, and test plan in SalesforceMobileSDK-Workspace#117 to remove default-browser fallback capability resolution. The documented check/warning is now scoped to an installed configured customTabBrowser; the existing fallback remains unchanged and unpinned.
| } | ||
|
|
||
| @Test | ||
| fun devSupportInfo_IncludesEphemeralAdvancedAuthentication_WithCurrentValue() { |
There was a problem hiding this comment.
Cosmetic: Plan §1 named DevSupportInfoTest.kt as the location for the developer-support test; it landed here in LoginOptionsActivityTest.kt instead. The test exists and passes, so this is purely a location note — no change needed unless you want to match the plan.
There was a problem hiding this comment.
Aligned the workspace plan and test plan with the actual test location in LoginOptionsActivityTest.kt; the unused DevSupportInfoTest.kt location is no longer listed.
wmathurin
left a comment
wmathurin
left a comment
There was a problem hiding this comment.
Approving. All four review findings are addressed:
- Default-browser fallback divergence and dev-support test location — reconciled by updating the spec/plan/test-plan in Workspace PR #117 to match the code.
prompt=loginindependence — covered by the extendedbuildCustomTabAuthorizeUrl_promptLoginDependsOnlyOnSharedBrowserSessiontest (loops both ephemeral × both sharedBrowserSession values).- Integration wiring — covered by the new
loadLoginPageInCustomTab_usesCurrentEphemeralSettingAndConfiguredBrowserscenario test, which asserts the real launched Intent's ephemeral extra tracks the SDK flag and the configured browser package. The injectedcallbackSchemeRegisteredseam keeps production behavior unchanged.
Implementation matches the spec, API usage is correct against AndroidX Browser 1.10.0, and the change is appropriately scoped. Please just confirm the pending unit-tests-pr / ui-tests-pr jobs go green before merging.
2 participants
Summary
useEphemeralSessionForAdvancedAuthSDK option