You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tamas Mate (Code Review)" <ge...@cloudera.org> on 2021/08/19 10:10:09 UTC
[Impala-ASF-CR] IMPALA-10822: Allow multiple group retuns for LDAP Search
Tamas Mate has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17794
Change subject: IMPALA-10822: Allow multiple group retuns for LDAP Search
......................................................................
IMPALA-10822: Allow multiple group retuns for LDAP Search
This commit updates the LdapSearch method to allow multiple results.
LdapSearch method was written generally and used for both user and group
searches. While it is reasonable to fail authentication requests where
the LDAP Search returns more than one user this should not be the case
for group searches, as a user can be part of multiple groups matching
the search criteria.
This commit moves the logic that checks the number of results to the
specific user and group search methods.
Testing:
- Updated the unit test to cover this use-case
Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
---
M be/src/util/ldap-search-bind.cc
M be/src/util/ldap-simple-bind.cc
M be/src/util/ldap-util.cc
M be/src/util/ldap-util.h
M fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
M fe/src/test/resources/users.ldif
6 files changed, 42 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/17794/2
--
To view, visit http://gerrit.cloudera.org:8080/17794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
Gerrit-Change-Number: 17794
Gerrit-PatchSet: 2
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10822: Allow multiple group returns for LDAP Search
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17794 )
Change subject: IMPALA-10822: Allow multiple group returns for LDAP Search
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/17794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
Gerrit-Change-Number: 17794
Gerrit-PatchSet: 7
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Aug 2021 15:28:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10822: Allow multiple group returns for LDAP Search
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17794 )
Change subject: IMPALA-10822: Allow multiple group returns for LDAP Search
......................................................................
Patch Set 7:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7428/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/17794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
Gerrit-Change-Number: 17794
Gerrit-PatchSet: 7
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Aug 2021 15:28:58 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10822: Allow multiple group returns for LDAP Search
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17794 )
Change subject: IMPALA-10822: Allow multiple group returns for LDAP Search
......................................................................
IMPALA-10822: Allow multiple group returns for LDAP Search
This commit updates the LdapSearch method to allow multiple results by
refactoring the condition that checks the number of results. The
condition was removed from the method and it is now the caller's
responsibility to decide if the returned number of results are correct.
The change was necessary, because the LdapSearch method was written
too generally and used for both user and group searches. While it is
reasonable to fail authentication requests where the LDAP Search
returns more than one user this should not be the case for group
searches, as a user can be part of multiple groups matching the search
criteria.
Testing:
- Updated the unit test to cover this use-case
Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
Reviewed-on: http://gerrit.cloudera.org:8080/17794
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/util/ldap-search-bind.cc
M be/src/util/ldap-simple-bind.cc
M be/src/util/ldap-util.cc
M be/src/util/ldap-util.h
M fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
M fe/src/test/resources/users.ldif
6 files changed, 60 insertions(+), 35 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/17794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
Gerrit-Change-Number: 17794
Gerrit-PatchSet: 8
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10822: Allow multiple group returns for LDAP Search
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/17794 )
Change subject: IMPALA-10822: Allow multiple group returns for LDAP Search
......................................................................
Patch Set 5: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17794/4/be/src/util/ldap-util.cc
File be/src/util/ldap-util.cc:
http://gerrit.cloudera.org:8080/#/c/17794/4/be/src/util/ldap-util.cc@a204
PS4, Line 204:
:
> Just to clarify, you mean >0? Because this commit tries to unblock
> the >=1 scenario for group searches.
I mean returning in the < 1 case - this would mean 0 matches or error in case of -1.
--
To view, visit http://gerrit.cloudera.org:8080/17794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
Gerrit-Change-Number: 17794
Gerrit-PatchSet: 5
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Aug 2021 14:37:47 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10822: Allow multiple group retuns for LDAP Search
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17794 )
Change subject: IMPALA-10822: Allow multiple group retuns for LDAP Search
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17794/2/be/src/util/ldap-search-bind.cc
File be/src/util/ldap-search-bind.cc:
http://gerrit.cloudera.org:8080/#/c/17794/2/be/src/util/ldap-search-bind.cc@105
PS2, Line 105: LOG(WARNING) << "LDAP search failed with base DN=" << FLAGS_ldap_user_search_basedn.c_str()
line too long (95 > 90)
http://gerrit.cloudera.org:8080/#/c/17794/2/be/src/util/ldap-search-bind.cc@147
PS2, Line 147: LOG(WARNING) << "LDAP search failed with base DN=" << FLAGS_ldap_user_search_basedn.c_str()
line too long (97 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/17794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
Gerrit-Change-Number: 17794
Gerrit-PatchSet: 2
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Aug 2021 10:11:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10822: Allow multiple group retuns for LDAP Search
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17794 )
Change subject: IMPALA-10822: Allow multiple group retuns for LDAP Search
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9325/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
Gerrit-Change-Number: 17794
Gerrit-PatchSet: 2
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 19 Aug 2021 10:33:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10822: Allow multiple group returns for LDAP Search
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17794 )
Change subject: IMPALA-10822: Allow multiple group returns for LDAP Search
......................................................................
Patch Set 7: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/17794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
Gerrit-Change-Number: 17794
Gerrit-PatchSet: 7
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Aug 2021 21:37:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10822: Allow multiple group returns for LDAP Search
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/17794 )
Change subject: IMPALA-10822: Allow multiple group returns for LDAP Search
......................................................................
IMPALA-10822: Allow multiple group returns for LDAP Search
This commit updates the LdapSearch method to allow multiple results by
refactoring the condition that checks the number of results. The
condition was removed from the method and it is now the caller's
responsibility to decide if the returned number of results are correct.
The change was necessary, because the LdapSearch method was written
too generally and used for both user and group searches. While it is
reasonable to fail authentication requests where the LDAP Search
returns more than one user this should not be the case for group
searches, as a user can be part of multiple groups matching the search
criteria.
Testing:
- Updated the unit test to cover this use-case
Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
---
M be/src/util/ldap-search-bind.cc
M be/src/util/ldap-simple-bind.cc
M be/src/util/ldap-util.cc
M be/src/util/ldap-util.h
M fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
M fe/src/test/resources/users.ldif
6 files changed, 60 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/17794/6
--
To view, visit http://gerrit.cloudera.org:8080/17794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
Gerrit-Change-Number: 17794
Gerrit-PatchSet: 6
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10822: Allow multiple group returns for LDAP Search
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17794 )
Change subject: IMPALA-10822: Allow multiple group returns for LDAP Search
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9384/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
Gerrit-Change-Number: 17794
Gerrit-PatchSet: 6
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Aug 2021 11:59:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10822: Allow multiple group returns for LDAP Search
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/17794 )
Change subject: IMPALA-10822: Allow multiple group returns for LDAP Search
......................................................................
IMPALA-10822: Allow multiple group returns for LDAP Search
This commit updates the LdapSearch method to allow multiple results.
LdapSearch method was written generally and used for both user and group
searches. While it is reasonable to fail authentication requests where
the LDAP Search returns more than one user this should not be the case
for group searches, as a user can be part of multiple groups matching
the search criteria.
This commit moves the logic that checks the number of results to the
specific user and group search methods.
Testing:
- Updated the unit test to cover this use-case
Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
---
M be/src/util/ldap-search-bind.cc
M be/src/util/ldap-simple-bind.cc
M be/src/util/ldap-util.cc
M be/src/util/ldap-util.h
M fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
M fe/src/test/resources/users.ldif
6 files changed, 42 insertions(+), 34 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/17794/4
--
To view, visit http://gerrit.cloudera.org:8080/17794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
Gerrit-Change-Number: 17794
Gerrit-PatchSet: 4
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-10822: Allow multiple group returns for LDAP Search
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17794 )
Change subject: IMPALA-10822: Allow multiple group returns for LDAP Search
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9368/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
Gerrit-Change-Number: 17794
Gerrit-PatchSet: 5
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Aug 2021 13:36:25 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10822: Allow multiple group returns for LDAP Search
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/17794 )
Change subject: IMPALA-10822: Allow multiple group returns for LDAP Search
......................................................................
Patch Set 6: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17794/4/be/src/util/ldap-util.cc
File be/src/util/ldap-util.cc:
http://gerrit.cloudera.org:8080/#/c/17794/4/be/src/util/ldap-util.cc@a204
PS4, Line 204:
:
> Ah, I see, I think that is not needed here, if anything would happen with t
Yes, ldap_first_message will handle it, it just that we'll log a less informative error message in some cases, e.g. when LdapSimpleBind::CheckGroupMembership calls LdapSearchObject.
But it's ok like this, +2 from my side.
--
To view, visit http://gerrit.cloudera.org:8080/17794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
Gerrit-Change-Number: 17794
Gerrit-PatchSet: 6
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Aug 2021 14:08:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10822: Allow multiple group returns for LDAP Search
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/17794 )
Change subject: IMPALA-10822: Allow multiple group returns for LDAP Search
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/17794/4/be/src/util/ldap-search-bind.cc
File be/src/util/ldap-search-bind.cc:
http://gerrit.cloudera.org:8080/#/c/17794/4/be/src/util/ldap-search-bind.cc@106
PS4, Line 106: " entries "
: << "have been found.";
We could add that we expected a unique result. I'm not familiar with this part of the code so it may be obvious for the users but for me it would be strange at first that the search failed although there are several results found.
http://gerrit.cloudera.org:8080/#/c/17794/4/be/src/util/ldap-search-bind.cc@149
PS4, Line 149: " entries have been found.";
Same as on L107.
http://gerrit.cloudera.org:8080/#/c/17794/4/be/src/util/ldap-simple-bind.cc
File be/src/util/ldap-simple-bind.cc:
http://gerrit.cloudera.org:8080/#/c/17794/4/be/src/util/ldap-simple-bind.cc@169
PS4, Line 169: (auto &
For me explicitly naming the type (etc. const string&) is cleaner and more readable but I realise that it is hugely just personal preference.
--
To view, visit http://gerrit.cloudera.org:8080/17794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
Gerrit-Change-Number: 17794
Gerrit-PatchSet: 4
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Aug 2021 10:53:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10822: Allow multiple group returns for LDAP Search
Posted by "Tamas Mate (Code Review)" <ge...@cloudera.org>.
Tamas Mate has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/17794 )
Change subject: IMPALA-10822: Allow multiple group returns for LDAP Search
......................................................................
IMPALA-10822: Allow multiple group returns for LDAP Search
This commit updates the LdapSearch method to allow multiple results by
refactoring the condition that checks the number of results. The
condition was removed from the method and it is now the caller's
responsibility to decide if the returned number of results are correct.
The change was necessary, because the LdapSearch method was written
too generally and used for both user and group searches. While it is
reasonable to fail authentication requests where the LDAP Search
returns more than one user this should not be the case for group
searches, as a user can be part of multiple groups matching the search
criteria.
Testing:
- Updated the unit test to cover this use-case
Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
---
M be/src/util/ldap-search-bind.cc
M be/src/util/ldap-simple-bind.cc
M be/src/util/ldap-util.cc
M be/src/util/ldap-util.h
M fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
M fe/src/test/resources/users.ldif
6 files changed, 58 insertions(+), 32 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/17794/5
--
To view, visit http://gerrit.cloudera.org:8080/17794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
Gerrit-Change-Number: 17794
Gerrit-PatchSet: 5
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10822: Allow multiple group returns for LDAP Search
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/17794 )
Change subject: IMPALA-10822: Allow multiple group returns for LDAP Search
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/17794/4/be/src/util/ldap-util.cc
File be/src/util/ldap-util.cc:
http://gerrit.cloudera.org:8080/#/c/17794/4/be/src/util/ldap-util.cc@a204
PS4, Line 204:
:
Maybe we could keep this logic with nr_of_entries >= 1. This would lead to a clearer warning in the log if no entries are found, for example if all the messages as references.
http://gerrit.cloudera.org:8080/#/c/17794/4/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java
File fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java:
http://gerrit.cloudera.org:8080/#/c/17794/4/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java@71
PS4, Line 71: TEST_USER_GROUP
This comment looks stale.
http://gerrit.cloudera.org:8080/#/c/17794/4/fe/src/test/java/org/apache/impala/customcluster/LdapSearchBindImpalaShellTest.java@75
PS4, Line 75: {1}
Shouldn't we create a new test instead of changing this one? So that both one and the two group case would be tested.
--
To view, visit http://gerrit.cloudera.org:8080/17794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
Gerrit-Change-Number: 17794
Gerrit-PatchSet: 4
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Aug 2021 11:15:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10822: Allow multiple group returns for LDAP Search
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17794 )
Change subject: IMPALA-10822: Allow multiple group returns for LDAP Search
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9354/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17794
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27eaf717985207e36d4fa47a2af757d06544474e
Gerrit-Change-Number: 17794
Gerrit-PatchSet: 4
Gerrit-Owner: Tamas Mate <tm...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Aug 2021 09:54:21 +0000
Gerrit-HasComments: No