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

Upgrade to be compatible with simplesamlphp v2 #2506

Merged
merged 12 commits into from
Oct 2, 2023

Conversation

bruce-ricard
Copy link
Contributor

Luckily, we only needed to update tests.

Context:

  1. The newer versions of cf-deployment only support cflinuxfs4 stemcells.
  2. The SimpleSamlPHP app used in UAA CI (version 1) is cf pushed to CF, and is only compatible with cflinuxfs3 stemcells.
  3. The version 2 of that SimpleSamlPHP app is compatible with cflinuxfs4, so we decided that we need to start using it instead of the old v1 one.
  4. When we run the integration tests against the new SimpleSamlPHP v2 server, some of the tests started failing, due to breaking changes in that new major version 2 of SimpleSamlPHP.
  5. Hence this PR, with fixes to the tests.

swalchemist and others added 12 commits September 26, 2023 14:07
* Better not to depend on what it looks like - consider making this more generic later.

Authored-by: Danny Faught <[email protected]>
Authored-by: Danny Faught <[email protected]>
* Generally better to use an ID anyway.

Authored-by: Danny Faught <[email protected]>
* Make the test less fragile in general.

Co-authored-by: Peter Chen <[email protected]>
- Unlike other tests where UAA obtains the SAML metadata xml with an URL,
this test uses a locally defined static SAML metadata xml value (in order to simulate
a test case where the "SingleLogoutService" entry inside the SAML metadata xml
is absent), which needs to be updated to match the new simplesaml server.
- This also fixes testSingleLogoutWithLogoutRedirect test for the same reason.

[#185279615]

Co-authored-by: Hongchol Sinn <[email protected]>
* This is the last failing test in this class needed to be fixed related to the simplesaml upgrade.

Co-authored-by: Peter Chen <[email protected]>
* Already fixed in one place, these are the other copypastas of the same thing.

Co-authored-by: Peter Chen <[email protected]>
* And remove TODOs.
* Missing cert error fixed in a config change in the simplesamlphp source.

Co-authored-by: Peter Chen <[email protected]>
- to accomodate to simplesaml server v2's UI changes

[#185279615]
* That server is currently deployed
* This change is to ensure that the tests will not be failing when we push, because they would be running against the old v1 server
* This change should be reverted after we have pushed the new saml v2 server to use the old http://simplesamlphp.uaa-acceptance.cf-app.com URL

Signed-off-by: Bruce Ricard <[email protected]>
@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/186136022

The labels on this github issue will be updated when the story is started.

@@ -86,10 +86,10 @@

public class IntegrationTestUtils {

public static final String SIMPLESAMLPHP_UAA_ACCEPTANCE = "http://simplesamlphp.uaa-acceptance.cf-app.com";
public static final String SIMPLESAMLPHP_UAA_ACCEPTANCE = "http://simplesamlphp2.uaa-acceptance.cf-app.com";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to break the tests when we merge and the tests run on CI, we temporarily changed the URL to point towards the already deployed v2 saml server. We'll revert this after we have deployed the new v2 server to the old URL.

Copy link
Member

@strehle strehle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cannot really judge but ok for me

@swalchemist swalchemist merged commit cb1ca35 into develop Oct 2, 2023
@swalchemist swalchemist deleted the simplsaml-185279615 branch October 2, 2023 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

6 participants