-
Notifications
You must be signed in to change notification settings - Fork 99
[PM-24195] fix: Pass userId to refreshToken and setTokens #2374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 7 commits
8b1762b
08eb750
e927d7a
0c069d7
4b4893d
0b57b8a
f0b166f
2ef74b9
0c24943
87c4115
2db926d
c758640
0c40b7e
c29a11a
6ea36da
1f2667a
a62f0ed
764ca8f
0e254f8
766613a
d667f2c
4ac7416
a7d3bcf
70f5629
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -22,12 +22,18 @@ actor DefaultAccountTokenProvider: AccountTokenProvider { | |||||||||||||||||||||||||||||||||||||
| /// The delegate to use for specific operations on the token provider. | ||||||||||||||||||||||||||||||||||||||
| private weak var accountTokenProviderDelegate: AccountTokenProviderDelegate? | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| /// The service used to report non-fatal errors. | ||||||||||||||||||||||||||||||||||||||
| private let errorReporter: ErrorReporter | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| /// The `HTTPService` used to make the API call to refresh the access token. | ||||||||||||||||||||||||||||||||||||||
| private let httpService: HTTPService | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| /// The task associated with refreshing the token, if one is in progress. | ||||||||||||||||||||||||||||||||||||||
| private(set) var refreshTask: Task<String, Error>? | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| /// The service used to manage account state. | ||||||||||||||||||||||||||||||||||||||
| private let stateService: StateService | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| /// The service used to get the present time. | ||||||||||||||||||||||||||||||||||||||
| private let timeProvider: TimeProvider | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
|
@@ -42,15 +48,21 @@ actor DefaultAccountTokenProvider: AccountTokenProvider { | |||||||||||||||||||||||||||||||||||||
| /// - httpService: The service used to make the API call to refresh the access token. | ||||||||||||||||||||||||||||||||||||||
| /// - timeProvider: The service used to get the present time. | ||||||||||||||||||||||||||||||||||||||
| /// - tokenService: The service used to get the current tokens from. | ||||||||||||||||||||||||||||||||||||||
| /// - errorReporter: The service used to report non-fatal errors. | ||||||||||||||||||||||||||||||||||||||
| /// - stateService: The service used to manage account state. | ||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||
| init( | ||||||||||||||||||||||||||||||||||||||
| httpService: HTTPService, | ||||||||||||||||||||||||||||||||||||||
| timeProvider: TimeProvider = CurrentTime(), | ||||||||||||||||||||||||||||||||||||||
| tokenService: TokenService, | ||||||||||||||||||||||||||||||||||||||
| errorReporter: ErrorReporter, | ||||||||||||||||||||||||||||||||||||||
| stateService: StateService, | ||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||
| self.httpService = httpService | ||||||||||||||||||||||||||||||||||||||
| self.timeProvider = timeProvider | ||||||||||||||||||||||||||||||||||||||
| self.tokenService = tokenService | ||||||||||||||||||||||||||||||||||||||
| self.errorReporter = errorReporter | ||||||||||||||||||||||||||||||||||||||
| self.stateService = stateService | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| // MARK: Methods | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -81,6 +93,9 @@ actor DefaultAccountTokenProvider: AccountTokenProvider { | |||||||||||||||||||||||||||||||||||||
| defer { self.refreshTask = nil } | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| do { | ||||||||||||||||||||||||||||||||||||||
| // TODO: PM-33074 Remove logs after confirmation that the error doesn't happen anymore. | ||||||||||||||||||||||||||||||||||||||
| let userIdBefore = try await stateService.getActiveAccountId() | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| let refreshToken = try await tokenService.getRefreshToken() | ||||||||||||||||||||||||||||||||||||||
| let response = try await httpService.send( | ||||||||||||||||||||||||||||||||||||||
| IdentityTokenRefreshRequest(refreshToken: refreshToken), | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -93,6 +108,16 @@ actor DefaultAccountTokenProvider: AccountTokenProvider { | |||||||||||||||||||||||||||||||||||||
| expirationDate: expirationDate, | ||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| let userIdAfter = try await stateService.getActiveAccountId() | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| if userIdBefore != userIdAfter { | ||||||||||||||||||||||||||||||||||||||
| let error = TokenRefreshRaceConditionError( | ||||||||||||||||||||||||||||||||||||||
| userIdBefore: userIdBefore, | ||||||||||||||||||||||||||||||||||||||
| userIdAfter: userIdAfter, | ||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||
| errorReporter.log(error: error) | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
| 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.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| import Foundation | ||
|
|
||
| // MARK: - TokenRefreshRaceConditionError | ||
|
|
||
| /// Error logged when the active account changes during a token refresh operation. | ||
| /// | ||
| struct TokenRefreshRaceConditionError: Error, CustomStringConvertible { | ||
| // MARK: Properties | ||
|
|
||
| /// The active user ID before the token refresh operation. | ||
| let userIdBefore: String | ||
|
|
||
| /// The active user ID after the token refresh operation. | ||
| let userIdAfter: String | ||
|
|
||
| // MARK: CustomStringConvertible | ||
|
|
||
| var description: String { | ||
| """ | ||
| Token refresh race condition detected: Active account changed from '\(userIdBefore)' to '\(userIdAfter)' \ | ||
| during token refresh operation. Tokens may have been stored under wrong account. | ||
| """ | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | |||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -698,7 +698,14 @@ final class AddEditItemProcessor: StateProcessor<// swiftlint:disable:this type_ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return | |||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch { | |||||||||||||||||||||||||||||||||||||||||||||||||||
| 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) | |
| 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@claude this is an old version, I already updated this. Review the last changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
-
AddEditItemProcessor.swift β Added
elsebranch at line 708 that callsservices.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. -
AccountTokenProvider.swift β The race condition diagnostic (
getActiveAccountIdcomparison) is now isolated in its owndo/catchblock (lines 111β122), preventing diagnostic failures from affecting the success path. Error type renamed fromTokenRefreshRaceConditionErrortoAccountTokenProviderError. -
AccountTokenProviderTests.swift β Added
@MainActor, fixed asynctearDown, setstateService.activeAccountfixture insetUp, and addedtest_refreshToken_logsError_whenGetUserIdAfterThrowsto verify the innerdo/catchisolation works correctly (token is returned even when the diagnostic check throws). -
AddEditItemProcessorTests.swift β Updated
test_perform_savePressed_serverError_otherMessage_doesNotLogto 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 β
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
π€ Should these be alphabetized?