Skip to content

Commit a6a1fe4

Browse files
slobodanadamovicslobodanadamovic
committed
testing and refactoring
1 parent 6f28681 commit a6a1fe4

File tree

3 files changed

+106
-46
lines changed

3 files changed

+106
-46
lines changed

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlAttributes.java‎

Lines changed: 29 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,6 @@ public class SamlAttributes implements Releasable {
3030
private final List<SamlAttribute> attributes;
3131
private final List<SamlSecureAttribute> secureAttributes;
3232

33-
SamlAttributes(SamlNameId name, String session, List<SamlAttribute> attributes) {
34-
this(name, session, attributes, List.of());
35-
}
36-
3733
SamlAttributes(SamlNameId name, String session, List<SamlAttribute> attributes, List<SamlSecureAttribute> secureAttributes) {
3834
this.name = name;
3935
this.session = session;
@@ -103,10 +99,32 @@ public void close() {
10399
IOUtils.closeWhileHandlingException(secureAttributes);
104100
}
105101

106-
static class SamlAttribute {
102+
abstract static class AbstractSamlAttribute<T> {
103+
107104
final String name;
108105
final String friendlyName;
109-
final List<String> values;
106+
final List<T> values;
107+
108+
protected AbstractSamlAttribute(String name, @Nullable String friendlyName, List<T> values) {
109+
this.name = Objects.requireNonNull(name, "Attribute name cannot be null");
110+
this.friendlyName = friendlyName;
111+
this.values = values;
112+
}
113+
114+
String name() {
115+
return name;
116+
}
117+
118+
String friendlyName() {
119+
return friendlyName;
120+
}
121+
122+
List<T> values() {
123+
return values;
124+
}
125+
}
126+
127+
static class SamlAttribute extends AbstractSamlAttribute<String> {
110128

111129
SamlAttribute(Attribute attribute) {
112130
this(
@@ -117,9 +135,7 @@ static class SamlAttribute {
117135
}
118136

119137
SamlAttribute(String name, @Nullable String friendlyName, List<String> values) {
120-
this.name = Objects.requireNonNull(name, "Attribute name cannot be null");
121-
this.friendlyName = friendlyName;
122-
this.values = values;
138+
super(name, friendlyName, values);
123139
}
124140

125141
@Override
@@ -135,14 +151,10 @@ public String toString() {
135151
}
136152
}
137153

138-
static class SamlSecureAttribute implements Releasable {
139-
140-
final String name;
141-
final String friendlyName;
142-
final List<SecureString> values;
154+
static class SamlSecureAttribute extends AbstractSamlAttribute<SecureString> implements Releasable {
143155

144156
SamlSecureAttribute(Attribute attribute) {
145-
this(
157+
super(
146158
attribute.getName(),
147159
attribute.getFriendlyName(),
148160
attribute.getAttributeValues()
@@ -156,21 +168,7 @@ static class SamlSecureAttribute implements Releasable {
156168
}
157169

158170
SamlSecureAttribute(String name, @Nullable String friendlyName, List<SecureString> values) {
159-
this.name = Objects.requireNonNull(name, "Attribute name cannot be null");
160-
this.friendlyName = friendlyName;
161-
this.values = values;
162-
}
163-
164-
String name() {
165-
return name;
166-
}
167-
168-
String friendlyName() {
169-
return friendlyName;
170-
}
171-
172-
List<SecureString> values() {
173-
return values;
171+
super(name, friendlyName, values);
174172
}
175173

176174
@Override
@@ -181,7 +179,7 @@ public String toString() {
181179
} else {
182180
str.append(friendlyName).append('(').append(name).append(')');
183181
}
184-
str.append("=").append("::es_redacted::").append("(len=").append(values.size()).append(')');
182+
str.append("=").append("[::es_redacted::]").append("(len=").append(values.size()).append(')');
185183
return str.toString();
186184
}
187185

‎x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlAttributesTests.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
package org.elasticsearch.xpack.security.authc.saml;
99

10+
import org.elasticsearch.common.settings.SecureString;
1011
import org.hamcrest.Matchers;
1112
import org.opensaml.saml.saml2.core.NameID;
1213

@@ -29,6 +30,13 @@ public void testToString() {
2930
"groups",
3031
List.of("employees", "engineering", "managers")
3132
)
33+
),
34+
List.of(
35+
new SamlAttributes.SamlSecureAttribute(
36+
"urn:oid:0.9.2342.19200300.100.1.3",
37+
"mail",
38+
List.of(new SecureString("[email protected]".toCharArray()))
39+
)
3240
)
3341
);
3442
assertThat(
@@ -45,6 +53,9 @@ public void testToString() {
4553
+ ", "
4654
+ "groups(urn:oid:1.3.6.1.4.1.5923.1.5.1.1)=[employees, engineering, managers](len=3)"
4755
+ "]}"
56+
+ "{["
57+
+ "mail(urn:oid:0.9.2342.19200300.100.1.3)=[::es_redacted::](len=1)"
58+
+ "]}"
4859
)
4960
);
5061
}

‎x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlRealmTests.java‎

Lines changed: 66 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import org.elasticsearch.action.ActionListener;
1111
import org.elasticsearch.action.support.PlainActionFuture;
1212
import org.elasticsearch.common.settings.MockSecureSettings;
13+
import org.elasticsearch.common.settings.SecureString;
1314
import org.elasticsearch.common.settings.Settings;
1415
import org.elasticsearch.common.settings.SettingsException;
1516
import org.elasticsearch.common.ssl.PemUtils;
@@ -72,6 +73,7 @@
7273
import java.util.Collections;
7374
import java.util.List;
7475
import java.util.Locale;
76+
import java.util.Map;
7577
import java.util.Set;
7678
import java.util.concurrent.CountDownLatch;
7779
import java.util.concurrent.TimeUnit;
@@ -83,6 +85,7 @@
8385
import static org.elasticsearch.core.Strings.format;
8486
import static org.elasticsearch.test.ActionListenerUtils.anyActionListener;
8587
import static org.elasticsearch.test.TestMatchers.throwableWithMessage;
88+
import static org.elasticsearch.xpack.security.authc.saml.SamlRealm.SECURE_ATTRIBUTES_METADATA;
8689
import static org.hamcrest.Matchers.arrayContainingInAnyOrder;
8790
import static org.hamcrest.Matchers.contains;
8891
import static org.hamcrest.Matchers.containsInAnyOrder;
@@ -392,7 +395,8 @@ public void testAuthenticateWithEmptyRoleMapping() throws Exception {
392395
randomBoolean() ? REALM_NAME : null,
393396
testWithDelimiter ? List.of("STRIKE Team: Delta$shield") : Arrays.asList("avengers", "shield"),
394397
testWithDelimiter ? "$" : null,
395-
randomBoolean() ? List.of("superuser", "kibana_admin") : randomFrom(List.of(), null)
398+
randomBoolean() ? List.of("superuser", "kibana_admin") : randomFrom(List.of(), null),
399+
null
396400
);
397401
assertThat(result, notNullValue());
398402
assertThat(result.getStatus(), equalTo(AuthenticationResult.Status.SUCCESS));
@@ -419,7 +423,8 @@ public void testAuthenticateWithRoleMapping() throws Exception {
419423
authenticatingRealm,
420424
testWithDelimiter ? List.of("STRIKE Team: Delta$shield") : Arrays.asList("avengers", "shield"),
421425
testWithDelimiter ? "$" : null,
422-
rolesToExclude
426+
rolesToExclude,
427+
Map.of("top_secret", List.of("Batman's secret identity is Bruce Wayne!"))
423428
);
424429

425430
assertThat(result, notNullValue());
@@ -440,8 +445,24 @@ public void testAuthenticateWithRoleMapping() throws Exception {
440445
assertThat(result.getValue().metadata().get("saml_nameid"), equalTo(nameIdValue));
441446
assertThat(result.getValue().metadata().get("saml_uid"), instanceOf(Iterable.class));
442447
assertThat((Iterable<?>) result.getValue().metadata().get("saml_uid"), contains(uidValue));
448+
assertThat(result.getValue().metadata().get("saml_top_secret"), nullValue());
443449
}
444450

451+
assertThat(result.getMetadata(), notNullValue());
452+
assertThat(result.getMetadata().containsKey(SECURE_ATTRIBUTES_METADATA), is(true));
453+
@SuppressWarnings("unchecked")
454+
Map<String, List<SecureString>> secureAttributesMetadata = (Map<String, List<SecureString>>) result.getMetadata()
455+
.get(SECURE_ATTRIBUTES_METADATA);
456+
assertThat(secureAttributesMetadata, notNullValue());
457+
assertThat(secureAttributesMetadata.keySet(), containsInAnyOrder("top_secret"));
458+
List<SecureString> secretAttribute = secureAttributesMetadata.get("top_secret");
459+
assertThat(secretAttribute, notNullValue());
460+
assertThat(secretAttribute.size(), equalTo(1));
461+
assertEquals(
462+
"SecureString has already been closed",
463+
expectThrows(IllegalStateException.class, () -> secretAttribute.getFirst().getChars()).getMessage()
464+
);
465+
445466
assertThat(userData.get().getUsername(), equalTo(useNameId ? "clint.barton" : "cbarton"));
446467
if (testWithDelimiter) {
447468
assertThat(userData.get().getGroups(), containsInAnyOrder("STRIKE Team: Delta", "shield"));
@@ -533,6 +554,7 @@ private AuthenticationResult<User> performAuthentication(
533554
authenticatingRealm,
534555
groups,
535556
groupsDelimiter,
557+
null,
536558
null
537559
);
538560
}
@@ -546,7 +568,8 @@ private AuthenticationResult<User> performAuthentication(
546568
String authenticatingRealm,
547569
List<String> groups,
548570
String groupsDelimiter,
549-
List<String> rolesToExclude
571+
List<String> rolesToExclude,
572+
Map<String, List<String>> secureAttributes
550573
) throws Exception {
551574
final EntityDescriptor idp = mockIdp();
552575
final SpConfiguration sp = new SingleSamlSpConfiguration("<sp>", "https://saml/", null, null, null, Collections.emptyList());
@@ -610,6 +633,12 @@ private AuthenticationResult<User> performAuthentication(
610633
String.join(",", rolesToExclude)
611634
);
612635
}
636+
if (secureAttributes != null) {
637+
settingsBuilder.put(
638+
SingleSpSamlRealmSettings.getFullSettingKey(REALM_NAME, SamlRealmSettings.SECURE_ATTRIBUTES),
639+
String.join(",", secureAttributes.keySet())
640+
);
641+
}
613642
if (useAuthorizingRealm) {
614643
settingsBuilder.putList(
615644
RealmSettings.getFullSettingKey(
@@ -640,15 +669,35 @@ private AuthenticationResult<User> performAuthentication(
640669
final SamlAttributes attributes = new SamlAttributes(
641670
new SamlNameId(NameIDType.PERSISTENT, nameIdValue, idp.getEntityID(), sp.getEntityId(), null),
642671
randomAlphaOfLength(16),
643-
Arrays.asList(
672+
List.of(
644673
new SamlAttributes.SamlAttribute("urn:oid:0.9.2342.19200300.100.1.1", "uid", Collections.singletonList(uidValue)),
645674
new SamlAttributes.SamlAttribute("urn:oid:1.3.6.1.4.1.5923.1.5.1.1", "groups", groups),
646675
new SamlAttributes.SamlAttribute("urn:oid:0.9.2342.19200300.100.1.3", "mail", Arrays.asList("[email protected]"))
647-
)
676+
),
677+
secureAttributes == null
678+
? List.of()
679+
: secureAttributes.entrySet()
680+
.stream()
681+
.map(
682+
a -> new SamlAttributes.SamlSecureAttribute(a.getKey(), null, a.getValue().stream().map(SecureString::new).toList())
683+
)
684+
.toList()
648685
);
649686
when(authenticator.authenticate(token)).thenReturn(attributes);
650687

651-
final PlainActionFuture<AuthenticationResult<User>> future = new PlainActionFuture<>();
688+
final PlainActionFuture<AuthenticationResult<User>> future = new PlainActionFuture<>() {
689+
@Override
690+
public void onResponse(AuthenticationResult<User> result) {
691+
if (secureAttributes != null && result.isAuthenticated()) {
692+
assertThat(result.getMetadata(), notNullValue());
693+
assertThat(result.getMetadata().containsKey(SECURE_ATTRIBUTES_METADATA), is(true));
694+
@SuppressWarnings("unchecked")
695+
var metadata = (Map<String, List<SecureString>>) result.getMetadata().get(SECURE_ATTRIBUTES_METADATA);
696+
secureAttributes.forEach((name, value) -> assertThat(metadata.get(name), equalTo(value)));
697+
}
698+
super.onResponse(result);
699+
}
700+
};
652701
realm.authenticate(token, future);
653702
return future.get();
654703
}
@@ -725,13 +774,14 @@ private List<String> performAttributeSelectionWithSplit(String delimiter, String
725774
final SamlAttributes attributes = new SamlAttributes(
726775
new SamlNameId(NameIDType.TRANSIENT, randomAlphaOfLength(24), null, null, null),
727776
randomAlphaOfLength(16),
728-
Collections.singletonList(
777+
List.of(
729778
new SamlAttributes.SamlAttribute(
730779
"departments",
731780
"departments",
732781
Collections.singletonList(String.join(delimiter, returnedGroups))
733782
)
734-
)
783+
),
784+
List.of()
735785
);
736786
return parser.getAttribute(attributes);
737787
}
@@ -796,13 +846,14 @@ public void testAttributeSelectionWithSplitAndListThrowsSecurityException() {
796846
final SamlAttributes attributes = new SamlAttributes(
797847
new SamlNameId(NameIDType.TRANSIENT, randomAlphaOfLength(24), null, null, null),
798848
randomAlphaOfLength(16),
799-
Collections.singletonList(
849+
List.of(
800850
new SamlAttributes.SamlAttribute(
801851
"departments",
802852
"departments",
803853
List.of("engineering", String.join(delimiter, "elasticsearch-admins", "employees"))
804854
)
805-
)
855+
),
856+
List.of()
806857
);
807858

808859
ElasticsearchSecurityException securityException = expectThrows(
@@ -828,13 +879,14 @@ public void testAttributeSelectionWithRegex() {
828879
final SamlAttributes attributes = new SamlAttributes(
829880
new SamlNameId(NameIDType.TRANSIENT, randomAlphaOfLength(24), null, null, null),
830881
randomAlphaOfLength(16),
831-
Collections.singletonList(
882+
List.of(
832883
new SamlAttributes.SamlAttribute(
833884
"urn:oid:0.9.2342.19200300.100.1.3",
834885
"mail",
835886
Arrays.asList("[email protected]", "[email protected]", "[email protected]")
836887
)
837-
)
888+
),
889+
List.of()
838890
);
839891

840892
final List<String> strings = parser.getAttribute(attributes);
@@ -952,9 +1004,8 @@ public void testNonMatchingPrincipalPatternThrowsSamlException() throws Exceptio
9521004
final SamlAttributes attributes = new SamlAttributes(
9531005
new SamlNameId(NameIDType.TRANSIENT, randomAlphaOfLength(12), null, null, null),
9541006
randomAlphaOfLength(16),
955-
Collections.singletonList(
956-
new SamlAttributes.SamlAttribute("urn:oid:0.9.2342.19200300.100.1.3", "mail", Collections.singletonList(mail))
957-
)
1007+
List.of(new SamlAttributes.SamlAttribute("urn:oid:0.9.2342.19200300.100.1.3", "mail", Collections.singletonList(mail))),
1008+
List.of()
9581009
);
9591010
when(authenticator.authenticate(token)).thenReturn(attributes);
9601011

0 commit comments

Comments
 (0)