Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Return CATALOG/SCHEMA_NOT_FOUND for SHOW CREATE statements #23197

Closed
wants to merge 1 commit into from
+60 −1
Closed

Return CATALOG/SCHEMA_NOT_FOUND for SHOW CREATE statements #23197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions core/trino-main/src/main/java/io/trino/sql/rewrite/ShowQueriesRewrite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,10 @@ protected Node visitShowSchemas(ShowSchemas node, Void context)
}

String catalog = node.getCatalog().map(Identifier::getValue).orElseGet(() -> session.getCatalog().orElseThrow());
if (!metadata.catalogExists(session, catalog)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this change the security behavior in the case where io.trino.spi.security.SystemAccessControl#canAccessCatalog returns false?

I see that method is called from every method in AccessControlManager, so the below checkCanShowSchemas() would fail with Cannot access catalog if the user doesn't have access. After this change, would SHOW SCHEMAS start failing with CATALOG_NOT_FOUND instead of ACCESS_DENIED?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now the code is inconsistent in different SHOW CREATE cases (i.e. SHOW CREATE SCHEMA checks for schema existence. I agree that the SHOW CREATE statements shouldn't reveal whether relation exists (even if the user has access to it) so I think that in both cases - relation does not exist or user has no access to it, we should throw not found exception. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dain What do you think is the right behavior?

throw semanticException(CATALOG_NOT_FOUND, node, "Catalog '%s' does not exist", catalog);
}

accessControl.checkCanShowSchemas(session.toSecurityContext(), catalog);

