Skip to content

Commit c6e3a5b

Browse files
jrivardjrivard
committed
fix issue 615 - improve sortyness and null handling for challenge policy values
1 parent 634cd3c commit c6e3a5b

File tree

7 files changed

+242
-18
lines changed

7 files changed

+242
-18
lines changed

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

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,13 @@
2121
package password.pwm.config.value;
2222

2323
import com.google.gson.reflect.TypeToken;
24+
import password.pwm.PwmConstants;
2425
import password.pwm.config.PwmSetting;
2526
import password.pwm.config.stored.XmlOutputProcessData;
2627
import password.pwm.config.value.data.ChallengeItemConfiguration;
28+
import password.pwm.util.i18n.LocaleComparators;
2729
import password.pwm.util.i18n.LocaleHelper;
30+
import password.pwm.util.java.JavaHelper;
2831
import password.pwm.util.java.JsonUtil;
2932
import password.pwm.util.java.XmlElement;
3033
import password.pwm.util.java.XmlFactory;
@@ -33,10 +36,13 @@
3336

3437
import java.util.ArrayList;
3538
import java.util.Collections;
39+
import java.util.Comparator;
40+
import java.util.LinkedHashMap;
3641
import java.util.List;
3742
import java.util.Locale;
3843
import java.util.Map;
3944
import java.util.TreeMap;
45+
import java.util.stream.Collectors;
4046

