Copilot commented on code in PR #646:
URL: https://github.com/apache/ranger/pull/646#discussion_r2305801789
##########
agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineImpl.java:
##########
@@ -1072,13 +1072,36 @@ private void updateFromGdsResult(RangerAccessResult
result) {
if (gdsResult != null) {
if (result.getPolicyType() == RangerPolicy.POLICY_TYPE_ACCESS) {
+ // pick access result from GDS policies only if there is no
decision yet
if (!result.getIsAccessDetermined() &&
gdsResult.getIsAllowed()) {
result.setIsAllowed(true);
result.setIsAccessDetermined(true);
result.setPolicyId(gdsResult.getPolicyId());
result.setPolicyVersion(gdsResult.getPolicyVersion());
result.setPolicyPriority(RangerPolicy.POLICY_PRIORITY_NORMAL);
}
+ } else if (result.getPolicyType() ==
RangerPolicy.POLICY_TYPE_ROWFILTER) {
+ // pick row-filter from GDS policies only if there is no
decision yet
+ if (result.getPolicyId() != -1 &&
CollectionUtils.isNotEmpty(gdsResult.getRowFilters())) {
+ result.setIsAllowed(true);
+ result.setIsAccessDetermined(true);
+ result.setPolicyId(gdsResult.getPolicyId());
+ result.setPolicyVersion(gdsResult.getPolicyVersion());
+
result.setPolicyPriority(RangerPolicy.POLICY_PRIORITY_NORMAL);
+ result.setFilterExpr(gdsResult.getRowFilters().get(0));
+ }
+ } else if (result.getPolicyType() ==
RangerPolicy.POLICY_TYPE_DATAMASK) {
+ // pick data-mask from GDS policies only if there is no
decision yet
+ if (result.getPolicyId() != -1 &&
StringUtils.isNotEmpty(gdsResult.getMaskType())) {
Review Comment:
Similar to the row-filter case, the condition `result.getPolicyId() != -1`
for data-mask evaluation seems incorrect. This would prevent applying GDS
data-masks when no previous policy was found, which contradicts the goal of
evaluating GDS policies independently.
```suggestion
if (!result.getIsAccessDetermined() &&
StringUtils.isNotEmpty(gdsResult.getMaskType())) {
```
##########
agents-common/src/main/java/org/apache/ranger/plugin/policyengine/RangerPolicyEngineImpl.java:
##########
@@ -1072,13 +1072,36 @@ private void updateFromGdsResult(RangerAccessResult
result) {
if (gdsResult != null) {
if (result.getPolicyType() == RangerPolicy.POLICY_TYPE_ACCESS) {
+ // pick access result from GDS policies only if there is no
decision yet
if (!result.getIsAccessDetermined() &&
gdsResult.getIsAllowed()) {
result.setIsAllowed(true);
result.setIsAccessDetermined(true);
result.setPolicyId(gdsResult.getPolicyId());
result.setPolicyVersion(gdsResult.getPolicyVersion());
result.setPolicyPriority(RangerPolicy.POLICY_PRIORITY_NORMAL);
}
+ } else if (result.getPolicyType() ==
RangerPolicy.POLICY_TYPE_ROWFILTER) {
+ // pick row-filter from GDS policies only if there is no
decision yet
+ if (result.getPolicyId() != -1 &&
CollectionUtils.isNotEmpty(gdsResult.getRowFilters())) {
Review Comment:
The condition `result.getPolicyId() != -1` seems incorrect for row-filter
evaluation. This condition suggests that a policy must already be set, but for
row-filters from GDS, we should apply them even when no previous policy was
found. Consider changing this to check if no row-filter is already set instead.
```suggestion
// pick row-filter from GDS policies only if there is no
row-filter already set
if (StringUtils.isEmpty(result.getFilterExpr()) &&
CollectionUtils.isNotEmpty(gdsResult.getRowFilters())) {
```
--
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]
