tfa: don't automatically drop empty recovery

This should only ever be explicitly removed.

Similarly, include an empty array of recovery keys in the
tfa challenge, so that clients know about empty recoveries
rather than getting an empty challenge when there are no
other factors available.

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>

diff --git a/proxmox-tfa/src/api/methods.rs b/proxmox-tfa/src/api/methods.rs
index 501e7030bfccf79075b437f7bf25e4430d0ade9b..ebb370929a49ea68dca8fe952e2bf08cc750847b 100644 (file)
--- a/proxmox-tfa/src/api/methods.rs
+++ b/proxmox-tfa/src/api/methods.rs
@@ -52,9 +52,9 @@ fn to_data(data: &TfaUserData) -> Vec<TypedTfaInfo> {
             + data.u2f.len()
             + data.webauthn.len()
             + data.yubico.len()
-            + if data.recovery().is_some() { 1 } else { 0 },
+            + if data.recovery.is_some() { 1 } else { 0 },
     );
-    if let Some(recovery) = data.recovery() {
+    if let Some(recovery) = &data.recovery {
         out.push(TypedTfaInfo {
             ty: TfaType::Recovery,
             info: TfaInfo::recovery(recovery.created),
@@ -145,7 +145,7 @@ pub fn get_tfa_entry(config: &TfaConfig, userid: &str, id: &str) -> Option<Typed
             let entry = tfa_id_iter(user_data).find(|(_ty, _index, entry_id)| id == *entry_id);
             entry.map(|(ty, index, _)| (ty, index))
         } {
-            Some((TfaType::Recovery, _)) => match user_data.recovery() {
+            Some((TfaType::Recovery, _)) => match &user_data.recovery {
                 Some(recovery) => TypedTfaInfo {
                     ty: TfaType::Recovery,
                     info: TfaInfo::recovery(recovery.created),

diff --git a/proxmox-tfa/src/api/mod.rs b/proxmox-tfa/src/api/mod.rs
index 5bde7e40eec4070c40453186159f03b07639b229..6800eab15b21cead42531cf7eeb3b21732447893 100644 (file)
--- a/proxmox-tfa/src/api/mod.rs
+++ b/proxmox-tfa/src/api/mod.rs
@@ -320,7 +320,7 @@ pub struct TfaUserData {
     pub webauthn: Vec<TfaEntry<WebauthnCredential>>,
 
     /// Recovery keys. (Unordered OTP values).
-    #[serde(skip_serializing_if = "Recovery::option_is_empty", default)]
+    #[serde(skip_serializing_if = "Option::is_none", default)]
     pub recovery: Option<Recovery>,
 
     /// Yubico keys for a user. NOTE: This is not directly supported currently, we just need this
@@ -330,22 +330,13 @@ pub struct TfaUserData {
 }
 
 impl TfaUserData {
-    /// Shortcut to get the recovery entry only if it is not empty!
-    pub fn recovery(&self) -> Option<&Recovery> {
-        if Recovery::option_is_empty(&self.recovery) {
-            None
-        } else {
-            self.recovery.as_ref()
-        }
-    }
-
     /// `true` if no second factors exist
     pub fn is_empty(&self) -> bool {
         self.totp.is_empty()
             && self.u2f.is_empty()
             && self.webauthn.is_empty()
             && self.yubico.is_empty()
-            && self.recovery().is_none()
+            && self.recovery.is_none()
     }
 
     /// Find an entry by id, except for the "recovery" entry which we're currently treating
@@ -577,7 +568,7 @@ impl TfaUserData {
 
         Ok(Some(TfaChallenge {
             totp: self.totp.iter().any(|e| e.info.enable),
-            recovery: RecoveryState::from(&self.recovery),
+            recovery: self.recovery_state(),
             webauthn: match webauthn {
                 Some(webauthn) => self.webauthn_challenge(access, userid, webauthn?)?,
                 None => None,
@@ -591,8 +582,8 @@ impl TfaUserData {
     }
 
     /// Get the recovery state.
-    pub fn recovery_state(&self) -> RecoveryState {
-        RecoveryState::from(&self.recovery)
+    pub fn recovery_state(&self) -> Option<RecoveryState> {
+        self.recovery.as_ref().map(RecoveryState::from)
     }
 
     /// Generate an optional webauthn challenge.
@@ -855,8 +846,8 @@ pub struct TfaChallenge {
     pub totp: bool,
 
     /// Whether there are recovery keys available.
-    #[serde(skip_serializing_if = "RecoveryState::is_unavailable", default)]
-    pub recovery: RecoveryState,
+    #[serde(skip_serializing_if = "Option::is_none", default)]
+    pub recovery: Option<RecoveryState>,
 
     /// If the user has any u2f tokens registered, this will contain the U2F challenge data.
     #[serde(skip_serializing_if = "Option::is_none")]

diff --git a/proxmox-tfa/src/api/recovery.rs b/proxmox-tfa/src/api/recovery.rs
index 92c0e9dc2f470d7a28e055d170e28c69a4e0cb49..970770b626ffef19b3dcfd907dbdf0e6edc976f5 100644 (file)
--- a/proxmox-tfa/src/api/recovery.rs
+++ b/proxmox-tfa/src/api/recovery.rs
@@ -96,12 +96,6 @@ impl Recovery {
         self.available().count()
     }
 
-    /// Convenience serde method to check if either the option is `None` or the content `is_empty`.
-    pub(super) fn option_is_empty(this: &Option<Self>) -> bool {
-        this.as_ref()
-            .map_or(true, |this| this.count_available() == 0)
-    }
-
     /// Verify a key and remove it. Returns whether the key was valid. Errors on openssl errors.
     pub(super) fn verify(&mut self, key: &str) -> Result<bool, Error> {
         let hash = self.hash(key.as_bytes())?;
@@ -131,15 +125,6 @@ impl RecoveryState {
     }
 }
 
-impl From<&Option<Recovery>> for RecoveryState {
-    fn from(r: &Option<Recovery>) -> Self {
-        match r {
-            Some(r) => Self::from(r),
-            None => Self::default(),
-        }
-    }
-}
-
 impl From<&Recovery> for RecoveryState {
     fn from(r: &Recovery) -> Self {
         Self(