[ISSUE-1206] improvement: Use the real user for audit information #1258

qqqttt123 · 2023-12-26T08:40:13Z

What changes were proposed in this pull request?

Use the real user for audit information.
Use a PrincipalContext to pass the value of user without interface modification referring to the design of Spark's TaskContext

Why are the changes needed?

Fix: #1206

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add UT.

qqqttt123 · 2023-12-26T08:40:47Z

Blocked by #1257

qqqttt123 · 2023-12-26T12:32:59Z

Blocked by #1257

#1257 merged.

jerryshao · 2023-12-27T11:42:35Z

common/src/main/java/com/datastrato/gravitino/auth/AuthConstants.java

@@ -13,4 +13,6 @@ public interface AuthConstants {
  String AUTHORIZATION_BASIC_HEADER = "Basic ";

  String ANONYMOUS_USER = "anonymous";
+
+  String AuthenticatedPrincipalAttributeName = AuthConstants.class.getName() + "-principal";


Why do you capitalize the first letter, also why don't you use the upper-case for this static variable?

jerryshao · 2023-12-27T11:52:03Z

core/src/main/java/com/datastrato/gravitino/PrincipalContext.java

+
+  private String currentUser;
+
+  private static final ThreadLocal<PrincipalContext> context = new ThreadLocal<>();


Is it safe to use thread local variable? I'm afraid if we change some code, we will not get the correct variable?

Also if we don't remove the thread local variable in time, we will get the wrong user if this thread is reused.

In the meantime, is it better to use "InheritableThreadLocal"?

If we use a thread pool, InheritableThreadLocal can't also guarantee that we get the correct user.

Also if we don't remove the thread local variable in time, we will get the wrong user if this thread is reused.

Now, we use try finally to guarantee the correctness. It's a regular way to solve the similar case.

jerryshao · 2023-12-27T12:02:35Z

server-common/src/main/java/com/datastrato/gravitino/server/auth/AuthenticationFilter.java

@@ -52,7 +53,8 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
        authData = headerData.nextElement().getBytes(StandardCharsets.UTF_8);
      }
      if (authenticator.isDataFromToken()) {
-        authenticator.authenticateToken(authData);
+        Principal principal = authenticator.authenticateToken(authData);
+        request.setAttribute(AuthConstants.AuthenticatedPrincipalAttributeName, principal);


Is it a common way to get user name from attribute?

Pulsar uses this way. I don't find other implements. Some systems will put the user formation into the session. But we don't have session manager in our Jetty server. It may bring some cost to add a session manager in our Jetty server. So I choose to use to put the user to the attributes.

My original purpose is to use this:

If you use another way, maybe you can remove that variable.

My original purpose is to use this:
If you use another way, maybe you can remove that variable.

I have used this variable. I need to set the attribute and get the attribute with this variable.

jerryshao · 2023-12-27T12:04:48Z

server/src/main/java/com/datastrato/gravitino/server/web/rest/TableOperations.java

@@ -80,7 +81,7 @@ public Response createTable(
      @PathParam("catalog") String catalog,
      @PathParam("schema") String schema,
      TableCreateRequest request) {
-    try {
+    try (PrincipalContext context = Utils.createUserPrincipalContext(httpRequest)) {


I think you would wrap this for all the operations later on, not just create and alter operations. For example, if you want to support proxy user for hive catalog operations.

OK, I will do.

qqqttt123 · 2023-12-28T04:01:41Z

core/src/main/java/com/datastrato/gravitino/utils/PrincipalUtils.java

+import javax.security.auth.Subject;
+
+@SuppressWarnings("removal")
+public class PrincipalUtils {


I refer to the doAs and getCurrentUser from Hadoop UserInformation.

This api will be removed in the future. But JDK doesn't provide alternative solution. Hadoop will encounter the similar situation. I add the anotation to avoid the failure of compile.

jerryshao · 2023-12-28T07:20:11Z

Please let me know when it is ready to review again.

qqqttt123 · 2023-12-28T08:18:47Z

Please let me know when it is ready to review again.

I have addressed all the comments. It's ready to review again.

jerryshao · 2023-12-28T10:06:34Z

common/src/main/java/com/datastrato/gravitino/auth/AuthConstants.java

@@ -13,4 +13,6 @@ public interface AuthConstants {
  String AUTHORIZATION_BASIC_HEADER = "Basic ";

  String ANONYMOUS_USER = "anonymous";
+
+  String AUTHENTICATED_PRINCIPAL_ATTRIBUTE_NAME = AuthConstants.class.getName() + "-principal";


What is the purpose of using AuthConstants.class.getName() as an attribute name?

Refer to the style of Pulsar. I have added the comment.

jerryshao · 2023-12-28T10:10:49Z

core/src/main/java/com/datastrato/gravitino/utils/PrincipalUtils.java

+      Throwable cause = pae.getCause();
+      if (!(cause instanceof Exception)) {
+        throw new RuntimeException(
+            "PrivilegedActionException with no "


Which exception will be thrown here, the exception message is not easy to understand.

I think you can use Guava's Throwables to simplify the code here.

jerryshao · 2023-12-28T11:02:28Z

core/src/test/java/com/datastrato/gravitino/utils/TestPrincipalUtils.java

+              executorService.submit(
+                  () ->
+                      Assertions.assertEquals(
+                          "testThreadPool", PrincipalUtils.getCurrentPrincipal().getName()));


I guess this may not be correct if the thread is cached and reused? You can only make sure to get correct principal when you doAs in the lambda function.

doAs doesn't use InherittedThreadLocal. The implement is referring to the UserGroupInformation. It get the principal from stack.

My fault. Removed.

qqqttt123 · 2023-12-28T11:47:44Z

All comments are addressed.

Removed the bad case
Use Throwables to simplify the code
Add the comment.

[ISSUE-1206] improvement: Use the real user for audit information

89dd66a

qqqttt123 marked this pull request as draft December 26, 2023 08:40

Heng Qin added 6 commits December 26, 2023 19:23

Merge remote-tracking branch 'upstream/main' into ISSUE-1206

0969cb4

Merge remote-tracking branch 'upstream/main' into ISSUE-1206

f9d9134

Merge remote-tracking branch 'upstream/main' into ISSUE-1206

5a19916

fix

b20c7fd

fix

391042a

fix

ab524ab

qqqttt123 marked this pull request as ready for review December 26, 2023 12:30

qqqttt123 requested review from yuqi1129, jerryshao, xunliu, FANNG1, diqiu50, mchades and Clearvive and removed request for yuqi1129 December 26, 2023 12:30

qqqttt123 self-assigned this Dec 26, 2023

jerryshao reviewed Dec 27, 2023

View reviewed changes

Heng Qin added 7 commits December 28, 2023 10:35

Use another method

aca126b

fix

854669b

fix

2167435

fix

320ca46

fix

12f5c93

fix

989d850

fix

50127a1

qqqttt123 commented Dec 28, 2023

View reviewed changes

Heng Qin added 2 commits December 28, 2023 14:49

fix

fc8d3fd

fix

0b1b3a9

jerryshao reviewed Dec 28, 2023

View reviewed changes

Heng Qin added 2 commits December 28, 2023 19:21

fix

9896175

fix

ba2384c

Heng Qin added 2 commits December 28, 2023 20:03

remove bad case

9911e19

fix

4a4bafd

jerryshao approved these changes Dec 28, 2023

View reviewed changes

jerryshao merged commit 1315755 into apache:main Dec 28, 2023
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ISSUE-1206] improvement: Use the real user for audit information #1258

[ISSUE-1206] improvement: Use the real user for audit information #1258

qqqttt123 commented Dec 26, 2023 •

edited

Loading

qqqttt123 commented Dec 26, 2023

qqqttt123 commented Dec 26, 2023

jerryshao Dec 27, 2023

qqqttt123 Dec 28, 2023

jerryshao Dec 27, 2023

jerryshao Dec 27, 2023

jerryshao Dec 27, 2023

qqqttt123 Dec 27, 2023

qqqttt123 Dec 27, 2023

jerryshao Dec 27, 2023

qqqttt123 Dec 27, 2023

jerryshao Dec 27, 2023

qqqttt123 Dec 27, 2023

jerryshao Dec 27, 2023

qqqttt123 Dec 27, 2023

qqqttt123 Dec 28, 2023

qqqttt123 Dec 28, 2023 •

edited

Loading

qqqttt123 Dec 28, 2023 •

edited

Loading

jerryshao commented Dec 28, 2023

qqqttt123 commented Dec 28, 2023 •

edited

Loading

jerryshao Dec 28, 2023

qqqttt123 Dec 28, 2023

jerryshao Dec 28, 2023

jerryshao Dec 28, 2023

qqqttt123 Dec 28, 2023

jerryshao Dec 28, 2023

qqqttt123 Dec 28, 2023

qqqttt123 Dec 28, 2023

qqqttt123 commented Dec 28, 2023 •

edited

Loading


		private String currentUser;

		private static final ThreadLocal<PrincipalContext> context = new ThreadLocal<>();

[ISSUE-1206] improvement: Use the real user for audit information #1258

[ISSUE-1206] improvement: Use the real user for audit information #1258

Conversation

qqqttt123 commented Dec 26, 2023 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

qqqttt123 commented Dec 26, 2023

qqqttt123 commented Dec 26, 2023

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qqqttt123 Dec 28, 2023 • edited Loading

Choose a reason for hiding this comment

qqqttt123 Dec 28, 2023 • edited Loading

Choose a reason for hiding this comment

jerryshao commented Dec 28, 2023

qqqttt123 commented Dec 28, 2023 • 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

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

qqqttt123 commented Dec 28, 2023 • edited Loading

qqqttt123 commented Dec 26, 2023 •

edited

Loading

qqqttt123 Dec 28, 2023 •

edited

Loading

qqqttt123 Dec 28, 2023 •

edited

Loading

qqqttt123 commented Dec 28, 2023 •

edited

Loading

qqqttt123 commented Dec 28, 2023 •

edited

Loading