chore(server): clear context after req done #2470

SunnyBoy-WYH · 2024-03-03T13:52:58Z

Note

complete the history todo

# Conflicts: # hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/config/ServerOptions.java

...raph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AccessLogFilter.java

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeAuthenticator.java

codecov · 2024-03-08T12:01:13Z

Codecov Report

Attention: Patch coverage is 58.62069% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 66.25%. Comparing base (c09982b) to head (5817ccd).
Report is 1 commits behind head on master.

Files	Patch %	Lines
...che/hugegraph/api/filter/AuthenticationFilter.java	47.36%	5 Missing and 5 partials ⚠️
...g/apache/hugegraph/api/filter/AccessLogFilter.java	66.66%	0 Missing and 1 partial ⚠️
...org/apache/hugegraph/auth/ConfigAuthenticator.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2470      +/-   ##
============================================
- Coverage     66.34%   66.25%   -0.09%     
+ Complexity      829      827       -2     
============================================
  Files           511      511              
  Lines         42624    42631       +7     
  Branches       5947     5948       +1     
============================================
- Hits          28278    28246      -32     
- Misses        11541    11563      +22     
- Partials       2805     2822      +17

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

...server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java

javeme · 2024-03-10T03:30:50Z

hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/api/ArthasApiTest.java

-
-        RestClient arthasApiClientWithAuth = new RestClient(ARTHAS_API_BASE_URL);
-        r = arthasApiClientWithAuth.post(ARTHAS_API_PATH, body);
-        assertResponseStatus(401, r);


prefer to add some negative test cases

...server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java

javeme · 2024-03-10T03:48:58Z

...server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java

            "versions",
            "openapi.json"
    );
-    private static final AntPathMatcher MATCHER = new AntPathMatcher();
+    // Remove auth/login API from white list


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.

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:

When we unset-context in AccessLogFilter, the CI will fail in login when(login) whiteList exist (refer CI link)

follow the login API input param & the validate logic

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

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.

the CI will fail in login when(login) whiteList exist (refer CI link)

@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

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeGraphAuthProxy.java

...raph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AccessLogFilter.java

hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/api/ArthasApiTest.java

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/ConfigAuthenticator.java

...server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeAuthenticator.java

...raph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AccessLogFilter.java

javeme · 2024-03-17T10:06:59Z

...server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java

            "versions",
            "openapi.json"
    );
-    private static final AntPathMatcher MATCHER = new AntPathMatcher();
+    // Remove auth/login API from white list


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.

imbajin

Mark some todo & merge this PR first (arthas test will enhance in #2429

Co-authored-by: vaughn.zhang <vaughn.zhang@zoom.us> Co-authored-by: imbajin <jin@apache.org>

javeme · 2024-03-23T12:47:02Z

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)


don't need TODO mark anymore since it's done

javeme · 2024-03-23T12:47:23Z

...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 */


one * is ok

javeme · 2024-03-23T12:47:38Z

...raph-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.


don't need TODO mark anymore since it's done

SunnyBoy-WYH added 10 commits January 19, 2024 19:52

chore(server): update swagger info for default server profile

ae90955

chore(server): update swagger info for default server profile

bb67693

fix(server): arthas default bind ip should not be 0.0.0.0

13f916f

Merge branch 'master' into arthas-bind-ip

30f1821

fix(server): arthas default bind ip should not be 0.0.0.0

2519b39

Merge branch 'master' into arthas-bind-ip

c787baf

# Conflicts: # hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/config/ServerOptions.java

fix(server): fix the bug which promtheus cant collect hg metric

7d75e0d

fix(server): fix the arthas default bind ip to 127.0.0.1

552dcb8

fix(server): clear auth context (TLS) after req done

ba74aaa

fix(server): clear auth context (TLS) after req done

4f7fc0f

dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. api Changes of API labels Mar 3, 2024

imbajin reviewed Mar 4, 2024

View reviewed changes

...raph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AccessLogFilter.java Outdated Show resolved Hide resolved

hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/auth/HugeAuthenticator.java Outdated Show resolved Hide resolved

zyxxoo and others added 5 commits March 6, 2024 21:41

fix: security bug

43288d9

improve

6104ed6

fix(server): white list change to fixed and flexable

906c0de

Merge branch 'arthas-bind-ip' into fix-context

177e513

fix(server): white list change to fixed and flexable

165b5c8

dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Mar 6, 2024

SunnyBoy-WYH and others added 4 commits March 6, 2024 22:33

fix(server): white list change to fixed and flexable

7eeda25

fix(server): white list change to fixed and flexable

5aa1a40

Merge branch 'master' into pr/2470

6030882

remove login from whiteList

41459f5

imbajin added 2 commits March 9, 2024 21:05

Merge branch 'master' into fix-context

1435e44

Update AccessLogFilter.java

3643116

imbajin previously approved these changes Mar 9, 2024

View reviewed changes

dosubot bot added the lgtm This PR has been approved by a maintainer label Mar 9, 2024

Update AuthenticationFilter.java

7484ab3

imbajin dismissed their stale review via 7484ab3 March 9, 2024 13:10

javeme reviewed Mar 10, 2024

View reviewed changes

SunnyBoy-WYH added 2 commits March 10, 2024 23:18

fix(server): white list change to fixed and flexable

6a50b99

fix(server): better code

ed2b24a

dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Mar 13, 2024

SunnyBoy-WYH added 2 commits March 13, 2024 21:55

fix(server): better code

80c1634

fix(server): better code

ed250f2

javeme reviewed Mar 17, 2024

View reviewed changes

SunnyBoy-WYH and others added 2 commits March 18, 2024 21:58

fix(server): better code

2e3325c

tiny improve

5817ccd

imbajin approved these changes Mar 19, 2024

View reviewed changes

imbajin requested a review from VGalaxies March 19, 2024 07:56

VGalaxies approved these changes Mar 19, 2024

View reviewed changes

VGalaxies merged commit 277f76e into apache:master Mar 19, 2024
19 of 21 checks passed

VGalaxies changed the title ~~chore(server): clear context after req done~~ chore(server): clear context after req done Mar 19, 2024

VGalaxies pushed a commit that referenced this pull request Mar 19, 2024

chore(server): clear context after req done (#2470)

6d04e9b

Co-authored-by: vaughn.zhang <vaughn.zhang@zoom.us> Co-authored-by: imbajin <jin@apache.org>

javeme reviewed Mar 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(server): clear context after req done #2470

chore(server): clear context after req done #2470

SunnyBoy-WYH commented Mar 3, 2024 •

edited by imbajin

Loading

codecov bot commented Mar 8, 2024 •

edited

Loading

javeme Mar 10, 2024

javeme Mar 10, 2024

imbajin Mar 10, 2024 •

edited

Loading

javeme Mar 17, 2024

imbajin Mar 19, 2024

javeme Mar 17, 2024

imbajin left a comment •

edited

Loading

javeme Mar 23, 2024

javeme Mar 23, 2024

javeme Mar 23, 2024

chore(server): clear context after req done #2470

chore(server): clear context after req done #2470

Conversation

SunnyBoy-WYH commented Mar 3, 2024 • edited by imbajin Loading

codecov bot commented Mar 8, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imbajin Mar 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imbajin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SunnyBoy-WYH commented Mar 3, 2024 •

edited by imbajin

Loading

codecov bot commented Mar 8, 2024 •

edited

Loading

imbajin Mar 10, 2024 •

edited

Loading

imbajin left a comment •

edited

Loading