> On Sept. 17, 2016, 1:36 a.m., Chaoyu Tang wrote: > > service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java, line 105 > > <https://reviews.apache.org/r/51694/diff/1/?file=1492948#file1492948line105> > > > > 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. > > Illya Yalovyy wrote: > Agree. The DN parsing in general implemented quite poorly. I have a task > already to re-implement it completely. There are many problems with current > one. Handling incorrect format or invalid input is only one of them. My > intention is to use RDN java class to do a correct parsing. > > https://docs.oracle.com/javase/7/docs/api/javax/naming/ldap/Rdn.html > > I think we can leave it for now, and I will submit another CR that > addresses this concern sortly.
OK > On Sept. 17, 2016, 1:36 a.m., Chaoyu Tang wrote: > > service/src/java/org/apache/hive/service/auth/ldap/LdapSearch.java, line 108 > > <https://reviews.apache.org/r/51694/diff/1/?file=1492946#file1492946line108> > > > > can getSingleLdapName be used to enforce only one returned entry? that > > API in SearachResultHandler is never used. > > Illya Yalovyy wrote: > I was trying to copy existing logic. At the moment, I don't want to do > any changes to that. This particular code should be improved in separate CR. > Does it make sense? OK > On Sept. 17, 2016, 1:36 a.m., Chaoyu Tang wrote: > > service/src/java/org/apache/hive/service/auth/ldap/LdapUtils.java, line 159 > > <https://reviews.apache.org/r/51694/diff/1/?file=1492948#file1492948line159> > > > > 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. > > Illya Yalovyy wrote: > Could you please give more details on the case when the behavior will be > different. > The logic seems to be same: It uses GUIDKEY/BASEDN only when DNPATTERN is > not configured: > > if (StringUtils.isBlank(patternsString)) { > ... > > Which means *only* if patternsString is blank, try to use GUIDKEY/BASEDN. > > Please let me know if I did not get it correctly. Never mind. - Chaoyu ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51694/#review148634 ----------------------------------------------------------- On Sept. 20, 2016, 7:39 p.m., Illya Yalovyy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51694/ > ----------------------------------------------------------- > > (Updated Sept. 20, 2016, 7:39 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 > >
