-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Adding schema access rules to file based system access control #15886
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -169,6 +169,125 @@ public void testSchemaOperations() | |
| })); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSchemaRulesForCheckCanCreateSchema() | ||
| { | ||
| TransactionManager transactionManager = createTestTransactionManager(); | ||
| AccessControlManager accessControlManager = newAccessControlManager(transactionManager, "file-based-system-access-schema.json"); | ||
|
|
||
| transaction(transactionManager, accessControlManager) | ||
| .execute(transactionId -> { | ||
| accessControlManager.checkCanCreateSchema(transactionId, bob, context, new CatalogSchemaName("alice-catalog", "bob")); | ||
| accessControlManager.checkCanCreateSchema(transactionId, bob, context, new CatalogSchemaName("bob-catalog", "bob")); | ||
| accessControlManager.checkCanCreateSchema(transactionId, admin, context, new CatalogSchemaName("some-catalog", "some-schema")); | ||
| accessControlManager.checkCanCreateSchema(transactionId, admin, context, new CatalogSchemaName("some-catalog", "bob")); | ||
| accessControlManager.checkCanCreateSchema(transactionId, admin, context, new CatalogSchemaName("some-catalog", "alice")); | ||
| }); | ||
|
|
||
| assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does one assertThrows have a bunch of different things in it? It will stop executing after the first one fails?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Made the changes.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are three cases now where there were five previously? It looks like it isn't validating whether the catalog can be accessed since coverage shows no cases where it is denied because the user can't access the catalog.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes look like those got removed while making other changes in test, added them back. |
||
| accessControlManager.checkCanCreateSchema(transactionId, bob, context, new CatalogSchemaName("alice-catalog", "alice")); | ||
| })); | ||
|
|
||
| assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { | ||
| accessControlManager.checkCanCreateSchema(transactionId, bob, context, new CatalogSchemaName("bob-catalog", "alice")); | ||
| })); | ||
|
|
||
| assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { | ||
| accessControlManager.checkCanCreateSchema(transactionId, bob, context, new CatalogSchemaName("secret-catalog", "secret")); | ||
| })); | ||
|
|
||
| assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { | ||
| accessControlManager.checkCanCreateSchema(transactionId, alice, context, new CatalogSchemaName("secret-catalog", "secret")); | ||
| })); | ||
|
|
||
| assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { | ||
| accessControlManager.checkCanCreateSchema(transactionId, admin, context, new CatalogSchemaName("secret-catalog", "secret")); | ||
| })); | ||
|
|
||
| assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { | ||
| accessControlManager.checkCanCreateSchema(transactionId, alice, context, new CatalogSchemaName("alice-catalog", "alice")); | ||
| })); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSchemaRulesForCheckCanDropSchema() | ||
| { | ||
| TransactionManager transactionManager = createTestTransactionManager(); | ||
| AccessControlManager accessControlManager = newAccessControlManager(transactionManager, "file-based-system-access-schema.json"); | ||
|
|
||
| transaction(transactionManager, accessControlManager) | ||
| .execute(transactionId -> { | ||
| accessControlManager.checkCanDropSchema(transactionId, bob, context, new CatalogSchemaName("alice-catalog", "bob")); | ||
| accessControlManager.checkCanDropSchema(transactionId, bob, context, new CatalogSchemaName("bob-catalog", "bob")); | ||
| accessControlManager.checkCanDropSchema(transactionId, admin, context, new CatalogSchemaName("some-catalog", "bob")); | ||
| accessControlManager.checkCanDropSchema(transactionId, admin, context, new CatalogSchemaName("some-catalog", "alice")); | ||
| accessControlManager.checkCanDropSchema(transactionId, admin, context, new CatalogSchemaName("some-catalog", "some-schema")); | ||
| }); | ||
|
|
||
| assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { | ||
| accessControlManager.checkCanDropSchema(transactionId, bob, context, new CatalogSchemaName("alice-catalog", "alice")); | ||
| })); | ||
|
|
||
| assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { | ||
| accessControlManager.checkCanDropSchema(transactionId, bob, context, new CatalogSchemaName("bob-catalog", "alice")); | ||
| })); | ||
|
|
||
| assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { | ||
| accessControlManager.checkCanDropSchema(transactionId, bob, context, new CatalogSchemaName("secret-catalog", "secret")); | ||
| })); | ||
|
|
||
| assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { | ||
| accessControlManager.checkCanDropSchema(transactionId, alice, context, new CatalogSchemaName("secret-catalog", "secret")); | ||
| })); | ||
|
|
||
| assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { | ||
| accessControlManager.checkCanDropSchema(transactionId, admin, context, new CatalogSchemaName("secret-catalog", "secret")); | ||
| })); | ||
|
|
||
| assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { | ||
| accessControlManager.checkCanDropSchema(transactionId, alice, context, new CatalogSchemaName("alice-catalog", "alice")); | ||
| })); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSchemaRulesForCheckCanRenameSchema() | ||
| { | ||
| TransactionManager transactionManager = createTestTransactionManager(); | ||
| AccessControlManager accessControlManager = newAccessControlManager(transactionManager, "file-based-system-access-schema.json"); | ||
|
|
||
| transaction(transactionManager, accessControlManager) | ||
| .execute(transactionId -> { | ||
| accessControlManager.checkCanRenameSchema(transactionId, bob, context, new CatalogSchemaName("alice-catalog", "bob"), "some-schema"); | ||
| accessControlManager.checkCanRenameSchema(transactionId, bob, context, new CatalogSchemaName("bob-catalog", "bob"), "some-schema"); | ||
| accessControlManager.checkCanRenameSchema(transactionId, admin, context, new CatalogSchemaName("some-catalog", "bob"), "new-schema-name"); | ||
| accessControlManager.checkCanRenameSchema(transactionId, admin, context, new CatalogSchemaName("some-catalog", "alice"), "new-schema-name"); | ||
| }); | ||
|
|
||
| assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { | ||
| accessControlManager.checkCanRenameSchema(transactionId, bob, context, new CatalogSchemaName("alice-catalog", "alice"), "new-schema-name"); | ||
| })); | ||
|
|
||
| assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { | ||
| accessControlManager.checkCanRenameSchema(transactionId, bob, context, new CatalogSchemaName("bob-catalog", "alice"), "new-schema-name"); | ||
| })); | ||
|
|
||
| assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { | ||
| accessControlManager.checkCanRenameSchema(transactionId, bob, context, new CatalogSchemaName("secret-catalog", "secret"), "new-schema-name"); | ||
| })); | ||
|
|
||
| assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { | ||
| accessControlManager.checkCanRenameSchema(transactionId, alice, context, new CatalogSchemaName("secret-catalog", "secret"), "new-schema-name"); | ||
| })); | ||
|
|
||
| assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { | ||
| accessControlManager.checkCanRenameSchema(transactionId, admin, context, new CatalogSchemaName("secret-catalog", "secret"), "new-schema-name"); | ||
| })); | ||
|
|
||
| assertThrows(AccessDeniedException.class, () -> transaction(transactionManager, accessControlManager).execute(transactionId -> { | ||
| accessControlManager.checkCanRenameSchema(transactionId, alice, context, new CatalogSchemaName("alice-catalog", "alice"), "new-schema-name"); | ||
| })); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSchemaOperationsReadOnly() | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| { | ||
| "catalogs": [ | ||
| { | ||
| "user": "alice", | ||
| "catalog": "alice-catalog", | ||
| "allow": "read-only" | ||
| }, | ||
| { | ||
| "allow": true | ||
| } | ||
| ], | ||
| "schemas": [ | ||
| { | ||
| "schema": "secret", | ||
| "owner": false | ||
| }, | ||
| { | ||
| "user": "admin", | ||
| "schema": ".*", | ||
| "owner": true | ||
| }, | ||
| { | ||
| "user": "bob", | ||
| "schema": "bob|some-schema", | ||
| "owner": true | ||
| }, | ||
| { | ||
| "user": "alice", | ||
| "schema": "alice", | ||
| "owner": true | ||
| } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,9 @@ | ||
| { | ||
| "schemas": [ | ||
| "sessionProperties": [ | ||
| { | ||
| "user": "(alice|bob)", | ||
| "schema": "(default|pv)", | ||
| "owner": true | ||
| "user": "admin", | ||
| "property": "max_split_size", | ||
| "allow": true | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We seem to deviate a lot on the tests? Are there "extra" tests that we don't need to cover? Can we do a better job of synchronizing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aweisberg I have tried covering all positive and negative tests for all the 3 schema operations. If you think something is missing here, I can add more tests to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see why we deviated in several ways like not supporting groups and thus not needing to test groups.
If you fix the assertThrows I think it will be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aweisberg I have made the required changes.