Feat/tvc passkeys#161
Conversation
richardpringle
left a comment
richardpringle
left a comment
There was a problem hiding this comment.
Not sure how useful this review is since this is in draft...
Basically all nits
| if let Some(session) = StoredPasskeySession::load(alias, org_config).await? { | ||
| return Ok(( | ||
| org_config.id.clone(), | ||
| org_config.api_base_url.clone(), |
There was a problem hiding this comment.
Do you need to clone here?
| } | ||
|
|
||
| #[derive(Debug, ClapArgs)] | ||
| pub struct ListArgs {} |
There was a problem hiding this comment.
Not sure why we don't have lint for this...
| pub struct ListArgs {} | |
| pub struct ListArgs; |
| pub enum AuthCommands { | ||
| /// Manage passkey authenticators. | ||
| Passkey(PasskeyArgs), | ||
| } |
There was a problem hiding this comment.
Might be worth mentioning other types of planned auth? Otherwise this seems over-abstracted.
(This is a nit though)
|
|
||
| if args.create_org { | ||
| return run_dashboard_signup_handoff(args).await; | ||
| } |
There was a problem hiding this comment.
What happens if the passkey and create-org options are passed in? With the --create-org arg just get ignored?
We should have proper validation before we get here
| async fn run_passkey_login(args: Args, is_non_interactive: bool) -> Result<()> { | ||
| if is_non_interactive { | ||
| bail!("passkey authentication requires an interactive terminal; remove --non-interactive"); | ||
| } |
There was a problem hiding this comment.
It's cleaner to do validation outside of the function instead of early returning immediately
| None => bail!( | ||
| "Organization is required for passkey login. Pass --org or run `tvc login` first." | ||
| ), | ||
| }; |
| None => { | ||
| bail!("Organization '{org_query}' not found. Run `tvc login` without --passkey first.") | ||
| } | ||
| }; |
|
|
||
| fn attest(&self, _challenge: &str, _name: &str) -> Result<WebAuthnAttestation> { | ||
| bail!("passkey registration is not implemented for this transport") | ||
| } |
There was a problem hiding this comment.
Why not use subtraits here?
|
|
||
| impl WebAuthnStamper<FixtureCeremony> { | ||
| /// Construct a deterministic stamper for CI tests without hardware. | ||
| pub fn new_for_tests(assertion: WebAuthnAssertion) -> Self { |
There was a problem hiding this comment.
Should this be behind a feature flag?
| let header = stamper.stamp(b"{}").unwrap(); | ||
|
|
||
| assert_eq!(header.name, "X-Stamp"); | ||
| } |
There was a problem hiding this comment.
Why put these in a separate test file?
r-n-o
left a comment
r-n-o
left a comment
There was a problem hiding this comment.
I'm not sure I follow the intent of this PR. Is the idea that we want to support popping open a browser / create a new org from the CLI? Or do we want to support true passkey auth where actions are authorized by a CLI-initiated passkey prompt? (and the user would then have to tap their yubikey or use touchID to allow activities like "CREATE_DEPLOYMENT" to be signed)
If we're going for easier onboarding I believe we should work with the dashboard APIs and make them more CLI friendly so that a passkey isn't required to create an org, potentially? Might be a medium/large project I'm not sure. And if we want to work on logging into the CLI for the first time (with an existing org), I think the challenge will be to prompt users with the right passkey. E.g. if a user creates their org with a passkey in Chrome vs. system passkey vs. 1password passkey we'll run into a serious lack of context if we try to pull the same passkey. Another challenge is the fact that we have non-resident keys; so you need to do email verification first, and then you can pull a list of credential IDs. Without the credential IDs from the server you can't prompt the user for a passkey signature.
3 participants
TODO