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

@@ -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),

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")]

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(