Skip to content
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

[PM-2572] Add new cipher encryption on attachments without key when moving cipher to an org #3238

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

LRNcardozoWDF
Copy link
Member

Type of change

  • Bug fix
  • New feature development
  • Tech debt (refactoring, code cleanup, dependency upgrades, etc)
  • Build/deploy pipeline (DevOps)
  • Other

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

  • Attachment.cs AttachmentView.cs: Added property CipherKey to store the cipher.Key
  • src/Core/Models/Domain/Cipher.cs: Added logic to use attachment.CipherKey for decryption if necessary
  • src/Core/Services/CipherService.cs: Update the cipher to use cipher.Key and save that value in the attachment object and metadata.

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

@LRNcardozoWDF LRNcardozoWDF marked this pull request as ready for review May 13, 2024 17:04
Comment on lines 579 to 588
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();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that was my interpretation of the steps in the task. I honestly thought we were just being extra careful.

if (cipher.Attachments != null)
Cipher cipher = null;
//If the cipher doesn't have a key, we update it
if(cipherView.Key == null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 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.

{
foreach (var attachment in cipher.Attachments)
cipher = await EncryptAsync(cipherView);
await UpdateAndUpsertAsync(async () => await _apiService.PutCipherAsync(cipherView.Id, new CipherRequest(cipher)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏️ You can omit the async and await inside the lambda.

await UpsertAsync(data);
cipherView.OrganizationId = organizationId;
cipherView.CollectionIds = collectionIds;
cipher = await EncryptAsync(cipherView);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎨 You can move encrypting into UpdateAndUpsertAsync as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, I think it's fine to pass the CipherView as a parameter to the func to construct such requests.

@@ -1355,6 +1360,13 @@ await _stateService.GetDisableAutoTotpCopyAsync() == true)
}
}

private async Task UpdateAndUpsertAsync(Func<Task<CipherResponse>> func, HashSet<string> collectionIds = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏️ I'd rename the func parameter to callPutCipherApi or something that indicates the purpose of such func.

Copy link
Member

@fedemkr fedemkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great just one small thing and it's done 😄 🎉

if (cipher.Attachments != null)
Cipher cipher = null;
//If the cipher doesn't have a key, we update it
if(await ShouldUseCipherKeyEncryptionAsync() && cipherView.Key == null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏️ Invert the order so it doesn't waste time firing the task when Key != null

Suggested change
if(await ShouldUseCipherKeyEncryptionAsync() && cipherView.Key == null)
if (cipherView.Key == null && await ShouldUseCipherKeyEncryptionAsync())

Copy link
Member

@fedemkr fedemkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants