From 221b312a72ccfb67c2d94236eeae41f265377d41 Mon Sep 17 00:00:00 2001 From: Tim van Dijen Date: Mon, 16 Dec 2024 20:56:44 +0100 Subject: [PATCH 1/2] Change of heart: offload schema validation to a trait+interface combo for easy re-use --- src/SchemaValidatableElementInterface.php | 23 ++++++++ src/SchemaValidatableElementTrait.php | 68 +++++++++++++++++++++++ 2 files changed, 91 insertions(+) create mode 100644 src/SchemaValidatableElementInterface.php create mode 100644 src/SchemaValidatableElementTrait.php diff --git a/src/SchemaValidatableElementInterface.php b/src/SchemaValidatableElementInterface.php new file mode 100644 index 0000000..82a3e31 --- /dev/null +++ b/src/SchemaValidatableElementInterface.php @@ -0,0 +1,23 @@ +schemaValidate($schemaFile); + + if ($result === false) { + $msgs = []; + foreach (libxml_get_errors() as $err) { + $msgs[] = trim($err->message) . ' on line ' . $err->line; + } + + throw new SchemaViolationException(sprintf( + "XML schema validation errors:\n - %s", + implode("\n - ", array_unique($msgs)), + )); + } + + return $document; + } + + + /** + * Get the schema file that can validate this element. + * + * @return string + */ + public static function getSchemaFile(): string + { + if (defined('static::SCHEMA')) { + $schemaFile = static::SCHEMA; + } + + Assert::true(file_exists($schemaFile), IOException::class); + return $schemaFile; + } +} From e67ade355757a0380780ba0496323a5254fe9471 Mon Sep 17 00:00:00 2001 From: Tim van Dijen Date: Mon, 16 Dec 2024 23:12:22 +0100 Subject: [PATCH 2/2] Refactor schema validation --- src/DOMDocumentFactory.php | 53 +------------------ src/SchemaValidatableElementInterface.php | 2 +- src/SchemaValidatableElementTrait.php | 29 +++++++--- src/TestUtils/SchemaValidationTestTrait.php | 13 +---- tests/Utils/BooleanElement.php | 9 +++- tests/Utils/ExtendableAttributesElement.php | 8 ++- tests/Utils/ExtendableElement.php | 8 ++- tests/Utils/StringElement.php | 8 ++- tests/XML/DOMDocumentFactoryTest.php | 41 -------------- tests/XML/ExtendableAttributesTest.php | 2 - tests/XML/ExtendableElementTest.php | 2 - .../XML/SchemaValidatableElementTraitTest.php | 53 +++++++++++++++++++ tests/resources/schemas/simplesamlphp.xsd | 12 +++++ 13 files changed, 120 insertions(+), 120 deletions(-) create mode 100644 tests/XML/SchemaValidatableElementTraitTest.php diff --git a/src/DOMDocumentFactory.php b/src/DOMDocumentFactory.php index 4db8fc6..ad24ff6 100644 --- a/src/DOMDocumentFactory.php +++ b/src/DOMDocumentFactory.php @@ -8,20 +8,14 @@ use SimpleSAML\Assert\Assert; use SimpleSAML\XML\Exception\IOException; use SimpleSAML\XML\Exception\RuntimeException; -use SimpleSAML\XML\Exception\SchemaViolationException; use SimpleSAML\XML\Exception\UnparseableXMLException; -use function array_unique; -use function file_exists; use function file_get_contents; use function func_num_args; -use function implode; use function libxml_clear_errors; -use function libxml_get_errors; use function libxml_set_external_entity_loader; use function libxml_use_internal_errors; use function sprintf; -use function trim; /** * @package simplesamlphp/xml-common @@ -37,14 +31,12 @@ final class DOMDocumentFactory /** * @param string $xml - * @param string|null $schemaFile * @param non-negative-int $options * * @return \DOMDocument */ public static function fromString( string $xml, - ?string $schemaFile = null, int $options = self::DEFAULT_OPTIONS, ): DOMDocument { libxml_set_external_entity_loader(null); @@ -64,11 +56,6 @@ public static function fromString( $options |= LIBXML_NO_XXE; } - // Perform optional schema validation - if (!empty($schemaFile)) { - self::schemaValidation($xml, $schemaFile, $options); - } - $domDocument = self::create(); $loaded = $domDocument->loadXML($xml, $options); @@ -97,7 +84,6 @@ public static function fromString( /** * @param string $file - * @param string|null $schemaFile * @param non-negative-int $options * * @return \DOMDocument @@ -117,9 +103,7 @@ public static function fromFile( } Assert::notWhitespaceOnly($xml, sprintf('File "%s" does not have content', $file), RuntimeException::class); - return (func_num_args() < 3) - ? static::fromString($xml, $schemaFile) - : static::fromString($xml, $schemaFile, $options); + return (func_num_args() < 2) ? static::fromString($xml) : static::fromString($xml, $options); } @@ -132,39 +116,4 @@ public static function create(string $version = '1.0', string $encoding = 'UTF-8 { return new DOMDocument($version, $encoding); } - - - /** - * Validate an XML-string against a given schema. - * - * @param string $xml - * @param string $schemaFile - * @param int $options - * - * @throws \SimpleSAML\XML\Exception\SchemaViolationException when validation fails. - */ - public static function schemaValidation( - string $xml, - string $schemaFile, - int $options = self::DEFAULT_OPTIONS, - ): void { - if (!file_exists($schemaFile)) { - throw new IOException('File not found.'); - } - - $document = DOMDocumentFactory::fromString($xml); - $result = $document->schemaValidate($schemaFile); - - if ($result === false) { - $msgs = []; - foreach (libxml_get_errors() as $err) { - $msgs[] = trim($err->message) . ' on line ' . $err->line; - } - - throw new SchemaViolationException(sprintf( - "XML schema validation errors:\n - %s", - implode("\n - ", array_unique($msgs)), - )); - } - } } diff --git a/src/SchemaValidatableElementInterface.php b/src/SchemaValidatableElementInterface.php index 82a3e31..3d249a9 100644 --- a/src/SchemaValidatableElementInterface.php +++ b/src/SchemaValidatableElementInterface.php @@ -16,7 +16,7 @@ interface SchemaValidatableElementInterface extends ElementInterface /** * Validate the given DOMDocument against the schema set for this element * - * @return void + * @return \DOMDocument * @throws \SimpleSAML\XML\Exception\SchemaViolationException */ public static function schemaValidate(DOMDocument $document): DOMDocument; diff --git a/src/SchemaValidatableElementTrait.php b/src/SchemaValidatableElementTrait.php index ded6d7a..ab8bb2a 100644 --- a/src/SchemaValidatableElementTrait.php +++ b/src/SchemaValidatableElementTrait.php @@ -6,16 +6,18 @@ use DOMDocument; use SimpleSAML\Assert\Assert; -use SimpleSAML\Exception\IOException; -use SimpleSAML\Exception\SchemaViolationException; +use SimpleSAML\XML\Exception\IOException; +use SimpleSAML\XML\Exception\SchemaViolationException; use function array_unique; use function defined; use function file_exists; use function implode; +use function libxml_get_errors; +use function restore_error_handler; +use function set_error_handler; use function sprintf; use function trim; -use function libxml_get_errors; /** * trait class to be used by all the classes that implement the SchemaValidatableElementInterface @@ -27,16 +29,28 @@ trait SchemaValidatableElementTrait /** * Validate the given DOMDocument against the schema set for this element * - * @return void + * @return \DOMDocument * @throws \SimpleSAML\XML\Exception\SchemaViolationException */ public static function schemaValidate(DOMDocument $document): DOMDocument { $schemaFile = self::getSchemaFile(); - $result = $document->schemaValidate($schemaFile); + // Dirty trick to catch the warnings emitted by XML-DOMs schemaValidate + // This will turn the warning into an exception + set_error_handler(static function (int $errno, string $errstr): never { + throw new SchemaViolationException($errstr, $errno); + }, E_WARNING); + + try { + $result = $document->schemaValidate($schemaFile); + } finally { + // Restore the error handler, whether we throw an exception or not + restore_error_handler(); + } + + $msgs = []; if ($result === false) { - $msgs = []; foreach (libxml_get_errors() as $err) { $msgs[] = trim($err->message) . ' on line ' . $err->line; } @@ -53,6 +67,7 @@ public static function schemaValidate(DOMDocument $document): DOMDocument /** * Get the schema file that can validate this element. + * The path must be relative to the project's base directory. * * @return string */ @@ -62,7 +77,7 @@ public static function getSchemaFile(): string $schemaFile = static::SCHEMA; } - Assert::true(file_exists($schemaFile), IOException::class); + Assert::true(file_exists($schemaFile), sprintf("File not found: %s", $schemaFile), IOException::class); return $schemaFile; } } diff --git a/src/TestUtils/SchemaValidationTestTrait.php b/src/TestUtils/SchemaValidationTestTrait.php index b3cbcab..f2894b8 100644 --- a/src/TestUtils/SchemaValidationTestTrait.php +++ b/src/TestUtils/SchemaValidationTestTrait.php @@ -6,7 +6,6 @@ use DOMDocument; use PHPUnit\Framework\Attributes\Depends; -use SimpleSAML\XML\DOMDocumentFactory; use function class_exists; @@ -20,9 +19,6 @@ trait SchemaValidationTestTrait /** @var class-string */ protected static string $testedClass; - /** @var string */ - protected static string $schemaFile; - /** @var \DOMDocument */ protected static DOMDocument $xmlRepresentation; @@ -38,11 +34,6 @@ public function testSchemaValidation(): void 'Unable to run ' . self::class . '::testSchemaValidation(). Please set ' . self::class . ':$testedClass to a class-string representing the XML-class being tested', ); - } elseif (empty(self::$schemaFile)) { - $this->markTestSkipped( - 'Unable to run ' . self::class . '::testSchemaValidation(). Please set ' . self::class - . ':$schema to point to a schema file', - ); } elseif (empty(self::$xmlRepresentation)) { $this->markTestSkipped( 'Unable to run ' . self::class . '::testSchemaValidation(). Please set ' . self::class @@ -50,14 +41,14 @@ public function testSchemaValidation(): void ); } else { // Validate before serialization - DOMDocumentFactory::schemaValidation(self::$xmlRepresentation->saveXML(), self::$schemaFile); + self::$testedClass::schemaValidate(self::$xmlRepresentation); // Perform serialization $class = self::$testedClass::fromXML(self::$xmlRepresentation->documentElement); $serializedClass = $class->toXML(); // Validate after serialization - DOMDocumentFactory::schemaValidation($serializedClass->ownerDocument->saveXML(), self::$schemaFile); + self::$testedClass::schemaValidate($serializedClass->ownerDocument); // If we got this far and no exceptions were thrown, consider this test passed! $this->addToAssertionCount(1); diff --git a/tests/Utils/BooleanElement.php b/tests/Utils/BooleanElement.php index 5f6f7dd..09f1ff3 100644 --- a/tests/Utils/BooleanElement.php +++ b/tests/Utils/BooleanElement.php @@ -6,15 +6,20 @@ use SimpleSAML\XML\AbstractElement; use SimpleSAML\XML\BooleanElementTrait; +use SimpleSAML\XML\SchemaValidatableElementInterface; +use SimpleSAML\XML\SchemaValidatableElementTrait; /** * Empty shell class for testing BooleanElement. * * @package simplesaml/xml-common + * + * Note: this class is not final for testing purposes. */ -final class BooleanElement extends AbstractElement +class BooleanElement extends AbstractElement implements SchemaValidatableElementInterface { use BooleanElementTrait; + use SchemaValidatableElementTrait; /** @var string */ public const NS = 'urn:x-simplesamlphp:namespace'; @@ -22,6 +27,8 @@ final class BooleanElement extends AbstractElement /** @var string */ public const NS_PREFIX = 'ssp'; + public const SCHEMA = '/file/does/not/exist.xsd'; + /** * @param string $content diff --git a/tests/Utils/ExtendableAttributesElement.php b/tests/Utils/ExtendableAttributesElement.php index f3d6f47..d44fea4 100644 --- a/tests/Utils/ExtendableAttributesElement.php +++ b/tests/Utils/ExtendableAttributesElement.php @@ -9,6 +9,8 @@ use SimpleSAML\XML\AbstractElement; use SimpleSAML\XML\Exception\InvalidDOMElementException; use SimpleSAML\XML\ExtendableAttributesTrait; +use SimpleSAML\XML\SchemaValidatableElementInterface; +use SimpleSAML\XML\SchemaValidatableElementTrait; use SimpleSAML\XML\XsNamespace as NS; /** @@ -16,9 +18,10 @@ * * @package simplesaml/xml-security */ -class ExtendableAttributesElement extends AbstractElement +class ExtendableAttributesElement extends AbstractElement implements SchemaValidatableElementInterface { use ExtendableAttributesTrait; + use SchemaValidatableElementTrait; /** @var string */ public const NS = 'urn:x-simplesamlphp:namespace'; @@ -29,6 +32,9 @@ class ExtendableAttributesElement extends AbstractElement /** @var string */ public const LOCALNAME = 'ExtendableAttributesElement'; + /** @var string */ + public const SCHEMA = 'tests/resources/schemas/simplesamlphp.xsd'; + /** @var string|\SimpleSAML\XML\XsNamespace */ final public const XS_ANY_ATTR_NAMESPACE = NS::ANY; diff --git a/tests/Utils/ExtendableElement.php b/tests/Utils/ExtendableElement.php index 78f92d6..f88fce9 100644 --- a/tests/Utils/ExtendableElement.php +++ b/tests/Utils/ExtendableElement.php @@ -7,6 +7,8 @@ use DOMElement; use SimpleSAML\XML\AbstractElement; use SimpleSAML\XML\ExtendableElementTrait; +use SimpleSAML\XML\SchemaValidatableElementInterface; +use SimpleSAML\XML\SchemaValidatableElementTrait; use SimpleSAML\XML\SerializableElementTrait; use SimpleSAML\XML\XsNamespace as NS; @@ -15,9 +17,10 @@ * * @package simplesaml/xml-security */ -class ExtendableElement extends AbstractElement +class ExtendableElement extends AbstractElement implements SchemaValidatableElementInterface { use ExtendableElementTrait; + use SchemaValidatableElementTrait; use SerializableElementTrait; /** @var string */ @@ -29,6 +32,9 @@ class ExtendableElement extends AbstractElement /** @var string */ public const LOCALNAME = 'ExtendableElement'; + /** @var string */ + public const SCHEMA = 'tests/resources/schemas/simplesamlphp.xsd'; + /** @var \SimpleSAML\XML\XsNamespace|array */ final public const XS_ANY_ELT_NAMESPACE = NS::ANY; diff --git a/tests/Utils/StringElement.php b/tests/Utils/StringElement.php index 93cef67..55df945 100644 --- a/tests/Utils/StringElement.php +++ b/tests/Utils/StringElement.php @@ -5,6 +5,8 @@ namespace SimpleSAML\Test\XML; use SimpleSAML\XML\AbstractElement; +use SimpleSAML\XML\SchemaValidatableElementInterface; +use SimpleSAML\XML\SchemaValidatableElementTrait; use SimpleSAML\XML\StringElementTrait; /** @@ -12,8 +14,9 @@ * * @package simplesaml/xml-common */ -final class StringElement extends AbstractElement +final class StringElement extends AbstractElement implements SchemaValidatableElementInterface { + use SchemaValidatableElementTrait; use StringElementTrait; /** @var string */ @@ -22,6 +25,9 @@ final class StringElement extends AbstractElement /** @var string */ public const NS_PREFIX = 'ssp'; + /** @var string */ + public const SCHEMA = 'tests/resources/schemas/simplesamlphp.xsd'; + /** * @param string $content diff --git a/tests/XML/DOMDocumentFactoryTest.php b/tests/XML/DOMDocumentFactoryTest.php index 9292a5c..d7cf333 100644 --- a/tests/XML/DOMDocumentFactoryTest.php +++ b/tests/XML/DOMDocumentFactoryTest.php @@ -4,14 +4,12 @@ namespace SimpleSAML\Test\XML; -use DOMDocument; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\Group; use PHPUnit\Framework\TestCase; use RuntimeException; use SimpleSAML\Assert\AssertionFailedException; use SimpleSAML\XML\DOMDocumentFactory; -use SimpleSAML\XML\Exception\SchemaViolationException; use SimpleSAML\XML\Exception\UnparseableXMLException; use function strval; @@ -116,43 +114,4 @@ public function testEmptyStringIsNotValid(): void ); DOMDocumentFactory::fromString(''); } - - - public function testSchemaValidationPasses(): void - { - $file = 'tests/resources/xml/ssp_Chunk.xml'; - $schemaFile = 'tests/resources/schemas/simplesamlphp.xsd'; - $doc = DOMDocumentFactory::fromFile($file, $schemaFile); - $this->assertInstanceOf(DOMDocument::class, $doc); - } - - - public function testSchemaValidationWrongElementFails(): void - { - $file = 'tests/resources/xml/ssp_BooleanElement.xml'; - $schemaFile = 'tests/resources/schemas/simplesamlphp.xsd'; - - $this->expectException(SchemaViolationException::class); - DOMDocumentFactory::fromFile($file, $schemaFile); - } - - - public function testSchemaValidationUnknownSchemaFileFails(): void - { - $file = 'tests/resources/xml/ssp_Chunk.xml'; - $schemaFile = 'tests/resources/schemas/doesnotexist.xsd'; - - $this->expectExceptionMessage('File not found.'); - DOMDocumentFactory::fromFile($file, $schemaFile); - } - - - public function testSchemaValidationInvalidSchemaFileFails(): void - { - $file = 'tests/resources/xml/ssp_Chunk.xml'; - $schemaFile = 'resources/schemas/xml.xsd'; - - $this->expectException(SchemaViolationException::class); - DOMDocumentFactory::fromFile($file, $schemaFile); - } } diff --git a/tests/XML/ExtendableAttributesTest.php b/tests/XML/ExtendableAttributesTest.php index 1b89cf6..2765c86 100644 --- a/tests/XML/ExtendableAttributesTest.php +++ b/tests/XML/ExtendableAttributesTest.php @@ -31,8 +31,6 @@ final class ExtendableAttributesTest extends TestCase */ public static function setUpBeforeClass(): void { - self::$schemaFile = dirname(__FILE__, 2) . '/resources/schemas/simplesamlphp.xsd'; - self::$testedClass = ExtendableAttributesElement::class; self::$xmlRepresentation = DOMDocumentFactory::fromFile( diff --git a/tests/XML/ExtendableElementTest.php b/tests/XML/ExtendableElementTest.php index a9b393f..8222f54 100644 --- a/tests/XML/ExtendableElementTest.php +++ b/tests/XML/ExtendableElementTest.php @@ -31,8 +31,6 @@ final class ExtendableElementTest extends TestCase */ public static function setUpBeforeClass(): void { - self::$schemaFile = dirname(__FILE__, 2) . '/resources/schemas/simplesamlphp.xsd'; - self::$testedClass = ExtendableElement::class; self::$xmlRepresentation = DOMDocumentFactory::fromFile( diff --git a/tests/XML/SchemaValidatableElementTraitTest.php b/tests/XML/SchemaValidatableElementTraitTest.php new file mode 100644 index 0000000..b3229c5 --- /dev/null +++ b/tests/XML/SchemaValidatableElementTraitTest.php @@ -0,0 +1,53 @@ +assertInstanceOf(DOMDocument::class, $document); + } + + + public function testSchemaValidationWrongElementFails(): void + { + $file = 'tests/resources/xml/ssp_Base64Element.xml'; + $chunk = DOMDocumentFactory::fromFile($file); + + $this->expectException(SchemaViolationException::class); + StringElement::schemaValidate($chunk); + } + + + public function testSchemaValidationUnknownSchemaFileFails(): void + { + $file = 'tests/resources/xml/ssp_BooleanElement.xml'; + $chunk = DOMDocumentFactory::fromFile($file); + + $this->expectException(IOException::class); + BooleanElement::schemaValidate($chunk); + } +} diff --git a/tests/resources/schemas/simplesamlphp.xsd b/tests/resources/schemas/simplesamlphp.xsd index a87b8ff..73c7daa 100644 --- a/tests/resources/schemas/simplesamlphp.xsd +++ b/tests/resources/schemas/simplesamlphp.xsd @@ -35,6 +35,18 @@ + + + + + + + + + + + +