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.

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

Added catalog and schema level access checks in USE statement #23882

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

annmegha
Copy link
Contributor

@annmegha annmegha commented Oct 24, 2024

Added catalog and schema level access checks when a user tries USE statement

Motivation and Context

A user with any role who has not been granted access to a specific catalog or schema is still able to execute the USE command. The expected behavior is that such users should receive an "Access Denied" error when attempting to access a catalog or schema for which they lack permission.
Refer : #23881

Test Plan

Added the related testcases to the code change

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Added catalog and schema level access checks in USE statement :pr:`23882 `

@annmegha annmegha requested a review from a team as a code owner October 24, 2024 08:51
Copy link

linux-foundation-easycla bot commented Oct 24, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: annmegha / name: Ann-Megha-Rajesh1 (1ed9830)

@tdcmeehan
Copy link
Contributor

What happens if the user tries to query Presto, but they set the default catalog to something they don't have permission for? For example, if they load up the Presto CLI with the --catalog flag to a catalog they do not have permissions to use--when do they receive an error?

@annmegha
Copy link
Contributor Author

annmegha commented Nov 6, 2024

@tdcmeehan , The moment the user runs a query involving the restricted catalog, Presto will attempt to access it, triggering a permissions check. If permissions are lacking, Presto immediately returns an error, something like Access Denied: Cannot access catalog <catalog_name>.

In the below example user "newuser" is restricted to catalog "tpcds"

image

@tdcmeehan
Copy link
Contributor

