Proposal - Decrypt refactor

[Hinton]

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 proposes a refactor of how we decrypt domain models.

It primarily showcases

• A way to remove decrypt from domain objects.
• Remove container service on window.
• Proposes a generic decrypt method which can accept an arbitrary view.

Some background

Our data model currently consists of multiple data types, with conversion happening in layers. Take folders for example.

The current flow is: FolderRequest -> FolderData -> Folder -> FolderView

From this view it's not clear which data model is in the center. In my opinion we should flip the relationships, and put Folder in the center. Every data model should know how to convert to Domain, and from Domain.

The way this is implemented is by adding a generic method decrypt to CryptoService. (Yes this is not an optimal place, I would rather move it to EncryptionService, but it needs a way to fetch the encryption keys which is only possible from the crypto service.

The generic method cryptoService.decrypt fetches the necessary encryption key, and calls the static FolderView.decrypt(cryptoService, key, folder). The way the decrypt method fetches the relevant key is by checking if the domain model contains an orgnaizationId. This is somewhat less than optimal, we could look into moving this responsibility to either the Domain model, or the View. (by having another static method which returns, userkey or orgkey(id).

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.

Code changes

• file.ext: Description of what was changed and why

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]

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,

1. to limit the scope of the refactor.
2. 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]

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]

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]

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.

[eliykat]

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]

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

[Hinton]

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.

[Hinton]

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]

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