You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@phoenix.apache.org by karanmehta93 <gi...@git.apache.org> on 2018/01/14 01:15:52 UTC
[GitHub] phoenix pull request #289: PHOENIX-4528 PhoenixAccessController checks permi...
GitHub user karanmehta93 opened a pull request:
https://github.com/apache/phoenix/pull/289
PHOENIX-4528 PhoenixAccessController checks permissions only at table…
… level when creating views
@ankitsinghal @twdsilva Please review.
@ankitsinghal Please suggest new tests that can be added to verify this patch. The test that I added only verifies that create views would succeed. The change that I have made is generic, however it will be good to add tests that cover scenarios that include creation or dropping of index tables.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/karanmehta93/phoenix PHOENX-4528
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/phoenix/pull/289.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #289
----
commit 9490469b1f4977e1cca63558caad9472d6f2b2a8
Author: Karan Mehta <ka...@...>
Date: 2018-01-14T01:10:31Z
PHOENIX-4528 PhoenixAccessController checks permissions only at table level when creating views
----
---
[GitHub] phoenix pull request #289: PHOENIX-4528 PhoenixAccessController checks permi...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 closed the pull request at:
https://github.com/apache/phoenix/pull/289
---
[GitHub] phoenix issue #289: PHOENIX-4528 PhoenixAccessController checks permissions ...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on the issue:
https://github.com/apache/phoenix/pull/289
@karanmehta93
LGTM. If you want to add more test coverage for indexes you can parameterize TableDDLPermissionsIT.testIndexAndView which currently grants permissions directly on the table.
---
[GitHub] phoenix pull request #289: PHOENIX-4528 PhoenixAccessController checks permi...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/289#discussion_r161662712
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/ChangePermissionsIT.java ---
@@ -267,4 +267,26 @@ public void testMultiTenantTables() throws Exception {
verifyAllowed(readMultiTenantTableWithIndex(VIEW1_TABLE_NAME, "o1"), regularUser2);
verifyAllowed(readMultiTenantTableWithoutIndex(VIEW2_TABLE_NAME, "o2"), regularUser2);
}
+
+ /**
+ * Grant RX permissions on the schema to regularUser1,
+ * Creating view on a table with that schema by regularUser1 should be allowed
+ */
+ @Test
+ public void testCreateViewOnTableWithRXPermsOnSchema() throws Exception {
+
+ startNewMiniCluster();
+ grantSystemTableAccess(superUser1, regularUser1, regularUser2, regularUser3);
+
+ if(isNamespaceMapped) {
+ verifyAllowed(createSchema(SCHEMA_NAME), superUser1);
+ verifyAllowed(createTable(FULL_TABLE_NAME), superUser1);
+ verifyAllowed(grantPermissions("RX", regularUser1, SCHEMA_NAME, true), superUser1);
+ } else {
+ verifyAllowed(createTable(FULL_TABLE_NAME), superUser1);
+ verifyAllowed(grantPermissions("RX", regularUser1, surroundWithDoubleQuotes(SchemaUtil.SCHEMA_FOR_DEFAULT_NAMESPACE), true), superUser1);
+ }
+
+ verifyAllowed(createView(VIEW1_TABLE_NAME, FULL_TABLE_NAME), regularUser1);
+ }
--- End diff --
If the user has access on the SCHEMA of FULL_TABLE_NAME, that should be sufficient, since namespace is bigger in scope that per table scope. Hence I merge all these permissions and then use the `hasAccess()` method to determine the final access.
---
[GitHub] phoenix pull request #289: PHOENIX-4528 PhoenixAccessController checks permi...
Posted by ankitsinghal <gi...@git.apache.org>.
Github user ankitsinghal commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/289#discussion_r161661425
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/ChangePermissionsIT.java ---
@@ -267,4 +267,26 @@ public void testMultiTenantTables() throws Exception {
verifyAllowed(readMultiTenantTableWithIndex(VIEW1_TABLE_NAME, "o1"), regularUser2);
verifyAllowed(readMultiTenantTableWithoutIndex(VIEW2_TABLE_NAME, "o2"), regularUser2);
}
+
+ /**
+ * Grant RX permissions on the schema to regularUser1,
+ * Creating view on a table with that schema by regularUser1 should be allowed
+ */
+ @Test
+ public void testCreateViewOnTableWithRXPermsOnSchema() throws Exception {
+
+ startNewMiniCluster();
+ grantSystemTableAccess(superUser1, regularUser1, regularUser2, regularUser3);
+
+ if(isNamespaceMapped) {
+ verifyAllowed(createSchema(SCHEMA_NAME), superUser1);
+ verifyAllowed(createTable(FULL_TABLE_NAME), superUser1);
+ verifyAllowed(grantPermissions("RX", regularUser1, SCHEMA_NAME, true), superUser1);
+ } else {
+ verifyAllowed(createTable(FULL_TABLE_NAME), superUser1);
+ verifyAllowed(grantPermissions("RX", regularUser1, surroundWithDoubleQuotes(SchemaUtil.SCHEMA_FOR_DEFAULT_NAMESPACE), true), superUser1);
+ }
+
+ verifyAllowed(createView(VIEW1_TABLE_NAME, FULL_TABLE_NAME), regularUser1);
+ }
--- End diff --
regularUser should also have read access on FULL_TABLE_NAME to create view right?
---
[GitHub] phoenix pull request #289: PHOENIX-4528 PhoenixAccessController checks permi...
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/289#discussion_r161893623
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/ChangePermissionsIT.java ---
@@ -267,4 +267,26 @@ public void testMultiTenantTables() throws Exception {
verifyAllowed(readMultiTenantTableWithIndex(VIEW1_TABLE_NAME, "o1"), regularUser2);
verifyAllowed(readMultiTenantTableWithoutIndex(VIEW2_TABLE_NAME, "o2"), regularUser2);
}
+
+ /**
+ * Grant RX permissions on the schema to regularUser1,
+ * Creating view on a table with that schema by regularUser1 should be allowed
+ */
+ @Test
+ public void testCreateViewOnTableWithRXPermsOnSchema() throws Exception {
+
+ startNewMiniCluster();
+ grantSystemTableAccess(superUser1, regularUser1, regularUser2, regularUser3);
+
+ if(isNamespaceMapped) {
+ verifyAllowed(createSchema(SCHEMA_NAME), superUser1);
+ verifyAllowed(createTable(FULL_TABLE_NAME), superUser1);
+ verifyAllowed(grantPermissions("RX", regularUser1, SCHEMA_NAME, true), superUser1);
+ } else {
+ verifyAllowed(createTable(FULL_TABLE_NAME), superUser1);
+ verifyAllowed(grantPermissions("RX", regularUser1, surroundWithDoubleQuotes(SchemaUtil.SCHEMA_FOR_DEFAULT_NAMESPACE), true), superUser1);
+ }
+
+ verifyAllowed(createView(VIEW1_TABLE_NAME, FULL_TABLE_NAME), regularUser1);
+ }
--- End diff --
+1 Please file a separate JIRA to add a test for the index codepath.
---