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.

Sign up for GitHub

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
Open

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

Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4332e7a
PM-2572 Added cipherKey to Attachment and logic to use it
LRNcardozoWDF May 13, 2024
354aec0
Minor refactor
LRNcardozoWDF May 13, 2024
25e394e
Minor change
LRNcardozoWDF May 13, 2024
265c95b
PM-2572 Code refactor
LRNcardozoWDF May 28, 2024
890eff7
PM-2572 Fix pr comments
LRNcardozoWDF May 28, 2024
df9fcd1
Merge branch 'main' into vault/pm-2572
LRNcardozoWDF May 28, 2024
c40eb9d
PM-2572 Fix pr comments
LRNcardozoWDF May 28, 2024
290266c
PM-2572 Fix pr comment
LRNcardozoWDF May 29, 2024
485aca0
Merge branch 'main' into vault/pm-2572
LRNcardozoWDF Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 24 additions & 12 deletions src/Core/Services/CipherService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,28 +571,33 @@ public async Task SaveWithServerAsync(Cipher cipher)
await UpsertAsync(data);
}

public async Task ShareWithServerAsync(CipherView cipher, string organizationId, HashSet<string> collectionIds)
public async Task ShareWithServerAsync(CipherView cipherView, string organizationId, HashSet<string> collectionIds)
{
var attachmentTasks = new List<Task>();
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.

cipher = await GetAsync(cipherView.Id);
cipherView = await cipher.DecryptAsync();
}
if (cipherView.Attachments != null)
{
foreach (var attachment in cipherView.Attachments)
{
if (attachment.Key == null)
{
attachmentTasks.Add(ShareAttachmentWithServerAsync(attachment, cipher.Id, organizationId));
attachmentTasks.Add(ShareAttachmentWithServerAsync(attachment, cipherView.Id, organizationId));
}
}
}
await Task.WhenAll(attachmentTasks);
cipher.OrganizationId = organizationId;
cipher.CollectionIds = collectionIds;
var encCipher = await EncryptAsync(cipher);
var request = new CipherShareRequest(encCipher);
var response = await _apiService.PutShareCipherAsync(cipher.Id, request);
var userId = await _stateService.GetActiveUserIdAsync();
var data = new CipherData(response, userId, collectionIds);
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.

await UpdateAndUpsertAsync(async () => await _apiService.PutShareCipherAsync(cipherView.Id, new CipherShareRequest(cipher)), collectionIds);
}

public async Task<Cipher> SaveAttachmentRawWithServerAsync(Cipher cipher, CipherView cipherView, string filename, byte[] data)
Expand Down Expand Up @@ -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.

{
var response = await func();
var data = new CipherData(response, await _stateService.GetActiveUserIdAsync(), collectionIds);
await UpsertAsync(data);
}

private class CipherLocaleComparer : IComparer<CipherView>
{
private readonly II18nService _i18nService;
Expand Down