[329832604] Fix password-related NPEs in Oracle, provide more descrip…

…tive errors (#369)

Fix password-related NPEs in Oracle, provide more descriptive errors (#369)

- introduce changes to how password prompts are handled by Dumper
- handle cases where incomplete authorization is provided to Oracle connectors
bug id: 329832604

dumper/app/src/main/java/com/google/edwmigration/dumper/application/dumper/ConnectorArguments.java

@@ -32,8 +32,8 @@
 import com.google.edwmigration.dumper.application.dumper.connector.Connector;
 import com.google.edwmigration.dumper.application.dumper.connector.ConnectorProperty;
 import com.google.edwmigration.dumper.application.dumper.connector.ConnectorPropertyWithDefault;
+import com.google.edwmigration.dumper.application.dumper.io.PasswordReader;
 import com.google.edwmigration.dumper.plugin.ext.jdk.annotation.Description;
-import java.io.Console;
 import java.io.File;
 import java.io.IOException;
 import java.io.PrintStream;
@@ -412,9 +412,7 @@ public class ConnectorArguments extends DefaultArguments {
 
   private ConnectorProperties connectorProperties;
 
-  // because of quoting of special characeters on command line... the -password is made optional,
-  // and if not given, asked at the console.
-  private String askedPassword;
+  private final PasswordReader passwordReader = new PasswordReader();
 
   public ConnectorArguments(@Nonnull String... args) throws IOException {
     super(args);
@@ -663,31 +661,36 @@ public String getUser() {
     return getOptions().valueOf(optionUser);
   }
 
-  // -password has optional argument, and if not provied
-  // should be asked from command line
-  @CheckForNull
-  public String getPassword() {
-    if (!getOptions().has(optionPass)) {
-      return null;
-    }
-    String pass = getOptions().valueOf(optionPass);
-    if (pass != null) {
-      return pass;
-    }
-    // Else need to ask & save.
-    if (askedPassword != null) {
-      return askedPassword;
+  @Nonnull
+  public String getUserOrFail() {
+    String user = getOptions().valueOf(optionUser);
+    if (user == null) {
+      throw new MetadataDumperUsageException(
+          "Required username was not provided. Please use the '--"
+              + OPT_USER
+              + "' flag to provide the username.");
     }
+    return user;
+  }
 
-    Console console = System.console();
-    if (console == null) {
-      LOG.info("Cannot prompt for password, Console not available");
-      return null;
+  /**
+   * Get a password depending on the --password flag.
+   *
+   * @return An empty optional if the --password flag is not provided. Otherwise, an optional
+   *     containing the result of getPasswordOrPrompt()
+   */
+  @Nonnull
+  public Optional<String> getPasswordIfFlagProvided() {
+    return optionallyWhen(getOptions().has(optionPass), this::getPasswordOrPrompt);
+  }
+
+  @Nonnull
+  public String getPasswordOrPrompt() {
+    String password = getOptions().valueOf(optionPass);
+    if (password != null) {
+      return password;
     } else {
-      console.printf("Password: ");
-      pass = new String(console.readPassword());
-      askedPassword = pass;
-      return askedPassword;
+      return passwordReader.getOrPrompt();
     }
   }
 

dumper/app/src/main/java/com/google/edwmigration/dumper/application/dumper/connector/AbstractJdbcConnector.java

@@ -170,6 +170,7 @@ protected SimpleDriverDataSource newSimpleDataSource(
     if (!driver.acceptsURL(url))
       throw new MetadataDumperUsageException(
           "JDBC driver " + driver + " does not accept URL " + url);
-    return new JdbcDataSource(driver, url, arguments.getUser(), arguments.getPassword());
+    String password = arguments.getPasswordIfFlagProvided().orElse(null);
+    return new JdbcDataSource(driver, url, arguments.getUser(), password);
   }
 }

dumper/app/src/main/java/com/google/edwmigration/dumper/application/dumper/connector/mysql/AbstractMysqlConnector.java

@@ -66,8 +66,8 @@ public Handle open(ConnectorArguments arguments) throws Exception {
 
     Driver driver =
         newDriver(arguments.getDriverPaths(), "com.mysql.jdbc.Driver", "org.mariadb.jdbc.Driver");
-    DataSource dataSource =
-        new SimpleDriverDataSource(driver, url, arguments.getUser(), arguments.getPassword());
+    String password = arguments.getPasswordIfFlagProvided().orElse(null);
+    DataSource dataSource = new SimpleDriverDataSource(driver, url, arguments.getUser(), password);
     return new JdbcHandle(dataSource);
   }
 }

dumper/app/src/main/java/com/google/edwmigration/dumper/application/dumper/connector/netezza/NetezzaMetadataConnector.java

@@ -317,8 +317,8 @@ public Handle open(ConnectorArguments arguments) throws Exception {
     }
 
     Driver driver = newDriver(arguments.getDriverPaths(), "org.netezza.Driver");
-    DataSource dataSource =
-        new SimpleDriverDataSource(driver, url, arguments.getUser(), arguments.getPassword());
+    String password = arguments.getPasswordIfFlagProvided().orElse(null);
+    DataSource dataSource = new SimpleDriverDataSource(driver, url, arguments.getUser(), password);
     return new JdbcHandle(dataSource);
   }
 

dumper/app/src/main/java/com/google/edwmigration/dumper/application/dumper/connector/oracle/AbstractOracleConnector.java

@@ -139,17 +139,18 @@ private boolean isOracleSid(ConnectorArguments arguments) throws MetadataDumperU
 
   @Nonnull
   @Override
-  public Handle open(ConnectorArguments arguments) throws Exception {
+  public Handle open(@Nonnull ConnectorArguments arguments) throws Exception {
     Driver driver = newDriver(arguments.getDriverPaths(), "oracle.jdbc.OracleDriver");
     DataSource dataSource =
         new SimpleDriverDataSource(driver, buildUrl(arguments), buildProperties(arguments));
     return new JdbcHandle(dataSource);
   }
 
-  private Properties buildProperties(ConnectorArguments arguments) {
+  @Nonnull
+  Properties buildProperties(@Nonnull ConnectorArguments arguments) {
     Properties properties = new Properties();
-    properties.setProperty("user", arguments.getUser());
-    properties.setProperty("password", arguments.getPassword());
+    properties.setProperty("user", arguments.getUserOrFail());
+    properties.setProperty("password", arguments.getPasswordOrPrompt());
     properties.setProperty(
         "useFetchSizeWithLongColumn",
         PropertyParser.getString(

dumper/app/src/main/java/com/google/edwmigration/dumper/application/dumper/connector/redshift/AbstractRedshiftConnector.java

@@ -34,6 +34,7 @@
 import java.time.ZoneOffset;
 import java.time.format.DateTimeFormatter;
 import java.util.List;
+import java.util.Optional;
 import javax.annotation.Nonnull;
 import javax.sql.DataSource;
 import org.slf4j.Logger;
@@ -139,6 +140,7 @@ private static String requireNonNull(String val, String msg) throws MetadataDump
   @Nonnull
   private String makeJdbcUrlPostgresql(ConnectorArguments arguments)
       throws MetadataDumperUsageException, UnsupportedEncodingException {
+    String password = arguments.getPasswordIfFlagProvided().orElse(null);
     return "jdbc:postgresql://"
         + requireNonNull(arguments.getHost(), "--host should be specified")
         + ":"
@@ -147,14 +149,15 @@ private String makeJdbcUrlPostgresql(ConnectorArguments arguments)
         + Iterables.getFirst(arguments.getDatabases(), "") //
         + new JdbcPropBuilder("?=&")
             .propOrWarn("user", arguments.getUser(), "--user must be specified")
-            .propOrWarn("password", arguments.getPassword(), "--password must be specified")
+            .propOrWarn("password", password, "--password must be specified")
             .prop("ssl", "true")
             .toJdbcPart();
   }
 
   @Nonnull
   private String makeJdbcUrlRedshiftSimple(ConnectorArguments arguments)
       throws MetadataDumperUsageException, UnsupportedEncodingException {
+    String password = arguments.getPasswordIfFlagProvided().orElse(null);
     return "jdbc:redshift://"
         + requireNonNull(arguments.getHost(), "--host should be specified")
         + ":"
@@ -163,7 +166,7 @@ private String makeJdbcUrlRedshiftSimple(ConnectorArguments arguments)
         + Iterables.getFirst(arguments.getDatabases(), "") //
         + new JdbcPropBuilder("?=&")
             .propOrWarn("UID", arguments.getUser(), "--user must be specified")
-            .propOrWarn("PWD", arguments.getPassword(), "--password must be specified")
+            .propOrWarn("PWD", password, "--password must be specified")
             .toJdbcPart();
   }
 
@@ -207,14 +210,15 @@ public Handle open(ConnectorArguments arguments) throws Exception {
     //        LOG.debug("DRIVER CAN IAM " + driver.acceptsURL("jdbc:redshift:iam://host/db"));
     //        LOG.debug("DRIVER CAN PG " + driver.acceptsURL("jdbc:postgresql://host/db"));
     String url = arguments.getUri();
+    Optional<String> password = arguments.getPasswordIfFlagProvided();
     if (url == null) {
       // driver can be pg or rs ( including rs-iam )
       // options can be username/passowrd , or iam secrets.
 
       boolean isDriverRedshift = driver.acceptsURL("jdbc:redshift:iam://host/db");
       // there may be no --iam options, in which case default ~/.aws/credentials to be used.
       // so this is the only reliable check
-      boolean isAuthenticationPassword = arguments.getPassword() != null;
+      boolean isAuthenticationPassword = password.isPresent();
 
       if (!isDriverRedshift) {
         if (!isAuthenticationPassword)
@@ -235,7 +239,7 @@ public Handle open(ConnectorArguments arguments) throws Exception {
     //   LogLevel 0/1
     LOG.trace("URI is " + url);
     DataSource dataSource =
-        new SimpleDriverDataSource(driver, url, arguments.getUser(), arguments.getPassword());
+        new SimpleDriverDataSource(driver, url, arguments.getUser(), password.orElse(null));
 
     return JdbcHandle.newPooledJdbcHandle(dataSource, arguments.getThreadPoolSize());
   }

dumper/app/src/main/java/com/google/edwmigration/dumper/application/dumper/connector/snowflake/AbstractSnowflakeConnector.java

@@ -96,8 +96,8 @@ public Handle open(@Nonnull ConnectorArguments arguments) throws Exception {
 
     Driver driver =
         newDriver(arguments.getDriverPaths(), "net.snowflake.client.jdbc.SnowflakeDriver");
-    DataSource dataSource =
-        new SimpleDriverDataSource(driver, url, arguments.getUser(), arguments.getPassword());
+    String password = arguments.getPasswordIfFlagProvided().orElse(null);
+    DataSource dataSource = new SimpleDriverDataSource(driver, url, arguments.getUser(), password);
     JdbcHandle jdbcHandle = new JdbcHandle(dataSource);
     setCurrentDatabase(databaseName, jdbcHandle.getJdbcTemplate());
     return jdbcHandle;

dumper/app/src/main/java/com/google/edwmigration/dumper/application/dumper/connector/sqlserver/SqlServerMetadataConnector.java

@@ -117,8 +117,8 @@ public Handle open(ConnectorArguments arguments) throws Exception {
     // LOG.info("Connecting to URL {}", url);
     Driver driver =
         newDriver(arguments.getDriverPaths(), "com.microsoft.sqlserver.jdbc.SQLServerDriver");
-    DataSource dataSource =
-        new SimpleDriverDataSource(driver, url, arguments.getUser(), arguments.getPassword());
+    String password = arguments.getPasswordIfFlagProvided().orElse(null);
+    DataSource dataSource = new SimpleDriverDataSource(driver, url, arguments.getUser(), password);
     return new JdbcHandle(dataSource);
   }
 }

dumper/app/src/main/java/com/google/edwmigration/dumper/application/dumper/connector/vertica/AbstractVerticaConnector.java

@@ -68,8 +68,8 @@ public Handle open(ConnectorArguments arguments) throws Exception {
     }
 
     Driver driver = newDriver(arguments.getDriverPaths(), "com.vertica.jdbc.Driver");
-    DataSource dataSource =
-        new SimpleDriverDataSource(driver, url, arguments.getUser(), arguments.getPassword());
+    String password = arguments.getPasswordIfFlagProvided().orElse(null);
+    DataSource dataSource = new SimpleDriverDataSource(driver, url, arguments.getUser(), password);
     return new JdbcHandle(dataSource);
   }
 }

dumper/app/src/main/java/com/google/edwmigration/dumper/application/dumper/io/PasswordReader.java

@@ -0,0 +1,50 @@
+/*
+ * Copyright 2022-2023 Google LLC
+ * Copyright 2013-2021 CompilerWorks
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.google.edwmigration.dumper.application.dumper.io;
+
+import com.google.edwmigration.dumper.application.dumper.MetadataDumperUsageException;
+import java.io.Console;
+import javax.annotation.Nonnull;
+
+/**
+ * Provides the functionality of reading a password from prompt.
+ *
+ * <p>Needed, because users may not want to provide the password on the command line. This might be
+ * for security reasons or due to issues caused by special characters.
+ */
+public class PasswordReader {
+
+  private boolean hasCachedValue = false;
+  @Nonnull private String cachedValue = "";
+
+  @Nonnull
+  public synchronized String getOrPrompt() {
+    if (hasCachedValue) {
+      return cachedValue;
+    }
+    Console console = System.console();
+    if (console == null) {
+      // user requested a prompt, but we can't even get a console, so let's just fail
+      throw new MetadataDumperUsageException(
+          "A password prompt was requested, but there is no console available.");
+    }
+    console.printf("Password: ");
+    cachedValue = new String(console.readPassword());
+    hasCachedValue = true;
+    return cachedValue;
+  }
+}

dumper/app/src/test/java/com/google/edwmigration/dumper/application/dumper/ConnectorArgumentsTest.java

@@ -17,6 +17,7 @@
 package com.google.edwmigration.dumper.application.dumper;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 
 import com.google.common.collect.ImmutableList;
@@ -32,7 +33,7 @@ public class ConnectorArgumentsTest {
   @Test
   public void getDatabases_success() throws IOException {
     ConnectorArguments arguments =
-        new ConnectorArguments(new String[] {"--connector", "teradata", "--database", "sample-db"});
+        new ConnectorArguments("--connector", "teradata", "--database", "sample-db");
 
     List<String> databaseNames = arguments.getDatabases();
 
@@ -41,7 +42,7 @@ public void getDatabases_success() throws IOException {
 
   @Test
   public void getDatabases_databaseOptionNotSpecified_success() throws IOException {
-    ConnectorArguments arguments = new ConnectorArguments(new String[] {"--connector", "teradata"});
+    ConnectorArguments arguments = new ConnectorArguments("--connector", "teradata");
 
     List<String> databaseNames = arguments.getDatabases();
 
@@ -51,7 +52,7 @@ public void getDatabases_databaseOptionNotSpecified_success() throws IOException
   @Test
   public void getDatabases_trimDatabaseNames() throws IOException {
     ConnectorArguments arguments =
-        new ConnectorArguments(new String[] {"--connector", "teradata", "--database", "db1, db2 "});
+        new ConnectorArguments("--connector", "teradata", "--database", "db1, db2 ");
 
     List<String> databaseNames = arguments.getDatabases();
 
@@ -61,11 +62,33 @@ public void getDatabases_trimDatabaseNames() throws IOException {
   @Test
   public void getDatabases_trimDatabaseNamesFilteringOutBlankStrings() throws IOException {
     ConnectorArguments arguments =
-        new ConnectorArguments(
-            new String[] {"--connector", "teradata", "--database", "db1, ,,, db2 "});
+        new ConnectorArguments("--connector", "teradata", "--database", "db1, ,,, db2 ");
 
     List<String> databaseNames = arguments.getDatabases();
 
     assertEquals(ImmutableList.of("db1", "db2"), databaseNames);
   }
+
+  @Test
+  public void getUserOrFail_noUserFlag_throwsException() throws IOException {
+    ConnectorArguments arguments = new ConnectorArguments("--connector", "abcABC123");
+
+    Exception exception =
+        assertThrows(MetadataDumperUsageException.class, arguments::getUserOrFail);
+
+    assertEquals(
+        "Required username was not provided. Please use the '--user' flag to provide the username.",
+        exception.getMessage());
+  }
+
+  @Test
+  public void getUserOrFail_success() throws IOException {
+    String expectedName = "admin456";
+    ConnectorArguments arguments =
+        new ConnectorArguments("--connector", "abcABC123", "--user", expectedName);
+
+    String actualName = arguments.getUserOrFail();
+
+    assertEquals(expectedName, actualName);
+  }
 }