[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10135 ) Change subject: IMPALA-6802 (part 1): Clean up authorization tests .. IMPALA-6802 (part 1): Clean up authorization tests The first patch of this patch is to introduce a new mechanism of testing authorization that tests authorization at every hierarchy. This patch rewrites the authorization tests for select statements. Testing: - Added new authorization tests - Ran all front-end tests Cherry-picks: not for 2.x Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b Reviewed-on: http://gerrit.cloudera.org:8080/10135 Reviewed-by: Alex Behm Tested-by: Impala Public Jenkins --- A fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java 2 files changed, 659 insertions(+), 3 deletions(-) Approvals: Alex Behm: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/10135 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b Gerrit-Change-Number: 10135 Gerrit-PatchSet: 13 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 ) Change subject: IMPALA-6802 (part 1): Clean up authorization tests .. Patch Set 12: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/10135 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b Gerrit-Change-Number: 10135 Gerrit-PatchSet: 12 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Tue, 01 May 2018 02:53:41 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 ) Change subject: IMPALA-6802 (part 1): Clean up authorization tests .. Patch Set 12: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2386/ -- To view, visit http://gerrit.cloudera.org:8080/10135 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b Gerrit-Change-Number: 10135 Gerrit-PatchSet: 12 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Mon, 30 Apr 2018 23:00:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 ) Change subject: IMPALA-6802 (part 1): Clean up authorization tests .. Patch Set 12: Code-Review+2 Very nice. Let's keep the cleanup going! -- To view, visit http://gerrit.cloudera.org:8080/10135 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b Gerrit-Change-Number: 10135 Gerrit-PatchSet: 12 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Fredy Wijaya Gerrit-Comment-Date: Mon, 30 Apr 2018 22:59:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
..
Patch Set 12:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10135/11/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10135/11/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@94
PS11, Line 94: Set expectedPrivileges = Sets.newHashSet(
> You can create this set once and save many lines
Done
http://gerrit.cloudera.org:8080/#/c/10135/11/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@195
PS11, Line 195: allExcept(TPrivilegeLevel.ALL,
TPrivilegeLevel.SELECT)))
> create expected set once
Done
http://gerrit.cloudera.org:8080/#/c/10135/11/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@209
PS11, Line 209:
.error(selectError("functional_seq_snap.subquery_view"), onServer(
> fix indentation
Done
http://gerrit.cloudera.org:8080/#/c/10135/11/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@271
PS11, Line 271: .ok(onServer(TPrivilegeLevel.SELECT))
> factor out the err msg here and elsewhere to condense the tests
Done
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 12
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Comment-Date: Mon, 30 Apr 2018 22:47:45 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Fredy Wijaya has uploaded a new patch set (#12). ( http://gerrit.cloudera.org:8080/10135 ) Change subject: IMPALA-6802 (part 1): Clean up authorization tests .. IMPALA-6802 (part 1): Clean up authorization tests The first patch of this patch is to introduce a new mechanism of testing authorization that tests authorization at every hierarchy. This patch rewrites the authorization tests for select statements. Testing: - Added new authorization tests - Ran all front-end tests Cherry-picks: not for 2.x Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b --- A fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java 2 files changed, 659 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/10135/12 -- To view, visit http://gerrit.cloudera.org:8080/10135 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b Gerrit-Change-Number: 10135 Gerrit-PatchSet: 12 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Fredy Wijaya
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Alex Behm has posted comments on this change. (
http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
..
Patch Set 11:
(4 comments)
Thanks! Almost there
http://gerrit.cloudera.org:8080/#/c/10135/11/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10135/11/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@94
PS11, Line 94: verifyPrivilegeReqs("select * from functional.alltypes",
Sets.newHashSet(
You can create this set once and save many lines
http://gerrit.cloudera.org:8080/#/c/10135/11/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@195
PS11, Line 195: ));
create expected set once
http://gerrit.cloudera.org:8080/#/c/10135/11/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@209
PS11, Line 209: authorize("select id from functional.alltypes")
fix indentation
http://gerrit.cloudera.org:8080/#/c/10135/11/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@271
PS11, Line 271: .error("User '%s' does not have privileges to execute
'SELECT' on: " +
factor out the err msg here and elsewhere to condense the tests
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 11
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Comment-Date: Mon, 30 Apr 2018 22:21:37 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Fredy Wijaya has uploaded a new patch set (#11). ( http://gerrit.cloudera.org:8080/10135 ) Change subject: IMPALA-6802 (part 1): Clean up authorization tests .. IMPALA-6802 (part 1): Clean up authorization tests The first patch of this patch is to introduce a new mechanism of testing authorization that tests authorization at every hierarchy. This patch rewrites the authorization tests for select statements. Testing: - Added new authorization tests - Ran all front-end tests Cherry-picks: not for 2.x Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b --- A fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java 2 files changed, 770 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/10135/11 -- To view, visit http://gerrit.cloudera.org:8080/10135 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b Gerrit-Change-Number: 10135 Gerrit-PatchSet: 11 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Fredy Wijaya
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 ) Change subject: IMPALA-6802 (part 1): Clean up authorization tests .. Patch Set 11: (2 comments) http://gerrit.cloudera.org:8080/#/c/10135/8/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java: http://gerrit.cloudera.org:8080/#/c/10135/8/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java@654 PS8, Line 654: > I think these should go into AuthorizationTestV2 Done http://gerrit.cloudera.org:8080/#/c/10135/8/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java@671 PS8, Line 671: > we should also test a "use functional" and referencing "alltypes" Done -- To view, visit http://gerrit.cloudera.org:8080/10135 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b Gerrit-Change-Number: 10135 Gerrit-PatchSet: 11 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Fredy Wijaya Gerrit-Comment-Date: Mon, 30 Apr 2018 22:16:17 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Alex Behm has posted comments on this change. (
http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
..
Patch Set 8:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10135/8/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java:
http://gerrit.cloudera.org:8080/#/c/10135/8/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java@654
PS8, Line 654: public void TestPrivilegeRequests() throws ImpalaException {
I think these should go into AuthorizationTestV2
http://gerrit.cloudera.org:8080/#/c/10135/8/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java@671
PS8, Line 671: verifyPrivilegeReqs("select alltypes.* from
functional.alltypes", Sets.newHashSet(
we should also test a "use functional" and referencing "alltypes"
http://gerrit.cloudera.org:8080/#/c/10135/8/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10135/8/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@89
PS8, Line 89: // Test qualified and unqualified names.
We've already tested those elsewhere, so no need.
http://gerrit.cloudera.org:8080/#/c/10135/8/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@143
PS8, Line 143: // Unqualified table name.
already tested elsewhere
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Comment-Date: Mon, 30 Apr 2018 21:41:18 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Fredy Wijaya has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/10135 ) Change subject: IMPALA-6802 (part 1): Clean up authorization tests .. IMPALA-6802 (part 1): Clean up authorization tests The first patch of this patch is to introduce a new mechanism of testing authorization that tests authorization at every hierarchy. This patch rewrites the authorization tests for select statements. Testing: - Added new authorization tests - Ran all front-end tests Cherry-picks: not for 2.x Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b --- M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java A fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java 3 files changed, 727 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/10135/8 -- To view, visit http://gerrit.cloudera.org:8080/10135 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b Gerrit-Change-Number: 10135 Gerrit-PatchSet: 8 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Fredy Wijaya
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
..
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@349
PS5, Line 349: .ok(onColumn("functional", "alltypes", new
String[]{"id", "bool_col",
> In my mind the auth tests should check whether the authorization rules are
Done. I created a separate test in the AnalyzerTest to test solely on different
SQL variants.
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Comment-Date: Fri, 27 Apr 2018 18:19:50 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Alex Behm has posted comments on this change. (
http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
..
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@47
PS5, Line 47: import static org.junit.Assert.fail;
> Done. Avoiding calling catalog_.reset() and making Sentry RPCs makes the wh
That's more like it! Fantastic.
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@349
PS5, Line 349: "functional.alltypes", onTable("functional",
"alltypes", allExcept(
> Shouldn't this be a separate test case since this is to test a table with a
In my mind the auth tests should check whether the authorization rules are
applied correctly.
A table reference with implicit or explicit aliases still refers to the same
underlying table,
so it's a variant of the same test, but not really a different auth test.
Ideally, we should separate testing different SQL variants of referencing the
same underlying entity
from the auth tests themselves.
For exampe, we could have a single test that asserts the following things
generate the same
PrivilegeRequests during analysis.
- table reference with implicit/explicit aliases
- qualified/unqualified table references
- qualified/unqualified column references
- qualified/unqualified star
Then when we do the auth tests, we can focus on whatever variant is most
convenient.
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Comment-Date: Fri, 27 Apr 2018 16:51:01 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Fredy Wijaya has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/10135 ) Change subject: IMPALA-6802 (part 1): Clean up authorization tests .. IMPALA-6802 (part 1): Clean up authorization tests The first patch of this patch is to introduce a new mechanism of testing authorization that tests authorization at every hierarchy. This patch rewrites the authorization tests for select statements. Testing: - Added new authorization tests - Ran all front-end tests Cherry-picks: not for 2.x Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b --- A fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java 2 files changed, 715 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/10135/6 -- To view, visit http://gerrit.cloudera.org:8080/10135 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b Gerrit-Change-Number: 10135 Gerrit-PatchSet: 6 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Fredy Wijaya
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
..
Patch Set 6:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@47
PS5, Line 47: import static org.junit.Assert.fail;
> I tried running the test and it feels very slow. Do you know what's making
Done. Avoiding calling catalog_.reset() and making Sentry RPCs makes the whole
tests faster in a significant way.
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@75
PS5, Line 75:
> Why do we need this?
To ensure we have existing roles created externally will not interfere with
these tests. I'll add some comment in the code.
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@122
PS5, Line 122: // Select a specific column on a table.
> What does this mean? My understanding is that columns can only have SELECT,
Done. It's allowed in Sentry but not in Impala.
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@157
PS5, Line 157: .error("User '%s' does not have privileges to execute
'SELECT' on: " +
> add comment that column-level privs are currently not supported
Done
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@183
PS5, Line 183: TPrivilegeLevel.ALL, TPrivilegeLevel.SELECT)))
> I feel like there's still a lot of redundancy in these tests, could we perh
Done
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@324
PS5, Line 324: "functional.alltypes", onColumn("functional",
"alltypes", "id", allExcept(
> also very similar to the other test blocks, can we condense them further?
Done
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@349
PS5, Line 349: "functional.alltypes", onTable("functional",
"alltypes", allExcept(
> condense further?
Shouldn't this be a separate test case since this is to test a table with an
alias?
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@451
PS5, Line 451: "month"}, allExcept(TPrivilegeLevel.ALL,
TPrivilegeLevel.SELECT)));
> I feel like these tests could be structured a little more. Not sure if you
Done. I restructured the tests. Let me know what you think.
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Comment-Date: Thu, 26 Apr 2018 02:47:05 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Alex Behm has posted comments on this change. (
http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
..
Patch Set 5:
(8 comments)
Overall the new mechanism seems fine to me. Main comments:
* test execution speed needs to be improved
* still a lot of redundancy in the tests
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@47
PS5, Line 47: public class AuthorizationTestV2 extends FrontendTestBase {
I tried running the test and it feels very slow. Do you know what's making it
slow? Might be good to address the low hanging perf fruit if we can.
At this execution speed this new test might ultimately take tens of minutes if
we add coverage for the other statements.
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@75
PS5, Line 75: public void before() throws ImpalaException {
Why do we need this?
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@122
PS5, Line 122: .ok(onColumn("functional", "alltypes", "id",
TPrivilegeLevel.ALL))
What does this mean? My understanding is that columns can only have SELECT, so
this should be illegal.
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@157
PS5, Line 157: allExcept(TPrivilegeLevel.ALL,
TPrivilegeLevel.SELECT)));
add comment that column-level privs are currently not supported
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@183
PS5, Line 183: authorize(createAnalysisCtx("functional"), "select id from
alltypes")
I feel like there's still a lot of redundancy in these tests, could we perhaps
eliminate some of it?
These ~20 lines are basically the same test as the ~20 lines in 115.
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@324
PS5, Line 324: authorize("select count(id) from functional.alltypes")
also very similar to the other test blocks, can we condense them further?
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@349
PS5, Line 349: authorize("select t.id from functional.alltypes t")
condense further?
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@451
PS5, Line 451: authorize("select * from functional.alltypes a cross join " +
I feel like these tests could be structured a little more. Not sure if you are
following the existing tests or not. We seem to have a few dimensions: view vs.
table, qualified vs. unqualified column/table reference.
Then we have tests for specific clauses in a SELECT like the FROM clause, GROUP
BY / HAVING clauses, etc.
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Comment-Date: Wed, 25 Apr 2018 22:51:14 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 ) Change subject: IMPALA-6802 (part 1): Clean up authorization tests .. Patch Set 5: Code-Review+1 Thanks for the updates. -- To view, visit http://gerrit.cloudera.org:8080/10135 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b Gerrit-Change-Number: 10135 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Fredy Wijaya Gerrit-Comment-Date: Tue, 24 Apr 2018 22:20:36 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Fredy Wijaya has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/10135 ) Change subject: IMPALA-6802 (part 1): Clean up authorization tests .. IMPALA-6802 (part 1): Clean up authorization tests The first patch of this patch is to introduce a new mechanism of testing authorization that tests authorization at every hierarchy. This patch rewrites the authorization tests for select statements. Testing: - Added new authorization tests - Ran all front-end tests Cherry-picks: not for 2.x Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b --- A fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java 1 file changed, 729 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/10135/5 -- To view, visit http://gerrit.cloudera.org:8080/10135 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b Gerrit-Change-Number: 10135 Gerrit-PatchSet: 5 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Fredy Wijaya
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Fredy Wijaya has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/10135 ) Change subject: IMPALA-6802 (part 1): Clean up authorization tests .. IMPALA-6802 (part 1): Clean up authorization tests The first patch of this patch is to introduce a new mechanism of testing authorization that tests authorization at every hierarchy. This patch rewrites the authorization tests for select statements. Testing: - Added new authorization tests - Ran all front-end tests Cherry-picks: not for 2.x Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b --- A fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java 1 file changed, 700 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/10135/4 -- To view, visit http://gerrit.cloudera.org:8080/10135 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b Gerrit-Change-Number: 10135 Gerrit-PatchSet: 4 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Fredy Wijaya
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 ) Change subject: IMPALA-6802 (part 1): Clean up authorization tests .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java: http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@95 PS2, Line 95: , > Maybe we don't need it for every test, but where is the test to ensure that Done http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@509 PS2, Line 509: > The problem is, if I expect an error to be "... not authorized on function Done. Instead of a boolean, there's a custom Matcher that we can use. -- To view, visit http://gerrit.cloudera.org:8080/10135 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b Gerrit-Change-Number: 10135 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Fredy Wijaya Gerrit-Comment-Date: Tue, 24 Apr 2018 18:54:55 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Fredy Wijaya has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/10135 ) Change subject: IMPALA-6802 (part 1): Clean up authorization tests .. IMPALA-6802 (part 1): Clean up authorization tests The first patch of this patch is to introduce a new mechanism of testing authorization that tests authorization at every hierarchy. This patch rewrites the authorization tests for select statements. Testing: - Added new authorization tests - Ran all front-end tests Cherry-picks: not for 2.x Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b --- A fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java 1 file changed, 701 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/10135/3 -- To view, visit http://gerrit.cloudera.org:8080/10135 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b Gerrit-Change-Number: 10135 Gerrit-PatchSet: 3 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Fredy Wijaya
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 ) Change subject: IMPALA-6802 (part 1): Clean up authorization tests .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java: http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@95 PS2, Line 95: ); > I don't know if we want to go through every permutation in the error. It ca Maybe we don't need it for every test, but where is the test to ensure that "REFRESH" or other privileges do not unintentionally allow you to do select? Shouldn't that be somewhere with the select tests? http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@509 PS2, Line 509: ) > Passing a full error string in expectedErrorString is essentially comparing The problem is, if I expect an error to be "... not authorized on functional", and the error is "... not authorized on functional.alltypes", I have no way to say I got the wrong error, i.e. there's information leakage on the error. -- To view, visit http://gerrit.cloudera.org:8080/10135 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b Gerrit-Change-Number: 10135 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Fredy Wijaya Gerrit-Comment-Date: Tue, 24 Apr 2018 15:37:00 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
..
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@95
PS2, Line 95: );
> Shouldn't there also be :
I don't know if we want to go through every permutation in the error. It can be
somewhat too excessive. The ok() ensures we have the minimum requirement. For
example:
.ok(onTable("functional", "alltypes", TPrivilegeLevel.SELECT)) means we must
have SELECT privilege on functional.alltypes and without it, we will get an
authorization error.
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@118
PS2, Line 118: // TODO: IMPALA-6718
: //.ok(onColumn("functional", "alltypes_view", new
String[]{"id", "bool_col",
: //"tinyint_col", "smallint_col", "int_col",
"bigint_col", "float_col",
: //"double_col", "date_string_col", "string_col",
"timestamp_col", "year",
: //"month"}, TPrivilegeLevel.SELECT))
: //.ok(onColumn("functional", "alltypes_view", new
String[]{"id", "bool_col",
: //"tinyint_col", "smallint_col", "int_col",
"bigint_col", "float_col",
: //"double_col", "date_string_col", "string_col",
"timestamp_col", "year",
: //"month"}, TPrivilegeLevel.SELECT))
> Take out this todo as it's waiting on future function. When the function i
I saw some existing code that adds TODO with a ticket number to give more
context especially if this is a bug found due to this CR. I'm okay with
removing it, though. What do others think?
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@138
PS2, Line 138: // TODO: IMPALA-6718
: //.ok(onColumn("functional", "alltypes_view", "id",
TPrivilegeLevel.SELECT))
> Remove. Add comment to Jira.
Same comment as above.
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@325
PS2, Line 325: // TODO: IMPALA-6891
: //.ok(onColumn("functional", "alltypes", new
String[]{"id", "bool_col",
: //"tinyint_col", "smallint_col", "int_col",
"bigint_col", "float_col",
: //"double_col", "date_string_col", "string_col",
"timestamp_col", "year",
: //"month"}, TPrivilegeLevel.SELECT),
: //onColumn("functional", "alltypessmall", new
String[]{"id", "bool_col",
: //"tinyint_col", "smallint_col", "int_col",
"bigint_col", "float_col",
: //"double_col", "date_string_col", "string_col",
"timestamp_col", "year",
: //"month"}, TPrivilegeLevel.SELECT))
> Remove. Add a comment to the Jira.
Same comment as above.
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@388
PS2, Line 388: TPrivilege[]... privileges
> There doesn't seem to be any tests that use this. If the other tests from
It's not used for the first part of this patch. In the next part like if we
want to do ALTER DATABASE RENAME, we need a CREATE privilege on a database and
an ALTER privilege on a table, this method can be handy.
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@509
PS2, Line 509: )
> Since we're changing this anyway, can we add another method that takes a bo
Passing a full error string in expectedErrorString is essentially comparing the
entire string. I can overload it, but then we need to overload bunch authzError
methods.
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Comment-Date: Tue, 24 Apr 2018 06:22:33 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Adam Holley has posted comments on this change. (
http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
..
Patch Set 2:
(6 comments)
I like the new format. Few first pass comments.
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@95
PS2, Line 95: );
Shouldn't there also be :
.error ("USER", onServer(TPrivilegeLevel.REFRESH,
TPrivilegeLevel.CREATE,TPrivilegeLevel.ALTER,TPrivilegeLevel.DROP))
.error ("USER", onDatabase("functional", TPrivilegeLevel.REFRESH,
TPrivilegeLevel.CREATE,TPrivilegeLevel.ALTER,TPrivilegeLevel.DROP))
.etc
for onServer, Database, Table, Column, and for the other tests as well to
ensure none of the other privileges allow the statement?
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@118
PS2, Line 118: // TODO: IMPALA-6718
: //.ok(onColumn("functional", "alltypes_view", new
String[]{"id", "bool_col",
: //"tinyint_col", "smallint_col", "int_col",
"bigint_col", "float_col",
: //"double_col", "date_string_col", "string_col",
"timestamp_col", "year",
: //"month"}, TPrivilegeLevel.SELECT))
: //.ok(onColumn("functional", "alltypes_view", new
String[]{"id", "bool_col",
: //"tinyint_col", "smallint_col", "int_col",
"bigint_col", "float_col",
: //"double_col", "date_string_col", "string_col",
"timestamp_col", "year",
: //"month"}, TPrivilegeLevel.SELECT))
Take out this todo as it's waiting on future function. When the function is
delivered, then appropriate tests should be added. Add a comment to the
Impala-6718 Jira.
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@138
PS2, Line 138: // TODO: IMPALA-6718
: //.ok(onColumn("functional", "alltypes_view", "id",
TPrivilegeLevel.SELECT))
Remove. Add comment to Jira.
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@325
PS2, Line 325: // TODO: IMPALA-6891
: //.ok(onColumn("functional", "alltypes", new
String[]{"id", "bool_col",
: //"tinyint_col", "smallint_col", "int_col",
"bigint_col", "float_col",
: //"double_col", "date_string_col", "string_col",
"timestamp_col", "year",
: //"month"}, TPrivilegeLevel.SELECT),
: //onColumn("functional", "alltypessmall", new
String[]{"id", "bool_col",
: //"tinyint_col", "smallint_col", "int_col",
"bigint_col", "float_col",
: //"double_col", "date_string_col", "string_col",
"timestamp_col", "year",
: //"month"}, TPrivilegeLevel.SELECT))
Remove. Add a comment to the Jira.
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@388
PS2, Line 388: TPrivilege[]... privileges
There doesn't seem to be any tests that use this. If the other tests from
comments above are not needed, then is this?
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@509
PS2, Line 509: )
Since we're changing this anyway, can we add another method that takes a
boolean to indicate it should use the entire string instead of just starts
with? Some of the authorization errors are important to distinguish that the
error returns just the table, not the table and column.
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Comment-Date: Tue, 24 Apr 2018 06:00:59 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Fredy Wijaya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10135 Change subject: IMPALA-6802 (part 1): Clean up authorization tests .. IMPALA-6802 (part 1): Clean up authorization tests The first patch of this patch is to introduce a new mechanism of testing authorization that tests authorization at every hierarchy. This patch rewrites the authorization tests for select statements. Testing: - Added new authorization tests - Ran all front-end tests Cherry-picks: not for 2.x Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b --- A fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java 1 file changed, 525 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/10135/2 -- To view, visit http://gerrit.cloudera.org:8080/10135 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b Gerrit-Change-Number: 10135 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya
