Skip to content
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

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

Merged
merged 20 commits into from
Dec 28, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,7 @@ public interface AuthConstants {
String AUTHORIZATION_BASIC_HEADER = "Basic ";

String ANONYMOUS_USER = "anonymous";

// Refer to the style of `AuthenticationFilter#AuthenticatedRoleAttributeName` of Apache Pulsar
String AUTHENTICATED_PRINCIPAL_ATTRIBUTE_NAME = AuthConstants.class.getName() + "-principal";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* This software is licensed under the Apache License version 2.
*/

package com.datastrato.gravitino.server.auth;
package com.datastrato.gravitino;

import com.google.common.base.Preconditions;
import java.security.Principal;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import com.datastrato.gravitino.rel.TableCatalog;
import com.datastrato.gravitino.storage.IdGenerator;
import com.datastrato.gravitino.utils.IsolatedClassLoader;
import com.datastrato.gravitino.utils.PrincipalUtils;
import com.datastrato.gravitino.utils.ThrowableFunction;
import com.github.benmanes.caffeine.cache.Cache;
import com.github.benmanes.caffeine.cache.Caffeine;
Expand Down Expand Up @@ -261,7 +262,7 @@ public Catalog createCatalog(
.withProperties(StringIdentifier.newPropertiesWithId(stringId, mergedConfig))
.withAuditInfo(
new AuditInfo.Builder()
.withCreator("gravitino") /* TODO. Should change to real user */
.withCreator(PrincipalUtils.getCurrentPrincipal().getName())
.withCreateTime(Instant.now())
.build())
.build();
Expand Down Expand Up @@ -366,8 +367,7 @@ public Catalog alterCatalog(NameIdentifier ident, CatalogChange... changes)
new AuditInfo.Builder()
.withCreator(catalog.auditInfo().creator())
.withCreateTime(catalog.auditInfo().createTime())
.withLastModifier(
catalog.auditInfo().creator()) /* TODO. We should use real user */
.withLastModifier(PrincipalUtils.getCurrentPrincipal().getName())
.withLastModifiedTime(Instant.now())
.build();
newCatalogBuilder.withAuditInfo(newInfo);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import com.datastrato.gravitino.rel.expressions.sorts.SortOrder;
import com.datastrato.gravitino.rel.expressions.transforms.Transform;
import com.datastrato.gravitino.storage.IdGenerator;
import com.datastrato.gravitino.utils.PrincipalUtils;
import com.datastrato.gravitino.utils.ThrowableFunction;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.Maps;
Expand Down Expand Up @@ -150,8 +151,7 @@ public Schema createSchema(NameIdentifier ident, String comment, Map<String, Str
.withNamespace(ident.namespace())
.withAuditInfo(
new AuditInfo.Builder()
.withCreator("gravitino") // TODO. hardcoded as user "gravitino" for now, will
// change to real user once user system is ready.
.withCreator(PrincipalUtils.getCurrentPrincipal().getName())
.withCreateTime(Instant.now())
.build())
.build();
Expand Down Expand Up @@ -262,8 +262,7 @@ public Schema alterSchema(NameIdentifier ident, SchemaChange... changes)
.withCreator(schemaEntity.auditInfo().creator())
.withCreateTime(schemaEntity.auditInfo().createTime())
.withLastModifier(
"gravitino") // TODO. hardcoded as user "gravitino"
// for now, will change to real user once user system is ready.
PrincipalUtils.getCurrentPrincipal().getName())
.withLastModifiedTime(Instant.now())
.build())
.build()),
Expand Down Expand Up @@ -455,7 +454,7 @@ public Table createTable(
.withNamespace(ident.namespace())
.withAuditInfo(
new AuditInfo.Builder()
.withCreator("gravitino")
.withCreator(PrincipalUtils.getCurrentPrincipal().getName())
.withCreateTime(Instant.now())
.build())
.build();
Expand Down Expand Up @@ -537,10 +536,7 @@ public Table alterTable(NameIdentifier ident, TableChange... changes)
new AuditInfo.Builder()
.withCreator(tableEntity.auditInfo().creator())
.withCreateTime(tableEntity.auditInfo().createTime())
.withLastModifier(
"gravitino") // hardcoded as user "gravitino" for now, will
// change
// to real user once user system is ready.
.withLastModifier(PrincipalUtils.getCurrentPrincipal().getName())
.withLastModifiedTime(Instant.now())
.build())
.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import com.datastrato.gravitino.exceptions.NoSuchEntityException;
import com.datastrato.gravitino.exceptions.NoSuchMetalakeException;
import com.datastrato.gravitino.storage.IdGenerator;
import com.datastrato.gravitino.utils.PrincipalUtils;
import com.google.common.collect.Maps;
import java.io.IOException;
import java.time.Instant;
Expand Down Expand Up @@ -107,7 +108,7 @@ public BaseMetalake createMetalake(
.withVersion(SchemaVersion.V_0_1)
.withAuditInfo(
new AuditInfo.Builder()
.withCreator("gravitino") /*TODO: Use real user later on. */
.withCreator(PrincipalUtils.getCurrentPrincipal().getName())
.withCreateTime(Instant.now())
.build())
.build();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* Copyright 2023 Datastrato Pvt Ltd.
* This software is licensed under the Apache License version 2.
*/

package com.datastrato.gravitino.utils;

import com.datastrato.gravitino.UserPrincipal;
import com.datastrato.gravitino.auth.AuthConstants;
import com.google.common.base.Throwables;
import java.security.Principal;
import java.security.PrivilegedActionException;
import java.security.PrivilegedExceptionAction;
import javax.security.auth.Subject;

@SuppressWarnings("removal")
public class PrincipalUtils {
Copy link
Contributor Author

@qqqttt123 qqqttt123 Dec 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refer to the doAs and getCurrentUser from Hadoop UserInformation.

Copy link
Contributor Author

@qqqttt123 qqqttt123 Dec 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

private PrincipalUtils() {}

public static <T> T doAs(Principal principal, PrivilegedExceptionAction<T> action)
throws Exception {
try {
Subject subject = new Subject();
subject.getPrincipals().add(principal);
return Subject.doAs(subject, action);
} catch (PrivilegedActionException pae) {
Throwable cause = pae.getCause();
Throwables.propagateIfPossible(cause, Exception.class);
throw new RuntimeException("doAs method occurs an unexpected exception", pae);
}
}

public static Principal getCurrentPrincipal() {
java.security.AccessControlContext context = java.security.AccessController.getContext();
Subject subject = Subject.getSubject(context);
if (subject == null || subject.getPrincipals(UserPrincipal.class).isEmpty()) {
return new UserPrincipal(AuthConstants.ANONYMOUS_USER);
}
return subject.getPrincipals(UserPrincipal.class).iterator().next();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.datastrato.gravitino.StringIdentifier;
import com.datastrato.gravitino.TestColumn;
import com.datastrato.gravitino.TestEntityStore;
import com.datastrato.gravitino.auth.AuthConstants;
import com.datastrato.gravitino.exceptions.IllegalNamespaceException;
import com.datastrato.gravitino.exceptions.NoSuchEntityException;
import com.datastrato.gravitino.meta.AuditInfo;
Expand Down Expand Up @@ -194,7 +195,7 @@ public void testCreateAndLoadSchema() throws IOException {
Assertions.assertEquals(schema.comment(), loadedSchema.comment());
testProperties(schema.properties(), loadedSchema.properties());
// Audit info is gotten from entity store
Assertions.assertEquals("gravitino", loadedSchema.auditInfo().creator());
Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, loadedSchema.auditInfo().creator());

// Case 2: Test if the schema is not found in entity store
doThrow(new NoSuchEntityException("mock error")).when(entityStore).get(any(), any(), any());
Expand All @@ -218,7 +219,7 @@ public void testCreateAndLoadSchema() throws IOException {
.withNamespace(Namespace.of(metalake, catalog))
.withAuditInfo(
new AuditInfo.Builder()
.withCreator("gravitino")
.withCreator(AuthConstants.ANONYMOUS_USER)
.withCreateTime(Instant.now())
.build())
.build();
Expand Down Expand Up @@ -252,8 +253,8 @@ public void testCreateAndAlterSchema() throws IOException {
Map<String, String> expectedProps = ImmutableMap.of("k2", "v2", "k3", "v3");
testProperties(expectedProps, alteredSchema.properties());
// Audit info is gotten from gravitino entity store.
Assertions.assertEquals("gravitino", alteredSchema.auditInfo().creator());
Assertions.assertEquals("gravitino", alteredSchema.auditInfo().lastModifier());
Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, alteredSchema.auditInfo().creator());
Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, alteredSchema.auditInfo().lastModifier());

// Case 2: Test if the schema is not found in entity store
doThrow(new NoSuchEntityException("mock error"))
Expand Down Expand Up @@ -287,7 +288,7 @@ public void testCreateAndAlterSchema() throws IOException {
.withNamespace(Namespace.of(metalake, catalog))
.withAuditInfo(
new AuditInfo.Builder()
.withCreator("gravitino")
.withCreator(AuthConstants.ANONYMOUS_USER)
.withCreateTime(Instant.now())
.build())
.build();
Expand Down Expand Up @@ -414,7 +415,7 @@ public void testCreateAndLoadTable() throws IOException {
Assertions.assertEquals(0, loadedTable1.partitioning().length);
Assertions.assertArrayEquals(table1.columns(), loadedTable1.columns());
// Audit info is gotten from the entity store
Assertions.assertEquals("gravitino", loadedTable1.auditInfo().creator());
Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, loadedTable1.auditInfo().creator());

// Case 2: Test if the table entity is not found in the entity store
reset(entityStore);
Expand Down Expand Up @@ -480,8 +481,8 @@ public void testCreateAndAlterTable() throws IOException {
Map<String, String> expectedProps = ImmutableMap.of("k2", "v2", "k3", "v3");
testProperties(expectedProps, alteredTable.properties());
// Audit info is gotten from gravitino entity store
Assertions.assertEquals("gravitino", alteredTable.auditInfo().creator());
Assertions.assertEquals("gravitino", alteredTable.auditInfo().lastModifier());
Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, alteredTable.auditInfo().creator());
Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, alteredTable.auditInfo().lastModifier());

// Case 2: Test if the table entity is not found in the entity store
reset(entityStore);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* Copyright 2023 Datastrato Pvt Ltd.
* This software is licensed under the Apache License version 2.
*/

package com.datastrato.gravitino.utils;

import com.datastrato.gravitino.UserPrincipal;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

public class TestPrincipalUtils {

@Test
public void testNormal() throws Exception {
UserPrincipal principal = new UserPrincipal("testNormal");
PrincipalUtils.doAs(
principal,
() -> {
Assertions.assertEquals("testNormal", PrincipalUtils.getCurrentPrincipal().getName());
return null;
});
}

@Test
public void testThread() throws Exception {
UserPrincipal principal = new UserPrincipal("testThread");
PrincipalUtils.doAs(
principal,
() -> {
Thread thread =
new Thread(
() ->
Assertions.assertEquals(
"testThread", PrincipalUtils.getCurrentPrincipal().getName()));
thread.start();
thread.join();
return null;
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import com.datastrato.gravitino.CatalogChange;
import com.datastrato.gravitino.MetalakeChange;
import com.datastrato.gravitino.NameIdentifier;
import com.datastrato.gravitino.auth.AuthConstants;
import com.datastrato.gravitino.catalog.hive.HiveClientPool;
import com.datastrato.gravitino.catalog.hive.HiveSchemaPropertiesMetadata;
import com.datastrato.gravitino.catalog.hive.HiveTablePropertiesMetadata;
Expand Down Expand Up @@ -728,14 +729,17 @@ void testAlterUnknownTable() {
@Test
public void testAlterHiveTable() throws TException, InterruptedException {
ColumnDTO[] columns = createColumns();
catalog
.asTableCatalog()
.createTable(
NameIdentifier.of(metalakeName, catalogName, schemaName, tableName),
columns,
TABLE_COMMENT,
createProperties(),
new Partitioning[] {IdentityPartitioningDTO.of(columns[2].name())});
Table createdTable =
catalog
.asTableCatalog()
.createTable(
NameIdentifier.of(metalakeName, catalogName, schemaName, tableName),
columns,
TABLE_COMMENT,
createProperties(),
new Partitioning[] {IdentityPartitioningDTO.of(columns[2].name())});
Assertions.assertNull(createdTable.auditInfo().lastModifier());
Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, createdTable.auditInfo().creator());
Table alteredTable =
catalog
.asTableCatalog()
Expand All @@ -754,7 +758,8 @@ public void testAlterHiveTable() throws TException, InterruptedException {
TableChange.updateColumnComment(new String[] {HIVE_COL_NAME1}, "comment_new"),
TableChange.updateColumnType(
new String[] {HIVE_COL_NAME1}, Types.IntegerType.get()));

Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, alteredTable.auditInfo().creator());
Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, alteredTable.auditInfo().lastModifier());
// Direct get table from hive metastore to check if the table is altered successfully.
org.apache.hadoop.hive.metastore.api.Table hiveTab =
hiveClientPool.run(client -> client.getTable(schemaName, ALTER_TABLE_NAME));
Expand Down Expand Up @@ -897,12 +902,19 @@ public void testAlterSchema() throws TException, InterruptedException {

GravitinoMetaLake metalake = client.loadMetalake(NameIdentifier.of(metalakeName));
Catalog catalog = metalake.loadCatalog(NameIdentifier.of(metalakeName, catalogName));
catalog
.asSchemas()
.alterSchema(
ident,
SchemaChange.removeProperty("key1"),
SchemaChange.setProperty("key2", "val2-alter"));
Schema schema = catalog.asSchemas().loadSchema(ident);
Assertions.assertNull(schema.auditInfo().lastModifier());
Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, schema.auditInfo().creator());
schema =
catalog
.asSchemas()
.alterSchema(
ident,
SchemaChange.removeProperty("key1"),
SchemaChange.setProperty("key2", "val2-alter"));

Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, schema.auditInfo().lastModifier());
Assertions.assertEquals(AuthConstants.ANONYMOUS_USER, schema.auditInfo().creator());

Map<String, String> properties2 = catalog.asSchemas().loadSchema(ident).properties();
Assertions.assertFalse(properties2.containsKey("key1"));
Expand Down
Loading
Loading