Treat roles as a SortedSet (#58988)

The Saml SP document stored the role mapping in a Set, but this made
the order in XContent inconsistent. This switched it to use a TreeSet.

Resolves: #54733
Backport of: #55201
This commit is contained in:
Tim Vernum 2020-07-03 13:40:58 +10:00 committed by GitHub
parent 65645217bc
commit 1133c29ce9
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 6 additions and 7 deletions

View File

@ -41,11 +41,11 @@ import java.util.HashSet;
import java.util.List; import java.util.List;
import java.util.Objects; import java.util.Objects;
import java.util.Set; import java.util.Set;
import java.util.SortedSet;
import java.util.TreeSet;
import java.util.function.BiConsumer; import java.util.function.BiConsumer;
import java.util.stream.Collectors; import java.util.stream.Collectors;
import static org.elasticsearch.common.collect.Set.copyOf;
/** /**
* This class models the storage of a {@link SamlServiceProvider} as an Elasticsearch document. * This class models the storage of a {@link SamlServiceProvider} as an Elasticsearch document.
*/ */
@ -57,14 +57,15 @@ public class SamlServiceProviderDocument implements ToXContentObject, Writeable
public static class Privileges { public static class Privileges {
public String resource; public String resource;
public Set<String> rolePatterns = Collections.emptySet(); // we use a sorted set so that the order is consistent in XContent APIs
public SortedSet<String> rolePatterns = new TreeSet<>();
public void setResource(String resource) { public void setResource(String resource) {
this.resource = resource; this.resource = resource;
} }
public void setRolePatterns(Collection<String> rolePatterns) { public void setRolePatterns(Collection<String> rolePatterns) {
this.rolePatterns = copyOf(rolePatterns); this.rolePatterns = new TreeSet<>(rolePatterns);
} }
@Override @Override
@ -268,7 +269,7 @@ public class SamlServiceProviderDocument implements ToXContentObject, Writeable
authenticationExpiryMillis = in.readOptionalVLong(); authenticationExpiryMillis = in.readOptionalVLong();
privileges.resource = in.readString(); privileges.resource = in.readString();
privileges.rolePatterns = in.readSet(StreamInput::readString); privileges.rolePatterns = new TreeSet<>(in.readSet(StreamInput::readString));
attributeNames.principal = in.readString(); attributeNames.principal = in.readString();
attributeNames.email = in.readOptionalString(); attributeNames.email = in.readOptionalString();

View File

@ -69,7 +69,6 @@ public class SamlServiceProviderDocumentTests extends IdpSamlTestCase {
assertThat(assertXContentRoundTrip(doc2), equalTo(doc1)); assertThat(assertXContentRoundTrip(doc2), equalTo(doc1));
} }
@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/54733")
public void testXContentRoundTripWithAllFields() throws Exception { public void testXContentRoundTripWithAllFields() throws Exception {
final SamlServiceProviderDocument doc1 = createFullDocument(); final SamlServiceProviderDocument doc1 = createFullDocument();
final SamlServiceProviderDocument doc2 = assertXContentRoundTrip(doc1); final SamlServiceProviderDocument doc2 = assertXContentRoundTrip(doc1);
@ -82,7 +81,6 @@ public class SamlServiceProviderDocumentTests extends IdpSamlTestCase {
assertThat(assertSerializationRoundTrip(doc2), equalTo(doc1)); assertThat(assertSerializationRoundTrip(doc2), equalTo(doc1));
} }
@AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/54733")
public void testStreamRoundTripWithAllFields() throws Exception { public void testStreamRoundTripWithAllFields() throws Exception {
final SamlServiceProviderDocument doc1 = createFullDocument(); final SamlServiceProviderDocument doc1 = createFullDocument();
final SamlServiceProviderDocument doc2 = assertXContentRoundTrip(doc1); final SamlServiceProviderDocument doc2 = assertXContentRoundTrip(doc1);