8000 KEYCLOAK-19866 Fix user-defined-/anyType parsing and introduce XPathAttributeMapper by KnauerSecunet · Pull Request #10650 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

KEYCLOAK-19866 Fix user-defined-/anyType parsing and introduce XPathAttributeMapper #10650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hmlnarik merged 1 commit into from
Aug 3, 2022
Merged

KEYCLOAK-19866 Fix user-defined-/anyType parsing and introduce XPathAttributeMapper #10650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 57 additions & 17 deletions ...va/org/keycloak/saml/processing/core/parsers/saml/assertion/SAMLAttributeValueParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,14 @@
*/
package org.keycloak.saml.processing.core.parsers.saml.assertion;

import java.util.Deque;
import java.util.HashMap;
import java.util.Iterator;
import java.util.LinkedList;
import java.util.Map;
import javax.xml.stream.XMLEventFactory;
import javax.xml.stream.XMLStreamException;
import javax.xml.stream.events.Namespace;
import org.keycloak.saml.common.PicketLinkLogger;
import org.keycloak.saml.common.PicketLinkLoggerFactory;
import org.keycloak.saml.common.constants.JBossSAMLURIConstants;
Expand Down Expand Up @@ -46,6 +54,8 @@ public class SAMLAttributeValueParser implements StaxParser {
private static final QName NIL = new QName(JBossSAMLURIConstants.XSI_NSURI.get(), "nil", JBossSAMLURIConstants.XSI_PREFIX.get());
private static final QName XSI_TYPE = new QName(JBossSAMLURIConstants.XSI_NSURI.get(), "type", JBossSAMLURIConstants.XSI_PREFIX.get());

private static final ThreadLocal<XMLEventFactory> XML_EVENT_FACTORY = ThreadLocal.withInitial(XMLEventFactory::newInstance);

public static SAMLAttributeValueParser getInstance() {
return INSTANCE;
}
Expand Down Expand Up @@ -88,15 +98,15 @@ public Object parse(XMLEventReader xmlEventReader) throws ParsingException {
}

// when no type attribute assigned -> assume anyType
< 10BC8 /td> return parseAnyTypeAsString(xmlEventReader);
return parseAsString(xmlEventReader);
}

// RK Added an additional type check for base64Binary type as calheers is passing this type
String typeValue = StaxParserUtil.getAttributeValue(type);
if (typeValue.contains(":string")) {
return StaxParserUtil.getElementText(xmlEventReader);
} else if (typeValue.contains(":anyType")) {
return parseAnyTypeAsString(xmlEventReader);
return parseAsString(xmlEventReader);
} else if(typeValue.contains(":base64Binary")){
return StaxParserUtil.getElementText(xmlEventReader);
} else if(typeValue.contains(":date")){
Expand All @@ -105,33 +115,29 @@ public Object parse(XMLEventReader xmlEventReader) throws ParsingException {
return StaxParserUtil.getElementText(xmlEventReader);
}

// KEYCLOAK-18417: Simply ignore unknown types
logger.debug("Skipping attribute value of unsupported type " + typeValue);
StaxParserUtil.bypassElementBlock(xmlEventReader);
return null;
return parseAsString(xmlEventReader);
}

public static String parseAnyTypeAsString(XMLEventReader xmlEventReader) throws ParsingException {
private static String parseAsString(XMLEventReader xmlEventReader) throws ParsingException {
try {
XMLEvent event = xmlEventReader.peek();
if (event.isStartElement()) {
event = xmlEventReader.nextTag();
if (xmlEventReader.peek().isStartElement()) {
StringWriter sw = new StringWriter();
XMLEventWriter writer = XMLOutputFactory.newInstance().createXMLEventWriter(sw);
//QName tagName = event.asStartElement().getName();
int tagLevel = 1;
do {
Deque<Map<String, String>> definedNamespaces = new LinkedList<>();
int tagLevel = 0;
while (xmlEventReader.hasNext() && (tagLevel > 0 || !xmlEventReader.peek().isEndElement())) {
XMLEvent event = (XMLEvent) xmlEventReader.next();
writer.add(event);
event = (XMLEvent) xmlEventReader.next();
if (event.isStartElement()) {
definedNamespaces.push(addNamespaceWhenMissing(definedNamespaces, writer, event.asStartElement()));
tagLevel++;
}
if (event.isEndElement()) {
definedNamespaces.pop();
tagLevel--;
}
} while (xmlEventReader.hasNext() && tagLevel > 0);
writer.add(event);
writer.flush();
}
writer.close();
return sw.toString();
} else {
return StaxParserUtil.getElementText(xmlEventReader);
Expand All @@ -141,4 +147,38 @@ public static String parseAnyTypeAsString(XMLEventReader xmlEventReader) throws
}
}

private static Map<String, String> addNamespaceWhenMissing(Deque<Map<String, String>> definedNamespaces, XMLEventWriter writer,
StartElement startElement) throws XMLStreamException {

final Map<String, String> necessaryNamespaces = new HashMap<>();
// Namespace in tag
if (startElement.getName().getPrefix() != null && !startElement.getName().getPrefix().isEmpty()) {
necessaryNamespaces.put(startElement.getName().getPrefix(), startElement.getName().getNamespaceURI());
}
// Namespaces in attributes
final Iterator<Attribute> attributes = startElement.getAttributes();
while (attributes.hasNext()) {
final Attribute attribute = attributes.next();
if (attribute.getName().getPrefix() != null && !attribute.getName().getPrefix().isEmpty()) {
necessaryNamespaces.put(attribute.getName().getPrefix(), attribute.getName().getNamespaceURI());
}
}

// Already contained in stack
necessaryNamespaces.entrySet().removeIf(nn -> definedNamespaces.stream().anyMatch(dn -> dn.containsKey(nn.getKey())));
// Contained in current element
Iterator<Namespace> namespaces = startElement.getNamespaces();
while (namespaces.hasNext() && !necessaryNamespaces.isEmpty()) {
necessaryNamespaces.remove(namespaces.next().getPrefix());
}

// Add all remaining necessaryNamespaces
if (!necessaryNamespaces.isEmpty()) {
XMLEventFactory xmlEventFactory = XML_EVENT_FACTORY.get();
for (Map.Entry<String, String> entry : necessaryNamespaces.entrySet()) {
writer.add(xmlEventFactory.createNamespace(entry.getKey(), entry.getValue()));
}
}
return necessaryNamespaces;
}
}
130 changes: 94 additions & 36 deletions ...est/java/org/keycloak/saml/processing/core/parsers/saml/SAMLAttributeValueParserTest.java
View file
Open in desktop
8
Original file line number Diff line number Diff line change
@@ -1,55 +1,113 @@
package org.keycloak.saml.processing.core.parsers.saml;

import javax.xml.stream.events.XMLEvent;
import javax.xml.stream.XMLEventReader;
import java.io.ByteArrayInputStream;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import org.junit.Assert;
import org.junit.Test;
import org.keycloak.saml.common.parsers.AbstractParser;
import org.keycloak.saml.processing.core.parsers.saml.assertion.SAMLAttributeValueParser;

import javax.xml.stream.XMLEventReader;
public class SAMLAttributeValueParserTest {

import java.io.ByteArrayInputStream;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
private static final String XML_DOC = "Some Text";

public class SAMLAttributeValueParserTest {
private static final String XML_DOC_NESTED_ELEMENTS =
"<%1$sStreet%2$s>Zillestraße</%1$sStreet><%1$sHouseNumber%2$s>17</%1$sHouseNumber>"
+ "<%1$sZipCode%2$s>10585</%1$sZipCode><%1$sCity%2$s>Berlin</%1$sCity><%1$sCountry%2$s>DE</%1$sCountry>";

private static final String XML_DOC =
"<saml2:Attribute xmlns:saml2=\"urn:oasis:names:tc:SAML:2.0:assertion\"\n>"
+ " <saml2:AttributeValue xmlns:myCustomType=\"http://www.whatever.de/schema/myCustomType/saml/extensions\"\n"
+ " xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\" xsi:type=\"myCustomType:Something\">\n"
+ " Some Text\n"
+ " </saml2:AttributeValue>\n"
+ "</saml2:Attribute>";

private static final String XML_DOC_WITH_NESTED_ELEMENTS =
"<saml2:Attribute xmlns:saml2=\"urn:oasis:names:tc:SAML:2.0:assertion\"\n>"
+ " <saml2:AttributeValue xmlns:myCustomType=\"http://www.whatever.de/schema/myCustomType/saml/extensions\"\n"
+ " xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\" xsi:type=\"myCustomType:AddressType\">\n"
+ " <myCustomType:Street>Zillestraße</myCustomType:Street>\n"
+ " <myCustomType:HouseNumber>17</myCustomType:HouseNumber>\n"
+ " <myCustomType:ZipCode>10585</myCustomType:ZipCode>\n"
+ " <myCustomType:City>Berlin</myCustomType:City>\n"
+ " <myCustomType:Country>DE</myCustomType:Country>\n"
+ " </saml2:AttributeValue>\n"
+ "</saml2:Attribute>";

@Test
public void parsesAttributeValueElementWithCustomTypes_ReturnsNull() throws Exception {
InputStream input = new ByteArrayInputStream(XML_DOC.getBytes(StandardCharsets.UTF_8));
XMLEventReader xmlEventReader = AbstractParser.createEventReader(input);
xmlEventReader.nextEvent();
final Object attributeValue = SAMLAttributeValueParser.getInstance().parse(xmlEventReader);
private static final String XML_DOC_SINGLE_ELEMENT =
"<%1$sAddressType%2$s>" + String.format(XML_DOC_NESTED_ELEMENTS, "%1$s", "") + "</%1$sAddressType>";

private static final String XML_DOC_NESTED_WITHOUT_PREFIX_AND_NAMESPACE = String.format(XML_DOC_NESTED_ELEMENTS, "", "");

Assert.assertNull(attributeValue);
@Test
public void parsesAttributeValueUserType() throws Exception {
Object actualAttributeValue = parseAttributeValue("xsi:type=\"myCustomType:Something\"", "\n " + XML_DOC + "\n ");
Assert.assertEquals(XML_DOC, actualAttributeValue);
}

@Test
public void parsesAttributeValueElementWithSubElements_ReturnsNull() throws Exception {
InputStream input = new ByteArrayInputStream(XML_DOC_WITH_NESTED_ELEMENTS.getBytes(StandardCharsets.UTF_8));
public void parsesAttributeValueUserTypeWithNamespace() throws Exception {
Object actualAttributeValue = parseAttributeValue(
"xmlns:myCustomType=\"http://my.custom.de/schema/saml/extensions\" xsi:type=\"myCustomType:Something\"",
"\n " + XML_DOC + "\n ");
Assert.assertEquals(XML_DOC, actualAttributeValue);
}

@Test
public void parseAttributeValueAnyType() throws Exception {
Object actualAttributeValue = parseAttributeValue("xsi:type=\"xs:anyType\"", XML_DOC_NESTED_WITHOUT_PREFIX_AND_NAMESPACE);
Assert.assertEquals(XML_DOC_NESTED_WITHOUT_PREFIX_AND_NAMESPACE, actualAttributeValue);
}

@Test
public void parsesAttributeValueUserTypeWithSingleElements() throws Exception {
final String xmlDoc = String.format(XML_DOC_SINGLE_ELEMENT, "", "");
Object actualAttributeValue = parseAttributeValue("xsi:type=\"AddressType\"", xmlDoc);
Assert.assertEquals(xmlDoc, actualAttributeValue);
}

@Test
public void parsesAttributeValueUserTypeWithSingleElementsAndNamespace() throws Exception {
Object actualAttributeValue = parseAttributeValue(
"xmlns:myCustomType=\"http://my.custom.de/schema/saml/extensions\" xsi:type=\"myCustomType:AddressType\"",
String.format(XML_DOC_SINGLE_ELEMENT, "myCustomType:", ""));
Assert.assertEquals(String.format(XML_DOC_SINGLE_ELEMENT, "myCustomType:",
" xmlns:myCustomType=\"http://my.custom.de/schema/saml/extensions\""),
actualAttributeValue);
}

@Test
public void parsesAttributeValueUserTypeWithNestedElements() throws Exception {
Object actualAttributeValue = parseAttributeValue("xsi:type=\"AddressType\"", XML_DOC_NESTED_WITHOUT_PREFIX_AND_NAMESPACE);
Assert.assertEquals(XML_DOC_NESTED_WITHOUT_PREFIX_AND_NAMESPACE, actualAttributeValue);
}

@Test
public void parsesAttributeValueUserTypeWithNestedElementsAndNamespace() throws Exception {
Object actualAttributeValue = parseAttributeValue(
"xmlns:myCustomType=\"http://my.custom.de/schema/saml/extensions\" xsi:type=\"myCustomType:AddressType\"",
String.format(XML_DOC_NESTED_ELEMENTS, "myCustomType:", ""));
Assert.assertEquals(String.format(XML_DOC_NESTED_ELEMENTS, "myCustomType:",
" xmlns:myCustomType=\"http://my.custom.de/schema/saml/extensions\""),
actualAttributeValue);
}

@Test
public void parsesAttributeValueUserTypeWithAttributeAndInnerNamespace() throws Exception {
String xmlDocPayload = "<%1$sAddress myCustomType3:restriction=\"one-way\"%3$s><%1$sStreet>Zillestraße</%1$sStreet><%2$sHouseNumber%4$s>17"
+ "</%2$sHouseNumber><myCustomType4:ZipCode xmlns:myCustomType4=\"http://my.custom4.de/schema/saml/extensions\">10585"
+ "</myCustomType4:ZipCode><City xmlns=\"http://my.custom4.de/schema/saml/extensions\">Berlin</City></%1$sAddress>";
String namespace1 = "xmlns:myCustomType1=\"http://my.custom1.de/schema/saml/extensions\"";
String namespace2 = "xmlns:myCustomType2=\"http://my.custom2.de/schema/saml/extensions\"";
String namespace3 = "xmlns:myCustomType3=\"http://my.custom3.de/schema/saml/extensions\"";
String namespace4 = "xmlns:myCustomType4=\"http://my.custom4.de/schema/saml/extensions\"";
Object actualAttributeValue = parseAttributeValue(
namespace1 + " " + namespace2 + " " + namespace3 + " " + namespace4 + " xsi:type=\"myCustomType1:AddressType\"",
String.format(xmlDocPayload, "myCustomType1:", "myCustomType2:", "", ""));
Assert.assertEquals(String.format(xmlDocPayload, "myCustomType1:", "myCustomType2:", " " + namespace3 + " " + namespace1, " " + namespace2),
actualAttributeValue);
}

private Object parseAttributeValue(String namespaceAndType, String payload) throws Exception {
InputStream input = new ByteArrayInputStream(asAttribute(namespaceAndType, payload).getBytes(StandardCharsets.UTF_8));
XMLEventReader xmlEventReader = AbstractParser.createEventReader(input);
xmlEventReader.nextEvent();
final Object attributeValue = SAMLAttributeValueParser.getInstance().parse(xmlEventReader);
Object attributeValue = SAMLAttributeValueParser.getInstance().parse(xmlEventReader);

XMLEvent nextXmlEvent = xmlEventReader.nextEvent();
Assert.assertTrue(nextXmlEvent.isEndElement());
final String nextName = nextXmlEvent.asEndElement().getName().getLocalPart();
Assert.assertTrue("Attribute".equals(nextName) || "AttributeValue".equals(nextName)); // both are valid

return attributeValue;
}

Assert.assertNull(attributeValue);
private String asAttribute(String namespaceAndType, String payload) {
return "<saml2:Attribute xmlns:saml2=\"urn:oasis:names:tc:SAML:2.0:assertion\"><saml2:AttributeValue xmlns:xsi=\"http://www.w3"
+ ".org/2001/XMLSchema-instance\" " + namespaceAndType + ">" + payload + "</saml2:AttributeValue></saml2:Attribute>";
}
}
17 changes: 13 additions & 4 deletions saml-core/src/test/java/org/keycloak/saml/processing/core/parsers/saml/SAMLParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -756,7 +756,7 @@ public void testAuthnRequestInvalidNamespace() throws Exception {
@Test
public void testInvalidEndElement() throws Exception {
thrown.expect(ParsingException.class);
// see KEYCLOAK-7444
// see KEYCLOAK-7444
thrown.expectMessage(containsString("NameIDFormat"));

assertParsed("saml20-entity-descriptor-idp-invalid-end-element.xml", EntityDescriptorType.class);
Expand Down Expand Up @@ -1003,7 +1003,7 @@ public void testSaml20AssertionExample() throws Exception {
AssertionType assertion = assertParsed("saml20-assertion-example.xml", AssertionType.class);

AttributeStatementType attributeStatementType = assertion.getAttributeStatements().iterator().next();
assertThat(attributeStatementType.getAttributes(), hasSize(9));
assertThat(attributeStatementType.getAttributes(), hasSize(12));

for (AttributeStatementType.ASTChoiceType choiceType: attributeStatementType.getAttributes()) {
AttributeType attr = choiceType.getAttribute();
Expand All @@ -1012,7 +1012,7 @@ public void testSaml20AssertionExample() throws Exception {
// test selected attributes
switch (attrName) {
case "portal_id":
assertEquals(value, "060D00000000SHZ");
assertEquals("060D00000000SHZ", value);
break;
case "organization_id":
assertThat(value, instanceOf(String.class));
Expand All @@ -1028,6 +1028,9 @@ public void testSaml20AssertionExample() throws Exception {
case "anytype_no_xml_test":
assertThat(value, is((Object) "value_no_xml"));
break;
case "anytype_xml_fragment":
assertThat(value, is((Object) "<elem1>Foo</elem1><elem2>Bar</elem2>"));
break;
case "logouturl":
assertThat(value, is((Object) "http://www.salesforce.com/security/del_auth/SsoLogoutPage.html"));
break;
Expand All @@ -1037,6 +1040,12 @@ public void testSaml20AssertionExample() throws Exception {
case "status":
assertThat(value, is((Object) "<status><code><status>XYZ</status></code></status>"));
break;
case "userDefined":
assertThat(value, is((Object) "<A><B>Foo</B><C>Bar</C></A>"));
break;
case "userDefinedFragmentWithNamespace":
assertThat(value, is((Object) "<myPrefix:B xmlns:myPrefix=\"urn:myNamespace\">Foo</myPrefix:B><myPrefix:C xmlns:myPrefix=\"urn:myNamespace\">Bar</myPrefix:C>"));
break;
default:
break;
}
Expand Down Expand Up @@ -1130,7 +1139,7 @@ public void testSaml20AssertionsAdviceTag() throws Exception {
assertThat(ac.getSequence(), notNullValue());

assertThat(ac.getSequence().getClassRef().getValue(), is(JBossSAMLURIConstants.AC_UNSPECIFIED.getUri()));

assertThat(ac.getSequence(), notNullValue());
assertThat(ac.getSequence().getAuthnContextDecl(), notNullValue());
assertThat(ac.getSequence().getAuthnContextDecl().getValue(), instanceOf(Element.class));
Expand Down
14 changes: 14 additions & 0 deletions ...est/resources/org/keycloak/saml/processing/core/parsers/saml/saml20-assertion-example.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,13 @@
<saml:AttributeValue>value_no_xml</saml:AttributeValue>
</saml:Attribute>

<saml:Attribute Name="anytype_xml_fragment">
<saml:AttributeValue xsi:type="xs:anyType">
<elem1>Foo</elem1>
<elem2>Bar</elem2>
</saml:AttributeValue>
</saml:Attribute>

<saml:Attribute Name="ssostartpage"
NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:unspecified">

Expand All @@ -138,6 +145,13 @@
<saml:AttributeValue xsi:nil="true" xsi:type="xs:anyType"/>
</saml:Attribute>

<saml:Attribute Name="userDefined">
<saml:AttributeValue xsi:type="MyType"><A><B>Foo</B><C>Bar</C></A></saml:AttributeValue>
</saml:Attribute>

<saml:Attribute Name="userDefinedFragmentWithNamespace">
<saml:AttributeValue xmlns:myPrefix="urn:myNamespace" xsi:type="myPrefix:MyType"><myPrefix:B>Foo</myPrefix:B><myPrefix:C>Bar</myPrefix:C></saml:AttributeValue>
</saml:Attribute>

</saml:AttributeStatement>
</saml:Assertion>
Loading
0