Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16072 )
Change subject: KUDU-3090 Add ownership privileges ...................................................................... Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/16072/7/src/kudu/master/authz_provider.h File src/kudu/master/authz_provider.h: http://gerrit.cloudera.org:8080/#/c/16072/7/src/kudu/master/authz_provider.h@94 PS7, Line 94: table_names nit: maybe call this 'is_owner_by_table_name' or something http://gerrit.cloudera.org:8080/#/c/16072/7/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/16072/7/src/kudu/master/catalog_manager.cc@1939 PS7, Line 1939: mismatched_table = true; Maybe consider populating some scoped_refptr<TableInfo> table_with_mismatched_name field that we can lock in the error case, rather than passing an empty string? I know it's super edge-casey, but it does seem like the correct behavior would be to complain about the mismatched table name rather than return an authorization error if the mismatched table is indeed owned by the user. http://gerrit.cloudera.org:8080/#/c/16072/7/src/kudu/master/catalog_manager.cc@1957 PS7, Line 1957: RETURN_NOT_OK(authorize(NormalizeTableName(table_identifier.table_name()), "")); nit: may be obvious, but it's a little jarring to see empty strings. Maybe add a comment along the lines of "If the table doesn't exist, we don't have ownership information we can pass" or somesuch. http://gerrit.cloudera.org:8080/#/c/16072/7/src/kudu/master/catalog_manager.cc@2960 PS7, Line 2960: user.value() nit: can use *user http://gerrit.cloudera.org:8080/#/c/16072/7/src/kudu/ranger/ranger_client-test.cc File src/kudu/ranger/ranger_client-test.cc: http://gerrit.cloudera.org:8080/#/c/16072/7/src/kudu/ranger/ranger_client-test.cc@253 PS7, Line 253: ASSERT_NE(tables.find("default.foobar"), tables.end()); : ASSERT_NE(tables.find("barbaz"), tables.end()); : ASSERT_EQ(tables.find("foo."), tables.end()); nit: you can use ContainsKey() from gutil/map-util.h -- To view, visit http://gerrit.cloudera.org:8080/16072 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id9c36b7d84863403d7d538cafc709d2aebd0b109 Gerrit-Change-Number: 16072 Gerrit-PatchSet: 7 Gerrit-Owner: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 26 Jun 2020 17:31:54 +0000 Gerrit-HasComments: Yes
