simplesamlphp / simplesamlphp-module-adfs

This module adds support for WS-Federation
GNU Lesser General Public License v2.1
5 stars 5 forks source link

Setup initial integration tests #8

Closed pradtke closed 3 years ago

pradtke commented 3 years ago
codecov[bot] commented 3 years ago

Codecov Report

Merging #8 (5475591) into master (d11fafd) will not change coverage. The diff coverage is 0.00%.

@@           Coverage Diff            @@
##             master      #8   +/-   ##
========================================
  Coverage      0.00%   0.00%           
  Complexity       78      78           
========================================
  Files             5       5           
  Lines           356     357    +1     
========================================
- Misses          356     357    +1     
tvdijen commented 3 years ago

Great work Patrick! My only suggestion would be to use the PEMCertificatesMock instead of adding the sample certs to this repo. Here's an example; https://github.com/simplesamlphp/saml2/blob/90ddb45233c6abd79c3a912d55f715519f7c9352/tests/SAML2/SignedElementTestTrait.php#L54

pradtke commented 3 years ago

Thanks for pointing out PEMCertificatesMock @tvdijen! That's a good change. I've updated the PR.

tvdijen commented 3 years ago

One more question I'm asking myself, without (I admit) checking too much of the code, is whether we still need the BuiltinServer when we have routing? The PRP-test you're adding seems a lot like a controller-test, but this one is not covering the AdfsController-class? Or should we add a @covers tag to see the added coverage?

pradtke commented 3 years ago

One more question I'm asking myself, without (I admit) checking too much of the code, is whether we still need the BuiltinServer when we have routing?

I was hoping I wouldn't. But as ADFS::receiveAuthnRequest does it's thing eventually IdP::postAuthProc is called which expects the Responder function to have called exit(). So as part of this PR ADFS::postResponse now calls exit. Without that exit the Assert::true(false); in postAuthProc would fail.

tvdijen commented 3 years ago

Superseded by #10