Skip to content

Commit 7c1d107

Browse files
tvdijentvdijen
committed
Improved error-handling for extendable attributes & elements
1 parent b5541c7 commit 7c1d107

File tree

7 files changed

+99
-28
lines changed

7 files changed

+99
-28
lines changed

‎src/XML/ExtendableAttributesTrait.php‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99
use SimpleSAML\XML\Assert\Assert;
1010
use SimpleSAML\XML\Attribute;
1111
use SimpleSAML\XML\Constants as C;
12+
use SimpleSAML\XMLSchema\Exception\InvalidDOMAttributeException;
13+
use SimpleSAML\XMLSchema\Exception\SchemaViolationException;
1214
use SimpleSAML\XMLSchema\Type\StringValue;
1315
use SimpleSAML\XMLSchema\XML\Constants\NS;
1416

@@ -204,7 +206,7 @@ function (Attribute $attr) {
204206

205207
if ($namespace === NS::LOCAL) {
206208
// If ##local then all namespaces must be null
207-
Assert::allNull($actual_namespaces);
209+
Assert::allNull($actual_namespaces, SchemaviolationException::class);
208210
} elseif (is_array($namespace)) {
209211
// Make a local copy of the property that we can edit
210212
$allowed_namespaces = $namespace;
@@ -227,25 +229,28 @@ function (Attribute $attr) {
227229
rtrim(implode(', ', $diff)),
228230
self::NS,
229231
),
232+
SchemaViolationException::class,
230233
);
231234
} else {
232235
if ($namespace === NS::OTHER) {
233236
// All attributes must be namespaced, ergo non-null
234-
Assert::allNotNull($actual_namespaces);
237+
Assert::allNotNull($actual_namespaces, SchemaViolationException::class);
235238

236239
// Must be any namespace other than the parent element
237-
Assert::allNotSame($actual_namespaces, self::NS);
240+
Assert::allNotSame($actual_namespaces, self::NS, SchemaViolationException::class);
238241
} elseif ($namespace === NS::TARGETNAMESPACE) {
239242
// Must be the same namespace as the one of the parent element
240-
Assert::allSame($actual_namespaces, self::NS);
243+
Assert::allSame($actual_namespaces, self::NS, SchemaViolationException::class);
241244
}
242245
}
243246

244247
$exclusionList = self::getAttributeExclusions();
245248
foreach ($attributes as $i => $attr) {
246-
if (in_array([$attr->getNamespaceURI(), $attr->getAttrName()], $exclusionList, true)) {
247-
unset($attributes[$i]);
248-
}
249+
Assert::true(
250+
!in_array([$attr->getNamespaceURI(), $attr->getAttrName()], $exclusionList, true)
251+
&& !in_array([$attr->getNamespaceURI(), '*'], $exclusionList, true),
252+
InvalidDOMAttributeException::class,
253+
);
249254
}
250255

251256
$this->namespacedAttributes = $attributes;

‎src/XML/ExtendableElementTrait.php‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,16 @@
1010
use SimpleSAML\XML\Chunk;
1111
use SimpleSAML\XML\Constants as C;
1212
use SimpleSAML\XML\Registry\ElementRegistry;
13+
use SimpleSAML\XMLSchema\Exception\InvalidDOMElementException;
14+
use SimpleSAML\XMLSchema\Exception\SchemaViolationException;
1315
use SimpleSAML\XMLSchema\XML\Constants\NS;
1416

1517
use function array_diff;
1618
use function array_map;
1719
use function array_search;
1820
use function defined;
1921
use function implode;
22+
use function in_array;
2023
use function is_array;
2124
use function rtrim;
2225
use function sprintf;
@@ -146,7 +149,7 @@ function (AbstractElement|Chunk $elt): ?string {
146149

147150
if ($namespace === NS::LOCAL) {
148151
// If ##local then all namespaces must be null
149-
Assert::allNull($actual_namespaces);
152+
Assert::allNull($actual_namespaces, SchemaViolationException::class);
150153
} elseif (is_array($namespace)) {
151154
// Make a local copy of the property that we can edit
152155
$allowed_namespaces = $namespace;
@@ -169,23 +172,26 @@ function (AbstractElement|Chunk $elt): ?string {
169172
rtrim(implode(', ', $diff)),
170173
self::NS,
171174
),
175+
SchemaViolationException::class,
172176
);
173177
} elseif ($namespace === NS::OTHER) {
174178
// Must be any namespace other than the parent element, excluding elements with no namespace
175-
Assert::notInArray(null, $actual_namespaces);
176-
Assert::allNotSame($actual_namespaces, self::NS);
179+
Assert::notInArray(null, $actual_namespaces, SchemaViolationException::class);
180+
Assert::allNotSame($actual_namespaces, self::NS, SchemaViolationException::class);
177181
} elseif ($namespace === NS::TARGETNAMESPACE) {
178182
// Must be the same namespace as the one of the parent element
179-
Assert::allSame($actual_namespaces, self::NS);
183+
Assert::allSame($actual_namespaces, self::NS, SchemaViolationexception::class);
180184
} else {
181185
// XS_ANY_NS_ANY
182186
}
183187

