----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51694/#review148634 -----------------------------------------------------------
service/pom.xml (line 165) <https://reviews.apache.org/r/51694/#comment216114> 1. Need remove the empty spaces (in red squares) 2. Should we add the version <version>${mockito-all.version}</version>. currently the ${mockito-all.version} in Hive is 1.9.5. service/src/java/org/apache/hive/service/auth/ldap/ChainFilterFactory.java (line 46) <https://reviews.apache.org/r/51694/#comment216120> Need remove all the extra empty spaces (in red squares) here and also other places. service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java (line 37) <https://reviews.apache.org/r/51694/#comment216588> Do we really need an extra factory layer and have a factory for each filter? In Hive, actaully each session instantiates its own LdapAuthenticationProviderImpl, which now contains different factories with each one generating only one instance of its filter. service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java (line 62) <https://reviews.apache.org/r/51694/#comment216560> Is not it the LdapUtils.patternsToBaseDns(userPatterns)? service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java (line 89) <https://reviews.apache.org/r/51694/#comment216631> I don't think it is necessary to cache the user/userDn and also it might be a potential security issue. service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java (line 108) <https://reviews.apache.org/r/51694/#comment216872> can getSingleLdapName be used to enforce only one returned entry? that API in SearachResultHandler is never used. service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java (line 105) <https://reviews.apache.org/r/51694/#comment216553> This method might throw out runtime exception such as NPE, IndexOutOfBoundsException, should we check the passed in parameter rdn? We might not run into this situation in old code, but since this line of code is refactored as a separate API, I think we should do the check. Same for the other methods like patternToBaseDn etc. service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java (line 159) <https://reviews.apache.org/r/51694/#comment216558> I am not sure if there is any precedence for these configurations, but here it seems that the GUIDKEY/BASEDN takes precedence over DNPATTERN, which is different from the existing implementation and cause the behavior change. service/src/java/org/apache/hive/service/auth/ldap/Query.java (line 122) <https://reviews.apache.org/r/51694/#comment216868> Will it improve the performance to set the search limit? I did not see it is used. - Chaoyu Tang On Sept. 7, 2016, 2:24 p.m., Illya Yalovyy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51694/ > ----------------------------------------------------------- > > (Updated Sept. 7, 2016, 2:24 p.m.) > > > Review request for hive, Ashutosh Chauhan, Chaoyu Tang, Naveen Gangam, and > Szehon Ho. > > > Repository: hive-git > > > Description > ------- > > Currently LdapAuthenticationProviderImpl class is not covered with unit > tests. To make this class testable some minor refactoring will be required. > > > Diffs > ----- > > service/pom.xml ecea719 > > service/src/java/org/apache/hive/service/auth/LdapAuthenticationProviderImpl.java > efd5393 > service/src/java/org/apache/hive/service/auth/ldap/ChainFilterFactory.java > PRE-CREATION > > service/src/java/org/apache/hive/service/auth/ldap/CustomQueryFilterFactory.java > PRE-CREATION > service/src/java/org/apache/hive/service/auth/ldap/DirSearch.java > PRE-CREATION > service/src/java/org/apache/hive/service/auth/ldap/DirSearchFactory.java > PRE-CREATION > service/src/java/org/apache/hive/service/auth/ldap/Filter.java PRE-CREATION > service/src/java/org/apache/hive/service/auth/ldap/FilterFactory.java > PRE-CREATION > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java > PRE-CREATION > service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java > PRE-CREATION > service/src/java/org/apache/hive/service/auth/ldap/LdapSearchFactory.java > PRE-CREATION > service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java > PRE-CREATION > service/src/java/org/apache/hive/service/auth/ldap/Query.java PRE-CREATION > service/src/java/org/apache/hive/service/auth/ldap/QueryFactory.java > PRE-CREATION > service/src/java/org/apache/hive/service/auth/ldap/SearchResultHandler.java > PRE-CREATION > service/src/java/org/apache/hive/service/auth/ldap/UserFilterFactory.java > PRE-CREATION > > service/src/java/org/apache/hive/service/auth/ldap/UserSearchFilterFactory.java > PRE-CREATION > > service/src/test/org/apache/hive/service/auth/TestLdapAtnProviderWithMiniDS.java > 089a059 > > service/src/test/org/apache/hive/service/auth/TestLdapAuthenticationProviderImpl.java > f276906 > service/src/test/org/apache/hive/service/auth/ldap/Credentials.java > PRE-CREATION > service/src/test/org/apache/hive/service/auth/ldap/LdapTestUtils.java > PRE-CREATION > service/src/test/org/apache/hive/service/auth/ldap/TestChainFilter.java > PRE-CREATION > > service/src/test/org/apache/hive/service/auth/ldap/TestCustomQueryFilter.java > PRE-CREATION > service/src/test/org/apache/hive/service/auth/ldap/TestGroupFilter.java > PRE-CREATION > service/src/test/org/apache/hive/service/auth/ldap/TestLdapSearch.java > PRE-CREATION > service/src/test/org/apache/hive/service/auth/ldap/TestLdapUtils.java > PRE-CREATION > service/src/test/org/apache/hive/service/auth/ldap/TestQuery.java > PRE-CREATION > service/src/test/org/apache/hive/service/auth/ldap/TestQueryFactory.java > PRE-CREATION > > service/src/test/org/apache/hive/service/auth/ldap/TestSearchResultHandler.java > PRE-CREATION > service/src/test/org/apache/hive/service/auth/ldap/TestUserFilter.java > PRE-CREATION > > service/src/test/org/apache/hive/service/auth/ldap/TestUserSearchFilter.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/51694/diff/ > > > Testing > ------- > > ...hive/service> mvn clean test > > ... > > Results : > > Tests run: 123, Failures: 0, Errors: 0, Skipped: 0 > > [INFO] > ------------------------------------------------------------------------ > [INFO] BUILD SUCCESS > [INFO] > ------------------------------------------------------------------------ > [INFO] Total time: 04:18 min > [INFO] Finished at: 2016-09-06T08:46:04-07:00 > [INFO] Final Memory: 66M/984M > [INFO] > ------------------------------------------------------------------------ > > > Thanks, > > Illya Yalovyy > >
