PM-11697: Fix potential crash when creating a new account #901
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #901 +/- ##
==========================================
+ Coverage 88.77% 88.80% +0.02%
==========================================
Files 617 617
Lines 30898 30932 +34
==========================================
+ Hits 27430 27469 +39
+ Misses 3468 3463 -5 ☔ View full report in Codecov by Sentry. |
|
No New Or Fixed Issues Found |
| let userId = try await stateService.getAccountIdOrActiveId(userId: userId) | ||
|
|
||
| // If the user has a client, return it. | ||
| guard let client = userClientArray[userId] else { | ||
| // If not, create one, map it to the user, then return it. | ||
| let newClient = createAndMapClient(for: userId) | ||
| return newClient | ||
| } | ||
| return client |
There was a problem hiding this comment.
🤔 Shouldn't we maintain the catch StateServiceError.noAccounts, StateServiceError.noActiveAccount logic in case this happens? Perhaps because of passing wrongly the isPreAuth flag.
There was a problem hiding this comment.
My thinking on removing it was that it would help us catch any potential uses where isPreAuth flag isn't set, but should be. Since if there's an active account and isPreAuth isn't set, it will just silently use the account's client.
But it would be safer to keep this, so I'll add it back and maybe see about logging an error.
| func passwordStrength(email: String, password: String) async throws -> UInt8 { | ||
| try await clientService.auth().passwordStrength(password: password, email: email, additionalInputs: []) | ||
| try await clientService.auth(isPreAuth: true) | ||
| .passwordStrength(password: password, email: email, additionalInputs: []) | ||
| } |
There was a problem hiding this comment.
🤔 Is there a potential drawback if someone in the future uses this passwordStrength method while the vault is unlocked? I mean someone could just see the protocol definition and miss this implementation detail. Maybe just a comment on the protocol definition would be enough, if needed.
There was a problem hiding this comment.
Maybe! I could expose it as a parameter here? I was thinking that passwordStrength probably isn't specific to the user, so maybe it wouldn't matter, but that could change.
|
Really nice catch, great job!!! 🎉 🎉 |
🎟️ Tracking
PM-11697
📔 Objective
When creating a new account on a device that has at least one existing account, it's possible for the app to crash when checking the strength of the new account's password.
The root cause is that
DefaultClientServicemaintains a cache of the SDK client as a dictionary, mapping a user ID to their SDK. It's possible for this dictionary to be modified from multiple threads. Making the service an actor fixes the concurrency issue.I also noticed another issue here. When adding a new account, a user ID doesn't exist for the account yet, so we have some logic that returns a new SDK for these calls, and this SDK doesn't end up being cached. If there's an existing account on the device when adding a new account, that existing account is still considered the active user, so we end up using the existing account's SDK for some operations for the new account. To fix this, I added a
isPreAuthproperty that can be used when getting a SDK client so we always return a new SDK instead of potentially using an SDK for an existing account.⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes