From 1b3acec905fba6bbccd6828c0dc4ee21794efc32 Mon Sep 17 00:00:00 2001 From: Kyle Spearrin Date: Sun, 21 Feb 2016 00:15:17 -0500 Subject: [PATCH] Transactionally safe user password and email change updates. --- src/Core/Repositories/ICipherRepository.cs | 4 +- .../SqlServer/CipherRepository.cs | 30 ++++++++------ src/Core/Services/UserService.cs | 41 ++++++++----------- src/Sql/Sql.sqlproj | 1 + .../User_UpdateEmailPassword.sql | 16 ++++++++ 5 files changed, 53 insertions(+), 39 deletions(-) create mode 100644 src/Sql/dbo/Stored Procedures/User_UpdateEmailPassword.sql diff --git a/src/Core/Repositories/ICipherRepository.cs b/src/Core/Repositories/ICipherRepository.cs index cfc09b2a65..02db3f9190 100644 --- a/src/Core/Repositories/ICipherRepository.cs +++ b/src/Core/Repositories/ICipherRepository.cs @@ -1,12 +1,12 @@ using System.Collections.Generic; using System.Threading.Tasks; +using Bit.Core.Domains; namespace Bit.Core.Repositories { public interface ICipherRepository { - Task DirtyCiphersAsync(string userId); - Task UpdateDirtyCiphersAsync(IEnumerable ciphers); + Task UpdateUserEmailPasswordAndCiphersAsync(User user, IEnumerable ciphers); Task CreateAsync(IEnumerable ciphers); } } diff --git a/src/Core/Repositories/SqlServer/CipherRepository.cs b/src/Core/Repositories/SqlServer/CipherRepository.cs index ca1b346b6c..ff9e1f96da 100644 --- a/src/Core/Repositories/SqlServer/CipherRepository.cs +++ b/src/Core/Repositories/SqlServer/CipherRepository.cs @@ -16,12 +16,7 @@ namespace Bit.Core.Repositories.SqlServer : base(connectionString) { } - public Task DirtyCiphersAsync(string userId) - { - return Task.FromResult(0); - } - - public Task UpdateDirtyCiphersAsync(IEnumerable ciphers) + public Task UpdateUserEmailPasswordAndCiphersAsync(User user, IEnumerable ciphers) { var cleanedCiphers = ciphers.Where(c => c is Cipher); if(cleanedCiphers.Count() == 0) @@ -29,9 +24,6 @@ namespace Bit.Core.Repositories.SqlServer return Task.FromResult(0); } - // Get the id of the expected user - var userId = ((Cipher)ciphers.First()).UserId; - using(var connection = new SqlConnection(ConnectionString)) { connection.Open(); @@ -40,7 +32,19 @@ namespace Bit.Core.Repositories.SqlServer { try { - // 1. Create temp tables to bulk copy into. + // 1. Update user. + + using(var cmd = new SqlCommand("[dbo].[User_UpdateEmailPassword]", connection, transaction)) + { + cmd.CommandType = CommandType.StoredProcedure; + cmd.Parameters.Add("@Id", SqlDbType.UniqueIdentifier).Value = new Guid(user.Id); + cmd.Parameters.Add("@Email", SqlDbType.NVarChar).Value = user.Email; + cmd.Parameters.Add("@MasterPassword", SqlDbType.NVarChar).Value = user.MasterPassword; + cmd.Parameters.Add("@SecurityStamp", SqlDbType.NVarChar).Value = user.SecurityStamp; + cmd.ExecuteNonQuery(); + } + + // 2. Create temp tables to bulk copy into. var sqlCreateTemp = @" SELECT TOP 0 * @@ -56,7 +60,7 @@ namespace Bit.Core.Repositories.SqlServer cmd.ExecuteNonQuery(); } - // 2. Bulk bopy into temp tables. + // 3. Bulk bopy into temp tables. using(var bulkCopy = new SqlBulkCopy(connection, SqlBulkCopyOptions.KeepIdentity, transaction)) { @@ -82,7 +86,7 @@ namespace Bit.Core.Repositories.SqlServer bulkCopy.WriteToServer(dataTable); } - // 3. Insert into real tables from temp tables and clean up. + // 4. Insert into real tables from temp tables and clean up. var sqlUpdate = @" UPDATE @@ -123,7 +127,7 @@ namespace Bit.Core.Repositories.SqlServer using(var cmd = new SqlCommand(sqlUpdate, connection, transaction)) { - cmd.Parameters.Add("@UserId", SqlDbType.UniqueIdentifier).Value = new Guid(userId); + cmd.Parameters.Add("@UserId", SqlDbType.UniqueIdentifier).Value = new Guid(user.Id); cmd.ExecuteNonQuery(); } diff --git a/src/Core/Services/UserService.cs b/src/Core/Services/UserService.cs index 204ff96f44..11c6ef760c 100644 --- a/src/Core/Services/UserService.cs +++ b/src/Core/Services/UserService.cs @@ -69,7 +69,7 @@ namespace Bit.Core.Services { throw new ApplicationException("Use register method to create a new user."); } - + await _userRepository.ReplaceAsync(user); } @@ -128,50 +128,43 @@ namespace Bit.Core.Services } var existingUser = await _userRepository.GetByEmailAsync(newEmail); - if(existingUser != null) + if(existingUser != null && existingUser.Id != user.Id) { return IdentityResult.Failed(_identityErrorDescriber.DuplicateEmail(newEmail)); } + var result = await UpdatePasswordHash(user, newMasterPassword); + if(!result.Succeeded) + { + return result; + } + user.Email = newEmail; - user.MasterPassword = _passwordHasher.HashPassword(user, newMasterPassword); - user.SecurityStamp = Guid.NewGuid().ToString(); - - await _cipherRepository.DirtyCiphersAsync(user.Id); - await _userRepository.ReplaceAsync(user); - await _cipherRepository.UpdateDirtyCiphersAsync(ciphers); - - // TODO: what if something fails? rollback? - + await _cipherRepository.UpdateUserEmailPasswordAndCiphersAsync(user, ciphers); return IdentityResult.Success; } - public override Task ChangePasswordAsync(User user, string currentMasterPasswordHash, string newMasterPasswordHash) + public override Task ChangePasswordAsync(User user, string masterPassword, string newMasterPassword) { throw new NotImplementedException(); } - public async Task ChangePasswordAsync(User user, string currentMasterPasswordHash, string newMasterPasswordHash, IEnumerable ciphers) + public async Task ChangePasswordAsync(User user, string masterPassword, string newMasterPassword, IEnumerable ciphers) { if(user == null) { throw new ArgumentNullException(nameof(user)); } - if(await base.CheckPasswordAsync(user, currentMasterPasswordHash)) + if(await base.CheckPasswordAsync(user, masterPassword)) { - var result = await UpdatePasswordHash(user, newMasterPasswordHash); + var result = await UpdatePasswordHash(user, newMasterPassword); if(!result.Succeeded) { return result; } - - await _cipherRepository.DirtyCiphersAsync(user.Id); - await _userRepository.ReplaceAsync(user); - await _cipherRepository.UpdateDirtyCiphersAsync(ciphers); - - // TODO: what if something fails? rollback? - + + await _cipherRepository.UpdateUserEmailPasswordAndCiphersAsync(user, ciphers); return IdentityResult.Success; } @@ -179,14 +172,14 @@ namespace Bit.Core.Services return IdentityResult.Failed(_identityErrorDescriber.PasswordMismatch()); } - public async Task RefreshSecurityStampAsync(User user, string masterPasswordHash) + public async Task RefreshSecurityStampAsync(User user, string masterPassword) { if(user == null) { throw new ArgumentNullException(nameof(user)); } - if(await base.CheckPasswordAsync(user, masterPasswordHash)) + if(await base.CheckPasswordAsync(user, masterPassword)) { var result = await base.UpdateSecurityStampAsync(user); if(!result.Succeeded) diff --git a/src/Sql/Sql.sqlproj b/src/Sql/Sql.sqlproj index 4fee35a9c5..da563843b1 100644 --- a/src/Sql/Sql.sqlproj +++ b/src/Sql/Sql.sqlproj @@ -87,5 +87,6 @@ + \ No newline at end of file diff --git a/src/Sql/dbo/Stored Procedures/User_UpdateEmailPassword.sql b/src/Sql/dbo/Stored Procedures/User_UpdateEmailPassword.sql new file mode 100644 index 0000000000..42e6712245 --- /dev/null +++ b/src/Sql/dbo/Stored Procedures/User_UpdateEmailPassword.sql @@ -0,0 +1,16 @@ +CREATE PROCEDURE [dbo].[User_UpdateEmailPassword] + @Id UNIQUEIDENTIFIER, + @Email NVARCHAR(50), + @MasterPassword NVARCHAR(300), + @SecurityStamp NVARCHAR(50) +AS +BEGIN + UPDATE + [dbo].[User] + SET + [Email] = @Email, + [MasterPassword] = @MasterPassword, + [SecurityStamp] = @SecurityStamp + WHERE + [Id] = @Id +END