Add regex support for attribute names in AttributeLimit authproc filter#1971
Conversation
… Previously we supported regex for attribute values, but no the names (keys).
…llowedAttributeRegex) could be null
|
Ok. I don't think I can make Scrutinizer complain less than it is without making the code less readable. I think it's now best to have somebody review and give feedback. |
|
Thanks, I like the idea and implementation! Also good that you provide documentation and test coverage. I haven’t yet reviewed it line by line. |
|
@thijskh Thanks. I think I've fixed the phpcs errors and markdown linting warnings. Could you please trigger a re-test for those? |
|
Thank you @nathanjrobertson ! I will backport this to the 2.2 release-branch |
…er (#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>
|
@nathanjrobertson You think you can help us fixing the warnings in the unit tests? Something's still off there, even though the tests pass; https://github.com/simplesamlphp/simplesamlphp/actions/runs/9028875799/job/24810192242#step:11:19 |
Yep - I'll take a look at it today or tomorrow. |
@tvdijen Proposed fix is in #2095 - it's a harmless warning though, because it's all handled in the function it calls with the null value that results from the warning. Looking through the rest of that file - the remaining warning is this test. I disagree with the comment there - surely an invalid regular expression in the configuration is an error condition, and it should error out, not silently (except for the warning) ignore the error and continue. That could leave people with a potentially insecure configuration, which they won't pick up on as easily as if it errors and bails out. Anyway, your call - just noticed it and that irked me. |
|
@nathanjrobertson Indeed, probably the tests should be split out and the no-match case should be correctly skipped and the invalid case should be handled in a different test with an "expectException" for the PHP warning that's emitted in that case? |
Up until now, AttributeLimit allows regexp matching on the values of an attribute, but the attribute name itself must be specified as a single value of string. This PR adds a new configuration parameter (
nameIsRegex) to support attribute names in the configuration being regular expressions, rather than just static strings.When
nameIsRegex => true, the key is treated as a regexp. When it is false or not specified, it is treated as a string (ie. evaluated in the traditional way).The implementation allows using both traditional plain strings and regexps side by side:
and is fully backward compatible. You can even use regex support for both attribute name and value simultaneously:
There is support for the metadata method of providing configuration as well. There is a new key available in the metadata
attributesRegex, which works as the existingattributeskey does, but for regular expression entries. Again, they can be used side by side. In the case where only one of the two is specified, it is assumed the other is an empty list (which is better than taking defaults independently - that would be confusing and inconsistent).If
nameIsRegex(authsources.php method) andattributesRegex(metadata method) aren't specified, the preexisting behaviour continues. No breaking changes.