Copilot commented on code in PR #1302:
URL: https://github.com/apache/ranger/pull/1302#discussion_r4229482033
##########
agents-common/src/main/java/org/apache/ranger/authorization/utils/JsonUtils.java:
##########
@@ -197,6 +199,20 @@ public static Map<String, String>
jsonToMapStringString(String jsonStr) {
return ret;
}
+ public static Map<String, Object> jsonToMapStringObject(String jsonStr) {
+ Map<String, Object> ret = null;
+
+ if (StringUtils.isNotEmpty(jsonStr)) {
+ try {
+ ret = getMapper().readValue(jsonStr, TYPE_MAP_STRING_OBJECT);
+ } catch (Exception excp) {
+ LOG.warn("jsonToMapStringObject() failed to convert json to
object: {}", jsonStr, excp);
Review Comment:
This log line includes the full `jsonStr`, which may contain sensitive
request context attributes (PII/transaction data). Consider avoiding logging
raw input (or logging only a truncated/redacted version and/or length) to
prevent sensitive-data disclosure in logs.
##########
agents-common/src/test/java/org/apache/ranger/plugin/util/TestRangerAccessRequestUtil.java:
##########
@@ -246,4 +247,43 @@ public ResourceMatchingScope getResourceMatchingScope() {
};
assertEquals(rs, RangerAccessRequestUtil.getUserRoles(req));
}
+
+ @Test
+ public void testContextAttributes() {
+ Map<String, Object> context = new HashMap<>();
+
+ // null context
+ assertEquals(Collections.emptyMap(),
JsonUtils.jsonToMapStringObject(null));
+
+ Map<String, Map<String, Object>> tests = new HashMap<>() {{
+ put(null, Collections.emptyMap());
// null
+ put("", Collections.emptyMap());
// empty
+ put("[]", Collections.emptyMap());
// invalid map value
+ put("xyz", Collections.emptyMap()); // invalid map value
+ put("xyz=43", Collections.emptyMap()); // invalid map value
+ put("xyz: 43", Collections.emptyMap()); // invalid map value
+ put("{xyz: 43", Collections.emptyMap()); // invalid map value
+ put("{}", Collections.emptyMap()); // valid map value:
empty
+ put("{'amount': 43}", toMap("amount", 43)); // valid map
value, one entry
+ put("{\"amount\": 43}", toMap("amount", 43)); // valid map
value, one entry
+ put("{\"amount\": 43, \"limit\": 127}", toMap("amount", 43,
"limit", 127)); // valid map value, two entries
+ }};
Review Comment:
This uses double-brace initialization, which creates an anonymous inner
class and can retain unintended references and complicate
debugging/serialization. Prefer building the map explicitly (e.g., sequential
`put()` calls, or `Map.ofEntries(...)`) to keep the test simpler and avoid the
anonymous class.
##########
agents-common/src/test/java/org/apache/ranger/plugin/conditionevaluator/RangerRequestScriptEvaluatorTest.java:
##########
@@ -605,6 +605,44 @@ void testBlockJavaClassReferencesWithDefaultConfig() {
Assertions.assertTrue(escaped.isEmpty(), "Sandbox escape OS command
executed via:\n " + String.join("\n ", escaped));
}
+ @Test
+ public void testContextAttributes() {
+ String attrJson = "{'trx_amount':10000}";
Review Comment:
This uses single quotes, which is not valid JSON per the spec and may break
if the underlying Jackson/ObjectMapper configuration changes (or differs across
environments). Prefer standard JSON with double quotes here (and in subsequent
`attrJson` strings) to keep the test robust and aligned with real-world JSON
producers.
##########
agents-common/src/main/java/org/apache/ranger/plugin/util/RangerAccessRequestUtil.java:
##########
@@ -60,11 +60,26 @@ public class RangerAccessRequestUtil {
public static final String KEY_CONTEXT_BATCH_EVAL_CONTEXT =
"BATCH_EVAL_CONTEXT";
public static final String KEY_CONTEXT_RESOURCE_ZONE_NAMES =
"RESOURCE_ZONE_NAMES";
public static final String KEY_CONTEXT_ACL_ENFORCER =
"_ACL_ENFORCER";
+ public static final String KEY_CONTEXT_ATTR_JSON =
"_ATTR_JSON";
private RangerAccessRequestUtil() {
// to avoid instantiation
}
+ public static void setContextAttrJson(Map<String, Object> context, String
attrJson) {
+ if (context != null) {
+ if (attrJson == null) {
+ context.remove(KEY_CONTEXT_ATTR_JSON);
+ } else {
+ context.put(KEY_CONTEXT_ATTR_JSON, attrJson);
+ }
+ }
+ }
+
+ public static String getContextAttrJson(Map<String, Object> context) {
+ return context != null ? (String) context.get(KEY_CONTEXT_ATTR_JSON) :
null;
Review Comment:
This unchecked cast can throw `ClassCastException` if a non-String value is
present under `_ATTR_JSON` (e.g., if another caller populates the context
directly). Consider using an `instanceof String` check (or
`String.valueOf(...)` with a null check) to make this helper more defensive and
avoid a runtime failure in request processing.
##########
agents-common/src/test/java/org/apache/ranger/plugin/util/TestRangerAccessRequestUtil.java:
##########
@@ -246,4 +247,43 @@ public ResourceMatchingScope getResourceMatchingScope() {
};
assertEquals(rs, RangerAccessRequestUtil.getUserRoles(req));
}
+
+ @Test
+ public void testContextAttributes() {
+ Map<String, Object> context = new HashMap<>();
+
+ // null context
+ assertEquals(Collections.emptyMap(),
JsonUtils.jsonToMapStringObject(null));
+
+ Map<String, Map<String, Object>> tests = new HashMap<>() {{
+ put(null, Collections.emptyMap());
// null
+ put("", Collections.emptyMap());
// empty
+ put("[]", Collections.emptyMap());
// invalid map value
+ put("xyz", Collections.emptyMap()); // invalid map value
+ put("xyz=43", Collections.emptyMap()); // invalid map value
+ put("xyz: 43", Collections.emptyMap()); // invalid map value
+ put("{xyz: 43", Collections.emptyMap()); // invalid map value
+ put("{}", Collections.emptyMap()); // valid map value:
empty
+ put("{'amount': 43}", toMap("amount", 43)); // valid map
value, one entry
+ put("{\"amount\": 43}", toMap("amount", 43)); // valid map
value, one entry
+ put("{\"amount\": 43, \"limit\": 127}", toMap("amount", 43,
"limit", 127)); // valid map value, two entries
+ }};
+
+ tests.forEach((key, value) -> {
+ RangerAccessRequestUtil.setContextAttrJson(context, key);
+ assertEquals(value,
JsonUtils.jsonToMapStringObject(RangerAccessRequestUtil.getContextAttrJson(context)));
+ });
+ }
+
+ private static Map<String, Object> toMap(String key1, Object val1,
Object...args) {
+ Map<String, Object> ret = new HashMap<>();
+
+ ret.put(key1, val1);
+
Review Comment:
The loop assumes `args` has an even length; if it is ever called with an odd
number of varargs, `args[idx + 1]` will throw `ArrayIndexOutOfBoundsException`.
Add a guard (e.g., validate `args.length % 2 == 0` and fail fast with a clear
exception) to make the helper safe and the failure mode clearer.
--
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]
