Uh oh!
There was an error while loading. Please reload this page.
add option zimbra in ldap app - #4567
Conversation
mention-bot
commented
Apr 27, 2017
@johuder33, thanks for your PR! By analyzing the history of the files in this pull request, we identified @blizzz, @LukasReschke and @leo-b to be potential reviewers. |
@johuder33 thanks for your contribution! May I ask you to look into:
|
johuder33
commented
May 2, 2017
@blizzz Ok, I will modify the code and explain it too. |
I just had a quick look. Wouldn't it be nice to have a general approach for Group Membership with E-Mails. There are some other ObjectClasses which work the same way only a different attribute. We could make group membership attributes configurable with a switch weather it contains a email address or a user id. |
johuder33
commented
May 3, 2017
@go2sh Thank you for your suggestion, right now I working on this, I hope for the next days I can make your suggestion, it would be nice too. I will considere your comment for the next PR. 👍 |
go2sh
commented
May 4, 2017
If you need some help with it, feel free to ask ;) |
dd5fe48 to
5c6a7fcCompareCodecov Report
@@ Coverage Diff @@## master #4567 +/- ##
============================================
- Coverage 54.28% 54.26% -0.03% - Complexity 22099 22106 +7
============================================
Files 1361 1361 Lines 84708 84739 +31 Branches 1322 1322 ============================================
- Hits 45983 45980 -3 - Misses 38725 38759 +34
|
…ra accounts Signed-off-by: Juorder Gonzalez quiñonez <juordergonzalezquinonez@MBPdeJuorder2.home>
5c6a7fc to
da20a58Comparejohuder33
commented
May 5, 2017
@blizzz the changes are done, please add me to the group @nextcloud/ldap, there's not problem with that. Any doubt you have please let me know. |
ChristophWurst
commented
May 8, 2017
FYI looks like your git config is not set to your real email address 😉 That's also why GitHub doesn't recognize it's your commit. |
| if(strtolower($this->access->connection->ldapGroupMemberAssocAttr) === 'zimbramailforwardingaddress') { | ||
| // array containing domains | ||
| $dns = array(); |
There was a problem hiding this comment.
these days you can use short notation $dns = []; :)
| if(strtolower($this->access->connection->ldapGroupMemberAssocAttr) === 'zimbramailforwardingaddress') { | ||
| // array containing domains | ||
| $dns = array(); | ||
| // loop througth array members |
| $dns = array(); | ||
| // loop througth array members | ||
| foreach($members as $mailOfMember) { | ||
| // split the email of each member eg. member_name@zimbra.com |
There was a problem hiding this comment.
intendentation here and the following four lines appears wrong
| // array containing domains | ||
| $dns = array(); | ||
| // loop througth array members | ||
| foreach($members as $mailOfMember) { |
There was a problem hiding this comment.
It seems this can take a lot of time on big groups with many members. Do you think it is possible to use the provided $uid attribute and attempt to look up the user directly?
| ); | ||
| if ($userMatch !== false) { | ||
| // match found so this user is in this group | ||
| $pos = strpos($dynamicGroup['dn'][0], ','); |
There was a problem hiding this comment.
did you test this against groups having a , in their cn?
| // we need to extract the group name | ||
| // so we extracted from cn=myGroup,ou=people,dc=domain,dc=com | ||
| // we extract from position 3 to 7 in the string above, so we have: myGroup | ||
| $groups[] = $membershipGroup; |
There was a problem hiding this comment.
This should be dealt with $this->access->dn2groupname, because we rely on mappings here and avoid name collisions (e.g. with groups from other backends). Adding the group just like this won't have it mapped and further operations against this group might not work.
| $member = $memberParts[0]; | ||
| // replace all '%uid' with the value of $member and get the filter query | ||
| $filter = $this->access->combineFilterWidthAnd(array( |
There was a problem hiding this comment.
Typo in method name, it should be combineFilterWithAnd
blizzz
commented
May 10, 2017
@johuder33 thanks a lot! Please see my review comments, important points are a typo in method name, one worry about group mapping and a question on speeding up one part. I also invited you to the group. |
blizzz
commented
Nov 13, 2017
@johuder33 any update? i move it 14 due to feature freeze. |
MorrisJobke
commented
Mar 7, 2018
No feedback for 10 months -> closing 😢 |
this PR allow us to connect zimbra LDAP with the user_ldap app, now zimbra users can login with their zimbra accounts to Nextcloud.