[PM-24195] fix: Pass userId to refreshToken and setTokens

[LRNcardozoWDF]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-24195

📔 Objective

There's a possible race condition when refreshing the token with that in mind I'm adding more logs and passing the userId of the active account.

[github-actions]

Checkmarx One – Scan Summary & Details – c7dde1e9-45c0-4bdc-b37a-919bc2254259

Great job! No new security vulnerabilities introduced in this pull request

[codecov]

Codecov Report

❌ Patch coverage is 96.47577% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.81%. Comparing base (daaf1e8) to head (70f5629).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...atform/Services/TestHelpers/MockTokenService.swift	84.61%	4 Missing ⚠️
...tform/Services/API/AccountTokenProviderTests.swift	96.34%	3 Missing ⚠️
...red/Core/Platform/Services/TokenServiceTests.swift	97.56%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2374      +/-   ##
==========================================
- Coverage   86.92%   85.81%   -1.12%     
==========================================
  Files        1852     2083     +231     
  Lines      163591   178622   +15031     
==========================================
+ Hits       142203   153285   +11082     
- Misses      21388    25337    +3949     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[claude]

Claude finished @LRNcardozoWDF's task in 3m 14s —— View job

---

Code Review — PR #2374

• Gather context and read changed files
• Review code changes against Bitwarden standards
• Post review findings

Overall Assessment: APPROVE

This PR fixes a race condition in token refresh by capturing the active userId upfront and threading it explicitly through getRefreshToken(userId:) and setTokens(..., userId:), preventing tokens from being stored under the wrong account during account switches. It also adds targeted Crashlytics logging for the "Cipher was not encrypted for the current user" server error by wrapping it in BitwardenError.generalError to bypass NonLoggableError suppression, while preserving generic error logging for all other failures.

The implementation addresses all previous review feedback (both Claude and fedemkr). Test coverage is comprehensive, including race condition simulation via MockHTTPClient.onRequest callbacks. No new issues found.

Code Review Details

No findings to report. All changed code follows established patterns, security requirements are met, and error handling is correct.

[claude]

⚠️ IMPORTANT - Generic error logging removed, creating a monitoring gap

The original code called services.errorReporter.log(error: error) for all errors from save operations. The new code only logs the specific ServerError matching "Cipher was not encrypted for the current user".

While ServerError (as a NonLoggableError) and URLError were already filtered out by CrashlyticsErrorReporter, other non-network errors (encryption failures, SDK errors, state service errors) were reaching Crashlytics before and will now be silently dropped.

The targeted BitwardenError.generalError logging should be added alongside the existing generic logging, not replace it:

Suggested change

	await coordinator.showErrorAlert(error: error)
	services.errorReporter.log(error: error)
	if case let ServerError.error(errorResponse: errorResponse) = error,
	errorResponse.message.contains("Cipher was not encrypted for the current user") {
	services.errorReporter.log(error: BitwardenError.generalError(
	type: "Save item failed",
	message: errorResponse.message,
	error: error,
	))
	}
	await coordinator.showErrorAlert(error: error)
	if case let ServerError.error(errorResponse: errorResponse) = error,
	errorResponse.message.contains("Cipher was not encrypted for the current user") {
	services.errorReporter.log(error: BitwardenError.generalError(
	type: "Save item failed",
	message: errorResponse.message,
	error: error,
	))
	}
	services.errorReporter.log(error: error)

[LRNcardozoWDF]

@claude this is an old version, I already updated this. Review the last changes.

[claude]

Claude finished @LRNcardozoWDF's task in 1m 32s —— View job

---

Re-Review of Latest Changes

• Gather context on latest changes
• Review updated code
• Post review findings

Commits reviewed since last review: 87c4115 ([PM-24195] Fix tests), c758640 ([PM-24195] Fix pr comment)

Previous Findings Status

Finding	Severity	Status
Generic error logging removed in AddEditItemProcessor	IMPORTANT	Addressed — else branch restores generic logging for non-targeted errors
Diagnostic code shares same do block as refresh logic	SUGGESTED	Addressed — Post-refresh diagnostic wrapped in its own do/catch

Changes Since Last Review

1. AddEditItemProcessor.swift — Added else branch at line 708 that calls services.errorReporter.log(error: error) for errors not matching the targeted "Cipher was not encrypted for the current user" message. This restores generic error logging for all other errors.

