[PM-2572] Add new cipher encryption on attachments without key when moving cipher to an org #3238
Conversation
src/Core/Services/CipherService.cs
Outdated
| if(cipherView.Key == null) | ||
| { | ||
| foreach (var attachment in cipher.Attachments) | ||
| cipher = await EncryptAsync(cipherView); | ||
| var putCipherRequest = new CipherRequest(cipher); | ||
| var putCipherResponse = await _apiService.PutCipherAsync(cipherView.Id, putCipherRequest); | ||
| var cipherData = new CipherData(putCipherResponse, await _stateService.GetActiveUserIdAsync()); | ||
| await UpsertAsync(cipherData); | ||
| cipher = await GetAsync(cipherView.Id); | ||
| cipherView = await cipher.DecryptAsync(); | ||
| } |
There was a problem hiding this comment.
💡 Nice idea to update the cipher key here if it doesn't have one before updating the attachments.
🤔 I think that doing this you don't need the CipherKey in the Attachment neither the cipherKey added in the StringContent for the MultipartFormDataContent. Because I don't see a situation where the cipher doesn't have an item level encryption key and the attachment is migrated with it. Did you find one and that's why you left the CipherKey in the attachment?
🎨 Could you extract the common parts into a method (can be local function as you want)? So it's shared with lines 604...609
There was a problem hiding this comment.
Well, that was my interpretation of the steps in the task. I honestly thought we were just being extra careful.
src/Core/Services/CipherService.cs
Outdated
| if (cipher.Attachments != null) | ||
| Cipher cipher = null; | ||
| //If the cipher doesn't have a key, we update it | ||
| if(cipherView.Key == null) |
There was a problem hiding this comment.
🎨 I think we could improve this checking if we should use cipher key encryption by calling ShouldUseCipherKeyEncryptionAsync, so we save a sever call if that's not the case.
src/Core/Services/CipherService.cs
Outdated
| { | ||
| foreach (var attachment in cipher.Attachments) | ||
| cipher = await EncryptAsync(cipherView); | ||
| await UpdateAndUpsertAsync(async () => await _apiService.PutCipherAsync(cipherView.Id, new CipherRequest(cipher))); |
There was a problem hiding this comment.
⛏️ You can omit the async and await inside the lambda.
src/Core/Services/CipherService.cs
Outdated
| await UpsertAsync(data); | ||
| cipherView.OrganizationId = organizationId; | ||
| cipherView.CollectionIds = collectionIds; | ||
| cipher = await EncryptAsync(cipherView); |
There was a problem hiding this comment.
🎨 You can move encrypting into UpdateAndUpsertAsync as well.
There was a problem hiding this comment.
I didn't do that because I need the cipher to create two different requests, CipherRequest and CipherShareRequest. I could add in the api call but I think there's already too much happening in one line of code.
await UpdateAndUpsertAsync(async () => await _apiService.PutShareCipherAsync(cipherView.Id, new CipherShareRequest(await EncryptAsync(cipherView))), collectionIds);
There was a problem hiding this comment.
Oh I see, I think it's fine to pass the CipherView as a parameter to the func to construct such requests.
src/Core/Services/CipherService.cs
Outdated
| @@ -1355,6 +1360,13 @@ await _stateService.GetDisableAutoTotpCopyAsync() == true) | |||
| } | |||
| } | |||
|
|
|||
| private async Task UpdateAndUpsertAsync(Func<Task<CipherResponse>> func, HashSet<string> collectionIds = null) | |||
There was a problem hiding this comment.
⛏️ I'd rename the func parameter to callPutCipherApi or something that indicates the purpose of such func.
src/Core/Services/CipherService.cs
Outdated
| if (cipher.Attachments != null) | ||
| Cipher cipher = null; | ||
| //If the cipher doesn't have a key, we update it | ||
| if(await ShouldUseCipherKeyEncryptionAsync() && cipherView.Key == null) |
There was a problem hiding this comment.
⛏️ Invert the order so it doesn't waste time firing the task when Key != null
| if(await ShouldUseCipherKeyEncryptionAsync() && cipherView.Key == null) | |
| if (cipherView.Key == null && await ShouldUseCipherKeyEncryptionAsync()) |
Type of change
Objective
When an attachment file gets reencrypted and reuploaded, the new key is ignored by the server. This PR prepares the fix for the mobile side by adding the cipher.key in the attachment's metadata.
Code changes
Before you submit
dotnet format --verify-no-changes) (required)