issue 615 - improve sortyness and null handling of challege policy values

server/src/main/java/password/pwm/config/value/ChallengeValue.java

@@ -21,11 +21,14 @@
 package password.pwm.config.value;
 
 import com.google.gson.reflect.TypeToken;
+import password.pwm.PwmConstants;
 import password.pwm.config.PwmSetting;
 import password.pwm.config.stored.StoredConfigXmlConstants;
 import password.pwm.config.stored.XmlOutputProcessData;
 import password.pwm.config.value.data.ChallengeItemConfiguration;
+import password.pwm.util.i18n.LocaleComparators;
 import password.pwm.util.i18n.LocaleHelper;
+import password.pwm.util.java.CollectionUtil;
 import password.pwm.util.java.JsonUtil;
 import password.pwm.util.java.StringUtil;
 import password.pwm.util.java.XmlElement;
@@ -35,11 +38,14 @@ import password.pwm.util.secure.PwmSecurityKey;
 
 import java.util.ArrayList;
 import java.util.Collections;
+import java.util.Comparator;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Optional;
 import java.util.TreeMap;
+import java.util.stream.Collectors;
 
 public class ChallengeValue extends AbstractValue implements StoredValue
 {
@@ -50,7 +56,15 @@ public class ChallengeValue extends AbstractValue implements StoredValue
 
     ChallengeValue( final Map<String, List<ChallengeItemConfiguration>> values )
     {
-        this.values = values == null ? Collections.emptyMap() : Collections.unmodifiableMap( values );
+        // values created via js, so need to be defensive and strip all nulls.
+        final Comparator<String> comparator = LocaleComparators.stringLocaleComparator( PwmConstants.DEFAULT_LOCALE, LocaleComparators.Flag.DefaultFirst );
+        final Map<String, List<ChallengeItemConfiguration>> sortedMap = new TreeMap<>( comparator );
+        sortedMap.putAll( CollectionUtil.stripNulls( values ).entrySet().stream()
+                .collect( Collectors.toUnmodifiableMap(
+                        Map.Entry::getKey,
+                        v -> CollectionUtil.stripNulls( v.getValue() )
+                ) ) );
+        this.values = Collections.unmodifiableMap( sortedMap );
     }
 
     public static StoredValueFactory factory( )
@@ -72,7 +86,7 @@ public class ChallengeValue extends AbstractValue implements StoredValue
                             {
                             }
                     );
-                    srcMap = srcMap == null ? Collections.emptyMap() : new TreeMap<>(
+                    srcMap = srcMap == null ? Collections.emptyMap() : new LinkedHashMap<>(
                             srcMap );
                     return new ChallengeValue( Collections.unmodifiableMap( srcMap ) );
                 }
@@ -142,7 +156,7 @@ public class ChallengeValue extends AbstractValue implements StoredValue
     @Override
     public Map<String, List<ChallengeItemConfiguration>> toNativeObject( )
     {
-        return Collections.unmodifiableMap( values );
+        return values;
     }
 
     @Override

server/src/main/java/password/pwm/http/servlet/ClientApiServlet.java

@@ -48,6 +48,7 @@ import password.pwm.svc.sessiontrack.UserAgentUtils;
 import password.pwm.svc.stats.EpsStatistic;
 import password.pwm.svc.stats.Statistic;
 import password.pwm.svc.stats.StatisticsService;
+import password.pwm.util.i18n.LocaleComparators;
 import password.pwm.util.i18n.LocaleHelper;
 import password.pwm.util.java.StringUtil;
 import password.pwm.util.java.TimeDuration;
@@ -406,7 +407,7 @@ public class ClientApiServlet extends ControlledPwmServlet
             final Map<String, String> localeFlags = new LinkedHashMap<>();
 
             final List<Locale> knownLocales = new ArrayList<>( pwmRequest.getAppConfig().getKnownLocales() );
