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 Sat, 17 Sep 2016 01:36:58 GMT

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


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