Optional<Expression> predicate = Optional.empty();
Expand Down Expand Up @@ -525,6 +529,8 @@ protected Node visitShowCreate(ShowCreate node, Void context)
private Query showCreateMaterializedView(ShowCreate node)
{
QualifiedObjectName objectName = createQualifiedObjectName(session, node, node.getName());
verifySchemaCatalogExists(node, objectName);

Optional<MaterializedViewDefinition> viewDefinition = metadata.getMaterializedView(session, objectName);

if (viewDefinition.isEmpty()) {
Expand Down Expand Up @@ -567,6 +573,7 @@ private Query showCreateMaterializedView(ShowCreate node)
private Query showCreateView(ShowCreate node)
{
QualifiedObjectName objectName = createQualifiedObjectName(session, node, node.getName());
verifySchemaCatalogExists(node, objectName);

if (metadata.isMaterializedView(session, objectName)) {
throw semanticException(NOT_SUPPORTED, node, "Relation '%s' is a materialized view, not a view", objectName);
Expand Down Expand Up @@ -605,9 +612,22 @@ private Query showCreateView(ShowCreate node)
return singleValueQuery("Create View", sql);
}

private void verifySchemaCatalogExists(Node node, QualifiedObjectName objectName)
{
if (!metadata.catalogExists(session, objectName.catalogName())) {
throw semanticException(CATALOG_NOT_FOUND, node, "Catalog '%s' does not exist", objectName.catalogName());
}

CatalogSchemaName catalogSchemaName = new CatalogSchemaName(objectName.catalogName(), objectName.schemaName());
if (!metadata.schemaExists(session, catalogSchemaName)) {
throw semanticException(SCHEMA_NOT_FOUND, node, "Schema '%s' does not exist", catalogSchemaName);
}
}

private Query showCreateTable(ShowCreate node)
{
QualifiedObjectName objectName = createQualifiedObjectName(session, node, node.getName());
verifySchemaCatalogExists(node, objectName);

if (metadata.isMaterializedView(session, objectName)) {
throw semanticException(NOT_SUPPORTED, node, "Relation '%s' is a materialized view, not a table", objectName);
Expand Down Expand Up @@ -686,6 +706,7 @@ private Query showCreateSchema(ShowCreate node)
private Node showCreateFunction(ShowCreate node)
{
QualifiedObjectName functionName = qualifiedFunctionName(functionSchema, node, node.getName());
verifySchemaCatalogExists(node, functionName);

accessControl.checkCanShowCreateFunction(session.toSecurityContext(), functionName);

Expand Down
32 changes: 32 additions & 0 deletions core/trino-main/src/test/java/io/trino/sql/query/TestShowQueries.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@

import java.util.Optional;

import static io.trino.spi.StandardErrorCode.CATALOG_NOT_FOUND;
import static io.trino.spi.StandardErrorCode.SCHEMA_NOT_FOUND;
import static io.trino.spi.StandardErrorCode.TABLE_NOT_FOUND;
import static io.trino.spi.type.BigintType.BIGINT;
import static io.trino.testing.TestingSession.testSessionBuilder;
import static org.assertj.core.api.Assertions.assertThat;
Expand Down Expand Up @@ -66,6 +69,9 @@ public TestShowQueries()
.withListSchemaNames(session -> ImmutableList.of("mockschema"))
.withListTables((session, schemaName) -> ImmutableList.of("mockTable"))
.withGetTableHandle((session, schemaTableName) -> {
if (schemaTableName.getTableName().equals("nonexistingtable")) {
return null;
}
if (schemaTableName.getTableName().equals("mockview")) {
return null;
}
Expand Down Expand Up @@ -205,6 +211,32 @@ public void testShowColumnsWithLikeWithEscape()
.matches("VALUES (VARCHAR 'cola_', VARCHAR 'bigint' , VARCHAR '', VARCHAR '')");
}

@Test
public void testNonExistingRelations()
{
testNonExistingRelations("TABLE", "Table");
testNonExistingRelations("VIEW", "View");
testNonExistingRelations("MATERIALIZED VIEW", "Materialized view");
}

private void testNonExistingRelations(String relationType, String relationName)
{
assertThat(assertions.query("SHOW CREATE " + relationType + " missing.mockSchema.nonExistingTable"))
.failure()
.hasErrorCode(CATALOG_NOT_FOUND)
.hasMessage("line 1:1: Catalog 'missing' does not exist");

assertThat(assertions.query("SHOW CREATE " + relationType + " mock.missingschema.nonExistingTable"))
.failure()
.hasErrorCode(SCHEMA_NOT_FOUND)
.hasMessage("line 1:1: Schema 'mock.missingschema' does not exist");

assertThat(assertions.query("SHOW CREATE " + relationType + " mock.mockSchema.nonExistingTable"))
.failure()
.hasErrorCode(TABLE_NOT_FOUND)
.hasMessage("line 1:1: " + relationName + " 'mock.mockschema.nonexistingtable' does not exist");
}

@Test
public void testShowCreateViewWithProperties()
{
Expand Down
3 changes: 3 additions & 0 deletions plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,9 @@ protected static QueryRunner createHiveQueryRunner(HiveQueryRunner.Builder<?> bu
"hive_timestamp_nanos",
"hive",
ImmutableMap.of("hive.timestamp-precision", "NANOSECONDS"));

queryRunner.execute("CREATE SCHEMA hive.functions");

return queryRunner;
}

Expand Down
5 changes: 4 additions & 1 deletion plugin/trino-memory/src/test/java/io/trino/plugin/memory/TestMemoryConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public class TestMemoryConnectorTest
protected QueryRunner createQueryRunner()
throws Exception
{
return MemoryQueryRunner.builder()
QueryRunner runner = MemoryQueryRunner.builder()
.addExtraProperties(ImmutableMap.<String, String>builder()
// Adjust DF limits to test edge cases
.put("dynamic-filtering.small.max-distinct-values-per-driver", "100")
Expand All @@ -80,6 +80,9 @@ protected QueryRunner createQueryRunner()
.add(TpchTable.LINE_ITEM)
.build())
.build();

runner.execute("CREATE SCHEMA memory.functions");
return runner;
}

@Override
Expand Down
Loading