epugh commented on code in PR #4688:
URL: https://github.com/apache/solr/pull/4688#discussion_r3690702846


##########
solr/core/src/java/org/apache/solr/cli/AuthTool.java:
##########
@@ -147,201 +165,178 @@ public Options getOptions() {
         .addOptionGroup(getConnectionOptions());
   }
 
-  private void ensureArgumentIsValidBooleanIfPresent(CommandLine cli, Option 
option) {
-    if (cli.hasOption(option)) {
-      final String value = cli.getOptionValue(option);
-      if (!"true".equalsIgnoreCase(value) && !"false".equalsIgnoreCase(value)) 
{
-        echo(
-            "Argument ["
-                + option.getLongOpt()
-                + "] must be either true or false, but was ["
-                + value
-                + "]");
-        runtime.exit(1);
-      }
+  private void ensureArgumentIsValidBooleanIfPresent(String optionName, String 
value) {
+    if (value != null && !"true".equalsIgnoreCase(value) && 
!"false".equalsIgnoreCase(value)) {
+      echo("Argument [" + optionName + "] must be either true or false, but 
was [" + value + "]");
+      runtime.exit(1);
     }
   }
 
-  private void handleBasicAuth(CommandLine cli) throws Exception {
-    String cmd = cli.getArgs()[0];
-    boolean prompt = Boolean.parseBoolean(cli.getOptionValue(PROMPT_OPTION, 
"false"));
-    boolean updateIncludeFileOnly =
-        Boolean.parseBoolean(cli.getOptionValue(UPDATE_INCLUDE_FILE_OPTION, 
"false"));
+  private void handleCommand(String cmd, AuthParams params) throws Exception {
     switch (cmd) {
-      case "enable":
-        {
-          if (!prompt && !cli.hasOption(CommonCLIOptions.CREDENTIALS_OPTION)) {
-            CLIO.out("Option --credentials or --prompt is required with 
enable.");
-            runtime.exit(1);
-          } else if (!prompt
-              && (cli.getOptionValue(CommonCLIOptions.CREDENTIALS_OPTION) == 
null
-                  || 
!cli.getOptionValue(CommonCLIOptions.CREDENTIALS_OPTION).contains(":"))) {
-            CLIO.out("Option --credentials is not in correct format.");
-            runtime.exit(1);
-          }
-
-          String zkHost = null;
-
-          if (!updateIncludeFileOnly) {
-            try {
-              zkHost = CLIUtils.getZkHost(cli);
-            } catch (Exception ex) {
-              if (cli.hasOption(CommonCLIOptions.ZK_HOST_OPTION)) {
-                CLIO.out(
-                    "Couldn't get ZooKeeper host. Please make sure that 
ZooKeeper is running and the correct zk-host has been passed in.");
-              } else {
-                CLIO.out(
-                    "Couldn't get ZooKeeper host. Please make sure Solr is 
running in cloud mode, or a zk-host has been passed in.");
-              }
-              runtime.exit(1);
-            }
-            if (zkHost == null) {
-              if (cli.hasOption(CommonCLIOptions.ZK_HOST_OPTION)) {
-                CLIO.out(
-                    "Couldn't get ZooKeeper host. Please make sure that 
ZooKeeper is running and the correct zk-host has been passed in.");
-              } else {
-                CLIO.out(
-                    "Couldn't get ZooKeeper host. Please make sure Solr is 
running in cloud mode, or a zk-host has been passed in.");
-              }
-              runtime.exit(1);
-            }
-
-            // check if security is already enabled or not
-            try (SolrZkClient zkClient = CLIUtils.getSolrZkClient(cli, 
zkHost)) {
-              checkSecurityJsonExists(zkClient);
-            }
-          }
-
-          String username, password;
-          if (cli.hasOption(CommonCLIOptions.CREDENTIALS_OPTION)) {
-            String credentials = 
cli.getOptionValue(CommonCLIOptions.CREDENTIALS_OPTION);
-            username = credentials.split(":")[0];
-            password = credentials.split(":")[1];
-          } else {
-            Console console = System.console();
-            // keep prompting until they've entered a non-empty username & 
password
-            do {
-              username = console.readLine("Enter username: ");
-            } while (username == null || username.trim().isEmpty());
-            username = username.trim();
-
-            do {
-              password = new String(console.readPassword("Enter password: "));
-            } while (password.isEmpty());
-          }
-
-          if (username.equals(password)
-              && !EnvUtils.getPropertyAsBool(
-                  Sha256AuthenticationProvider.ALLOW_USER_AS_PASSWORD_PROP, 
false)) {
-            CLIO.err(
-                "Error: username and password must not be identical."
-                    + " This credential would never authenticate.");
-            runtime.exit(1);
-          }
-
-          String resourceName = "security.json";
-          final URL resource = 
SolrCore.class.getClassLoader().getResource(resourceName);
-          if (null == resource) {
-            throw new IllegalArgumentException("invalid resource name: " + 
resourceName);
-          }
-
-          ObjectMapper mapper = new ObjectMapper();
-          JsonNode securityJson1 = mapper.readTree(resource.openStream());
-          // Only override blockUnknown if explicitly passed; otherwise let 
the template decide
-          if (cli.hasOption(BLOCK_UNKNOWN_OPTION)) {
-            boolean blockUnknown = 
Boolean.parseBoolean(cli.getOptionValue(BLOCK_UNKNOWN_OPTION));
-            ((ObjectNode) 
securityJson1.get("authentication")).put("blockUnknown", blockUnknown);
-          }
-          JsonNode credentialsNode = 
securityJson1.get("authentication").get("credentials");
-          ((ObjectNode) credentialsNode)
-              .put(username, 
Sha256AuthenticationProvider.getSaltedHashedValue(password));
-          JsonNode userRoleNode = 
securityJson1.get("authorization").get("user-role");
-          String[] predefinedRoles = {"superadmin", "admin", "search", 
"index"};
-          ArrayNode rolesNode = mapper.createArrayNode();
-          for (String role : predefinedRoles) {
-            rolesNode.add(role);
-          }
-          ((ObjectNode) userRoleNode).set(username, rolesNode);
-          String securityJson = securityJson1.toPrettyString();
-
-          if (!updateIncludeFileOnly) {
-            echoIfVerbose("Uploading following security.json: " + 
securityJson);
-            try (SolrZkClient zkClient = CLIUtils.getSolrZkClient(cli, 
zkHost)) {
-              zkClient.makePath(
-                  "/security.json", 
securityJson.getBytes(StandardCharsets.UTF_8), false);
-            }
-          }
-
-          String solrIncludeFilename = 
cli.getOptionValue(SOLR_INCLUDE_FILE_OPTION);
-          Path includeFile = Path.of(solrIncludeFilename);
-          if (Files.notExists(includeFile) || !Files.isWritable(includeFile)) {
-            CLIO.out(
-                "Solr include file " + solrIncludeFilename + " doesn't exist 
or is not writeable.");
-            printAuthEnablingInstructions(username, password);
-            runtime.exit(0);
-          }
-          String authConfDir = cli.getOptionValue(AUTH_CONF_DIR_OPTION);
-          Path basicAuthConfFile = Path.of(authConfDir, "basicAuth.conf");
-
-          if (!Files.isWritable(basicAuthConfFile.getParent())) {
-            CLIO.out("Cannot write to file: " + 
basicAuthConfFile.toAbsolutePath());
-            printAuthEnablingInstructions(username, password);
-            runtime.exit(0);
-          }
-
-          Files.writeString(
-              basicAuthConfFile,
-              "httpBasicAuthUser=" + username + "\nhttpBasicAuthPassword=" + 
password,
-              StandardCharsets.UTF_8);
-
-          // update the solr.in.sh file to contain the necessary 
authentication lines
-          updateIncludeFileEnableAuth(includeFile, basicAuthConfFile);
-          final String successMessage =
-              String.format(
-                  Locale.ROOT,
-                  "Successfully enabled basic auth with username [%s] assigned 
to all roles (superadmin, admin, index, search).",
-                  username);
-          echo(successMessage);
-          if (!updateIncludeFileOnly) {
-            Map<String, String> templateUsers = new LinkedHashMap<>();
-            templateUsers.put("admin", "admin, index, search");
-            templateUsers.put("index", "index, search");
-            templateUsers.put("search", "search");
-            templateUsers.remove(username);
-            CLIO.out(
-                "\nIMPORTANT: The following template users have been created 
with NO password set"
-                    + " and cannot log in until passwords are assigned:");
-            templateUsers.forEach((u, roles) -> CLIO.out("  - " + u + "  
(roles: " + roles + ")"));
-            CLIO.out(
-                "Set their passwords using the Admin UI Security page or the 
authentication API.");
-          }
-          return;
-        }
-      case "disable":
-        {
-          clearSecurityJson(cli, updateIncludeFileOnly);
-
-          String solrIncludeFilename = 
cli.getOptionValue(SOLR_INCLUDE_FILE_OPTION);
-          Path includeFile = Path.of(solrIncludeFilename);
-          if (Files.notExists(includeFile) || !Files.isWritable(includeFile)) {
-            CLIO.out(
-                "Solr include file " + solrIncludeFilename + " doesn't exist 
or is not writeable.");
-            CLIO.out(
-                "Security has been disabled. Please remove any SOLR_AUTH_TYPE 
or SOLR_AUTHENTICATION_OPTS configuration from solr.in.sh/solr.in.cmd.\n");
-            runtime.exit(0);
-          }
-
-          // update the solr.in.sh file to comment out the necessary 
authentication lines
-          updateIncludeFileDisableAuth(includeFile);
-          return;
-        }
-      default:
+      case "enable" -> enableBasicAuth(params);

Review Comment:
   nice!



##########
solr/core/src/java/org/apache/solr/cli/AuthTool.java:
##########
@@ -103,6 +105,22 @@ public class AuthTool extends ToolBase {
               "This is where any authentication related configuration files, 
if any, would be placed.  Defaults to $SOLR_HOME.")
           .get();
 
+  /**
+   * Parameters for the auth sub-commands, independent of the command line 
parser.
+   *
+   * @param zkHost resolved ZooKeeper connection string, or null if {@code 
updateIncludeFileOnly} is
+   *     set or the connection string could not be resolved
+   */
+  record AuthParams(

Review Comment:
   nice!



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to