Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update test suite for no single string endpoints #2132

Conversation

monkeyiq
Copy link
Contributor

@monkeyiq monkeyiq commented Jun 14, 2024

This is an update after single string and single array endpoints were removed. See #2131

Note that there are some heuristics that seem to have been implicit.

This will effect all endpoints ArtifactResolutionService, SingleSignOnService, and SingleLogoutService.

If there is no binding set then one is set for you. This matches the default value table from
https://github.com/simplesamlphp/simplesamlphp/blob/simplesamlphp-2.2/docs/simplesamlphp-metadata-endpoints.md

If there is only one end point in the array and the binding is not valid then it is refreshed for you to something that is valid.

On the other hand, for a list of end points no such refreshing is done so that the case of a non default Location with a valid Binding can be the one that is used.

This is an update after single string and single array endpoints were
removed. See simplesamlphp#2131

Some simpler finds such as $acs_eps && $acs_expected_eps expressing
and expecting some endpoints in the old format.

It seems that ArtifactResolutionService needs to be an [[]] too.

TODO

* SingleLogoutService using a binding of valid_binding is not being
  set to Constants::BINDING_HTTP_REDIRECT when it is seen by the
  configuration system.

* The last test in testGetDefaultEndpoint is calling fail()
The presence of a "Default binding" on the below page would seem
to indicate that if the binding is not present in the Array of arrays
then it will be set to the default:

https://github.com/simplesamlphp/simplesamlphp/blob/simplesamlphp-2.2/docs/simplesamlphp-metadata-endpoints.md

The test suite was expecting to be able to do this, though it was also
using the old direct single string representation.

It probably makes sense if we are moving to on the the array of arrays
syntax to either not show a default on the
simplesamlphp-metadata-endpoints page or to use the default if the
binding is not explicitly supplied.
Copy link

codecov bot commented Jun 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.03%. Comparing base (7d6f499) to head (c75ae64).
Report is 6 commits behind head on simplesamlphp-2.3.

Additional details and impacted files
@@                   Coverage Diff                   @@
##             simplesamlphp-2.3    #2132      +/-   ##
=======================================================
- Coverage                45.04%   45.03%   -0.02%     
- Complexity                3871     3873       +2     
=======================================================
  Files                      162      162              
  Lines                    12919    12918       -1     
=======================================================
- Hits                      5820     5818       -2     
- Misses                    7099     7100       +1     

Note that there are some heuristics that seem to have been implicit.

If there is no binding set then one is set for you. This matches the
default value table from
https://github.com/simplesamlphp/simplesamlphp/blob/simplesamlphp-2.2/docs/simplesamlphp-metadata-endpoints.md

If there is only one end point in the array and the binding is not
valid then it is refreshed for you to something that is valid.

On the other hand, for a list of end points no such refreshing is done
so that the case of a non default Location with a valid Binding can
be the one that is used.
@monkeyiq monkeyiq requested a review from tvdijen June 15, 2024 03:36
@tvdijen
Copy link
Member

tvdijen commented Jun 15, 2024

I think, if you agree with the changes I made, this is ready to be merged.
Don't forget to cherry-pick this into master as well!

@monkeyiq
Copy link
Contributor Author

Your updates look good. I knew there was a better way to write isValidBinding(). I added it along the way while experimenting and didn't get back to levering it out for the proper solution. Thus the lack of method comments etc, I guess a "FIXME: make this real code" might have been a nicer metadata.

@monkeyiq monkeyiq merged commit 91f8b5f into simplesamlphp:simplesamlphp-2.3 Jun 15, 2024
12 checks passed
@monkeyiq
Copy link
Contributor Author

This has been picked over to master with commits starting at
2d0b4a7

@tvdijen
Copy link
Member

tvdijen commented Jun 16, 2024

Next time maybe squash & merge?.. It will save you from having to cherry-pick every single commit

@monkeyiq
Copy link
Contributor Author

This is a good idea since I think the semantics of the individual 10 or so commits is not super interesting in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants