Fixed dynamic group ldap access #23344

alexweirig · 2016-03-17T10:31:51Z

getUserGroups:
Using $userDN instead of $uid to query LDAP
Converting groupDN to group name using API instead of substring
Removing cache processing at the end of the method

getUserGroups: Using $userDN instead of $uid to query LDAP Converting groupDN to group name using API instead of substring Removing cache processing at the end of the method

mention-bot · 2016-03-17T10:31:53Z

By analyzing the blame information on this pull request, we identified @blizzz, @leo-b and @GreenArchon to be potential reviewers

ghost · 2016-03-17T10:46:03Z

@alexweirig

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

MorrisJobke · 2016-03-18T09:52:27Z

cc @owncloud/ldap

blizzz · 2016-03-18T13:02:40Z

apps/user_ldap/group_ldap.php

-		if(isset($this->cachedGroupsByMember[$uid])) {
-			$groups = $this->cachedGroupsByMember[$uid];
-		} else {
-			$groups = array_values($this->getGroupsByMember($uid));


this and following line is still necessary, or did I oversee anything?

Of course it should not overwrite, but append to $groups

@blizzz
I don't know (yet) how your caching is working? Is that cache in the DB or in memory. I have the impression (can't prove it yet) that something goes wrong with the caching because of the cachekeys. At the top of the function the cachekey is generate with the uid and then the cache is read, but at the end of the method, the cachedGroupByMember is written with the userDN (in our case).
I'm trying to put back the cache code in place and just noticed that although the dynamic groups are now working in my code, the "static" groups (based on "uniquemember") are no longer being added to the groups list...

$this->cachedGroupsByMember[$uid]

only exists on run-time. It is a member array of the class instance.

What should not be kept is the caching part, but the fetching of the other groups.

@blizzz
I think I've got the groups working (both dynamic and static) ... but I need to do some more tests and get back on Monday.

@alexweirig sounds great, thank you.

added back the cache processing and fixed

spaces -> tab conversion

alexweirig · 2016-03-21T07:17:06Z

@blizzz �

I committed my latest code changes that have both dynamic and static groups being correctly returned.

I'm still having performance issues but I'm wondering if this might not be a rendering time issue. Maybe it's that multi-select combobox that is causing the problem since it's being loaded with some 50 groups for each user if LDAP returns 500 users, thats 25.000 entries?

alexweirig · 2016-03-21T07:58:46Z

@blizzz

When looking at the server processes and the rendering in the browser, I get the impression that the poor performance is indeed in the Javascript processing of the users list.
Has anybody else reported a similar concern? I know you've got huge customers that have gazillions of users so they should face this problem too?

alexweirig · 2016-03-21T08:10:34Z

@blizzz
Here're some interesting results for the rendering performance:
Safari : +/- 30 seconds to render the page
Chrome: +/- 11 seconds to render the page
Firefox: +/- 11 seconds to render the page

These results are reproducible at will.

Even though FF and Chrome are not fast, they're significantly faster than Safari. I don't have any other browser to compare to. All browsers are updated at their latest version.

ghost · 2016-03-21T08:42:04Z

@alexweirig

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

alexweirig · 2016-03-21T09:55:30Z

@blizzz
I checked how our patched (to support dynamic groups) oc 8.2.1 performs when building the users list:
Safari takes about 11 seconds while Chrome and Firefox take about 5 seconds to build the users list.

So something has changed that performs very badly ... twice as slow on Chrome and FF and about 3 times as slow on Safari.

blizzz · 2016-03-21T12:29:59Z

