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

[eliykat]

Type of change

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

Objective

This PR is an alternative approach to #3732 for discussion.

Main features/goals:

• get rid of decrypt methods on models completely (whether view or domain)
• remove ContainerService
• add a generic decryptItem method which accepts any domain object and can decrypt it. Importantly, I don't want us to keep passing services into objects and having several nested layers of calls, I just want a service that takes a simple data object and manipulates it.
• bonus goal which is negotiable, but recommended by @MGibson1: stop caching decryption results in domain objects, which risks mixing and potentially saving encrypted & decrypted data.

One thing this PR doesn't address is @Hinton's preferred data flow outlined in #3732, where all objects can convert themselves to/from the domain object.

It also doesn't address objects like Attachments or Sends, which first decrypt a key on the object, and then use that key to decrypt the rest of the object. I expect we can address that through (a) specifying the "encrypted key" property as an argument to the @encrypted decorator, or (b) a separate decryption method, or (c) just making a second call to decryptItem with the key.

Code changes

• libs/common/src/misc/encrypted.decorator.ts - this is a decorator that you use to annotate properties that are either (a) EncStrings or (b) objects containing EncStrings (that is, nested domain objects). All it does is build a list of these properties which is attached to the prototype's metadata, which you can access with getEncryptedProperties.

• libs/common/src/services/encrypt.service.ts - this takes an encrypted domain object and iterates through the list of encrypted properties, either decrypting EncStrings or recursively decrypting nested domain objects. To avoid caching results in the domain object, it builds up an intermediate object of decrypted properties, and then passes it to a toView factory method on the domain object.

• libs/common/src/interfaces/decryptable.interface.ts - enforces the requirement that each "decryptable" (domain) object has a toView method, which knows how to instantiate and populate its view model. See the example test objects in libs/common/spec/services/encrypted-objects.ts for how this is implemented. The reason for having this on the domain object is that we can enforce it via an interface, whereas on the View object it would be a static method that we can't enforce.

The rest of the code are tests to assert that this works as intended, using test objects in libs/common/spec/services/encrypted-objects.ts.

Screenshots

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

[Hinton]

How do you envision what would actually decrypt the objects? One of the goals of #3732 was to remove keys from components. Components in my opinion should never need to first fetch a key, and then decrypt it.

We could put decrypt and encrypt methods on each domain service, but

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.

One variant is to define a FolderEncryptionService but that's a lot of services just for abstracting away key fetching.

[eliykat]

How do you envision what would actually decrypt the objects? One of the goals of #3732 was to remove keys from components. Components in my opinion should never need to first fetch a key, and then decrypt it.

We could put decrypt and encrypt methods on each domain service, but

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.

One variant is to define a FolderEncryptionService but that's a lot of services just for abstracting away key fetching.

I agree we should move any crypto logic out of components, but that's relatively rare at the moment, and should be moved into domain services. Mostly it'll be domain services calling encrypt/decrypt methods to manage their own objects. And it's not that the components will be calling something like FolderEncryptionService.encrypt(folder), that's too low level - components are concerned with user actions, not encrypting a single folder. The component will call FolderService.upsert(folder) (or whatever goal they're trying to achieve) and FolderService will know what encryption needs to happen for that to occur.

In that scenario, I think it's fine that FolderService gets the key it needs for decryption. Getting a key requires business logic: you need to know whether you're dealing with a personal item or an org item, and as a domain service it's already dealing with business logic. The other reason is pragmatic: having a higher level service orchestrate low-level services like this allows us to have simpler, smaller low-level services which are easier to test.

My mental model is something like:

Top layer (components - presentation layer)
      ↑↓ 
Middle layer (domain services - business logic. e.g. `FolderService`)
      ↑↓ 
Bottom layer (low-level services. e.g.`StorageService`, `EncryptService`)

...where the top layer rarely/never communicates with the bottom layer directly, and each layer communicates up or down but rarely/never sideways. This is not a formal distinction we have at the moment, but it seems to be roughly what we're moving towards, and I think it's a useful way to think about it.

[MGibson1]

Probably BufferArray is needed here, too.

[MGibson1]

I think it's a good idea to convert this to a fromDomain and come up with a new name for the interface, that way we could have a single Domain hydrate multiple views for different purposes, as Oscar proposed.

[eliykat]

I'm fine with that, but fromDomain would be a static factory method, right? Any ideas on how to enforce that interface when it's static? Or would we hand the factory method explicitly to the decryptItem call like

decryptItem<TEncrypted, TDecrypted>(item: TEncrypted, key: SymmetricCryptoKey, viewFactory: (props: DecryptedProperties<TEncrypted>) => TDecrypted): TDecrypted

assuming that DecryptedProperties<TEncrypted> is the mapped type for the decrypted props.

[MGibson1]

I don't particularly like how this turned out of any of the examples.

For one, the decrypted properties is an any rather than some inferred type (think Jsonify magic).

Second, it assumes that decrypted properties are going to be named the same in the View layer.

I think some work needs to be done to improve this interface a bit before it's ready to go.

[eliykat]

Yeah this was a bit of a hack to illustrate the idea of an intermediate "decrypted properties" object, as a way to get decrypted values to the View without caching them on Domain. It can be improved for sure.

I wasn't sure whether mapping property values to different properties on the View object was actually required. We have that capability at the moment and I don't think we actually use it anywhere. But it's probably best to keep it for flexibility, especially if we're considering having multiple Views.

Typing this correctly was a TODO, in theory we should be able to have a mapped type like:

• EncStrings become strings
• Domain becomes the corresponding View (?)
• everything else stays the same

So I can definitely experiment with that further. As you say, jsonify magic.

[MGibson1]

In general, I like this approach of Domain's defining encrypted data and the EncryptService being able to locate and decrypt it through decorators. It smacks of the current alreadyEncrypted stuff we do in Domain today, but implemented in a more discoverable way.

However, I agree with Oscar that we need to shift the responsibility of converting to/from Domain to the other models i.e. View in this case. This also means the decrypted parameters type needs to be discoverable so that we can use it in the View classes

[Hinton]

I'm not a huge fan of magically inspecting objects and figuring out what to do with them (especially not if it needs a custom reflection polyfill). It's one of the issues I have the buildDomainModel and decryptObj. Not sure what we can do about it though without an explicit method on each view.

Having a method that do deep inspection of other objects and do different things depending on which type they are, feels like quite an anti pattern of object oriented programming. Having EncString and EncObject (or other objects) implement their own decrypt methods we separate the concerns of how to decrypt things which will handle further types.

Then again maybe EncString and EncObject are the lowest level entities that denote different things. But we also have Attachments which has a different logic, and possibly more.

[eliykat]

I'm reconsidering this approach. The biggest practical limitation of this PR is that we can't do partial decryption because everything is marked for decryption with the @encrypted decorator. If we were constructing different View objects, we'd still be decrypting all encrypted properties, and then throwing some away - which is not performant. (One use case for partial decryption is to improve load times, but that doesn't work if we're decrypting everything anyway.) Especially if we're no longer caching decryption results in the domain object, we're potentially re-decrypting values repeatedly to discard them.

I also think I might be handwaving more complex objects. I'm not sure how this would handle switch statements in the Cipher object, for example. (I mean, the Cipher object should be multiple subclasses, but now we're creating more problems for ourselves.) This might be figured out but I suspect it would add complexity to the pattern.

Having the View object know how to decrypt and process its own data does have big advantages here in flexibility. So I'm probably leaning towards #3732 today. I'll go revisit that PR.

[eliykat]

Closing after our recent discussions