> On Dec. 2, 2022, 11:02 p.m., Madhan Neethiraj wrote: > > security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java > > Line 608 (original), 628 (patched) > > <https://reviews.apache.org/r/74229/diff/1/?file=2272257#file2272257line637> > > > > addition to deny-exception can/must be skipped in following cases: > > 1. policy has no deny policy items > > 2. policy has denyAllElse=true > > > > Case #1 is covered by #629. > > > > To cover #1, how about updating processApplyPolicyForItemType() as > > below: > > > > List<RangerPolicy.RangerPolicyItem> appliedPolicyItems = null; > > > > if (policyItemType == ALLOW) { > > appliedPolicyItems = appliedPolicy.getPolicyItems(); > > } else if (existingPolicy.getIsDenyAllElse() || > > appliedPolicy.getIsDenyAllElse()) { > > LOG.warn("processApplyPolicyForItemType(): isDenyAllElse=true, > > ignoring invalid policyItemType=" + policyItemType); > > } else { > > if (policyItemType == DENY) { > > appliedPolicyItems = appliedPolicy.getDenyPolicyItems(); > > } else if (policyItemType == ALLOW_EXCEPTIONS) { > > appliedPolicyItems = appliedPolicy.getAllowExceptions(); > > } else if (policyItemType == DENY_EXCEPTIONS) { > > appliedPolicyItems = appliedPolicy.getDenyExceptions(); > > } else { > > LOG.warn("processApplyPolicyForItemType(): ignoring invalid > > policyItemType=" + policyItemType); > > } > > }
I think making this change will quietly ignore the (erroneously) specified deny/denyException policyItems in the applied-policy. The caller of the API will not come to know about the wrong input provided to the applyPolicy() function. I think it is better to do the computations here, and let the subsequent updatePolicy() function fail with validation errors. Moreover, the code above does not take care of the case where existingPolicy has isDenyAllElse set to true, but appliedPolicy has isDenyAllElse set to false. In that case, it is legitimate to have DENY and DENY_EXCEPTIONs specified in the applied policy. - Abhay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/74229/#review224943 ----------------------------------------------------------- On Dec. 2, 2022, 6:39 p.m., Abhay Kulkarni wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/74229/ > ----------------------------------------------------------- > > (Updated Dec. 2, 2022, 6:39 p.m.) > > > Review request for ranger, madhan, Madhan Neethiraj, Pradeep Agrawal, Ramesh > Mani, and Velmurugan Periasamy. > > > Bugs: RANGER-3995 > https://issues.apache.org/jira/browse/RANGER-3995 > > > Repository: ranger > > > Description > ------- > > Steps to reproduce :- > > 1. Make a POST request to the below mentioned API endpoint, using a policy > json where isDenyAllElse flag is set true > > /service/public/v2/api/policy/apply > 2. Fetch the policy using the newly created policy id, and try to make a POST > request to "/policy/apply" using the policy json obtained from the GET > request. The request results in an error > > > Diffs > ----- > > security-admin/src/main/java/org/apache/ranger/rest/ServiceRESTUtil.java > b56fd3966 > > > Diff: https://reviews.apache.org/r/74229/diff/1/ > > > Testing > ------- > > Tested per the steps listed above. No error was reported and the policy was > applied correctly > > > Thanks, > > Abhay Kulkarni > >
