Merge ee46216093 into 848af4cecd

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

book/src/SUMMARY.md

@@ -85,8 +85,11 @@
   - [Cryptography Key Domains (2024)](developers/designs/cryptography_key_domains.md)
   - [Domain Join - Machine Accounts](developers/designs/domain_join_machine_accounts.md)
   - [Elevated Priv Mode](developers/designs/elevated_priv_mode.md)
+  - [Ephemeral Entries](developers/designs/ephemeral_entries.md)
   - [OAuth2 Device Flow](developers/designs/oauth2_device_flow.md)
   - [OAuth2 Refresh Tokens](developers/designs/oauth2_refresh_tokens.md)
+  - [SubEntries (2024)](developers/designs/subentries.md)
+  - [Schema (2024)](developers/designs/schema.md)
   - [Replication Coordinator](developers/designs/replication_coordinator.md)
   - [Replication Design and Notes](developers/designs/replication_design_and_notes.md)
   - [REST Interface](developers/designs/rest_interface.md)

book/src/developers/designs/ephemeral_entries.md

@@ -0,0 +1,24 @@
+# Ephemeral Entries
+
+We have a number of data types and entries that may need to be automatically deleted
+after some time window has past. This could be an event notification, a group for a
+temporary group membership, a session token, or more.
+
+To achieve this we need a way to mark entries as ephemeral. After a set time has past
+the entry will be automatically deleted.
+
+## Class
+
+A new class `EphemeralObject` will be added. It will have a must attribute of `removedAt`
+which will contain a time at which the entry will be deleted.
+
+## Automatic Deletion
+
+A new interval task similar to the recycle/tombstone tasks will be added that checks for
+and deletes ephemeral objects once removedAt has past.
+
+## Ordering Index
+
+To make this effecient we should consider addition of an "ordering" index on the `removedAt`
+attribute to improve searching for these. Initially this won't be needed as there will be
+very few of these, but it should be added in future.

book/src/developers/designs/schema.md

@@ -0,0 +1,47 @@
+# Schema Changes 2024 / 2025
+
+Our current schema structure has served us very well, remaining almost unchanged since nearl 2018.
+
+The current design is a heavily adapted LDAP/AD style structure with classes that define a set
+of may and must attributes, and attributes that define properties like single value, multivalue,
+the types of indexes to apply, and the syntax of the attribute.
+
+However, after 6 years we are starting to finally run into some limits.
+
+## Proposed Changes
+
+### Removal of Multivalue
+
+We currently have many types that have to be multivalue capable out of syntax compliance but are never
+actually made to be multivalue types. This creates overheads in the server but also in how we code
+the valuesets themself.
+
+The multivalue type should be removed. The syntax should imply if the type is single or multivalue.
+For example, bool is always single value. utf8 is single value. utf8strings is multivalue.
+
+This allows consistent handling with SCIM which has separate handling of multi/single value types.
+
+### Indexing
+
+Currently we have a number of indexing flags like equality, substring, presence. In the future we
+would like to add ordering. However, these don't make sense on all types. How do you "order" certificates?
+How do you "substring" an integer? How do you perform equality on two passkeys?
+
+To resolve this schema should indicate a boolean for "indexed" or not based on if the value will be
+queried. The syntax will then imply the class of indexes that will be emitted for the type.
+
+### Migration Behaviour
+
+Certain attributes for internal server migrations need to have their content asserted, merged, or
+ignored. This behaviour should be flagged in the schema to make it more consistent and visible how
+these types will be affected during a migration, and to prevent human error.
+
+### SubAttributes and SubAttribute Syntax
+
+SCIM allows complex structure types to exist. We could consider a schema syntax to allow generic
+structures of these based on a set of limited and restricted SubAttributes. For example we might
+have a SubAttribute of "Mail" and it allows two SubAttributeValues of "value": email, and "primary": bool.
+
+We would need more thought here about this, and it's likely it's own whole separate topic including
+how to handle it with access controls.
+

book/src/developers/designs/subentries.md