2. AccountTokenProvider.swift — The race condition diagnostic (getActiveAccountId comparison) is now isolated in its own do/catch block (lines 111–122), preventing diagnostic failures from affecting the success path. Error type renamed from TokenRefreshRaceConditionError to AccountTokenProviderError.

3. AccountTokenProviderTests.swift — Added @MainActor, fixed async tearDown, set stateService.activeAccount fixture in setUp, and added test_refreshToken_logsError_whenGetUserIdAfterThrows to verify the inner do/catch isolation works correctly (token is returned even when the diagnostic check throws).

4. AddEditItemProcessorTests.swift — Updated test_perform_savePressed_serverError_otherMessage_doesNotLog to expect 1 logged error (matching restored generic logging).

Verdict

Both previous findings have been properly addressed. No new issues found in the incremental changes. The code is clean, well-tested, and ready to merge.

Approved ✅

[claude]

🎨 SUGGESTED - Diagnostic code shares the same do block as the main refresh logic

If getActiveAccountId() on line 111 throws (e.g., the active account was removed during the refresh -- precisely the race condition being investigated), the error propagates to the catch block, which calls onRefreshTokenError and re-throws. This would make a successful token refresh appear as a failure to callers, since setTokens() on line 105 already completed.

Consider wrapping the post-refresh diagnostic section in its own do/catch to keep it isolated from the success path:

// After setTokens succeeds:
do {
    let userIdAfter = try await stateService.getActiveAccountId()
    if userIdBefore != userIdAfter {
        let error = TokenRefreshRaceConditionError(
            userIdBefore: userIdBefore,
            userIdAfter: userIdAfter,
        )
        errorReporter.log(error: error)
    }
} catch {
    errorReporter.log(error: error)
}

[fedemkr]

🤔 Is this the only endpoint that can throw this error? If not, can we extract to some other place more generic?
🎨 Instead of using if case you can use catch with the specific error and avoid the if else:

catch let ServerError.error(errorResponse):

[LRNcardozoWDF]

I didn't found other endpoints, I'll look into it again.

[fedemkr]

🤔 If this would happen then I think we shouldn't be setting the tokens in the wrong account. So you could just add the check before setTokens and add the userId: String? as parameter to setTokens so you can pass the user id you have in there instead of getting it again inside the function:

Suggested change

	let userIdAfter = try await stateService.getActiveAccountId()
	guard userIdBefore == userIdAfter else {
	let error = AccountTokenProviderError(
	userIdBefore: userIdBefore,
	userIdAfter: userIdAfter,
	)
	errorReporter.log(error: error)
	return
	}
	try await tokenService.setTokens(
	accessToken: response.accessToken,
	refreshToken: response.refreshToken,
	expirationDate: expirationDate,
	userId: userIdBefore // <-- maybe rename to expectedUserId instead of userIdBefore
	)

🤔 Moreover, there could potentially be a race condition between these two as well:

                let userIdBefore = try await stateService.getActiveAccountId()

                let refreshToken = try await tokenService.getRefreshToken()

So perhaps you could add userId: String? to getRefreshToken as well thus you can pass it, so the userId used throughout this refreshToken is as stable as possible.

[fedemkr]

🤔 I think you can remove the comment if you start passing the user ID to the token service functions. Also I think userIdBefore should be renamed to userIdToRefresh or something like that so it's easier to know what it refers to.

[fedemkr]

🎨 No need to recreate the server error, you already have it in the error variable.

[fedemkr]

👍 Looks good to me, I'd wait for @KatherineInCode or @matt-livefront's approval as well in case I've missed anything as this touches the account tokens.

[matt-livefront]

⛏️ Does it make sense to keep these next to getRefreshTokenResult and getRefreshTokenResult. That's the pattern we've used elsewhere and keeps them alphabetized.

[matt-livefront]

🤔 Instead of exposing this via TokenService, would it be better to pass ActiveAccountStateProvider (implemented by StateService) to AccountTokenProvider?

[matt-livefront]

⛏️ Can you alphabetize these with the other properties?

[matt-livefront]

⛏️ Can you alphabetize these methods?

[KatherineInCode]

🤔 Should these be alphabetized?