-
Notifications
You must be signed in to change notification settings - Fork 590
chore(server): clear context after req done #2470
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ae90955
bb67693
13f916f
30f1821
2519b39
c787baf
7d75e0d
552dcb8
ba74aaa
4f7fc0f
43288d9
6104ed6
906c0de
177e513
165b5c8
7eeda25
5aa1a40
6030882
41459f5
1435e44
3643116
7484ab3
6a50b99
ed2b24a
80c1634
ed250f2
2e3325c
5817ccd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,7 +45,7 @@ | |
| import org.slf4j.Logger; | ||
|
|
||
| import com.alipay.remoting.util.StringUtils; | ||
| import com.google.common.collect.ImmutableList; | ||
| import com.google.common.collect.ImmutableSet; | ||
|
|
||
| import jakarta.annotation.Priority; | ||
| import jakarta.ws.rs.BadRequestException; | ||
|
|
@@ -71,15 +71,15 @@ public class AuthenticationFilter implements ContainerRequestFilter { | |
|
|
||
| private static final Logger LOG = Log.logger(AuthenticationFilter.class); | ||
|
|
||
| private static final List<String> WHITE_API_LIST = ImmutableList.of( | ||
| "graphs/*/auth/login", | ||
| private static final AntPathMatcher MATCHER = new AntPathMatcher(); | ||
| private static final Set<String> FIXED_WHITE_API_SET = ImmutableSet.of( | ||
| "versions", | ||
| "openapi.json" | ||
| ); | ||
| private static final AntPathMatcher MATCHER = new AntPathMatcher(); | ||
|
|
||
| private static String whiteIpStatus; | ||
| /** Remove auth/login API from whitelist */ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. one * is ok |
||
| private static final Set<String> FLEXIBLE_WHITE_API_SET = ImmutableSet.of(); | ||
|
|
||
| private static Boolean enabledWhiteIpCheck; | ||
| private static final String STRING_WHITE_IP_LIST = "whiteiplist"; | ||
| private static final String STRING_ENABLE = "enable"; | ||
|
|
||
|
|
@@ -94,7 +94,7 @@ public class AuthenticationFilter implements ContainerRequestFilter { | |
|
|
||
| @Override | ||
| public void filter(ContainerRequestContext context) throws IOException { | ||
| if (AuthenticationFilter.isWhiteAPI(context)) { | ||
| if (isWhiteAPI(context)) { | ||
| return; | ||
| } | ||
| User user = this.authenticate(context); | ||
|
|
@@ -107,7 +107,7 @@ protected User authenticate(ContainerRequestContext context) { | |
| E.checkState(manager != null, "Context GraphManager is absent"); | ||
|
|
||
| if (!manager.requireAuthentication()) { | ||
| // Return anonymous user with admin role if disable authentication | ||
| // Return anonymous user with an admin role if disable authentication | ||
| return User.ANONYMOUS; | ||
| } | ||
|
|
||
|
|
@@ -121,11 +121,12 @@ protected User authenticate(ContainerRequestContext context) { | |
| } | ||
|
|
||
| // Check whiteIp | ||
| if (whiteIpStatus == null) { | ||
| whiteIpStatus = this.configProvider.get().get(WHITE_IP_STATUS); | ||
| if (enabledWhiteIpCheck == null) { | ||
| String whiteIpStatus = this.configProvider.get().get(WHITE_IP_STATUS); | ||
| enabledWhiteIpCheck = Objects.equals(whiteIpStatus, STRING_ENABLE); | ||
| } | ||
|
|
||
| if (Objects.equals(whiteIpStatus, STRING_ENABLE) && request != null) { | ||
| if (enabledWhiteIpCheck && request != null) { | ||
| peer = request.getRemoteAddr() + ":" + request.getRemotePort(); | ||
| path = request.getRequestURI(); | ||
|
|
||
|
|
@@ -134,38 +135,32 @@ protected User authenticate(ContainerRequestContext context) { | |
| boolean whiteIpEnabled = manager.authManager().getWhiteIpStatus(); | ||
| if (!path.contains(STRING_WHITE_IP_LIST) && whiteIpEnabled && | ||
| !whiteIpList.contains(remoteIp)) { | ||
| throw new ForbiddenException( | ||
| String.format("Remote ip '%s' is not permitted", | ||
| remoteIp)); | ||
| throw new ForbiddenException(String.format("Remote ip '%s' is not permitted", | ||
| remoteIp)); | ||
| } | ||
| } | ||
|
|
||
| Map<String, String> credentials = new HashMap<>(); | ||
| // Extract authentication credentials | ||
| String auth = context.getHeaderString(HttpHeaders.AUTHORIZATION); | ||
| if (auth == null) { | ||
| throw new NotAuthorizedException( | ||
| "Authentication credentials are required", | ||
| "Missing authentication credentials"); | ||
| throw new NotAuthorizedException("Authentication credentials are required", | ||
| "Missing authentication credentials"); | ||
| } | ||
|
|
||
| if (auth.startsWith(BASIC_AUTH_PREFIX)) { | ||
| auth = auth.substring(BASIC_AUTH_PREFIX.length()); | ||
| auth = new String(DatatypeConverter.parseBase64Binary(auth), | ||
| Charsets.ASCII_CHARSET); | ||
| auth = new String(DatatypeConverter.parseBase64Binary(auth), Charsets.ASCII_CHARSET); | ||
| String[] values = auth.split(":"); | ||
| if (values.length != 2) { | ||
| throw new BadRequestException( | ||
| "Invalid syntax for username and password"); | ||
| throw new BadRequestException("Invalid syntax for username and password"); | ||
| } | ||
|
|
||
| final String username = values[0]; | ||
| final String password = values[1]; | ||
|
|
||
| if (StringUtils.isEmpty(username) || | ||
| StringUtils.isEmpty(password)) { | ||
| throw new BadRequestException( | ||
| "Invalid syntax for username and password"); | ||
| if (StringUtils.isEmpty(username) || StringUtils.isEmpty(password)) { | ||
| throw new BadRequestException("Invalid syntax for username and password"); | ||
| } | ||
|
|
||
| credentials.put(HugeAuthenticator.KEY_USERNAME, username); | ||
|
|
@@ -174,8 +169,7 @@ protected User authenticate(ContainerRequestContext context) { | |
| String token = auth.substring(BEARER_TOKEN_PREFIX.length()); | ||
| credentials.put(HugeAuthenticator.KEY_TOKEN, token); | ||
| } else { | ||
| throw new BadRequestException( | ||
| "Only HTTP Basic or Bearer authentication is supported"); | ||
| throw new BadRequestException("Only HTTP Basic or Bearer authentication is supported"); | ||
| } | ||
|
|
||
| credentials.put(HugeAuthenticator.KEY_ADDRESS, peer); | ||
|
|
@@ -185,8 +179,7 @@ protected User authenticate(ContainerRequestContext context) { | |
| try { | ||
| return manager.authenticate(credentials); | ||
| } catch (AuthenticationException e) { | ||
| throw new NotAuthorizedException("Authentication failed", | ||
| e.getMessage()); | ||
| throw new NotAuthorizedException("Authentication failed", e.getMessage()); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -250,7 +243,7 @@ private boolean matchPermission(String required) { | |
| requiredPerm = RequiredPerm.fromPermission(required); | ||
|
|
||
| /* | ||
| * Replace owner value(it may be a variable) if the permission | ||
| * Replace owner value (it may be a variable) if the permission | ||
| * format like: "$owner=$graph $action=vertex_write" | ||
| */ | ||
| String owner = requiredPerm.owner(); | ||
|
|
@@ -316,7 +309,11 @@ public boolean equals(Object obj) { | |
|
|
||
| public static boolean isWhiteAPI(ContainerRequestContext context) { | ||
| String path = context.getUriInfo().getPath(); | ||
| for (String whiteApi : WHITE_API_LIST) { | ||
| if (FIXED_WHITE_API_SET.contains(path)) { | ||
| return true; | ||
| } | ||
|
|
||
| for (String whiteApi : FLEXIBLE_WHITE_API_SET) { | ||
| if (MATCHER.match(whiteApi, path)) { | ||
| return true; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,8 @@ | |
| import org.apache.tinkerpop.gremlin.server.auth.Authenticator; | ||
| import org.apache.tinkerpop.shaded.jackson.annotation.JsonProperty; | ||
|
|
||
| import jakarta.ws.rs.core.SecurityContext; | ||
|
|
||
| public interface HugeAuthenticator extends Authenticator { | ||
|
|
||
| String KEY_USERNAME = CredentialGraphTokens.PROPERTY_USERNAME; | ||
|
|
@@ -64,6 +66,8 @@ public interface HugeAuthenticator extends Authenticator { | |
|
|
||
| UserWithRole authenticate(String username, String password, String token); | ||
|
|
||
| void unauthorize(SecurityContext context); | ||
|
|
||
| AuthManager authManager(); | ||
|
|
||
| HugeGraph graph(); | ||
|
|
@@ -103,10 +107,7 @@ default User authenticate(final Map<String, String> credentials) | |
| } | ||
|
|
||
| HugeGraphAuthProxy.logUser(user, credentials.get(KEY_PATH)); | ||
| /* | ||
| * Set authentication context | ||
| * TODO: unset context after finishing a request | ||
| */ | ||
| // TODO: Ensure context lifecycle in GraphServer & AuthServer(#AccessLogFilter) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't need TODO mark anymore since it's done |
||
| HugeGraphAuthProxy.setContext(new Context(user)); | ||
|
|
||
| return user; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,21 +38,35 @@ public void testArthasStart() { | |
|
|
||
| @Test | ||
| public void testArthasApi() { | ||
| String body = "{\n" + | ||
| " \"action\": \"exec\",\n" + | ||
| " \"command\": \"version\"\n" + | ||
| "}"; | ||
| // command exec | ||
| String execBody = "{\n" + | ||
| " \"action\": \"exec\",\n" + | ||
| " \"command\": \"version\"\n" + | ||
| "}"; | ||
| RestClient arthasApiClient = new RestClient(ARTHAS_API_BASE_URL, false); | ||
| // If the request header contains basic auth, | ||
| // and if we are not set auth when arthas attach hg, arthas will auth it and return 401. | ||
| // ref:https://arthas.aliyun.com/en/doc/auth.html#configure-username-and-password | ||
| Response r = arthasApiClient.post(ARTHAS_API_PATH, body); | ||
| String result = assertResponseStatus(200, r); | ||
| Response execResponse = arthasApiClient.post(ARTHAS_API_PATH, execBody); | ||
| String result = assertResponseStatus(200, execResponse); | ||
| assertJsonContains(result, "state"); | ||
| assertJsonContains(result, "body"); | ||
|
|
||
| RestClient arthasApiClientWithAuth = new RestClient(ARTHAS_API_BASE_URL); | ||
| r = arthasApiClientWithAuth.post(ARTHAS_API_PATH, body); | ||
| assertResponseStatus(401, r); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. prefer to add some negative test cases |
||
| // command session | ||
| String sessionBody = "{\n" + | ||
| " \"action\":\"init_session\"\n" + | ||
| "}"; | ||
| Response sessionResponse = arthasApiClient.post(ARTHAS_API_PATH, sessionBody); | ||
| String sessionResult = assertResponseStatus(200, sessionResponse); | ||
| assertJsonContains(sessionResult, "sessionId"); | ||
| assertJsonContains(sessionResult, "consumerId"); | ||
| assertJsonContains(sessionResult, "state"); | ||
|
|
||
imbajin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // join session: using invalid sessionId | ||
| String joinSessionBody = "{\n" + | ||
| " \"action\":\"join_session\",\n" + | ||
| " \"sessionId\" : \"xxx\"\n" + | ||
| "}"; | ||
| Response joinSessionResponse = arthasApiClient.post(ARTHAS_API_PATH, joinSessionBody); | ||
| String joinSessionResult = assertResponseStatus(200, joinSessionResponse); | ||
| assertJsonContains(joinSessionResult, "message"); | ||
| assertJsonContains(joinSessionResult, "state"); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't need TODO mark anymore since it's done