184188
$exclusionList = self::getElementExclusions();
185189
foreach ($elements as $i => $elt) {
186-
if (in_array([$elt->getNamespaceURI(), $elt->getLocalName()], $exclusionList, true)) {
187-
unset($elements[$i]);
188-
}
190+
Assert::true(
191+
!in_array([$elt->getNamespaceURI(), $elt->getLocalName()], $exclusionList, true)
192+
&& !in_array([$elt->getNamespaceURI(), '*'], $exclusionList, true),
193+
InvalidDOMElementException::class,
194+
);
189195
}
190196

191197
$this->elements = $elements;

‎src/XMLSchema/Exception/InvalidDOMAttributeException.php‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
namespace SimpleSAML\XMLSchema\Exception;
6+
7+
use InvalidArgumentException;
8+
9+
/**
10+
* This exception may be raised when the passed DOMAttr is of the wrong type
11+
*
12+
* @package simplesamlphp/xml-common
13+
*/
14+
class InvalidDOMAttributeException extends InvalidArgumentException
15+
{
16+
}

‎tests/XML/ExtendableAttributesTest.php‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
use SimpleSAML\XML\DOMDocumentFactory;
1111
use SimpleSAML\XML\TestUtils\SchemaValidationTestTrait;
1212
use SimpleSAML\XML\TestUtils\SerializableElementTestTrait;
13+
use SimpleSAML\XMLSchema\Exception\InvalidDOMAttributeException;
1314
use SimpleSAML\XMLSchema\Type\StringValue;
1415

1516
use function dirname;
@@ -46,7 +47,6 @@ public function testMarshalling(): void
4647
[
4748
new Attribute('urn:x-simplesamlphp:namespace', 'ssp', 'attr1', StringValue::fromString('testval1')),
4849
new Attribute('urn:x-simplesamlphp:namespace', 'ssp', 'attr2', StringValue::fromString('testval2')),
49-
new Attribute('urn:x-simplesamlphp:namespace', 'ssp', 'attr3', StringValue::fromString('testval3')),
5050
],
5151
);
5252

@@ -57,6 +57,21 @@ public function testMarshalling(): void
5757
}
5858

5959

60+
/**
61+
*/
62+
public function testMarshallingWithExcludedAttribute(): void
63+
{
64+
$this->expectException(InvalidDOMAttributeException::class);
65+
new ExtendableAttributesElement(
66+
[
67+
new Attribute('urn:x-simplesamlphp:namespace', 'ssp', 'attr1', StringValue::fromString('testval1')),
68+
new Attribute('urn:x-simplesamlphp:namespace', 'ssp', 'attr2', StringValue::fromString('testval2')),
69+
new Attribute('urn:x-simplesamlphp:namespace', 'ssp', 'attr3', StringValue::fromString('testval3')),
70+
],
71+
);
72+
}
73+
74+
6075
/**
6176
*/
6277
public function testGetAttributesNSFromXML(): void

‎tests/XML/ExtendableAttributesTraitTest.php‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
use SimpleSAML\Assert\AssertionFailedException;
99
use SimpleSAML\Test\Helper\ExtendableAttributesElement;
1010
use SimpleSAML\XML\Attribute;
11+
use SimpleSAML\XMLSchema\Exception\SchemaViolationException;
1112
use SimpleSAML\XMLSchema\Type\StringValue;
1213
use SimpleSAML\XMLSchema\XML\Constants\NS;
1314

