> On Jan. 26, 2016, 2:59 a.m., Colin Ma wrote: > > Thanks for the patch, it looks good to me overall with some minor comments. > > I'm wonder if no update for sentryShell, how this feature works for Solr > > with command line?
It doesn't work from the sentryShell command line, it allows you to write another shell. I'm not convinced sentryShell is the right thing to use here; it uses hadoop commands which aren't required for solr. If you think adding sentryShell support is a useful feature, file a jira. > On Jan. 26, 2016, 2:59 a.m., Colin Ma wrote: > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SolrTSentryPrivilegeConvertor.java, > > line 114 > > <https://reviews.apache.org/r/42682/diff/4/?file=1220907#file1220907line114> > > > > Currently, only collection for Solr, and validate this already in the > > previous code: > > > > if (authz instanceof Collection) { > > .... > > } else { > > throw new IllegalArgumentException > > } > > > > This method is not necessary for current model. I don't know what you mean. What is the previous code? If you are referring to SearchModelAuthorizables.from(keyValue), that returns null when it encounters an unknown authorizable, so it doesn't do any validation. If you don't do this, you can put bogus Authorizables (i.e. bogus1=bogus1->collection=collection2->bogus2=bogus2->action=UPDATE The question is where the validations should happen. It seems like it should happen on the sentry server because: 1) it can't be done on the client, because we can't guarantee everyone uses our client 2) it can't be done well on the service (i.e. solr), because there's no push from sentry to the service. So the user experience is pretty bad in that you enter a bogus privilege, it gets accepted, and then the service starts misbehaving. There appears to be some checking on the sentry service (i.e. that collection exists, although bogus privileges seem to be accepted). This is just locking down the client as much as possible, to only accept privileges we know are valid. We can open it up later once we have the privilege validation more thought out. - Gregory ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42682/#review116263 ----------------------------------------------------------- On Jan. 26, 2016, 12:10 a.m., Gregory Chanan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42682/ > ----------------------------------------------------------- > > (Updated Jan. 26, 2016, 12:10 a.m.) > > > Review request for sentry. > > > Repository: sentry > > > Description > ------- > > Here is a solr shell, based on the existing hive shell. Actually most of the > work is specific to the generic model, there is only a small amount of > solr-specific code. So it should be easily extensible for other clients. > > Some limitations: > > It is not integrated with bin/sentryShell yet > It does not support any grant options > It is pretty restrictive in what it supports, i.e. it checks for know > authorizables and throws an exception if it doesn't know about any. So it > can't be used for anything outside of collection permissions. > > > Diffs > ----- > > sentry-provider/sentry-provider-db/pom.xml > 7514a7cdfcc7934f2dd0386996fdaf88c0ccbb14 > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SentryShellSolr.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/SolrTSentryPrivilegeConvertor.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/command/Command.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/command/CreateRoleCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/command/DropRoleCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/command/GrantPrivilegeToRoleCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/command/ListPrivilegesByRoleCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/command/ListRolesCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/command/RevokePrivilegeFromRoleCmd.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/generic/tools/command/TSentryPrivilegeConvertor.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/main/java/org/apache/sentry/provider/db/tools/SentryShellCommon.java > b1353c5312e9d53b6dd184edd1fa8c11a92b4374 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/SentryGenericServiceIntegrationBase.java > PRE-CREATION > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/service/thrift/TestSentryGenericServiceIntegration.java > 4732ea2fcac1f9cb6a885592378f7916edbde3a1 > > sentry-provider/sentry-provider-db/src/test/java/org/apache/sentry/provider/db/generic/tools/TestSentryShellSolr.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/42682/diff/ > > > Testing > ------- > > Ran the unit tests. > > > Thanks, > > Gregory Chanan > >
