add non-null to store non-default values when patching security config (

#1444) * add non-null to store non-default values when patching security config Signed-off-by: Nidhi Sridhar <srnidhi@amazon.com>
opensearch-project · Jan 7, 2022 · dc02fac · dc02fac
1 parent 6cb5afb
commit dc02fac
Show file tree

Hide file tree

Showing 6 changed files with 248 additions and 44 deletions.
diff --git a/src/main/java/org/opensearch/security/securityconf/impl/v6/ConfigV6.java b/src/main/java/org/opensearch/security/securityconf/impl/v6/ConfigV6.java
@@ -40,6 +40,7 @@
 import com.fasterxml.jackson.annotation.JsonAnyGetter;
 import com.fasterxml.jackson.annotation.JsonAnySetter;
 import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonInclude;
 import com.fasterxml.jackson.core.JsonProcessingException;
 import org.opensearch.security.DefaultObjectMapper;
 import org.opensearch.security.auth.internal.InternalAuthenticationBackend;
@@ -83,6 +84,7 @@ public String toString() {
 
     public static class Kibana {
 
+        @JsonInclude(JsonInclude.Include.NON_NULL)
         public boolean multitenancy_enabled = true;
         public String server_username = "kibanaserver";
         public String opendistro_role = null;
@@ -97,7 +99,7 @@ public String toString() {
 
 
     }
-    
+
     public static class Http {
         public boolean anonymous_auth_enabled = false;
         public Xff xff = new Xff();
@@ -108,7 +110,7 @@ public String toString() {
 
 
     }
-    
+
     public static class AuthFailureListeners {
         @JsonIgnore
         private final Map<String, AuthFailureListener> listeners = new HashMap<>();
@@ -125,7 +127,7 @@ public Map<String, AuthFailureListener> getListeners() {
 
 
     }
-    
+
     public static class AuthFailureListener {
         public String type;
         public String authentication_backend;
@@ -148,8 +150,9 @@ public String asJson() {
             }
         }
     }
-    
+
     public static class Xff {
+        @JsonInclude(JsonInclude.Include.NON_NULL)
         public boolean enabled = true;
         public String internalProxies = Pattern.compile(
                 "10\\.\\d{1,3}\\.\\d{1,3}\\.\\d{1,3}|" +
@@ -170,7 +173,7 @@ public String toString() {
 
 
     }
-    
+
     public static class Authc {
 
         @JsonIgnore
@@ -193,10 +196,13 @@ public String toString() {
 
 
     }
-    
+
     public static class AuthcDomain {
+        @JsonInclude(JsonInclude.Include.NON_NULL)
         public boolean http_enabled= true;
+        @JsonInclude(JsonInclude.Include.NON_NULL)
         public boolean transport_enabled= true;
+        @JsonInclude(JsonInclude.Include.NON_NULL)
         public boolean enabled= true;
         public int order = 0;
         public HttpAuthenticator http_authenticator = new HttpAuthenticator();
@@ -211,6 +217,7 @@ public String toString() {
     }
 
     public static class HttpAuthenticator {
+        @JsonInclude(JsonInclude.Include.NON_NULL)
         public boolean challenge = true;
         public String type;
         public Map<String, Object> config = Collections.emptyMap();
@@ -231,7 +238,7 @@ public String toString() {
 
 
     }
-    
+
     public static class AuthzBackend {
         public String type = "noop";
         public Map<String, Object> config = Collections.emptyMap();
@@ -252,7 +259,7 @@ public String toString() {
 
 
     }
-    
+
     public static class AuthcBackend {
         public String type = InternalAuthenticationBackend.class.getName();
         public Map<String, Object> config = Collections.emptyMap();
@@ -273,7 +280,7 @@ public String toString() {
 
 
     }
-    
+
     public static class Authz {
         @JsonIgnore
         private final Map<String, AuthzDomain> domains = new HashMap<>();
@@ -295,10 +302,13 @@ public String toString() {
 
 
     }
-    
+
     public static class AuthzDomain {
+        @JsonInclude(JsonInclude.Include.NON_NULL)
         public boolean http_enabled = true;
+        @JsonInclude(JsonInclude.Include.NON_NULL)
         public boolean transport_enabled = true;
+        @JsonInclude(JsonInclude.Include.NON_NULL)
         public boolean enabled = true;
         public AuthzBackend authorization_backend = new AuthzBackend();
         @Override

diff --git a/src/main/java/org/opensearch/security/securityconf/impl/v7/ConfigV7.java b/src/main/java/org/opensearch/security/securityconf/impl/v7/ConfigV7.java
@@ -40,6 +40,7 @@
 import com.fasterxml.jackson.annotation.JsonAnyGetter;
 import com.fasterxml.jackson.annotation.JsonAnySetter;
 import com.fasterxml.jackson.annotation.JsonIgnore;
+import com.fasterxml.jackson.annotation.JsonInclude;
 import com.fasterxml.jackson.core.JsonProcessingException;
 import org.opensearch.security.DefaultObjectMapper;
 import org.opensearch.security.auth.internal.InternalAuthenticationBackend;
@@ -120,6 +121,7 @@ public static class Dynamic {
         public Authz authz = new Authz();
         public AuthFailureListeners auth_failure_listeners = new AuthFailureListeners();
         public boolean do_not_fail_on_forbidden;
+        @JsonInclude(JsonInclude.Include.NON_NULL)
         public boolean multi_rolespan_enabled = true;
         public String hosts_resolver_mode = "ip-only";
         public String transport_userrname_attribute;
@@ -134,6 +136,7 @@ public String toString() {
 
     public static class Kibana {
 
+        @JsonInclude(JsonInclude.Include.NON_NULL)
         public boolean multitenancy_enabled = true;
         public String server_username = "kibanaserver";
         public String opendistro_role = null;
@@ -147,7 +150,7 @@ public String toString() {
 
 
     }
-    
+
     public static class Http {
         public boolean anonymous_auth_enabled = false;
         public Xff xff = new Xff();
@@ -158,7 +161,7 @@ public String toString() {
 
 
     }
-    
+
     public static class AuthFailureListeners {
         @JsonIgnore
         private final Map<String, AuthFailureListener> listeners = new HashMap<>();
@@ -175,7 +178,7 @@ public Map<String, AuthFailureListener> getListeners() {
 
 
     }
-    
+
     public static class AuthFailureListener {
         public String type;
         public String authentication_backend;
@@ -211,7 +214,7 @@ public String asJson() {
             }
         }
     }
-    
+
     public static class Xff {
         public boolean enabled = false;
         public String internalProxies = Pattern.compile(
@@ -230,7 +233,7 @@ public String toString() {
 
 
     }
-    
+
     public static class Authc {
 
         @JsonIgnore
@@ -254,10 +257,12 @@ public String toString() {
 
 
     }
-    
+
     public static class AuthcDomain {
 
+        @JsonInclude(JsonInclude.Include.NON_NULL)
         public boolean http_enabled= true;
+        @JsonInclude(JsonInclude.Include.NON_NULL)
         public boolean transport_enabled= true;
         //public boolean enabled= true;
         public int order = 0;
@@ -294,6 +299,7 @@ public String toString() {
     }
 
     public static class HttpAuthenticator {
+        @JsonInclude(JsonInclude.Include.NON_NULL)
         public boolean challenge = true;
         public String type;
         public Map<String, Object> config = Collections.emptyMap();
@@ -327,7 +333,7 @@ public String toString() {
 
 
     }
-    
+
     public static class AuthzBackend {
         public String type = "noop";
         public Map<String, Object> config = Collections.emptyMap();
@@ -365,7 +371,7 @@ public String toString() {
 
 
     }
-    
+
     public static class AuthcBackend {
         public String type = InternalAuthenticationBackend.class.getName();
         public Map<String, Object> config = Collections.emptyMap();
@@ -403,7 +409,7 @@ public String toString() {
 
 
     }
-    
+
     public static class Authz {
         @JsonIgnore
         private final Map<String, AuthzDomain> domains = new HashMap<>();
@@ -425,9 +431,11 @@ public String toString() {
 
 
     }
-    
+
     public static class AuthzDomain {
+        @JsonInclude(JsonInclude.Include.NON_NULL)
         public boolean http_enabled = true;
+        @JsonInclude(JsonInclude.Include.NON_NULL)
         public boolean transport_enabled = true;
         public AuthzBackend authorization_backend = new AuthzBackend();
         public String description;

diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiTest.java
@@ -23,6 +23,7 @@
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
 
+import org.opensearch.security.DefaultObjectMapper;
 import org.opensearch.security.support.ConfigConstants;
 import org.opensearch.security.test.helper.file.FileHelper;
 import org.opensearch.security.test.helper.rest.RestHelper.HttpResponse;
@@ -108,4 +109,36 @@ public void testSecurityConfigApiWrite() throws Exception {
         Assert.assertEquals(HttpStatus.SC_METHOD_NOT_ALLOWED, response.getStatusCode());
 
     }
+
+    @Test
+    public void testSecurityConfigForHTTPPatch() throws Exception {
+
+        Settings settings = Settings.builder().put(ConfigConstants.SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION, true).build();
+        setup(settings);
+
+        rh.keystore = "restapi/kirk-keystore.jks";
+        rh.sendAdminCertificate = true;
+
+        //non-default config
+        String updatedConfig = FileHelper.loadFile("restapi/securityconfig_nondefault.json");
+
+        //update config
+        HttpResponse response = rh.executePutRequest(ENDPOINT + "/securityconfig/config", updatedConfig, new Header[0]);
+        Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());
+
+        //make patch request
+        response = rh.executePatchRequest(ENDPOINT + "/securityconfig", "[{\"op\": \"add\",\"path\": \"/config/dynamic/do_not_fail_on_forbidden\",\"value\": \"false\"}]", new Header[0]);
+        Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());
+
+        //get config
+        response = rh.executeGetRequest(ENDPOINT + "/securityconfig", new Header[0]);
+        Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());
+
+        // verify configs are same
+        Assert.assertEquals(DefaultObjectMapper.readTree(updatedConfig), DefaultObjectMapper.readTree(response.getBody()).get("config"));
+
+
+    }
 }
+
+
diff --git a/src/test/java/org/opensearch/security/securityconf/impl/v6/ConfigV6Test.java b/src/test/java/org/opensearch/security/securityconf/impl/v6/ConfigV6Test.java
@@ -20,12 +20,7 @@ public static Iterable<Boolean> omitDefaults() {
     }
 
     public void assertEquals(ConfigV6.Kibana expected, JsonNode node) {
-        if (omitDefaults && !expected.multitenancy_enabled) {
-            // false (default) is not persisted
-            Assert.assertNull(node.get("multitenancy_enabled"));
-        } else {
-            Assert.assertEquals(expected.multitenancy_enabled, node.get("multitenancy_enabled").asBoolean());
-        }
+        Assert.assertEquals(expected.multitenancy_enabled, node.get("multitenancy_enabled").asBoolean());
         if (expected.server_username == null) {
             Assert.assertNull(node.get("server_username"));
         } else {
@@ -51,12 +46,7 @@ public void assertEquals(ConfigV6.Kibana expected, JsonNode node) {
     }
 
     private void assertEquals(ConfigV6.Kibana expected, ConfigV6.Kibana actual) {
-        if (omitDefaults && !expected.multitenancy_enabled) {
-            // BUG: false is omitted and is restored to default (which is true) instead of false
-            Assert.assertTrue(actual.multitenancy_enabled);
-        } else {
-            Assert.assertEquals(expected.multitenancy_enabled, actual.multitenancy_enabled);
-        }
+        Assert.assertEquals(expected.multitenancy_enabled, actual.multitenancy_enabled);
         if (expected.server_username == null) {
             // null is restored to default instead of null
             Assert.assertEquals(new ConfigV6.Kibana().server_username, actual.server_username);

diff --git a/src/test/java/org/opensearch/security/securityconf/impl/v7/ConfigV7Test.java b/src/test/java/org/opensearch/security/securityconf/impl/v7/ConfigV7Test.java
@@ -20,12 +20,7 @@ public static Iterable<Boolean> omitDefaults() {
     }
 
     public void assertEquals(ConfigV7.Kibana expected, JsonNode node) {
-        if (omitDefaults && !expected.multitenancy_enabled) {
-            // false (default) is not persisted
-            Assert.assertNull(node.get("multitenancy_enabled"));
-        } else {
-            Assert.assertEquals(expected.multitenancy_enabled, node.get("multitenancy_enabled").asBoolean());
-        }
+        Assert.assertEquals(expected.multitenancy_enabled, node.get("multitenancy_enabled").asBoolean());
         if (expected.server_username == null) {
             Assert.assertNull(node.get("server_username"));
         } else {
@@ -45,12 +40,7 @@ public void assertEquals(ConfigV7.Kibana expected, JsonNode node) {
     }
 
     private void assertEquals(ConfigV7.Kibana expected, ConfigV7.Kibana actual) {
-        if (omitDefaults && !expected.multitenancy_enabled) {
-            // BUG: false is omitted and is restored to default (which is true) instead of false
-            Assert.assertTrue(actual.multitenancy_enabled);
-        } else {
-            Assert.assertEquals(expected.multitenancy_enabled, actual.multitenancy_enabled);
-        }
+        Assert.assertEquals(expected.multitenancy_enabled, actual.multitenancy_enabled);
         if (expected.server_username == null) {
             // null is restored to default instead of null
             Assert.assertEquals(new ConfigV7.Kibana().server_username, actual.server_username);