Skip to content

Commit 9f4d7aa

Browse files
nathanjrobertsontvdijen
nathanjrobertson
and
tvdijen
authored
Add regex support for attribute names in AttributeLimit authproc filter (#1971)
* Add regular expression support for attribute names to AttributeLimit. Previously we supported regex for attribute values, but no the names (keys). * Add nameIsRegex to AttributeLimit documentation * AttributeLimit - add test case for regexp on name and value similtaneously * Fix pre-existing scrutinizer bug where $allowedAttributes (and now $allowedAttributeRegex) could be null * More scruitinizer fixes * Scrutinizer styling nit-pick * Simplify __construct() in an attempt to calm scrutinizer down * More srutinizer nit-picks on styling * Refactor process() to reduce complexity for improved scrutinizer score * Further optimizations for scrutinizer score improvements * Provide test case and fix the unlikely case where nameIsRegex=>false is configured * More scrutinizer optimizations * Fix double EOL triggering warning in GitHub Actions lint job for markdown files * Fixes for phpcs checks * Use sprintf for exception messages --------- Co-authored-by: Tim van Dijen <tvdijen@gmail.com>
1 parent b9d4243 commit 9f4d7aa

File tree

3 files changed

+188
-25
lines changed

3 files changed

+188
-25
lines changed

‎modules/core/docs/authproc_attributelimit.md‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,15 @@ Only allow specific values for an attribute ignoring case.
5353
],
5454
],
5555

56+
Only allow attributes that match a regex pattern
57+
58+
'authproc' => [
59+
50 => [
60+
'class' => 'core:AttributeLimit',
61+
'/^eduPerson' => [ 'nameIsRegex' => true ]
62+
],
63+
],
64+
5665
Only allow specific values for an attribute that match a regex pattern
5766

5867
'authproc' => [
@@ -82,13 +91,14 @@ like this:
8291
50 => 'core:AttributeLimit',
8392
],
8493

85-
Then, add the allowed attributes to each service provider metadata, in the `attributes` option:
94+
Then, add the allowed attributes to each service provider metadata, in the `attributes` option (for exact matches) or `attributesRegex` (for regular expression matches):
8695

8796
$metadata['https://saml2sp.example.org'] = [
8897
'AssertionConsumerService' => 'https://saml2sp.example.org/simplesaml/module.php/saml/sp/saml2-acs.php/default-sp',
8998
'SingleLogoutService' => 'https://saml2sp.example.org/simplesaml/module.php/saml/sp/saml2-logout.php/default-sp',
9099
...
91-
'attributes' => ['cn', 'mail'],
100+
'attributes' => ['cn', ... ],
101+
'attributesRegex' => [ '/^mail$/', ... ],
92102
...
93103
];
94104

‎modules/core/src/Auth/Process/AttributeLimit.php‎

Lines changed: 98 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use function is_int;
1616
use function is_string;
1717
use function preg_match;
18+
use function sprintf;
1819
use function var_export;
1920

2021
/**
@@ -30,6 +31,12 @@ class AttributeLimit extends Auth\ProcessingFilter
3031
*/
3132
private array $allowedAttributes = [];
3233

34+
/**
35+
* List of regular expressions for attributes which this filter will allow through.
36+
* @var array
37+
*/
38+
private array $allowedAttributeRegex = [];
39+
3340
/**
3441
* Whether the 'attributes' option in the metadata takes precedence.
3542
*
@@ -48,22 +55,29 @@ class AttributeLimit extends Auth\ProcessingFilter
4855
public function __construct(array &$config, $reserved)
4956
{
5057
parent::__construct($config, $reserved);
51-
5258
foreach ($config as $index => $value) {
5359
if ($index === 'default') {
5460
$this->isDefault = (bool) $value;
5561
} elseif (is_int($index)) {
5662
if (!is_string($value)) {
57-
throw new Error\Exception('AttributeLimit: Invalid attribute name: ' .
58-
var_export($value, true));
63+
throw new Error\Exception(sprintf(
64+
'AttributeLimit: Invalid attribute name: %s',
65+
var_export($value, true),
66+
));
5967
}
6068
$this->allowedAttributes[] = $value;
61-
} else { // Can only be string since PHP only allows string|int for array keys
62-
if (!is_array($value)) {
63-
throw new Error\Exception('AttributeLimit: Values for ' .
64-
var_export($index, true) . ' must be specified in an array.');
65-
}
69+
} elseif (!is_array($value)) {
70+
throw new Error\Exception(sprintf(
71+
'AttributeLimit: Values for %s must be specified in an array.',
72+
var_export($index, true),
73+
));
74+
} elseif (array_key_exists('nameIsRegex', $value) && (true === (bool) $value['nameIsRegex'])) {
75+
$this->allowedAttributeRegex[$index] = $value;
76+
unset($this->allowedAttributeRegex[$index]['nameIsRegex']);
77+
} else {
6678
$this->allowedAttributes[$index] = $value;
79+
// In case user sets nameIsRegex=false
80+
unset($this->allowedAttributes[$index]['nameIsRegex']);
6781
}
6882
}
6983
}
@@ -73,7 +87,7 @@ public function __construct(array &$config, $reserved)
7387
* Get list of allowed from the SP/IdP config.
7488
*
7589
* @param array &$state The current request.
76-
* @return array|null Array with attribute names, or NULL if no limit is placed.
90+
* @return array|null Array with attribute names, or null if no limit is placed.
7791
*/
7892
private static function getSPIdPAllowed(array &$state): ?array
7993
{
@@ -89,6 +103,26 @@ private static function getSPIdPAllowed(array &$state): ?array
89103
}
90104