4147
public class ChallengeValue extends AbstractValue implements StoredValue
4248
{
@@ -47,7 +53,15 @@ public class ChallengeValue extends AbstractValue implements StoredValue
4753

4854
ChallengeValue( final Map<String, List<ChallengeItemConfiguration>> values )
4955
{
50-
this.values = values == null ? Collections.emptyMap() : Collections.unmodifiableMap( values );
56+
// values created via js, so need to be defensive and strip all nulls.
57+
final Comparator<String> comparator = LocaleComparators.stringLocaleComparator( PwmConstants.DEFAULT_LOCALE, LocaleComparators.Flag.DefaultFirst );
58+
final Map<String, List<ChallengeItemConfiguration>> sortedMap = new TreeMap<>( comparator );
59+
sortedMap.putAll( JavaHelper.stripNulls( values ).entrySet().stream()
60+
.collect( Collectors.toUnmodifiableMap(
61+
Map.Entry::getKey,
62+
v -> JavaHelper.stripNulls( v.getValue() )
63+
) ) );
64+
this.values = Collections.unmodifiableMap( sortedMap );
5165
}
5266

5367
public static StoredValueFactory factory( )
@@ -69,7 +83,7 @@ public ChallengeValue fromJson( final String input )
6983
{
7084
}
7185
);
72-
srcMap = srcMap == null ? Collections.emptyMap() : new TreeMap<>(
86+
srcMap = srcMap == null ? Collections.emptyMap() : new LinkedHashMap<>(
7387
srcMap );
7488
return new ChallengeValue( Collections.unmodifiableMap( srcMap ) );
7589
}
@@ -83,7 +97,7 @@ public ChallengeValue fromXmlElement(
8397
)
8498
{
8599
final List<XmlElement> valueElements = settingElement.getChildren( "value" );
86-
final Map<String, List<ChallengeItemConfiguration>> values = new TreeMap<>();
100+
final Map<String, List<ChallengeItemConfiguration>> values = new LinkedHashMap<>();
87101
final boolean oldStyle = "LOCALIZED_STRING_ARRAY".equals( settingElement.getAttributeValue( "syntax" ) );
88102
for ( final XmlElement loopValueElement : valueElements )
89103
{
@@ -92,7 +106,7 @@ public ChallengeValue fromXmlElement(
92106
final String value = loopValueElement.getText();
93107
if ( !values.containsKey( localeString ) )
94108
{
95-
values.put( localeString, new ArrayList<ChallengeItemConfiguration>() );
109+
values.put( localeString, new ArrayList<>() );
96110
}
97111
final ChallengeItemConfiguration challengeItemBean;
98112
if ( oldStyle )
@@ -140,7 +154,7 @@ public List<XmlElement> toXmlValues( final String valueElementName, final XmlOut
140154
@Override
141155
public Map<String, List<ChallengeItemConfiguration>> toNativeObject( )
142156
{
143-
return Collections.unmodifiableMap( values );
157+
return values;
144158
}
145159

146160
@Override

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
import password.pwm.svc.stats.EpsStatistic;
4848
import password.pwm.svc.stats.Statistic;
4949
import password.pwm.svc.stats.StatisticsManager;
50+
import password.pwm.util.i18n.LocaleComparators;
5051
import password.pwm.util.i18n.LocaleHelper;
5152
import password.pwm.util.java.StringUtil;
5253
import password.pwm.util.java.TimeDuration;
@@ -404,7 +405,7 @@ private static Map<String, Object> makeClientData(
404405
final Map<String, String> localeFlags = new LinkedHashMap<>();
405406

406407
final List<Locale> knownLocales = new ArrayList<>( pwmApplication.getConfig().getKnownLocales() );
407-
knownLocales.sort( LocaleHelper.localeComparator( PwmConstants.DEFAULT_LOCALE ) );
408+
knownLocales.sort( LocaleComparators.localeComparator( ) );
408409

409410
for ( final Locale locale : knownLocales )
410411
{

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

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
/*
2+
* Password Management Servlets (PWM)
3+
* http://www.pwm-project.org
4+
*
5+
* Copyright (c) 2006-2009 Novell, Inc.
6+
* Copyright (c) 2009-2021 The PWM Project
7+
*
8+
* Licensed under the Apache License, Version 2.0 (the "License");
9+
* you may not use this file except in compliance with the License.
10+
* You may obtain a copy of the License at
11+
*
12+
* http://www.apache.org/licenses/LICENSE-2.0
13+
*
14+
* Unless required by applicable law or agreed to in writing, software
15+
* distributed under the License is distributed on an "AS IS" BASIS,
16+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
17+
* See the License for the specific language governing permissions and
18+
* limitations under the License.
19+
*/
20+
21+
package password.pwm.util.i18n;
22+
23+
import password.pwm.PwmConstants;
24+
import password.pwm.util.java.JavaHelper;
25+
26+
import java.io.Serializable;
27+
import java.util.Comparator;
28+
import java.util.Locale;
29+
import java.util.Objects;
30+
31+
public class LocaleComparators
32+
{
33+
private static final Comparator<Locale> LOCALE_COMPARATOR
34+
= new LocaleComparator( PwmConstants.DEFAULT_LOCALE );
35+
private static final Comparator<Locale> LOCALE_COMPARATOR_DEFAULT_FIRST
36+
= new LocaleComparator( PwmConstants.DEFAULT_LOCALE, Flag.DefaultFirst );
37+
private static final Comparator<String> STR_COMPARATOR
38+
= new StringLocaleComparator( LOCALE_COMPARATOR );
39+
private static final Comparator<String> STR_COMPARATOR_DEFAULT_FIRST
40+
= new StringLocaleComparator( LOCALE_COMPARATOR_DEFAULT_FIRST );
41+
42+
// string with high ascii sort value
43+
private static final String FIRST_POSITION_PSEUDO_VALUE = "!!!";
44+
45+
public enum Flag
46+
{
47+
/** Always sort default locale to first position. */
48+
DefaultFirst
49+
}
50+
51+
public static Comparator<Locale> localeComparator( final Flag... flag )
52+
{
53+
return localeComparator( PwmConstants.DEFAULT_LOCALE, flag );
54+
}
55+
56+
public static Comparator<Locale> localeComparator( final Locale comparisonLocale, final Flag... flag )
57+
{
58+
if ( Objects.equals( comparisonLocale, PwmConstants.DEFAULT_LOCALE ) )
59+
{
60+
if ( JavaHelper.enumArrayContainsValue( flag, Flag.DefaultFirst ) )
61+
{
62+
return LOCALE_COMPARATOR_DEFAULT_FIRST;
63+
}
64+
else
65+
{
66+
return LOCALE_COMPARATOR;
67+
}
68+
}
69+
return new LocaleComparator( comparisonLocale, flag );
70+
}
71+
72+
public static Comparator<String> stringLocaleComparator( final Flag... flag )
73+
{
74+
return stringLocaleComparator( PwmConstants.DEFAULT_LOCALE, flag );
75+
}
76+
77+
public static Comparator<String> stringLocaleComparator( final Locale comparisonLocale, final Flag... flag )
78+
{
79+
if ( Objects.equals( comparisonLocale, PwmConstants.DEFAULT_LOCALE ) )
80+
{
81+
if ( JavaHelper.enumArrayContainsValue( flag, Flag.DefaultFirst ) )
82+
{
83+
return STR_COMPARATOR_DEFAULT_FIRST;
84+
}
85+
else
86+
{
87+
return STR_COMPARATOR;
88+
}
89+
}
90+
return new StringLocaleComparator( new LocaleComparator( comparisonLocale, flag ) );
91+
}
92+
93+
private static class LocaleComparator implements Comparator<Locale>, Serializable
94+
{
95+
private final boolean defaultFirst;
96+
private final Locale comparisonLocale;
97+
98+
LocaleComparator( final Locale comparisonLocale, final Flag... flag )
99+
{
100+
this.defaultFirst = JavaHelper.enumArrayContainsValue( flag, Flag.DefaultFirst );
101+
this.comparisonLocale = comparisonLocale;
102+
}
103+
104+
@Override
105+
public int compare( final Locale o1, final Locale o2 )
106+
{
107+
final String name1 = defaultFirst && Objects.equals( o1, PwmConstants.DEFAULT_LOCALE )
108+
? FIRST_POSITION_PSEUDO_VALUE
109+
: o1.getDisplayName( comparisonLocale );
110+
111+
final String name2 = defaultFirst && Objects.equals( o2, PwmConstants.DEFAULT_LOCALE )
112+
? FIRST_POSITION_PSEUDO_VALUE
113+
: o2.getDisplayName( comparisonLocale );
114+
115+
return name1.compareToIgnoreCase( name2 );
116+
}
117+
}
118+
119+
private static class StringLocaleComparator implements Comparator<String>, Serializable
120+
{
121+
private final Comparator<Locale> localeComparator;
122+
123+
StringLocaleComparator( final Comparator<Locale> localeComparator )
124+
{
125+
this.localeComparator = localeComparator;
126+
}
127+
128+
@Override
129+
public int compare( final String o1, final String o2 )
130+
{
131+
final Locale locale1 = LocaleHelper.parseLocaleString( o1 );
132+
final Locale locale2 = LocaleHelper.parseLocaleString( o2 );
133+
return localeComparator.compare( locale1, locale2 );
134+
}
135+
}
136+
}

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

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@
4141
import java.util.ArrayList;
4242
import java.util.Collection;
4343
import java.util.Collections;
44-
import java.util.Comparator;
4544
import java.util.Iterator;
4645
import java.util.LinkedHashMap;
4746
import java.util.List;
@@ -58,6 +57,7 @@ public class LocaleHelper
5857
{
5958
private static final PwmLogger LOGGER = PwmLogger.forClass( LocaleHelper.class );
6059

60+
6161
public enum TextDirection
6262
{
6363
rtl,
@@ -440,16 +440,6 @@ public static String getBrowserLocaleString( final Locale locale )
440440
: locale.toString().replace( "_", "-" );
441441
}
442442

443-
public static Comparator<Locale> localeComparator( final Locale comparisonLocale )
444-
{
445-
return ( o1, o2 ) ->
446-
{
447-
final String name1 = o1.getDisplayName( comparisonLocale );
448-
final String name2 = o2.getDisplayName( comparisonLocale );
449-
return name1.compareToIgnoreCase( name2 );
450-
};
451-
}
452-
453443
public static List<Locale> highLightedLocales()
454444
{
455445
final List<String> strValues = PwmConstants.HIGHLIGHT_LOCALES;

‎server/src/main/java/password/pwm/util/java/JavaHelper.java

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858
import java.util.EnumSet;
5959
import java.util.Enumeration;
6060
import java.util.LinkedHashSet;
61+
import java.util.List;
6162
import java.util.Map;
6263
import java.util.Objects;
6364
import java.util.Optional;
@@ -723,5 +724,28 @@ public static <K extends Enum<K>, V> EnumMap<K, V> copiedEnumMap( final Map<K, V
723724
}
724725
return returnMap;
725726
}
726-
727+
728+
public static <K, V> Map<K, V> stripNulls( final Map<K, V> input )
729+
{
730+
if ( input == null )
731+
{
732+
return Collections.emptyMap();
733+
}
734+
735+
return input.entrySet().stream()
736+
.filter( e -> e.getKey() != null && e.getValue() != null )
737+
.collect( Collectors.toUnmodifiableMap( Map.Entry::getKey, Map.Entry::getValue ) );
738+
}
739+
740+
public static <V> List<V> stripNulls( final List<V> input )
741+
{
742+
if ( input == null )
743+
{
744+
return Collections.emptyList();
745+
}
746+
747+
return input.stream()
748+
.filter( Objects::nonNull )
749+
.collect( Collectors.toUnmodifiableList() );
750+
}
727751
}

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

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
/*
2+
* Password Management Servlets (PWM)
3+
* http://www.pwm-project.org
4+
*
5+
* Copyright (c) 2006-2009 Novell, Inc.
6+
* Copyright (c) 2009-2021 The PWM Project
7+
*
8+
* Licensed under the Apache License, Version 2.0 (the "License");
9+
* you may not use this file except in compliance with the License.
10+
* You may obtain a copy of the License at
11+
*
12+
* http://www.apache.org/licenses/LICENSE-2.0
13+
*
14+
* Unless required by applicable law or agreed to in writing, software
15+
* distributed under the License is distributed on an "AS IS" BASIS,
16+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
17+
* See the License for the specific language governing permissions and
18+
* limitations under the License.
19+
*/
20+
21+
package password.pwm.util.i18n;
22+
23+
import org.junit.Assert;
24+
import org.junit.Test;
25+
26+
import java.util.ArrayList;
27+
import java.util.List;
28+
29+
public class LocaleComparatorTest
30+
{
31+
@Test
32+
public void stringLocaleComparator()
33+
{
34+
final List<String> list = new ArrayList<>();
35+
list.add( "ja" );
36+
list.add( "en" );
37+
list.add( "br" );
38+
list.add( "fr" );
39+
40+
list.sort( LocaleComparators.stringLocaleComparator() );
41+
42+
Assert.assertEquals( "br", list.get( 0 ) );
43+
Assert.assertEquals( "en", list.get( 1 ) );
44+
Assert.assertEquals( "fr", list.get( 2 ) );
45+
Assert.assertEquals( "ja", list.get( 3 ) );
46+
47+
list.sort( LocaleComparators.stringLocaleComparator( LocaleComparators.Flag.DefaultFirst ) );
48+
49+
// default (english) first
50+
Assert.assertEquals( "en", list.get( 0 ) );
51+
Assert.assertEquals( "br", list.get( 1 ) );
52+
Assert.assertEquals( "fr", list.get( 2 ) );
53+
Assert.assertEquals( "ja", list.get( 3 ) );
54+
}
55+
}

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -365,7 +365,11 @@ ChallengeSettingHandler.toggleAdminDefinedRow = function(toggleElement,inputID,k
365365
};
366366

367367
ChallengeSettingHandler.deleteRow = function(keyName, localeKey, rowName) {
368+
var questionText = PWM_VAR['clientSettingCache'][keyName][localeKey][rowName]['text'];
369+
var adminDefined = PWM_VAR['clientSettingCache'][keyName][localeKey][rowName]['adminDefined'];
370+
var output = (adminDefined ? 'the question "' + questionText + '"': 'the [User Defined] question?');
368371
PWM_MAIN.showConfirmDialog({
372+
text: 'Are you sure you want to remove ' + output,
369373
okAction:function(){
370374
PWM_MAIN.showWaitDialog({loadFunction:function(){
371375
delete PWM_VAR['clientSettingCache'][keyName][localeKey][rowName];

0 commit comments

Comments
 (0)