search

simplesamlphp / simplesamlphp-module-metarefresh

The metarefresh module will download and parse metadata documents and store them locally
GNU Lesser General Public License v2.1
7 stars 13 forks source link

Add attributewhitelist to support e.g. R&S+Sirtfi #11

Closed msalle closed 4 years ago

msalle commented 4 years ago

In order to whitelist IdPs based on the combination of e.g. R&S and Sirtfi, we introduce a new attributewhitelist source, which is similar to the standard blacklist and whitelist except we have multi-level arrays. See for example https://github.com/rcauth-eu/aarc-ansible-wayf/blob/master/roles/simplesamlphp/templates/config_patch/config_config-metarefresh.php.patch.j2#L26_L51

codecov[bot] commented 4 years ago

Codecov Report

Merging #11 into master will increase coverage by 7.75%. The diff coverage is 65.00%.

@@             Coverage Diff              @@
##             master      #11      +/-   ##
============================================
+ Coverage     30.67%   38.43%   +7.75%     
- Complexity      103      123      +20     
============================================
  Files             2        2              
  Lines           251      294      +43     
============================================
+ Hits             77      113      +36     
- Misses          174      181       +7
msalle commented 4 years ago

R&S+Sirtfi means squat to me, so do you mind adding some docs so I can try to understand what this does? Also:

* Please fix the coding style according to PSR-12

* Use strict comparisons wherever possible

* Add unit tests to test this new feature

I've fixed all three, but in writing a test I ran into a bug in SimpleSAMLphp itself which causes the tests to fails. I'll open a bug and fix for it but need to run now.

msalle commented 4 years ago

R&S+Sirtfi means squat to me, so do you mind adding some docs so I can try to understand what this does? Also:

* Please fix the coding style according to PSR-12

* Use strict comparisons wherever possible

* Add unit tests to test this new feature

I've fixed all three, but in writing a test I ran into a bug in SimpleSAMLphp itself which causes the tests to fails. I'll open a bug and fix for it but need to run now.

See https://github.com/simplesamlphp/simplesamlphp/pull/1337 This never showed up before in the tests because it didn't have any EntityAttributes yet.

msalle commented 4 years ago

As #1337 has been merged now, can you trigger a rebuild? That would hopefully also show a better code coverage result.

I think the only way for me would be to close this PR and reopen it. I can do that of course, but I think you can just restart it (following https://stackoverflow.com/a/17624403).

I understand the use case. But I'm not sure why it needs to use regular expressions. For what reason do we need that extra complexity? The strings to match on should be clearly defined values so could be compared stringwise only, right?

In principle yes, and in most case that's most probably more than sufficient, but if you like to match for example multiple registration authorities, a regexp would save quite some space. Of course in that case it's still only for the values. Another, somewhat forced example would be if you like to match on e.g. either OrganizationName or OrganizationDisplayName to contain let's say Nikhef.
In short, given that we're adding a new matching mechanism, I think it's good to make it flexible and "futureproof", but it's also fine with me to make it exact string matches which indeed is sufficient for my current use-case.

Maybe the documentation can be slightly clarified if you point out that the described use cases are in the example configuration snippet above.

I have added essentially that blob to the documentation file in the example config, docs/simplesamlphp-automated_metadata.md with clarifying text, do you think it's sufficiently clear?

tvdijen commented 4 years ago

I've triggered a rebuild and there's one issue:

1) SimpleSAML\Test\Module\metarefresh\MetaLoaderTest::testAttributewhitelist

Undefined variable: metadata

/home/travis/build/simplesamlphp/simplesamlphp-module-metarefresh/tests/lib/MetaLoaderTest.php:139

tvdijen commented 4 years ago

Great stuff! Just a few Psalm-issues left

msalle commented 4 years ago

I've triggered a rebuild and there's one issue:

1. SimpleSAML\Test\Module\metarefresh\MetaLoaderTest::testAttributewhitelist

Undefined variable: metadata

/home/travis/build/simplesamlphp/simplesamlphp-module-metarefresh/tests/lib/MetaLoaderTest.php:139

Ok that's fixed (I was a bit too quick on that one, my apologies for wasting your time). I've added two more tests to test in total 3 different attributewhitelist scenarios. The build passes all tests.

tvdijen commented 4 years ago

Not all.. A few were hidden (my bad), but since they are caused by this PR I'd like to see them taken care of as well.. I can help if necessary Forget what I said.. It's stuff being detected by a newer version of Psalm and does not originate from this PR..

tvdijen commented 4 years ago

@thijskh Are you satisfied regarding the docs?

tvdijen commented 4 years ago

Are we cool with backporting this to 0.9? Considering the changes, I am fine with it..

thijskh commented 4 years ago

Is there someone that wants/needs that?

msalle commented 4 years ago

For how long is the 0.9 still going to be the one shipped with the latest stable simplesamlphp? In principle there are more people in the NRENs and European research communities that do filtering based on entity-attributes, but they might all already have their own patch (like we did).

tvdijen commented 4 years ago

0.9 is going to be there for a while.. The 1.0 branch is targeted at SimpleSAMLphp 2.0, which is not going to happen any time soon..

I mean, I can tag 0.10 if we strictly follow semver (this is not a bugfix), but given the changes in this PR that would probably be overkill.. It's not that big of a change

NicolasLiampotis commented 4 years ago

Is there someone that wants/needs that?

It would be really useful if the attribute based entity filtering was available upstream before SSP 2.0, so yes please :-)

thijskh commented 4 years ago

Then GO!