From ea1d023a61cf6f06bc221c6d0f0fc8538c2d0975 Mon Sep 17 00:00:00 2001
From: Wolfgang Bumiller <w.bumiller@proxmox.com>
Date: Tue, 18 Apr 2023 10:39:16 +0200
Subject: [PATCH] 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>
---
 proxmox-tfa/src/api/methods.rs  |  6 +++---
 proxmox-tfa/src/api/mod.rs      | 23 +++++++----------------
 proxmox-tfa/src/api/recovery.rs | 15 ---------------
 3 files changed, 10 insertions(+), 34 deletions(-)

diff --git a/proxmox-tfa/src/api/methods.rs b/proxmox-tfa/src/api/methods.rs
index 501e7030..ebb37092 100644
--- 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 5bde7e40..6800eab1 100644
--- 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 92c0e9dc..970770b6 100644
--- 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(