[ADR-16] Refactor Folders #3732

Hinton · 2022-10-10T14:03:18Z

Type of change

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

Objective

Refactors the Folder domain to follow the new method described in ADR-16. Please read the ADR for some background and the motivation for this move.

This PR focuses on introducing the core services needed for the new data model, and migrates the Folder domain.

The way this is implemented is by adding a generic method decryptDomain to CryptoService. Which currently fetches the relevant encryption key and calls decryptDomain on EncryptionService. Which in turns calls the static method FolderView.decrypt(encryptionService, key, domain). Which performs the actual mapping from domain to view.

Fetching of the encryption key is implemented for organizations and personal owned items. Personal items gets a null value assigned while organization items provides the organization ID as the identifier. In the future we can support more keys by adding more lookup locations.

Encryption works in a similar fashion using the encryptView which fetches the key and proxies the request to EncryptionService which calls decrypt on the view.

Why not just put the decrypt on FolderService? The FolderService already has a responsibility, and that is to maintain the state of Folders. We shouldn't give it more responsibilities, particularly not when the EncryptService is already responsible for encrypting and decrypting things.

Before you submit

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

eliykat

I've also been noodling away on this, please review #3738 as an alternative proposal and let me know what you think. We can line up a call to discuss in more detail if it's useful.

The biggest difference is that I don't want to have this pattern where we:

pass an object to a decrypt method
that method passes a service into a method on the object
there are several levels of nested decrypt calls, passing down the same parameters each time, which you have to trace to find the actual decryption call still nested in EncString.

To me that is the real source of the tech debt here, it feels like decryption happens from the inside out, it's difficult to understand and extend. ContainerService is just a symptom of that. I'd like to flatten it out to a method which acts on an object.

libs/common/src/models/view/folderView.ts

libs/common/src/services/crypto.service.ts

Hinton · 2022-10-11T08:25:18Z

One goal that hasn't been noted in this PR, is the potential use case for a domain model to have multiple views with different amount of data.

I kept the decrypt method for a few reasons,

to limit the scope of the refactor.
Support objects that decrypt differently attachments seems to have some custom logic, and there has been some discussion around potentially having an EncObject which contains a json blob instead of individual fields.

eliykat · 2022-10-12T00:45:28Z

Support objects that decrypt differently attachments seems to have some custom logic, and there has been some discussion around potentially having an EncObject which contains a json blob instead of individual fields.

Just a note on this topic, I'd like to avoid having lots of custom logic here or different ways of encrypting objects. I'd rather define a few ways of encrypting objects (e.g. individual EncStrings or an encrypted blob) and make people implement those, rather than give every object the ability to do something different.

I probably have to experiment with more complex objects though to make sure that we can impose this kind of uniformity across our domain objects.

One goal that hasn't been noted in this PR, is the potential use case for a domain model to have multiple views with different amount of data.

Nice idea!

MGibson1 · 2022-10-12T01:59:30Z

I don't particularly like that it's the View's responsibility to determine what data on the Domain is encrypted and what is not. To me, #3738 and this are complimentary -- #3732 describes the data flow and #3738 is a better interface for defining encrypted data/decryption. @eliykat, the big change that would be necessary in your PR is updating the decryptable interface to belong on the View instead of the Domain. This would enable @Hinton's idea of multiple views for a given domain. The issue is how do we expose to the encryptservice which fields to decrypt and which are not needed? Not sure on that one, yet...

The other issue I see here is this doesn't solve the central issue of how to grab the key to my liking. I think I'd like to see an interface on Domain that determines the appropriate key, given a 2d array of [Id, Key][].

Hinton · 2022-10-12T11:51:51Z

Having an interface on Domain to determine which key is used to encrypt things seems reasonable, it could be argued it's part of the domain model.

Something that came to mind are Attachments, Name is an EncString, but the key has custom logic, and the data is encrypted using the key. Not sure if we have more edge cases for how things are encrypted/decrypted.

libs/common/src/models/view/folderView.ts

libs/angular/src/components/folder-add-edit.component.ts

libs/common/src/services/crypto.service.ts

…crypt-test # Conflicts: # apps/cli/src/commands/create.command.ts # apps/cli/src/commands/edit.command.ts # libs/common/src/abstractions/crypto.service.ts # libs/common/src/abstractions/folder/folder.service.abstraction.ts # libs/common/src/importers/bitwardenJsonImporter.ts # libs/common/src/models/domain/folder.ts # libs/common/src/models/view/folder.view.ts # libs/common/src/services/crypto.service.ts # libs/common/src/services/folder/folder.service.ts

libs/common/src/models/domain/folder.ts

libs/common/src/services/crypto.service.ts

eliykat

Only one minor comment so far, unfortunately I didn't get to spend a lot of time on this today. Although no surprises immediately jump out at me, which is good. I'll continue Monday.

libs/common/src/models/view/folder.view.ts

libs/common/src/models/domain/enc-string.ts

libs/common/src/abstractions/crypto.service.ts

libs/common/src/models/domain/folder.ts

libs/common/src/models/view/encryptable.ts

