TOTP label verification (#3419)

* Adding TOTP Label verification (for both empty and duplicate)

proto/src/internal/credupdate.rs

@@ -130,6 +130,7 @@ pub enum CURegState {
     None,
     TotpCheck(TotpSecret),
     TotpTryAgain,
+    TotpNameTryAgain(String),
     TotpInvalidSha1,
     BackupCodes(Vec<String>),
     Passkey(CreationChallengeResponse),

server/core/src/https/v1.rs

@@ -1396,6 +1396,7 @@ pub async fn credential_update_update(
             return Err(WebError::InternalServerError(errmsg));
         }
     };
+
     let session_token = match serde_json::from_value(cubody[1].clone()) {
         Ok(val) => val,
         Err(err) => {
@@ -1406,6 +1407,7 @@ pub async fn credential_update_update(
     };
     debug!("session_token: {:?}", session_token);
     debug!("scr: {:?}", scr);
+
     state
         .qe_r_ref
         .handle_idmcredentialupdate(session_token, scr, kopid.eventid)

server/core/src/https/views/reset.rs

@@ -210,6 +210,8 @@ pub(crate) struct TotpInit {
 pub(crate) struct TotpCheck {
     wrong_code: bool,
     broken_app: bool,
+    bad_name: bool,
+    taken_name: Option<String>,
 }
 
 #[derive(Template)]
@@ -599,6 +601,25 @@ pub(crate) async fn add_totp(
     let cu_session_token = get_cu_session(&jar).await?;
 
     let check_totpcode = u32::from_str(&new_totp_form.check_totpcode).unwrap_or_default();
+    let swapped_handler_trigger =
+        HxResponseTrigger::after_swap([HxEvent::new("addTotpSwapped".to_string())]);
+
+    // If the user has not provided a name or added only spaces we exit early
+    if new_totp_form.name.trim().is_empty() {
+        return Ok((
+            swapped_handler_trigger,
+            AddTotpPartial {
+                totp_init: None,
+                totp_name: "".into(),
+                totp_value: new_totp_form.check_totpcode.clone(),
+                check: TotpCheck {
+                    bad_name: true,
+                    ..Default::default()
+                },
+            },
+        )
+            .into_response());
+    }
 
     let cu_status = if new_totp_form.ignore_broken_app {
         // Cope with SHA1 apps because the user has intended to do so, their totp code was already verified
@@ -624,6 +645,10 @@ pub(crate) async fn add_totp(
             wrong_code: true,
             ..Default::default()
         },
+        CURegState::TotpNameTryAgain(val) => TotpCheck {
+            taken_name: Some(val.clone()),
+            ..Default::default()
+        },
         CURegState::TotpInvalidSha1 => TotpCheck {
             broken_app: true,
             ..Default::default()
@@ -646,9 +671,6 @@ pub(crate) async fn add_totp(
         new_totp_form.check_totpcode.clone()
     };
 
-    let swapped_handler_trigger =
-        HxResponseTrigger::after_swap([HxEvent::new("addTotpSwapped".to_string())]);
-
     Ok((
         swapped_handler_trigger,
         AddTotpPartial {

server/core/templates/credential_update_add_totp_partial.html

@@ -19,7 +19,8 @@
             <label for="new-totp-name" class="form-label">Enter a name for your TOTP</label>
             <input
                     aria-describedby="totp-name-validation-feedback"
-                    class="form-control"
+                    class="form-control (%- if let Some(_) = check.taken_name -%)is-invalid(%- endif -%)
+                           (%- if check.bad_name -%)is-invalid(%- endif -%)"
                     name="name"
                     id="new-totp-name"
                     value="(( totp_name ))"
@@ -51,6 +52,18 @@
                         <li>Incorrect TOTP code - Please try again</li>
                     </ul>
                 </div>
+            (% else if check.bad_name %)
+                <div id="neq-totp-validation-feedback">
+                    <ul>
+                        <li>The name you provided was empty or blank. Please provide a proper name</li>
+                    </ul>
+                </div>
+            (% else if let Some(name) = check.taken_name %)
+                <div id="neq-totp-validation-feedback">
+                    <ul>
+                        <li>The name "((name))" is either invalid or already taken, Please pick a different one</li>
+                    </ul>
+                </div>
             (% endif %)
 
         </form>

server/lib/src/credential/mod.rs

@@ -702,6 +702,13 @@ impl Credential {
         }
     }
 
+    pub(crate) fn has_totp_by_name(&self, label: &str) -> bool {
+        match &self.type_ {
+            CredentialType::PasswordMfa(_, totp, _, _) => totp.contains_key(label),
+            _ => false,
+        }
+    }
+
     pub(crate) fn new_from_generatedpassword(pw: Password) -> Self {
         Credential {
             type_: CredentialType::GeneratedPassword(pw),

server/lib/src/idm/credupdatesession.rs

@@ -86,6 +86,7 @@ enum MfaRegState {
     None,
     TotpInit(Totp),
     TotpTryAgain(Totp),
+    TotpNameTryAgain(Totp, String),
     TotpInvalidSha1(Totp, Totp, String),
     Passkey(Box<CreationChallengeResponse>, PasskeyRegistration),
     #[allow(dead_code)]
@@ -98,6 +99,7 @@ impl fmt::Debug for MfaRegState {
             MfaRegState::None => "MfaRegState::None",
             MfaRegState::TotpInit(_) => "MfaRegState::TotpInit",
             MfaRegState::TotpTryAgain(_) => "MfaRegState::TotpTryAgain",
+            MfaRegState::TotpNameTryAgain(_, _) => "MfaRegState::TotpNameTryAgain",
             MfaRegState::TotpInvalidSha1(_, _, _) => "MfaRegState::TotpInvalidSha1",
             MfaRegState::Passkey(_, _) => "MfaRegState::Passkey",
             MfaRegState::AttestedPasskey(_, _) => "MfaRegState::AttestedPasskey",
@@ -273,6 +275,7 @@ pub enum MfaRegStateStatus {
     None,
     TotpCheck(TotpSecret),
     TotpTryAgain,
+    TotpNameTryAgain(String),
     TotpInvalidSha1,
     BackupCodes(HashSet<String>),
     Passkey(CreationChallengeResponse),
@@ -283,8 +286,9 @@ impl fmt::Debug for MfaRegStateStatus {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         let t = match self {
             MfaRegStateStatus::None => "MfaRegStateStatus::None",
-            MfaRegStateStatus::TotpCheck(_) => "MfaRegStateStatus::TotpCheck(_)",
+            MfaRegStateStatus::TotpCheck(_) => "MfaRegStateStatus::TotpCheck",
             MfaRegStateStatus::TotpTryAgain => "MfaRegStateStatus::TotpTryAgain",
+            MfaRegStateStatus::TotpNameTryAgain(_) => "MfaRegStateStatus::TotpNameTryAgain",
             MfaRegStateStatus::TotpInvalidSha1 => "MfaRegStateStatus::TotpInvalidSha1",
             MfaRegStateStatus::BackupCodes(_) => "MfaRegStateStatus::BackupCodes",
             MfaRegStateStatus::Passkey(_) => "MfaRegStateStatus::Passkey",
@@ -389,6 +393,7 @@ impl Into<CUStatus> for CredentialUpdateSessionStatus {
                 MfaRegStateStatus::None => CURegState::None,
                 MfaRegStateStatus::TotpCheck(c) => CURegState::TotpCheck(c),
                 MfaRegStateStatus::TotpTryAgain => CURegState::TotpTryAgain,
+                MfaRegStateStatus::TotpNameTryAgain(label) => CURegState::TotpNameTryAgain(label),
                 MfaRegStateStatus::TotpInvalidSha1 => CURegState::TotpInvalidSha1,
                 MfaRegStateStatus::BackupCodes(s) => {
                     CURegState::BackupCodes(s.into_iter().collect())
@@ -469,6 +474,9 @@ impl From<&CredentialUpdateSession> for CredentialUpdateSessionStatus {
                 MfaRegState::TotpInit(token) => MfaRegStateStatus::TotpCheck(
                     token.to_proto(session.account.name.as_str(), session.issuer.as_str()),
                 ),
+                MfaRegState::TotpNameTryAgain(_, name) => {
+                    MfaRegStateStatus::TotpNameTryAgain(name.clone())
+                }
                 MfaRegState::TotpTryAgain(_) => MfaRegStateStatus::TotpTryAgain,
                 MfaRegState::TotpInvalidSha1(_, _, _) => MfaRegStateStatus::TotpInvalidSha1,
                 MfaRegState::Passkey(r, _) => MfaRegStateStatus::Passkey(r.as_ref().clone()),
@@ -1899,7 +1907,22 @@ impl IdmServerCredUpdateTransaction<'_> {
         match &session.mfaregstate {
             MfaRegState::TotpInit(totp_token)
             | MfaRegState::TotpTryAgain(totp_token)
+            | MfaRegState::TotpNameTryAgain(totp_token, _)
             | MfaRegState::TotpInvalidSha1(totp_token, _, _) => {
+                if session
+                    .primary
+                    .as_ref()
+                    .map(|cred| cred.has_totp_by_name(label))
+                    .unwrap_or_default()
+                    || label.trim().is_empty()
+                    || !Value::validate_str_escapes(label)
+                {
+                    // The user is trying to add a second TOTP under the same name. Lets save them from themselves
+                    session.mfaregstate =
+                        MfaRegState::TotpNameTryAgain(totp_token.clone(), label.into());
+                    return Ok(session.deref().into());
+                }
+
                 if totp_token.verify(totp_chal, ct) {
                     // It was valid. Update the credential.
                     let ncred = session
@@ -3368,10 +3391,39 @@ mod tests {
             .credential_primary_check_totp(&cust, ct, chal + 1, "totp")
             .expect("Failed to update the primary cred totp");
 
-        assert!(matches!(
-            c_status.mfaregstate,
-            MfaRegStateStatus::TotpTryAgain
-        ));
+        assert!(
+            matches!(c_status.mfaregstate, MfaRegStateStatus::TotpTryAgain),
+            "{:?}",
+            c_status.mfaregstate
+        );
+
+        // Check that the user actually put something into the label
+        let c_status = cutxn
+            .credential_primary_check_totp(&cust, ct, chal, "")
+            .expect("Failed to update the primary cred totp");
+
+        assert!(
+            matches!(
+                c_status.mfaregstate,
+                MfaRegStateStatus::TotpNameTryAgain(ref val) if val == ""
+            ),
+            "{:?}",
+            c_status.mfaregstate
+        );
+
+        // Okay, Now they are trying to be smart...
+        let c_status = cutxn
+            .credential_primary_check_totp(&cust, ct, chal, "           ")
+            .expect("Failed to update the primary cred totp");
+
+        assert!(
+            matches!(
+                c_status.mfaregstate,
+                MfaRegStateStatus::TotpNameTryAgain(ref val) if val == "           "
+            ),
+            "{:?}",
+            c_status.mfaregstate
+        );
 
         let c_status = cutxn
             .credential_primary_check_totp(&cust, ct, chal, "totp")
@@ -3383,6 +3435,40 @@ mod tests {
             _ => false,
         });
 
+        {
+            let c_status = cutxn
+                .credential_primary_init_totp(&cust, ct)
+                .expect("Failed to update the primary cred password");
+
+            // Check the status has the token.
+            let totp_token: Totp = match c_status.mfaregstate {
+                MfaRegStateStatus::TotpCheck(secret) => Some(secret.try_into().unwrap()),
+                _ => None,
+            }
+            .expect("Unable to retrieve totp token, invalid state.");
+
+            trace!(?totp_token);
+            let chal = totp_token
+                .do_totp_duration_from_epoch(&ct)
+                .expect("Failed to perform totp step");
+
+            // They tried to add a second totp under the same name
+            let c_status = cutxn
+                .credential_primary_check_totp(&cust, ct, chal, "totp")
+                .expect("Failed to update the primary cred totp");
+
+            assert!(
+                matches!(
+                    c_status.mfaregstate,
+                    MfaRegStateStatus::TotpNameTryAgain(ref val) if val == "totp"
+                ),
+                "{:?}",
+                c_status.mfaregstate
+            );
+
+            assert!(cutxn.credential_update_cancel_mfareg(&cust, ct).is_ok())
+        }
+
         // Should be okay now!
 
         drop(cutxn);

tools/cli/src/cli/person.rs

@@ -831,6 +831,13 @@ async fn totp_enroll_prompt(session_token: &CUSessionToken, client: &KanidmClien
 
     let label: String = Input::new()
         .with_prompt("TOTP Label")
+        .validate_with(|input: &String| -> Result<(), &str> {
+            if input.trim().is_empty() {
+                Err("Label cannot be empty")
+            } else {
+                Ok(())
+            }
+        })
         .interact_text()
         .expect("Failed to interact with interactive session");
 
@@ -919,6 +926,13 @@ async fn totp_enroll_prompt(session_token: &CUSessionToken, client: &KanidmClien
                 eprintln!("Incorrect TOTP code entered. Please try again.");
                 continue;
             }
+            Ok(CUStatus {
+                mfaregstate: CURegState::TotpNameTryAgain(label),
+                ..
+            }) => {
+                eprintln!("{label} is either invalid or already taken. Please try again.");
+                continue;
+            }
             Ok(CUStatus {
                 mfaregstate: CURegState::TotpInvalidSha1,
                 ..