91105

106+
/**
107+
* Get list of regular expressions of attribute names allowed from the SP/IdP config.
108+
*
109+
* @param array &$state The current request.
110+
* @return array|null Array with attribute names, or null if no limit is placed.
111+
*/
112+
private static function getSPIdPAllowedRegex(array &$state): ?array
113+
{
114+
if (array_key_exists('Destination', $state) && array_key_exists('attributesRegex', $state['Destination'])) {
115+
// SP Config
116+
return $state['Destination']['attributesRegex'];
117+
}
118+
if (array_key_exists('Source', $state) && array_key_exists('attributesRegex', $state['Source'])) {
119+
// IdP Config
120+
return $state['Source']['attributesRegex'];
121+
}
122+
return null;
123+
}
124+
125+
92126
/**
93127
* Apply filter to remove attributes.
94128
*
@@ -102,18 +136,23 @@ public function process(array &$state): void
102136
assert::keyExists($state, 'Attributes');
103137

104138
if ($this->isDefault) {
105-
$allowedAttributes = self::getSPIdPAllowed($state);
106-
if ($allowedAttributes === null) {
107-
$allowedAttributes = $this->allowedAttributes;
139+
$allowedAttributes = self::getSPIdPAllowed($state) ?? [];
140+
$allowedAttributeRegex = self::getSPIdPAllowedRegex($state) ?? [];
141+
if (empty($allowedAttributes) && empty($allowedAttributeRegex)) {
142+
$allowedAttributes = $this->allowedAttributes ?? [];
143+
$allowedAttributeRegex = $this->allowedAttributeRegex ?? [];
108144
}
109-
} elseif (!empty($this->allowedAttributes)) {
110-
$allowedAttributes = $this->allowedAttributes;
145+
} elseif (!(empty($this->allowedAttributes) && empty($this->allowedAttributeRegex))) {
146+
$allowedAttributes = $this->allowedAttributes ?? [];
147+
$allowedAttributeRegex = $this->allowedAttributeRegex ?? [];
111148
} else {
112-
$allowedAttributes = self::getSPIdPAllowed($state);
113-
if ($allowedAttributes === null) {
114-
// No limit on attributes
115-
return;
116-
}
149+
$allowedAttributes = self::getSPIdPAllowed($state) ?? [];
150+
$allowedAttributeRegex = self::getSPIdPAllowedRegex($state) ?? [];
151+
}
152+
153+
if (empty($allowedAttributes) && empty($allowedAttributeRegex)) {
154+
// No limit on attributes
155+
return;
117156
}
118157

119158
$attributes = &$state['Attributes'];
@@ -124,14 +163,26 @@ public function process(array &$state): void
124163
if (array_key_exists($name, $allowedAttributes)) {
125164
// but it is an index of the array
126165
if (!is_array($allowedAttributes[$name])) {
127-
throw new Error\Exception('AttributeLimit: Values for ' .
128-
var_export($name, true) . ' must be specified in an array.');
166+
throw new Error\Exception(sprintf(
167+
'AttributeLimit: Values for %s must be specified in an array.',
168+
var_export($name, true),
169+
));
129170
}
171+
130172
$attributes[$name] = $this->filterAttributeValues($attributes[$name], $allowedAttributes[$name]);
131173
if (!empty($attributes[$name])) {
132174
continue;
133175
}
176+
} elseif (($regexpMatch = self::matchAnyRegex($name, $allowedAttributeRegex)) !== null) {
177+
$attributes[$name] = $this->filterAttributeValues(
178+
$attributes[$name],
179+
$allowedAttributeRegex[$regexpMatch]
180+
);
181+
if (!empty($attributes[$name])) {
182+
continue;
183+
}
134184
}
185+
135186
unset($attributes[$name]);
136187
}
137188
}
@@ -144,8 +195,12 @@ public function process(array &$state): void
144195
* @param array $allowedConfigValues The allowed values, and possibly configuration options.
145196
* @return array The filtered values
146197
*/
147-
private function filterAttributeValues(array $values, array $allowedConfigValues): array
198+
private function filterAttributeValues(array $values, ?array $allowedConfigValues): array
148199
{
200+
if (($allowedConfigValues === null) || empty($allowedConfigValues)) {
201+
return $values;
202+
}
203+
149204
if (array_key_exists('regex', $allowedConfigValues) && $allowedConfigValues['regex'] === true) {
150205
$matchedValues = [];
151206
foreach ($allowedConfigValues as $option => $pattern) {
@@ -179,4 +234,25 @@ private function filterAttributeValues(array $values, array $allowedConfigValues
179234

180235
return array_intersect($values, $allowedConfigValues);
181236
}
237+
238+
239+
/**
240+
* Check if a string matches any of the regular expressions in the array of regexps
241+
*
242+
* @param string $needle The string we're searching on
243+
* @param array|null Array with regular expressions to test against. null is equivalent to an empty array.
244+
* @return string|null Regular expression that matched, or null if no match.
245+
*/
246+
private static function matchAnyRegex(string $needle, ?array $regexps = null): string | null
247+
{
248+
if ($regexps !== null) {
249+
foreach ($regexps as $x => $y) {
250+
$regexp = is_int($x) ? $y : $x;
251+
if (preg_match($regexp, $needle)) {
252+
return $regexp;
253+
}
254+
}
255+
}
256+
return null;
257+
}
182258
}

‎tests/modules/core/src/Auth/Process/AttributeLimitTest.php‎

Lines changed: 78 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,12 @@ public static function setUpBeforeClass(): void
154154
'mail' => ['user@example.org'],
155155
],
156156
'Destination' => [
157-
'attributes' => ['cn', 'mail'],
157+
'attributes' => [
158+
'cn',
159+
],
160+
'attributesRegex' => [
161+
'/^mail$/',
162+
]
158163
],
159164
'Source' => [
160165
],
@@ -245,6 +250,78 @@ public function testInvalidAttributeName(): void
245250
}
246251

