> On Aug. 1, 2014, 8:23 p.m., Thejas Nair wrote: > > itests/hive-unit-hadoop2/src/test/java/org/apache/hadoop/hive/ql/security/TestStorageBasedMetastoreAuthorizationProviderWithACL.java, > > line 43 > > <https://reviews.apache.org/r/24170/diff/1/?file=647603#file647603line43> > > > > make it private ? > >
Ok, will change. > On Aug. 1, 2014, 8:23 p.m., Thejas Nair wrote: > > shims/common/src/main/java/org/apache/hadoop/fs/DefaultFileAccess.java, > > line 69 > > <https://reviews.apache.org/r/24170/diff/1/?file=647611#file647611line69> > > > > I think we should add an else here, to make it actually compatible with > > HDFS/POSIX way of checking permissions. > > Ok. Note this was taken from the original implementation from StorageBasedAuthorizationProvider, so this is changing the original implementation. > On Aug. 1, 2014, 8:23 p.m., Thejas Nair wrote: > > shims/common/src/main/java/org/apache/hadoop/fs/DefaultFileAccess.java, > > line 74 > > <https://reviews.apache.org/r/24170/diff/1/?file=647611#file647611line74> > > > > another else here as well > > ok, will change. > On Aug. 1, 2014, 8:23 p.m., Thejas Nair wrote: > > shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java, > > line 683 > > <https://reviews.apache.org/r/24170/diff/1/?file=647612#file647612line683> > > > > I think we should remove the user, and groups from the arguments, and > > make it compatible with the arguments of access method. > > > > If the group is different from what HDFS is going to see, then we are > > not really checking against the HDFS permissions. Keeping the metadata > > authorization in sync with HDFS permissions is goal of SBA. > > So nobody should be using a authentication provider that is > > inconsistent with HDFS groups, and I don't think anybody would be doing > > that. > > > > We are also ignoring those arguments when the newer versions of hadoop > > get used. > > > > I think removing the arguments from here will make that more clear to > > other developers, and is better for longer term code cleanliness. > > > > By the time this method is called any doAs() should have been called already, so we can probably rely on the current UGI to get the user name, and also the group names. A bit of a departure from existing behavior since the authorizer (not UGI) was responsible for providing the list of groups for the user, but I agree this is more in sync with HDFS permissions. > On Aug. 1, 2014, 8:23 p.m., Thejas Nair wrote: > > common/src/java/org/apache/hadoop/hive/common/FileUtils.java, line 379 > > <https://reviews.apache.org/r/24170/diff/1/?file=647602#file647602line379> > > > > do we need to have "Exception" class in the throws ? > > Shims.doAs() throws IOException, InterruptedException Will fix this. - Jason ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24170/#review49375 ----------------------------------------------------------- On Aug. 1, 2014, 12:54 a.m., Jason Dere wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24170/ > ----------------------------------------------------------- > > (Updated Aug. 1, 2014, 12:54 a.m.) > > > Review request for hive and Thejas Nair. > > > Bugs: HIVE-7583 > https://issues.apache.org/jira/browse/HIVE-7583 > > > Repository: hive-git > > > Description > ------- > > Create shims method to either use new FileSystem.access() API, or fall back > to Hive implementation of file access checks. > > > Diffs > ----- > > common/src/java/org/apache/hadoop/hive/common/FileUtils.java 8dcd4cf > > itests/hive-unit-hadoop2/src/test/java/org/apache/hadoop/hive/ql/security/TestStorageBasedMetastoreAuthorizationProviderWithACL.java > PRE-CREATION > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/TestMetastoreAuthorizationProvider.java > 97fb7ba > > itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/security/TestStorageBasedMetastoreAuthorizationProvider.java > 223f155 > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/StorageBasedAuthorizationProvider.java > f803cc4 > > ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java > 6a283ab > shims/0.20/src/main/java/org/apache/hadoop/hive/shims/Hadoop20Shims.java > d03f270 > shims/0.23/src/main/java/org/apache/hadoop/hive/shims/Hadoop23Shims.java > e98b54d > > shims/common-secure/src/main/java/org/apache/hadoop/hive/shims/HadoopShimsSecure.java > 605ea55 > shims/common/src/main/java/org/apache/hadoop/fs/DefaultFileAccess.java > PRE-CREATION > shims/common/src/main/java/org/apache/hadoop/hive/shims/HadoopShims.java > eefd5e5 > > Diff: https://reviews.apache.org/r/24170/diff/ > > > Testing > ------- > > > Thanks, > > Jason Dere > >
