SOLR-18010: Fix security.json corruption from parallel edits

changelog/unreleased/SOLR-18010-security-json-corruption.yml

@@ -0,0 +1,7 @@
+title: Concurrent security edits in standalone mode, such as creating a role with permissions in the Admin UI, no longer corrupt security.json.
+type: fixed
+authors:
+  - name: Amrit Sarkar
+links:
+  - name: SOLR-18010
+    url: https://issues.apache.org/jira/browse/SOLR-18010

solr/core/src/java/org/apache/solr/handler/admin/SecurityConfHandlerLocal.java

@@ -28,6 +28,7 @@
 import org.apache.solr.common.util.CommandOperation;
 import org.apache.solr.common.util.Utils;
 import org.apache.solr.core.CoreContainer;
+import org.apache.solr.request.SolrQueryRequest;
 import org.apache.solr.response.SolrQueryResponse;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -36,12 +37,24 @@
 public class SecurityConfHandlerLocal extends SecurityConfHandler {
   private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
   protected Path securityJsonPath;
+  private final Object securityJsonLock = new Object();
 
   public SecurityConfHandlerLocal(CoreContainer coreContainer) {
     super(coreContainer);
     securityJsonPath = coreContainer.getSolrHome().resolve("security.json");
   }
 
+  /**
+   * Serializes requests, since a local file has no versioned write to detect concurrent edits the
+   * way ZooKeeper does. Reads are included so they never see a partially written file.
+   */
+  @Override
+  public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throws Exception {
+    synchronized (securityJsonLock) {
+      super.handleRequestBody(req, rsp);
+    }
+  }
+
   /**
    * Fetches security props from SOLR_HOME
    *

solr/core/src/test/org/apache/solr/security/BasicAuthStandaloneTest.java

@@ -23,10 +23,18 @@
 
 import java.io.IOException;
 import java.nio.file.Path;
+import java.util.ArrayList;
 import java.util.Base64;
+import java.util.List;
 import java.util.Map;
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
 import org.apache.solr.SolrTestCaseJ4;
 import org.apache.solr.common.params.MapSolrParams;
+import org.apache.solr.common.util.ExecutorUtil;
+import org.apache.solr.common.util.SolrNamedThreadFactory;
 import org.apache.solr.common.util.Utils;
 import org.apache.solr.embedded.JettySolrRunner;
 import org.apache.solr.handler.admin.SecurityConfHandler;
@@ -124,6 +132,53 @@ public void testBasicAuth() throws Exception {
     }
   }
 
+  @Test
+  public void testConcurrentEdits() throws Exception {
+    String authzPrefix = "/admin/authorization";
+    HttpClient httpClient = jetty.getSolrClient().getHttpClient();
+    String url = buildUrl(jetty.getLocalPort()) + authzPrefix;
+
+    securityConfHandler.persistConf(
+        new SecurityConfHandler.SecurityConfig()
+            .setData(Utils.fromJSONString(STD_CONF.replace("'", "\""))));
+    securityConfHandler.securityConfEdited();
+    verifySecurityStatus(httpClient, url, "authorization.enabled", "true", 20);
+
+    int numEdits = 20;
+    // Release all edits at once so they overlap, like the Admin UI's per-permission POSTs. The
+    // await timeout fails the test instead of hanging if the pool ever has fewer threads than
+    // numEdits.
+    CyclicBarrier barrier = new CyclicBarrier(numEdits);
+    ExecutorService executor =
+        ExecutorUtil.newMDCAwareFixedThreadPool(
+            numEdits, new SolrNamedThreadFactory("securityEdit"));
+    try {
+      List<Future<?>> futures = new ArrayList<>();
+      for (int i = 0; i < numEdits; i++) {
+        String command = "{'set-user-role': {'user" + i + "': ['dev']}}";
+        futures.add(
+            executor.submit(
+                () -> {
+                  barrier.await(30, TimeUnit.SECONDS);
+                  doHttpPost(httpClient, url, command, "solr", "SolrRocks");
+                  return null;
+                }));
+      }
+      for (Future<?> future : futures) {
+        future.get();
+      }
+    } finally {
+      ExecutorUtil.shutdownAndAwaitTermination(executor);
+    }
+
+    Map<?, ?> authz =
+        (Map<?, ?>) securityConfHandler.getSecurityConfig(false).getData().get("authorization");
+    Map<?, ?> userRoles = (Map<?, ?>) authz.get("user-role");
+    for (int i = 0; i < numEdits; i++) {
+      assertTrue("missing user" + i + " in " + userRoles, userRoles.containsKey("user" + i));
+    }
+  }
+
   static void doHttpPost(
       HttpClient cl, String url, String jsonCommand, String basicUser, String basicPass)
       throws IOException {