From ad9210a7a7a6b4e59df500349ea8ea829cd1aacd Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Wed, 4 Jan 2017 15:47:16 +0100 Subject: [PATCH] KEYCLOAK-4148 Prevent unnecessary deserialization when supported ... and gain another ~ 5-10 % --- .../saml/common/parsers/AbstractParser.java | 65 +++++++++++++------ .../saml/common/util/StaxParserUtil.java | 31 +++++++++ .../api/saml/v2/request/SAML2Request.java | 6 +- .../api/saml/v2/response/SAML2Response.java | 9 ++- .../testsuite/saml/SamlEcpProfileTest.java | 4 +- 5 files changed, 86 insertions(+), 29 deletions(-) diff --git a/saml-core/src/main/java/org/keycloak/saml/common/parsers/AbstractParser.java b/saml-core/src/main/java/org/keycloak/saml/common/parsers/AbstractParser.java index fd12b63738..7e7daa5f0b 100755 --- a/saml-core/src/main/java/org/keycloak/saml/common/parsers/AbstractParser.java +++ b/saml-core/src/main/java/org/keycloak/saml/common/parsers/AbstractParser.java @@ -33,6 +33,9 @@ import javax.xml.stream.events.Characters; import javax.xml.stream.events.XMLEvent; import javax.xml.stream.util.EventReaderDelegate; import java.io.InputStream; +import javax.xml.transform.Source; +import javax.xml.transform.dom.DOMSource; +import org.w3c.dom.Node; /** * Base class for parsers @@ -49,28 +52,28 @@ public abstract class AbstractParser implements ParserNamespaceSupport { protected XMLInputFactory initialValue() { return getXMLInputFactory(); } - }; - /** - * Get the JAXP {@link XMLInputFactory} - * - * @return - */ - private static XMLInputFactory getXMLInputFactory() { - boolean tccl_jaxp = SystemPropertiesUtil.getSystemProperty(GeneralConstants.TCCL_JAXP, "false") - .equalsIgnoreCase("true"); - ClassLoader prevTCCL = SecurityActions.getTCCL(); - try { - if (tccl_jaxp) { - SecurityActions.setTCCL(AbstractParser.class.getClassLoader()); - } - return XMLInputFactory.newInstance(); - } finally { - if (tccl_jaxp) { - SecurityActions.setTCCL(prevTCCL); + /** + * Get the JAXP {@link XMLInputFactory} + * + * @return + */ + private XMLInputFactory getXMLInputFactory() { + boolean tccl_jaxp = SystemPropertiesUtil.getSystemProperty(GeneralConstants.TCCL_JAXP, "false") + .equalsIgnoreCase("true"); + ClassLoader prevTCCL = SecurityActions.getTCCL(); + try { + if (tccl_jaxp) { + SecurityActions.setTCCL(AbstractParser.class.getClassLoader()); + } + return XMLInputFactory.newInstance(); + } finally { + if (tccl_jaxp) { + SecurityActions.setTCCL(prevTCCL); + } } } - } + }; /** * Parse an InputStream for payload @@ -87,6 +90,15 @@ public abstract class AbstractParser implements ParserNamespaceSupport { return parse(xmlEventReader); } + public Object parse(Source source) throws ParsingException { + XMLEventReader xmlEventReader = createEventReader(source); + return parse(xmlEventReader); + } + + public Object parse(Node node) throws ParsingException { + return parse(new DOMSource(node)); + } + public XMLEventReader createEventReader(InputStream configStream) throws ParsingException { if (configStream == null) throw logger.nullArgumentError("InputStream"); @@ -102,6 +114,21 @@ public abstract class AbstractParser implements ParserNamespaceSupport { return xmlEventReader; } + public XMLEventReader createEventReader(Source source) throws ParsingException { + if (source == null) + throw logger.nullArgumentError("Source"); + + XMLEventReader xmlEventReader = StaxParserUtil.getXMLEventReader(source); + + try { + xmlEventReader = filterWhitespaces(xmlEventReader); + } catch (XMLStreamException e) { + throw logger.parserException(e); + } + + return xmlEventReader; + } + protected XMLEventReader filterWhitespaces(XMLEventReader xmlEventReader) throws XMLStreamException { XMLInputFactory xmlInputFactory = XML_INPUT_FACTORY.get(); diff --git a/saml-core/src/main/java/org/keycloak/saml/common/util/StaxParserUtil.java b/saml-core/src/main/java/org/keycloak/saml/common/util/StaxParserUtil.java index 1b06e9d707..f0f6e2b7ba 100755 --- a/saml-core/src/main/java/org/keycloak/saml/common/util/StaxParserUtil.java +++ b/saml-core/src/main/java/org/keycloak/saml/common/util/StaxParserUtil.java @@ -24,6 +24,8 @@ import org.keycloak.saml.common.constants.JBossSAMLConstants; import org.keycloak.saml.common.constants.JBossSAMLURIConstants; import org.keycloak.saml.common.exceptions.ConfigurationException; import org.keycloak.saml.common.exceptions.ParsingException; +import org.keycloak.saml.common.exceptions.ProcessingException; + import org.w3c.dom.Document; import org.w3c.dom.Element; @@ -47,6 +49,7 @@ import javax.xml.validation.Schema; import javax.xml.validation.SchemaFactory; import javax.xml.validation.Validator; import java.io.InputStream; +import java.util.concurrent.atomic.AtomicBoolean; /** * Utility for the stax based parser @@ -250,6 +253,34 @@ public class StaxParserUtil { return xmlEventReader; } + private static AtomicBoolean XML_EVENT_READER_ON_SOURCE_SUPPORTED = new AtomicBoolean(true); + + /** + * Get the XML event reader + * + * @param source + * + * @return + */ + public static XMLEventReader getXMLEventReader(Source source) { + XMLInputFactory xmlInputFactory = XML_INPUT_FACTORY.get(); + try { + if (XML_EVENT_READER_ON_SOURCE_SUPPORTED.get()) { + try { + // The following method is optional per specification + return xmlInputFactory.createXMLEventReader(source); + } catch (UnsupportedOperationException ex) { + XML_EVENT_READER_ON_SOURCE_SUPPORTED.set(false); + return getXMLEventReader(source); + } + } else { + return getXMLEventReader(DocumentUtil.getSourceAsStream(source)); + } + } catch (ConfigurationException | ProcessingException | XMLStreamException ex) { + throw new RuntimeException(ex); + } + } + /** * Given a {@code Location}, return a formatted string [lineNum,colNum] * diff --git a/saml-core/src/main/java/org/keycloak/saml/processing/api/saml/v2/request/SAML2Request.java b/saml-core/src/main/java/org/keycloak/saml/processing/api/saml/v2/request/SAML2Request.java index 9830482471..2bfa41f9db 100755 --- a/saml-core/src/main/java/org/keycloak/saml/processing/api/saml/v2/request/SAML2Request.java +++ b/saml-core/src/main/java/org/keycloak/saml/processing/api/saml/v2/request/SAML2Request.java @@ -165,7 +165,7 @@ public class SAML2Request { SAMLParser samlParser = new SAMLParser(); JAXPValidationUtil.checkSchemaValidation(samlDocument); - SAML2Object requestType = (SAML2Object) samlParser.parse(DocumentUtil.getNodeAsStream(samlDocument)); + SAML2Object requestType = (SAML2Object) samlParser.parse(samlDocument); samlDocumentHolder = new SAMLDocumentHolder(requestType, samlDocument); return requestType; @@ -192,7 +192,7 @@ public class SAML2Request { SAMLParser samlParser = new SAMLParser(); JAXPValidationUtil.checkSchemaValidation(samlDocument); - RequestAbstractType requestType = (RequestAbstractType) samlParser.parse(DocumentUtil.getNodeAsStream(samlDocument)); + RequestAbstractType requestType = (RequestAbstractType) samlParser.parse(samlDocument); samlDocumentHolder = new SAMLDocumentHolder(requestType, samlDocument); return requestType; @@ -220,7 +220,7 @@ public class SAML2Request { SAMLParser samlParser = new SAMLParser(); JAXPValidationUtil.checkSchemaValidation(samlDocument); - AuthnRequestType requestType = (AuthnRequestType) samlParser.parse(DocumentUtil.getNodeAsStream(samlDocument)); + AuthnRequestType requestType = (AuthnRequestType) samlParser.parse(samlDocument); samlDocumentHolder = new SAMLDocumentHolder(requestType, samlDocument); return requestType; } diff --git a/saml-core/src/main/java/org/keycloak/saml/processing/api/saml/v2/response/SAML2Response.java b/saml-core/src/main/java/org/keycloak/saml/processing/api/saml/v2/response/SAML2Response.java index 76155fe691..a650c7a25f 100755 --- a/saml-core/src/main/java/org/keycloak/saml/processing/api/saml/v2/response/SAML2Response.java +++ b/saml-core/src/main/java/org/keycloak/saml/processing/api/saml/v2/response/SAML2Response.java @@ -376,7 +376,7 @@ public class SAML2Response { SAMLParser samlParser = new SAMLParser(); JAXPValidationUtil.checkSchemaValidation(samlDocument); - return (EncryptedAssertionType) samlParser.parse(DocumentUtil.getNodeAsStream(samlDocument)); + return (EncryptedAssertionType) samlParser.parse(samlDocument); } @@ -398,7 +398,7 @@ public class SAML2Response { SAMLParser samlParser = new SAMLParser(); JAXPValidationUtil.checkSchemaValidation(samlDocument); - return (AssertionType) samlParser.parse(DocumentUtil.getNodeAsStream(samlDocument)); + return (AssertionType) samlParser.parse(samlDocument); } /** @@ -429,7 +429,7 @@ public class SAML2Response { SAMLParser samlParser = new SAMLParser(); JAXPValidationUtil.checkSchemaValidation(samlResponseDocument); - ResponseType responseType = (ResponseType) samlParser.parse(DocumentUtil.getNodeAsStream(samlResponseDocument)); + ResponseType responseType = (ResponseType) samlParser.parse(samlResponseDocument); samlDocumentHolder = new SAMLDocumentHolder(responseType, samlResponseDocument); return responseType; @@ -460,8 +460,7 @@ public class SAML2Response { SAMLParser samlParser = new SAMLParser(); JAXPValidationUtil.checkSchemaValidation(samlResponseDocument); - InputStream responseStream = DocumentUtil.getNodeAsStream(samlResponseDocument); - SAML2Object responseType = (SAML2Object) samlParser.parse(responseStream); + SAML2Object responseType = (SAML2Object) samlParser.parse(samlResponseDocument); samlDocumentHolder = new SAMLDocumentHolder(responseType, samlResponseDocument); return responseType; diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/saml/SamlEcpProfileTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/saml/SamlEcpProfileTest.java index 778085ca5e..bc6d316a72 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/saml/SamlEcpProfileTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/saml/SamlEcpProfileTest.java @@ -146,7 +146,7 @@ public class SamlEcpProfileTest { assertNotNull(samlResponse); - ResponseType responseType = (ResponseType) new SAMLParser().parse(DocumentUtil.getNodeAsStream(samlResponse)); + ResponseType responseType = (ResponseType) new SAMLParser().parse(samlResponse); StatusCodeType statusCode = responseType.getStatus().getStatusCode(); assertEquals(statusCode.getValue().toString(), JBossSAMLURIConstants.STATUS_SUCCESS.get()); @@ -229,7 +229,7 @@ public class SamlEcpProfileTest { assertNotNull(samlResponse); - StatusResponseType responseType = (StatusResponseType) new SAMLParser().parse(DocumentUtil.getNodeAsStream(samlResponse)); + StatusResponseType responseType = (StatusResponseType) new SAMLParser().parse(samlResponse); StatusCodeType statusCode = responseType.getStatus().getStatusCode(); assertNotEquals(statusCode.getStatusCode().getValue().toString(), JBossSAMLURIConstants.STATUS_SUCCESS.get());