diff --git a/proto/src/internal/credupdate.rs b/proto/src/internal/credupdate.rs index 7a0201bc5..5a7fa66d6 100644 --- a/proto/src/internal/credupdate.rs +++ b/proto/src/internal/credupdate.rs @@ -160,6 +160,7 @@ pub enum CURegWarning { AttestedResidentKeyRequired, Unsatisfiable, WebauthnAttestationUnsatisfiable, + WebauthnUserVerificationRequired, } #[derive(Debug, Clone, Serialize, Deserialize, ToSchema)] diff --git a/server/core/src/https/views/reset.rs b/server/core/src/https/views/reset.rs index 2caa70cbe..1d00e206b 100644 --- a/server/core/src/https/views/reset.rs +++ b/server/core/src/https/views/reset.rs @@ -73,6 +73,7 @@ struct CredStatusView { #[template(path = "credentials_update_partial.html")] struct CredResetPartialView { ext_cred_portal: CUExtPortal, + can_commit: bool, warnings: Vec, attested_passkeys_state: CUCredState, passkeys_state: CUCredState, @@ -899,6 +900,7 @@ pub(crate) async fn view_reset_get( fn get_cu_partial(cu_status: CUStatus) -> CredResetPartialView { let CUStatus { ext_cred_portal, + can_commit, warnings, passkeys_state, attested_passkeys_state, @@ -913,6 +915,7 @@ fn get_cu_partial(cu_status: CUStatus) -> CredResetPartialView { CredResetPartialView { ext_cred_portal, + can_commit, warnings, attested_passkeys_state, passkeys_state, diff --git a/server/core/templates/credentials_update_partial.html b/server/core/templates/credentials_update_partial.html index 5e62ff4a4..277f47583 100644 --- a/server/core/templates/credentials_update_partial.html +++ b/server/core/templates/credentials_update_partial.html @@ -49,6 +49,9 @@ (% when CURegWarning::Unsatisfiable %) An account policy conflict has occurred and you will not be able to save your credentials. + (% when CURegWarning::WebauthnUserVerificationRequired %) + The passkey you attempted to register did not provide user verification. Please + ensure that you have a PIN or alternative configured on your authenticator. (% endmatch %) (% if is_danger %) @@ -158,7 +161,7 @@ type="submit" hx-post="/ui/api/cu_commit" hx-boost="false" - (% if !warnings.is_empty() %)disabled(% endif + (% if !can_commit %)disabled(% endif %)>Save Changes diff --git a/server/lib/src/idm/credupdatesession.rs b/server/lib/src/idm/credupdatesession.rs index 6e949d207..227f438d5 100644 --- a/server/lib/src/idm/credupdatesession.rs +++ b/server/lib/src/idm/credupdatesession.rs @@ -203,6 +203,7 @@ impl CredentialUpdateSession { // Vec of the issues with the current session so that UI's can highlight properly how to proceed. fn can_commit(&self) -> (bool, Vec) { let mut warnings = Vec::with_capacity(0); + let mut can_proceed = true; let cred_type_min = self.resolved_account_policy.credential_policy(); @@ -219,6 +220,7 @@ impl CredentialUpdateSession { // parts. .unwrap_or(false) { + can_proceed = false; warnings.push(CredentialUpdateSessionStatusWarnings::MfaRequired); } } @@ -226,12 +228,14 @@ impl CredentialUpdateSession { // NOTE: Technically this is unreachable, but we keep it for correctness. // Primary can't be set at all. if self.primary.is_some() { + can_proceed = false; warnings.push(CredentialUpdateSessionStatusWarnings::PasskeyRequired); } } CredentialType::AttestedPasskey => { // Also unreachable - during these sessions, there will be no values present here. if !self.passkeys.is_empty() || self.primary.is_some() { + can_proceed = false; warnings.push(CredentialUpdateSessionStatusWarnings::AttestedPasskeyRequired); } } @@ -241,12 +245,14 @@ impl CredentialUpdateSession { || !self.passkeys.is_empty() || self.primary.is_some() { + can_proceed = false; warnings .push(CredentialUpdateSessionStatusWarnings::AttestedResidentKeyRequired); } } CredentialType::Invalid => { // special case, must always deny all changes. + can_proceed = false; warnings.push(CredentialUpdateSessionStatusWarnings::Unsatisfiable) } } @@ -258,7 +264,7 @@ impl CredentialUpdateSession { } } - (warnings.is_empty(), warnings) + (can_proceed, warnings) } } @@ -296,6 +302,7 @@ pub enum CredentialUpdateSessionStatusWarnings { AttestedResidentKeyRequired, Unsatisfiable, WebauthnAttestationUnsatisfiable, + WebauthnUserVerificationRequired, } impl Display for CredentialUpdateSessionStatusWarnings { @@ -319,6 +326,9 @@ impl From for CURegWarning { CredentialUpdateSessionStatusWarnings::WebauthnAttestationUnsatisfiable => { CURegWarning::WebauthnAttestationUnsatisfiable } + CredentialUpdateSessionStatusWarnings::WebauthnUserVerificationRequired => { + CURegWarning::WebauthnUserVerificationRequired + } } } } @@ -350,6 +360,13 @@ pub struct CredentialUpdateSessionStatus { } impl CredentialUpdateSessionStatus { + /// Append a single warning to this session status, which will only be displayed to the + /// user once. This is different to other warnings that are derived from the state of the + /// session as a whole. + pub fn append_ephemeral_warning(&mut self, warning: CredentialUpdateSessionStatusWarnings) { + self.warnings.push(warning) + } + pub fn can_commit(&self) -> bool { self.can_commit } @@ -2172,30 +2189,36 @@ impl IdmServerCredUpdateTransaction<'_> { match &session.mfaregstate { MfaRegState::Passkey(_ccr, pk_reg) => { - let result = self - .webauthn - .finish_passkey_registration(reg, pk_reg) - .map_err(|e| { - error!(eclass=?e, emsg=%e, "Unable to complete passkey registration"); - match e { - WebauthnError::UserNotVerified => { - OperationError::CU0003WebauthnUserNotVerified - } - _ => OperationError::CU0002WebauthnRegistrationError, - } - }); + let reg_result = self.webauthn.finish_passkey_registration(reg, pk_reg); - // The reg is done. Clean up state before returning errors. + // Clean up state before returning results. session.mfaregstate = MfaRegState::None; - let passkey = result?; + match reg_result { + Ok(passkey) => { + let pk_id = Uuid::new_v4(); + session.passkeys.insert(pk_id, (label, passkey)); - let pk_id = Uuid::new_v4(); - session.passkeys.insert(pk_id, (label, passkey)); - - Ok(session.deref().into()) + let cu_status: CredentialUpdateSessionStatus = session.deref().into(); + Ok(cu_status) + } + Err(WebauthnError::UserNotVerified) => { + let mut cu_status: CredentialUpdateSessionStatus = session.deref().into(); + cu_status.append_ephemeral_warning( + CredentialUpdateSessionStatusWarnings::WebauthnUserVerificationRequired, + ); + Ok(cu_status) + } + Err(err) => { + error!(eclass=?err, emsg=%err, "Unable to complete passkey registration"); + Err(OperationError::CU0002WebauthnRegistrationError) + } + } + } + invalid_state => { + warn!(?invalid_state); + Err(OperationError::InvalidRequestState) } - _ => Err(OperationError::InvalidRequestState), } } diff --git a/tools/cli/src/cli/lib.rs b/tools/cli/src/cli/lib.rs index 9c2721b4c..d1dc531b1 100644 --- a/tools/cli/src/cli/lib.rs +++ b/tools/cli/src/cli/lib.rs @@ -74,7 +74,9 @@ pub(crate) fn handle_group_account_policy_error(response: ClientError, _output_m use kanidm_proto::internal::OperationError::SchemaViolation; use kanidm_proto::internal::SchemaError::AttributeNotValidForClass; - if let ClientError::Http(_status, Some(SchemaViolation(AttributeNotValidForClass(att))), opid) = response { + if let ClientError::Http(_status, Some(SchemaViolation(AttributeNotValidForClass(att))), opid) = + response + { error!("OperationId: {:?}", opid); error!("Cannot update account-policy attribute {att}. Is account-policy enabled on this group?"); } else { diff --git a/tools/cli/src/cli/person.rs b/tools/cli/src/cli/person.rs index 2469fb85b..1d0a44f8d 100644 --- a/tools/cli/src/cli/person.rs +++ b/tools/cli/src/cli/person.rs @@ -1282,6 +1282,11 @@ fn display_warnings(warnings: &[CURegWarning]) { CURegWarning::Unsatisfiable => { println!("Account policy is unsatisfiable. Contact your administrator."); } + CURegWarning::WebauthnUserVerificationRequired => { + println!( + "The passkey you attempted to register did not provide user verification, please ensure a PIN or equivalent is set." + ); + } } } }