[stable9] Fixed dynamic group ldap access #24950

Merged
merged 6 commits into from Jun 10, 2016

Projects

None yet

5 participants

@PVince81
Collaborator
PVince81 commented Jun 1, 2016

Backport of #23450 to stable9

Please review and test @owncloud/ldap @owncloud/qa @alexweirig

alexweirig and others added some commits Mar 17, 2016
@alexweirig @PVince81 alexweirig Fixed dynamic group ldap access
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
6d74ef7
@alexweirig @PVince81 alexweirig Fixing group handling
added back the cache processing and fixed
c342dcb
@alexweirig @PVince81 alexweirig fixed possible indention problem
spaces -> tab conversion
f112ece
@blizzz @PVince81 blizzz formatting, white-space changes only
087f005
@PVince81 PVince81 added this to the 9.0.3-current-maintenance milestone Jun 1, 2016
@mention-bot

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

@PVince81
Collaborator
PVince81 commented Jun 1, 2016

Fixes #23081

@nickvergessen nickvergessen and 2 others commented on an outdated diff Jun 1, 2016
apps/user_ldap/group_ldap.php
@@ -530,11 +531,12 @@ public function getUserGroups($uid) {
}
if(isset($this->cachedGroupsByMember[$uid])) {
- $groups = $this->cachedGroupsByMember[$uid];
+ $groups[] = $this->cachedGroupsByMember[$uid];
@nickvergessen
nickvergessen Jun 1, 2016 Contributor

This looks wrong, do you mean array_merge?

@PVince81
PVince81 Jun 2, 2016 Collaborator

@nickvergessen it was like this in the original PR, so probably intentional ?

@alexweirig can you comment (I believe it was your change)

@alexweirig
alexweirig Jun 2, 2016 Contributor

@PVince81
yes, that's what seems to work.

@PVince81
PVince81 Jun 8, 2016 Collaborator

I did some debugging to check the values of the arrays. It seems that in the cases I tested this part of the code never gets executed, probably because the user's groups are already cached.

I suppose it is very rare that this code path will be reached. For that one would have to call getUserGroups for multiple users within the same PHP request.

@nickvergessen is right that this needs an array_merge. I'll make a PR to master.

@PVince81
PVince81 Jun 8, 2016 Collaborator

even the cron jobs don't go there

@alexweirig
Contributor

@PVince81
Hi Vincent,
could you please confirm this is the right approach to test this?

  1. I download owncloud 9.0.2
  2. I apply the patch
  3. Test if everything is OK

or do I need to download a different version of owncloud?

Many thanks in advance

Alex

@alexweirig
Contributor

@PVince81
do we need to apply both the patch and the diff file?

@PVince81
Collaborator
PVince81 commented Jun 2, 2016

@alexweirig you could download the daily stable9 build and then apply the patched from this PR on top of it. This way there is less risk of conflicts.

@alexweirig
Contributor

@PVince81 @blizzz
I downloaded 9.0.2 and applied the .patch file to the code.

I then tested the dynamic group memberships and everything seems to work just as I expected.
The users list is shown with the dynamic group memberships, when I click on a dynamic group, I see the users that belong to the group.

Code is OK for me!

@PVince81
Collaborator
PVince81 commented Jun 2, 2016

@alexweirig thanks, counting as thumbs up 👍

@alexweirig
Contributor

@PVince81
Yes, please 👍

BTW: I can't apply the patch to the daily build because the user_ldap application has completely changed ... The php have moved and the patch doesn't find them anymore.
But wasn't this patch supposed to be for the backporting to 9.0.x?

@alexweirig
Contributor

@PVince81
I also tested the daily stable build without applying the patch (as the patch doesn't work) and everything the dynamic groups are also working as expected.
👍

@PVince81
Collaborator
PVince81 commented Jun 6, 2016

With daily stable you mean the master one ? (9.1)

The patch of this PR is intended to be used for 9.0.x

@alexweirig
Contributor

@PVince81
Agreed, I tested both:

  • daily master thus 9.1 which seems to already include the patched version
  • patch applied to 9.0.x

So this looks good

@PVince81
Collaborator
PVince81 commented Jun 6, 2016

I suggest we merge this then directly fix this piece #24950 (comment) both on master + backport to 9.0

@nickvergessen

@nickvergessen
Contributor

Well fine by me, but I think it needs fixing

@PVince81 PVince81 Use array_merge when reading cached groups members
b37f2e2
@PVince81
Collaborator
PVince81 commented Jun 8, 2016

PR for master for missing array_merge: #25024

I pushed the commit here too: b37f2e2

@alexweirig mind redoing a test on 9.0 with that extra commit ?

@PVince81 PVince81 Add unit test for LDAP multi group caching
fba4460
@PVince81
Collaborator
@PVince81 PVince81 merged commit 9edcdb3 into stable9 Jun 10, 2016

19 of 21 checks passed

core-ci-linux-swift-primary-storage/database=mysql,label=SLAVE Build #57204 failed in 1 hr 15 min
Details
server-master-linux-externals-ci/database=sqlite,external=swift-ceph,label=SLAVE Build #11001 failed in 22 min
Details
cla-bot-core Build #4723 succeeded in 32 sec
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
core-ci-linux-jsunit/database=sqlite,label=SLAVE Build #62684 succeeded in 1 hr 12 min
Details
core-ci-linux/database=mysql,label=SLAVE Build #31600 succeeded in 1 hr 3 min
Details
core-ci-linux/database=oci,label=SLAVE Build #31600 succeeded in 1 hr 35 min
Details
core-ci-linux/database=pgsql,label=SLAVE Build #31600 succeeded in 1 hr 5 min
Details
core-ci-linux/database=sqlite,label=SLAVE Build #31600 succeeded in 40 min
Details
ocs-api-integration-tests-ci Build #11487 succeeded in 24 min
Details
server-master-linux-externals-ci/database=sqlite,external=smb-silvershell,label=SLAVE Build #11001 succeeded in 13 min
Details
server-master-linux-externals-ci/database=sqlite,external=webdav-ownCloud,label=SLAVE Build #11001 succeeded in 21 min
Details
server-master-linux-externals-smb-windows-ext-ci/database=sqlite,external=smb-windows,label=master Build #21897 succeeded in 3 min 13 sec
Details
server-master-linux-php7-ci/database=sqlite,label=SLAVE Build #39886 succeeded in 58 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=litmus,mirallBranch=v2.0.2,slave=SMASH Build #15346 succeeded in 15 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_basicSync@0,mirallBranch=v2.0.2,slave=SMASH Build #15346 succeeded in 59 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_basicSync@1,mirallBranch=v2.0.2,slave=SMASH Build #15346 succeeded in 27 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_shareLink,mirallBranch=v2.0.2,slave=SMASH Build #15346 succeeded in 1 hr 10 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePermissions,mirallBranch=v2.0.2,slave=SMASH Build #15346 succeeded in 54 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePropagationGroups,mirallBranch=v2.0.2,slave=SMASH Build #15346 succeeded in 22 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePropagationInsideGroups,mirallBranch=v2.0.2,slave=SMASH Build #15346 succeeded in 33 min
Details
@PVince81 PVince81 deleted the stable9-fixdynamicldapgroupaccess branch Jun 10, 2016
@MorrisJobke MorrisJobke referenced this pull request in nextcloud/server Jun 10, 2016
Merged

Downstream stable9 16-06-10 #37

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment