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

fix: /info docs test expectation #2884

Merged
merged 2 commits into from
May 15, 2024

Conversation

peterhaochen47
Copy link
Member

  • the value of "idpDefinitions.*" of /info response is in reality a String, not an array. Here is how it looks like:
 "idpDefinitions": {
    "testsaml-redirect-binding": "http://localhost:8080/uaa/saml/discovery?returnIDParam=idp&entityID=cloudfoundry-saml-login&idp=testsaml-redirect-binding&isPassive=true",
    "testsaml-post-binding": "http://localhost:8080/uaa/saml/discovery?returnIDParam=idp&entityID=cloudfoundry-saml-login&idp=testsaml-post-binding&isPassive=true"
  },

which is consistent with the description of "idpDefinitions" field: "A list of alias/url pairs of SAML IDP providers configured. Each url is the starting point to initiate the authentication process for the SAML identity provider."

  • previously, this test is passing only because the test uaa.yml used in this test contains no IDP configs; but once you input some valid IDP configs, this test would fail since the value is actually a String.

  • also clarify the description text

- the value of "idpDefinitions.*" of /info response is in reality
a String, not an array. Here is how it looks like:
```
 "idpDefinitions": {
    "testsaml-redirect-binding": "http://localhost:8080/uaa/saml/discovery?returnIDParam=idp&entityID=cloudfoundry-saml-login&idp=testsaml-redirect-binding&isPassive=true",
    "testsaml-post-binding": "http://localhost:8080/uaa/saml/discovery?returnIDParam=idp&entityID=cloudfoundry-saml-login&idp=testsaml-post-binding&isPassive=true"
  },
```
which is consistent with the description of "idpDefinitions" field: "A
list of alias/url pairs of SAML IDP providers configured. Each url is
the starting point to initiate the authentication process for the SAML
identity provider."

- previously, this test is passing only because the test uaa.yml used
in this test contains no IDP configs; but once you input some valid IDP
configs, this test would fail since the value is actually a String.

- also clarify the description text
@cf-gitbot
Copy link

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

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

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

