> On May 2, 2014, 1:12 a.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/resources/sentry_policy_service.thrift, > > line 144 > > <https://reviews.apache.org/r/20803/diff/2/?file=572066#file572066line144> > > > > 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.
This will be handled as part of Sentry-191. > On May 2, 2014, 1:12 a.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 540 > > <https://reviews.apache.org/r/20803/diff/2/?file=572063#file572063line540> > > > > Where is the API that allows getting a TSentryRole back from the > > service? This function gets TSentryRoles: getTSentryRolesByGroupName > On May 2, 2014, 1:12 a.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 552 > > <https://reviews.apache.org/r/20803/diff/2/?file=572063#file572063line552> > > > > 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". Fixed it. > On May 2, 2014, 1:12 a.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 559 > > <https://reviews.apache.org/r/20803/diff/2/?file=572063#file572063line559> > > > > I see this "this.groupName == t" and "this.roleName == t" strings > > duplicated a lot, would it make sense to make these string constants? Will handle this in a refactoring jira, as it will touch more parts of the code. > On May 2, 2014, 1:12 a.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 573 > > <https://reviews.apache.org/r/20803/diff/2/?file=572063#file572063line573> > > > > Should "rollbackTransaction=false" be set before or after commitTx > > completes successfully? Fixed > On May 2, 2014, 1:12 a.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 583 > > <https://reviews.apache.org/r/20803/diff/2/?file=572063#file572063line583> > > > > Would it be good to comment on the public methods? Added java doc for all the public functions added by this patch. There are still many public functions outside of this patch, which will need java doc /comments, will handle it in a separate jira. > On May 2, 2014, 1:12 a.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/persistent/SentryStore.java, > > line 710 > > <https://reviews.apache.org/r/20803/diff/2/?file=572063#file572063line710> > > > > 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? Changed the function names to TSentry* to be more descriptive. I moved a few functions like building the MSentry* objects itself to the MSentry* classes, but left the conversions itself to SentryStore for now. > On May 2, 2014, 1:12 a.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java, > > line 152 > > <https://reviews.apache.org/r/20803/diff/2/?file=572064#file572064line152> > > > > comment on that this does and what it returns. Done. > On May 2, 2014, 1:12 a.m., Lenni Kuff wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/thrift/SentryPolicyServiceClient.java, > > line 169 > > <https://reviews.apache.org/r/20803/diff/2/?file=572064#file572064line169> > > > > this "Thrift exception ..." is duplicated many times, consider a > > constant format string. Will do some refactoring of these constants in a following jira. - Sravya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20803/#review41985 ----------------------------------------------------------- On May 5, 2014, 6:34 p.m., Sravya Tirukkovalur wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20803/ > ----------------------------------------------------------- > > (Updated May 5, 2014, 6:34 p.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/SimpleDBProviderBackend.java > b068aca22dbcc930a25b014a3140659fb55537de > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/service/model/MSentryRole.java > 1dfc0cf99cfe1b740e0b20a0d7f06ab06d8dfc1d > > 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 > >
