diff --git a/changelog/unreleased/SOLR-18010-security-json-corruption.yml b/changelog/unreleased/SOLR-18010-security-json-corruption.yml new file mode 100644 index 000000000000..251218632156 --- /dev/null +++ b/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 diff --git a/solr/core/src/java/org/apache/solr/handler/admin/SecurityConfHandlerLocal.java b/solr/core/src/java/org/apache/solr/handler/admin/SecurityConfHandlerLocal.java index 79d86c936f9a..a5d951968001 100644 --- a/solr/core/src/java/org/apache/solr/handler/admin/SecurityConfHandlerLocal.java +++ b/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 * diff --git a/solr/core/src/test/org/apache/solr/security/BasicAuthStandaloneTest.java b/solr/core/src/test/org/apache/solr/security/BasicAuthStandaloneTest.java index bdd263211837..35ff50dd0507 100644 --- a/solr/core/src/test/org/apache/solr/security/BasicAuthStandaloneTest.java +++ b/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> 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 {