Refactor key backup recovery to prepare for rust #3708
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BillCarsonFr
force-pushed
the
valere/element-r/backup/restore/pr1
branch
from
4e78cf3 to
4c4ab28
Compare
BillCarsonFr
force-pushed
the
valere/element-r/backup/restore/pr1
branch
from
4c4ab28 to
641f07a
Compare
3 tasks
BillCarsonFr
requested review from
florianduros,
kerryarchibald and
richvdh
and removed request for
kerryarchibald
3 tasks
richvdh
previously requested changes
Sep 7, 2023
BillCarsonFr
requested review from
a team and
SimonBrandner
and removed request for
a team
BillCarsonFr
requested review from
richvdh and
t3chguy
and removed request for
richvdh
andybalaam
requested changes
Sep 12, 2023
src/client.ts
Outdated
| const algorithm = await BackupManager.makeAlgorithm(backupInfo, async () => { | ||
| return privKey; | ||
| }); | ||
| const backupDecryptor = await this.cryptoBackend!.getBackupDecryptor(backupInfo, privKey); |
There was a problem hiding this comment.
How do we know that this.cryptoBackend is defined? Can we have a comment explaining?
There was a problem hiding this comment.
Oh wait, I see it on line 3786. In that case, I think we shouldn't need the !? If we do, please add a little comment saying that we bailed out earlier if we had no cryptoBackend
src/crypto/index.ts
Outdated
| public async getBackupDecryptor(backupInfo: IKeyBackupInfo, privKey: ArrayLike<number>): Promise<BackupDecryptor> { | ||
| if (!(privKey instanceof Uint8Array)) { | ||
| // eslint-disable-next-line @typescript-eslint/no-base-to-string | ||
| throw new Error(`getBackupDecryptor expects Uint8Array, got ${privKey}`); |
There was a problem hiding this comment.
Is it potentially dangerous to log the private key here? Maybe we could log its type, or omit it.
There was a problem hiding this comment.
(would also mean we could remove the eslint-disable...
There was a problem hiding this comment.
Right, was copied from old code but agree that it doesn't look right. Removed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce a new
BackupDecryptorinterface, which we can implement separately for legacy and Rust crypto.Checklist
This change is marked as an internal change (Task), so will not be included in the changelog.