From f2f15c78f5ca311cfb9b84014ad32afa5dba5be2 Mon Sep 17 00:00:00 2001
From: Alex Harries <alex.harries@redactive.co.uk>
Date: Thu, 29 Aug 2024 15:41:48 +0100
Subject: [PATCH] Fix confusing custom NameID UX

This commit:

1. Adds the full NameID format string into the drop-down select options, after the "User-friendly" name, so administrators can see the exact format name when configuring NameID, and

2. Fixes the problem where the custom NameID field's value is being incorrectly set to "*".

Signed-off-by: Alex Harries <alex.harries@redactive.co.uk>
---
 src/Form/SamlauthConfigureTrait.php | 49 ++++++++++++++++++++---------
 1 file changed, 34 insertions(+), 15 deletions(-)

diff --git a/src/Form/SamlauthConfigureTrait.php b/src/Form/SamlauthConfigureTrait.php
index bfb5e74..1d7d910 100644
--- a/src/Form/SamlauthConfigureTrait.php
+++ b/src/Form/SamlauthConfigureTrait.php
@@ -94,15 +94,15 @@ trait SamlauthConfigureTrait {
     $build['sp_name_id_format']['#type'] = 'select';
     $build['sp_name_id_format']['#options'] = [
       '' => $this->t('- Select -'),
-      SamlConstants::NAMEID_UNSPECIFIED => $this->t('Unspecified'),
-      SamlConstants::NAMEID_PERSISTENT => $this->t('Persistent'),
-      SamlConstants::NAMEID_EMAIL_ADDRESS => $this->t('Email address'),
-      SamlConstants::NAMEID_ENTITY => $this->t('Entity'),
-      SamlConstants::NAMEID_WINDOWS_DOMAIN_QUALIFIED_NAME => $this->t('Windows domain qualified name'),
-      SamlConstants::NAMEID_X509_SUBJECT_NAME => $this->t('X.509 subject name'),
-      SamlConstants::NAMEID_TRANSIENT => $this->t('Transient'),
-      SamlConstants::NAMEID_ENCRYPTED => $this->t('Encrypted'),
-      SamlConstants::NAMEID_KERBEROS => $this->t('Kerberos'),
+      SamlConstants::NAMEID_UNSPECIFIED => $this->t('Unspecified (@nameid_format)', ['@nameid_format' => SamlConstants::NAMEID_UNSPECIFIED]),
+      SamlConstants::NAMEID_PERSISTENT => $this->t('Persistent (@nameid_format)', ['@nameid_format' => SamlConstants::NAMEID_PERSISTENT]),
+      SamlConstants::NAMEID_EMAIL_ADDRESS => $this->t('Email address (@nameid_format)', ['@nameid_format' => SamlConstants::NAMEID_EMAIL_ADDRESS]),
+      SamlConstants::NAMEID_ENTITY => $this->t('Entity (@nameid_format)', ['@nameid_format' => SamlConstants::NAMEID_ENTITY]),
+      SamlConstants::NAMEID_WINDOWS_DOMAIN_QUALIFIED_NAME => $this->t('Windows domain qualified name (@nameid_format)', ['@nameid_format' => SamlConstants::NAMEID_WINDOWS_DOMAIN_QUALIFIED_NAME]),
+      SamlConstants::NAMEID_X509_SUBJECT_NAME => $this->t('X.509 subject name (@nameid_format)', ['@nameid_format' => SamlConstants::NAMEID_X509_SUBJECT_NAME]),
+      SamlConstants::NAMEID_TRANSIENT => $this->t('Transient (@nameid_format)', ['@nameid_format' => SamlConstants::NAMEID_TRANSIENT]),
+      SamlConstants::NAMEID_ENCRYPTED => $this->t('Encrypted (@nameid_format)', ['@nameid_format' => SamlConstants::NAMEID_ENCRYPTED]),
+      SamlConstants::NAMEID_KERBEROS => $this->t('Kerberos (@nameid_format)', ['@nameid_format' => SamlConstants::NAMEID_KERBEROS]),
       '*' => $this->t('- Other -'),
     ];
     $build['sp_name_id_format_'] = [
@@ -121,14 +121,33 @@ trait SamlauthConfigureTrait {
           ],
         ],
       ],
+      '#default_value' => $config->get('sp_name_id_format_'),
     ];
-    $default_value = $config->get('sp_name_id_format');
-    // If $default_value === '*', it cannot be saved as
-    // such while submitting the form.
-    if ($default_value && (!isset($build['sp_name_id_format']['#options'][$default_value])
-        || $default_value === '*')) {
+
+    $sp_name_id_format_default_value = $config->get('sp_name_id_format');
+    $sp_name_id_format_custom_value = $config->get('sp_name_id_format_');
+
+    // If a custom sp_name_id_format is specified which matches a known
+    // NameID format, then select that format and empty the custom NameID
+    // format field.
+    if (($sp_name_id_format_default_value === '*')
+      && !empty($build['sp_name_id_format']['#options'][$sp_name_id_format_custom_value])) {
+      // The custom value matches an already-known NameID format, so reset it.
+      // @todo: consider whether changing this can lead to a confusing UX
+      $build['sp_name_id_format']['#default_value'] = $sp_name_id_format_custom_value;
+      $build['sp_name_id_format_']['#default_value'] = '';
+    }
+    // If the sp_name_id_format field is not set to a known type (i.e. it is
+    // set to custom), then explicitly select the custom option to force the
+    // user to either provide a custom format, or choose an existing format
+    // from the drop-down.
+    elseif ((!empty($sp_name_id_format_default_value) && (empty($build['sp_name_id_format']['#options'][$sp_name_id_format_default_value]))
+      || ($sp_name_id_format_default_value === '*'))) {
+
       $build['sp_name_id_format']['#default_value'] = '*';
-      $build['sp_name_id_format_']['#default_value'] = $default_value;
+
+      // Don't overwrite any existing default NameID format - i.e. the value
+      // of $build['sp_name_id_format_']['#default_value'].
     }
   }
 
-- 
2.23.0