> On Aug. 17, 2018, 8:18 p.m., Na Li wrote:
> > sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
> > Lines 255 (patched)
> > <https://reviews.apache.org/r/64661/diff/10/?file=2074734#file2074734line256>
> >
> > 1) I believe that when kafka calls
> >
> > the groups is a list with only one member, which is "null".
> >
> > 1.1) the group name is null in SentryGenericServiceClientDefaultImpl
> > @Override
> > public Set<TSentryRole> listAllRoles(String requestorUserName, String
> > component)
> > throws SentryUserException {
> > return listRolesByGroupName(requestorUserName, null, component);
> > }
> >
> > 1.2) the group name of "null" is in request to generic service
> >
> > public Set<TSentryRole> listRolesByGroupName(
> > String requestorUserName,
> > String groupName,
> > String component)
> > throws SentryUserException {
> > TListSentryRolesRequest request = new TListSentryRolesRequest();
> >
> > request.setProtocol_version(sentry_common_serviceConstants.TSENTRY_SERVICE_V2);
> > request.setRequestorUserName(requestorUserName);
> > request.setGroupName(groupName);
> > request.setComponent(component);
> > TListSentryRolesResponse response;
> > try {
> > response = client.list_sentry_roles_by_group(request);
> > Status.throwIfNotOk(response.getStatus());
> > return response.getRoles();
> > } catch (TException e) {
> > throw new SentryUserException(THRIFT_EXCEPTION_MESSAGE, e);
> > }
> > }
> >
> > 1.3) groups contains "null" when calling
> > DelegateSentryStore.getRolesByGroups().
> >
> > public TListSentryRolesResponse list_sentry_roles_by_group(
> > final TListSentryRolesRequest request) throws TException {
> > Response<Set<TSentryRole>> respose = requestHandle(new
> > RequestHandler<Set<TSentryRole>>() {
> > @Override
> > public Response<Set<TSentryRole>> handle() throws Exception {
> > validateClientVersion(request.getProtocol_version());
> > Set<String> groups = getRequestorGroups(conf,
> > request.getRequestorUserName());
> > if
> > (!AccessConstants.ALL.equalsIgnoreCase(request.getGroupName())) {
> > boolean admin = inAdminGroups(groups);
> > //Only admin users can list all roles in the system (
> > groupname = null)
> > //Non admin users are only allowed to list only groups which
> > they belong to
> > if(!admin && (request.getGroupName() == null ||
> > !groups.contains(request.getGroupName()))) {
> > throw new SentryAccessDeniedException(ACCESS_DENIAL_MESSAGE
> > + request.getRequestorUserName());
> > }
> > groups.clear();
> > groups.add(request.getGroupName());
> > }
> > Set<String> roleNames =
> > store.getRolesByGroups(request.getComponent(), groups);
> >
> >
> > 2) This implementation using SentryStore.getRolesForGroups() does not
> > have groups contains a single element of "null" gracefully. That is why I
> > am really worried about the test you have done for this update.
> >
> > public Set<MSentryRole> getRolesForGroups(PersistenceManager pm,
> > Set<String> groups) {
> > Set<MSentryRole> result = Sets.newHashSet();
> > if (groups != null) {
> > Query query = pm.newQuery(MSentryGroup.class);
> > query.addExtension(LOAD_RESULTS_AT_COMMIT, "false");
> > query.setFilter(":p1.contains(this.groupName)");
> > List<MSentryGroup> sentryGroups = (List)
> > query.execute(groups.toArray());
> > if (sentryGroups != null) {
> > for (MSentryGroup sentryGroup : sentryGroups) {
> > result.addAll(sentryGroup.getRoles());
> > }
> > }
> > }
> > return result;
> > }
> >
> > 3) You should check if there is a member of null in groups and handle
> > it properly, and have testing case for that scenario.
Still going through your comments. But I did add the null check under
getRolesByGroups method. See below:
if(groups.contains(null)) {
true)) {
roles = delegate.getAllRoleNames();
roles.add(tSentryRole.getRoleName());
} else {
roles = delegate.getRoleNamesForGroups(groups);
}
}
- Arjun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64661/#review207538
-----------------------------------------------------------
On Aug. 17, 2018, 7:01 p.m., Arjun Mishra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64661/
> -----------------------------------------------------------
>
> (Updated Aug. 17, 2018, 7:01 p.m.)
>
>
> Review request for sentry, kalyan kumar kalvagadda, Na Li, Steve Moist, and
> Sergio Pena.
>
>
> Bugs: SENTRY-1944
> https://issues.apache.org/jira/browse/SENTRY-1944
>
>
> Repository: sentry
>
>
> Description
> -------
>
> When Solr is using Sentry server for authorization, it issues a lot of calls
> to getGroupsByRoles() function in DelegateSentryStore.
>
> This function isn't very efficient - it walks over each role in the set,
> obtains role by name, get groups for each role, and collects all group names
> into a set.
>
> It may be possible to optimize it.
>
> Also, in SentryGenericPolicyProcessor class method
> list_sentry_roles_by_group() would make N transactions to build the roles to
> set of groups map. Instead, make it to a single transaction. This will
> significantly speed up operation
>
> Attach one or more files to this issue
>
>
> Diffs
> -----
>
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/api/generic/thrift/SentryGenericPolicyProcessor.java
> 1cc4b1b37
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/DelegateSentryStore.java
> 3026a6225
>
> sentry-service/sentry-service-server/src/main/java/org/apache/sentry/provider/db/generic/service/persistent/SentryStoreLayer.java
> eec2757d3
>
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/api/generic/thrift/TestSentryGenericPolicyProcessor.java
> 4c207e9b4
>
> sentry-service/sentry-service-server/src/test/java/org/apache/sentry/provider/db/generic/service/persistent/TestDelegateSentryStore.java
> 69d16238f
>
>
> Diff: https://reviews.apache.org/r/64661/diff/10/
>
>
> Testing
> -------
>
> mvn -f sentry-provider/sentry-provider-db/pom.xml test
>
>
> Thanks,
>
> Arjun Mishra
>
>
