From 2593c3dbc44f5ca2b45181d0d26edbe62e683f4f Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Wed, 14 Oct 2020 12:36:31 -0300 Subject: [PATCH] [KEYCLOAK-15893] - Incorrect resource match is returned for some cases when using wildcard in uri --- .../org/keycloak/common/util/PathMatcher.java | 49 ++++++++------- .../keycloak/common/util/PathMatcherTest.java | 2 +- ...ResourceManagementWithAuthzClientTest.java | 59 +++++++++++++++++++ 3 files changed, 89 insertions(+), 21 deletions(-) diff --git a/common/src/main/java/org/keycloak/common/util/PathMatcher.java b/common/src/main/java/org/keycloak/common/util/PathMatcher.java index d7c609f7f4..ce6213c342 100644 --- a/common/src/main/java/org/keycloak/common/util/PathMatcher.java +++ b/common/src/main/java/org/keycloak/common/util/PathMatcher.java @@ -42,7 +42,7 @@ public abstract class PathMatcher

{ String matchingUri = null; - if (exactMatch(expectedUri, targetUri, expectedUri)) { + if (exactMatch(expectedUri, targetUri)) { matchingUri = expectedUri; } @@ -53,7 +53,7 @@ public abstract class PathMatcher

{ int length = expectedUri.split("\\/").length; int bracketsLength = expectedUri.split("\\{").length; - if (exactMatch(expectedUri, targetUri, templateUri) && (patternCount == 0 || length > patternCount || bracketsLength < bracketsPatternCount)) { + if (exactMatch(templateUri, targetUri) && (patternCount == 0 || length > patternCount || bracketsLength < bracketsPatternCount)) { matchingUri = templateUri; P resolved = resolvePathConfig(entry, targetUri); @@ -96,8 +96,14 @@ public abstract class PathMatcher

{ } if (WILDCARD == expectedUri.charAt(expectedUri.length() - 1)) { - if (matchingAnyPath == null || getPath(matchingAnyPath).length() < matchingUri.length()) { + if (matchingAnyPath == null) { matchingAnyPath = entry; + } else { + String resourcePath = getPath(matchingAnyPath); + + if (resourcePath.split("/").length < matchingUri.split("/").length) { + matchingAnyPath = entry; + } } } else { int suffixIndex = expectedUri.indexOf(WILDCARD + "."); @@ -128,9 +134,9 @@ public abstract class PathMatcher

{ protected abstract Collection

getPaths(); - private boolean exactMatch(String expectedUri, String targetUri, String value) { - if (targetUri.equals(value)) { - return value.equals(targetUri); + private boolean exactMatch(String expectedUri, String targetUri) { + if (targetUri.equals(expectedUri)) { + return true; } if (endsWithWildcard(expectedUri)) { @@ -148,7 +154,8 @@ public abstract class PathMatcher

{ } protected String buildUriFromTemplate(String template, String targetUri, boolean onlyFirstParam) { - String expectedUri = template; + StringBuilder uri = new StringBuilder(template); + String expectedUri = uri.toString(); int patternStartIndex = expectedUri.indexOf("{"); if (expectedUri.endsWith("/*")) { @@ -171,6 +178,7 @@ public abstract class PathMatcher

{ if (Arrays.equals(matchingUri, Arrays.copyOf(targetUri.toCharArray(), matchingUri.length))) { matchingUri = Arrays.copyOf(matchingUri, targetUri.length()); int paramIndex = 0; + int lastPattern = 0; for (int i = patternStartIndex; i < expectedUriChars.length; i++) { if (matchingUriLastIndex >= matchingUri.length) { @@ -192,9 +200,21 @@ public abstract class PathMatcher

{ if (matchingUriLastIndex + paramLength > matchingUri.length) { return null; } + + StringBuilder value = new StringBuilder(); for (int j = 0; j < paramLength; j++) { - matchingUri[matchingUriLastIndex++] = params[k].charAt(j); + char valueChar = params[k].charAt(j); + value.append(valueChar); + matchingUri[matchingUriLastIndex++] = valueChar; + } + + if (c == '{') { + uri.replace(uri.indexOf("{", lastPattern), uri.indexOf("}", lastPattern) + 1, value.toString()); + } + + if (value.charAt(value.length() - 1) == '}') { + lastPattern = uri.indexOf(value.toString()) + value.length(); } if (c == '*' && matchingUriLastIndex < matchingUri.length) { @@ -217,18 +237,7 @@ public abstract class PathMatcher

{ } } - if (matchingUri[matchingUri.length - 1] == '\u0000') { - if (template.endsWith("*")) { - StringBuilder firstParam = new StringBuilder(String.valueOf(matchingUri).substring(0, matchingUriLastIndex)); - - firstParam.append(targetUri.substring(firstParam.length())); - - return firstParam.toString(); - } - return null; - } - - return String.valueOf(matchingUri); + return uri.toString(); } return null; diff --git a/common/src/test/java/org/keycloak/common/util/PathMatcherTest.java b/common/src/test/java/org/keycloak/common/util/PathMatcherTest.java index 2af7c68ffe..985faf948c 100644 --- a/common/src/test/java/org/keycloak/common/util/PathMatcherTest.java +++ b/common/src/test/java/org/keycloak/common/util/PathMatcherTest.java @@ -11,7 +11,7 @@ public class PathMatcherTest { public void keycloak15833Test() { TestingPathMatcher matcher = new TestingPathMatcher(); - Assert.assertNull(matcher.customBuildUriFromTemplate("/api/v1/{clientId}/campaigns/*/excelFiles", "/api/v1/1/contentConnectorConfigs/29/contentConnectorContents", false)); + Assert.assertEquals("/api/v1/1/campaigns/*/excelFiles", matcher.customBuildUriFromTemplate("/api/v1/{clientId}/campaigns/*/excelFiles", "/api/v1/1/contentConnectorConfigs/29/contentConnectorContents", false)); } private static final class TestingPathMatcher extends PathMatcher { diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/client/authorization/ResourceManagementWithAuthzClientTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/client/authorization/ResourceManagementWithAuthzClientTest.java index aa0319e9f8..996bc0d04d 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/client/authorization/ResourceManagementWithAuthzClientTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/client/authorization/ResourceManagementWithAuthzClientTest.java @@ -56,6 +56,11 @@ public class ResourceManagementWithAuthzClientTest extends ResourceManagementTes doCreateResource(new ResourceRepresentation("/resources/{pattern}/{pattern}/*", Collections.emptySet(), "/resources/{pattern}/{pattern}/*", null)); doCreateResource(new ResourceRepresentation("/resources/{pattern}/sub-resources/{pattern}/*", Collections.emptySet(), "/resources/{pattern}/sub-resources/{pattern}/*", null)); doCreateResource(new ResourceRepresentation("/resources/{pattern}/sub-resource", Collections.emptySet(), "/resources/{pattern}/sub-resources/{pattern}/*", null)); + doCreateResource(new ResourceRepresentation("/rest/{version}/loader/loadTwo", Collections.emptySet(), "/rest/{version}/loader/loadTwo", null)); + doCreateResource(new ResourceRepresentation("/rest/{version}/loader/load", Collections.emptySet(), "/rest/{version}/loader/load", null)); + doCreateResource(new ResourceRepresentation( + "/rest/{version}/carts/{cartId}/cartactions/{actionId}", Collections.emptySet(), "/rest/{version}/carts/{cartId}/cartactions/{actionId}", null)); + doCreateResource(new ResourceRepresentation("/rest/v1/carts/{cartId}/cartactions/123", Collections.emptySet(), "/rest/v1/carts/{cartId}/cartactions/123", null)); AuthzClient authzClient = getAuthzClient(); @@ -100,6 +105,60 @@ public class ResourceManagementWithAuthzClientTest extends ResourceManagementTes assertNotNull(resources); assertEquals(1, resources.size()); assertEquals("/resources/{pattern}/sub-resources/{pattern}/*", resources.get(0).getUri()); + + resources = authzClient.protection().resource().findByMatchingUri("/rest/v1/loader/load"); + + assertNotNull(resources); + assertEquals(1, resources.size()); + assertEquals("/rest/{version}/loader/load", resources.get(0).getUri()); + + resources = authzClient.protection().resource().findByMatchingUri("/rest/v2/carts/123/cartactions/123"); + + assertNotNull(resources); + assertEquals(1, resources.size()); + assertEquals("/rest/{version}/carts/{cartId}/cartactions/{actionId}", resources.get(0).getUri()); + + resources = authzClient.protection().resource().findByMatchingUri("/rest/v2/carts/{cartId}/cartactions/123"); + + assertNotNull(resources); + assertEquals(1, resources.size()); + assertEquals("/rest/{version}/carts/{cartId}/cartactions/{actionId}", resources.get(0).getUri()); + + resources = authzClient.protection().resource().findByMatchingUri("/rest/{version}/carts/123/cartactions/123"); + + assertNotNull(resources); + assertEquals(1, resources.size()); + assertEquals("/rest/{version}/carts/{cartId}/cartactions/{actionId}", resources.get(0).getUri()); + + resources = authzClient.protection().resource().findByMatchingUri("/rest/{version}/carts/{cartId}/cartactions/123"); + + assertNotNull(resources); + assertEquals(1, resources.size()); + assertEquals("/rest/{version}/carts/{cartId}/cartactions/{actionId}", resources.get(0).getUri()); + + resources = authzClient.protection().resource().findByMatchingUri("/rest/v1/carts/123/cartactions/123"); + + assertNotNull(resources); + assertEquals(1, resources.size()); + assertEquals("/rest/v1/carts/{cartId}/cartactions/123", resources.get(0).getUri()); + + resources = authzClient.protection().resource().findByMatchingUri("/rest/v1/carts/{cartId}/cartactions/123"); + + assertNotNull(resources); + assertEquals(1, resources.size()); + assertEquals("/rest/v1/carts/{cartId}/cartactions/123", resources.get(0).getUri()); + + resources = authzClient.protection().resource().findByMatchingUri("/rest/v1/carts/345/cartactions/123"); + + assertNotNull(resources); + assertEquals(1, resources.size()); + assertEquals("/rest/v1/carts/{cartId}/cartactions/123", resources.get(0).getUri()); + + resources = authzClient.protection().resource().findByMatchingUri("/rest/v2/carts/345/cartactions/123"); + + assertNotNull(resources); + assertEquals(1, resources.size()); + assertEquals("/rest/{version}/carts/{cartId}/cartactions/{actionId}", resources.get(0).getUri()); } @Test