@peterhaochen47 peterhaochen47 requested review from a team and strehle May 10, 2024 23:17
@@ -59,7 +59,7 @@ void info_endpoint_for_json() throws Exception {
fieldWithPath("commit_id").type(STRING).description("The GIT sha for the UAA version"),
fieldWithPath("timestamp").type(STRING).description("JSON timestamp for the commit of the UAA version"),
fieldWithPath("idpDefinitions").optional().type(OBJECT).description("A list of alias/url pairs of SAML IDP providers configured. Each url is the starting point to initiate the authentication process for the SAML identity provider."),
fieldWithPath("idpDefinitions.*").optional().type(ARRAY).description("A list of alias/url pairs of SAML IDP providers configured. Each url is the starting point to initiate the authentication process for the SAML identity provider."),
fieldWithPath("idpDefinitions.*").optional().type(STRING).description("The URL to initiate the authentication process for the SAML identity provider."),
Copy link
Member

Choose a reason for hiding this comment

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

ok, it is not a JSON array but it is also not a pure String, but an JSON object simply...

So depends from which side you want solve it. Pure syntax, then object or human readable, then it is more an array

Copy link
Member Author

@peterhaochen47 peterhaochen47 May 13, 2024

Choose a reason for hiding this comment

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

Hi, why is idpDefinitions.* a JSON object?
Based on the sample /info endpoint output in the PR description, it currently looks like idpDefinitions's value is an object (hence line 61 in the quoted code block is asserting so). But it looks like the value of idpDefinitions.* is an URL string. Also see test here showing that idpDefinitions is a Map<String, String> where the values are strings.

Copy link
Member

Choose a reason for hiding this comment

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

because these Maps are JSON objects. see https://www.w3schools.com/js/js_json_objects.asp

My point is, array maybe was not the best choice, but String is even worse, so either stay with this type or use the correct one, which is Object

Copy link
Member Author

@peterhaochen47 peterhaochen47 May 14, 2024

Choose a reason for hiding this comment

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

because these Maps are JSON objects.

In the fieldWithPath syntax here, idpDefinitions is the map (key-value pairs), but idpDefinitions.* is the key in the key-value pair.

The line changed in this commit is a test (that will get turned into doc), and the test tries to validate if the type you put here matches the type in the real mockmvc api response. If we put:

fieldWithPath("idpDefinitions.*").optional().type(OBJECT).description("The URL to initiate the authentication process for the SAML identity provider.")

or

fieldWithPath("idpDefinitions.*").optional().type(ARRAY).description("The URL to initiate the authentication process for the SAML identity provider.")

This test will fail, once you configure one or more SAML IDP in its UAA.yml.

Copy link
Member

@strehle strehle May 14, 2024

Choose a reason for hiding this comment

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

ok, understood, you had a test failure if you fill the yml...
Then I propose VARIES. It generates this map, which to me is no STRING

"idpDefinitions" : {
    "dummy" : "http://localhost:8080/uaa/saml/discovery?returnIDParam=idp&entityID=cloudfoundry-saml-login&idp=dummy&isPassive=true",
    "xs2security" : "http://localhost:8080/uaa/saml/discovery?returnIDParam=idp&entityID=cloudfoundry-saml-login&idp=xs2security&isPassive=true"
  },

Copy link
Member Author

@peterhaochen47 peterhaochen47 May 14, 2024

Choose a reason for hiding this comment

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

I think we still have an misunderstanding of the fieldWithPath syntax. idpDefinitions's value is 100% a map, NOT a string. That's why the test fieldWithPath("idpDefinitions").optional().type(OBJECT).description("xxx") is passing.

But fieldWithPath("idpDefinitions.*") refers NOT to idpDefinitions's value, but idpDefinitions.dummy's value, where the * is matching dummy. So idpDefinitions.*'s value is really "http://localhost:8080/uaa/saml/discovery?returnIDParam=idp&entityID=cloudfoundry-saml-login&idp=dummy&isPassive=true". That's why fieldWithPath("idpDefinitions.*").optional().type(STRING).description("xxxx.") is passing with some IDPs configured.

Copy link
Member

Choose a reason for hiding this comment

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

I know and I have run the tests with content you suggested and I could verify the test failures, however with VARIES the tests were succeed with OK

Another solution could be to have 2 extra entries, means instead idpDefinitions.*

  • idpDefinitions.alias
  • idpDefinitions.url

both with type string .

@strehle strehle requested a review from a team May 11, 2024 08:49
@strehle
Copy link
Member

strehle commented May 14, 2024

  • the value of "idpDefinitions.*" of /info response is in reality a String, not an array. Here is how it looks like:
 "idpDefinitions": {
    "testsaml-redirect-binding": "http://localhost:8080/uaa/saml/discovery?returnIDParam=idp&entityID=cloudfoundry-saml-login&idp=testsaml-redirect-binding&isPassive=true",
    "testsaml-post-binding": "http://localhost:8080/uaa/saml/discovery?returnIDParam=idp&entityID=cloudfoundry-saml-login&idp=testsaml-post-binding&isPassive=true"
  },

which is consistent with the description of "idpDefinitions" field: "A list of alias/url pairs of SAML IDP providers configured. Each url is the starting point to initiate the authentication process for the SAML identity provider."

  • previously, this test is passing only because the test uaa.yml used in this test contains no IDP configs; but once you input some valid IDP configs, this test would fail since the value is actually a String.
  • also clarify the description text

As always, if you have a deeper look into old UAA behavior. I find SAML links in "uaac info" (JSON output) , but on OAUTH.

After thinking, I would like to know if you have use /info in your products ? I think SAP never has used it really, but for us it was always a problem, therefore we introduced account chooser etc... (@torsten-sap , @tack-sap correct ?)

The point is, in /info I also see entityID, which is only for SAML. Why no OAUITH / OIDC issuer ?
In general I would omit such info, because we want in future a UAA without SAML SP , e.g. #2741

@peterhaochen47 peterhaochen47 merged commit bd2cc45 into develop May 15, 2024
20 checks passed
@peterhaochen47 peterhaochen47 deleted the pr/develop/fix-info-endpoint-doc-test branch May 15, 2024 17:41
hsinn0 pushed a commit that referenced this pull request May 17, 2024
* fix: /info docs test expectation

- the value of "idpDefinitions.*" of /info response is in reality
a String, not an array. Here is how it looks like:
```
 "idpDefinitions": {
    "testsaml-redirect-binding": "http://localhost:8080/uaa/saml/discovery?returnIDParam=idp&entityID=cloudfoundry-saml-login&idp=testsaml-redirect-binding&isPassive=true",
    "testsaml-post-binding": "http://localhost:8080/uaa/saml/discovery?returnIDParam=idp&entityID=cloudfoundry-saml-login&idp=testsaml-post-binding&isPassive=true"
  },
```
which is consistent with the description of "idpDefinitions" field: "A
list of alias/url pairs of SAML IDP providers configured. Each url is
the starting point to initiate the authentication process for the SAML
identity provider."

- previously, this test is passing only because the test uaa.yml used
in this test contains no IDP configs; but once you input some valid IDP
configs, this test would fail since the value is actually a String.

- also clarify the description text

Additional commit squashed:
* change to VARIES

see https://docs.spring.io/spring-restdocs/docs/current/api/org/springframework/restdocs/payload/JsonFieldType.html
-> A variety of different types.

---------

Co-authored-by: d036670 <[email protected]>
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.

3 participants