Conversation
Community/Experience Cloud resource servers (identity, REST) never issue their own DPoP-Nonce challenge; they hard-reject a proof with no nonce instead. The client must reuse the nonce most recently issued at the /token endpoint. DPoPNonceCache was keyed strictly by (credentialsIdentifier, host) with no fallback, so a nonce harvested for the login/token host was never available when building a proof for a different resource host, causing community DPoP logins to fail. DPoPNonceCache.get() now falls back to the most recently stored nonce for the credentialsIdentifier (any host) when there is no entry for the exact host. An exact host match always takes precedence. clear()/clearAll() also remove the fallback entry. This mirrors the credential-scoped fallback already used by the iOS implementation.
JohnsonEricAtSalesforce
left a comment
JohnsonEricAtSalesforce
left a comment
There was a problem hiding this comment.
Scoped this review to the top commit (the one specific to this PR, on top of the
fix-dpop-nonce-cross-host branch it's stacked on) — not re-reviewing the DPoP nonce-cache
change underneath it.
Skip mechanism — verified, works as described. Pulled the actual JUnit XML from the live
ui-tests-pr run rather than trusting the PR description: all 12 CommunityLoginTests
scenarios report skipped="12" with
org.junit.AssumptionViolatedException: community_auth login host not present in ui_test_config.json; skipping community login tests at CommunityLoginTests.kt:81, on a shard
with failures="0". CI is genuinely unaffected by this suite today.
I also tried to break the gate itself rather than just confirming today's happy path: hasLoginHost
(UITestConfig.kt:97-99) only checks that a LoginHost entry with a matching name exists in
loginHosts — it doesn't check that entry has any configured users. users has no default, so a
community_auth block missing that key entirely fails at JSON decode time, for every test, not
just these — but a community_auth block that's present with an empty or too-short users list
would pass hasLoginHost and assumeTrue cleanly, then crash with an IndexOutOfBoundsException
on getUser's users[knownUserConfig.ordinal] instead of skipping. See the Requested Changes
below.
One thing worth asking about while we're looking at that run: 8 of the 12 skipped scenarios take
45-47 seconds to reach the skip, while the other 4 take about 2 seconds — same @Before, same
one-line assumeTrue. That's roughly 6 minutes of wall-clock per CI run spent on tests that
should be a no-op. I couldn't find a code-level reason for the split (no other test class in this
suite uses the assumeTrue-skip pattern to compare against), so it may just be Android Test
Orchestrator's per-test relaunch overhead rather than anything in this file — flagging as a
question rather than a finding.
Both currently-failing CI checks look inherited from the stacked commit, not this one. The
one unit-tests-pr (SalesforceSDK) failure is AuthenticationUtilitiesIntegrationUserTest
asserting on pre-redirect-host nonce storage — that's nonce-cache behavior this commit doesn't
touch. The ui-tests-pr failures are concentrated in DPoP-flavored assertions in
DPoPLoginTests/MultiUserLoginTests, which also doesn't match anything in this commit's diff
(this commit never touches production nonce/auth code — just test helpers). Both read as
carried over from the DPoP nonce-reuse fix underneath this branch (or pre-existing harness
flake, in the MultiUserLoginTests username-timeout case) rather than something to resolve here.
Shared-helper regression check. Read AuthFlowTest.kt's three new COMMUNITY_AUTH branches
and the switchToUser call-site change directly, since the PR touches helpers that
DPoPLoginTests/RTRLoginTests also depend on:
- The three
expectedLMarkerbranches only add a new case when
knownLoginHostConfig == COMMUNITY_AUTH; every existing call site passesREGULAR_AUTHor
ADVANCED_AUTH, so the existingelsebranches are unreached by this change. No behavior
change for existing suites. switchToUserAndValidateUsernow forwardsknownLoginHostConfiginto
app.switchToUser(...), whose second parameter already defaulted toREGULAR_AUTHbefore this
PR. Existing callers that rely on the default get the same value passed explicitly — no
behavior change.- The Custom Tab button match change (
textContains("Log In")→
textMatches("(?i).*log in.*")) is a strict superset of the old match (same literal text,
case-insensitive), so it can't stop matching a button the old code already found.
No regression found in any of the three.
Requested Changes
-
CommunityLoginTests.kt:412-418(and the equivalent legs elsewhere in the file) — none of
the 12 scenarios assert that the refresh/API request actually targets the community host. The
closest check is theFEATURE_LOGIN_SERVER_OTHER(L5) user-agent marker added in
AuthFlowTest.kt:953, which confirms the SDK's login-server classification telemetry, not
the literal resolved request host. The app's own UI already exposes this as a readable node —
UserCredentialsView.ktrenders bothInstance URLandAPI Instance URL
(currentUser?.instanceServer/apiInstanceServer) — andvalidateOAuthValues/
validateSIDsalready read sibling domain nodes (CONTENT_DOMAIN,LIGHTNING_DOMAIN) the
same way. Since the whole point of this suite is confirming the refresh leg actually lands on
the community host rather than silently falling back to the login pool, consider adding an
explicit assertion against the Instance URL node, or note in the class doc why the L5-marker
check is treated as sufficient in its place. -
UITestConfig.kt:97-99—hasLoginHostonly checks that aLoginHostentry with the
right name exists, not that it has any configured users. Acommunity_authentry present but
with an empty (or shorter than expected)userslist would pass theassumeTruegate and then
crash ingetUserwith anIndexOutOfBoundsExceptionrather than skipping cleanly — the same
failure mode the skip gate exists to avoid. Consider havinghasLoginHostalso check
users.isNotEmpty()(or whatever minimum the caller needs), so a partially-provisioned host
fails closed into a skip instead of a crash.
Open Questions
- The
ui_test_config.json.samplechange also fixes a pre-existing issue unrelated to community
coverage — a trailing comma that made the sample invalid JSON, plus placeholder org URLs
replacing real-looking subdomains. Worth confirming this bundling is intentional rather than
something that should be a separate, smaller PR — it doesn't block this one either way. - The 45-47s vs. ~2s skip-duration split noted above — is this expected Test Orchestrator
overhead, or worth a closer look before this suite is running on every PR permanently?
This review was drafted by an AI agent on behalf of @JohnsonEricAtSalesforce.
| isJwt = true, | ||
| ) | ||
| app.validateOAuthValues(knownAppConfig = ECA_JWT_DPOP, scopeSelection = EMPTY) | ||
| assertRevokeAndRefreshWorks( |
There was a problem hiding this comment.
This multi-host scenario (and the other 11 in this file) confirms the refresh/revoke cycle
succeeds and the user-agent carries the L5 "Other" marker, but doesn't assert that the
request actually targeted the community host rather than, say, a silent fallback to the
login pool. UserCredentialsView.kt already exposes Instance URL / API Instance URL as
readable nodes (the same way CONTENT_DOMAIN/LIGHTNING_DOMAIN are read in
validateOAuthValues/validateSIDs) — worth asserting against one of those directly here.
This review was drafted by an AI agent on behalf of @JohnsonEricAtSalesforce.
| useLoginPoolHost -> Features.FEATURE_LOGIN_SERVER_PRODUCTION | ||
| // community_auth is a *.my.site.com Experience Cloud site, not a *.my.salesforce.com | ||
| // My Domain host, so the SDK classifies it L5 (Other) rather than L4 (My Domain). | ||
| knownLoginHostConfig == COMMUNITY_AUTH -> Features.FEATURE_LOGIN_SERVER_OTHER |
There was a problem hiding this comment.
This marker confirms the SDK's login-server classification telemetry (L5 vs. L4), which is a
reasonable proxy but not the same as asserting the actual resolved request host — see the
note on CommunityLoginTests.kt's multi-host scenario.
This review was drafted by an AI agent on behalf of @JohnsonEricAtSalesforce.
| // via Assume.assumeTrue(testConfig.hasLoginHost(...)) rather than fail. | ||
| fun hasLoginHost(knownLoginHostConfig: KnownLoginHostConfig): Boolean = loginHosts.any { | ||
| (name, _, _) -> name == knownLoginHostConfig.name.toLowerCase(Locale.current) | ||
| } |
There was a problem hiding this comment.
This only checks that a LoginHost entry with this name exists, not that it has any
configured users. A community_auth entry present with an empty or too-short users list
would pass this check and assumeTrue, then crash in getUser with an
IndexOutOfBoundsException instead of skipping cleanly — worth also checking
users.isNotEmpty() here so a partially-provisioned host fails closed into a skip.
This review was drafted by an AI agent on behalf of @JohnsonEricAtSalesforce.
This branch has not been deployed
2 participants
Summary
Adds
CommunityLoginTeststo the AuthFlowTester UI tests. It logs in as a community (Experience Cloud site) user through the community URL, with and without DPoP, across these scenarios:The community is
https://authflowtestingmsdksdb38.test1.my.pc-rnd.site.com/customerportal, in the test1 org. These match the iOS suite (forcedotcom/SalesforceMobileSDK-iOS#4184). Spec/plan: SalesforceMobileSDK-Workspacespecs/W-23075121-W-23075124-community-login-tests/.Changes
ui_test_config.json.sample:community_authlogin host with usertandroid@community.authflowtesting.msdk.sdb38.com. No new app.loginPoolHost, and uses placeholder org URLs.servers.xml(AuthFlowTester): aUITests Communityentry for the test1 org community, so the tests select it from the server list like the other test hosts.CommunityLoginTests.kt: 12 scenarios. Each uses the same existing app as itsDPoPLoginTests/RTRLoginTestsequivalent:ECA_OPAQUE,ECA_JWT_DPOP,ECA_JWT_DPOP_RTRandECA_JWT.community_authisn't configured, so CI is unaffected.COMMUNITY_AUTHis handled intapAllowAfterLogin.switchToUserAndValidateUsernow forwards the login host.README.md: documents the community tests and the new config entry.Testing
AssumptionViolatedException) whencommunity_authisn't configured.