Encrypting necessary local storage data #286
Conversation
|
|
||
| // TODO: Store users key that is generated from the password in memory so we | ||
| // don't keep a copy of the password | ||
| const PASSWORD = 'TEMP_PASSWORD'; |
There was a problem hiding this comment.
I thought for a first PR it would be good to just get the encryption working. I figure as a separate PR I'll store the pass key in memory.
If anyone has any recommendations on how I should do that, let me know. I know that metamask uses obs-store to keep the password in memory.
| @@ -32,7 +35,7 @@ function QuillStorageCells(storage: CellCollection) { | |||
| completed: false, | |||
| }), | |||
| ), | |||
| keyring: storage.Cell( | |||
| keyring: encryptedStorage.Cell( | |||
There was a problem hiding this comment.
This was the cleanest way I could think of to allow both encrypted and decrypted storage. If anyone has alternative ideas let me know.
This approach allows us to only encrypt specific cells. So for example, we can keep the preferences decrypted
There was a problem hiding this comment.
This is perfect! I was wishing for exactly the same setup. Just change the storage type and your collection is good to go.
|
|
||
| const payload = JSON.parse(readResult); | ||
| const { salt } = payload; | ||
| const passwordKey = await encryptor.keyFromPassword(PASSWORD, salt); |
There was a problem hiding this comment.
This key from password part will be moved somewhere else once we are keeping the key in memory
blakecduncan
changed the title
| @@ -32,7 +35,7 @@ function QuillStorageCells(storage: CellCollection) { | |||
| completed: false, | |||
| }), | |||
| ), | |||
| keyring: storage.Cell( | |||
| keyring: encryptedStorage.Cell( | |||
There was a problem hiding this comment.
This is perfect! I was wishing for exactly the same setup. Just change the storage type and your collection is good to go.
| @@ -19,7 +19,10 @@ import { QuillTransaction } from './types/Rpc'; | |||
| // fields that always have concrete values incrementally without breaking | |||
| // existing clients. | |||
|
|
|||
| function QuillStorageCells(storage: CellCollection) { | |||
| function QuillStorageCells( | |||
| storage: CellCollection, | |||
There was a problem hiding this comment.
lets make this change to be more explicit about the storage type
- storage
+ standardStorageA few more places within the file
| public storage: CellCollection, | ||
| public encryptedStorage: CellCollection, | ||
| public currencyConversionConfig: CurrencyConversionConfig, | ||
| ) { | ||
| this.cells = QuillStorageCells(storage); | ||
| this.cells = QuillStorageCells(storage, encryptedStorage); |
There was a problem hiding this comment.
considering storage and encryptedStorage are types of the same entity, we can group these and pass as an object instead of 2 different constructor arguments
lets create a new type
export type StorageConfig = {
standardStorage: CellCollection;
encryptedStorage: CellCollection;
};and modify the constructor to accept a single storage object
public storage: StorageConfig,and access standardStorage and encryptedStorage separately
- this.cells = QuillStorageCells(storage, encryptedStorage);
+ this.cells = QuillStorageCells(storage.standardStorage, storage.encryptedStorage);| @@ -19,7 +19,10 @@ import { QuillTransaction } from './types/Rpc'; | |||
| // fields that always have concrete values incrementally without breaking | |||
| // existing clients. | |||
|
|
|||
| function QuillStorageCells(storage: CellCollection) { | |||
| function QuillStorageCells( | |||
There was a problem hiding this comment.
the separate arguments are fine here since this function only handles storage 👍🏼
extension/source/QuillContext.tsx
Outdated
| storage: CellCollection, | ||
| encryptedStorage: CellCollection, |
There was a problem hiding this comment.
Same changes as QuillController's constructor can be made here by importing the StorageConfig type since both arguments are part of same entity.
extension/source/background/index.ts
Outdated
| extensionLocalCellCollection, | ||
| encryptedLocalCellCollection, |
There was a problem hiding this comment.
this would be converted to a single object as per changes mentioned above.
|
|
||
| // TODO: Store users key that is generated from the password in memory so we | ||
| // don't keep a copy of the password | ||
| const PASSWORD = 'TEMP_PASSWORD'; |
What is this PR doing?
Adding a local cell collection that encrypts the
valuesbefore persisting to local storageHow can these changes be manually tested?
Yes this was tested. There should be no functional UX changes to the app.
Does this PR resolve or contribute to any issues?
277
Checklist
Guidelines
resolve conversationbutton is for reviewers, not authors