Yeah, rendering performance is really bad. What also changed compared to older version is that we initially load more users. This will add up to the fun :( Otherwise nothing really changed with the Users page AFAIK.

blizzz · 2016-03-21T13:06:42Z

The code looks good, I'll give it some tests after lunch.

@alexweirig in the past you declared your code MIT. Do you do again? #23344 (comment)

blizzz · 2016-03-21T15:15:39Z

apps/user_ldap/group_ldap.php

-						}
+						$groupName = $this->access->dn2groupname($dynamicGroup['dn'][0]);
+                                                if(is_string($groupName)) {
+                                                        // be sure to never return false if the dn could not be


the indentation is broken (seen in my IDE), please use tabs instead of spaces.

Using tabs now

fixed another line with spaces instead of tab

ghost · 2016-03-21T15:33:12Z

@alexweirig

Thanks a lot for your contribution!
Contributions to the core repo require a signed contributors agreement http://owncloud.org/about/contributor-agreement/

Alternatively you can add a comment here where you state that this contribution is MIT licensed.

Some more details about out pull request workflow can be found here: http://owncloud.org/code-reviews-on-github/

blizzz · 2016-03-21T15:54:13Z

👍 only formatting issues (I'd just fix intention and ship it in the jenkins-PR)

blizzz · 2016-03-21T16:04:39Z

Jenkins PR here: #23450

We need a second reviewer. At least make sure that group memberships are fetched and represented correctly for LDAP users, with and without memory cache confiugred. Maybe @owncloud/qa @PeteSSS @dbenz04 ?

blizzz · 2016-03-23T14:28:33Z

@rperezb can we have some testing?

PVince81 · 2016-03-24T14:49:28Z

@SergioBertolinSG @davitol can you help testing?

SergioBertolinSG · 2016-03-28T11:28:36Z

What are the steps here?
@blizzz

blizzz · 2016-03-30T09:11:48Z

@SergioBertolinSG make sure that reading user groups (e.g. users page, personal page) continues to work. With and without configured memcache.

As plus, get an LDAP setup where dynamic group memberships are existing. @alexweirig perhaps might give pointers, if needed?

@alexweirig a wourd about your code's license or CLA?

ghost · 2016-04-08T19:20:31Z

@SergioBertolinSG @davitol ping

alexweirig · 2016-04-11T06:18:24Z

@blizzz our code is MIT licensed.

in order to use dynamic groups:

create a group in LDAP with objectClass:
top, groupofurls, groupofnames, posixgroup, extensibleObject
set a CN, gidNumber
add the attribute memberURL with a value like
ldap://cn=users,dc=......??one?(some LDAP filter here to filter your users)

Then you can try to add/remove the attribute used in the filter to see if the users is added or removed to/from the group.

alexweirig · 2016-04-11T06:35:26Z

@blizzz When looking at 9.0.1 it seems the contribution did not make it? Am I right?
Do you know what version will include our code?

blizzz · 2016-04-11T09:30:07Z

@alexweirig yes, two reviewers are required to get this merged to master, subsequently we can do the backport for 9.0x. Thus, next possible release is 9.0.2.

alexweirig · 2016-04-11T14:08:58Z

@blizzz OK, so you're one of the reviewers?

alexweirig · 2016-04-20T07:39:44Z

@blizzz Anything new about this? Have you found a second reviewer?

I'd like to upgrade our owncloud but want to do so if I don't have to patch the code again and again on our server.

PVince81 · 2016-04-20T08:26:18Z

@SergioBertolinSG @davitol can you help testing this ?

SergioBertolinSG · 2016-04-20T08:41:49Z

can you help testing this ?

Yes, I'll give it a try.

SergioBertolinSG · 2016-04-20T10:40:53Z

@alexweirig regarding #23344 (comment)

How can I add this steps to an existing openldap server?

alexweirig · 2016-04-20T13:58:18Z

@SergioBertolinSG Thanks for offering your support. Actually I don't think you need to add anything to the server. I would expect that it is enough to define a new group with the following objectClass(es):
groupofurls
nestedgroup
groupofnames (might event not be needed)
posixgroup
extensibleObject

You then add the following attributes:
cn
gidNumber
mail
memberURL

memberURL must have the following format:
ldap://the DN to your users??one?(an LDAP filter to identify a group of users)

this should give you a dynamic group that owncloud would be able to process and show you the list of users matching the filter

We're using a 389DS/IPA from RHEL so some terms tend to change compared to OpenLDAP.
Here's a link to a document with more details ...

HTH

Alex

SergioBertolinSG · 2016-04-20T15:50:18Z

Trying to add ldapadd -Y EXTERNAL -H ldapi:/// -f /etc/ldap/schema/dyngroup.schema
to our ldap server, apparently this bug has appeared:
https://bugs.launchpad.net/ubuntu/+source/openldap/+bug/1392018

ghost · 2016-04-20T16:32:20Z

#23344 (comment) @karlitschek what's the process for MIT-licensed code?

karlitschek · 2016-04-20T16:42:03Z

if stated that the code is MIT licensed as above then this is fine. fully compatible and no cla needed. but this has been done in every pull request. in the long run a signed cla is easier for everyone

ghost · 2016-04-20T18:55:31Z

thanks

alexweirig · 2016-04-21T06:47:55Z

@karlitschek Hello, I'm sending you the CLA to the email address specified on the CLA website.

alexweirig · 2016-05-09T08:17:47Z

Hello,
anything new on integrating the contribution into owncloud code?

Thanks

Alex

PVince81 · 2016-05-09T08:29:31Z

@SergioBertolinSG @davitol @blizzz did you guys eventually managed to test this ?

MorrisJobke · 2016-05-12T07:48:53Z

Let's move this forward in the internal PR because there are additional fixes included: #23450

MorrisJobke · 2016-05-12T07:50:27Z

@alexweirig This is not dead - it was just moved to another branch and therefore another PR :)

blizzz · 2016-05-17T21:14:02Z

@alexweirig the testing-branch was finally merged #23450 \o/ Thank you for the contribution and my sincere apologies it took so long to review and test it!

lock · 2019-08-05T22:01:45Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Fixed dynamic group ldap access

bb1d5fa

getUserGroups: Using $userDN instead of $uid to query LDAP Converting groupDN to group name using API instead of substring Removing cache processing at the end of the method

MorrisJobke added 3 - To Review app:user_ldap labels Mar 18, 2016

blizzz reviewed Mar 18, 2016
View reviewed changes

MorrisJobke added 2 - Developing and removed 3 - To Review labels Mar 18, 2016

alexweirig added 2 commits March 21, 2016 08:06

Fixing group handling

25b643f

added back the cache processing and fixed

fixed possible indention problem

7e46dae

spaces -> tab conversion

blizzz added 3 - To Review Type:Bug sev2-high and removed 2 - Developing labels Mar 21, 2016

blizzz added this to the 9.1-current milestone Mar 21, 2016

blizzz reviewed Mar 21, 2016
View reviewed changes

alexweirig added 2 commits March 21, 2016 16:28

indention problem

e97c67c

Using tabs now

indention problem

c0a15a7

fixed another line with spaces instead of tab

blizzz mentioned this pull request Mar 21, 2016

Problem with dynamic ldap group processing in 9.0.0 #23081

Closed

MorrisJobke closed this May 12, 2016

PVince81 mentioned this pull request Jun 14, 2016

LDAP Feature request: custom Group-Member association LDAP attribute #4547

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 5, 2019

Fixed dynamic group ldap access #23344

Fixed dynamic group ldap access #23344

Conversation

alexweirig commented Mar 17, 2016

mention-bot commented Mar 17, 2016

ghost commented Mar 17, 2016

MorrisJobke commented Mar 18, 2016

blizzz Mar 18, 2016

Choose a reason for hiding this comment

blizzz Mar 18, 2016

Choose a reason for hiding this comment

alexweirig Mar 18, 2016

Choose a reason for hiding this comment

blizzz Mar 18, 2016

Choose a reason for hiding this comment

alexweirig Mar 18, 2016

Choose a reason for hiding this comment

blizzz Mar 18, 2016

Choose a reason for hiding this comment

alexweirig commented Mar 21, 2016

alexweirig commented Mar 21, 2016

alexweirig commented Mar 21, 2016

ghost commented Mar 21, 2016

alexweirig commented Mar 21, 2016

blizzz commented Mar 21, 2016

blizzz commented Mar 21, 2016

blizzz Mar 21, 2016

Choose a reason for hiding this comment

ghost commented Mar 21, 2016

blizzz commented Mar 21, 2016

blizzz commented Mar 21, 2016

blizzz commented Mar 23, 2016

PVince81 commented Mar 24, 2016

SergioBertolinSG commented Mar 28, 2016

blizzz commented Mar 30, 2016

ghost commented Apr 8, 2016

alexweirig commented Apr 11, 2016

alexweirig commented Apr 11, 2016

blizzz commented Apr 11, 2016

alexweirig commented Apr 11, 2016

alexweirig commented Apr 20, 2016

PVince81 commented Apr 20, 2016

SergioBertolinSG commented Apr 20, 2016

SergioBertolinSG commented Apr 20, 2016

alexweirig commented Apr 20, 2016

SergioBertolinSG commented Apr 20, 2016

ghost commented Apr 20, 2016

karlitschek commented Apr 20, 2016

ghost commented Apr 20, 2016

alexweirig commented Apr 21, 2016

alexweirig commented May 9, 2016

PVince81 commented May 9, 2016

MorrisJobke commented May 12, 2016

MorrisJobke commented May 12, 2016

blizzz commented May 17, 2016

lock bot commented Aug 5, 2019