Remove deprecated SystemAccessControl#checkCanExecuteQuery

It was deprecated in 445 when a replacement method was added; I guess 10 versions is long enough for a transition period.
trinodb · Nov 8, 2024 · 3f5df6f · 3f5df6f
1 parent 57e4c7f
commit 3f5df6f
Show file tree

Hide file tree

Showing 7 changed files with 10 additions and 33 deletions.
diff --git a/core/trino-spi/src/main/java/io/trino/spi/security/SystemAccessControl.java b/core/trino-spi/src/main/java/io/trino/spi/security/SystemAccessControl.java
@@ -127,26 +127,14 @@ default void checkCanSetUser(Optional<Principal> principal, String userName)
         denySetUser(principal, userName);
     }
 
-    /**
-     * Checks if identity can execute a query.
-     *
-     * @throws AccessDeniedException if not allowed
-     * @deprecated use {@link #checkCanExecuteQuery(Identity, QueryId)}
-     */
-    @Deprecated
-    default void checkCanExecuteQuery(Identity identity)
-    {
-        denyExecuteQuery();
-    }
-
     /**
      * Checks if identity can execute a query.
      *
      * @throws AccessDeniedException if not allowed
      */
     default void checkCanExecuteQuery(Identity identity, QueryId queryId)
     {
-        checkCanExecuteQuery(identity);
+        denyExecuteQuery();
     }
 
     /**

diff --git a/...ugin-toolkit/src/main/java/io/trino/plugin/base/security/AllowAllSystemAccessControl.java b/...ugin-toolkit/src/main/java/io/trino/plugin/base/security/AllowAllSystemAccessControl.java
@@ -80,9 +80,6 @@ public void checkCanReadSystemInformation(Identity identity) {}
     @Override
     public void checkCanWriteSystemInformation(Identity identity) {}
 
-    @Override
-    public void checkCanExecuteQuery(Identity identity) {}
-
     @Override
     public void checkCanExecuteQuery(Identity identity, QueryId queryId) {}
 

diff --git a/...gin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedSystemAccessControl.java b/...gin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedSystemAccessControl.java
@@ -304,19 +304,13 @@ public void checkCanSetUser(Optional<Principal> principal, String userName)
     }
 
     @Override
-    public void checkCanExecuteQuery(Identity identity)
+    public void checkCanExecuteQuery(Identity identity, QueryId queryId)
     {
         if (!canAccessQuery(identity, Optional.empty(), QueryAccessRule.AccessMode.EXECUTE)) {
             denyExecuteQuery();
         }
     }
 
-    @Override
-    public void checkCanExecuteQuery(Identity identity, QueryId queryId)
-    {
-        checkCanExecuteQuery(identity);
-    }
-
     @Override
     public void checkCanViewQueryOwnedBy(Identity identity, Identity queryOwner)
     {

diff --git a/...in-toolkit/src/main/java/io/trino/plugin/base/security/ForwardingSystemAccessControl.java b/...in-toolkit/src/main/java/io/trino/plugin/base/security/ForwardingSystemAccessControl.java
@@ -83,12 +83,6 @@ public void checkCanWriteSystemInformation(Identity identity)
         delegate().checkCanWriteSystemInformation(identity);
     }
 
-    @Override
-    public void checkCanExecuteQuery(Identity identity)
-    {
-        delegate().checkCanExecuteQuery(identity);
-    }
-
     @Override
     public void checkCanExecuteQuery(Identity identity, QueryId queryId)
     {

diff --git a/plugin/trino-opa/src/main/java/io/trino/plugin/opa/OpaAccessControl.java b/plugin/trino-opa/src/main/java/io/trino/plugin/opa/OpaAccessControl.java
@@ -31,6 +31,7 @@
 import io.trino.plugin.opa.schema.TrinoSchema;
 import io.trino.plugin.opa.schema.TrinoTable;
 import io.trino.plugin.opa.schema.TrinoUser;
+import io.trino.spi.QueryId;
 import io.trino.spi.connector.CatalogSchemaName;
 import io.trino.spi.connector.CatalogSchemaRoutineName;
 import io.trino.spi.connector.CatalogSchemaTableName;
@@ -115,7 +116,7 @@ public void checkCanSetUser(Optional<Principal> principal, String userName)
     {}
 
     @Override
-    public void checkCanExecuteQuery(Identity identity)
+    public void checkCanExecuteQuery(Identity identity, QueryId queryId)
     {
         opaHighLevelClient.queryAndEnforce(buildQueryContext(identity), "ExecuteQuery", AccessDeniedException::denyExecuteQuery);
     }

diff --git a/plugin/trino-opa/src/test/java/io/trino/plugin/opa/TestConstants.java b/plugin/trino-opa/src/test/java/io/trino/plugin/opa/TestConstants.java
@@ -17,6 +17,7 @@
 import io.opentelemetry.api.OpenTelemetry;
 import io.opentelemetry.api.trace.Tracer;
 import io.trino.execution.QueryIdGenerator;
+import io.trino.spi.QueryId;
 import io.trino.spi.connector.CatalogSchemaTableName;
 import io.trino.spi.security.Identity;
 import io.trino.spi.security.SystemAccessControlFactory;
@@ -59,6 +60,7 @@ private TestConstants() {}
     public static final URI OPA_ROW_FILTERING_URI = URI.create("http://my-row-filtering-uri/");
     public static final URI OPA_COLUMN_MASKING_URI = URI.create("http://my-column-masking-uri/");
     public static final Identity TEST_IDENTITY = Identity.forUser("source-user").withGroups(ImmutableSet.of("some-group")).build();
+    public static final QueryId TEST_QUERY_ID = QueryId.valueOf("abcde");
     public static final SystemSecurityContext TEST_SECURITY_CONTEXT = new SystemSecurityContext(TEST_IDENTITY, new QueryIdGenerator().createNextQueryId(), Instant.now());
     public static final CatalogSchemaTableName TEST_COLUMN_MASKING_TABLE_NAME = new CatalogSchemaTableName("some_catalog", "some_schema", "some_table");
 

diff --git a/plugin/trino-opa/src/test/java/io/trino/plugin/opa/TestOpaAccessControl.java b/plugin/trino-opa/src/test/java/io/trino/plugin/opa/TestOpaAccessControl.java
@@ -61,6 +61,7 @@
 import static io.trino.plugin.opa.TestConstants.SERVER_ERROR_RESPONSE;
 import static io.trino.plugin.opa.TestConstants.TEST_COLUMN_MASKING_TABLE_NAME;
 import static io.trino.plugin.opa.TestConstants.TEST_IDENTITY;
+import static io.trino.plugin.opa.TestConstants.TEST_QUERY_ID;
 import static io.trino.plugin.opa.TestConstants.TEST_SECURITY_CONTEXT;
 import static io.trino.plugin.opa.TestConstants.UNDEFINED_RESPONSE;
 import static io.trino.plugin.opa.TestConstants.columnMaskingOpaConfig;
@@ -93,13 +94,13 @@ public void testResponseHasExtraFields()
                         }\
                         """));
         OpaAccessControl authorizer = createOpaAuthorizer(simpleOpaConfig(), mockClient);
-        authorizer.checkCanExecuteQuery(TEST_IDENTITY);
+        authorizer.checkCanExecuteQuery(TEST_IDENTITY, TEST_QUERY_ID);
     }
 
     @Test
     public void testNoResourceAction()
     {
-        testNoResourceAction("ExecuteQuery", OpaAccessControl::checkCanExecuteQuery);
+        testNoResourceAction("ExecuteQuery", (opaAccessControl, identity) -> opaAccessControl.checkCanExecuteQuery(identity, TEST_QUERY_ID));
         testNoResourceAction("ReadSystemInformation", OpaAccessControl::checkCanReadSystemInformation);
         testNoResourceAction("WriteSystemInformation", OpaAccessControl::checkCanWriteSystemInformation);
     }
@@ -665,7 +666,7 @@ private void testRequestContextContentsForGivenTrinoVersion(Optional<SystemAcces
                 accessControlContext);
         Identity sampleIdentityWithGroups = Identity.forUser("test_user").withGroups(ImmutableSet.of("some_group")).build();
 
-        authorizer.checkCanExecuteQuery(sampleIdentityWithGroups);
+        authorizer.checkCanExecuteQuery(sampleIdentityWithGroups, TEST_QUERY_ID);
 
         String expectedRequest =
                 """