From 44b93804e61ad4ed553455bf4a64a978bc2ff290 Mon Sep 17 00:00:00 2001 From: Firstyear Date: Thu, 3 Oct 2024 11:08:47 +1000 Subject: [PATCH] Don't reprompt for login when no session exists in cli (#3082) * Fix handling when session is already removed so that we don't re-prompt for login. * Remove async recursion --- Cargo.lock | 12 ---- Cargo.toml | 1 - tools/cli/Cargo.toml | 1 - tools/cli/src/cli/common.rs | 105 +++++++++++++++++------------------ tools/cli/src/cli/session.rs | 25 +++++++-- 5 files changed, 71 insertions(+), 73 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b56732413..0f4b5376f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -259,17 +259,6 @@ dependencies = [ "tokio", ] -[[package]] -name = "async-recursion" -version = "1.1.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3b43422f69d8ff38f95f1b2bb76517c91589a924d1559a0e935d7c8ce0274c11" -dependencies = [ - "proc-macro2", - "quote", - "syn 2.0.79", -] - [[package]] name = "async-stream" version = "0.3.5" @@ -3322,7 +3311,6 @@ name = "kanidm_tools" version = "1.4.0-dev" dependencies = [ "anyhow", - "async-recursion", "clap", "clap_complete", "compact_jwt 0.4.2", diff --git a/Cargo.toml b/Cargo.toml index 56fb6c356..55eb68e82 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -142,7 +142,6 @@ sketching = { path = "./libs/sketching", version = "=1.4.0-dev" } anyhow = { version = "1.0.89" } argon2 = { version = "0.5.3", features = ["alloc"] } askama = { version = "0.12.1", features = ["serde"] } -async-recursion = "1.1.0" async-trait = "^0.1.83" axum = { version = "0.7.7", features = [ "form", diff --git a/tools/cli/Cargo.toml b/tools/cli/Cargo.toml index 1794db991..c69789108 100644 --- a/tools/cli/Cargo.toml +++ b/tools/cli/Cargo.toml @@ -36,7 +36,6 @@ doctest = false [dependencies] anyhow = { workspace = true } -async-recursion = { workspace = true } clap = { workspace = true, features = ["derive", "env"] } compact_jwt = { workspace = true, features = ["openssl"] } dialoguer = { workspace = true } diff --git a/tools/cli/src/cli/common.rs b/tools/cli/src/cli/common.rs index d03e52d16..009a9c07b 100644 --- a/tools/cli/src/cli/common.rs +++ b/tools/cli/src/cli/common.rs @@ -1,6 +1,5 @@ use std::env; -use async_recursion::async_recursion; use compact_jwt::{traits::JwsVerifiable, JwsCompact, JwsEs256Verifier, JwsVerifier, JwtError}; use dialoguer::theme::ColorfulTheme; use dialoguer::{Confirm, Select}; @@ -22,7 +21,7 @@ pub enum OpType { #[derive(Debug)] pub enum ToClientError { NeedLogin(String), - NeedReauth(String), + NeedReauth(String, KanidmClient), Other, } @@ -98,7 +97,10 @@ impl CommonOpt { }) } - async fn try_to_client(&self, optype: OpType) -> Result { + pub(crate) async fn try_to_client( + &self, + optype: OpType, + ) -> Result { let client = self.to_unauth_client(); // Read the token file. @@ -247,6 +249,9 @@ impl CommonOpt { } } + // It's probably valid, set into the client + client.set_token(jwsc.to_string()).await; + // Check what we are doing based on op. match optype { OpType::Read => {} @@ -256,7 +261,7 @@ impl CommonOpt { "Privileges have expired for {} - you need to re-authenticate again.", uat.spn ); - return Err(ToClientError::NeedReauth(spn)); + return Err(ToClientError::NeedReauth(spn, client)); } } } @@ -268,63 +273,55 @@ impl CommonOpt { } }; - // Set it into the client - client.set_token(jwsc.to_string()).await; - Ok(client) } - #[async_recursion] pub async fn to_client(&self, optype: OpType) -> KanidmClient { - match self.try_to_client(optype.clone()).await { - Ok(c) => c, - Err(e) => { - match e { - ToClientError::NeedLogin(username) => { - if !Confirm::new() - .with_prompt("Would you like to login again?") - .default(true) - .interact() - .expect("Failed to interact with interactive session") - { - std::process::exit(1); - } - let mut copt = self.clone(); - copt.username = Some(username); - let login_opt = LoginOpt { - copt, - password: env::var("KANIDM_PASSWORD").ok(), - }; - login_opt.exec().await; - // we still use `to_client` instead of `try_to_client` because we may need to prompt user to re-auth again. - // since reauth_opt will call `to_client`, this function is recursive anyway. - // we use copt since it's username is updated. - return login_opt.copt.to_client(optype).await; - } - ToClientError::NeedReauth(username) => { - if !Confirm::new() - .with_prompt("Would you like to re-authenticate?") - .default(true) - .interact() - .expect("Failed to interact with interactive session") - { - std::process::exit(1); - } - let mut copt = self.clone(); - copt.username = Some(username); - let reauth_opt = ReauthOpt { copt }; - // calls `to_client` recursively - // but should not goes into `NeedLogin` branch again - reauth_opt.exec().await; - if let Ok(c) = reauth_opt.copt.try_to_client(optype).await { - return c; - } - } - ToClientError::Other => { + let mut copt_mut = self.clone(); + loop { + match self.try_to_client(optype.clone()).await { + Ok(c) => break c, + Err(ToClientError::NeedLogin(username)) => { + if !Confirm::new() + .with_prompt("Would you like to login again?") + .default(true) + .interact() + .expect("Failed to interact with interactive session") + { std::process::exit(1); } + + copt_mut.username = Some(username); + let copt = copt_mut.clone(); + let login_opt = LoginOpt { + copt, + password: env::var("KANIDM_PASSWORD").ok(), + }; + + login_opt.exec().await; + // Okay, try again ... + continue; + } + Err(ToClientError::NeedReauth(username, client)) => { + if !Confirm::new() + .with_prompt("Would you like to re-authenticate?") + .default(true) + .interact() + .expect("Failed to interact with interactive session") + { + std::process::exit(1); + } + copt_mut.username = Some(username); + let copt = copt_mut.clone(); + let reauth_opt = ReauthOpt { copt }; + reauth_opt.inner(client).await; + + // Okay, re-auth should have passed, lets loop + continue; + } + Err(ToClientError::Other) => { + std::process::exit(1); } - std::process::exit(1); } } } diff --git a/tools/cli/src/cli/session.rs b/tools/cli/src/cli/session.rs index edf0627e0..45df46ac4 100644 --- a/tools/cli/src/cli/session.rs +++ b/tools/cli/src/cli/session.rs @@ -1,4 +1,4 @@ -use crate::common::OpType; +use crate::common::{OpType, ToClientError}; use std::cmp::Reverse; use std::collections::BTreeMap; use std::fs::{create_dir, File}; @@ -569,9 +569,7 @@ impl ReauthOpt { self.copt.debug } - pub async fn exec(&self) { - let client = self.copt.to_client(OpType::Read).await; - + pub(crate) async fn inner(&self, client: KanidmClient) { let instance_name = &self.copt.instance; let allowed = client.reauth_begin().await.unwrap_or_else(|e| { @@ -581,6 +579,12 @@ impl ReauthOpt { process_auth_state(allowed, client, &None, instance_name).await; } + + pub async fn exec(&self) { + let client = self.copt.to_client(OpType::Read).await; + // This is to break a recursion loop in re-auth with to_client + self.inner(client).await + } } impl LogoutOpt { @@ -626,7 +630,18 @@ impl LogoutOpt { } } } else { - let client = self.copt.to_client(OpType::Read).await; + let client = match self.copt.try_to_client(OpType::Read).await { + Ok(c) => c, + Err(ToClientError::NeedLogin(_)) => { + // There are no session tokens, so return a success. + std::process::exit(0); + } + Err(ToClientError::NeedReauth(_, _)) | Err(ToClientError::Other) => { + // This can only occur in bad cases, so fail. + std::process::exit(1); + } + }; + let token = match client.get_token().await { Some(t) => t, None => {