Web compact to load devices #9554

FirelightFlagboy · 2025-02-04T09:48:29Z

No description provided.

- Implement device-loader for wasm32 using local storage web API. - Update integration tests to be platform agnostic. - Group all tests as single module (simplify configuring tests for wasm32)

touilleMan · 2025-03-05T08:06:48Z

libparsec/crates/platform_device_loader/Cargo.toml

@@ -7,6 +7,7 @@ homepage.workspace = true
 license.workspace = true
 version.workspace = true
 repository.workspace = true
+autotests = false


This shouldn't be needed since all tests are unit tests here

touilleMan · 2025-03-05T08:07:57Z

libparsec/crates/platform_device_loader/src/web/mod.rs

@@ -1,48 +1,96 @@
 // Parsec Cloud (https://parsec.cloud) Copyright (c) BUSL-1.1 2016-present Scille SAS

+mod error;
+mod internal;
+#[cfg(test)]


For consistency with the codebase, this test module should be moved at the end of the file

touilleMan · 2025-03-05T08:13:24Z

libparsec/crates/platform_device_loader/tests/units/list.rs

@@ -26,28 +30,21 @@ async fn list_no_devices(tmp_path: TmpPath, #[case] path_exists: bool) {
 #[parsec_test]
 async fn ignore_invalid_items(tmp_path: TmpPath) {
    let devices_dir = tmp_path.join("devices");
+    #[cfg(not(target_arch = "wasm32"))]


I think we should move this creation of the parent directory into create_device_file:

it simplifies the test

this utils function is only used for tests as part of the setup phase of said test, so the intention of the caller is obvious: we want to have a device available at this location, and returning an error if the parent doesn't exist only means the caller has to do more chores

touilleMan · 2025-03-05T08:14:34Z

libparsec/crates/platform_device_loader/tests/units/load.rs

 enum BadPathKind {
    UnknownPath,
    ExistingParent,
    NotAFile,
 }

+// Wasm impl do not use a filesystem, so we do not have invalidPath error except if the path


Should we add test using a Path with bad utf8 characters then ?

touilleMan · 2025-03-05T08:21:54Z

Cargo.lock

@@ -2153,7 +2181,9 @@ dependencies = [
 name = "libparsec_platform_device_loader"
 version = "3.3.1-a.0+dev"
 dependencies = [
+ "base64",


use data_encoding crate instead for base64
(also see #9863)

touilleMan · 2025-03-05T08:48:06Z

libparsec/crates/platform_device_loader/src/web/internal.rs

+    pub(crate) fn new() -> Result<Self, NewStorageError> {
+        let window = web_sys::window().ok_or(NewStorageError::NoWindow)?;
+        let storage = if cfg!(test) {
+            // Use session storage for tests, So the browser does not try to persist the data.


Just to be sure: we only use this code in case we are not using the testbed (since with the testbed, the whole platform implementation of device loader is skipped in favor of a testbed-dedicated code)

touilleMan · 2025-03-05T09:05:01Z

libparsec/crates/platform_device_loader/src/web/internal.rs

+    ) -> Result<(), SaveDeviceFileError> {
+        let data = file_data.dump();
+        let b64_data = BASE64_STANDARD.encode(&data);
+        self.storage


I'd rather avoid having to rely on a non-atomic operation here (i.e. storing two things in the localStorage, since there is no transaction in localStorage)

Non-atomicity is always more paint since we have to take into account weird corner cases. like an item is present in the list, but its corresponding device is not, or vice-versa. Regarding the last point, it theory it shouldn't occur since we store the device first, then modify the list, however I didn't see any guarantee in localStorage specification about that so who knows....

Also it saves more error handling when adding/removing devices since we no longer have to deal with the fact we might not be able to deserialize the list (and this case should we overwrite the list and risk losing access ? or should we do nothing and potentially never be able to add new device ever again...)

On top of that if add_item_to_list/remove_item_from_list is buggy at some point, we will lose access to the previous device :/

So ideally we should remove the list entirely and rely on querying the localStorage to discover the available devices. Given the number of devices is expected to be very low, I think we can simply do something like this:

> sessionStorage.setItem("device/123", "<device 123 payload>") undefined > sessionStorage.setItem("device/abc", "<device abc payload>") undefined > for (i = 0; i < localStorage.length; i++) { var key = sessionStorage.key(i); if (key.startsWith("device/")) { console.log(sessionStorage.getItem(key)); } } <device abc payload> <device 123 payload>

touilleMan · 2025-03-05T09:17:19Z

libparsec/crates/platform_device_loader/src/web/error.rs

+    JsonDeserializationError = {
+        JsonDecode(serde_json::Error)
+    };
+    JsonSerError = {


In the codebase we consider the encoding is always possible (since we encode only valid data), so there is no need for this error (we should just panic if it occurs)

see what is done in the libparsec_types::format_v0_dump function

touilleMan · 2025-03-05T09:20:59Z

libparsec/crates/platform_device_loader/src/web/error.rs

+            LoadDeviceError::InvalidFileType => Self::InvalidData,
+            LoadDeviceError::GetSecretKey(_) => Self::DecryptionFailed,
+            LoadDeviceError::DecryptAndLoad(e) => e.into(),
+            LoadDeviceError::InvalidPath { .. } => Self::InvalidPath(anyhow::anyhow!("{value}")),


In web, the path is totally under our control:

in prod it is a dummy value set at compilation time

in test, we are generating this value to whatever suits us

So I think it is safe to assume the path is always valid utf8, and just panic if that's not the case (we should just have a comment next to panic to explain why it should never occur in practice)

touilleMan · 2025-03-05T09:27:13Z

libparsec/crates/platform_device_loader/src/web/error.rs

+    crate::RemoveDeviceError
+);
+
+error_set::error_set! {


This approach is interesting, we should definitely investigate to see how well suited it is to simplify our current cumbersome error handling system ^^

Next time, it would be better to inform us prior to introduce change that add new dependencies or challenge the standard way we do certain things 🙏

FirelightFlagboy force-pushed the load-parsec-device-web branch 7 times, most recently from a7bb449 to 16a7648 Compare February 6, 2025 15:59

FirelightFlagboy force-pushed the load-parsec-device-web branch from 16a7648 to 4cff926 Compare February 13, 2025 10:04

FirelightFlagboy changed the base branch from rework-load-device to master February 13, 2025 10:04

FirelightFlagboy force-pushed the load-parsec-device-web branch 3 times, most recently from d7194ba to 403671a Compare February 17, 2025 14:02

FirelightFlagboy linked an issue Feb 17, 2025 that may be closed by this pull request

Store Parsec device in browser on web mode #9367

Open

FirelightFlagboy force-pushed the load-parsec-device-web branch 10 times, most recently from 6186c21 to 1385bf1 Compare February 20, 2025 12:32

FirelightFlagboy force-pushed the load-parsec-device-web branch from 1385bf1 to 0f85bc3 Compare March 3, 2025 12:23

FirelightFlagboy marked this pull request as ready for review March 3, 2025 12:48

FirelightFlagboy requested a review from a team as a code owner March 3, 2025 12:48

feat(device-loader): Impl for wasm32 using local storage

552256b

- Implement device-loader for wasm32 using local storage web API. - Update integration tests to be platform agnostic. - Group all tests as single module (simplify configuring tests for wasm32)

FirelightFlagboy force-pushed the load-parsec-device-web branch from 0f85bc3 to 552256b Compare March 3, 2025 12:57

touilleMan reviewed Mar 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Web compact to load devices #9554

Web compact to load devices #9554

FirelightFlagboy commented Feb 4, 2025

touilleMan Mar 5, 2025

touilleMan Mar 5, 2025

touilleMan Mar 5, 2025

touilleMan Mar 5, 2025

touilleMan Mar 5, 2025

touilleMan Mar 5, 2025

touilleMan Mar 5, 2025

touilleMan Mar 5, 2025

touilleMan Mar 5, 2025

touilleMan Mar 5, 2025

Web compact to load devices #9554

Are you sure you want to change the base?

Web compact to load devices #9554

Conversation

FirelightFlagboy commented Feb 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment