You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hugegraph.apache.org by "SunnyBoy-WYH (via GitHub)" <gi...@apache.org> on 2024/03/03 13:53:01 UTC
[PR] chore(server): clear context after req done [incubator-hugegraph]
SunnyBoy-WYH opened a new pull request, #2470:
URL: https://github.com/apache/incubator-hugegraph/pull/2470
complete the history todo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore(server): clear context after req done [incubator-hugegraph]
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on code in PR #2470:
URL: https://github.com/apache/incubator-hugegraph/pull/2470#discussion_r1518716802
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java:
##########
@@ -1818,7 +1818,7 @@ protected static Context setContext(Context context) {
return old;
}
- protected static void resetContext() {
+ public static void resetContext() {
Review Comment:
prefer to keep protected, just let HugeAuthenticator call resetContext()
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore(server): clear context after req done [incubator-hugegraph]
Posted by "VGalaxies (via GitHub)" <gi...@apache.org>.
VGalaxies merged PR #2470:
URL: https://github.com/apache/incubator-hugegraph/pull/2470
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore(server): clear context after req done [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2470:
URL: https://github.com/apache/incubator-hugegraph/pull/2470#discussion_r1518799326
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java:
##########
@@ -71,15 +72,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();
+ // Remove auth/login API from white list
Review Comment:
> we need to make sure the login api is accessible, it's used to get a user token. and also update the comment after checked.
@javeme Some context for the modification:
1. When we `unset-context` in AccessLogFilter, the CI will **fail** in `login` when(login) whiteList exist (refer [CI link](https://github.com/apache/incubator-hugegraph/actions/runs/8173840336/job/22347189327?pr=2470))
<img width="1434" alt="image" src="https://github.com/apache/incubator-hugegraph/assets/17706099/a11c895d-56d8-4e10-a0e1-0bb9f604414e">
2. follow the `login` API input param & the validate logic
<img width="942" alt="image" src="https://github.com/apache/incubator-hugegraph/assets/17706099/1cc81ec4-303e-4d5e-b093-08cfa96aa2e5">
3. `login` is only used for hubble, and if we need set whitelist for `login.jsp`(to make user visit it without auth) , seems we should **set it** in `hubble` rather than set it in `Graph/AuthServer` -> AuthFilter, and after we remove the `login` API from the whitelist, the login is fine now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore(server): clear context after req done [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2470:
URL: https://github.com/apache/incubator-hugegraph/pull/2470#discussion_r1518799326
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java:
##########
@@ -71,15 +72,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();
+ // Remove auth/login API from white list
Review Comment:
> we need to make sure the login api is accessible, it's used to get a user token. and also update the comment after checked.
Some context for the modification:
1. When we `unset-context` in AccessLogFilter, the CI will **fail** in `login` when(login) whiteList exist (refer [CI link](https://github.com/apache/incubator-hugegraph/actions/runs/8173840336/job/22347189327?pr=2470))
<img width="1434" alt="image" src="https://github.com/apache/incubator-hugegraph/assets/17706099/a11c895d-56d8-4e10-a0e1-0bb9f604414e">
2. follow the `login` API input param & the validate logic
<img width="942" alt="image" src="https://github.com/apache/incubator-hugegraph/assets/17706099/1cc81ec4-303e-4d5e-b093-08cfa96aa2e5">
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java:
##########
@@ -71,15 +72,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();
+ // Remove auth/login API from white list
Review Comment:
> we need to make sure the login api is accessible, it's used to get a user token. and also update the comment after checked.
@javeme Some context for the modification:
1. When we `unset-context` in AccessLogFilter, the CI will **fail** in `login` when(login) whiteList exist (refer [CI link](https://github.com/apache/incubator-hugegraph/actions/runs/8173840336/job/22347189327?pr=2470))
<img width="1434" alt="image" src="https://github.com/apache/incubator-hugegraph/assets/17706099/a11c895d-56d8-4e10-a0e1-0bb9f604414e">
2. follow the `login` API input param & the validate logic
<img width="942" alt="image" src="https://github.com/apache/incubator-hugegraph/assets/17706099/1cc81ec4-303e-4d5e-b093-08cfa96aa2e5">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore(server): clear context after req done [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2470:
URL: https://github.com/apache/incubator-hugegraph/pull/2470#discussion_r1510762781
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AccessLogFilter.java:
##########
@@ -114,6 +115,9 @@ public void filter(ContainerRequestContext requestContext,
executeTime, null, method, path, uri.getQuery());
}
}
+
+ // Release the context
Review Comment:
```suggestion
// Unset the context in "HugeAuthenticator"
```
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeAuthenticator.java:
##########
@@ -105,7 +105,6 @@ 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
*/
Review Comment:
change multi to one line comment -like-> `Set authentication context & unset in AccessLogFilter`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore(server): clear context after req done [incubator-hugegraph]
Posted by "imbajin (via GitHub)" <gi...@apache.org>.
imbajin commented on code in PR #2470:
URL: https://github.com/apache/incubator-hugegraph/pull/2470#discussion_r1529876770
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java:
##########
@@ -71,15 +72,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();
+ // Remove auth/login API from white list
Review Comment:
> 1. the CI will **fail** in `login` when(login) whiteList exist (refer [CI link](https://github.com/apache/incubator-hugegraph/actions/runs/8173840336/job/22347189327?pr=2470))
@javeme Yes, but after we fix the TLS leak, the `login/tests` **can't run well** if we only check auth once in LoginAPI, and we'll dig it out the reason, cc @SunnyBoy-WYH mark it as a TODO issue
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore(server): clear context after req done [incubator-hugegraph]
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on code in PR #2470:
URL: https://github.com/apache/incubator-hugegraph/pull/2470#discussion_r1527457151
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/api/ArthasApiTest.java:
##########
@@ -38,21 +38,36 @@ public void testArthasStart() {
@Test
public void testArthasApi() {
- String body = "{\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);
+ // 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");
+
+
Review Comment:
expect just one blank line
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/ConfigAuthenticator.java:
##########
@@ -80,6 +81,11 @@ public UserWithRole authenticate(final String username,
return new UserWithRole(IdGenerator.of(username), username, role);
}
+ @Override
+ public void unAuthenticate(AuthenticationFilter.Authorizer authorizer) {
+ throw new NotImplementedException("unAuthenticate is unsupported by ConfigAuthenticator");
Review Comment:
I think it's ok to just keep blank logic:"// pass"
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java:
##########
@@ -121,11 +121,11 @@ protected User authenticate(ContainerRequestContext context) {
}
// Check whiteIp
- if (whiteIpStatus == null) {
- whiteIpStatus = this.configProvider.get().get(WHITE_IP_STATUS);
+ if (enabledWhiteIpCheck == null) {
+ enabledWhiteIpCheck = Objects.equals(this.configProvider.get().get(WHITE_IP_STATUS), STRING_ENABLE);
Review Comment:
it's too long, prefer split into 2 lines:
```
xx = this.configProvider.get().get(WHITE_IP_STATUS);
enabledWhiteIpCheck = Objects.equals(xx, STRING_ENABLE);
```
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeAuthenticator.java:
##########
@@ -64,6 +65,8 @@ public interface HugeAuthenticator extends Authenticator {
UserWithRole authenticate(String username, String password, String token);
+ void unAuthenticate(AuthenticationFilter.Authorizer authorizer);
Review Comment:
sorry, can we change to this signature: `void unauthorize(SecurityContext context)`
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java:
##########
@@ -71,15 +72,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();
+ // Remove auth/login API from white list
Review Comment:
Because the login method itself will authenticate the user, there is no need to authenticate twice. Of course, it seems there is no problem when authenticating twice.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/api/ArthasApiTest.java:
##########
@@ -38,21 +38,36 @@ public void testArthasStart() {
@Test
public void testArthasApi() {
- String body = "{\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);
+ // 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");
+
+
+ // 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");
Review Comment:
expect a space after ","
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AccessLogFilter.java:
##########
@@ -114,6 +119,13 @@ public void filter(ContainerRequestContext requestContext,
executeTime, null, method, path, uri.getQuery());
}
}
+
+ // Unset the context in "HugeAuthenticator", need distinguish Graph/Auth server lifecycle
+ GraphManager manager = managerProvider.get();
+ // TODO transfer Authorizer if we need after.
Review Comment:
can we just pass the SecurityContext into it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore(server): clear context after req done [incubator-hugegraph]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #2470:
URL: https://github.com/apache/incubator-hugegraph/pull/2470#issuecomment-1985572971
## [Codecov](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2470?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: Patch coverage is `55.55556%` with `4 lines` in your changes are missing coverage. Please review.
> Project coverage is 52.73%. Comparing base [(`eb6570c`)](https://app.codecov.io/gh/apache/incubator-hugegraph/commit/eb6570c99234f22950c035d137bffccab3dcb1a6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`41459f5`)](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2470?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
| [Files](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2470?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...che/hugegraph/api/filter/AuthenticationFilter.java](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2470?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVnZWdyYXBoLXNlcnZlci9odWdlZ3JhcGgtYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWdlZ3JhcGgvYXBpL2ZpbHRlci9BdXRoZW50aWNhdGlvbkZpbHRlci5qYXZh) | 33.33% | [1 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2470?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #2470 +/- ##
=============================================
- Coverage 66.26% 52.73% -13.54%
+ Complexity 827 545 -282
=============================================
Files 511 511
Lines 42623 42625 +2
Branches 5947 5948 +1
=============================================
- Hits 28245 22479 -5766
- Misses 11563 17708 +6145
+ Partials 2815 2438 -377
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-hugegraph/pull/2470?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore(server): clear context after req done [incubator-hugegraph]
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on code in PR #2470:
URL: https://github.com/apache/incubator-hugegraph/pull/2470#discussion_r1518714096
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java:
##########
@@ -71,15 +72,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();
+ // Remove auth/login API from white list
+ private static final Set<String> FLEXIBLE_WHITE_API_LIST = ImmutableSet.of();
Review Comment:
can also keep XX_SET: FLEXIBLE_WHITE_API_SET
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AccessLogFilter.java:
##########
@@ -114,6 +115,9 @@ public void filter(ContainerRequestContext requestContext,
executeTime, null, method, path, uri.getQuery());
}
}
+
+ // Unset the context in "HugeAuthenticator", need distinguish Graph/Auth server lifecycle
+ HugeGraphAuthProxy.resetContext();
Review Comment:
to make the code look more modular, can we add a method `HugeAuthenticator(Authorizer authorizer)`? and here we can call `manager.unauthenticate(Authorizer authorizer)`, note authorizer=requestContext.getSecurityContext().
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/api/ArthasApiTest.java:
##########
@@ -43,16 +43,9 @@ public void testArthasApi() {
" \"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);
assertJsonContains(result, "state");
assertJsonContains(result, "body");
-
- RestClient arthasApiClientWithAuth = new RestClient(ARTHAS_API_BASE_URL);
- r = arthasApiClientWithAuth.post(ARTHAS_API_PATH, body);
- assertResponseStatus(401, r);
Review Comment:
prefer to add some negative test cases
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java:
##########
@@ -71,15 +72,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();
+ // Remove auth/login API from white list
Review Comment:
we need to make sure the login api is accessible, it's used to get a user token.
and also update the comment after checked.
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java:
##########
@@ -71,15 +72,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();
+ // Remove auth/login API from white list
+ private static final Set<String> FLEXIBLE_WHITE_API_LIST = ImmutableSet.of();
private static String whiteIpStatus;
Review Comment:
can we rename `String whiteIpStatus` to `Boolean enabledWhiteIpCheck`
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java:
##########
@@ -1818,7 +1818,7 @@ protected static Context setContext(Context context) {
return old;
}
- protected static void resetContext() {
+ public static void resetContext() {
Review Comment:
prefer to keep protected
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore(server): clear context after req done [incubator-hugegraph]
Posted by "SunnyBoy-WYH (via GitHub)" <gi...@apache.org>.
SunnyBoy-WYH commented on code in PR #2470:
URL: https://github.com/apache/incubator-hugegraph/pull/2470#discussion_r1522830136
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AccessLogFilter.java:
##########
@@ -114,6 +115,9 @@ public void filter(ContainerRequestContext requestContext,
executeTime, null, method, path, uri.getQuery());
}
}
+
+ // Unset the context in "HugeAuthenticator", need distinguish Graph/Auth server lifecycle
+ HugeGraphAuthProxy.resetContext();
Review Comment:
> to make the code look more modular, can we add a method `HugeAuthenticator(Authorizer authorizer)`? and here we can call `manager.unauthenticate(Authorizer authorizer)`, note authorizer=requestContext.getSecurityContext().
thanks, but why we need transport the arg name authorizer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org
Re: [PR] chore(server): clear context after req done [incubator-hugegraph]
Posted by "javeme (via GitHub)" <gi...@apache.org>.
javeme commented on code in PR #2470:
URL: https://github.com/apache/incubator-hugegraph/pull/2470#discussion_r1536628021
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeAuthenticator.java:
##########
@@ -107,7 +107,7 @@ default User authenticate(final Map<String, String> credentials)
}
HugeGraphAuthProxy.logUser(user, credentials.get(KEY_PATH));
- // Set authentication context & unset in AccessLogFilter
+ // TODO: Ensure context lifecycle in GraphServer & AuthServer(#AccessLogFilter)
Review Comment:
don't need TODO mark anymore since it's done
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AccessLogFilter.java:
##########
@@ -122,7 +122,7 @@ public void filter(ContainerRequestContext requestContext,
// Unset the context in "HugeAuthenticator", need distinguish Graph/Auth server lifecycle
GraphManager manager = managerProvider.get();
- // TODO transfer Authorizer if we need after.
+ // TODO: transfer Authorizer if we need after.
Review Comment:
don't need TODO mark anymore since it's done
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java:
##########
@@ -76,7 +76,7 @@ public class AuthenticationFilter implements ContainerRequestFilter {
"versions",
"openapi.json"
);
- // Remove auth/login API from white list
+ /** Remove auth/login API from whitelist */
Review Comment:
one * is ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@hugegraph.apache.org
For additional commands, e-mail: issues-help@hugegraph.apache.org