DRILL-8554: Update Jackcess and Fix Linked Table Issue

contrib/format-access/README.md

@@ -11,6 +11,24 @@ Simply add the following to any Drill file system configuration.  Typically, MS
 }
 ```
 
+### Linked Tables
+An Access file can define tables that live in another database file, referenced by an absolute local path or a UNC
+share embedded in the file itself.  Drill refuses to follow those references by default: reading an untrusted file
+would otherwise make the Drillbit open a path of the file author's choosing, as the Drillbit service account.
+
+**Note:** Before Drill 1.23, linked tables were followed automatically.  Queries against a linked table now fail with a
+permission error unless an administrator enables them.  Listing the tables in a file (a query without `tableName`)
+still works: linked tables are shown with null dates and counts.
+
+If you trust every file that can be queried and need linked tables to work, an administrator can enable them by
+setting the following boot option in `drill-override.conf` on every Drillbit:
+
+```
+drill.exec.storage.msaccess.allow_linked_databases: true
+```
+
+This is deliberately not a format config or table-function option, so that a query user cannot turn it on.
+
 ## Schemas
 Drill will discover the schema automatically from the Access file.  The plugin does support schema provisioning for consistency, but is not recommended.
 

contrib/format-access/pom.xml

@@ -39,7 +39,7 @@
         <dependency>
             <groupId>com.healthmarketscience.jackcess</groupId>
             <artifactId>jackcess</artifactId>
-            <version>4.0.8</version>
+            <version>4.0.11</version>
             <exclusions>
                 <exclusion>
                     <groupId>commons-logging</groupId>

contrib/format-access/src/main/java/org/apache/drill/exec/store/msaccess/MSAccessBatchReader.java

@@ -24,6 +24,8 @@
 import com.healthmarketscience.jackcess.DatabaseBuilder;
 import com.healthmarketscience.jackcess.Row;
 import com.healthmarketscience.jackcess.Table;
+import com.healthmarketscience.jackcess.TableMetaData;
+import com.healthmarketscience.jackcess.util.LinkResolver;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.drill.common.AutoCloseables;
 import org.apache.drill.common.exceptions.CustomErrorContext;
@@ -58,13 +60,24 @@ public class MSAccessBatchReader implements ManagedReader {
 
   private static final Logger logger = LoggerFactory.getLogger(MSAccessBatchReader.class);
 
+  /**
+   * An Access file can name another database (a local path or a UNC share) as the source of a
+   * linked table.  Jackcess's default resolver would open it as the Drillbit service account, so
+   * refuse instead unless {@link MSAccessFormatPlugin#ALLOW_LINKED_DATABASES} is set.  Linked tables
+   * are normally caught earlier with a clearer error; this is the backstop.
+   */
+  private static final LinkResolver REJECT_LINKED_DATABASES = (linkerDb, linkeeFileName) -> {
+    throw new IOException("Refusing to open linked database referenced by this MS Access file: " + linkeeFileName);
+  };
+
   private final FileDescrip file;
   private final CustomErrorContext errorContext;
   private final RowSetLoader rowWriter;
   private final File tempDir;
   private final List<MSAccessColumn> columnList;
   private final MSAccessFormatConfig config;
   private final boolean metadataOnly;
+  private final boolean allowLinkedDatabases;
   private File tempFile;
   private Set<String> tableList;
   private Iterator<Row> rowIterator;
@@ -73,8 +86,10 @@ public class MSAccessBatchReader implements ManagedReader {
   private Table table;
   private Database db;
 
-  public MSAccessBatchReader(FileSchemaNegotiator negotiator, File tempDir, MSAccessFormatConfig config) {
+  public MSAccessBatchReader(FileSchemaNegotiator negotiator, File tempDir, MSAccessFormatConfig config,
+                             boolean allowLinkedDatabases) {
     this.tempDir = tempDir;
+    this.allowLinkedDatabases = allowLinkedDatabases;
     this.columnList = new ArrayList<>();
     this.config = config;
     this.file = negotiator.file();
@@ -123,22 +138,40 @@ private void buildSchema(FileSchemaNegotiator negotiator) {
   private TupleMetadata buildMetadataSchema(SchemaBuilder builder) {
     // Adds the table name
     builder.add("table", MinorType.VARCHAR);
-    builder.add("created_date", MinorType.TIMESTAMP);
-    builder.add("updated_date", MinorType.TIMESTAMP);
-    builder.add("row_count", MinorType.INT);
-    builder.add("col_count", MinorType.INT);
+    // Nullable because linked tables are listed without being opened, so these are unknown.
+    builder.addNullable("created_date", MinorType.TIMESTAMP);
+    builder.addNullable("updated_date", MinorType.TIMESTAMP);
+    builder.addNullable("row_count", MinorType.INT);
+    builder.addNullable("col_count", MinorType.INT);
     builder.addArray("columns", MinorType.VARCHAR);
 
     return builder.buildSchema();
   }
 
   private TupleMetadata buildSchemaFromTable(SchemaBuilder builder, String tableName) {
     try {
+      TableMetaData metaData = db.getTableMetaData(tableName);
+      if (metaData == null) {
+        deleteTempFile();
+        throw UserException.validationError()
+            .message("Table " + tableName + " not found.")
+            .addContext(errorContext)
+            .build(logger);
+      }
+      if (isBlockedLink(metaData)) {
+        deleteTempFile();
+        throw UserException.permissionError()
+            .message("Table " + tableName + " is a linked table that refers to another database (" +
+                metaData.getLinkedDbName() + "). Reading linked tables is disabled. An administrator can " +
+                "enable it by setting " + MSAccessFormatPlugin.ALLOW_LINKED_DATABASES + " to true in drill-override.conf.")
+            .addContext(errorContext)
+            .build(logger);
+      }
       table = db.getTable(tableName);
     } catch (IOException e) {
       deleteTempFile();
       throw UserException.dataReadError(e)
-          .message("Table " + config.getTableName() + " not found. " + e.getMessage())
+          .message("Error opening table " + tableName + ": " + e.getMessage())
           .addContext(errorContext)
           .build(logger);
     }
@@ -247,7 +280,12 @@ public boolean next() {
   private void openFile() {
     try {
       fsStream = file.fileSystem().openPossiblyCompressedStream(file.split().getPath());
-      db = DatabaseBuilder.open(convertInputStreamToFile(fsStream));
+      db = new DatabaseBuilder(convertInputStreamToFile(fsStream))
+          .setReadOnly(true)
+          .open();
+      if (!allowLinkedDatabases) {
+        db.setLinkResolver(REJECT_LINKED_DATABASES);
+      }
       tableList = db.getTableNames();
     } catch (IOException e) {
       deleteTempFile();
@@ -258,9 +296,21 @@ private void openFile() {
     }
   }
 
+  private boolean isBlockedLink(TableMetaData metaData) {
+    return metaData.isLinked() && !allowLinkedDatabases;
+  }
+
   private void processMetadataRow(String tableName) throws IOException {
     Table table;
     try {
+      TableMetaData metaData = db.getTableMetaData(tableName);
+      if (metaData != null && isBlockedLink(metaData)) {
+        // List the linked table without opening it, so the rest of the file can still be explored.
+        rowWriter.start();
+        rowWriter.scalar("table").setString(tableName);
+        rowWriter.save();
+        return;
+      }
       table = db.getTable(tableName);
     } catch (IOException e) {
       deleteTempFile();
@@ -274,7 +324,7 @@ private void processMetadataRow(String tableName) throws IOException {
     rowWriter.scalar("table").setString(tableName);
     LocalDateTime createdDate = table.getCreatedDate();
     rowWriter.scalar("created_date").setTimestamp(createdDate.toInstant(ZoneOffset.UTC));
-    LocalDateTime updatedDate = table.getCreatedDate();
+    LocalDateTime updatedDate = table.getUpdatedDate();
     rowWriter.scalar("updated_date").setTimestamp(updatedDate.toInstant(ZoneOffset.UTC));
     rowWriter.scalar("row_count").setInt(table.getRowCount());
     rowWriter.scalar("col_count").setInt(table.getColumnCount());

contrib/format-access/src/main/java/org/apache/drill/exec/store/msaccess/MSAccessFormatPlugin.java

@@ -39,20 +39,30 @@
 public class MSAccessFormatPlugin extends EasyFormatPlugin<MSAccessFormatConfig> {
 
   protected static final String DEFAULT_NAME = "msaccess";
+
+  /**
+   * Boot option that lets the reader follow linked tables to other database files. It is
+   * deliberately not part of {@link MSAccessFormatConfig}: every format config field can be
+   * overridden from a table function, which would let any query user turn the protection off.
+   */
+  public static final String ALLOW_LINKED_DATABASES = "drill.exec.storage.msaccess.allow_linked_databases";
   private final DrillbitContext context;
 
   private static class MSAccessReaderFactory extends FileReaderFactory {
 
     private final File tempDir;
     private final MSAccessFormatConfig config;
-    public MSAccessReaderFactory(File tempDir, MSAccessFormatConfig config) {
+    private final boolean allowLinkedDatabases;
+
+    public MSAccessReaderFactory(File tempDir, MSAccessFormatConfig config, boolean allowLinkedDatabases) {
       this.tempDir = tempDir;
       this.config = config;
+      this.allowLinkedDatabases = allowLinkedDatabases;
     }
 
     @Override
     public ManagedReader newReader(FileSchemaNegotiator negotiator) {
-      return new MSAccessBatchReader(negotiator, tempDir, config);
+      return new MSAccessBatchReader(negotiator, tempDir, config, allowLinkedDatabases);
     }
   }
 
@@ -82,7 +92,12 @@ private static EasyFormatConfig easyConfig(Configuration fsConf, MSAccessFormatC
   @Override
   protected void configureScan(FileScanLifecycleBuilder builder, EasySubScan scan) {
     builder.nullType(Types.optional(TypeProtos.MinorType.VARCHAR));
-    builder.readerFactory(new MSAccessReaderFactory(getTmpDir(), formatConfig));
+    builder.readerFactory(new MSAccessReaderFactory(getTmpDir(), formatConfig, allowLinkedDatabases()));
+  }
+
+  private boolean allowLinkedDatabases() {
+    DrillConfig config = context.getConfig();
+    return config.hasPath(ALLOW_LINKED_DATABASES) && config.getBoolean(ALLOW_LINKED_DATABASES);
   }
 
   /**

contrib/format-access/src/main/resources/drill-module.conf

@@ -22,3 +22,11 @@
 drill.classpath.scanning: {
   packages += "org.apache.drill.exec.store.msaccess"
 }
+
+drill.exec.storage.msaccess: {
+  # An Access file can define linked tables that point at another database file by an absolute
+  # local path or a UNC share. Following them makes the Drillbit open a path chosen by the file's
+  # author, as the Drillbit service account, so this is off by default. It is a boot option so
+  # that it can only be enabled by an administrator, not by a query.
+  allow_linked_databases: false
+}

contrib/format-access/src/test/java/org/apache/drill/exec/store/msaccess/TestMSAccessLinkedTables.java

@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you 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 org.apache.drill.exec.store.msaccess;
+
+import com.healthmarketscience.jackcess.ColumnBuilder;
+import com.healthmarketscience.jackcess.DataType;
+import com.healthmarketscience.jackcess.Database;
+import com.healthmarketscience.jackcess.DatabaseBuilder;
+import com.healthmarketscience.jackcess.Table;
+import com.healthmarketscience.jackcess.TableBuilder;
+import org.apache.drill.categories.RowSetTest;
+import org.apache.drill.common.types.TypeProtos.MinorType;
+import org.apache.drill.exec.physical.rowSet.RowSet;
+import org.apache.drill.exec.physical.rowSet.RowSetBuilder;
+import org.apache.drill.exec.record.metadata.SchemaBuilder;
+import org.apache.drill.exec.record.metadata.TupleMetadata;
+import org.apache.drill.test.ClusterFixture;
+import org.apache.drill.test.ClusterTest;
+import org.apache.drill.test.rowSet.RowSetComparison;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.io.File;
+
+/**
+ * Linked tables with {@link MSAccessFormatPlugin#ALLOW_LINKED_DATABASES} enabled by the administrator.
+ * The default (disabled) behaviour is covered in {@link TestMSAccessReader}.
+ */
+@Category(RowSetTest.class)
+public class TestMSAccessLinkedTables extends ClusterTest {
+
+  @BeforeClass
+  public static void setup() throws Exception {
+    ClusterTest.startCluster(ClusterFixture.builder(dirTestWatcher)
+        .configProperty(MSAccessFormatPlugin.ALLOW_LINKED_DATABASES, true));
+  }
+
+  @Test
+  public void testLinkedTableResolvedWhenAllowed() throws Exception {
+    File target = new File(dirTestWatcher.getRootDir(), "linked_target.mdb");
+    try (Database db = DatabaseBuilder.create(Database.FileFormat.V2003, target)) {
+      Table table = new TableBuilder("Table1")
+          .addColumn(new ColumnBuilder("name", DataType.TEXT))
+          .toTable(db);
+      table.addRow("from the linked database");
+    }
+
+    File linker = new File(dirTestWatcher.getRootDir(), "linker.mdb");
+    try (Database db = DatabaseBuilder.create(Database.FileFormat.V2003, linker)) {
+      db.createLinkedTable("Linked", target.getAbsolutePath(), "Table1");
+    }
+
+    String sql = "SELECT * FROM table(dfs.`" + linker.getName() + "` (type=> 'msaccess', tableName => 'Linked'))";
+    RowSet results = client.queryBuilder().sql(sql).rowSet();
+
+    TupleMetadata expectedSchema = new SchemaBuilder()
+        .addNullable("name", MinorType.VARCHAR)
+        .buildSchema();
+
+    RowSet expected = new RowSetBuilder(client.allocator(), expectedSchema)
+        .addRow("from the linked database")
+        .build();
+
+    new RowSetComparison(expected).verifyAndClearAll(results);
+  }
+}