The commit message needs to be reworked. [Add UseTask.java class changes](https://github.com/prestodb/presto/pull/23882/commits/0686d0e8d542ef0c79cb72aea77f5cc997ff113d) doesn't describe the feature.

Perhaps just use the PR title as the commit message, but use the present tense and imperative mood.

@annmegha
Copy link
Contributor Author

annmegha commented Nov 6, 2024

I have updated the commit message. Please check

@tdcmeehan tdcmeehan merged commit aa67906 into prestodb:master Nov 6, 2024
56 checks passed
@ZacBlanco
Copy link
Contributor

ZacBlanco commented Nov 6, 2024

FYI I started seeing actions failures in presto-main once this PR merged:

Details

2024-11-06T17:47:09.2387433Z 2024-11-06T11:47:09.237-0600	INFO	TestNG-test=Surefire test-2	com.facebook.presto.storage.TempStorageManager	-- Loaded temp storage local --
2024-11-06T17:47:17.7455702Z [ERROR] Tests run: 8549, Failures: 2, Errors: 0, Skipped: 1, Time elapsed: 1,305.838 s <<< FAILURE! - in TestSuite
2024-11-06T17:47:17.7457560Z [ERROR] com.facebook.presto.execution.TestUseTask.testUseAccessDenied  Time elapsed: 0.465 s  <<< FAILURE!
2024-11-06T17:47:17.7460211Z com.facebook.presto.spi.PrestoException: Unknown transaction ID: b48d06df-15b5-423c-8ccc-354fc146892a. Possibly expired? Commands ignored until end of transaction block
2024-11-06T17:47:17.7462557Z 	at com.facebook.presto.transaction.InMemoryTransactionManager.unknownTransactionError(InMemoryTransactionManager.java:336)
2024-11-06T17:47:17.7464655Z 	at com.facebook.presto.transaction.InMemoryTransactionManager.getTransactionMetadata(InMemoryTransactionManager.java:300)
2024-11-06T17:47:17.7466743Z 	at com.facebook.presto.transaction.InMemoryTransactionManager.getOptionalCatalogMetadata(InMemoryTransactionManager.java:221)
2024-11-06T17:47:17.7468731Z 	at com.facebook.presto.metadata.MetadataManager.getCatalogHandle(MetadataManager.java:953)
2024-11-06T17:47:17.7470042Z 	at com.facebook.presto.execution.UseTask.checkAndSetCatalog(UseTask.java:111)
2024-11-06T17:47:17.7471127Z 	at com.facebook.presto.execution.UseTask.execute(UseTask.java:70)
2024-11-06T17:47:17.7472210Z 	at com.facebook.presto.execution.TestUseTask.executeUse(TestUseTask.java:167)
2024-11-06T17:47:17.7473524Z 	at com.facebook.presto.execution.TestUseTask.executeUse(TestUseTask.java:146)
2024-11-06T17:47:17.7474805Z 	at com.facebook.presto.execution.TestUseTask.testUseAccessDenied(TestUseTask.java:137)
2024-11-06T17:47:17.7475967Z 	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
2024-11-06T17:47:17.7477042Z 	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
2024-11-06T17:47:17.7478390Z 	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
2024-11-06T17:47:17.7479462Z 	at java.lang.reflect.Method.invoke(Method.java:498)
2024-11-06T17:47:17.7480645Z 	at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:135)
2024-11-06T17:47:17.7482051Z 	at org.testng.internal.invokers.TestInvoker.invokeMethod(TestInvoker.java:673)
2024-11-06T17:47:17.7483548Z 	at org.testng.internal.invokers.TestInvoker.invokeTestMethod(TestInvoker.java:220)
2024-11-06T17:47:17.7484794Z 	at org.testng.internal.invokers.MethodRunner.runInSequence(MethodRunner.java:50)
2024-11-06T17:47:17.7486119Z 	at org.testng.internal.invokers.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:945)
2024-11-06T17:47:17.7487484Z 	at org.testng.internal.invokers.TestInvoker.invokeTestMethods(TestInvoker.java:193)
2024-11-06T17:47:17.7488892Z 	at org.testng.internal.invokers.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
2024-11-06T17:47:17.7490241Z 	at org.testng.internal.invokers.TestMethodWorker.run(TestMethodWorker.java:128)
2024-11-06T17:47:17.7491465Z 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
2024-11-06T17:47:17.7492216Z 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
2024-11-06T17:47:17.7493076Z 	at java.lang.Thread.run(Thread.java:750)
2024-11-06T17:47:17.7493466Z 
2024-11-06T17:47:17.7494154Z [ERROR] com.facebook.presto.execution.TestUseTask.testUseInvalidSchema  Time elapsed: 0.428 s  <<< FAILURE!
2024-11-06T17:47:17.7495568Z java.lang.AssertionError: expected [NOT_FOUND:5] but found [UNKNOWN_TRANSACTION:20]
2024-11-06T17:47:17.7496422Z 	at org.testng.Assert.fail(Assert.java:110)
2024-11-06T17:47:17.7497044Z 	at org.testng.Assert.failNotEquals(Assert.java:1413)
2024-11-06T17:47:17.7497759Z 	at org.testng.Assert.assertEqualsImpl(Assert.java:149)
2024-11-06T17:47:17.7498461Z 	at org.testng.Assert.assertEquals(Assert.java:131)
2024-11-06T17:47:17.7499138Z 	at org.testng.Assert.assertEquals(Assert.java:643)
2024-11-06T17:47:17.7500151Z 	at com.facebook.presto.execution.TestUseTask.testUseInvalidSchema(TestUseTask.java:122)
2024-11-06T17:47:17.7501410Z 	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
2024-11-06T17:47:17.7502442Z 	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
2024-11-06T17:47:17.7503639Z 	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
2024-11-06T17:47:17.7504721Z 	at java.lang.reflect.Method.invoke(Method.java:498)
2024-11-06T17:47:17.7505781Z 	at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:135)
2024-11-06T17:47:17.7507049Z 	at org.testng.internal.invokers.TestInvoker.invokeMethod(TestInvoker.java:673)
2024-11-06T17:47:17.7508162Z 	at org.testng.internal.invokers.TestInvoker.invokeTestMethod(TestInvoker.java:220)
2024-11-06T17:47:17.7509347Z 	at org.testng.internal.invokers.MethodRunner.runInSequence(MethodRunner.java:50)
2024-11-06T17:47:17.7510553Z 	at org.testng.internal.invokers.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:945)
2024-11-06T17:47:17.7511959Z 	at org.testng.internal.invokers.TestInvoker.invokeTestMethods(TestInvoker.java:193)
2024-11-06T17:47:17.7513541Z 	at org.testng.internal.invokers.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
2024-11-06T17:47:17.7514964Z 	at org.testng.internal.invokers.TestMethodWorker.run(TestMethodWorker.java:128)
2024-11-06T17:47:17.7516150Z 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
2024-11-06T17:47:17.7517631Z 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
2024-11-06T17:47:17.7518588Z 	at java.lang.Thread.run(Thread.java:750)
2024-11-06T17:47:17.7519014Z 
2024-11-06T17:47:17.9209995Z [INFO] 
2024-11-06T17:47:17.9210446Z [INFO] Results:
2024-11-06T17:47:17.9210843Z [INFO] 
2024-11-06T17:47:17.9211187Z [ERROR] Failures: 
2024-11-06T17:47:17.9215084Z [ERROR]   TestUseTask.testUseAccessDenied:137->executeUse:146->executeUse:167 » Presto Unknown transaction ID: b48d06df-15b5-423c-8ccc-354fc146892a. Possibly expired? Commands ignored until end of transaction block
2024-11-06T17:47:17.9216625Z [ERROR]   TestUseTask.testUseInvalidSchema:122 expected [NOT_FOUND:5] but found [UNKNOWN_TRANSACTION:20]
2024-11-06T17:47:17.9217206Z [INFO] 
2024-11-06T17:47:17.9217516Z [ERROR] Tests run: 8549, Failures: 2, Errors: 0, Skipped: 1
2024-11-06T17:47:17.9217906Z [INFO] 
2024-11-06T17:47:17.9254299Z [INFO] ------------------------------------------------------------------------
2024-11-06T17:47:17.9255028Z [INFO] BUILD FAILURE
2024-11-06T17:47:17.9255750Z [INFO] ------------------------------------------------------------------------
2024-11-06T17:47:17.9263425Z [INFO] Total time:  21:51 min (Wall Clock)
2024-11-06T17:47:17.9264891Z [INFO] Finished at: 2024-11-06T17:47:17Z
2024-11-06T17:47:17.9265710Z [INFO] ------------------------------------------------------------------------

