Make identity naming more consistent + fix bugs #1140

jdetter · 2024-04-23T11:00:36Z

Description of Changes

Please describe your change, mention any related tickets, and so on here.

There are several places in the CLI where you can set the name of an identity:
- spacetime identity new --name <name> --no-email
- spacetime identity set-name <identity> <name>
- spacetime identity import <identity> <token> --name <name>

In these places we had slightly different logic for validating that an identity name looks okay. I've made the logic in these places more consistent. This will prevent people from accidentally adding identities with the same name or with an invalid name (identity names cannot look like identities).

API and ABI breaking changes

If this is an API or ABI breaking change, please apply the
corresponding GitHub label.

Not breaking

Expected complexity level and risk

How complicated do you think these changes are? Grade on a scale from 1 to 5,
where 1 is a trivial change, and 5 is a deep-reaching and complex change.

This complexity rating applies not only to the complexity apparent in the diff,
but also to its interactions with existing and future code.

If you answered more than a 2, explain what is complex about the PR,
and what other components it interacts with in potentially concerning ways.

0

Testing

Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!

Write a test you've completed here.
Create a new identity with the name admin. Then create another identity with no name. Export this new identity. Delete the identity. Now import this identity with the name admin, you will get an error.

bfops · 2024-04-24T18:37:35Z

crates/cli/src/subcommands/identity.rs

@@ -585,6 +577,7 @@ async fn exec_token(config: Config, args: &ArgMatches) -> Result<(), anyhow::Err
 async fn exec_set_name(mut config: Config, args: &ArgMatches) -> Result<(), anyhow::Error> {
    let new_name = args.get_one::<String>("name").unwrap();
    let identity_or_name = args.get_one::<String>("identity").unwrap();
+    config.can_set_name(Some(identity_or_name.as_str()))?;


This is a regression. It essentially renders unnamed identities unusable.

$ spacetime identity new -s local --no-email | tee output IDENTITY 631d3896d10a5fa9db73a95a5a2e20981ab71bc32cb908859e449b5eeb886e9a NAME EMAIL $ cat output | grep 'IDENTITY' | awk '{print $2;}' | tee identity 631d3896d10a5fa9db73a95a5a2e20981ab71bc32cb908859e449b5eeb886e9a $ spacetime identity set-name $(cat identity) new-identity-name - Updated identity: - IDENTITY 631d3896d10a5fa9db73a95a5a2e20981ab71bc32cb908859e449b5eeb886e9a - NAME new-identity-name + Error: An identity name cannot be an identity. + [1]

Thank you for catching this Zeke that would have been a disaster 😦

jdetter · 2024-04-25T04:14:33Z

crates/cli/src/subcommands/identity.rs

+
+    if config.identity_configs().iter().any(|ic| ic.identity == identity) {
+        return Err(anyhow::anyhow!("Identity already exists in config"));
+    };


I've also added a check here to prevent adding the same identity multiple times

(factored out config.identity_exists so it's somewhat self-describing)

bfops · 2024-04-25T05:01:56Z

crates/cli/src/util.rs

@@ -149,6 +149,10 @@ pub async fn select_identity_config(
    server: Option<&str>,
 ) -> Result<IdentityConfig, anyhow::Error> {
    if let Some(identity_or_name) = identity_or_name {
+        if identity_or_name.is_empty() {
+            return Err(anyhow::anyhow!("Identity value cannot be empty."));
+        }


why did we need this check? shouldn't get_identity_config("") fail anyway?

bfops · 2024-04-25T05:03:26Z

crates/cli/src/config.rs

+        }
+
+        Ok(())
+    }


nit: how would you feel about making this like:

pub fn can_set_name(&self, new_nickname: &str) -> Result<(), anyhow::Error> {

(it seems a bit weird to call can_set_name(None)?)

bfops · 2024-04-25T05:10:51Z

crates/cli/src/subcommands/publish.rs

@@ -111,6 +112,8 @@ pub async fn exec(mut config: Config, args: &ArgMatches) -> Result<(), anyhow::E
    let build_debug = args.get_flag("debug");
    let wasm_file = args.get_one::<PathBuf>("wasm_file");
    let database_host = config.get_host_url(server)?;
+    // Validate the identity first to provide an early error message
+    util::select_identity_config(&mut config, identity, server).await?;


could we just move the later code to here, rather than effectively duplicating this check? like could we put this up here:

let (auth_header, identity) = get_auth_header(&mut config, anon_identity, identity, server) .await? .unzip();

bfops · 2024-04-26T17:34:40Z

crates/cli/src/util.rs

@@ -152,7 +152,7 @@ pub async fn select_identity_config(
        config
            .get_identity_config(identity_or_name)
            .cloned()
-            .ok_or_else(|| anyhow::anyhow!("No such identity credentials for identity: {}", identity_or_name))
+            .ok_or_else(|| anyhow::anyhow!("No identity credentials for identity \"{}\"", identity_or_name))


(the extra "" make it clearer when it's just the empty string)

bfops · 2024-04-26T17:55:54Z

crates/cli/src/subcommands/publish.rs

+    //  easily create a new identity with an email
+    let (auth_header, identity) = get_auth_header(&mut config, anon_identity, identity, server)
+        .await?
+        .unzip();


moving this earlier means that we check identity validity before we go through a (potentially very long) build.

bfops

LGTM once the base PR merges

config

jdetter requested review from bfops and kurtismullins April 23, 2024 12:36

jdetter mentioned this pull request Apr 24, 2024

Prevent importing an identity with a name that already exists in the config #1139

Closed

2 tasks

bfops added release-any To be landed in any release window CLI only This change only affects the CLI behavior labels Apr 24, 2024

bfops requested changes Apr 24, 2024

View reviewed changes

jdetter requested a review from bfops April 25, 2024 04:13

jdetter commented Apr 25, 2024

View reviewed changes

bfops reviewed Apr 25, 2024

View reviewed changes

bfops reviewed Apr 26, 2024

View reviewed changes

bfops mentioned this pull request Apr 26, 2024

CLI - Small refactors to identity code #1169

Merged

1 task

bfops changed the base branch from master to bfops/cli-identity-refactor April 26, 2024 17:49

bfops reviewed Apr 26, 2024

View reviewed changes

bfops added the Do not merge Do not merge PRs with this label without coordinating further label Apr 26, 2024

bfops approved these changes Apr 26, 2024

View reviewed changes

Base automatically changed from bfops/cli-identity-refactor to master May 14, 2024 11:39

jdetter removed the Do not merge Do not merge PRs with this label without coordinating further label May 14, 2024

John Detter and others added 9 commits May 14, 2024 11:03

Prevent importing an identity with a name that already exists in the

f056ddf

config

Make identity naming more consistent

56bcb4b

Fix clippy lints

558ff29

Several bug fixes and UX improvements

8a40a13

Prevent someone from adding the same identity twice

1e6c2af

Fix lint

561d7aa

[jdetter/make-identity-naming-more-consistent]: review

df303c9

[jdetter/make-identity-naming-more-consistent]: review

aaf1bf3

[jdetter/make-identity-naming-more-consistent]: review

ce04e2d

jdetter force-pushed the jdetter/make-identity-naming-more-consistent branch from ae62610 to ce04e2d Compare May 14, 2024 16:15

Reverted file

6a92e17

jdetter enabled auto-merge May 14, 2024 16:22

jdetter added this pull request to the merge queue May 14, 2024

Merged via the queue into master with commit f13a143 May 14, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make identity naming more consistent + fix bugs #1140

Make identity naming more consistent + fix bugs #1140

jdetter commented Apr 23, 2024 •

edited

Loading

bfops Apr 24, 2024

jdetter Apr 25, 2024

jdetter Apr 25, 2024

bfops Apr 26, 2024

bfops Apr 25, 2024

bfops Apr 25, 2024 •

edited

Loading

bfops Apr 25, 2024 •

edited

Loading

bfops Apr 26, 2024

bfops Apr 26, 2024

bfops left a comment

+                      }
+                      Ok(())
+                  }

Make identity naming more consistent + fix bugs #1140

Make identity naming more consistent + fix bugs #1140

Conversation

jdetter commented Apr 23, 2024 • edited Loading

Description of Changes

API and ABI breaking changes

Expected complexity level and risk

Testing

bfops Apr 24, 2024

Choose a reason for hiding this comment

jdetter Apr 25, 2024

Choose a reason for hiding this comment

jdetter Apr 25, 2024

Choose a reason for hiding this comment

bfops Apr 26, 2024

Choose a reason for hiding this comment

bfops Apr 25, 2024

Choose a reason for hiding this comment

bfops Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

bfops Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

bfops Apr 26, 2024

Choose a reason for hiding this comment

bfops Apr 26, 2024

Choose a reason for hiding this comment

bfops left a comment

Choose a reason for hiding this comment

jdetter commented Apr 23, 2024 •

edited

Loading

bfops Apr 25, 2024 •

edited

Loading

bfops Apr 25, 2024 •

edited

Loading