LuciferYang commented on code in PR #13833:
URL: https://github.com/apache/gravitino/pull/13833#discussion_r4268517573
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java:
##########
@@ -108,58 +108,111 @@ public class MetadataAuthzHelper {
*/
private static final Map<Entity.EntityType, Map<String,
List<ParentScopeAccessPath>>>
LIST_SHORT_CIRCUITS =
- Map.of(
- Entity.EntityType.USER,
- principalListPaths(
-
AuthorizationExpressionConstants.LOAD_USER_AUTHORIZATION_EXPRESSION,
- Privilege.Name.MANAGE_USERS),
- Entity.EntityType.GROUP,
- principalListPaths(
-
AuthorizationExpressionConstants.LOAD_GROUP_AUTHORIZATION_EXPRESSION,
- Privilege.Name.MANAGE_GROUPS),
- Entity.EntityType.ROLE,
- principalListPaths(
-
AuthorizationExpressionConstants.LOAD_ROLE_AUTHORIZATION_EXPRESSION,
- Privilege.Name.MANAGE_GRANTS),
- Entity.EntityType.TABLE,
- Map.of(
-
AuthorizationExpressionConstants.FILTER_TABLE_AUTHORIZATION_EXPRESSION,
- List.of(
- parentOwnerPath(TABLE_PARENT_SCOPES),
- parentPrivilegePath(Privilege.Name.SELECT_TABLE,
TABLE_PARENT_SCOPES),
- parentPrivilegePath(Privilege.Name.MODIFY_TABLE,
TABLE_PARENT_SCOPES)),
-
AuthorizationExpressionConstants.LIST_TABLE_LIKE_AUTHORIZATION_EXPRESSION,
- List.of(
- parentOwnerPath(TABLE_PARENT_SCOPES),
-
tableLikeParentPrivilegePath(Privilege.Name.PROBE_TABLE_LIKE),
-
tableLikeParentPrivilegePath(Privilege.Name.SELECT_TABLE),
-
tableLikeParentPrivilegePath(Privilege.Name.MODIFY_TABLE),
-
tableLikeParentPrivilegePath(Privilege.Name.CREATE_TABLE),
-
tableLikeParentPrivilegePath(Privilege.Name.CREATE_VIEW))),
- Entity.EntityType.MODEL,
- Map.of(
-
AuthorizationExpressionConstants.FILTER_MODEL_AUTHORIZATION_EXPRESSION,
- List.of(
- parentOwnerPath(MODEL_PARENT_SCOPES),
- parentPrivilegePath(Privilege.Name.USE_MODEL,
MODEL_PARENT_SCOPES))),
- Entity.EntityType.JOB_TEMPLATE,
- Map.of(
-
AuthorizationExpressionConstants.LOAD_JOB_TEMPLATE_AUTHORIZATION_EXPRESSION,
- List.of(
- parentOwnerPath(METALAKE_ONLY_SCOPE),
- parentPrivilegePath(Privilege.Name.USE_JOB_TEMPLATE,
METALAKE_ONLY_SCOPE))),
- Entity.EntityType.SCHEMA,
- Map.of(
-
AuthorizationExpressionConstants.FILTER_SCHEMA_AUTHORIZATION_EXPRESSION,
- List.of(
- parentOwnerPath(SCHEMA_PARENT_SCOPES),
- parentPrivilegePath(Privilege.Name.USE_SCHEMA,
SCHEMA_PARENT_SCOPES))),
- Entity.EntityType.CATALOG,
- Map.of(
-
AuthorizationExpressionConstants.LOAD_CATALOG_AUTHORIZATION_EXPRESSION,
- List.of(
- parentOwnerPath(CATALOG_PARENT_SCOPES),
- parentPrivilegePath(Privilege.Name.USE_CATALOG,
CATALOG_PARENT_SCOPES))));
+ Map.ofEntries(
+ Map.entry(
+ Entity.EntityType.USER,
+ principalListPaths(
+
AuthorizationExpressionConstants.LOAD_USER_AUTHORIZATION_EXPRESSION,
+ Privilege.Name.MANAGE_USERS)),
+ Map.entry(
+ Entity.EntityType.GROUP,
+ principalListPaths(
+
AuthorizationExpressionConstants.LOAD_GROUP_AUTHORIZATION_EXPRESSION,
+ Privilege.Name.MANAGE_GROUPS)),
+ Map.entry(
+ Entity.EntityType.ROLE,
+ principalListPaths(
+
AuthorizationExpressionConstants.LOAD_ROLE_AUTHORIZATION_EXPRESSION,
+ Privilege.Name.MANAGE_GRANTS)),
+ Map.entry(
+ Entity.EntityType.TABLE,
+ Map.of(
+
AuthorizationExpressionConstants.FILTER_TABLE_AUTHORIZATION_EXPRESSION,
+ List.of(
+ parentOwnerPath(TABLE_PARENT_SCOPES),
+ parentPrivilegePath(Privilege.Name.SELECT_TABLE,
TABLE_PARENT_SCOPES),
+ parentPrivilegePath(Privilege.Name.MODIFY_TABLE,
TABLE_PARENT_SCOPES)),
+
AuthorizationExpressionConstants.LIST_TABLE_LIKE_AUTHORIZATION_EXPRESSION,
+ List.of(
+ parentOwnerPath(TABLE_PARENT_SCOPES),
+
tableLikeParentPrivilegePath(Privilege.Name.PROBE_TABLE_LIKE),
+
tableLikeParentPrivilegePath(Privilege.Name.SELECT_TABLE),
+
tableLikeParentPrivilegePath(Privilege.Name.MODIFY_TABLE),
+
tableLikeParentPrivilegePath(Privilege.Name.CREATE_TABLE),
+
tableLikeParentPrivilegePath(Privilege.Name.CREATE_VIEW)))),
+ Map.entry(
+ Entity.EntityType.MODEL,
+ Map.of(
+
AuthorizationExpressionConstants.FILTER_MODEL_AUTHORIZATION_EXPRESSION,
+ List.of(
+ parentOwnerPath(MODEL_PARENT_SCOPES),
+ parentPrivilegePath(Privilege.Name.USE_MODEL,
MODEL_PARENT_SCOPES)))),
+ Map.entry(
+ Entity.EntityType.VIEW,
+ Map.of(
+
AuthorizationExpressionConstants.FILTER_VIEW_AUTHORIZATION_EXPRESSION,
+ List.of(
+ parentOwnerPath(TABLE_PARENT_SCOPES),
+ parentPrivilegePath(Privilege.Name.SELECT_VIEW,
TABLE_PARENT_SCOPES)))),
+ Map.entry(
+ Entity.EntityType.FUNCTION,
+ Map.of(
+
AuthorizationExpressionConstants.FILTER_FUNCTION_AUTHORIZATION_EXPRESSION,
+ List.of(
+ parentOwnerPath(TABLE_PARENT_SCOPES),
+ parentPrivilegePath(Privilege.Name.EXECUTE_FUNCTION,
TABLE_PARENT_SCOPES),
+ parentPrivilegePath(
+ Privilege.Name.MODIFY_FUNCTION,
TABLE_PARENT_SCOPES)))),
Review Comment:
Added SCHEMA_OBJECT_PARENT_SCOPES for the new entries.
##########
server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataAuthzHelper.java:
##########
@@ -489,6 +489,377 @@ public void testListShortCircuitModelViaSchemaGrant() {
}
}
+ /** A SELECT_VIEW grant on the schema makes every view visible without
per-view checks. */
+ @Test
+ public void testListShortCircuitViewViaSchemaGrant() {
+ EntityStore store = mock(EntityStore.class);
+ when(gravitinoEnv.entityStore()).thenReturn(store);
+ when(gravitinoEnv.cacheEnabled()).thenReturn(true);
+ when(gravitinoEnv.internalAccessControlDispatcher())
+ .thenReturn(mock(AccessControlDispatcher.class));
+ GravitinoAuthorizer authorizer =
+ mockParentGrantAuthorizer(MetadataObject.Type.SCHEMA,
Privilege.Name.SELECT_VIEW);
+ NameIdentifier[] identifiers = views(2000);
+ try {
+ withAuthorizer(
+ authorizer,
+ () -> {
+ NameIdentifier[] filtered =
+ MetadataAuthzHelper.filterByExpression(
+ "testMetalake",
+
AuthorizationExpressionConstants.FILTER_VIEW_AUTHORIZATION_EXPRESSION,
+ Entity.EntityType.VIEW,
+ identifiers);
+ Assertions.assertSame(identifiers, filtered);
Review Comment:
Switched to assertArrayEquals. The never().authorize(X) check could not fail
either way, since the per-object path stops at the schema term, so the tests
now verify isOwner is never called on the listed objects, which the per-object
path does for each of them.
##########
server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataAuthzHelper.java:
##########
@@ -489,6 +489,377 @@ public void testListShortCircuitModelViaSchemaGrant() {
}
}
+ /** A SELECT_VIEW grant on the schema makes every view visible without
per-view checks. */
+ @Test
+ public void testListShortCircuitViewViaSchemaGrant() {
+ EntityStore store = mock(EntityStore.class);
+ when(gravitinoEnv.entityStore()).thenReturn(store);
+ when(gravitinoEnv.cacheEnabled()).thenReturn(true);
+ when(gravitinoEnv.internalAccessControlDispatcher())
+ .thenReturn(mock(AccessControlDispatcher.class));
Review Comment:
Extracted withPreloadEnv for the fileset and topic tests; views, functions
and semantic models never preload, so their tests no longer set up the store at
all.
##########
server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataAuthzHelper.java:
##########
@@ -489,6 +489,377 @@ public void testListShortCircuitModelViaSchemaGrant() {
}
}
+ /** A SELECT_VIEW grant on the schema makes every view visible without
per-view checks. */
+ @Test
+ public void testListShortCircuitViewViaSchemaGrant() {
+ EntityStore store = mock(EntityStore.class);
+ when(gravitinoEnv.entityStore()).thenReturn(store);
+ when(gravitinoEnv.cacheEnabled()).thenReturn(true);
+ when(gravitinoEnv.internalAccessControlDispatcher())
+ .thenReturn(mock(AccessControlDispatcher.class));
+ GravitinoAuthorizer authorizer =
+ mockParentGrantAuthorizer(MetadataObject.Type.SCHEMA,
Privilege.Name.SELECT_VIEW);
+ NameIdentifier[] identifiers = views(2000);
+ try {
+ withAuthorizer(
+ authorizer,
+ () -> {
+ NameIdentifier[] filtered =
+ MetadataAuthzHelper.filterByExpression(
+ "testMetalake",
+
AuthorizationExpressionConstants.FILTER_VIEW_AUTHORIZATION_EXPRESSION,
+ Entity.EntityType.VIEW,
+ identifiers);
+ Assertions.assertSame(identifiers, filtered);
+ verify(authorizer, never())
+ .authorize(
+ any(),
+ eq("testMetalake"),
+ argThat(object -> object.type() ==
MetadataObject.Type.VIEW),
+ any(),
+ any());
+ verifyNoInteractions(store);
+ });
+ } finally {
+ when(gravitinoEnv.cacheEnabled()).thenReturn(false);
+ when(gravitinoEnv.entityStore()).thenReturn(null);
+ when(gravitinoEnv.internalAccessControlDispatcher()).thenReturn(null);
+ }
Review Comment:
Same change as above.
##########
server-common/src/test/java/org/apache/gravitino/server/authorization/TestMetadataAuthzHelper.java:
##########
@@ -775,6 +1123,27 @@ private GravitinoAuthorizer mockParentGrantAuthorizer(
return authorizer;
}
+ /**
+ * Runs {@code body} with the entity cache enabled and the store and schema
dispatcher that the
+ * list preload uses replaced by the given mocks.
+ */
+ private void withPreloadEnv(EntityStore store, SchemaDispatcher
schemaDispatcher, Runnable body) {
+ when(schemaDispatcher.schemaExists(any())).thenReturn(true);
+ when(gravitinoEnv.entityStore()).thenReturn(store);
+ when(gravitinoEnv.internalSchemaDispatcher()).thenReturn(schemaDispatcher);
+ when(gravitinoEnv.internalAccessControlDispatcher())
+ .thenReturn(mock(AccessControlDispatcher.class));
+ when(gravitinoEnv.cacheEnabled()).thenReturn(true);
+ try {
+ body.run();
+ } finally {
+ when(gravitinoEnv.cacheEnabled()).thenReturn(false);
+ when(gravitinoEnv.entityStore()).thenReturn(null);
+ when(gravitinoEnv.internalSchemaDispatcher()).thenReturn(null);
+ when(gravitinoEnv.internalAccessControlDispatcher()).thenReturn(null);
+ }
+ }
Review Comment:
`@BeforeAll` only stubs `config()`, so `false`/`null` is already the default
for these four methods, and the other tests in this file reset them the same
way. `Mockito.reset(gravitinoEnv)` would also drop the `config()` stub that the
rest of the class depends on.
##########
server-common/src/main/java/org/apache/gravitino/server/authorization/MetadataAuthzHelper.java:
##########
@@ -96,6 +96,7 @@ public class MetadataAuthzHelper {
private static final String TABLE_PARENT_SCOPES = "METALAKE, CATALOG,
SCHEMA";
private static final String MODEL_PARENT_SCOPES = "METALAKE, CATALOG,
SCHEMA";
+ private static final String SCHEMA_OBJECT_PARENT_SCOPES = "METALAKE,
CATALOG, SCHEMA";
Review Comment:
`TABLE_PARENT_SCOPES` and `MODEL_PARENT_SCOPES` already share this value on
main as separate constants, and the previous round asked for a non-table name
for the new entries. Folding all three together would rewrite the existing
table and model entries, so I'd keep that out of this PR.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
