Repository navigation
@W-23075124@: Add community login UI tests - #3055
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.
Correct the class/get() KDoc and matching test comments: Salesforce issues DPoP-Nonce only from the token endpoint, never from a resource server (identity, REST) — this isn't specific to communities. Resource-server responses never carry DPoP-Nonce, so the client reuses the latest token-endpoint nonce for the credential on every DPoP call; an exact per-host entry still takes precedence if a server ever does issue one. Logins where the token host differs from the resource hosts (communities, login.* pool servers) rely on this fallback.
Pre-seed the pre-redirect host with its own distinct nonce so the cross-host-redirect test still proves the harvest lands under the response host rather than the pre-redirect host. The new credential- wide fallback in DPoPNonceCache.get() made the old assertNull on the pre-redirect host pass vacuously (via the fallback) regardless of which host the harvest actually wrote to.
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.
- validateSIDs/validateOAuthValues: add an isCommunity exception so the content/lightning/visualforce domain+SID checks aren't required for community (Experience Cloud) logins. These fields are parsed verbatim from the token endpoint's response (see OAuth2.java) and are genuinely empty for the test1 org's community, confirmed against ECA_OPAQUE hybrid login where mainSid was correctly populated. - Thread knownLoginHostConfig through validateOAuthValues call sites (loginAndValidate, migrateAndValidate, upgradeToDPoPAndValidate, downgradeFromDPoPAndValidate) and one direct CommunityLoginTests call site so the exception applies to every community scenario. - Add a "UITests Community" row to AuthFlowTester's servers.xml with the real community URL, consistent with the existing real UITests/Adv Auth rows already there, so changeServer can select it without any runtime-added-connection test code. - Fix a placeholder username typo in ui_test_config.json.sample (tandroid, not testandroid) to match the real test1 org user. Temporary WIP commit before rebasing onto fix-dpop-nonce-cross-host.
…oP relogin, refresh-endpoint assertions Add testCommunity_WithRestart, testCommunityJwt_Hybrid and testCommunityJwt_NoHybrid. Replace testCommunity_LogoutAndRelogin_DPoP with testCommunity_BearerLogoutThenReloginWithDPoP. Assert the stored community URL and that token refresh requests target the configured community host and path. Harden the community login page objects (readback-and-retype, CSS selectors). Update the AuthFlowTester README.
856a6f0 to
f09c956
Compare
Generated by 🚫 Danger |
…L, refresh-and-replay loop Community users got 403 Wrong_Org on the identity call when the id host was swapped for the instance host. Use the token-response id URL for the initial call and every replay, and keep refreshing and replaying on 401/known 403 (iOS is unbounded and needed up to 10 cycles; Android is capped at 20 and logs a warning when it gives up). Non-community behaviour is unchanged. Adds unit tests for URL selection, success on the Nth replay and the cap, and updates docs/auth/token-lifecycle.md.
iOS needed up to 13 refresh/replay cycles before the identity service accepted a community token.
Token migration built the authorize path and did the code exchange against the instance host, dropping the community path prefix, so upgrade/downgrade in place never completed for community users.
- Fire input/change/blur events after typing into the community Aura login form in the in-app WebView - Pass the login host to the final refresh assertion of upgrade/downgrade - Read the token type of a Bearer session without requiring a DPoP nonce - Restart the app after logging out the last user before the DPoP relogin
Migration uses the account's community URL, so the community path prefix is kept. iOS reaches the same result via the global login host preference (host plus path), with the singleaccess call going to the user's community URL.
…refresh loop, attempt logging
…st, community_auth, IDENTITY_ATTEMPTS; fix sample community username
…admin and multi-user login tests
|
Thanks for the review. All three requested items are addressed: the resolved-host assertions in f09c956 and the CI failures: these are not inherited from #3054 (now merged). The Open question 1 (sample config cleanups): intentional. The sample did not parse as JSON (trailing comma) and was missing Open question 2 (45–47 s vs ~2 s skips): I have not investigated it. My guess is Test Orchestrator per-test relaunch overhead, since the |
The identity fetch that finishes a token migration can hit the intermittent login-host /id 403 (W-24433488) and refresh, which rotates the refresh token on an RTR app. migrateAndValidate asserted the pre-migration RT state and failed with 'Expected no RT flag' in testMigrate_ECAJwtDPoP_To_ECAJwtDPoPRtr. Seed from the persisted lastTokenRotationTime, as loginAndValidate does.
brandonpage
left a comment
brandonpage
left a comment
There was a problem hiding this comment.
Approving as to not block the new community UI tests, but I would like to see some changes in the identity call retry flow in the future.
| var refreshes = 0 | ||
| var refreshTokenRotated = false | ||
|
|
||
| fun finish(status: String) = logIdentityAttempts( |
There was a problem hiding this comment.
Why is this called finish? That name does not describe what the function does and at a glance can easily be mistaken for the common Activity function.
| while (true) { | ||
| try { | ||
| attempts++ | ||
| val identity = identityFetcher(tokenResponse.idUrl, tokenResponse) | ||
| finish("200") | ||
| return IdentityFetchResult( | ||
| identity = identity, | ||
| refreshTokenRotationTime = if (refreshTokenRotated) Instant.now().toString() else null, | ||
| ) |
There was a problem hiding this comment.
I really don't like using while (true) in any circumstance. A reasonably large attempt threshold would be much safer.
And this is using try catch as control flow.
JohnsonEricAtSalesforce
left a comment
JohnsonEricAtSalesforce
left a comment
There was a problem hiding this comment.
Thanks for the updates. The two test-code requests are addressed. Approving, in line with
Brandon's approval and because the identity retry matches iOS. We would still like to see these two
items addressed, but neither blocks merge:
-
Follow-up: bound the identity retry.
fetchUserIdentityWithRetry
(AuthenticationUtilities.kt:565) loopswhile (true)with no cap or delay, and refreshes the
token on every 401/403 (:595). We agree with Brandon's comment on the same loop.The only reason we can see for the loop is the transient
Wrong_Orgrejection right after
login, which is now reported fixed server-side. An earlier revision of this PR capped it at 20,
then 30, against the 10 to 13 consecutive rejections observed. Dropping the cap to match iOS
copies a retry that predates this failure mode. On iOS the same uncapped retry dates from 2015,
and its code comment describes handling an expired session.A persistent 403 would keep the login spinner up indefinitely and hit the token endpoint on
every cycle, rotating the refresh token each time for RTR apps. Please:- Restore a cap as a bounded
forloop that rethrows the last identity error when exhausted,
with a distinctIDENTITY_ATTEMPTSstatus such asexhausted. A cap of 30 (see below). - Replace
keepsRefreshingWithoutCapUntilReplaySucceedswith a test that succeeds within the
cap and one that never succeeds. - Update the KDoc,
token-lifecycle.mdand the AuthFlowTester README, which currently say "no
cap". - Fix the comment at
:654("permits exactly one refresh"), which contradicts the loop.
Why 30 is enough. The most consecutive rejections we know of in testing is 13, and an earlier
revision of this PR already used a cap of 30. In AuthFlowTester runs on iOS, the identity
service rejected 74 requests across 39 logins. Each login ends in exactly one success, which
works out to about two in three requests being rejected, or about 1.9 rejections per login on
average. Each replay used a freshly refreshed token, so we treat the rejections as roughly
independent. On that model the chance of N rejections in a row is about 0.65 to the power N:Cap Chance a login is still rejected 13 about 1 in 250 20 about 1 in 5,000 30 about 3 in a million So 30 covers every observed case with a wide margin and still ends a persistent failure instead
of spinning forever. This is an estimate from a small sample, so please make the cap a named
constant that is easy to adjust.This changes the OAuth flow, so it also needs maintainer sign-off.
- Restore a cap as a bounded
-
Test evidence. Each of these needs a CI change or output from a real-org run:
- Community tests: At
771a90fb6CI skipped all 15 (pr-loginresult: 15 skipped, all
CommunityLoginTests), so the new token-endpoint assertion has not executed anywhere. Is
community_auth, with at least one user, provisioned in the CIUI_TEST_CONFIGsecret? The
skips suggest it is not. The new dedicated community shard cannot run in this PR's CI:
pr.yamltriggers onpull_request_target, so the workflow comes from the base branch, and
the run has no community step and nopr-communityresults. After merge it would first run
in the nightly job, and it would skip again unless the secret is provisioned. If it is not,
please attach output from a real-org run ofCommunityLoginTestsbefore merge, and say when
the secret will be updated. - Community token migration:
TokenMigrationActivity.kt:148now authorizes and exchanges the
code againstcommunityUrl ?: instanceServer. The in-place DPoP upgrade and downgrade community
tests exercise this, but they were among the skipped tests. Please include them in the run above
so a path-bearing community URL is shown working end to end. - Regular login: the first identity request now uses the raw
idUrlfor every login
(:568). This is unit-tested with a fake fetcher only. ShareIDENTITY_ATTEMPTSoutput from a
regular My Domain login. We expect attempts=1 and refreshes=0.
- Community tests: At
This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.
…(review follow-ups from forcedotcom#3055)
Summary
Adds
CommunityLoginTests(15 tests) to the AuthFlowTester UI tests: login as a community (Experience Cloud) user through the community URL, with opaque, JWT and DPoP tokens. It covers hybrid and non-hybrid flow, refresh-token rotation, restart, Bearer-then-DPoP relogin, DPoP upgrade/downgrade, multi-user isolation and the in-app WebView. Each test also asserts the refresh goes to the community host and path (communityUrl > instanceUrl > domain), including after restart.The same 15 tests exist on iOS (forcedotcom/SalesforceMobileSDK-iOS#4184).
Spec/plan: SalesforceMobileSDK-Workspace
specs/W-23075121-W-23075124-community-login-tests/.SDK changes
fetchUserIdentityWithRetry): the request goes to the token responseidURL as returned, for all users. A 401 or 403 refreshes the credentials and replays, repeating until it succeeds (no cap, as on iOS). This is what made community JWT logins fail on Android, because the identity service rejects valid JWT tokens (403Wrong_Org) a variable number of times right after login.IDENTITY_ATTEMPTS ...info line per login (same format as iOS).TokenMigrationActivity): community users migrate against their community URL instead of the instance host.RTat login: Android already recordslastTokenRotationTimeand registersRT. iOS now does too (#4184).docs/auth/token-lifecycle.md.Test changes
CommunityLoginTests.kt: 15 scenarios, each using the same existing app as itsDPoPLoginTests/RTRLoginTestsequivalent. Skips (JUnit assumption) whencommunity_authisn't configured.community_authlogin host inui_test_config.json.sample(no new app; also fixes the trailing comma,loginPoolHostand placeholder URLs) and aUITests Communityentry in AuthFlowTesterservers.xml.RecordingHttpAccessrecords refresh URLs. Community hosts are reported as login server L5 (Other). Custom Tab "Log in" match is case-insensitive, plus small fixes for Aura form input,getDpopInfoon Bearer sessions and restart handling.RTexpectations:MultiUserLoginTestsandadminLoginAndValidatenow use tracked per-user state seeded fromlastTokenRotationTime, instead of hard-coding "noRT".ECALoginTests.kt: three opaque tests (testECAOpaque_Hybrid,_NoHybrid,_ViaLoginPoolServer). The CI check for the expected number of logical tests per API level is raised from 110 to 128.README.mdupdated.reusable-ui-workflow.yamlgets a dedicatedCommunityLoginTestsshard (PR and nightly), excluded from the catch-all Login Tests shard. The tests skip untilcommunity_authis added to theUI_TEST_CONFIGsecret.migrateAndValidate: folds the persistedlastTokenRotationTimeinto the expectedRTmarker after a migration. The identity fetch that ends a migration can hit the intermittent403 Wrong_Org(W-24433488), refresh, and rotate on an RTR app. TODO(W-24433488): assert noRTagain once that is fixed on the test org.