Missing Response Status Checks in IntegrationTestUtils

[adrianhoelzl-sap]

What version of UAA are you running?

77.8.0 - older versions also affected

How are you deploying the UAA?

executing integration tests locally

Summary

Some methods in IntegrationTestUtils do not check the response code of an HTTP call against the expected one. Instead, these methods just return the body of the response.

Furthermore, the used HTTP client (at least in the methods we checked) does not fail when calling response.getBody() on the response object of a failed request. Instead, it returns an object of the payload type where all properties are set to null.

This means that in the test cases that call these IntegrationTestUtils methods, failing requests are often either left unnoticed or cause other assertions to fail, making it harder to understand the cause for failing test cases.

Example

uaa/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/ScimGroupEndpointsIntegrationTests.java

Line 261 in ef939f7

public void relyOnDefaultGroupsShouldAllowedGroupSucceed() throws URISyntaxException {

In this test case, we update an identity zone's configuration to include a certain group in its allowlist. Later in the test, we expect that it should now be possible to create the group in the zone.

However, we did not notice that the creation of the zone failed due to a bug in the zone endpoints because the error response was not checked in the corresponding IntegrationTestUtils method (here: createClientAdminTokenInZone).

Some lines below that, we then successfully create the group, which leads to the overall test case being successful.

BUT: If the zone creation would have been successful, the group would already be created then and subsequent creations of the group should now fail (409 CONFLICT). When we fixed the zone creation bug, the test now fails. However, it did not fail - as one would expect - during the createGroup call, but rather at the assertion in line 274:

uaa/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/ScimGroupEndpointsIntegrationTests.java

Line 274 in ef939f7

assertEquals("cloud_controller_service_permissions.read", g2.getDisplayName());

This is because for failing requests, the HTTP client returns an instance of the response class with all properties being set to null, which lead to the assertion failing.

Solution

To make root cause analyses of failing integration tests easier, we suggest that all methods in IntegrationTestUtils should perform checks against the response status code. For some, this is already the case, for most of them however not.

Example: added assertions for the status code in this commit 884416d

[cf-gitbot]

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

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

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