> On May 16, 2018, 3:52 p.m., Na Li wrote: > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java > > Lines 245 (patched) > > <https://reviews.apache.org/r/67131/diff/2/?file=2023763#file2023763line248> > > > > should you check the type if user here? Just to guard against future > > change that the AclEntryType is not group nor user.
will add check. > On May 16, 2018, 3:52 p.m., Na Li wrote: > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java > > Lines 278 (patched) > > <https://reviews.apache.org/r/67131/diff/2/?file=2023763#file2023763line281> > > > > Is it abnormal for this to happen? If so, we need to log at warning > > level. If it is normal, log at debug level It is abnormal to happen. it is just a safeguard. we need to think very carefully before adding a log here as this code gets hit every time there is autirozation request from HDFS. > On May 16, 2018, 3:52 p.m., Na Li wrote: > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java > > Lines 288 (patched) > > <https://reviews.apache.org/r/67131/diff/2/?file=2023763#file2023763line291> > > > > why should this be FsAction.NONE instead of "fsAction = > > perms.get(aclEntry) first. When it is null, then assign it to be > > FsAction.NONE"? "perms" map is constructed every time when there is authorization check done from HDFS. I see that your comment is based on what we have for ROLE. While getting perms for roles, code looks for permissions for all the groups that this is granted so we have logic "fsAction = perms.get(aclEntry)" there. when it come to users, entry in perms is not expected that is why there is no need to have similar logic for users. - kalyan kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67131/#review203235 ----------------------------------------------------------- On May 15, 2018, 11:45 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67131/ > ----------------------------------------------------------- > > (Updated May 15, 2018, 11:45 p.m.) > > > Review request for sentry, Arjun Mishra, Na Li, and Sergio Pena. > > > Bugs: SENTRY-2174 > https://issues.apache.org/jira/browse/SENTRY-2174 > > > Repository: sentry > > > Description > ------- > > SentryAuthorizationProvider should now additionally provide the ACL entries > with the permissions that users have along with the permissions for the > groups. > > With the changes proposed in SENTRY-2173, PrivilegeInfo will not only have > role to permission mapping. it will also have user to privilege mapping > information. > > SentryAuthorizationProvider should be using the new information added in > PrivilegeInfo to add ACL for users. > > > Diffs > ----- > > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryPermissions.java > a88d8e25ad745effe354aa6267252998b189a252 > > sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryPermissions.java > dbce40538cf4721b601cf63b1da713f3d57fc981 > > > Diff: https://reviews.apache.org/r/67131/diff/2/ > > > Testing > ------- > > Added new unit tests and also made sure that all the existing tests pass. > > > Thanks, > > kalyan kumar kalvagadda > >