libs/common/src/models/view/folder.view.ts

eliykat · 2022-11-10T01:52:59Z

I'm still a little unsure what the end state of CryptoService and EncryptService is. The duplicated interfaces between them are starting to get messy.

iirc, the last we discussed was potentially changing CryptoService to be KeyService, then have EncryptService depend on KeyService so that it can fetch its own keys (with a static key store for web workers). That brought up circular dependency issues that we did not fully resolve. (see my notes). While we don't need to do all of this in 1 PR, I'd like to know where we're going with it.

Hinton · 2022-11-10T09:53:19Z

@eliykat I understand and I didn't want to muddle this PR with any refactoring of CryptoService.

I agree with the move to KeyService and have EncryptService depend on KeyService. But I view that as something we'll get around to at a later stage. I would like to refactor CryptoService to observables and do this refactor prior which should make that change fairly simple.

patch.diff

This reverts commit e298cb1.

Hinton · 2022-12-02T15:59:05Z

I've removed the Cipher changes from this again, and changed the target to a new feature branch. My thinking is that we'll review this to ratify the general patterns, get QA to begin testing folders while I move the cipher portions to a new PR and we can review it prior to merging the feature branch.

This lets QA test both potions in isolation and we don't introduce the change until we've migrated Ciphers which prevents us from having 3 ways of doing encryption/decryption of domain models.

eliykat

Mostly minor comments given that the cipher changes have been taken out and I think we've discussed the folder stuff quite a bit already.

To be honest I'm struggling with the various type/interface names (Decryptable/Encryptable) etc, but I don't have any better suggestions for them. I'll chew on it though.

libs/common/src/services/crypto.service.ts

libs/common/src/interfaces/crypto.interface.ts

libs/common/src/services/crypto.service.ts

eliykat · 2022-12-05T04:04:53Z

libs/common/src/models/view/folder.view.ts

+    folder.id = this.id;
+    folder.revisionDate = this.revisionDate;
+
+    folder.name = this.name != null ? await encryptService.encrypt(this.name, key) : null;


I think we can also use NullableFactory here as well, right?

Not really, it's not a constructor. We could make a nullable factory that accepts an argument with a factory in it.

True. It's a similar pattern though (call a method if value isn't null), and this ternary condition will be repeated 100 times as we encrypt properties on objects under this new pattern.

libs/common/src/interfaces/crypto.interface.ts

eliykat · 2022-12-05T04:30:36Z

libs/common/src/interfaces/crypto.interface.ts

