From ad3c491d07de986747a51f0ac18f9842200a7934 Mon Sep 17 00:00:00 2001 From: James Hodgkinson Date: Fri, 27 Oct 2023 15:30:38 +1000 Subject: [PATCH] Bug chasing (#2257) * service-account validity expire-at doesn't accept all time nouns as defined by docs Fixes #2153 * realised a logic bug * making clippy happy while I'm here * returning an empty set from the creds if the creds attribute is not found, which is then handled downstream --- server/core/src/https/v1.rs | 32 ++++++++-- tools/cli/src/cli/common.rs | 36 +++++++++++ tools/cli/src/cli/person.rs | 97 ++++++++--------------------- tools/cli/src/cli/serviceaccount.rs | 55 ++++++++-------- 4 files changed, 114 insertions(+), 106 deletions(-) diff --git a/server/core/src/https/v1.rs b/server/core/src/https/v1.rs index 9ab51a159..6f2a31846 100644 --- a/server/core/src/https/v1.rs +++ b/server/core/src/https/v1.rs @@ -1336,12 +1336,22 @@ pub async fn service_account_id_credential_status_get( Extension(kopid): Extension, Path(id): Path, ) -> Result, WebError> { - state + match state .qe_r_ref - .handle_idmcredentialstatus(kopid.uat, id, kopid.eventid) + .handle_idmcredentialstatus(kopid.uat, id.clone(), kopid.eventid) .await .map(Json::from) - .map_err(WebError::from) + { + Ok(val) => Ok(val), + Err(err) => { + if let OperationError::NoMatchingAttributes = err { + debug!("No credentials set on account {}, returning empty list", id); + Ok(Json(CredentialStatus { creds: Vec::new() })) + } else { + Err(WebError::from(err)) + } + } + } } #[utoipa::path( @@ -1362,12 +1372,22 @@ pub async fn person_get_id_credential_status( Extension(kopid): Extension, Path(id): Path, ) -> Result, WebError> { - state + match state .qe_r_ref - .handle_idmcredentialstatus(kopid.uat, id, kopid.eventid) + .handle_idmcredentialstatus(kopid.uat, id.clone(), kopid.eventid) .await .map(Json::from) - .map_err(WebError::from) + { + Ok(val) => Ok(val), + Err(err) => { + if let OperationError::NoMatchingAttributes = err { + debug!("No credentials set on person {}, returning empty list", id); + Ok(Json(CredentialStatus { creds: Vec::new() })) + } else { + Err(WebError::from(err)) + } + } + } } #[utoipa::path( diff --git a/tools/cli/src/cli/common.rs b/tools/cli/src/cli/common.rs index eee52bd26..b635ea0c3 100644 --- a/tools/cli/src/cli/common.rs +++ b/tools/cli/src/cli/common.rs @@ -8,6 +8,8 @@ use dialoguer::{Confirm, Select}; use kanidm_client::{KanidmClient, KanidmClientBuilder}; use kanidm_proto::constants::{DEFAULT_CLIENT_CONFIG_PATH, DEFAULT_CLIENT_CONFIG_PATH_HOME}; use kanidm_proto::v1::UserAuthToken; +use time::format_description::well_known::Rfc3339; +use time::OffsetDateTime; use crate::session::read_tokens; use crate::{CommonOpt, LoginOpt, ReauthOpt}; @@ -375,3 +377,37 @@ pub fn prompt_for_username_get_token() -> Result { } } */ + +/// This parses the input for the person/service-account expire-at CLI commands +/// +/// If it fails, return error, if it needs to *clear* the result, return Ok(None), +/// otherwise return Ok(Some(String)) which is the new value to set. +pub(crate) fn try_expire_at_from_string(input: &str) -> Result, ()> { + match input { + "any" | "never" | "clear" => Ok(None), + "now" => match OffsetDateTime::now_utc().format(&Rfc3339) { + Ok(s) => Ok(Some(s)), + Err(e) => { + error!(err = ?e, "Unable to format current time to rfc3339"); + Err(()) + } + }, + "epoch" => match OffsetDateTime::UNIX_EPOCH.format(&Rfc3339) { + Ok(val) => Ok(Some(val)), + Err(err) => { + error!("Failed to format epoch timestamp as RFC3339: {:?}", err); + Err(()) + } + }, + _ => { + // fall back to parsing it as a date + match OffsetDateTime::parse(input, &Rfc3339) { + Ok(_) => Ok(Some(input.to_string())), + Err(err) => { + error!("Failed to parse supplied timestamp: {:?}", err); + Err(()) + } + } + } + } +} diff --git a/tools/cli/src/cli/person.rs b/tools/cli/src/cli/person.rs index 7d29fcd6d..3436719fb 100644 --- a/tools/cli/src/cli/person.rs +++ b/tools/cli/src/cli/person.rs @@ -1,4 +1,4 @@ -use crate::common::OpType; +use crate::common::{try_expire_at_from_string, OpType}; use std::fmt::{self, Debug}; use std::str::FromStr; @@ -410,78 +410,33 @@ impl PersonOpt { } AccountValidity::ExpireAt(ano) => { let client = ano.copt.to_client(OpType::Write).await; - if matches!(ano.datetime.as_str(), "never" | "clear") { - // Unset the value - match client - .idm_person_account_purge_attr( - ano.aopts.account_id.as_str(), - ATTR_ACCOUNT_EXPIRE, - ) - .await - { - Err(e) => handle_client_error(e, &ano.copt.output_mode), - _ => println!("Success"), + let validity = match try_expire_at_from_string(ano.datetime.as_str()) { + Ok(val) => val, + Err(()) => return, + }; + let res = match validity { + None => { + client + .idm_person_account_purge_attr( + ano.aopts.account_id.as_str(), + ATTR_ACCOUNT_EXPIRE, + ) + .await } - } else if matches!(ano.datetime.as_str(), "now") { - // set the expiry to *now* - let now = match OffsetDateTime::now_utc().format(&Rfc3339) { - Ok(s) => s, - Err(e) => { - error!(err = ?e, "Unable to format current time to rfc3339"); - return; - } - }; - debug!("Setting expiry to {}", now); - match client - .idm_person_account_set_attr( - ano.aopts.account_id.as_str(), - ATTR_ACCOUNT_EXPIRE, - &[&now], - ) - .await - { - Err(e) => error!("Error setting expiry to 'now' -> {:?}", e), - _ => println!("Success"), + Some(new_expiry) => { + client + .idm_person_account_set_attr( + ano.aopts.account_id.as_str(), + ATTR_ACCOUNT_EXPIRE, + &[&new_expiry], + ) + .await } - } else if matches!(ano.datetime.as_str(), "epoch") { - // set the expiry to the epoch - let epoch_str = match OffsetDateTime::UNIX_EPOCH.format(&Rfc3339) { - Ok(s) => s, - Err(e) => { - error!(err = ?e, "Unable to format unix epoch to rfc3339"); - return; - } - }; - debug!("Setting expiry to {}", epoch_str); - match client - .idm_person_account_set_attr( - ano.aopts.account_id.as_str(), - ATTR_ACCOUNT_EXPIRE, - &[&epoch_str], - ) - .await - { - Err(e) => error!("Error setting expiry to 'epoch' -> {:?}", e), - _ => println!("Success"), - } - } else { - if let Err(e) = OffsetDateTime::parse(ano.datetime.as_str(), &Rfc3339) { - error!("Error -> {:?}", e); - return; - } - - match client - .idm_person_account_set_attr( - ano.aopts.account_id.as_str(), - ATTR_ACCOUNT_EXPIRE, - &[ano.datetime.as_str()], - ) - .await - { - Err(e) => handle_client_error(e, &ano.copt.output_mode), - _ => println!("Success"), - } - } + }; + match res { + Err(e) => handle_client_error(e, &ano.copt.output_mode), + _ => println!("Success"), + }; } AccountValidity::BeginFrom(ano) => { let client = ano.copt.to_client(OpType::Write).await; diff --git a/tools/cli/src/cli/serviceaccount.rs b/tools/cli/src/cli/serviceaccount.rs index a12bc31f8..64f5dea9d 100644 --- a/tools/cli/src/cli/serviceaccount.rs +++ b/tools/cli/src/cli/serviceaccount.rs @@ -1,4 +1,4 @@ -use crate::common::OpType; +use crate::common::{try_expire_at_from_string, OpType}; use kanidm_proto::constants::{ATTR_ACCOUNT_EXPIRE, ATTR_ACCOUNT_VALID_FROM}; use kanidm_proto::messages::{AccountChangeMessage, ConsoleOutputMode, MessageStatus}; use time::OffsetDateTime; @@ -423,36 +423,33 @@ impl ServiceAccountOpt { } AccountValidity::ExpireAt(ano) => { let client = ano.copt.to_client(OpType::Write).await; - if matches!(ano.datetime.as_str(), "never" | "clear") { - // Unset the value - match client - .idm_service_account_purge_attr( - ano.aopts.account_id.as_str(), - ATTR_ACCOUNT_EXPIRE, - ) - .await - { - Err(e) => error!("Error -> {:?}", e), - _ => println!("Success"), + let validity = match try_expire_at_from_string(ano.datetime.as_str()) { + Ok(val) => val, + Err(()) => return, + }; + let res = match validity { + None => { + client + .idm_service_account_purge_attr( + ano.aopts.account_id.as_str(), + ATTR_ACCOUNT_EXPIRE, + ) + .await } - } else { - if let Err(e) = OffsetDateTime::parse(ano.datetime.as_str(), &Rfc3339) { - error!("Error -> {:?}", e); - return; + Some(new_expiry) => { + client + .idm_service_account_set_attr( + ano.aopts.account_id.as_str(), + ATTR_ACCOUNT_EXPIRE, + &[&new_expiry], + ) + .await } - - match client - .idm_service_account_set_attr( - ano.aopts.account_id.as_str(), - ATTR_ACCOUNT_EXPIRE, - &[ano.datetime.as_str()], - ) - .await - { - Err(e) => handle_client_error(e, &ano.copt.output_mode), - _ => println!("Success"), - } - } + }; + match res { + Err(e) => handle_client_error(e, &ano.copt.output_mode), + _ => println!("Success"), + }; } AccountValidity::BeginFrom(ano) => { let client = ano.copt.to_client(OpType::Write).await;