@@ -124,7 +125,7 @@ public function getAttributeNamespace(): array|string
124125
*/
125126
public function testOtherNamespacePassingLocalThrowsAnException(): void
126127
{
127-
$this->expectException(AssertionFailedException::class);
128+
$this->expectException(SchemaViolationException::class);
128129
new class ([self::$local]) extends ExtendableAttributesElement {
129130
/**
130131
* @return array<int, string>|string
@@ -199,7 +200,7 @@ public function getAttributeNamespace(): array|string
199200
*/
200201
public function testTargetNamespacePassingOtherThrowsAnException(): void
201202
{
202-
$this->expectException(AssertionFailedException::class);
203+
$this->expectException(SchemaViolationException::class);
203204
new class ([self::$other]) extends ExtendableAttributesElement {
204205
/**
205206
* @return array<int, string>|string
@@ -255,7 +256,7 @@ public function getAttributeNamespace(): array|string
255256
*/
256257
public function testLocalNamespacePassingOtherThrowsAnException(): void
257258
{
258-
$this->expectException(AssertionFailedException::class);
259+
$this->expectException(SchemaViolationException::class);
259260
new class ([self::$other]) extends ExtendableAttributesElement {
260261
/**
261262
* @return array<int, string>|string

‎tests/XML/ExtendableElementTest.php‎

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
use SimpleSAML\XML\DOMDocumentFactory;
1111
use SimpleSAML\XML\TestUtils\SchemaValidationTestTrait;
1212
use SimpleSAML\XML\TestUtils\SerializableElementTestTrait;
13+
use SimpleSAML\XMLSchema\Exception\InvalidDOMElementException;
1314

1415
use function dirname;
1516
use function strval;
@@ -47,22 +48,16 @@ public function testMarshalling(): void
4748
$dummyDocument2 = DOMDocumentFactory::fromString(
4849
'<dummy:Chunk xmlns:dummy="urn:custom:dummy">some</dummy:Chunk>',
4950
);
50-
$dummyDocument3 = DOMDocumentFactory::fromString(
51-
'<other:Chunk xmlns:other="urn:custom:other">some</other:Chunk>',
52-
);
5351

5452
/** @var \DOMElement $dummyElement1 */
5553
$dummyElement1 = $dummyDocument1->documentElement;
5654
/** @var \DOMElement $dummyElement2 */
5755
$dummyElement2 = $dummyDocument2->documentElement;
58-
/** @var \DOMElement $dummyElement3 */
59-
$dummyElement3 = $dummyDocument3->documentElement;
6056

6157
$extendableElement = new ExtendableElement(
6258
[
6359
new Chunk($dummyElement1),
6460
new Chunk($dummyElement2),
65-
new Chunk($dummyElement3),
6661
],
6762
);
6863

@@ -73,6 +68,38 @@ public function testMarshalling(): void
7368
}
7469

7570

71+
/**
72+
*/
73+
public function testMarshallingWithExcludedElement(): void
74+
{
75+
$dummyDocument1 = DOMDocumentFactory::fromString(
76+
'<ssp:Chunk xmlns:ssp="urn:x-simplesamlphp:namespace">some</ssp:Chunk>',
77+
);
78+
$dummyDocument2 = DOMDocumentFactory::fromString(
79+
'<dummy:Chunk xmlns:dummy="urn:custom:dummy">some</dummy:Chunk>',
80+
);
81+
$dummyDocument3 = DOMDocumentFactory::fromString(
82+
'<other:Chunk xmlns:other="urn:custom:other">some</other:Chunk>',
83+
);
84+
85+
/** @var \DOMElement $dummyElement1 */
86+
$dummyElement1 = $dummyDocument1->documentElement;
87+
/** @var \DOMElement $dummyElement2 */
88+
$dummyElement2 = $dummyDocument2->documentElement;
89+
/** @var \DOMElement $dummyElement3 */
90+
$dummyElement3 = $dummyDocument3->documentElement;
91+
92+
$this->expectException(InvalidDOMElementException::class);
93+
new ExtendableElement(
94+
[
95+
new Chunk($dummyElement1),
96+
new Chunk($dummyElement2),
97+
new Chunk($dummyElement3),
98+
],
99+
);
100+
}
101+
102+
76103
/**
77104
*/
78105
public function testGetChildElementsFromXML(): void

‎tests/XML/ExtendableElementTraitTest.php‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
use SimpleSAML\XML\Chunk;
1111
use SimpleSAML\XML\DOMDocumentFactory;
1212
use SimpleSAML\XML\ElementInterface;
13+
use SimpleSAML\XMLSchema\Exception\SchemaViolationException;
1314
use SimpleSAML\XMLSchema\XML\Constants\NS;
1415

1516
/**
@@ -139,7 +140,7 @@ public function getElementNamespace(): array|string
139140
*/
140141
public function testOtherNamespacePassingLocalThrowsAnException(): void
141142
{
142-
$this->expectException(AssertionFailedException::class);
143+
$this->expectException(SchemaViolationException::class);
143144
new class ([self::$local]) extends ExtendableElement {
144145
/**
145146
* @return array<int, string>|string
@@ -214,7 +215,7 @@ public function getElementNamespace(): array|string
214215
*/
215216
public function testTargetNamespacePassingOtherThrowsAnException(): void
216217
{
217-
$this->expectException(AssertionFailedException::class);
218+
$this->expectException(SchemaViolationException::class);
218219
new class ([self::$other]) extends ExtendableElement {
219220
/**
220221
* @return array<int, string>|string
@@ -270,7 +271,7 @@ public function getElementNamespace(): array|string
270271
*/
271272
public function testLocalNamespacePassingTargetThrowsAnException(): void
272273
{
273-
$this->expectException(AssertionFailedException::class);
274+
$this->expectException(SchemaViolationException::class);
274275
new class ([self::$target]) extends ExtendableElement {
275276
/**
276277
* @return array<int, string>|string
@@ -288,7 +289,7 @@ public function getElementNamespace(): array|string
288289
*/
289290
public function testLocalNamespacePassingOtherThrowsAnException(): void
290291
{
291-
$this->expectException(AssertionFailedException::class);
292+
$this->expectException(SchemaViolationException::class);
292293
new class ([self::$other]) extends ExtendableElement {
293294
/**
294295
* @return array<int, string>|string

0 commit comments

Comments
 (0)