hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chaoyu Tang <ctang...@gmail.com>
Subject Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider should be covered with unit tests
Date Wed, 21 Sep 2016 03:20:18 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message