From 3e8f7292f76cd235e7966a60ccb1e1a1608eefd9 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Tue, 3 May 2022 23:27:38 +1000 Subject: [PATCH] [Test] More robust comparison for OIDC URLs (#86374) The String representation of OIDC url is not stable because the query parameters can be different orders (underlying backed by a HashMap). This PR improves the robustness of URL comparison by comparing the parsed query parameters instead of the non-stable serialised string form. Resolves: #86291 --- .../authc/oidc/OpenIdConnectRealmTests.java | 97 ++++++++++--------- 1 file changed, 51 insertions(+), 46 deletions(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealmTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealmTests.java index 6834c0e219d61..e644cedd23611 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealmTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/oidc/OpenIdConnectRealmTests.java @@ -59,6 +59,7 @@ import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.greaterThan; import static org.hamcrest.Matchers.hasKey; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.notNullValue; @@ -280,16 +281,14 @@ public void testBuildRelyingPartyConfigWithoutOpenIdScope() { final OpenIdConnectPrepareAuthenticationResponse response = realm.buildAuthenticationRequestUri(null, null, null); final String state = response.getState(); final String nonce = response.getNonce(); - assertThat( + assertEqualUrlStrings( response.getAuthenticationRequestUrl(), - equalTo( - "https://op.example.com/login?scope=scope1+scope2+openid&response_type=code" - + "&redirect_uri=https%3A%2F%2Frp.my.com%2Fcb&state=" - + state - + "&nonce=" - + nonce - + "&client_id=rp-my" - ) + "https://op.example.com/login?scope=scope1+scope2+openid&response_type=code" + + "&redirect_uri=https%3A%2F%2Frp.my.com%2Fcb&state=" + + state + + "&nonce=" + + nonce + + "&client_id=rp-my" ); assertThat(response.getRealmName(), equalTo(REALM_NAME)); } @@ -313,16 +312,14 @@ public void testBuildingAuthenticationRequest() { final OpenIdConnectPrepareAuthenticationResponse response = realm.buildAuthenticationRequestUri(null, null, null); final String state = response.getState(); final String nonce = response.getNonce(); - assertThat( + assertEqualUrlStrings( response.getAuthenticationRequestUrl(), - equalTo( - "https://op.example.com/login?scope=openid+scope1+scope2&response_type=code" - + "&redirect_uri=https%3A%2F%2Frp.my.com%2Fcb&state=" - + state - + "&nonce=" - + nonce - + "&client_id=rp-my" - ) + "https://op.example.com/login?scope=openid+scope1+scope2&response_type=code" + + "&redirect_uri=https%3A%2F%2Frp.my.com%2Fcb&state=" + + state + + "&nonce=" + + nonce + + "&client_id=rp-my" ); assertThat(response.getRealmName(), equalTo(REALM_NAME)); } @@ -343,16 +340,14 @@ public void testBuilidingAuthenticationRequestWithDefaultScope() { final OpenIdConnectPrepareAuthenticationResponse response = realm.buildAuthenticationRequestUri(null, null, null); final String state = response.getState(); final String nonce = response.getNonce(); - assertThat( + assertEqualUrlStrings( response.getAuthenticationRequestUrl(), - equalTo( - "https://op.example.com/login?scope=openid&response_type=code" - + "&redirect_uri=https%3A%2F%2Frp.my.com%2Fcb&state=" - + state - + "&nonce=" - + nonce - + "&client_id=rp-my" - ) + "https://op.example.com/login?scope=openid&response_type=code" + + "&redirect_uri=https%3A%2F%2Frp.my.com%2Fcb&state=" + + state + + "&nonce=" + + nonce + + "&client_id=rp-my" ); assertThat(response.getRealmName(), equalTo(REALM_NAME)); } @@ -413,16 +408,14 @@ public void testBuildingAuthenticationRequestWithExistingStateAndNonce() { final String nonce = new Nonce().getValue(); final OpenIdConnectPrepareAuthenticationResponse response = realm.buildAuthenticationRequestUri(state, nonce, null); - assertThat( + assertEqualUrlStrings( response.getAuthenticationRequestUrl(), - equalTo( - "https://op.example.com/login?scope=openid&response_type=code" - + "&redirect_uri=https%3A%2F%2Frp.my.com%2Fcb&state=" - + state - + "&nonce=" - + nonce - + "&client_id=rp-my" - ) + "https://op.example.com/login?scope=openid&response_type=code" + + "&redirect_uri=https%3A%2F%2Frp.my.com%2Fcb&state=" + + state + + "&nonce=" + + nonce + + "&client_id=rp-my" ); assertThat(response.getRealmName(), equalTo(REALM_NAME)); } @@ -445,21 +438,33 @@ public void testBuildingAuthenticationRequestWithLoginHint() { final String thehint = randomAlphaOfLength(8); final OpenIdConnectPrepareAuthenticationResponse response = realm.buildAuthenticationRequestUri(state, nonce, thehint); - assertThat( + assertEqualUrlStrings( response.getAuthenticationRequestUrl(), - equalTo( - "https://op.example.com/login?login_hint=" - + thehint - + "&scope=openid&response_type=code&redirect_uri=https%3A%2F%2Frp.my.com%2Fcb&state=" - + state - + "&nonce=" - + nonce - + "&client_id=rp-my" - ) + "https://op.example.com/login?login_hint=" + + thehint + + "&scope=openid&response_type=code&redirect_uri=https%3A%2F%2Frp.my.com%2Fcb&state=" + + state + + "&nonce=" + + nonce + + "&client_id=rp-my" ); assertThat(response.getRealmName(), equalTo(REALM_NAME)); } + private void assertEqualUrlStrings(String actual, String expected) { + final int endOfPath = actual.indexOf('?'); + assertThat(endOfPath, greaterThan(-1)); + assertThat(actual.substring(0, endOfPath + 1), equalTo(expected.substring(0, endOfPath + 1))); + + final HashMap actualParams = new HashMap<>(); + RestUtils.decodeQueryString(actual, endOfPath + 1, actualParams); + + final HashMap expectedParams = new HashMap<>(); + RestUtils.decodeQueryString(expected, endOfPath + 1, expectedParams); + + assertThat(actualParams, equalTo(expectedParams)); + } + private AuthenticationResult authenticateWithOidc( String principal, UserRoleMapper roleMapper,