mneethiraj commented on code in PR #1073:
URL: https://github.com/apache/ranger/pull/1073#discussion_r3601851069
##########
security-admin/src/main/java/org/apache/ranger/patch/PatchForOzoneServiceDefPolicyConditionUpdate_J10065.java:
##########
@@ -131,7 +136,21 @@ private void updateOzoneServiceDef() {
return;
}
- dbOzoneServiceDef.setPolicyConditions(embeddedPolicyConditions);
+ final boolean enableActionMatcherInPoliciesCondition =
RangerAdminConfig.getInstance().getBoolean(RangerServiceDefService.PROP_ENABLE_ACTION_MATCHER_IN_POLICIES_CONDITION,
false);
Review Comment:
Are changes in `PatchForOzoneServiceDefPolicyConditionUpdate_J10065.java`
necessary, given `RangerServiceDefService.java` handles service-def updates
based on feature-flag while retrieving the service-def from the database? If no
additional cases are handled here, I suggest reverting changes in
`PatchForOzoneServiceDefPolicyConditionUpdate_J10065.java`.
##########
plugin-ozone/src/main/java/org/apache/ranger/authorization/ozone/authorizer/RangerOzoneAuthorizer.java:
##########
@@ -384,4 +395,36 @@ private static String toRangerPermission(ACLType acl) {
return null;
}
+
+ private static boolean isOzoneActionPolicyEnabled(final RangerBasePlugin
plugin) {
+ if (plugin == null || plugin.getServiceDef() == null) {
Review Comment:
I suggest replacing lines 400 to 410 with:
```
final RangerServceDef serviceDef = plugin != null ?
plugin.getServiceDef() : null;
final Map<String, String> options = serviceDef != null ?
serviceDef.getOptions() : null;
if (options == null) {
return false;
}
return ServiceDefUtil.getBooleanValue(options,
OPTION_ENABLE_ACTION_MATCHER_IN_POLICIES_CONDITION, false) ||
ServiceDefUtil.getBooleanValue(options,
LEGACY_OPTION_ENABLE_OZONE_ACTION_POLICY, false);
##########
security-admin/src/main/webapp/react-webapp/src/views/PolicyListing/AddUpdatePolicyForm.jsx:
##########
@@ -511,15 +511,18 @@ export default function AddUpdatePolicyForm() {
data.conditions = {};
for (let val of policyData.conditions) {
let conditionObj = find(
- policyConditionUpdatedJSON(serviceCompData?.policyConditions),
+ policyConditionUpdatedJSON(serviceCompData?.policyConditions ||
[]),
function (m) {
if (m.name == val.type) {
return m;
}
}
);
- if (!isEmpty(conditionObj.uiHint)) {
+ // Service-def conditions can be feature-flagged off (e.g. ozone
action-matches).
Review Comment:
I suggest removing references to "feature-flag" here. From the UI
perspective, it will simply ignore conditions in policies that are not
specified in the service-def.
Same for the comment in line 715 as well.
##########
security-admin/src/main/java/org/apache/ranger/service/RangerServiceDefService.java:
##########
@@ -21,17 +21,25 @@
import org.apache.ranger.authorization.hadoop.config.RangerAdminConfig;
import org.apache.ranger.entity.XXServiceDef;
import org.apache.ranger.plugin.model.RangerServiceDef;
+import
org.apache.ranger.plugin.model.RangerServiceDef.RangerPolicyConditionDef;
import org.apache.ranger.plugin.store.EmbeddedServiceDefsUtil;
import org.springframework.context.annotation.Scope;
import org.springframework.stereotype.Service;
import java.util.ArrayList;
+import java.util.HashMap;
import java.util.List;
import java.util.Map;
@Service
@Scope("singleton")
public class RangerServiceDefService extends
RangerServiceDefServiceBase<XXServiceDef, RangerServiceDef> {
+ public static final String
PROP_ENABLE_ACTION_MATCHER_IN_POLICIES_CONDITION =
"ranger.servicedef.ozone.enableActionMatcherInPoliciesCondition";
Review Comment:
`PROP_ENABLE_ACTION_MATCHER_IN_POLICIES_CONDITION` - rename this to include
Ozone in the name, to make it clear that this is about Ozone service-def.
##########
security-admin/src/main/java/org/apache/ranger/service/RangerServiceDefService.java:
##########
@@ -101,4 +109,112 @@ protected RangerServiceDef
mapEntityToViewBean(RangerServiceDef vObj, XXServiceD
return ret;
}
+
+ @Override
+ protected RangerServiceDef populateViewBean(XXServiceDef xServiceDef) {
+ final RangerServiceDef ret = super.populateViewBean(xServiceDef);
+
+ applyActionMatcherInPoliciesConditionHiddenOption(ret);
+
+ return ret;
+ }
+
+ void applyActionMatcherInPoliciesConditionHiddenOption(RangerServiceDef
serviceDef) {
+ if (serviceDef == null) {
+ return;
+ }
+
+ Map<String, String> serviceDefOptions = serviceDef.getOptions();
+
+ if (serviceDefOptions == null) {
+ serviceDefOptions = new HashMap<>();
+ serviceDef.setOptions(serviceDefOptions);
+ }
+
+ if (!StringUtils.equalsIgnoreCase(serviceDef.getName(),
EmbeddedServiceDefsUtil.EMBEDDED_SERVICEDEF_OZONE_NAME)) {
+
serviceDefOptions.put(OPTION_ENABLE_ACTION_MATCHER_IN_POLICIES_CONDITION,
Boolean.FALSE.toString());
Review Comment:
Automatically disabling `action-matcher` condition in all service-defs
doesn't make sense. This makes it impossible for a service-def to include this
custom condition. This doesn't make sense.
##########
plugin-ozone/src/main/java/org/apache/ranger/authorization/ozone/authorizer/RangerOzoneAuthorizer.java:
##########
@@ -168,7 +171,11 @@ public boolean checkAccess(IOzoneObj ozoneObject,
RequestContext context) {
rangerResource.setOwnerUser(context.getOwnerName());
rangerRequest.setResource(rangerResource);
rangerRequest.setAccessType(accessType);
- rangerRequest.setAction(context.getS3Action());
+ if (isOzoneActionPolicyEnabled(plugin)) {
Review Comment:
Is it necessary to set `action` field differently based on the feature flag?
I suggest setting it to `context.getS3Action()` irrespective of the feature
flag.
--
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]
