You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Illya Yalovyy <ya...@amazon.com> on 2016/09/07 14:24:09 UTC
Review Request 51694: HIVE-14713 LDAP Authentication Provider should
be covered with unit tests
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51694/
-----------------------------------------------------------
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
Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider
should be covered with unit tests
Posted by Illya Yalovyy <ya...@amazon.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51694/#review149493
-----------------------------------------------------------
Chaoyu,
Thank you for a great review. I have also done some minor changes on my side. I'll update this CR in a couple of days.
- Illya Yalovyy
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
>
>
Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider
should be covered with unit tests
Posted by Illya Yalovyy <ya...@amazon.com>.
> 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.
>
> Illya Yalovyy wrote:
> 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.
>
> Chaoyu Tang wrote:
> I thought in the existing implementation, the search limits in some methods like findGroupDNByName, findUserDNByName are set to 2 to reduce the returned results in case there are many, is not it?
It does make sense. I'll make this change.
- Illya
-----------------------------------------------------------
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
>
>
Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider
should be covered with unit tests
Posted by Illya Yalovyy <ya...@amazon.com>.
> 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
>
>
Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider
should be covered with unit tests
Posted by Chaoyu Tang <ct...@gmail.com>.
> 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
>
>
Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider
should be covered with unit tests
Posted by Chaoyu Tang <ct...@gmail.com>.
> 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.
>
> Illya Yalovyy wrote:
> 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.
I thought in the existing implementation, the search limits in some methods like findGroupDNByName, findUserDNByName are set to 2 to reduce the returned results in case there are many, is not it?
- 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
>
>
Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider
should be covered with unit tests
Posted by Chaoyu Tang <ct...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider
should be covered with unit tests
Posted by Chaoyu Tang <ct...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51694/#review150064
-----------------------------------------------------------
Ship it!
LGTM. Thanks Illya!.
- Chaoyu Tang
On Sept. 22, 2016, 5:46 p.m., Illya Yalovyy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51694/
> -----------------------------------------------------------
>
> (Updated Sept. 22, 2016, 5:46 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
>
>
Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider
should be covered with unit tests
Posted by Illya Yalovyy <ya...@amazon.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51694/
-----------------------------------------------------------
(Updated Sept. 22, 2016, 5:46 p.m.)
Review request for hive, Ashutosh Chauhan, Chaoyu Tang, Naveen Gangam, and Szehon Ho.
Changes
-------
Use "limit 2" for LDAP search queries that are expected to return a single result.
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 (updated)
-----
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
Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider
should be covered with unit tests
Posted by Illya Yalovyy <ya...@amazon.com>.
> On Sept. 21, 2016, 12:50 a.m., Szehon Ho wrote:
> > This looks like a great refactoring to me. This class was always hard to understand, and this makes it a little easier. I'll defer to Chaoyu to the comments.
Thank you!
- Illya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51694/#review149769
-----------------------------------------------------------
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
>
>
Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider
should be covered with unit tests
Posted by Szehon Ho <sz...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51694/#review149769
-----------------------------------------------------------
Ship it!
This looks like a great refactoring to me. This class was always hard to understand, and this makes it a little easier. I'll defer to Chaoyu to the comments.
- Szehon Ho
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
>
>
Re: Review Request 51694: HIVE-14713 LDAP Authentication Provider
should be covered with unit tests
Posted by Illya Yalovyy <ya...@amazon.com>.
-----------------------------------------------------------
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.
Changes
-------
1. Removed trailing spaces
2. Removed usedDN caching
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 (updated)
-----
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