+import { EncryptService } from "../abstractions/encrypt.service";
+import { SymmetricCryptoKey } from "../models/domain/symmetric-crypto-key";
+
+export function nullableFactory<T extends new (...args: any) => any>(


nullableFactory seems broadly useful to me, I think it should stand outside of the crypto-specific code.

libs/common/src/interfaces/decryptable.interface.ts

Hinton · 2022-12-05T09:06:01Z

We can discuss it in the future PR, but I don't want to mix in any knowledge of views in the domain. Since domain is in the center and the ambition to support multiple views per domain.

eliykat

Looks good to me, but I'd like a final review from @MGibson1.

libs/common/src/models/domain/folder.ts

libs/common/src/models/view/folder.view.ts

apps/cli/src/commands/edit.command.ts

…crypt-test # Conflicts: # apps/web/src/app/settings/change-password.component.ts

…crypt-test

eliykat · 2023-06-13T23:27:50Z

Removing myself from reviewers on the understanding this will be done in the SDK instead.

Hinton added 2 commits October 8, 2022 22:35

Random test

53574c4

Showcase how we can fetch either the user or organization key

d93b6b9

Hinton requested review from eliykat and MGibson1 October 10, 2022 14:03

eliykat mentioned this pull request Oct 10, 2022

[TDL-11] Proposal - item decryption refactor (alternative proposal) #3738

Closed

eliykat reviewed Oct 11, 2022

View reviewed changes

libs/common/src/models/view/folderView.ts Outdated Show resolved Hide resolved

libs/common/src/services/crypto.service.ts Outdated Show resolved Hide resolved

eliykat reviewed Oct 12, 2022

View reviewed changes

libs/common/src/models/view/folderView.ts Outdated Show resolved Hide resolved

eliykat reviewed Oct 12, 2022

View reviewed changes

libs/angular/src/components/folder-add-edit.component.ts Outdated Show resolved Hide resolved

eliykat reviewed Oct 13, 2022

View reviewed changes

libs/common/src/services/crypto.service.ts Outdated Show resolved Hide resolved

Hinton added 5 commits October 14, 2022 11:12

WIP encrypt

2922aa4

Typescript madness

1ecc318

Update other clients

e2cb8fd

Refactor with discussed changes

9e53267

Hinton commented Oct 31, 2022

View reviewed changes

libs/common/src/models/domain/folder.ts Show resolved Hide resolved

Hinton added 2 commits October 31, 2022 11:03

Refactor

3e22060

Rename

0df5f41

Hinton commented Oct 31, 2022

View reviewed changes

libs/common/src/services/crypto.service.ts Outdated Show resolved Hide resolved

Hinton requested a review from eliykat October 31, 2022 10:06

Hinton added 2 commits October 31, 2022 11:18

Fix encrypt/decrypt tests

ceba8a9

Fix last test

d312a54

eliykat reviewed Nov 4, 2022

View reviewed changes

libs/common/src/models/view/folder.view.ts Outdated Show resolved Hide resolved

Remove constructor

cfa927a

eliykat reviewed Nov 10, 2022

View reviewed changes

Showcase multithreaded decryption

e298cb1

MGibson1 reviewed Dec 1, 2022

View reviewed changes

patch.diff Outdated Show resolved Hide resolved

Hinton added 2 commits December 1, 2022 19:59

Remove patch

30ddb42

Revert "Showcase multithreaded decryption"

0b58d3d

This reverts commit e298cb1.

Hinton changed the base branch from master to feature/decrypt-refactor December 2, 2022 15:53

Hinton added 2 commits December 2, 2022 17:01

Fix cli

4677176

Swap to nullableFactory

bb46b75

eliykat requested changes Dec 5, 2022

View reviewed changes

Resolve review feedback

3eec7ba

eliykat reviewed Dec 12, 2022

View reviewed changes

MGibson1 requested changes Dec 13, 2022

View reviewed changes

libs/common/src/models/domain/folder.ts Show resolved Hide resolved

libs/common/src/models/view/folder.view.ts Show resolved Hide resolved

apps/cli/src/commands/edit.command.ts Outdated Show resolved Hide resolved

Merge branch 'master' of github.com:bitwarden/clients into feature/de…

43850ad

…crypt-test # Conflicts: # apps/web/src/app/settings/change-password.component.ts

Hinton force-pushed the feature/decrypt-test branch from af7c425 to 43850ad Compare January 12, 2023 12:53

Hinton added 2 commits January 12, 2023 13:55

Resolve review feedback

84ff880

Fix test

ae4cc31

Hinton requested review from eliykat and MGibson1 January 12, 2023 16:13

Hinton added 2 commits January 12, 2023 17:27

Fix cli build error

d73f480

Fix browser

e69c8dc

Hinton mentioned this pull request Jan 14, 2023

Cipher Decrypt refactor #4473

Closed

Merge branch 'master' of github.com:bitwarden/clients into feature/de…

4fc30c6

…crypt-test

Hinton changed the base branch from feature/decrypt-refactor to master January 27, 2023 12:30

Hinton changed the title ~~Proposal - Decrypt refactor~~ [ADR-16] Refactor Folders Jan 27, 2023

Move specs to live next to source

ca685a6

Hinton mentioned this pull request Feb 27, 2023

Migrate SecretsManager to new crypto interface #4879

Closed

eliykat removed their request for review June 13, 2023 23:27

Hinton closed this Sep 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ADR-16] Refactor Folders #3732

[ADR-16] Refactor Folders #3732

Hinton commented Oct 10, 2022 •

edited

Loading

eliykat left a comment •

edited

Loading

Hinton commented Oct 11, 2022

eliykat commented Oct 12, 2022 •

edited

Loading

MGibson1 commented Oct 12, 2022

Hinton commented Oct 12, 2022 •

edited

Loading

eliykat left a comment •

edited

Loading

eliykat commented Nov 10, 2022

Hinton commented Nov 10, 2022

Hinton commented Dec 2, 2022

eliykat left a comment •

edited

Loading

eliykat Dec 5, 2022

Hinton Dec 5, 2022

eliykat Dec 6, 2022

eliykat Dec 5, 2022

Hinton commented Dec 5, 2022

eliykat left a comment

eliykat commented Jun 13, 2023

[ADR-16] Refactor Folders #3732

[ADR-16] Refactor Folders #3732

Conversation

Hinton commented Oct 10, 2022 • edited Loading

Type of change

Objective

Before you submit

eliykat left a comment • edited Loading

Choose a reason for hiding this comment

Hinton commented Oct 11, 2022

eliykat commented Oct 12, 2022 • edited Loading

MGibson1 commented Oct 12, 2022

Hinton commented Oct 12, 2022 • edited Loading

eliykat left a comment • edited Loading

Choose a reason for hiding this comment

eliykat commented Nov 10, 2022

Hinton commented Nov 10, 2022

Hinton commented Dec 2, 2022

eliykat left a comment • edited Loading

Choose a reason for hiding this comment

eliykat Dec 5, 2022

Choose a reason for hiding this comment

Hinton Dec 5, 2022

Choose a reason for hiding this comment

eliykat Dec 6, 2022

Choose a reason for hiding this comment

eliykat Dec 5, 2022

Choose a reason for hiding this comment

Hinton commented Dec 5, 2022

eliykat left a comment

Choose a reason for hiding this comment

eliykat commented Jun 13, 2023

Hinton commented Oct 10, 2022 •

edited

Loading

eliykat left a comment •

edited

Loading

eliykat commented Oct 12, 2022 •

edited

Loading

Hinton commented Oct 12, 2022 •

edited

Loading

eliykat left a comment •

edited

Loading

eliykat left a comment •

edited

Loading