[PM-2572] Add new cipher encryption on attachments without key when moving cipher to an org by LRNcardozoWDF · Pull Request #3238 · bitwarden/android

LRNcardozoWDF · 2024-05-13T16:51:48Z

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

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

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

* Update AppResources.af.resx * Update AnonAddy references * Reverted AnonAddy to AddyIo refactor, keeping text and url changes --------- Co-authored-by: Andre Rosado <arosado@bitwarden.com>

* [PM-3606] Fix 2FA for autofill * [PM-3606] Fix autofill when user doesn't have a login method available. * [PM-3606] PR fixes * [PM-3606] Add logout logic to other extension projects * [PM-3606] Move code to base class. * Transform into property instead of field Co-authored-by: Federico Maccaroni <fedemkr@gmail.com> * Remove double ";" Co-authored-by: Federico Maccaroni <fedemkr@gmail.com> * [PM-3606] Fix iOS extension by changing base class of LockPasswordViewController --------- Co-authored-by: Federico Maccaroni <fedemkr@gmail.com>

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

…cally log out TDE users (#2747) * [PM-3393] Log user out on biometric exceed attempts * [PM-3393] Move duplicated code to AppHelpers * [PM-3393] Update copy on new pop up * [PM-3393] Moved VaultTimeoutService to LazyResolve. * [PM-3382] Change IVaultTimeoutService for messaging * [PM-3393] Use default values.

…roduction environments (#2773)

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

* [PM-3726] prevent legacy user login * [PM-3726] prevent unlock or auto key migration if legacy user * [PM-3726] add legacy checks to lock page and refactor * [PM-3726] rethrow exception from pin * formatting * [PM-3726] add changes to LockViewController, consolidate logout calls * formatting * [PM-3726] pr feedback * generate resx * formatting

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

* PM-3811 Unified passkeys view and moved both inside Login as an array of FIdo2Key * PM-3811 Passkeys unification => updated cipher details view an helpers * PM-3811 Updated passkeys creation date time format

…2772)

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

…oard for autofill (#2764) * [PM-3446] Check if user has mp and allow autofill to use items with mp re-prompt

* [PM-2658] Settings Reorganization Init (#2697) * PM-2658 Started settings reorganization (settings main + vault + about) * PM-2658 Added settings controls based on templates and implemented OtherSettingsPage * PM-2658 Fix format * [PM-3512] Settings Appearance (#2703) * PM-3512 Implemented new Appearance Settings * PM-3512 Fix format * [PM-3510] Implement Account Security Settings view (#2714) * PM-3510 Implemented Security settings view * PM-3510 Fix format * PM-3510 Added empty placeholder to pending login requests and also improved a11y on security settings view. * PM-3511 Implemented autofill settings view (#2735) * [PM-3695] Add Connect to Watch to Other settings (#2736) * PM-3511 Implemented autofill settings view * PM-3695 Add Connect to watch setting to other settings view * [PM-3693] Clear old Settings approach (#2737) * PM-3511 Implemented autofill settings view * PM-3693 Remove old Settings approach * PM-3845 Fix default dark theme description verbiage (#2759) * PM-3839 Fix allow screen capture and submit crash logs to init their state when the page appears (#2760) * PM-3834 Fix dialogs strings on settings (#2758) * [PM-3834] Fix import items link (#2782) * PM-3834 Fix import items link * PM-3834 Fix import items link, removed old link. * [PM-4092] Fix vault timeout policies on new Settings (#2796) * PM-4092 Fix vault timeout policy on settings for disabling controls and reset timeout when surpassing maximum * PM-4092 Removed testing hardcoding of policy data

* PM-115 Added new cipher key and encryption/decryption mechanisms on cipher * PM-115 fix format * PM-115 removed ForceKeyRotation from new cipher encryption model given that another approach will be taken * [PM-1690] Added minimum server version restriction to cipher key encryption (#2463) * PM-1690 added minimum server version restriction to cipher key encryption and also change the force key rotation flag * PM-1690 Updated min server version for new cipher encryption key and fixed configService registration * PM-1690 removed forcekeyrotation * PM-115 Temporarily Changed cipher key new encryption config to help testing (this change should be reseted eventually) * PM-2456 Fix attachment encryption on new cipher item encryption model (#2556) * PM-2531 Fix new cipher encryption on adding attachments on ciphers with no item level key (#2559) * PM-115 Changed temporarily cipher key encryption min server version to 2023.6.0 to test * PM-115 Reseted cipher key encryption minimum server version to 2023.5.0 and disable new cipher key on local cipher creation * Added Key value to the cipher export model (#2628) * Update Constants.cs Updated minimum encryption server version to 2023.9.0 so QA can test its behavior * PM-115 Fix file format * PM-115 Changed new encryption off and minimum new encryption server version to 2023.8.0 for testing purposes * PM-115 Changed CIpher key encryption minimum server version to 2023.9.0 * PM-3737 Remove suffix on client version sent to server (#2779) * PM-115 QA testing server min version and enable new cipher key encryption * PM-115 Disable new cipher encryption creation and change minimum server encryption version to 2023.9.1 --------- Co-authored-by: aj-rosado <109146700+aj-rosado@users.noreply.github.com>

* [PM-868] Check for previous page before loading vault. Remove exception throw. * [PM-868] Continue to throw exceptions

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

#2810)

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

* [PM-3741] [PM-3750] Improvements to local storage handling * Update src/Android/MainActivity.cs Co-authored-by: Federico Maccaroni <fedemkr@gmail.com> --------- Co-authored-by: Federico Maccaroni <fedemkr@gmail.com>

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

fedemkr · 2024-05-15T14:35:40Z

src/Core/Services/CipherService.cs

+            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();
+            }


💡 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

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

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

…mp workflow (#3264)

fedemkr · 2024-05-28T14:10:46Z

src/Core/Services/CipherService.cs

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


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

fedemkr · 2024-05-28T14:11:05Z

src/Core/Services/CipherService.cs

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


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

fedemkr · 2024-05-28T15:33:15Z

src/Core/Services/CipherService.cs

-            await UpsertAsync(data);
+            cipherView.OrganizationId = organizationId;
+            cipherView.CollectionIds = collectionIds;
+            cipher = await EncryptAsync(cipherView);


🎨 You can move encrypting into UpdateAndUpsertAsync as well.

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

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

fedemkr · 2024-05-28T15:34:27Z

src/Core/Services/CipherService.cs

            }
        }

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


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

fedemkr

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

fedemkr · 2024-05-29T12:23:27Z

src/Core/Services/CipherService.cs

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


⛏️ 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())

fedemkr

LGTM 🎉

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

…h-actions/download-artifacts` (#3278)

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

andrebispo5 and others added 30 commits September 7, 2023 16:30

[PM-3593] Fix enable biometric on autofill when there is not MP (#2717)

9026dd1

[PM-3551] Expired SSO token cached (#2718)

5961a00

Autosync the updated translations (#2751)

b0f1dd0

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

[PM-3556] Change anonaddy to addy io (#2711)

382eee2

* Update AppResources.af.resx * Update AnonAddy references * Reverted AnonAddy to AddyIo refactor, keeping text and url changes --------- Co-authored-by: Andre Rosado <arosado@bitwarden.com>

Autosync the updated translations (#2771)

efd1671

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

Make dept-development-mobile default code owner (#2780)

b932824

[PM-3522] Keep variable value after logout. (#2761)

11922c6

[PM-3086] Account switcher endpoint use domain string for Bitwarden p…

43bf0fb

…roduction environments (#2773)

Bumped version to 2023.9.0 (#2783)

8b9658d

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

Bumped version to 2023.9.1 (#2784)

b8cef16

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

Autosync the updated translations (#2785)

597f629

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

Adding missing IDs (#2786)

6ef6cf5

[PM-3811] Passkeys unification (#2774)

a4a0d31

* PM-3811 Unified passkeys view and moved both inside Login as an array of FIdo2Key * PM-3811 Passkeys unification => updated cipher details view an helpers * PM-3811 Updated passkeys creation date time format

[PM-3893] Make PreLogin and Register endpoint use identity endpoints (#…

b25c8b0

…2772)

Bumped version to 2023.9.2 (#2797)

828043e

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

[PM-3446] User without MP, item with MP does not show on Android keyb…

218a30b

…oard for autofill (#2764) * [PM-3446] Check if user has mp and allow autofill to use items with mp re-prompt

PM-4138 Remove hyphen from clip-board => clipboard on resources. (#2804)

bbef0f8

[PM-2915] Fix config service null exception error bug (#2778)

685e0f4

Added GetOrDeriveMasterKey to UserVerificationService (#2808)

f2be840

PM-2658 Removed deprecated strings resources. (#2812)

71731bb

[PM-868] Re-open app to item could crash the app (#2813)

b23f295

* [PM-868] Check for previous page before loading vault. Remove exception throw. * [PM-868] Continue to throw exceptions

Autosync the updated translations (#2805)

757e5ea

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

Added CultureInfo in char.ToLower at GetServiceRegistrationName method (

a67f50b

#2810)

Bumped version to 2023.9.3 (#2820)

5a0c211

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

[PM-3741] [PM-3750] Improvements to local storage handling (#2795)

bb2f1f0

* [PM-3741] [PM-3750] Improvements to local storage handling * Update src/Android/MainActivity.cs Co-authored-by: Federico Maccaroni <fedemkr@gmail.com> --------- Co-authored-by: Federico Maccaroni <fedemkr@gmail.com>

vgrassia and others added 5 commits May 17, 2024 00:15

Update F-Droid release process (#3249)

a333e72

Autosync the updated translations (#3250)

e16074a

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

Autosync the updated translations (#3255)

e86a01a

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

[deps]: Update gh minor (#3252)

0af78d0

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

BRE-40 ADD: step to report upcoming release version to Slack (#3247)

c3bd4b8

fedemkr reviewed May 23, 2024

View reviewed changes

github-actions bot and others added 3 commits May 24, 2024 12:09

Autosync the updated translations (#3257)

d458f17

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

BRE-87 ADD: Slack notification enable feature and check in version-bu…

f165135

…mp workflow (#3264)

PM-2572 Code refactor

265c95b

LRNcardozoWDF requested a review from fedemkr May 28, 2024 10:18

fedemkr reviewed May 28, 2024

View reviewed changes

LRNcardozoWDF added 3 commits May 28, 2024 18:34

PM-2572 Fix pr comments

890eff7

Merge branch 'main' into vault/pm-2572

df9fcd1

PM-2572 Fix pr comments

c40eb9d

LRNcardozoWDF requested a review from fedemkr May 28, 2024 20:35

fedemkr reviewed May 29, 2024

View reviewed changes

PM-2572 Fix pr comment

290266c

fedemkr approved these changes May 29, 2024

View reviewed changes

fedemkr added the needs-qa label May 29, 2024

github-actions bot and others added 8 commits May 29, 2024 20:07

Autosync the updated translations (#3267)

fc208b0

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

Autosync Crowdin Translations (#3269)

02f5936

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

Bump version to 2024.6.0 (#3274)

67b76a7

[deps]: Update crowdin/github-action action to v2 (#3271)

bad5673

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

[BRE-94] Replace dawidd6/action-download-artifact with `bitwarden/g…

14d2660

…h-actions/download-artifacts` (#3278)

[PM-8666] Fix Password Autofill from Quick Type on iOS < 17 (#3282)

8120515

Autosync Crowdin Translations (#3283)

2b78859

Co-authored-by: bitwarden-devops-bot <106330231+bitwarden-devops-bot@users.noreply.github.com>

Merge branch 'main' into vault/pm-2572

485aca0

vvolkgang closed this Jun 20, 2024

vvolkgang force-pushed the main branch from ad6ffad to 959a60c Compare June 20, 2024 15:45

vvolkgang deleted the vault/pm-2572 branch June 20, 2024 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

[PM-2572] Add new cipher encryption on attachments without key when moving cipher to an org#3238
LRNcardozoWDF wants to merge 3957 commits intomainfrom
vault/pm-2572

LRNcardozoWDF commented May 13, 2024

Uh oh!

fedemkr May 15, 2024

Uh oh!

LRNcardozoWDF May 28, 2024

Uh oh!

fedemkr May 28, 2024

Uh oh!

fedemkr May 28, 2024

Uh oh!

fedemkr May 28, 2024

Uh oh!

LRNcardozoWDF May 28, 2024

Uh oh!

fedemkr May 28, 2024

Uh oh!

fedemkr May 28, 2024

Uh oh!

fedemkr left a comment

Uh oh!

fedemkr May 29, 2024

Uh oh!

fedemkr left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

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

Conversation

LRNcardozoWDF commented May 13, 2024

Type of change

Objective

Code changes

Before you submit

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fedemkr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fedemkr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants