Copilot commented on code in PR #1310:
URL: https://github.com/apache/ranger/pull/1310#discussion_r4272012769
##########
hdfs-agent/src/test/java/org/apache/ranger/services/hdfs/RangerHdfsAuthorizerTest.java:
##########
@@ -240,6 +299,79 @@ private static INode createNode(String[] pathSegments, int
i, String owner, Stri
return mock;
}
+ private static INode[] createPath(String path, boolean file, boolean
exists) {
+ String[] segments = path.split("/");
+ INode[] ret = new INode[segments.length];
+
+ for (int i = 0; i < segments.length; i++) {
+ boolean isLast = i == segments.length - 1;
+
+ ret[i] = isLast && !exists ? null : createNode(segments, i,
FILE_OWNER, FILE_GROUP, isLast && file);
+ }
+
+ return ret;
+ }
+
+ private static void runWithAuthzOptimizationEnabled(boolean enabled,
ThrowingRunnable runnable) throws Exception {
+ boolean previous = setAuthzOptimizationEnabled(enabled);
+
+ try {
+ runnable.run();
+ } finally {
+ setAuthzOptimizationEnabled(previous);
+ }
+ }
+
+ private static boolean setAuthzOptimizationEnabled(boolean enabled) throws
ReflectiveOperationException {
+ Field pluginField =
RangerHdfsAuthorizer.class.getDeclaredField("rangerPlugin");
+ Object plugin;
+ Field flagField;
+ boolean previous;
+
+ pluginField.setAccessible(true);
+ plugin = pluginField.get(authorizer);
+ flagField =
plugin.getClass().getDeclaredField("authzOptimizationEnabled");
+ flagField.setAccessible(true);
+ previous = flagField.getBoolean(plugin);
+ flagField.setBoolean(plugin, enabled);
+
+ return previous;
+ }
Review Comment:
The test relies on reflective access to private fields by hard-coded names
(`rangerPlugin`, `authzOptimizationEnabled`). This is brittle
(renames/refactors will silently break tests) and can be blocked under stricter
reflective-access settings. Prefer a test-visible API to toggle optimization
(e.g., package-private setter, constructor/config injection, or a dedicated
test hook) so tests remain stable across internal refactors.
##########
hdfs-agent/src/test/java/org/apache/ranger/services/hdfs/RangerHdfsAuthorizerTest.java:
##########
@@ -240,6 +299,79 @@ private static INode createNode(String[] pathSegments, int
i, String owner, Stri
return mock;
}
+ private static INode[] createPath(String path, boolean file, boolean
exists) {
+ String[] segments = path.split("/");
+ INode[] ret = new INode[segments.length];
+
+ for (int i = 0; i < segments.length; i++) {
+ boolean isLast = i == segments.length - 1;
+
+ ret[i] = isLast && !exists ? null : createNode(segments, i,
FILE_OWNER, FILE_GROUP, isLast && file);
+ }
+
+ return ret;
+ }
+
+ private static void runWithAuthzOptimizationEnabled(boolean enabled,
ThrowingRunnable runnable) throws Exception {
+ boolean previous = setAuthzOptimizationEnabled(enabled);
+
+ try {
+ runnable.run();
+ } finally {
+ setAuthzOptimizationEnabled(previous);
+ }
+ }
+
+ private static boolean setAuthzOptimizationEnabled(boolean enabled) throws
ReflectiveOperationException {
+ Field pluginField =
RangerHdfsAuthorizer.class.getDeclaredField("rangerPlugin");
+ Object plugin;
+ Field flagField;
+ boolean previous;
+
+ pluginField.setAccessible(true);
+ plugin = pluginField.get(authorizer);
+ flagField =
plugin.getClass().getDeclaredField("authzOptimizationEnabled");
+ flagField.setAccessible(true);
+ previous = flagField.getBoolean(plugin);
+ flagField.setBoolean(plugin, enabled);
+
+ return previous;
+ }
Review Comment:
These tests mutate a shared authorizer/plugin flag
(`authzOptimizationEnabled`) via reflection. If the test suite runs in parallel
(common in Maven/Surefire configurations), this introduces a race between tests
and can cause flaky failures. Consider forcing same-thread execution for this
class/tests (e.g., JUnit 5 execution control) or isolating state by
constructing a dedicated authorizer instance per test so flag changes are not
global.
##########
hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/OperationOptimizer.java:
##########
@@ -121,6 +121,30 @@ private OptimizedAuthzContext optimizeCreateOp() {
}
private OptimizedAuthzContext optimizeDeleteOp() {
+ return optimizeDirectoryTreeMutationOp();
+ }
+
+ private OptimizedAuthzContext optimizeRenameOp() {
+ if (inode != null && inode.isDirectory()) {
+ return optimizeDirectoryTreeMutationOp();
+ }
+
+ INode nodeToAuthorize = getINodeToAuthorize();
+
+ if (nodeToAuthorize == null) {
+ return OPT_BYPASS_AUTHZ;
+ }
+
+ if (!nodeToAuthorize.isDirectory()) {
+ LOG.debug("nodeToCheck is not a directory for a rename operation!
Optimization skipped");
+
+ return null;
+ }
Review Comment:
The debug message says `nodeToCheck`, but the variable/context here is
`nodeToAuthorize`. Updating the message to match the actual identifier/action
will make troubleshooting optimization decisions less confusing.
##########
hdfs-agent/src/test/java/org/apache/ranger/services/hdfs/RangerHdfsAuthorizerTest.java:
##########
@@ -192,6 +195,62 @@ public void hdfsBaseFileNameTokenReadTest() throws
AccessControlException {
readFailWithPath("/tmp/tmpdir5/t/data-file.txt");
}
+ @Test
+ public void testOperationOptimizer_Rename_DirectorySourceEvaluated()
throws Exception {
Review Comment:
New test method names use underscores, which appears inconsistent with
existing test naming in this class (e.g., `hdfsBaseFileNameTokenReadTest`).
Consider renaming these to the prevailing style (camelCase) to keep test
discovery/readability consistent.
##########
hdfs-agent/src/test/java/org/apache/ranger/services/hdfs/RangerHdfsAuthorizerTest.java:
##########
@@ -240,6 +299,79 @@ private static INode createNode(String[] pathSegments, int
i, String owner, Stri
return mock;
}
+ private static INode[] createPath(String path, boolean file, boolean
exists) {
+ String[] segments = path.split("/");
+ INode[] ret = new INode[segments.length];
+
+ for (int i = 0; i < segments.length; i++) {
+ boolean isLast = i == segments.length - 1;
+
+ ret[i] = isLast && !exists ? null : createNode(segments, i,
FILE_OWNER, FILE_GROUP, isLast && file);
+ }
+
+ return ret;
+ }
+
+ private static void runWithAuthzOptimizationEnabled(boolean enabled,
ThrowingRunnable runnable) throws Exception {
+ boolean previous = setAuthzOptimizationEnabled(enabled);
+
+ try {
+ runnable.run();
+ } finally {
+ setAuthzOptimizationEnabled(previous);
+ }
+ }
+
+ private static boolean setAuthzOptimizationEnabled(boolean enabled) throws
ReflectiveOperationException {
+ Field pluginField =
RangerHdfsAuthorizer.class.getDeclaredField("rangerPlugin");
+ Object plugin;
+ Field flagField;
+ boolean previous;
+
+ pluginField.setAccessible(true);
+ plugin = pluginField.get(authorizer);
+ flagField =
plugin.getClass().getDeclaredField("authzOptimizationEnabled");
+ flagField.setAccessible(true);
+ previous = flagField.getBoolean(plugin);
+ flagField.setBoolean(plugin, enabled);
+
+ return previous;
+ }
+
+ private static void checkPermission(AccessControlEnforcer enforcer, String
userName, INode[] inodes, String path, FsAction ancestorAccess, FsAction
parentAccess, FsAction access, FsAction subAccess, boolean ignoreEmptyDir,
String operationName) throws AccessControlException {
+ INodeAttributes[] attrs = new INodeAttributes[inodes.length];
+
+ for (int i = 0; i < inodes.length; i++) {
+ attrs[i] = inodes[i];
+ }
+
+ INodeAttributeProvider.AuthorizationContext.Builder builder =
+ new INodeAttributeProvider.AuthorizationContext.Builder()
+ .fsOwner(FILE_OWNER)
+ .supergroup(FILE_GROUP)
+
.callerUgi(UserGroupInformation.createUserForTesting(userName, new String[0]))
+ .inodeAttrs(attrs)
+ .inodes(inodes)
+ .pathByNameArr(new byte[0][0])
+ .snapshotId(SNAPSHOT_ID)
+ .path(path)
+ .ancestorIndex(inodes.length - 2)
+ .doCheckOwner(false)
Review Comment:
The helper hard-codes `ancestorIndex(inodes.length - 2)`, which assumes the
provided `inodes` array always has at least 2 elements (and that the ancestor
index semantics always match this derivation). To prevent accidental misuse in
future tests (e.g., root path or short paths), add a clear
precondition/validation in the helper (or compute the ancestor index based on
the AuthorizationContext contract) so failures are deterministic and easier to
diagnose.
##########
hdfs-agent/src/test/java/org/apache/ranger/services/hdfs/RangerHdfsAuthorizerTest.java:
##########
@@ -192,6 +195,62 @@ public void hdfsBaseFileNameTokenReadTest() throws
AccessControlException {
readFailWithPath("/tmp/tmpdir5/t/data-file.txt");
}
+ @Test
+ public void testOperationOptimizer_Rename_DirectorySourceEvaluated()
throws Exception {
+ runWithAuthzOptimizationEnabled(true, () -> {
+ AccessControlEnforcer enforcer =
authorizer.getExternalAccessControlEnforcer(null);
+ INode[] inodes = createPath("/tmp/srcdir", false,
true);
+
+ checkPermission(enforcer, "bob", inodes, "/tmp/srcdir", null,
null, null, null, false, OPERATION_NAME_RENAME);
+
+ Assertions.assertThrows(AccessControlException.class, () ->
+ checkPermission(enforcer, "bob", inodes, "/tmp/srcdir",
null, FsAction.WRITE, null, FsAction.ALL, true, OPERATION_NAME_RENAME));
+
+ // mv of a directory: no subAccess
+ Assertions.assertThrows(AccessControlException.class, () ->
+
checkPermission(authorizer.getExternalAccessControlEnforcer(null), "bob",
inodes, "/tmp/srcdir", null, FsAction.WRITE, null, null, false,
OPERATION_NAME_RENAME));
+
+ checkPermission(authorizer.getExternalAccessControlEnforcer(null),
"bob", createPath("/tmp/tmpdir2/srcdir", false, true), "/tmp/tmpdir2/srcdir",
null, FsAction.WRITE, null, null, false, OPERATION_NAME_RENAME);
+ });
+
+ runWithAuthzOptimizationEnabled(false, () -> {
+ Assertions.assertThrows(AccessControlException.class, () ->
+
checkPermission(authorizer.getExternalAccessControlEnforcer(null), "bob",
createPath("/tmp/srcdir", false, true), "/tmp/srcdir", null, FsAction.WRITE,
null, FsAction.ALL, true, OPERATION_NAME_RENAME));
+ });
+ }
+
+ @Test
+ public void testOperationOptimizer_Rename_FileUnchanged() throws Exception
{
+ runWithAuthzOptimizationEnabled(true, () -> {
+ AccessControlEnforcer enforcer =
authorizer.getExternalAccessControlEnforcer(null);
+
+ checkPermission(enforcer, "bob",
createPath("/tmp/tmpdir2/data-file3", true, true), "/tmp/tmpdir2/data-file3",
null, null, null, null, false, OPERATION_NAME_RENAME);
+
+ Assertions.assertThrows(AccessControlException.class, () ->
+
checkPermission(authorizer.getExternalAccessControlEnforcer(null), "bob",
createPath("/tmp/data-file3", true, true), "/tmp/data-file3", null, null, null,
null, false, OPERATION_NAME_RENAME));
+ Assertions.assertThrows(AccessControlException.class, () ->
+
checkPermission(authorizer.getExternalAccessControlEnforcer(null), "bob",
createPath("/tmp/data-file3", true, true), "/tmp/data-file3", null,
FsAction.WRITE, null, null, true, OPERATION_NAME_RENAME));
+ });
+
+ runWithAuthzOptimizationEnabled(false, () -> {
+ Assertions.assertThrows(AccessControlException.class, () ->
+
checkPermission(authorizer.getExternalAccessControlEnforcer(null), "bob",
createPath("/tmp/data-file3", true, true), "/tmp/data-file3", null,
FsAction.WRITE, null, null, true, OPERATION_NAME_RENAME));
+ });
+ }
+
+ @Test
+ public void testOperationOptimizer_DeleteRegression() throws Exception {
Review Comment:
New test method names use underscores, which appears inconsistent with
existing test naming in this class (e.g., `hdfsBaseFileNameTokenReadTest`).
Consider renaming these to the prevailing style (camelCase) to keep test
discovery/readability consistent.
##########
hdfs-agent/src/test/java/org/apache/ranger/services/hdfs/RangerHdfsAuthorizerTest.java:
##########
@@ -192,6 +195,62 @@ public void hdfsBaseFileNameTokenReadTest() throws
AccessControlException {
readFailWithPath("/tmp/tmpdir5/t/data-file.txt");
}
+ @Test
+ public void testOperationOptimizer_Rename_DirectorySourceEvaluated()
throws Exception {
+ runWithAuthzOptimizationEnabled(true, () -> {
+ AccessControlEnforcer enforcer =
authorizer.getExternalAccessControlEnforcer(null);
+ INode[] inodes = createPath("/tmp/srcdir", false,
true);
+
+ checkPermission(enforcer, "bob", inodes, "/tmp/srcdir", null,
null, null, null, false, OPERATION_NAME_RENAME);
+
+ Assertions.assertThrows(AccessControlException.class, () ->
+ checkPermission(enforcer, "bob", inodes, "/tmp/srcdir",
null, FsAction.WRITE, null, FsAction.ALL, true, OPERATION_NAME_RENAME));
+
+ // mv of a directory: no subAccess
+ Assertions.assertThrows(AccessControlException.class, () ->
+
checkPermission(authorizer.getExternalAccessControlEnforcer(null), "bob",
inodes, "/tmp/srcdir", null, FsAction.WRITE, null, null, false,
OPERATION_NAME_RENAME));
+
+ checkPermission(authorizer.getExternalAccessControlEnforcer(null),
"bob", createPath("/tmp/tmpdir2/srcdir", false, true), "/tmp/tmpdir2/srcdir",
null, FsAction.WRITE, null, null, false, OPERATION_NAME_RENAME);
+ });
+
+ runWithAuthzOptimizationEnabled(false, () -> {
+ Assertions.assertThrows(AccessControlException.class, () ->
+
checkPermission(authorizer.getExternalAccessControlEnforcer(null), "bob",
createPath("/tmp/srcdir", false, true), "/tmp/srcdir", null, FsAction.WRITE,
null, FsAction.ALL, true, OPERATION_NAME_RENAME));
+ });
+ }
+
+ @Test
+ public void testOperationOptimizer_Rename_FileUnchanged() throws Exception
{
Review Comment:
New test method names use underscores, which appears inconsistent with
existing test naming in this class (e.g., `hdfsBaseFileNameTokenReadTest`).
Consider renaming these to the prevailing style (camelCase) to keep test
discovery/readability consistent.
--
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]