@@ -0,0 +1,131 @@
+# Sub-Entries
+
+As Kanidm has grown we have encountered issues with growing complexity of values and valuesets. These
+can be hard to create and add, they touch a lot of the codebase, and they add complexity to new
+features or changes.
+
+These complex valueset types (such as authsession, oauth2session, application passwords) arose out
+of a need to have data associated to an account, but that data required structure and nesting
+of certain components.
+
+Rather than continue to add more complex and unwieldy valuesets, we need a way to create entries
+that refer to others.
+
+## Existing Referential Code
+
+The existing referential integrity code is designed to ensure that values from one entry are removed
+cleanly if the referenced entry is deleted. As an example, a group with a member "ellie" should have
+the reference deleted when the entry "ellie" is deleted.
+
+If the group were deleted, this has no impact on ellie, since the reference is defining a weak
+relationship - the user is a member of a group.
+
+## What Is Required
+
+What we need in a new reference type are the following properties.
+
+* A sub-entry references an owning entry
+* A sub-entry is deleted when the owning entry is deleted (aka recycled)
+* Sub-entries can not exist without a related owning entry
+* Deletion of the sub-entry does not delete the entry
+* When an entry is searched, specific types of sub-entries can be fetched at the same time
+* The owning entry can imply access controls to related sub-entries
+* Conditional creation of sub-entries and adherence to certain rules (such as, "identity X can create sub-entry Y only if the owning entry is itself/X")
+* Subentries may have a minimal / flattened representation that can inline to the owning entry via a phantomAttribute
+
+Properties we can not maintain are
+
+* An entry has a `must` relationship for a sub-entry to exist
+* SubEntries may not have SubEntries
+
+## Example SubEntry
+
+Auth Sessions, OAuth2 Sessions, ApiTokens, Application Passwords, are examples of candidates to become SubEntries.
+
+```
+class: person
+name: ellie
+uuid: A
+
+class: subentry
+class: authsession
+SubEntryOf: A
+sessionStartTime: ...
+sessionEntTime: ...
+sessionId: ...
+```
+
+Good candidates are structured data that are logically indendent from the owning entry and may not
+always need presentation with the owning entry. Displaying a person does not always require it's
+subentries to be displayed.
+
+## Non-Examples
+
+Some attributes should not become subentries, generally things with minimal or small structures
+that benefit from being present on the owning entry for human consumption.
+
+* Mail
+* Address
+* Certificates
+* Passkeys
+
+## AccessControls
+
+Access Controls need to be able to express a relationship between an owner and the subEntry. For
+example we want rules that can express:
+
+* Identity X can create an AuthSession where the AuthSession must reference Identity X
+* `idm_admins` can delete/modify ApiTokens where the owning entries are persons and not members of `idm_high_priv`
+
+We need to extend the `filter` type to support a `SubEntryOfSelf`. This
+is similar to the `SelfUUID` type, but rather than expanding to `Uuid(...)` it would expand to
+`SubEntryOf(...)`. As `create` access controls define that the resultant entry *must* match
+the target filter, this achieves the goal.
+
+We also need a new ACP Target Type. This new target type needs two filters - one
+to express the relationship to the SubEntry, and the other to the relationship of the SubEntryOwner. This
+would constitute two filters
+
+```
+SubEntryTarget: class eq apitokens
+EntryTarget: person and not memberOf idm_high_priv
+```
+
+Both conditions must be met for the access control to apply. In the case of a `create`, the SubEntryTarget
+is used for assertion of the SubEntry adherence to the filter. SubEntryTarget implies "class eq SubEntry". EntryTarget
+implies `and not class eq SubEntry`.
+
+## Search / Access
+
+How to handle where we need to check the entryTarget if we don't have the entry? Do SubEntries need
+to auto-dereference and link to their owning entry for filter application?
+
+If we deref, we need to be careful to avoid ref-count loops, since we would need to embed Arc or Weak
+references into the results.
+
+
+Alternately, is this where we need pre-extraction of access controls?
+
+Could SubEntries only be accessed via their Parent Entry via embedding?
+
+
+
+## Deletion
+
+During a deletion, all deleted entries will also imply the deletion of their SubEntries. These SubEntries
+will be marked with a flag to distinguish them as an indirect delete.
+
+## Reviving
+
+During a revive, a revived entry implies the revival of it's SubEntries that are marked as indirect
+deleted.
+
+## Replication / Consistency
+
+If a SubEntry is created with out an owner, or becomes a orphaned due to a replication conflict of
+it's owning entry, the SubEntries are deleted.
+
+
+
+
+

book/src/integrations/oauth2.md

@@ -70,36 +70,10 @@ anything special for Kanidm (or another provider).
 **Note:** some apps automatically append `/.well-known/openid-configuration` to
 the end of an OIDC Discovery URL, so you may need to omit that.
 
-
-<dl>
-<dt>[Webfinger](https://datatracker.ietf.org/doc/html/rfc7033) URL</dt>
-
-<dd>
-
-`https://idm.example.com/oauth2/openid/:client_id:/.well-known/webfinger`
-
-The webfinger URL is implemented for each OpenID client, under its specific endpoint, giving full control to the administrator regarding which to use.
-
-To make this compliant with the standard, it must be made available under the correct [well-known endpoint](https://datatracker.ietf.org/doc/html/rfc7033#section-10.1) (e.g `example.com/.well-known/webfinger`), typically via a reverse proxy or similar. Kanidm doesn't currently provide a mechanism for this URI rewrite.
-
-One example would be dedicating one client as the "primary" or "default" and redirecting all requests to that. Alternatively, source IP or other request metadata could be used to decide which client to forward the request to.
-
-### Caddy
-`Caddyfile`
-```caddy
-# assuming a kanidm service with domain "example.com"
-example.com {
-  redir /.well-known/webfinger https://idm.example.com/oauth2/openid/:client_id:{uri} 307
-}
-```
-**Note:** the `{uri}` is important as it preserves the original request past the redirect.
-
-
-</dd>
-
 <dt>
 
-[RFC 8414 OAuth 2.0 Authorisation Server Metadata](https://datatracker.ietf.org/doc/html/rfc8414) URL
+[RFC 8414 OAuth 2.0 Authorisation Server Metadata](https://datatracker.ietf.org/doc/html/rfc8414)
+URL **(recommended)**
 
 </dt>
 
@@ -111,6 +85,21 @@ example.com {
 
 <dt>
 
+[WebFinger URL **(discouraged)**](#webfinger)
+
+</dt>
+
+<dd>
+
+`https://idm.example.com/oauth2/openid/:client_id:/.well-known/webfinger`
+
+See [the WebFinger section](#webfinger) for more details, as there a number of
+caveats for WebFinger clients.
+
+</dd>
+
+<dt>
+
 User auth
 
 </dt>
@@ -466,3 +455,61 @@ kanidm system oauth2 reset-secrets
 ```
 
 Each client has unique signing keys and access secrets, so this is limited to each service.
+
+## WebFinger
+
+[WebFinger](https://datatracker.ietf.org/doc/html/rfc7033) provides a mechanism
+for discovering information about people or other entities. It can be used by an
+identity provider to supply OpenID Connect discovery information.
+
+Kanidm provides
+[an Identity Provider Discovery for OIDC URL](https://datatracker.ietf.org/doc/html/rfc7033#section-3.1)
+response to all incoming WebFinger requests, using a user's SPN as their account
+ID. This does not match on email addresses as they are not guaranteed to be
+unique.
+
+However, WebFinger has a number of flaws which make it difficult to use with
+Kanidm:
+
+* WebFinger assumes that the identity provider will give the same `iss`
+  (Issuer) for every OAuth 2.0/OIDC client, and there is no standard way for a
+  WebFinger client to report its client ID.
+
+  Kanidm uses a *different* `iss` (Issuer) value for each client.
+
+* WebFinger requires that this be served at the *root* of the domain of a user's
+  SPN (ie: information about the user with SPN `user@idm.example.com` is at
+  `https://idm.example.com/.well-known/webfinger`).
+
+  Kanidm *does not* provide a WebFinger endpoint at its root URL, because it has
+  no way to know *which* OAuth 2.0/OIDC client a WebFinger request is associated
+  with, so could report an incorrect `iss` (Issuer).
+
+  You will need a load balancer in front of Kanidm's HTTPS server to redirect
+  requests to the appropriate `/oauth2/openid/:client_id:/.well-known/webfinger`
+  URL. If the client does not follow redirects, you may need to rewrite the
+  request in the load balancer instead.
+
+  If you have *multiple* WebFinger clients, it will need to map some other
+  property of the request (such as a source IP address or `User-Agent` header)
+  to a client ID, and redirect to the appropriate WebFinger URL for that client.
+
+* Kanidm responds to *all* WebFinger queries with
+  [an Identity Provider Discovery for OIDC URL](https://datatracker.ietf.org/doc/html/rfc7033#section-3.1),
+  **regardless** of what
+  [`rel` parameter](https://datatracker.ietf.org/doc/html/rfc7033#section-4.4.4.1)
+  was specified.
+
+  This is to work around
+  [a broken client](https://tailscale.com/kb/1240/sso-custom-oidc) which doesn't
+  send a `rel` parameter, but expects an Identity Provider Discovery issuer URL
+  in response.
+
+  If you want to use WebFinger in any *other* context on Kanidm's hostname,
+  you'll need a load balancer in front of Kanidm which matches on some property
+  of the request.
+
+Because of the flaws of the WebFinger specification and the deployment
+difficulties they introduce, we recommend that applications use OpenID Connect
+Discovery or OAuth 2.0 Authorisation Server Metadata for client configuration
+instead of WebFinger.

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,
                 ..