> On Sept. 17, 2016, 1:36 a.m., Chaoyu Tang wrote: > > service/src/java/org/apache/hive/service/auth/ldap/GroupFilterFactory.java, > > line 37 > > <https://reviews.apache.org/r/51694/diff/1/?file=1492945#file1492945line37> > > > > 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.
I think so. Factories encapsulate logic related to choosing whether particular filter is required or not based on the provided configuration. > 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. 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? > 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. 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. > 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. 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. > On Sept. 17, 2016, 1:36 a.m., Chaoyu Tang wrote: > > service/src/java/org/apache/hive/service/auth/ldap/Query.java, line 122 > > <https://reviews.apache.org/r/51694/diff/1/?file=1492949#file1492949line122> > > > > Will it improve the performance to set the search limit? I did not see > > it is used. I will be used for different filters. Do you think we should use it for existing filters? Which one in particular? Or you would prefer me to remove this option? Please keep in mind that this CR is not about performance. - Illya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51694/#review148634 ----------------------------------------------------------- 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 > >
