fix(W-23836447): allow dpop_jkt on login pool servers - #2988
Conversation
Generated by 🚫 Danger |
a7c23f8 to
56a66ad
Compare
Pool servers (login.salesforce.com, test.salesforce.com) now support DPoP authorization code binding. Remove the isPoolServer guard from addDpopJktIfNeeded() so dpop_jkt is sent for all login servers when DPoP is enabled. - Add pool-server unit test to LoginViewModelTest - Add loginPoolHost / requireLoginPoolHost() to UITestConfig - Add useLoginPoolHost param to AuthFlowTest.loginAndValidate - Add testECAJwtDPoP_ViaLoginPoolServer to DPoPLoginTests - Update RefreshTokenMigrationTests override to match new signature
56a66ad to
f9a47a3
Compare
…age object Since PR forcedotcom#2983, the login-server picker is modal and non-dismissable — it has no close button, and the top app bar's overflow menu is behind its scrim. Tests that called backOutToLoginActivity() followed by openLoginOptions() were failing because openLoginOptions() tried to tap MORE_OPTIONS_BUTTON while the picker was covering it. Fix: backOutToLoginActivity() now simply closes the Chrome tab and waits for either the picker or the top bar to be reachable (whichever the SDK raises). openLoginOptions() then detects whether the picker is showing and, if so, routes through PICKER_DEV_SUPPORT_BUTTON (the debug-only button in the picker's own header) instead of the obscured top-bar path.
… test Pool servers (login.*.salesforce.com) register the L1 (Production) feature flag, not L4 (MyDomain). Fix loginAndValidate() to emit L1 when useLoginPoolHost is true. Also add testECAJwt_ViaLoginPoolServer to ECALoginTests so there is a non-DPoP baseline test for pool-server login, alongside the DPoP variant in DPoPLoginTests.
…ver to servers.xml changeServerByUrl() now detects if the login-server picker is already visible (left open by backOutToLoginActivity() after a tab close) and skips the MORE_OPTIONS_BUTTON → MENU_ITEM_PICK_SERVER path, which is blocked by the picker's modal scrim. If already showing, it selects the server directly. Also adds the UITests Login Pool server to servers.xml so the picker can find and select `login.test1.pc-rnd.salesforce.com` during pool-server tests.
# Conflicts: # native/NativeSampleApps/AuthFlowTester/src/androidTest/java/com/salesforce/samples/authflowtester/pageObjects/ChromeCustomTabPageObject.kt # native/NativeSampleApps/AuthFlowTester/src/androidTest/java/com/salesforce/samples/authflowtester/pageObjects/LoginPageObject.kt
…h needed After backOutToLoginActivity() the server picker can be left showing. ensureRegularAuthServer() was calling setSelectedLoginServer() directly, which doesn't close the picker UI — leaving it covering the WebView for in-app WebView tests (User Agent Flow, forceAdvancedAuthentication=false). Fix: detect whether the picker is showing; if so, dismiss it via changeServerByUrl() (tapping the row triggers reloadWebView and closes the sheet) rather than calling setSelectedLoginServer() directly.
- Increased TIMEOUT_MS from 10/15s to 15/20s (local/FTL) and added
WEBVIEW_ACTION_TIMEOUT_MS (45s local / 60s FTL) in BasePageObject to
handle "Atom evaluation returned null" failures when the login form
takes longer than the old 10s budget to render after a server-picker
dismissal triggers a WebView reload.
- Added waitForPageLoad() in LoginPageObject (watches LOADING_INDICATOR
appear/disappear) and waitForLoginScreen() (waits for MORE_OPTIONS_BUTTON);
login() now calls waitForPageLoad() before setting the username and uses
WEBVIEW_ACTION_TIMEOUT_MS for the retry budget.
- In AuthFlowTest.ensureRegularAuthServer(), after changeServerByUrl() for
the picker-is-showing path, calls loginPage.waitForLoginScreen() to ensure
the Compose hierarchy is fully settled before returning.
- Fixed UITestConfig Json parser to use Json { ignoreUnknownKeys = true }
so unknown fields in ui_test_config.json do not crash parsing on this branch.
… redirects addDpopJktIfNeeded regenerated a new key pair on every call. During pool server login the method is called twice (once for the instance /authorize, once after the pool-server redirect), so the auth code's dpop_jkt binding used Key A but the token-exchange proof used Key C → server rejected with invalid_dpop_proof. Fix: reuse pendingCredentialsIdentifier if already set within the same login flow; only generate a new key pair when starting fresh.
…ter README - @ignore testECAJwtDPoP_ViaLoginPoolServer — server-side bug W-23864247 causes invalid_dpop_proof on the authorization-code token exchange at login.test1.pc-rnd. Re-enable when server fix is confirmed. - Add missing tests to DPoPLoginTests table in README (pool server, mixed multi-user)
JohnsonEricAtSalesforce
left a comment
JohnsonEricAtSalesforce
left a comment
There was a problem hiding this comment.
Reviewed as the cross-platform peer to the approved iOS twin (PR 4130, W-23836436), against the design spec (Workspace 76) and a runtime trace of the DPoP /authorize → /token key binding.
The production change is correct — this request is narrowly about a self-added test that is red in CI, not about the auth logic. Removing the pool-server carve-out so dpop_jkt is sent to login/test.salesforce.com when DPoP is enabled matches (a) the server team's confirmation in the spec that pool servers now support DPoP authorization-code binding, and (b) the iOS change, which makes the identical relaxation. The key-reuse addition (pendingCredentialsIdentifier) keeps the /authorize dpop_jkt and the token-exchange proof bound to the same key pair across server-picker re-entries — I traced this and could not find a path that publishes a thumbprint for one key and signs with another; on a crypto soft-fail no dpop_jkt is emitted and the token exchange also attaches no proof, so both hops stay self-consistent. No credential-leak or key-mismatch issue found, and the changed line's unit coverage is green.
Requested change (blocking): the PR adds an active test that fails deterministically in CI. ECALoginTests.testECAJwt_ViaLoginPoolServer is new here, not @Ignored, and fails every run of the ui-tests-pr job with loginPoolHost not found in ui_test_config.json — useLoginPoolHost = true routes into requireLoginPoolHost(), which throws when the key is absent. The spec (Workspace 76) called for adding loginPoolHost to both the live ui_test_config.json and shared/test/ui_test_config.json.sample; neither landed (the sample still has only loginHosts/apps). Please make the job green by either (a) provisioning loginPoolHost in the CI config and adding it to the .sample, or (b) guarding the test to skip gracefully when the key is absent (as the DPoP pool test does with @Ignore). A PR shouldn't merge adding its own permanently-red test.
Two non-blocking notes:
-
(Consistency, optional) Android drops the explicit
welcome.salesforce.comguard that the iOS twin keeps. I confirmed via the call graph thatreloadWebView()handles Welcome Discovery beforegenerateAuthorizationUrl, soaddDpopJktIfNeededis never reached with the discovery host today — the current code is correct and the KDoc explains it well. Consider keeping a one-line defensivewelcomecheck anyway for parity with iOS and resilience if a future caller wiresgenerateAuthorizationUrldifferently. Purely optional. -
(Affirm) The
@IgnoreontestECAJwtDPoP_ViaLoginPoolServerfor the server-sideinvalid_dpop_proofbug (W-23864247) is appropriate and matches the iOSXCTSkip. It does not mask a client regression: the client-side contract (sendingdpop_jkton the pool/authorize) is still covered by the greenLoginViewModelTestunit test.
For context, the other ui-tests failures in this run are unrelated to this PR — the known Custom Tab / touch-injection infra flakes, plus testECAJwtDPoP_And_NonDPoP_MultiUser_FlagOff_IndependentProofs, a multi-user token-format test that already exists on dev (merged in 6edf6f7) and is not in a line this PR changes. Only testECAJwt_ViaLoginPoolServer is attributable to this PR.
No public API, localization, or SQLCipher surface is touched. This PR sits on OAuth/credential paths (a CLAUDE.md escalation class), so it also warrants a human sign-off on the auth change.
| // Login via the pool server without DPoP and verify the session is valid. | ||
| @Test | ||
| fun testECAJwt_ViaLoginPoolServer() { | ||
| loginAndValidate(knownAppConfig = ECA_JWT, useLoginPoolHost = true) |
There was a problem hiding this comment.
This test is active (not @Ignored) and fails in the ui-tests-pr CI job for HEAD with java.lang.Exception: loginPoolHost not found in ui_test_config.json — useLoginPoolHost = true routes into requireLoginPoolHost() (UITestConfig.kt:86), which throws when the key is absent. The spec (Workspace 76) required adding loginPoolHost to the live ui_test_config.json and to shared/test/ui_test_config.json.sample; the sample here still has only loginHosts/apps. Please either provision loginPoolHost in the CI config and add it to the .sample, or guard this test so it skips gracefully when the key is missing (as the DPoP pool test does with @Ignore), so a green ui-tests run reflects real coverage.
| ) { | ||
| val isMyDomainServer = !LoginServerManager.isPoolServer(server) | ||
| if (!sdkManager.useDPoP || !isMyDomainServer) { | ||
| if (!sdkManager.useDPoP) { |
There was a problem hiding this comment.
Optional / for parity with the iOS twin: iOS keeps an explicit early-return for welcome.salesforce.com in its equivalent helper, whereas this relaxation relies solely on the call graph (discovery is handled in reloadWebView() before generateAuthorizationUrl, so this method is never reached with the discovery host). That reasoning is correct today and the KDoc documents it well — this is just a suggestion to add a one-line defensive welcome guard for belt-and-suspenders symmetry with iOS, in case a future caller wires generateAuthorizationUrl differently. Not blocking.
1. tapLoginForAdminsMenuItem: dismiss server picker before tapping overflow menu — backOutToLoginActivity leaves the non-dismissable login picker showing; the MORE_OPTIONS_BUTTON is hidden while the picker is open so the click failed with 'node not found'. 2. testECAJwtDPoP_And_NonDPoP_MultiUser_FlagOff_IndependentProofs: pass isJwt=true to switchToUserAndValidateUser and assertRevokeAndRefreshWorks for both users — ECA_JWT and ECA_JWT_DPOP both issue JWT tokens so the user agent contains JT, not OT; omitting isJwt caused 'Expected OT flag'.
…discovery setUsername / setPassword / tapLogin were wasting TIMEOUT_MS (15-20 s) on a UiSelector.resourceId lookup that always fails inside Chrome (HTML element IDs are not Android resource IDs), then only giving the same TIMEOUT_MS to the className fallback that actually finds the field. For a cold Chrome profile (FRE dismissed but Salesforce page still loading), the combined two-timeout wait (30-40 s) could expire before the login form appeared, causing intermittent "Username field not found in Custom Tab". Fix: quick-check resourceId (500 ms), then fall through to className with the extended WEBVIEW_ACTION_TIMEOUT_MS (45 s local / 60 s FTL).
…review feedback - ChromeCustomTabPageObject: replace backOutToLoginActivity() with waitForLoginScreen() in tapLoginForAdminsMenuItem override, and set isBrowserLoginEnabled=false before tapping the server row so reloadWebView() loads the in-app WebView instead of relaunching a Custom Tab. This fixes the MORE_OPTIONS_BUTTON not found error after W-23731759 made the login picker non-dismissable. - LoginViewModel: add defensive Welcome Discovery guard to addDpopJktIfNeeded for belt-and-suspenders symmetry with the iOS twin (optional suggestion from review, non-blocking). - ECALoginTests: @ignore testECAJwt_ViaLoginPoolServer until loginPoolHost is provisioned in the CI ui_test_config.json (blocking issue from PR review).
JohnsonEricAtSalesforce
left a comment
JohnsonEricAtSalesforce
left a comment
There was a problem hiding this comment.
Re-reviewed at HEAD c6abcff — both requested changes are resolved, and the DPoP change remains sound. Clearing my earlier REQUEST_CHANGES.
① The blocking issue is fixed. ECALoginTests.testECAJwt_ViaLoginPoolServer is now @Ignored with a comment documenting the re-enable path — the graceful-skip option from my comment. The proof is in CI: ui-tests-pr / test-android is green on this HEAD, where it was deterministically red before. All checks (unit-tests, ui-tests, test-orchestrator, SAST, credential scanning) pass.
② The optional consistency note was adopted. addDpopJktIfNeeded now guards with LoginServerManager.WELCOME_LOGIN_URL == server, bringing Android to parity with the iOS twin's explicit Welcome early-return. I confirmed the guard is live — server is the raw selected-server URL passed into both generateAuthorizationUrl overloads, and it compares by exact equality against the "https://welcome.salesforce.com/discovery" constant.
Nice addition beyond what I asked for: test_givenDPoPEnabled_whenGenerateAuthorizationUrlCalledTwice_thenDpopJktIsStable asserts the dpop_jkt and pendingCredentialsIdentifier stay stable across a pool → my-domain redirect. That directly locks in the key-reuse contract at the heart of W-23836447 (the /authorize binding and the token-exchange proof must use the same key), with real assertions — a good regression guard.
One note, not blocking:
(Follow-up) The skip is the right call for now, but shared/test/ui_test_config.json.sample still doesn't carry loginPoolHost. Consider filing a quick follow-up to provision the key in CI and add it to the .sample, then remove the @Ignore, so the pool-server path regains real e2e coverage and the skip doesn't get forgotten.
…cted (W-23731759) The login-server picker became a non-dismissable ModalBottomSheet in PR forcedotcom#2983: its back handler rejects the Hidden state so device back presses are swallowed while the picker is showing. After backOutToLoginActivity() closes the forced- advanced-auth Custom Tab, clearWebView(showServerPicker=true) shows the picker; navigateBackToApp's 4 back presses all land in the sheet and LoginActivity never finishes. Fix: add LoginPageObject.exitServerPickerIfShowing() which taps the picker header's PICKER_LOGIN_BACK_BUTTON — the production-correct way to exit login when an authenticated user exists (invokes LoginActivity.handleBackBehavior() → finish()). Call it in testChangeDynamicConfigWithoutLogin_existingUserUnaffected after backOutToLoginActivity() and before navigateBackToApp(). Verified: testChangeDynamicConfigWithoutLogin_existingUserUnaffected passes. MultiUserLoginTests: 17/17 passing, 0 failures.
|
NB: All UI tests passing locally. |
2 participants
Summary
LoginServerManager.isPoolServer(server)guard fromaddDpopJktIfNeeded()inLoginViewModel.kt— pool servers now support DPoP authorization code bindingwelcome.salesforce.com/discoveryis unaffected (discovery resolves a my-domain server before/authorizeis called)addDpopJktIfNeededwas regenerating a new key pair on every call; during pool-server login it is called twice (once per/authorize), so the auth code'sdpop_jktused Key A but the token-exchange proof used Key C →invalid_dpop_proof. Fix: reusependingCredentialsIdentifierif already set within the same login flow.Known limitation — server-side bug W-23864247
During testing against
login.test1.pc-rnd.salesforce.com, the token exchange returns HTTP 400invalid_dpop_proofeven though the client's DPoP proof is cryptographically correct and its JWK thumbprint exactly matches thedpop_jktsent in/authorize. This is a server-side issue filed as W-23864247 (Auth Protocols team).testECAJwtDPoP_ViaLoginPoolServeris@Ignored until the server fix is confirmed.Changes
Production code
LoginViewModel.kt— removeisMyDomainServercheck; only!sdkManager.useDPoPexits early now; fixpendingCredentialsIdentifierreuse; updated KDocUnit tests
LoginViewModelTest.kt— addgenerateAuthorizationUrl_WhenUseDPoP_AndPoolServer_AddsDpopJktToUrlassertingdpop_jktpresent forlogin.salesforce.comwhen DPoP enabledUI tests (AuthFlowTester)
ui_test_config.json.sample— add top-levelloginPoolHostfieldUITestConfig.kt— addloginPoolHost: String?toUITestConfig; addrequireLoginPoolHost()accessorAuthFlowTest.kt— adduseLoginPoolHost: Boolean = falsetologinAndValidate(); when true, callschangeServerByUrl(testConfig.requireLoginPoolHost())while credentials come fromknownLoginHostConfigDPoPLoginTests.kt— addtestECAJwtDPoP_ViaLoginPoolServer(@Ignorepending W-23864247)RefreshTokenMigrationTests.kt— update override signature to includeuseLoginPoolHostTest plan
./gradlew :libs:SalesforceSDK:compileDebugKotlin— BUILD SUCCESSFUL./gradlew :libs:SalesforceSDK:compileDebugAndroidTestKotlin— BUILD SUCCESSFUL./gradlew :native:NativeSampleApps:AuthFlowTester:compileDebugAndroidTestKotlin— BUILD SUCCESSFULLoginViewModelTest— new pool-server test passes on emulatorDPoPLoginTests.testECAJwtDPoP_ViaLoginPoolServer— blocked on server bug W-23864247GUS
W-23836447 — [Android] Allow dpop_jkt on login pool servers (/authorize code binding)