[GitHub] [lucene-solr] madrob commented on a change in pull request #1471: Revised SOLR-14014 PR Against Master

[GitBox <gi...@apache.org>]

madrob commented on a change in pull request #1471:
URL: https://github.com/apache/lucene-solr/pull/1471#discussion_r418622731



##########
File path: solr/bin/solr.cmd
##########
@@ -1288,6 +1295,7 @@ REM '-OmitStackTraceInFastThrow' ensures stack traces in errors,
 REM users who don't care about useful error msgs can override in SOLR_OPTS with +OmitStackTraceInFastThrow
 set "START_OPTS=%START_OPTS% -XX:-OmitStackTraceInFastThrow"
 set START_OPTS=%START_OPTS% !GC_TUNE! %GC_LOG_OPTS%
+set START_OPTS=-DdisableAdminUI=%DISABLE_ADMIN_UI%

Review comment:
       This clobbers the previous START_OPTS

##########
File path: solr/core/src/java/org/apache/solr/servlet/LoadAdminUiServlet.java
##########
@@ -15,6 +15,13 @@
  * limitations under the License.
  */
 package org.apache.solr.servlet;
+import javax.servlet.http.HttpServletRequest;

Review comment:
       nit: did you mean to move these imports around?

##########
File path: solr/bin/solr
##########
@@ -2097,6 +2097,14 @@ else
   SECURITY_MANAGER_OPTS=()
 fi
 
+# Enable ADMIN UI by default, and give the option for users to disable it
+if [ "$SOLR_ADMIN_UI_DISABLED" == "true" ]; then
+  SOLR_ADMIN_UI="-DdisableAdminUI=true"
+  echo -e "ADMIN UI Disabled"
+else
+  SOLR_ADMIN_UI="-DdisableAdminUI=false"

Review comment:
       It doesn't look like this property is ever passed to the run command?

##########
File path: solr/core/src/java/org/apache/solr/servlet/LoadAdminUiServlet.java
##########
@@ -24,29 +31,26 @@
 import org.apache.solr.core.CoreContainer;
 import org.apache.solr.core.SolrCore;
 
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
-
-import java.io.IOException;
-import java.io.InputStream;
-import java.io.OutputStreamWriter;
-import java.io.Writer;
-import java.nio.charset.StandardCharsets;
-
 /**
  * A simple servlet to load the Solr Admin UI
  * 
  * @since solr 4.0
  */
 public final class LoadAdminUiServlet extends BaseSolrServlet {
 
+  // check system properties for whether or not admin UI is disabled, default is false
+  private static final boolean disabled = Boolean.parseBoolean(System.getProperty("disableAdminUI", "false"));
+
   @Override
-  public void doGet(HttpServletRequest _request,
-                    HttpServletResponse _response)
-      throws IOException {
+  public void doGet(HttpServletRequest _request, HttpServletResponse _response) throws IOException {
     HttpServletRequest request = SolrDispatchFilter.closeShield(_request, false);
     HttpServletResponse response = SolrDispatchFilter.closeShield(_response, false);
-    
+
+    if(disabled){

Review comment:
       Would this make sense to do before the close shield methods?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org