@W-23201591: [Android] Surface RTR state in developer info screen - #2974
Conversation
| * @param appFeatureCode The app feature code | ||
| * @param user The user account, or null to use the current user | ||
| */ | ||
| internal fun isUserFeatureRegistered(appFeatureCode: String, user: UserAccount? = null): Boolean { |
There was a problem hiding this comment.
New read accessor for per-user feature flags. RTR-active state is stored as the per-user feature flag RT that ClientManager registers on a confirmed rotation. The backing perUserFeatures map is private and the existing public surface only lets you write (registerUsedAppFeature) or read the whole aggregated user-agent string — there was no narrow "is this one code set for this user?" read.
Rather than widen perUserFeatures visibility, this adds a focused internal accessor. It resolves the target user (explicit arg → current user → false if neither), builds the same "orgId/userId" key the writers use (registerUsedAppFeature/unregisterUsedAppFeature/getUserAgent), and does a null-safe, thread-safe (ConcurrentHashMap/ConcurrentSkipListSet), case-insensitive membership test. The optional user param defaults to the current user for ergonomics; the sole caller today passes the user explicitly, and the fallback branch is covered by its own test.
This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.
| @VisibleForTesting(otherwise = PRIVATE) | ||
| internal fun forceTokenRefresh( | ||
| user: UserAccount, | ||
| restClient: RestClient = clientManager.peekRestClient(user) |
There was a problem hiding this comment.
Default-argument seam instead of widening production surface. forceTokenRefresh backs the debug-only "Force Token Refresh" dev action; it drives the SDK's standard refresh path so a developer can watch RTR state update without waiting for natural token expiry. It's the testable core — the dev-action lambda only dispatches it on a coroutine and shows the result in a Toast.
To make it testable without a network call, restClient defaults to clientManager.peekRestClient(user) (the production path, unchanged at the forceTokenRefresh(user) call site) and tests pass a mock RestClient. This mirrors the existing invokeServerNotificationAction(..., restClient: RestClient = clientManager.peekRestClient(...)) idiom in this same file. An earlier revision made clientManager open for a test subclass to override — this seam replaces that, so there is no production-visibility change. It's @VisibleForTesting(otherwise = PRIVATE) internal, and it never throws: any failure is caught, logged, and returned as a message.
This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.
There was a problem hiding this comment.
Could peekRestClient throw?
There was a problem hiding this comment.
Good catch — yes. peekRestClient(user) throws AccountInfoNotFoundException (a RuntimeException) when there's no account, the user is mid-logout, or auth-token/URL/id data is missing. Because it was a default argument it was evaluated before the try, so it could escape uncaught — contradicting the "never throws" contract. Fixed by resolving the client inside the try ((restClient ?: clientManager.peekRestClient(user))) and correcting the KDoc, plus a regression test covering the default path. Commit: dd055af
This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.
| * when the token last rotated. | ||
| */ | ||
| val currentUser = userAccountManager.cachedCurrentUser | ||
| additionalSections.add( |
There was a problem hiding this comment.
Why the RTR section is appended via additionalSections rather than a new field. createFromLegacyDevInfos(devSupportInfos) returns a DevSupportInfo that is already fully populated — its five standard sections (basic info, auth config, boot config, current user, runtime config) are immutable vals set at construction. additionalSections is the only mutable member, and it's the designed extension point for sections the fixed schema didn't anticipate. So .apply { additionalSections.add(...) } is the one legal, minimal-touch way to inject RTR post-construction.
Deliberately not done: (a) editing createFromLegacyDevInfos, which is marked // TODO: Remove in 14.0; (b) adding a dedicated rtrSection field, which would change the data-class signature and require wiring into both the legacy path and the future implementation.
devSupportInfo getter just above builds DevSupportInfo via its structured constructor and does not append this RTR section. Whoever swaps that in when devSupportInfos is removed must carry the RTR additionalSections.add(...) over, or RTR silently drops from the screen.
This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.
There was a problem hiding this comment.
@JohnsonEricAtSalesforce If you cannot add RTR to the new impl comment above, please add a comment noting that RTR needs to be added so it does not get lost.
There was a problem hiding this comment.
Done — added a note in the commented-out 14.0 devSupportInfo implementation (and expanded the TODO) that the RTR additionalSections.add(...) from the live getter must be carried over, so it can't be missed when the block is swapped in. Commit: dd055af
This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.
Generated by 🚫 Danger |
99273ba to
bcf8970
Compare
Demo — RTR state in the developer info screenShort screen recording of the feature running end to end in RestExplorer against an Tip The clip is brief and moves quickly — playing it at half speed (0.5×) makes the What it shows:
rtr_demo_clean.mp4Note The 
AC1 (per-user fields show This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce. |
Note on CodeCov reportingThis pull request has no CodeCov status or coverage comment. I dug into the CI logs, and this is Root cause: CodeCov isn't a standalone check; it's an upload step at the end of each Evidence it's not this PR: the identical Coverage for this change was measured locally: 84.9% patch coverage on the changed lines, The CI fix (collect the per-shard This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce. |
🔎 Root-cause analysis: CodeCov coverage has been silently broken repo-wide since test shardingPosting this separately from the coverage note above because it's a CI-pipeline issue that affects Symptom: No CodeCov status or coverage comment appears on PRs. There's no red check, because the Root cause — the sharded Firebase Test Lab run never produces the
Evidence it's environment-wide (not one branch/lib/fork): the identical
So it has been broken for every PR since sharding landed. Suggested fix (out of scope for this PR): adapt coverage collection to the sharded layout — This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce. |
brandonpage
left a comment
brandonpage
left a comment
There was a problem hiding this comment.
LGTM!
But please add a comment to the new dev info impl so RTR status does not get lost. I also would not mind seeing this added to the AuthFlowTester app's UI, but we can do that another day 😉
| public void setLastTokenRotationTime(String lastTokenRotationTime) { | ||
| this.lastTokenRotationTime = lastTokenRotationTime; | ||
| } |
There was a problem hiding this comment.
It will be nice when everything is Kotlin and we can make things like this internal. Not necessary for this PR, but curious if you have an opinion on RestrictTo? It does not actually prevent someone from using the API, but adds a stern lint warning/error indicating that we do not want them to.
There was a problem hiding this comment.
Agree that's the right direction for library-internal API that has to stay public from Java. I'd rather not add it to just these two accessors here, since the sibling UserAccount accessors (e.g. tokenType) aren't annotated and a partial application would be inconsistent — better as a uniform sweep across the account accessors. Filed W-23667824 to track that. Out of scope for this PR.
This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.
| * menu is only shown when isDevSupportEnabled() is true (debug | ||
| * builds by default). | ||
| */ | ||
| actions["Force Token Refresh"] = object : DevActionHandler { |
There was a problem hiding this comment.
@brandonpage Ditto. Let me file a ticket for it and we can take care of it next week.
There was a problem hiding this comment.
Sounds good — thanks for filing the iOS ticket, @sfdctaka. Happy to help with the parity work when it's up. Likewise the AuthFlowTester RTR UI is a nice follow-up for another day.
This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.
| * when the token last rotated. | ||
| */ | ||
| val currentUser = userAccountManager.cachedCurrentUser | ||
| additionalSections.add( |
There was a problem hiding this comment.
@JohnsonEricAtSalesforce If you cannot add RTR to the new impl comment above, please add a comment noting that RTR needs to be added so it does not get lost.
…s in forceTokenRefresh; note RTR carry-over for 14.0 - forceTokenRefresh: resolve peekRestClient inside the try so AccountInfoNotFoundException (no account / logging out / missing token data) is caught rather than thrown; fix the KDoc that claimed it never throws. Add a regression test for the default (production) path. - devSupportInfo: note in the commented-out 14.0 implementation that the RTR additionalSections.add(...) must be carried over, or RTR silently drops from the dev info screen.
3 participants
Summary
Refresh Token Rotation (RTR) is a server-driven security feature: when the token endpoint
issues a new refresh token in place of the one that was presented, the old refresh token is
retired. This pull request surfaces that RTR state in the Salesforce Mobile SDK developer info
screen (the on-device diagnostics screen reachable from the developer support menu) so that
developers can confirm, on-device, whether Refresh Token Rotation is active for the currently
logged-in user and when the refresh token last rotated.
It adds a new "RTR" section to the developer info screen with two rows:
trueorfalse, reflecting whether the Refresh Token Rotation per-userfeature flag (an entry the SDK records per user under the code
RT) is registered for thecurrent user; shows
N/Awhen no user is logged in.ISO 8601 timestamp (the internationally standardized date-time format, for example
2026-07-30T12:34:56Z); showsNeveruntil the first rotation, orN/Awhen no user islogged in.
To make the rotation timestamp durable across app restarts, a new
lastTokenRotationTimefieldis persisted on the
UserAccountobject. It travels through the full account persistence path,including the encrypt-on-write and decrypt-on-read handling used to store account data in
Android's
AccountManager. The timestamp is stamped at the existing point inClientManagerwhere a rotation is confirmed.
This work builds on the Refresh Token Rotation per-user feature flag (feature code
RT) thatlanded in work item W-23195021. It is the Android counterpart to the iOS work item W-23201588,
and uses the same field labels and display semantics.
Work item: W-23201591
Acceptance criteria
N/Awhen no user is logged in. (Verified by unit test and on-device.)RTR Active: falseandLast Rotation: Never. (Verified by unit test and by the on-device demo below.)RTR ActiveshowstrueandLast Rotationshows a valid timestamp. (Verified by unit test and by the on-device demo below.)UserAccountobject rather than held only in memory. (Verified by an encrypt/decrypt round-trip unit test.)Changes
Production (7 files)
UserAccount.java— newlastTokenRotationTimefield carried across the full persistence path (the string constant, the field, the JSON constructor, theBundleconstructor, the getter and setter,toJson, andtoBundle), mirroring how the existingtokenTypefield is handled.UserAccountBuilder.kt— alastTokenRotationTime(...)builder method, plus the populate-from-existing-account and allow-unset wiring.UserAccountManager.java— encrypt on write and decrypt on read when storing the field in Android'sAccountManageruser data.AuthenticatorService.java— a newKEY_LAST_TOKEN_ROTATION_TIMEuser-data key.ClientManager.java— stampssetLastTokenRotationTimeat the point where a rotation is confirmed (before the primaryupdateAccountcall), then registers the Refresh Token Rotation feature.SalesforceSDKManager.kt— anisUserFeatureRegistered(...)helper; the RTR section wired into thedevSupportInfogetter; a debug-only "Force Token Refresh" developer action; and an extractedforceTokenRefresh(user)function to make that action testable.forceTokenRefreshtakes an optionalrestClientparameter that defaults to the current user's client, so tests can inject a mock without any production-visibility change (matching the existinginvokeServerNotificationActionpattern).DevSupportInfo.kt— aparseRtrSection(...)function that builds the "RTR" section.Tests (6 files) — 134 scoped tests, all passing:
DevSupportInfoTest.kt,UserAccountTest.java,UserAccountManagerTest.java,SalesforceSDKManagerTests.kt,ClientManagerMockTest.kt,DevInfoActivityTest.kt.Testing
SalesforceSDKManagerTests,DevSupportInfoTest,UserAccountTest,UserAccountManagerTest,ClientManagerMockTest) plus 10 instrumented UI tests (DevInfoActivityTest).Toast(a brief on-screen message); that block is not practically unit-testable, and all of its real logic lives in the testedforceTokenRefreshfunction../gradlew :libs:SalesforceSDK:lintDebug(the Android lint static-analysis check) runs clean, with no new warnings.Reviewer escalation flags (per the repository's CLAUDE.md "Stop and Flag" guidance)
UserAccount, plus the encrypt/decrypt handling inUserAccountManager— this touches the account persistence path.ClientManagerrotation block was reordered to stamp the timestamp before the primary account write.UserAccount.getLastTokenRotationTime()/setLastTokenRotationTime(...), andUserAccountBuilder.lastTokenRotationTime(...).sf__strings.xmlchanges — the developer-info labels are hardcoded, consistent with the existing sections on that screen.This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.