Fix EF UserRepository batched UpdateUserData transactional behavior#7567
Open
carladams1299-lab wants to merge 1 commit intobitwarden:mainfrom
Open
Conversation
The EF UserRepository.UpdateUserDataAsync and SetV2AccountCryptographicStateAsync opened a transaction on a DatabaseContext resolved from the outer scope, but each UpdateUserData delegate (SetMasterPassword, SetKeyConnectorUserKey, UpdateMasterPasswordUnlockData) opened its own scope and resolved its own DatabaseContext. Each SaveChangesAsync auto-committed on a different connection from the one that owned the outer transaction, so the outer CommitAsync committed an empty unit of work. There was no try/catch around the action loop, so a mid-batch throw left earlier delegates' writes durably committed - leaving a user with cryptographic state that could lock them out of their vault (e.g. a new master-password hash without the matching master-key-wrapped user key). Self-hosted EF deployments (Postgres / MySQL / SQLite, plus EF-mode MSSQL) were exposed; cloud (MSSQL/Dapper) was not. Fix: - Add an AsyncLocal<DatabaseContext?> on the EF UserRepository that flows the active batch DbContext into each delegate so all writes in a batch share the connection that owns the open transaction. The UpdateUserData delegate signature is preserved (no public-API change). - Add try/catch with explicit RollbackAsync() to UpdateUserDataAsync and SetV2AccountCryptographicStateAsync. - Fall back to a fresh scope when no batch is active, preserving the standalone-invocation path used by existing test helpers. Add an integration test that batches [SetMasterPassword, throwing-delegate], asserts the throw, and verifies the prior write was rolled back. Runs against every configured [DatabaseData] provider. Resolves bitwarden#7566
|
|
|
Thank you for your contribution! We've added this to our internal Community PR board for review. Details on our contribution process can be found here: https://contributing.bitwarden.com/contributing/pull-requests/community-pr-process. |
Author
|
Hi, @justindbaur , |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
Resolves #7566.
📔 Objective
Fixes a transactional-correctness bug on the EF code path of
UserRepository.UpdateUserDataAsyncandSetV2AccountCryptographicStateAsyncopened a transaction on aDatabaseContextresolved from oneIServiceScope, but eachUpdateUserDatadelegate (SetMasterPassword,SetKeyConnectorUserKey,UpdateMasterPasswordUnlockData) opened its own scope and resolved its ownDatabaseContext. EachSaveChangesAsynctherefore auto-committed on a different connection from the one that owned the outer transaction, and the absence of atry/catcharound the action loop meant a mid-batch throw left earlier delegates' writes durably committed. For master-password / KDF / Key Connector mutations this could leave a user with mismatched cryptographic state (e.g. a new master-password hash without the matching master-key-wrapped user key), locking them out of their vault until manual database reconciliation. Self-hosted EF deployments (Postgres / MySQL / SQLite, plus EF-mode MSSQL) were exposed; cloud (MSSQL/Dapper) was not — see issue #7566 for the full mechanism.The fix flows the active batch
DatabaseContextinto each delegate via anAsyncLocal<DatabaseContext?>on the EFUserRepository, so all writes in a batch share the connection that owns the open transaction. TheUpdateUserDatadelegate signature is preserved (no public-API change). Both methods now wrap their action loops intry/catchwith explicitRollbackAsync()on failure. When no batch is active, delegates fall back to a fresh scope, preserving the standalone-invocation path used by existing test helpers. The DapperUserRepositorywas already correct and is unchanged.Test plan
UpdateUserDataAsync_WhenSubsequentActionThrows_RollsBackPriorWritesbatches[SetMasterPassword, throwing-delegate], asserts the call throws, then re-reads the user and asserts the prior delegate's write was rolled back. Runs against every configured[DatabaseData]provider — proves the fix on EF backends and regression-protects Dapper.Core.Testfor the consumer commands (MasterPasswordServiceTests,TdeSetPasswordCommandTests,FinishSsoJitProvisionMasterPasswordCommandTests,SetKeyConnectorKeyCommandTests) continue to pass — confirming theIUserRepositorycontract relied on by production callers is unchanged.Out of scope
Microsoft.Data.SqlClient.SqlConnection?/SqlTransaction?from the publicUpdateUserDatadelegate inIUserRepository. That's an API-shape change discussed as a follow-up in the original issue.UpdateEncryptedDataForKeyRotationdelegate path inUpdateUserKeyAndEncryptedDataAsync/UpdateUserKeyAndEncryptedDataV2Async— separate delegate type with its own callers.📸 Screenshots
N/A — backend correctness fix.