Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Device Dehydration | js-sdk: store/load dehydration key #4599

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from

Conversation

BillCarsonFr
Copy link
Member

@BillCarsonFr BillCarsonFr commented Dec 26, 2024

Fixes #4483

Added basic support to cache created keys and read from store instead of in memory.
Then adding some basic signalling to DeviceDehydrationManager, helpful for tests and also for apps to have some feedback on device rehydration (reports progress for example)

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

Copy link
Member

@uhoreg uhoreg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only did a quick review because the the compiler errors are distracting

src/rust-crypto/DehydratedDeviceManager.ts Outdated Show resolved Hide resolved
src/rust-crypto/DehydratedDeviceManager.ts Outdated Show resolved Hide resolved
src/crypto-api/index.ts Outdated Show resolved Hide resolved
@BillCarsonFr BillCarsonFr marked this pull request as ready for review January 14, 2025 08:57
@BillCarsonFr BillCarsonFr requested review from a team as code owners January 14, 2025 08:57
@richvdh richvdh self-requested a review January 14, 2025 11:38
@richvdh

This comment was marked as resolved.

Copy link
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me, but definitely needs review from people who are closer to the dehydrated devices design.

src/crypto-api/index.ts Outdated Show resolved Hide resolved
src/crypto-api/index.ts Outdated Show resolved Hide resolved
src/crypto-api/index.ts Outdated Show resolved Hide resolved
src/crypto-api/index.ts Outdated Show resolved Hide resolved
spec/integ/crypto/device-dehydration.spec.ts Outdated Show resolved Hide resolved
src/crypto-api/index.ts Outdated Show resolved Hide resolved
@uhoreg uhoreg force-pushed the valere/cache_dehydration_key_in_store branch from d10fd42 to a1e0bbf Compare January 22, 2025 02:01
@uhoreg uhoreg requested a review from richvdh January 22, 2025 02:05
@uhoreg
Copy link
Member

uhoreg commented Jan 22, 2025

I think that I've addressed all of @richvdh 's comments.

(Of course, this can't be merged until #4634 is fixed)

src/crypto-api/CryptoEventHandlerMap.ts Outdated Show resolved Hide resolved
src/rust-crypto/DehydratedDeviceManager.ts Outdated Show resolved Hide resolved
src/rust-crypto/DehydratedDeviceManager.ts Outdated Show resolved Hide resolved
src/crypto-api/CryptoEvent.ts Outdated Show resolved Hide resolved
src/crypto-api/CryptoEvent.ts Outdated Show resolved Hide resolved
src/crypto-api/CryptoEvent.ts Outdated Show resolved Hide resolved
src/crypto-api/CryptoEvent.ts Outdated Show resolved Hide resolved
Comment on lines 170 to 171
const cachedKey = await this.getCachedKey();
if (!cachedKey) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest an early return here, to avoid two calls to getCachedKey and dodgy ! operators

Suggested change
const cachedKey = await this.getCachedKey();
if (!cachedKey) {
const cachedKey = await this.getCachedKey();
if (cachedKey) return cachedKey;

(can we then inline getCachedKey?)

@uhoreg
Copy link
Member

uhoreg commented Jan 24, 2025

SonarCloud says I need to add more tests, but should be ready for re-review otherwise

@uhoreg uhoreg requested a review from richvdh January 24, 2025 14:45
@uhoreg uhoreg added the T-Feature Request to add a new feature which does not exist right now label Jan 24, 2025
@uhoreg uhoreg added T-Enhancement and removed T-Feature Request to add a new feature which does not exist right now labels Jan 24, 2025
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits, but LGTM.

And yes, looks like we need a test for loading the key from 4S?

src/crypto-api/CryptoEvent.ts Outdated Show resolved Hide resolved
src/rust-crypto/DehydratedDeviceManager.ts Outdated Show resolved Hide resolved
src/rust-crypto/DehydratedDeviceManager.ts Show resolved Hide resolved
src/rust-crypto/DehydratedDeviceManager.ts Outdated Show resolved Hide resolved
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Device Dehydration | js-sdk: store dehydration key
5 participants