-            knownLocales.sort( LocaleHelper.localeComparator( PwmConstants.DEFAULT_LOCALE ) );
+            knownLocales.sort( LocaleComparators.localeComparator( ) );
 
             for ( final Locale locale : knownLocales )
             {

server/src/main/java/password/pwm/util/i18n/LocaleComparators.java

@@ -0,0 +1,136 @@
+/*
+ * Password Management Servlets (PWM)
+ * http://www.pwm-project.org
+ *
+ * Copyright (c) 2006-2009 Novell, Inc.
+ * Copyright (c) 2009-2021 The PWM Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package password.pwm.util.i18n;
+
+import password.pwm.PwmConstants;
+import password.pwm.util.java.JavaHelper;
+
+import java.io.Serializable;
+import java.util.Comparator;
+import java.util.Locale;
+import java.util.Objects;
+
+public class LocaleComparators
+{
+    private static final Comparator<Locale> LOCALE_COMPARATOR
+            = new LocaleComparator( PwmConstants.DEFAULT_LOCALE );
+    private static final Comparator<Locale> LOCALE_COMPARATOR_DEFAULT_FIRST
+            = new LocaleComparator( PwmConstants.DEFAULT_LOCALE, Flag.DefaultFirst );
+    private static final Comparator<String> STR_COMPARATOR
+            = new StringLocaleComparator( LOCALE_COMPARATOR );
+    private static final Comparator<String> STR_COMPARATOR_DEFAULT_FIRST
+            = new StringLocaleComparator( LOCALE_COMPARATOR_DEFAULT_FIRST );
+
+    // string with high ascii sort value
+    private static final String FIRST_POSITION_PSEUDO_VALUE = "!!!";
+
+    public enum Flag
+    {
+        /** Always sort default locale to first position. */
+        DefaultFirst
+    }
+
+    public static Comparator<Locale> localeComparator( final Flag... flag )
+    {
+        return localeComparator( PwmConstants.DEFAULT_LOCALE, flag );
+    }
+
+    public static Comparator<Locale> localeComparator( final Locale comparisonLocale, final Flag... flag )
+    {
+        if ( Objects.equals( comparisonLocale, PwmConstants.DEFAULT_LOCALE ) )
+        {
+            if ( JavaHelper.enumArrayContainsValue( flag, Flag.DefaultFirst ) )
+            {
+                return LOCALE_COMPARATOR_DEFAULT_FIRST;
+            }
+            else
+            {
+                return LOCALE_COMPARATOR;
+            }
+        }
+        return new LocaleComparator( comparisonLocale, flag );
+    }
+
+    public static Comparator<String> stringLocaleComparator( final Flag... flag )
+    {
+        return stringLocaleComparator( PwmConstants.DEFAULT_LOCALE, flag );
+    }
+
+    public static Comparator<String> stringLocaleComparator( final Locale comparisonLocale, final Flag... flag )
+    {
+        if ( Objects.equals( comparisonLocale, PwmConstants.DEFAULT_LOCALE ) )
+        {
+            if ( JavaHelper.enumArrayContainsValue( flag, Flag.DefaultFirst ) )
+            {
+                return STR_COMPARATOR_DEFAULT_FIRST;
+            }
+            else
+            {
+                return STR_COMPARATOR;
+            }
+        }
+        return new StringLocaleComparator( new LocaleComparator( comparisonLocale, flag ) );
+    }
+
+    private static class LocaleComparator implements Comparator<Locale>, Serializable
+    {
+        private final boolean defaultFirst;
+        private final Locale comparisonLocale;
+
+        LocaleComparator( final Locale comparisonLocale, final Flag... flag )
+        {
+            this.defaultFirst = JavaHelper.enumArrayContainsValue( flag, Flag.DefaultFirst );
+            this.comparisonLocale = comparisonLocale;
+        }
+
+        @Override
+        public int compare( final Locale o1, final Locale o2 )
+        {
+            final String name1 = defaultFirst && Objects.equals( o1, PwmConstants.DEFAULT_LOCALE )
+                    ? FIRST_POSITION_PSEUDO_VALUE
+                    : o1.getDisplayName( comparisonLocale );
+
+            final String name2 = defaultFirst && Objects.equals( o2, PwmConstants.DEFAULT_LOCALE )
+                    ? FIRST_POSITION_PSEUDO_VALUE
+                    : o2.getDisplayName( comparisonLocale );
+
+            return name1.compareToIgnoreCase( name2 );
+        }
+    }
+
+    private static class StringLocaleComparator implements Comparator<String>, Serializable
+    {
+        private final Comparator<Locale> localeComparator;
+
+        StringLocaleComparator( final Comparator<Locale> localeComparator )
+        {
+            this.localeComparator = localeComparator;
+        }
+
+        @Override
+        public int compare( final String o1, final String o2 )
+        {
+            final Locale locale1 = LocaleHelper.parseLocaleString( o1 );
+            final Locale locale2 = LocaleHelper.parseLocaleString( o2 );
+            return localeComparator.compare( locale1, locale2 );
+        }
+    }
+}

