Copilot commented on code in PR #13833:
URL: https://github.com/apache/gravitino/pull/13833#discussion_r4260218591
##########
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:
These tests assert reference identity (`assertSame`) for the filtered array,
which makes them brittle to harmless implementation changes (e.g., returning an
equal array copy while still short-circuiting per-object checks). Prefer
asserting content equality (e.g., `assertArrayEquals`) and keep the behavioral
assertion via `verify(authorizer, never()).authorize(...)`. Apply similarly to
the other newly added `assertSame` checks in this file.
##########
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:
`TABLE_PARENT_SCOPES` is now used for several non-table entity types (views,
functions, etc.). This is confusing and makes the code harder to reason about
(the constant name no longer matches its usage). Consider renaming the constant
to something scope-based (e.g., `METALAKE_CATALOG_SCHEMA_PARENT_SCOPES`) or
introducing a new alias constant with a clearer name and using it in these 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);
+ 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:
The new tests repeat substantial setup/teardown boilerplate (mocking
`EntityStore`, toggling `cacheEnabled`, wiring dispatchers, and resetting them
in `finally`). Consider extracting a small helper (e.g., a method that takes a
lambda and sets up/resets `gravitinoEnv` consistently) to reduce duplication
and make future additions less error-prone.
##########
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:
The new tests repeat substantial setup/teardown boilerplate (mocking
`EntityStore`, toggling `cacheEnabled`, wiring dispatchers, and resetting them
in `finally`). Consider extracting a small helper (e.g., a method that takes a
lambda and sets up/resets `gravitinoEnv` consistently) to reduce duplication
and make future additions less error-prone.
--
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]