247252

253+
/**
254+
* Test for attribute matching using regular expresssion support
255+
*/
256+
public function testMatchAttributeRegex(): void
257+
{
258+
$config = [
259+
'/^eduPersonTargetedID$/' => ['nameIsRegex' => true]
260+
];
261+
$result = self::processFilter($config, self::$request);
262+
$attributes = $result['Attributes'];
263+
$this->assertArrayHasKey('eduPersonTargetedID', $attributes);
264+
$this->assertCount(1, $attributes);
265+
266+
$config = [
267+
'/^eduPerson/' => ['nameIsRegex' => true]
268+
];
269+
$result = self::processFilter($config, self::$request);
270+
$attributes = $result['Attributes'];
271+
$this->assertArrayHasKey('eduPersonTargetedID', $attributes);
272+
$this->assertArrayHasKey('eduPersonAffiliation', $attributes);
273+
$this->assertCount(2, $attributes);
274+
275+
$config = [
276+
'/.*nomatch.*/' => ['nameIsRegex' => true]
277+
];
278+
$result = self::processFilter($config, self::$request);
279+
$attributes = $result['Attributes'];
280+
$this->assertCount(0, $attributes);
281+
282+
// Test combination of plain and regexp matches
283+
$config = [
284+
'/^eduPersonTargetedID$/' => ['nameIsRegex' => true],
285+
'cn'
286+
];
287+
$result = self::processFilter($config, self::$request);
288+
$attributes = $result['Attributes'];
289+
$this->assertCount(2, $attributes);
290+
291+
// Test case where both a plain and regexp match one of the attributes that
292+
// they're not double counted
293+
$config = [
294+
'/^eduPerson/' => ['nameIsRegex' => true],
295+
'cn',
296+
'eduPersonTargetedID'
297+
];
298+
$result = self::processFilter($config, self::$request);
299+
$attributes = $result['Attributes'];
300+
$this->assertCount(3, $attributes);
301+
302+
// Both name and value regexps on the same item
303+
$config = [
304+
'/^eduPerson/' => [
305+
'nameIsRegex' => true,
306+
'regex' => true,
307+
'/@example.org$/'
308+
],
309+
];
310+
$result = self::processFilter($config, self::$request);
311+
$attributes = $result['Attributes'];
312+
$this->assertArrayHasKey('eduPersonTargetedID', $attributes);
313+
$this->assertCount(1, $attributes);
314+
315+
$config = [
316+
'eduPersonTargetedID' => ['nameIsRegex' => false]
317+
];
318+
$result = self::processFilter($config, self::$request);
319+
$attributes = $result['Attributes'];
320+
$this->assertArrayHasKey('eduPersonTargetedID', $attributes);
321+
$this->assertCount(1, $attributes);
322+
}
323+
324+
248325
/**
249326
* Test for attribute value matching
250327
*/

0 commit comments

Comments
 (0)