Add forget all certificates & poll to client CLI by touilleMan · Pull Request #10017 · Scille/parsec-cloud

touilleMan · 2025-03-27T16:12:16Z

No description provided.

AureliaDolo

A few nits but otherwise LGTM

AureliaDolo · 2025-03-27T16:19:59Z

+    println!("You are about to clear the local certificates database for device:");
+    println!("{YELLOW}{short_id}{RESET} - {organization_id}: {human_handle} @ {device_label}");
+    println!("Are you sure? (y/n)");


If you want to be shinny, you can use the Confirm prompt from dialogue like here

I'm all okay for additional shininess, though I'd rather not do this change in this PR

This is because I've copied this code from another command, so it should also be modified

So I've opened issue #10020

AureliaDolo · 2025-03-27T16:23:53Z

+crate::build_main_with_client!(main, poll);
+
+pub async fn poll(_args: Args, client: &StartedClient) -> anyhow::Result<()> {
+    let mut spinner = start_spinner("Poll server for new certificates".into());


Here is an helper

I used it initially, however this helper doesn't display the number of new certificates

I think not showing the number of new certificates is fine when the poll is a side task, but for this CLI command it is the main thing so reporting is a good thing

The function could easily updated to return the number of certificate

AureliaDolo · 2025-03-27T16:25:57Z

        &self,
        latest_known_timestamps: Option<&PerTopicLastTimestamps>,
-    ) -> Result<(), CertifPollServerError> {
+    ) -> Result<usize, CertifPollServerError> {


What's that number ?

returns the number of new certificates

I've added a comment (there was already one for Client::poll_server_for_new_certificates but not for CertficateOps::poll_server_for_new_certificates

AureliaDolo · 2025-03-27T16:27:32Z

+    let alice = env.local_device("alice@dev1");
+    let ops = certificates_ops_factory(env, &alice).await;
+
+    let add_first_certifcate = async || {


Suggested change

let add_first_certifcate = async || {

let add_first_certificate = async || {

Max-7 · 2025-03-28T05:23:43Z

-    return { ok: true, value: null };
+    return {
+      ok: true,
+      // @ts-ignore: Too cumbersome to create a full `AvailableDevice` object,


You can just c/c from

parsec-cloud/client/src/parsec/claim_device.ts

Line 253 in 050fceb

this.device = {

It may be easier than trying to trick the linter.
This code will be gone very soon anyway.

done, thanks 🙏

FirelightFlagboy

Missing newsfragment

touilleMan · 2025-03-28T08:22:42Z

@FirelightFlagboy I've added the newsfragment 🙏

FirelightFlagboy

Missing test for cli change-authentication
Missing test for cli overwrite-server-url

FirelightFlagboy · 2025-03-28T08:38:11Z

+   #[clap(long, short, action)]
+        password: bool,
+   #[clap(long, short, action)]
+        keyring: bool,


suggestion (blocking): Use a enum instead that simply the pattern matching

I've created issue #10028 to track this, since we want to also standardize the other CLI commands

touilleMan · 2025-03-28T10:12:44Z

@FirelightFlagboy I've added a test for each new CLI command (and I found an error in the naming of a command thank this them ^^)

FirelightFlagboy · 2025-03-28T12:39:33Z

+
+#[rstest::rstest]
+#[tokio::test]
+async fn ok(tmp_path: TmpPath) {


issue (blocking): The test is not complete, we need to check if the device can be unlocked with the new authentication

FirelightFlagboy · 2025-03-28T12:40:48Z

+
+#[rstest::rstest]
+#[tokio::test]
+async fn ok(tmp_path: TmpPath) {


todo: Add a todo comment about inspecting the info of the device via device info (command not exiting for now)

…new certificates fetched

…ds to the client CLI

…ice_change_authentication` and add `update_device_overwrite_server_addr`

…ange_authentication` & `update_device_overwrite_organization_addr`

…write-server-url`

… by `libparsec.updateDeviceChangeAuthentication`

touilleMan requested review from a team as code owners March 27, 2025 16:12

AureliaDolo approved these changes Mar 27, 2025

View reviewed changes

touilleMan force-pushed the cli-forget_all_certificates branch 2 times, most recently from 709182a to 5111633 Compare March 27, 2025 21:30

touilleMan mentioned this pull request Mar 27, 2025

CLI use shinny command for prompt #10020

Closed

touilleMan force-pushed the cli-forget_all_certificates branch from 5111633 to 1e5b8d5 Compare March 27, 2025 21:49

Max-7 approved these changes Mar 28, 2025

View reviewed changes

FirelightFlagboy requested changes Mar 28, 2025

View reviewed changes

touilleMan added a commit that referenced this pull request Mar 28, 2025

Add newsfragment for #10017

871bd18

touilleMan force-pushed the cli-forget_all_certificates branch from 1e5b8d5 to 871bd18 Compare March 28, 2025 08:22

touilleMan added a commit that referenced this pull request Mar 28, 2025

Add newsfragment for #10017

848bd0a

touilleMan force-pushed the cli-forget_all_certificates branch from 871bd18 to 848bd0a Compare March 28, 2025 08:32

FirelightFlagboy requested changes Mar 28, 2025

View reviewed changes

touilleMan added a commit that referenced this pull request Mar 28, 2025

Add newsfragment for #10017

8f6fe3a

touilleMan force-pushed the cli-forget_all_certificates branch from 848bd0a to 8f6fe3a Compare March 28, 2025 08:50

touilleMan mentioned this pull request Mar 28, 2025

Cient CLI: standardize on doing --authentication=<flag> to setup the device authentication #10028

Open

touilleMan added a commit that referenced this pull request Mar 28, 2025

Add newsfragment for #10017

168056a

touilleMan force-pushed the cli-forget_all_certificates branch from 8f6fe3a to debeeee Compare March 28, 2025 10:12

mmmarcos reviewed Mar 28, 2025

View reviewed changes

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

Comment thread libparsec/crates/platform_device_loader/src/lib.rs

Comment thread cli/src/commands/device/overwrite_server_url.rs

FirelightFlagboy reviewed Mar 28, 2025

View reviewed changes

touilleMan added a commit that referenced this pull request Mar 28, 2025

Add newsfragment for #10017

82bb9a0

touilleMan force-pushed the cli-forget_all_certificates branch from bd0cf68 to 2f543ca Compare March 28, 2025 16:08

touilleMan added a commit that referenced this pull request Mar 28, 2025

Add newsfragment for #10017

fe794a1

touilleMan force-pushed the cli-forget_all_certificates branch from 2f543ca to d432b0d Compare March 28, 2025 16:26

touilleMan added a commit that referenced this pull request Mar 29, 2025

Add newsfragment for #10017

3b11958

touilleMan force-pushed the cli-forget_all_certificates branch from d432b0d to 0f11f1e Compare March 29, 2025 13:38

Add Client::forget_all_certificates

7eea62e

touilleMan added 11 commits April 1, 2025 14:13

Add test for CertificateOps::forget_all_certificates

90a63dd

Make Client::poll_server_for_new_certificates return the number of …

ff87754

…new certificates fetched

Add certificate poll & certificate forget-all-certificates comman…

c9074a9

…ds to the client CLI

Replace plaform_device_loader::change_authentication by `update_dev…

01b6b2e

…ice_change_authentication` and add `update_device_overwrite_server_addr`

Replace libparsec client_change_authentication by `update_device_ch…

7f83a6d

…ange_authentication` & `update_device_overwrite_organization_addr`

Add client CLI commands device change-authentication & `device over…

fdae30c

…write-server-url`

Re-generate electron&web bindings

f1f2081

Update GUI with libparsec.clientChangeAuthentication being replaced…

fcee98f

… by `libparsec.updateDeviceChangeAuthentication`

Add newsfragment for #10017

225dae5

Minor cleanups in libparsec_platform_device_loader

9cd6e7a

Display the newly created device in client CLI commands

f650a0b

touilleMan force-pushed the cli-forget_all_certificates branch from 0f11f1e to f650a0b Compare April 1, 2025 12:23

FirelightFlagboy approved these changes Apr 1, 2025

View reviewed changes

touilleMan added this pull request to the merge queue Apr 1, 2025

github-merge-queue Bot pushed a commit that referenced this pull request Apr 1, 2025

Add newsfragment for #10017

dc04b54

Merged via the queue into master with commit 7c77b60 Apr 1, 2025
15 checks passed

touilleMan deleted the cli-forget_all_certificates branch April 1, 2025 13:06

	let add_first_certifcate = async \|\| {
	let add_first_certificate = async \|\| {

Conversation

touilleMan commented Mar 27, 2025

Uh oh!

AureliaDolo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FirelightFlagboy left a comment

Choose a reason for hiding this comment

Uh oh!

touilleMan commented Mar 28, 2025

Uh oh!

FirelightFlagboy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

touilleMan commented Mar 28, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

FirelightFlagboy left a comment •

edited

Loading