This is an automated email from the ASF dual-hosted git repository.
chenzhida pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/servicecomb-java-chassis.git
The following commit(s) were added to refs/heads/master by this push:
new 0c1216c48 [#4722] Fix bug in `ProviderAuthFilter` where `requestPath`
not absolute problem (#4725)
0c1216c48 is described below
commit 0c1216c4872a3533df0d99f3942b3b5b87a18ee2
Author: Aithusa <[email protected]>
AuthorDate: Fri Feb 28 15:02:55 2025 +0800
[#4722] Fix bug in `ProviderAuthFilter` where `requestPath` not absolute
problem (#4725)
* [#4722] fix provider auth filter requestPath not absolute problem
1. Fix problem that ProviderAuthFilter not use absolute path.
2. same logic has come up twice, move it to SwaggerUtils.
* [#4722] add tests for `excludePathPatterns`/`includePathPatterns` feature
in `handler-publickey-auth`
* [#4722] fix test assertion formatting issue
* [#4722] replaced hardcoded path strings in tests with predefined
constants for better maintainability and readability.
---
.../common/rest/definition/RestOperationMeta.java | 17 +--
.../servicecomb/core/governance/MatchType.java | 17 +--
.../provider/ProviderAuthFilter.java | 5 +-
.../provider/TestPathCheckUtils.java | 161 +++++++++++++++++++++
.../apache/servicecomb/swagger/SwaggerUtils.java | 20 +++
5 files changed, 187 insertions(+), 33 deletions(-)
diff --git
a/common/common-rest/src/main/java/org/apache/servicecomb/common/rest/definition/RestOperationMeta.java
b/common/common-rest/src/main/java/org/apache/servicecomb/common/rest/definition/RestOperationMeta.java
index 6b2e3b332..580175ec6 100644
---
a/common/common-rest/src/main/java/org/apache/servicecomb/common/rest/definition/RestOperationMeta.java
+++
b/common/common-rest/src/main/java/org/apache/servicecomb/common/rest/definition/RestOperationMeta.java
@@ -102,7 +102,7 @@ public class RestOperationMeta {
}
}
- setAbsolutePath(concatPath(SwaggerUtils.getBasePath(swagger),
operationMeta.getOperationPath()));
+ setAbsolutePath(SwaggerUtils.concatAbsolutePath(swagger,
operationMeta.getOperationPath()));
}
private void addRestParamByName(OperationMeta operationMeta, String name,
Operation operation) {
@@ -168,21 +168,6 @@ public class RestOperationMeta {
this.operationMeta = operationMeta;
}
- /**
- * Concat the two paths to an absolute path, end of '/' added.
- *
- * e.g. "/" + "/ope" = /ope/
- * e.g. "/prefix" + "/ope" = /prefix/ope/
- */
- private String concatPath(String basePath, String operationPath) {
- return ("/" + nonNullify(basePath) + "/" + nonNullify(operationPath))
- .replaceAll("/{2,}", "/");
- }
-
- private String nonNullify(String path) {
- return path == null ? "" : path;
- }
-
public String getAbsolutePath() {
return this.absolutePath;
}
diff --git
a/core/src/main/java/org/apache/servicecomb/core/governance/MatchType.java
b/core/src/main/java/org/apache/servicecomb/core/governance/MatchType.java
index 2c1006706..15d58d666 100644
--- a/core/src/main/java/org/apache/servicecomb/core/governance/MatchType.java
+++ b/core/src/main/java/org/apache/servicecomb/core/governance/MatchType.java
@@ -36,7 +36,7 @@ public final class MatchType {
public String apiPath() {
if
(MatchType.REST.equalsIgnoreCase(invocation.getOperationMeta().getConfig().getGovernanceMatchType()))
{
if (!invocation.isProducer()) {
- return
concatAbsolutePath(SwaggerUtils.getBasePath(invocation.getSchemaMeta().getSwagger()),
+ return
SwaggerUtils.concatAbsolutePath(invocation.getSchemaMeta().getSwagger(),
invocation.getOperationMeta().getOperationPath());
}
// not highway
@@ -114,19 +114,4 @@ public final class MatchType {
public static GovernanceRequestExtractor createGovHttpRequest(Invocation
invocation) {
return new GovernanceRequestExtractorImpl(invocation);
}
-
- /**
- * Concat the two paths to an absolute path, without end of '/'.
- *
- * e.g. "/" + "/ope" = /ope
- * e.g. "/prefix" + "/ope" = /prefix/ope
- */
- private static String concatAbsolutePath(String basePath, String
operationPath) {
- return ("/" + nonNullify(basePath) + "/" + nonNullify(operationPath))
- .replaceAll("/{2,}", "/");
- }
-
- private static String nonNullify(String path) {
- return path == null ? "" : path;
- }
}
diff --git
a/handlers/handler-publickey-auth/src/main/java/org/apache/servicecomb/authentication/provider/ProviderAuthFilter.java
b/handlers/handler-publickey-auth/src/main/java/org/apache/servicecomb/authentication/provider/ProviderAuthFilter.java
index 02b1108a5..f525ae88c 100644
---
a/handlers/handler-publickey-auth/src/main/java/org/apache/servicecomb/authentication/provider/ProviderAuthFilter.java
+++
b/handlers/handler-publickey-auth/src/main/java/org/apache/servicecomb/authentication/provider/ProviderAuthFilter.java
@@ -24,6 +24,7 @@ import org.apache.servicecomb.core.filter.AbstractFilter;
import org.apache.servicecomb.core.filter.Filter;
import org.apache.servicecomb.core.filter.FilterNode;
import org.apache.servicecomb.core.filter.ProviderFilter;
+import org.apache.servicecomb.swagger.SwaggerUtils;
import org.apache.servicecomb.swagger.invocation.Response;
import org.apache.servicecomb.swagger.invocation.exception.InvocationException;
import org.springframework.beans.factory.annotation.Autowired;
@@ -54,7 +55,9 @@ public class ProviderAuthFilter extends AbstractFilter
implements ProviderFilter
@Override
public CompletableFuture<Response> onFilter(Invocation invocation,
FilterNode nextNode) {
- if
(PathCheckUtils.isNotRequiredAuth(invocation.getOperationMeta().getOperationPath(),
env)) {
+ String requestFullPath =
SwaggerUtils.concatAbsolutePath(invocation.getSchemaMeta().getSwagger(),
+ invocation.getOperationMeta().getOperationPath());
+ if (PathCheckUtils.isNotRequiredAuth(requestFullPath, env)) {
return nextNode.onFilter(invocation);
}
String token = invocation.getContext(CoreConst.AUTH_TOKEN);
diff --git
a/handlers/handler-publickey-auth/src/test/java/org/apache/servicecomb/authentication/provider/TestPathCheckUtils.java
b/handlers/handler-publickey-auth/src/test/java/org/apache/servicecomb/authentication/provider/TestPathCheckUtils.java
new file mode 100644
index 000000000..b8ff46230
--- /dev/null
+++
b/handlers/handler-publickey-auth/src/test/java/org/apache/servicecomb/authentication/provider/TestPathCheckUtils.java
@@ -0,0 +1,161 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.servicecomb.authentication.provider;
+
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.mockito.Mockito.when;
+
+import io.swagger.v3.oas.models.OpenAPI;
+import io.swagger.v3.oas.models.servers.Server;
+import org.apache.servicecomb.swagger.SwaggerUtils;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+import org.springframework.core.env.Environment;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.stream.Collectors;
+
+public class TestPathCheckUtils {
+ private static final String KEY_INCLUDE_PATH =
"servicecomb.publicKey.accessControl.includePathPatterns";
+
+ private static final String KEY_EXCLUDE_PATH =
"servicecomb.publicKey.accessControl.excludePathPatterns";
+
+ private static final String BASE_PATH = "/api/v1";
+
+ private Environment environment;
+
+ private OpenAPI swagger;
+
+ @BeforeEach
+ public void setUp() {
+ environment = Mockito.mock(Environment.class);
+ swagger = new OpenAPI();
+ swagger.setServers(new ArrayList<>());
+ swagger.getServers().add(new Server().url(BASE_PATH));
+ }
+
+ @Test
+ public void testExcludePathWithBasePathAndExactMatch() {
+ String operationPath = "/public";
+ when(environment.getProperty(KEY_EXCLUDE_PATH, "")).thenReturn(BASE_PATH +
operationPath);
+
+ String fullPath = SwaggerUtils.concatAbsolutePath(swagger, operationPath);
+ assertTrue(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
+ "Should not require auth for excluded path with exact match");
+ }
+
+ @Test
+ public void testExcludePathWithBasePathAndWildcard() {
+ when(environment.getProperty(KEY_EXCLUDE_PATH, "")).thenReturn(BASE_PATH +
"/public/*");
+
+ String fullPath = SwaggerUtils.concatAbsolutePath(swagger, "/public/test");
+ assertTrue(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
+ "Should not require auth for excluded path with wildcard");
+
+ fullPath = SwaggerUtils.concatAbsolutePath(swagger, "/public/nested/path");
+ assertTrue(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
+ "Should not require auth for excluded nested path with wildcard");
+ }
+
+ @Test
+ public void testIncludePathWithBasePath() {
+ when(environment.getProperty(KEY_EXCLUDE_PATH, "")).thenReturn("");
+ when(environment.getProperty(KEY_INCLUDE_PATH, "")).thenReturn(BASE_PATH +
"/private/*");
+
+ String fullPath = SwaggerUtils.concatAbsolutePath(swagger,
"/private/resource");
+ assertFalse(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
+ "Should require auth for included path");
+
+ fullPath = SwaggerUtils.concatAbsolutePath(swagger, "/public/resource");
+ assertTrue(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
+ "Should not require auth for non-included path");
+ }
+
+ @Test
+ public void testExcludeOverrideIncludePath() {
+ when(environment.getProperty(KEY_EXCLUDE_PATH, "")).thenReturn(BASE_PATH +
"/resource");
+ when(environment.getProperty(KEY_INCLUDE_PATH, "")).thenReturn(BASE_PATH +
"/*");
+
+ String fullPath = SwaggerUtils.concatAbsolutePath(swagger, "/resource");
+ assertTrue(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
+ "Exclude patterns should override include patterns");
+ }
+
+ @Test
+ public void testMultipleExcludePaths() {
+ List<String> operationPaths = List.of("/public", "/health", "/metrics/*");
+ when(environment.getProperty(KEY_EXCLUDE_PATH,
"")).thenReturn(concatPath(BASE_PATH, operationPaths));
+
+ String fullPath = SwaggerUtils.concatAbsolutePath(swagger, "/public");
+ assertTrue(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
+ "Should not require auth for first exclude path");
+
+ fullPath = SwaggerUtils.concatAbsolutePath(swagger, "/health");
+ assertTrue(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
+ "Should not require auth for second exclude path");
+
+ fullPath = SwaggerUtils.concatAbsolutePath(swagger, "/metrics/jvm");
+ assertTrue(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
+ "Should not require auth for wildcard exclude path");
+ }
+
+ @Test
+ public void testDifferentBasePath() {
+ String basePath = "/different/base";
+ String publicOperationPath = "/public";
+ String privateOperationPath = "/private";
+ swagger.getServers().clear();
+ swagger.getServers().add(new Server().url(basePath));
+ when(environment.getProperty(KEY_EXCLUDE_PATH, "")).thenReturn(basePath +
publicOperationPath);
+
+ String fullPath = SwaggerUtils.concatAbsolutePath(swagger,
publicOperationPath);
+ assertTrue(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
+ "Should not require auth with different base path");
+
+ fullPath = SwaggerUtils.concatAbsolutePath(swagger, privateOperationPath);
+ assertFalse(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
+ "Should require auth for non-excluded path with different base path");
+ }
+
+ @Test
+ public void testNoBasePath() {
+ String operationPath = "/public";
+ swagger.setServers(null);
+ when(environment.getProperty(KEY_EXCLUDE_PATH,
"")).thenReturn(operationPath);
+
+ String fullPath = SwaggerUtils.concatAbsolutePath(swagger, operationPath);
+ assertTrue(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
+ "Should not require auth when no base path is set");
+ }
+
+ @Test
+ public void testEmptyConfiguration() {
+ when(environment.getProperty(KEY_EXCLUDE_PATH, "")).thenReturn("");
+ when(environment.getProperty(KEY_INCLUDE_PATH, "")).thenReturn("");
+
+ String fullPath = SwaggerUtils.concatAbsolutePath(swagger, "/any/path");
+ assertFalse(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
+ "Should require auth by default when no patterns are configured");
+ }
+
+ private String concatPath(String basePath, List<String> paths) {
+ return paths.stream().map(path -> basePath +
path).collect(Collectors.joining(","));
+ }
+}
diff --git
a/swagger/swagger-generator/generator-core/src/main/java/org/apache/servicecomb/swagger/SwaggerUtils.java
b/swagger/swagger-generator/generator-core/src/main/java/org/apache/servicecomb/swagger/SwaggerUtils.java
index 211a612a8..c1751b728 100644
---
a/swagger/swagger-generator/generator-core/src/main/java/org/apache/servicecomb/swagger/SwaggerUtils.java
+++
b/swagger/swagger-generator/generator-core/src/main/java/org/apache/servicecomb/swagger/SwaggerUtils.java
@@ -445,4 +445,24 @@ public final class SwaggerUtils {
default -> false;
};
}
+
+ public static String concatAbsolutePath(OpenAPI swagger, String
operationPath) {
+ String basePath = getBasePath(swagger);
+ return concatPath(basePath, operationPath);
+ }
+
+ /**
+ * Concat the two paths to an absolute path, without end of '/'.
+ * <p>
+ * e.g. "/" + "/ope" = /ope
+ * e.g. "/prefix" + "/ope" = /prefix/ope
+ */
+ public static String concatPath(String basePath, String operationPath) {
+ return ("/" + nonNullify(basePath) + "/" + nonNullify(operationPath))
+ .replaceAll("/{2,}", "/");
+ }
+
+ private static String nonNullify(String path) {
+ return path == null ? "" : path;
+ }
}
