dsmiley commented on code in PR #3504: URL: https://github.com/apache/solr/pull/3504#discussion_r2296327489
########## solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java: ########## @@ -132,10 +133,10 @@ public void handleRequestBody(SolrQueryRequest req, SolrQueryResponse rsp) throw String httpMethod = (String) req.getContext().get("httpMethod"); Command command = new Command(req, rsp, httpMethod); if ("POST".equals(httpMethod)) { - if (configEditing_disabled || isImmutableConfigSet) { + if (!configEditingEnabled || isImmutableConfigSet) { final String reason = - configEditing_disabled - ? "due to " + CONFIGSET_EDITING_DISABLED_ARG + !configEditingEnabled + ? "due to " + CONFIGSET_EDITING_ENABLED_ARG : "because ConfigSet is immutable"; Review Comment: to my point -- even "immutable" is the word chosen to communicate this to the user ########## solr/core/src/java/org/apache/solr/handler/SolrConfigHandler.java: ########## @@ -102,9 +103,9 @@ public class SolrConfigHandler extends RequestHandlerBase implements SolrCoreAware, PermissionNameProvider { private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); - public static final String CONFIGSET_EDITING_DISABLED_ARG = "disable.configEdit"; - public static final boolean configEditing_disabled = - Boolean.getBoolean(CONFIGSET_EDITING_DISABLED_ARG); + public static final String CONFIGSET_EDITING_ENABLED_ARG = "solr.configset.edit.enabled"; Review Comment: I'm not sure we can equate "config editing" with "configset editing". Maybe. This flag controls the [Config API ](https://solr.apache.org/guide/solr/latest/configuration-guide/config-api.html) which is conceptually similar to but not identical to configset. It's a separate area of the code and documentation, so I think it's setting should be reflective of that. Perhaps `solr.api.config.immutable` or `solr.configset.configapi.immutable` In my example there, I suggest `immutable` instead of `edit` as I think that a more general word and is in fact how we display a message to the user below... ########## solr/solr-ref-guide/modules/configuration-guide/pages/configsets-api.adoc: ########## @@ -88,7 +88,7 @@ The output will look like: Upload a configset, which is sent as a zipped file. A single, non-zipped file can also be uploaded with the `filePath` parameter. -This functionality is enabled by default, but can be disabled via a runtime parameter `-Dconfigset.upload.enabled=false`. +This functionality is enabled by default, but can be disabled via a runtime parameter `-Dsolr.configset.upload.enabled=false`. Review Comment: Maybe `solr.configset.immutable` would be a more universal way to communicate.... immutability? Instead of "updload", "edit", .... Oh, I see this flag only controls upload while it's possible to delete and clone. @gerlowskija , shouldn't the underlying protection be about immutability generally instead of merely upload specifically? Just upload seems like half of a security measure. ########## solr/solrj/src/java/org/apache/solr/common/util/EnvUtils.java: ########## @@ -232,6 +232,14 @@ static synchronized void init( deprecatedKey, key); setProperty(key, String.valueOf(!Boolean.getBoolean(deprecatedKey))); + } else if (deprecatedKey.equals("disable.config.edit") + || deprecatedKey.equals("disable.v2.api")) { Review Comment: why do these need to be called out specifically as a special case here? I thought you had a general mechanism, wherein you need to only list these names in a file. -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org