Skip to content

Commit 0c1216c

Browse files
AithosaAithosa
authored
[#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.
1 parent 2951a6d commit 0c1216c

File tree

5 files changed

+187
-33
lines changed

5 files changed

+187
-33
lines changed

‎common/common-rest/src/main/java/org/apache/servicecomb/common/rest/definition/RestOperationMeta.java‎

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ public void init(OperationMeta operationMeta) {
102102
}
103103
}
104104

105-
setAbsolutePath(concatPath(SwaggerUtils.getBasePath(swagger), operationMeta.getOperationPath()));
105+
setAbsolutePath(SwaggerUtils.concatAbsolutePath(swagger, operationMeta.getOperationPath()));
106106
}
107107

108108
private void addRestParamByName(OperationMeta operationMeta, String name, Operation operation) {
@@ -168,21 +168,6 @@ public void setOperationMeta(OperationMeta operationMeta) {
168168
this.operationMeta = operationMeta;
169169
}
170170

171-
/**
172-
* Concat the two paths to an absolute path, end of '/' added.
173-
*
174-
* e.g. "/" + "/ope" = /ope/
175-
* e.g. "/prefix" + "/ope" = /prefix/ope/
176-
*/
177-
private String concatPath(String basePath, String operationPath) {
178-
return ("/" + nonNullify(basePath) + "/" + nonNullify(operationPath))
179-
.replaceAll("/{2,}", "/");
180-
}
181-
182-
private String nonNullify(String path) {
183-
return path == null ? "" : path;
184-
}
185-
186171
public String getAbsolutePath() {
187172
return this.absolutePath;
188173
}

‎core/src/main/java/org/apache/servicecomb/core/governance/MatchType.java‎

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ private GovernanceRequestExtractorImpl(Invocation invocation) {
3636
public String apiPath() {
3737
if (MatchType.REST.equalsIgnoreCase(invocation.getOperationMeta().getConfig().getGovernanceMatchType())) {
3838
if (!invocation.isProducer()) {
39-
return concatAbsolutePath(SwaggerUtils.getBasePath(invocation.getSchemaMeta().getSwagger()),
39+
return SwaggerUtils.concatAbsolutePath(invocation.getSchemaMeta().getSwagger(),
4040
invocation.getOperationMeta().getOperationPath());
4141
}
4242
// not highway
@@ -114,19 +114,4 @@ public Object sourceRequest() {
114114
public static GovernanceRequestExtractor createGovHttpRequest(Invocation invocation) {
115115
return new GovernanceRequestExtractorImpl(invocation);
116116
}
117-
118-
/**
119-
* Concat the two paths to an absolute path, without end of '/'.
120-
*
121-
* e.g. "/" + "/ope" = /ope
122-
* e.g. "/prefix" + "/ope" = /prefix/ope
123-
*/
124-
private static String concatAbsolutePath(String basePath, String operationPath) {
125-
return ("/" + nonNullify(basePath) + "/" + nonNullify(operationPath))
126-
.replaceAll("/{2,}", "/");
127-
}
128-
129-
private static String nonNullify(String path) {
130-
return path == null ? "" : path;
131-
}
132117
}

‎handlers/handler-publickey-auth/src/main/java/org/apache/servicecomb/authentication/provider/ProviderAuthFilter.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
import org.apache.servicecomb.core.filter.Filter;
2525
import org.apache.servicecomb.core.filter.FilterNode;
2626
import org.apache.servicecomb.core.filter.ProviderFilter;
27+
import org.apache.servicecomb.swagger.SwaggerUtils;
2728
import org.apache.servicecomb.swagger.invocation.Response;
2829
import org.apache.servicecomb.swagger.invocation.exception.InvocationException;
2930
import org.springframework.beans.factory.annotation.Autowired;
@@ -54,7 +55,9 @@ public String getName() {
5455

5556
@Override
5657
public CompletableFuture<Response> onFilter(Invocation invocation, FilterNode nextNode) {
57-
if (PathCheckUtils.isNotRequiredAuth(invocation.getOperationMeta().getOperationPath(), env)) {
58+
String requestFullPath = SwaggerUtils.concatAbsolutePath(invocation.getSchemaMeta().getSwagger(),
59+
invocation.getOperationMeta().getOperationPath());
60+
if (PathCheckUtils.isNotRequiredAuth(requestFullPath, env)) {
5861
return nextNode.onFilter(invocation);
5962
}
6063
String token = invocation.getContext(CoreConst.AUTH_TOKEN);

‎handlers/handler-publickey-auth/src/test/java/org/apache/servicecomb/authentication/provider/TestPathCheckUtils.java‎

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one or more
3+
* contributor license agreements. See the NOTICE file distributed with
4+
* this work for additional information regarding copyright ownership.
5+
* The ASF licenses this file to You under the Apache License, Version 2.0
6+
* (the "License"); you may not use this file except in compliance with
7+
* the License. You may obtain a copy of the License at
8+
*
9+
* http://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
package org.apache.servicecomb.authentication.provider;
18+
19+
import static org.junit.jupiter.api.Assertions.assertTrue;
20+
import static org.junit.jupiter.api.Assertions.assertFalse;
21+
import static org.mockito.Mockito.when;
22+
23+
import io.swagger.v3.oas.models.OpenAPI;
24+
import io.swagger.v3.oas.models.servers.Server;
25+
import org.apache.servicecomb.swagger.SwaggerUtils;
26+
import org.junit.jupiter.api.BeforeEach;
27+
import org.junit.jupiter.api.Test;
28+
import org.mockito.Mockito;
29+
import org.springframework.core.env.Environment;
30+
31+
import java.util.ArrayList;
32+
import java.util.List;
33+
import java.util.stream.Collectors;
34+
35+
public class TestPathCheckUtils {
36+
private static final String KEY_INCLUDE_PATH = "servicecomb.publicKey.accessControl.includePathPatterns";
37+
38+
private static final String KEY_EXCLUDE_PATH = "servicecomb.publicKey.accessControl.excludePathPatterns";
39+
40+
private static final String BASE_PATH = "/api/v1";
41+
42+
private Environment environment;
43+
44+
private OpenAPI swagger;
45+
46+
@BeforeEach
47+
public void setUp() {
48+
environment = Mockito.mock(Environment.class);
49+
swagger = new OpenAPI();
50+
swagger.setServers(new ArrayList<>());
51+
swagger.getServers().add(new Server().url(BASE_PATH));
52+
}
53+
54+
@Test
55+
public void testExcludePathWithBasePathAndExactMatch() {
56+
String operationPath = "/public";
57+
when(environment.getProperty(KEY_EXCLUDE_PATH, "")).thenReturn(BASE_PATH + operationPath);
58+
59+
String fullPath = SwaggerUtils.concatAbsolutePath(swagger, operationPath);
60+
assertTrue(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
61+
"Should not require auth for excluded path with exact match");
62+
}
63+
64+
@Test
65+
public void testExcludePathWithBasePathAndWildcard() {
66+
when(environment.getProperty(KEY_EXCLUDE_PATH, "")).thenReturn(BASE_PATH + "/public/*");
67+
68+
String fullPath = SwaggerUtils.concatAbsolutePath(swagger, "/public/test");
69+
assertTrue(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
70+
"Should not require auth for excluded path with wildcard");
71+
72+
fullPath = SwaggerUtils.concatAbsolutePath(swagger, "/public/nested/path");
73+
assertTrue(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
74+
"Should not require auth for excluded nested path with wildcard");
75+
}
76+
77+
@Test
78+
public void testIncludePathWithBasePath() {
79+
when(environment.getProperty(KEY_EXCLUDE_PATH, "")).thenReturn("");
80+
when(environment.getProperty(KEY_INCLUDE_PATH, "")).thenReturn(BASE_PATH + "/private/*");
81+
82+
String fullPath = SwaggerUtils.concatAbsolutePath(swagger, "/private/resource");
83+
assertFalse(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
84+
"Should require auth for included path");
85+
86+
fullPath = SwaggerUtils.concatAbsolutePath(swagger, "/public/resource");
87+
assertTrue(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
88+
"Should not require auth for non-included path");
89+
}
90+
91+
@Test
92+
public void testExcludeOverrideIncludePath() {
93+
when(environment.getProperty(KEY_EXCLUDE_PATH, "")).thenReturn(BASE_PATH + "/resource");
94+
when(environment.getProperty(KEY_INCLUDE_PATH, "")).thenReturn(BASE_PATH + "/*");
95+
96+
String fullPath = SwaggerUtils.concatAbsolutePath(swagger, "/resource");
97+
assertTrue(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
98+
"Exclude patterns should override include patterns");
99+
}
100+
101+
@Test
102+
public void testMultipleExcludePaths() {
103+
List<String> operationPaths = List.of("/public", "/health", "/metrics/*");
104+
when(environment.getProperty(KEY_EXCLUDE_PATH, "")).thenReturn(concatPath(BASE_PATH, operationPaths));
105+
106+
String fullPath = SwaggerUtils.concatAbsolutePath(swagger, "/public");
107+
assertTrue(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
108+
"Should not require auth for first exclude path");
109+
110+
fullPath = SwaggerUtils.concatAbsolutePath(swagger, "/health");
111+
assertTrue(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
112+
"Should not require auth for second exclude path");
113+
114+
fullPath = SwaggerUtils.concatAbsolutePath(swagger, "/metrics/jvm");
115+
assertTrue(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
116+
"Should not require auth for wildcard exclude path");
117+
}
118+
119+
@Test
120+
public void testDifferentBasePath() {
121+
String basePath = "/different/base";
122+
String publicOperationPath = "/public";
123+
String privateOperationPath = "/private";
124+
swagger.getServers().clear();
125+
swagger.getServers().add(new Server().url(basePath));
126+
when(environment.getProperty(KEY_EXCLUDE_PATH, "")).thenReturn(basePath + publicOperationPath);
127+
128+
String fullPath = SwaggerUtils.concatAbsolutePath(swagger, publicOperationPath);
129+
assertTrue(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
130+
"Should not require auth with different base path");
131+
132+
fullPath = SwaggerUtils.concatAbsolutePath(swagger, privateOperationPath);
133+
assertFalse(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
134+
"Should require auth for non-excluded path with different base path");
135+
}
136+
137+
@Test
138+
public void testNoBasePath() {
139+
String operationPath = "/public";
140+
swagger.setServers(null);
141+
when(environment.getProperty(KEY_EXCLUDE_PATH, "")).thenReturn(operationPath);
142+
143+
String fullPath = SwaggerUtils.concatAbsolutePath(swagger, operationPath);
144+
assertTrue(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
145+
"Should not require auth when no base path is set");
146+
}
147+
148+
@Test
149+
public void testEmptyConfiguration() {
150+
when(environment.getProperty(KEY_EXCLUDE_PATH, "")).thenReturn("");
151+
when(environment.getProperty(KEY_INCLUDE_PATH, "")).thenReturn("");
152+
153+
String fullPath = SwaggerUtils.concatAbsolutePath(swagger, "/any/path");
154+
assertFalse(PathCheckUtils.isNotRequiredAuth(fullPath, environment),
155+
"Should require auth by default when no patterns are configured");
156+
}
157+
158+
private String concatPath(String basePath, List<String> paths) {
159+
return paths.stream().map(path -> basePath + path).collect(Collectors.joining(","));
160+
}
161+
}

‎swagger/swagger-generator/generator-core/src/main/java/org/apache/servicecomb/swagger/SwaggerUtils.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -445,4 +445,24 @@ public static boolean methodExists(PathItem pathItem, String httpMethod) {
445445
default -> false;
446446
};
447447
}
448+
449+
public static String concatAbsolutePath(OpenAPI swagger, String operationPath) {
450+
String basePath = getBasePath(swagger);
451+
return concatPath(basePath, operationPath);
452+
}
453+
454+
/**
455+
* Concat the two paths to an absolute path, without end of '/'.
456+
* <p>
457+
* e.g. "/" + "/ope" = /ope
458+
* e.g. "/prefix" + "/ope" = /prefix/ope
459+
*/
460+
public static String concatPath(String basePath, String operationPath) {
461+
return ("/" + nonNullify(basePath) + "/" + nonNullify(operationPath))
462+
.replaceAll("/{2,}", "/");
463+
}
464+
465+
private static String nonNullify(String path) {
466+
return path == null ? "" : path;
467+
}
448468
}

0 commit comments

Comments
 (0)