2024-11-06T17:47:17.9215084Z [ERROR]   TestUseTask.testUseAccessDenied:137->executeUse:146->executeUse:167 » Presto Unknown transaction ID: b48d06df-15b5-423c-8ccc-354fc146892a. Possibly expired? Commands ignored until end of transaction block
2024-11-06T17:47:17.9216625Z [ERROR]   TestUseTask.testUseInvalidSchema:122 expected [NOT_FOUND:5] but found [UNKNOWN_TRANSACTION:20]
2024-11-06T17:47:17.9217206Z [INFO] 
2024-11-06T17:47:17.9217516Z [ERROR] Tests run: 8549, Failures: 2, Errors: 0, Skipped: 1
2024-11-06T17:47:17.9217906Z [INFO] 
2024-11-06T17:47:17.9254299Z [INFO] ------------------------------------------------------------------------
2024-11-06T17:47:17.9255028Z [INFO] BUILD FAILURE
2024-11-06T17:47:17.9255750Z [INFO] ------------------------------------------------------------------------
2024-11-06T17:47:17.9263425Z [INFO] Total time:  21:51 min (Wall Clock)
2024-11-06T17:47:17.9264891Z [INFO] Finished at: 2024-11-06T17:47:17Z
2024-11-06T17:47:17.9265710Z [INFO] ------------------------------------------------------------------------

@annmegha could you investigate?

@tdcmeehan
Copy link
Contributor

Strangely, it seems this test wasn't even executed until after the merge. I've opened the revert at #23965.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants