----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20803/#review41985 -----------------------------------------------------------
sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/20803/#comment75710> Where is the API that allows getting a TSentryRole back from the service? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/20803/#comment75705> nit: The SQL "SHOW ROLES" is a client implementation detail and this may not be the place to comment on that. Perhaps instead say something like "No group name was specified, so return all roles". sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/20803/#comment75704> I see this "this.groupName == t" and "this.roleName == t" strings duplicated a lot, would it make sense to make these string constants? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/20803/#comment75707> Preconditions.checkNotNull(roles); ? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/20803/#comment75706> Should "rollbackTransaction=false" be set before or after commitTx completes successfully? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/20803/#comment75703> Would it be good to comment on the public methods? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java <https://reviews.apache.org/r/20803/#comment75708> There is not type called SentryGroup, it is TSentryGroup. A less verbose name might be: "toTSentryGroup()". I also wonder whether these conversion functions should live in this class. Ideally, this would be an instance method on MSentryGroup (but I'm not sure if that's generated code?). Otherwise, consider a utility class? sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java <https://reviews.apache.org/r/20803/#comment75709> comment on that this does and what it returns. sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java <https://reviews.apache.org/r/20803/#comment75711> this "Thrift exception ..." is duplicated many times, consider a constant format string. sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift <https://reviews.apache.org/r/20803/#comment75716> Not related to your change, but this requestorUserName+groupNames is very clunky and it's getting duplicated lots of places. This doesn't seem like a good way to secure the service and once this is released we will need to maintain some thrift compatibility. It would be really nice to strip all of this out before 5.1. sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift <https://reviews.apache.org/r/20803/#comment75715> for this group, or all roles for all groups if null. - Lenni Kuff On May 1, 2014, 1:04 a.m., Sravya Tirukkovalur wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20803/ > ----------------------------------------------------------- > > (Updated May 1, 2014, 1:04 a.m.) > > > Review request for sentry, Lenni Kuff and Prasad Mujumdar. > > > Bugs: SENTRY-184 > https://issues.apache.org/jira/browse/SENTRY-184 > > > Repository: sentry > > > Description > ------- > > This patch adds the following thrift API to query the list of privileges > granted for a given role: > * TListSentryPrivilegesResponse > list_sentry_privileges_by_role(1:TListSentryPrivilegesRequest request) > ** Allows to get granted privileges for a given role > > > It also fixes the issues with: > * TListSentryRolesResponse > list_sentry_roles_by_group(1:TListSentryRolesRequest request) > ** Looks up the roles for a given group. > ** Also can be used to list all roles in the system (when group_name = null) > > Getting rid of following API as we do not support mapping a role to multiple > roles. > * TListSentryRolesResponse > list_sentry_roles_by_role_name(1:TListSentryRolesRequest request) > > Outstanding issues: > > * We still need to decide on the semantics of "Show grant" and "show role > grant": > ** Right now, these meta data query operations can be performed only by the > admin user. But ideally we should allow even non admin users to query as long > as they are part of the group/role they are requesting info from. > > > Diffs > ----- > > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java > 33c8d1af864282b2b56a54038573cfdcaa9a44f3 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java > 7e73b2c7195c1f4561a30dab018bb5ba0d68f3ec > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyStoreProcessor.java > 6c52fa421eaa5d036b522f827a9d0f8433676275 > > sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift > 677047fe359e346c2c3cd9f7bcb6597288d83cc5 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/service/thrift/TestSentryServiceIntegration.java > dcaa246ea72e37c7aa7aed51f5c65b96bec61405 > > Diff: https://reviews.apache.org/r/20803/diff/ > > > Testing > ------- > > Added new tests. > > > Thanks, > > Sravya Tirukkovalur > >
