From 339a20947a5f818088577e54ceecf0eb376d7536 Mon Sep 17 00:00:00 2001
From: ToxicMushroom <32853531+ToxicMushroom@users.noreply.github.com>
Date: Sat, 5 Apr 2025 17:22:07 +0200
Subject: [PATCH] Fixup email submission, tested

---
 Cargo.lock                                    |   2 +-
 server/core/src/https/views/mod.rs            |   2 +-
 server/core/src/https/views/profile.rs        | 103 +++++++++++++-----
 server/core/static/external/forms.js          |   4 +-
 .../form_email_entry_partial.html             |  17 +++
 .../profile_changes_partial.html              |   4 +-
 .../user_settings_profile_partial.html        |  39 +++----
 7 files changed, 114 insertions(+), 57 deletions(-)
 create mode 100644 server/core/templates/user_settings/form_email_entry_partial.html

diff --git a/Cargo.lock b/Cargo.lock
index 0f4fa00ad..f3b24271c 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -5144,7 +5144,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "9d2de91cf02bbc07cde38891769ccd5d4f073d22a40683aa4bc7a95781aaa2c4"
 dependencies = [
  "form_urlencoded",
- "indexmap 2.7.1",
+ "indexmap 2.8.0",
  "itoa",
  "ryu",
  "serde",
diff --git a/server/core/src/https/views/mod.rs b/server/core/src/https/views/mod.rs
index ffc74c916..8157257a5 100644
--- a/server/core/src/https/views/mod.rs
+++ b/server/core/src/https/views/mod.rs
@@ -132,7 +132,7 @@ pub fn view_router() -> Router<ServerState> {
         .route("/api/cu_commit", post(reset::commit))
         .route(
             "/api/user_settings/add_email",
-            post(profile::view_new_email_entry_partial),
+            get(profile::view_new_email_entry_partial),
         )
         .route(
             "/api/user_settings/edit_profile",
diff --git a/server/core/src/https/views/profile.rs b/server/core/src/https/views/profile.rs
index fbfa0c581..a3678010f 100644
--- a/server/core/src/https/views/profile.rs
+++ b/server/core/src/https/views/profile.rs
@@ -1,4 +1,3 @@
-use kanidm_proto::attribute::Attribute;
 use super::constants::{ProfileMenuItems, Urls};
 use super::errors::HtmxError;
 use super::login::{LoginDisplayCtx, Reauth, ReauthPurpose};
@@ -9,7 +8,7 @@ use crate::https::middleware::KOpId;
 use crate::https::ServerState;
 use askama::Template;
 use askama_axum::IntoResponse;
-use axum::extract::State;
+use axum::extract::{Query, State};
 use axum::http::Uri;
 use axum::response::Response;
 use axum::Extension;
@@ -17,9 +16,11 @@ use axum_extra::extract::cookie::CookieJar;
 use axum_extra::extract::Form;
 use axum_htmx::{HxEvent, HxPushUrl, HxResponseTrigger};
 use futures_util::TryFutureExt;
+use kanidm_proto::attribute::Attribute;
 use kanidm_proto::constants::{ATTR_DISPLAYNAME, ATTR_MAIL};
 use kanidm_proto::internal::UserAuthToken;
 use kanidm_proto::scim_v1::server::{ScimEffectiveAccess, ScimPerson};
+use kanidm_proto::scim_v1::ScimMail;
 use kanidmd_lib::filter::{f_id, Filter};
 use kanidmd_lib::prelude::f_and;
 use kanidmd_lib::prelude::FC;
@@ -46,17 +47,26 @@ struct ProfilePartialView {
 }
 
 #[derive(Clone, Debug, Serialize, Deserialize)]
-pub(crate) struct ProfileAttributes {
+pub(crate) struct SaveProfileQuery {
     #[serde(rename = "name")]
     account_name: String,
     #[serde(rename = "displayname")]
     display_name: String,
+    #[serde(rename = "email_index")]
+    emails_indexes: Vec<u16>,
     #[serde(rename = "emails[]")]
     emails: Vec<String>,
-    // radio buttons are used to pick a primary index
+    // radio buttons are used to pick a primary index, remove causes holes, map back into [emails] using [emails_indexes]
     primary_email_index: u16,
 }
 
+#[derive(Clone, Debug, Serialize, Deserialize)]
+pub(crate) struct ProfileAttributes {
+    account_name: String,
+    display_name: String,
+    emails: Vec<ScimMail>,
+}
+
 #[derive(Template, Clone)]
 #[template(path = "user_settings/profile_changes_partial.html")]
 struct ProfileChangesPartialView {
@@ -67,14 +77,12 @@ struct ProfileChangesPartialView {
 }
 
 #[derive(Template, Clone)]
-#[template(path = "user_settings/form_modifiable_entry_modifiable_list_partial.html")]
-// Modifiable entry in a modifiable list partial
-pub(crate) struct FormModEntryModListPartial {
-    can_rw: bool,
-    r#type: String,
-    name: String,
+#[template(path = "user_settings/form_email_entry_partial.html")]
+pub(crate) struct FormEmailEntryListPartial {
+    can_edit: bool,
     value: String,
-    invalid_feedback: String,
+    primary: bool,
+    index: u16,
 }
 
 impl Display for ProfileAttributes {
@@ -94,12 +102,14 @@ pub(crate) async fn view_profile_get(
         .handle_whoami_uat(client_auth_info.clone(), kopid.eventid)
         .await?;
 
-    let (scim_person, scim_effective_access) = crate::https::views::admin::persons::get_person_info(
-        uat.uuid,
-        state,
-        &kopid,
-        client_auth_info.clone()).await?;
-
+    let (scim_person, scim_effective_access) =
+        crate::https::views::admin::persons::get_person_info(
+            uat.uuid,
+            state,
+            &kopid,
+            client_auth_info.clone(),
+        )
+        .await?;
 
     let time = time::OffsetDateTime::now_utc() + time::Duration::new(60, 0);
 
@@ -112,7 +122,7 @@ pub(crate) async fn view_profile_get(
             menu_active_item: ProfileMenuItems::UserProfile,
             can_rw,
             person: scim_person,
-            scim_effective_access
+            scim_effective_access,
         },
     })
 }
@@ -123,7 +133,7 @@ pub(crate) async fn view_profile_diff_start_save_post(
     VerifiedClientInformation(client_auth_info): VerifiedClientInformation,
     DomainInfo(domain_info): DomainInfo,
     // Form must be the last parameter because it consumes the request body
-    Form(new_attrs): Form<ProfileAttributes>,
+    Form(query): Form<SaveProfileQuery>,
 ) -> axum::response::Result<Response> {
     let uat: UserAuthToken = state
         .qe_r_ref
@@ -138,13 +148,34 @@ pub(crate) async fn view_profile_diff_start_save_post(
         uat.uuid,
         state,
         &kopid,
-        client_auth_info.clone()).await?;
+        client_auth_info.clone(),
+    )
+    .await?;
+
+    let primary_index = query
+        .emails_indexes
+        .iter()
+        .position(|ei| ei == &query.primary_email_index)
+        .unwrap_or(0);
+    let new_mails = query
+        .emails
+        .iter()
+        .enumerate()
+        .map(|(ei, email)| ScimMail {
+            primary: ei == primary_index,
+            value: email.to_string(),
+        })
+        .collect();
 
     let profile_view = ProfileChangesPartialView {
         menu_active_item: ProfileMenuItems::UserProfile,
         can_rw,
         person: scim_person,
-        new_attrs
+        new_attrs: ProfileAttributes {
+            account_name: query.account_name,
+            display_name: query.display_name,
+            emails: new_mails,
+        },
     };
 
     Ok((
@@ -160,7 +191,7 @@ pub(crate) async fn view_profile_diff_confirm_save_post(
     VerifiedClientInformation(client_auth_info): VerifiedClientInformation,
     DomainInfo(domain_info): DomainInfo,
     // Form must be the last parameter because it consumes the request body
-    Form(new_attrs): Form<ProfileAttributes>,
+    Form(mut new_attrs): Form<ProfileAttributes>,
 ) -> axum::response::Result<Response> {
     let uat: UserAuthToken = state
         .qe_r_ref
@@ -184,13 +215,17 @@ pub(crate) async fn view_profile_diff_confirm_save_post(
         .map_err(|op_err| HtmxError::new(&kopid, op_err, domain_info.clone()))
         .await?;
 
+    new_attrs
+        .emails
+        .sort_by_key(|sm| if sm.primary { 0 } else { 1 });
+    let email_addresses = new_attrs.emails.into_iter().map(|sm| sm.value).collect();
     state
         .qe_w_ref
         .handle_setattribute(
             client_auth_info.clone(),
             uat.uuid.to_string(),
             ATTR_MAIL.to_string(),
-            new_attrs.emails,
+            email_addresses,
             filter.clone(),
             kopid.eventid,
         )
@@ -229,29 +264,37 @@ pub(crate) async fn view_profile_diff_confirm_save_post(
         State(state),
         Extension(kopid),
         VerifiedClientInformation(client_auth_info),
-        DomainInfo(domain_info)
-    ).await {
+        DomainInfo(domain_info),
+    )
+    .await
+    {
         Ok(pv) => Ok(pv.into_response()),
         Err(e) => Ok(e.into_response()),
     }
 }
 
+#[derive(Deserialize)]
+pub(crate) struct AddEmailQuery {
+    // the last email index is passed so we can return an incremented id
+    email_index: Option<u16>,
+}
+
 // Sends the user a new email input to fill in :)
 pub(crate) async fn view_new_email_entry_partial(
     State(_state): State<ServerState>,
     VerifiedClientInformation(_client_auth_info): VerifiedClientInformation,
     Extension(_kopid): Extension<KOpId>,
+    Query(email_query): Query<AddEmailQuery>,
 ) -> axum::response::Result<Response> {
     let add_email_trigger =
         HxResponseTrigger::after_swap([HxEvent::new("addEmailSwapped".to_string())]);
     Ok((
         add_email_trigger,
-        FormModEntryModListPartial {
-            can_rw: true,
-            r#type: "email".to_string(),
-            name: "emails[]".to_string(),
+        FormEmailEntryListPartial {
+            can_edit: true,
             value: "".to_string(),
-            invalid_feedback: "Please enter a valid email address.".to_string(),
+            primary: email_query.email_index.is_none(),
+            index: email_query.email_index.map(|i| i + 1).unwrap_or(0),
         },
     )
         .into_response())
diff --git a/server/core/static/external/forms.js b/server/core/static/external/forms.js
index 7e5bfe74c..7c34fc76f 100644
--- a/server/core/static/external/forms.js
+++ b/server/core/static/external/forms.js
@@ -6,9 +6,9 @@ function rehook_string_list_removers() {
         if (button.getAttribute("kanidm_hooked") !== null) continue
 
         button.addEventListener("click", (e) => {
-            // Expected html nesting: li > div.input-group > button.kanidm-remove-list-entry
+            // Expected html nesting: div.email-entry > div.input-group > button.kanidm-remove-list-entry
             let li = button.parentElement?.parentElement;
-            if (li && li.tagName === "LI") {
+            if (li && li.classList.contains("email-entry")) {
                 li.remove();
             }
         })
diff --git a/server/core/templates/user_settings/form_email_entry_partial.html b/server/core/templates/user_settings/form_email_entry_partial.html
new file mode 100644
index 000000000..032c29bc4
--- /dev/null
+++ b/server/core/templates/user_settings/form_email_entry_partial.html
@@ -0,0 +1,17 @@
+<div class="email-entry">
+    <input hidden class="email-index-state" type="text" name="email_index" value="((index))">
+    <div class="input-group mb-1">
+
+        <div class="input-group-text">
+            <input class="form-check-input mt-0" name="primary_email_index" type="radio" value="((index))" aria-label="Primary email radio button for following text input" (% if primary %) checked (% endif %)(% if !can_edit %) disabled (% endif %)>
+        </div>
+
+        (% if can_edit %)
+        <input type="email" aria-label="Email address input ((index))" class="form-control" name="emails[]" value="(( value ))" hx-validate="true" required>
+        <button type="button" class="btn btn-secondary kanidm-remove-list-entry">Remove</button>
+        (% else %)
+        <input type="email" aria-label="Email address input ((index))" class="form-control" name="emails[]" value="(( value ))" hx-validate="true" required disabled>
+        (% endif %)
+    </div>
+    <div class="invalid-feedback">Please enter a valid email address.</div>
+</div>
\ No newline at end of file
diff --git a/server/core/templates/user_settings/profile_changes_partial.html b/server/core/templates/user_settings/profile_changes_partial.html
index 6288868aa..078c6f134 100644
--- a/server/core/templates/user_settings/profile_changes_partial.html
+++ b/server/core/templates/user_settings/profile_changes_partial.html
@@ -13,7 +13,7 @@ Profile Difference
     <input type="hidden" name="display_name" value="(( new_attrs.display_name ))"/>
 <!--    <input type="hidden" name="legal_name" value=" new_attrs.legal_name "/>-->
     (% for email in new_attrs.emails %)
-        <input type="hidden" name="emails[]" value="(( email ))"/>
+        <input type="hidden" name="emails[]" value="(( email.value ))"/>
     (% endfor %)
 
     <table class="table table-bordered table-responsive">
@@ -54,7 +54,7 @@ Profile Difference
             <td class="text-break">
                 <ul>
                 (% for email in new_attrs.emails %)
-                    <li>(( email ))</li>
+                    <li>(( email.value ))</li>
                 (% endfor %)
                 </ul>
             </td>
diff --git a/server/core/templates/user_settings_profile_partial.html b/server/core/templates/user_settings_profile_partial.html
index 2ab46ff69..0f170280b 100644
--- a/server/core/templates/user_settings_profile_partial.html
+++ b/server/core/templates/user_settings_profile_partial.html
@@ -21,17 +21,26 @@ Profile
 
 (% block settings_window %)
 
-<form id="user_settings_container" class="needs-validation" hx-post="/ui/api/user_settings/edit_profile"
-      hx-target="#user_settings_container" hx-swap="outerHTML" hx-validate="true" hx-ext="bs-validation" novalidate>
+<form id="user_settings_container" class="needs-validation"
+      hx-post="/ui/api/user_settings/edit_profile"
+      hx-target="#user_settings_container"
+      hx-swap="outerHTML"
+      hx-validate="true"
+      hx-ext="bs-validation"
+      novalidate>
     (% call string_attr("Name", "name", person.name, true, Attribute::Name) %)
 
     (% call string_attr("Displayname", "displayname", person.displayname, true, Attribute::DisplayName) %)
 
     <div class="mb-2">
         <div class="mt-3 mb-2 col-12 col-md-11 col-lg-8">
-            <label for="profileEmail" class="fw-bold">Email addresses (select primary)</label>
+            <label for="profileEmail" class="fw-bold">Email addresses (% if can_rw %)(selected => primary)(% else %)(select primary)(% endif %)</label>
             (% if can_rw %)
-            <a class="cursor-pointer float-end" hx-boost="true" hx-post="/ui/api/user_settings/add_email" hx-target="#emailAddresses"
+            <a class="cursor-pointer float-end"
+               hx-boost="true"
+               hx-get="/ui/api/user_settings/add_email"
+               hx-target="#emailAddresses"
+               hx-include="#emailAddresses :last-child .email-index-state"
                hx-swap="beforeend">
                 <svg xmlns="http://www.w3.org/2000/svg" fill="currentColor" class="bi bi-plus-square"
                      viewBox="0 0 16 16" width="20" height="20">
@@ -39,29 +48,17 @@ Profile
                     <path d="M8 4a.5.5 0 0 1 .5.5v3h3a.5.5 0 0 1 0 1h-3v3a.5.5 0 0 1-1 0v-3h-3a.5.5 0 0 1 0-1h3v-3A.5.5 0 0 1 8 4"></path>
                 </svg>
             </a>
+            (% endif %)
         </div>
-        (% endif %)
         <div>
             <div class="row g-0">
                 <div class="col-12 col-md-11 col-lg-8" id="emailAddresses">
-                    (% for (i, email) in person.mails.iter().enumerate() %)
-                    (% let type = "email" %)
-                    (% let name = "emails[]" %)
+                    (% for (index, email) in person.mails.iter().enumerate() %)
                     (% let value = email.value.clone() %)
-                    (% let invalid_feedback = "Please enter a valid email address." %)
+                    (% let primary = email.primary %)
+                    (% let can_edit = scim_effective_access.modify_present.check(Attribute::Mail|as_ref) %)
 
-                        <div class="input-group mb-1">
-                            <div class="input-group-text">
-                                <input class="form-check-input mt-0" name="primary_email_index" type="radio" value="((i))" aria-label="Primary email radio button for following text input">
-                            </div>
-
-                            <input type="(( type ))" class="form-control" name="(( name ))" value="(( value ))" hx-validate="true" required>
-                            (% if can_rw %)
-                            <button type="button" class="btn btn-secondary kanidm-remove-list-entry">Remove</button>
-                            (% endif %)
-
-                        </div>
-                        <div class="invalid-feedback">(( invalid_feedback ))</div>
+                    (% include "user_settings/form_email_entry_partial.html" %)
 
                     (% endfor %)
                 </div>