server/src/main/java/password/pwm/util/i18n/LocaleHelper.java

@@ -42,7 +42,6 @@ import java.time.Instant;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.Comparator;
 import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -61,6 +60,7 @@ public class LocaleHelper
 {
     private static final PwmLogger LOGGER = PwmLogger.forClass( LocaleHelper.class );
 
+
     public enum TextDirection
     {
         rtl,
@@ -437,16 +437,6 @@ public class LocaleHelper
                 : locale.toString().replace( "_", "-" );
     }
 
-    public static Comparator<Locale> localeComparator( final Locale comparisonLocale )
-    {
-        return ( o1, o2 ) ->
-        {
-            final String name1 = o1.getDisplayName( comparisonLocale );
-            final String name2 = o2.getDisplayName( comparisonLocale );
-            return name1.compareToIgnoreCase( name2 );
-        };
-    }
-
     public static List<Locale> highLightedLocales()
     {
         final List<String> strValues = PwmConstants.HIGHLIGHT_LOCALES;

server/src/main/java/password/pwm/util/java/CollectionUtil.java

@@ -29,6 +29,7 @@ import java.util.Iterator;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.Spliterator;
 import java.util.Spliterators;
@@ -43,6 +44,18 @@ public class CollectionUtil
         return StreamSupport.stream( Spliterators.spliteratorUnknownSize( iterator, Spliterator.ORDERED ), false );
     }
 
+    public static <V> List<V> stripNulls( final List<V> input )
+    {
+        if ( input == null )
+        {
+            return Collections.emptyList();
+        }
+
+        return input.stream()
+                .filter( Objects::nonNull )
+                .collect( Collectors.toUnmodifiableList() );
+    }
+
     public static <K, V> Map<K, V> stripNulls( final Map<K, V> input )
     {
         if ( input == null )

server/src/test/java/password/pwm/util/i18n/LocaleComparatorTest.java

@@ -0,0 +1,55 @@
+/*
+ * Password Management Servlets (PWM)
+ * http://www.pwm-project.org
+ *
+ * Copyright (c) 2006-2009 Novell, Inc.
+ * Copyright (c) 2009-2021 The PWM Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package password.pwm.util.i18n;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.ArrayList;
+import java.util.List;
+
+public class LocaleComparatorTest
+{
+    @Test
+    public void stringLocaleComparator()
+    {
+        final List<String> list = new ArrayList<>();
+        list.add( "ja" );
+        list.add( "en" );
+        list.add( "br" );
+        list.add( "fr" );
+
+        list.sort( LocaleComparators.stringLocaleComparator() );
+
+        Assert.assertEquals( "br", list.get( 0 ) );
+        Assert.assertEquals( "en", list.get( 1 ) );
+        Assert.assertEquals( "fr", list.get( 2 ) );
+        Assert.assertEquals( "ja", list.get( 3 ) );
+
+        list.sort( LocaleComparators.stringLocaleComparator( LocaleComparators.Flag.DefaultFirst ) );
+
+        // default (english) first
+        Assert.assertEquals( "en", list.get( 0 ) );
+        Assert.assertEquals( "br", list.get( 1 ) );
+        Assert.assertEquals( "fr", list.get( 2 ) );
+        Assert.assertEquals( "ja", list.get( 3 ) );
+    }
+}

webapp/src/main/webapp/public/resources/js/configeditor-settings-challenges.js

@@ -365,7 +365,11 @@ ChallengeSettingHandler.toggleAdminDefinedRow = function(toggleElement,inputID,k
 };
 
 ChallengeSettingHandler.deleteRow = function(keyName, localeKey, rowName) {
+    var questionText = PWM_VAR['clientSettingCache'][keyName][localeKey][rowName]['text'];
+    var adminDefined = PWM_VAR['clientSettingCache'][keyName][localeKey][rowName]['adminDefined'];
+    var output = (adminDefined ? 'the question "' + questionText + '"': 'the [User Defined] question?');
     PWM_MAIN.showConfirmDialog({
+        text: 'Are you sure you want to remove ' + output,
         okAction:function(){
             PWM_MAIN.showWaitDialog({loadFunction:function(){
                     delete PWM_VAR['clientSettingCache'][keyName][localeKey][rowName];