unify count filters and introduce display name attribute detection #11837

blizzz · 2014-10-29T12:34:28Z

This fixes #11328 and introduces some bigger changes.

we introduce the detection of the display name attribute, because on not so few LDAP setups "displayname" is not present. The detectors first checks whether there are entries having "displayname" according to the current user filter. If not it checks "cn". "cn", short for "common name" should be available everywhere. If necessary we can extend it with "uid" and "samaccountname" although those are not meant to be displayed to users. This also ensures that users will be able to log in since a present display name is a condition.
The wizard and the user backend created different filters for counting users by there own. They have been consolidated in Access and are used by both. The created filter also takes display name into account, and the number of LDAP users in user managament and wizard is now the same.
We now have three detectors, one for the email attribute and one for the group member association was already there. They were called in several places, now I cleaned it up so that they are always one run just before a count action. For setups using the totally assisted mode it means that the detectors will run anyway. They will also run in experienced mode, when "test filter" is clicked. To make sure they run even if the button is not clicked, we watch the state and run them ONCE when the user respectively group tab is left. All in all that saves some initiated and cancelled calls.
The wizard backend was improved in a way that for certain count operations a flag can be set limits the query to just 1. We need this to verifiy whether an attribute does still work with the current filter (email, displayname) where it is not necessary to count every single object, one presence is enough. This is done to avoid another full detection run. Also, when determining the display name this limit is used. I.e. those actions are faster now. It incorporates a fix where the limit was not respected properly in the count method (no side effects as it was not used anywhere else).
it incorporates a fix where the email attribute was detected and presented in the user interface, but not saved.

While testing i have seen some disbehaviour in the wizard which however is also present in master. Those I will investigated independtly from this PR.

Call for comments, tests and reviews @craigpg @jnfrmarks @PVince81 @Xenopathic @butonic

PVince81 · 2014-10-29T16:36:01Z

apps/user_ldap/js/settings.js

@@ -151,8 +151,10 @@ var LdapWizard = {
 	ajaxRequests: {},

 	ajax: function(param, fnOnSuccess, fnOnError, reqID) {
-		if(reqID !== undefined) {
+		if(typeof reqID !== 'undefined') {


You can also use _.isUndefined(reqID)

PVince81 · 2014-10-29T16:40:14Z

Soooo much code for detectors. I hope that one day we'll be able to refactor these JS classes and make them a bit easier to understand.

PVince81 · 2014-10-29T16:41:42Z

apps/user_ldap/js/ldapFilter.js

-	} else if (this.target === 'Group') {
-		LdapWizard.countGroups(doneCallback);
-	}
+	var beforeUpdateCountDone = $.Deferred();


You could make beforeUpdateCount create the $.Deferred() instead of here.
This way you only need to write $.when(this.beforeUpdateCount()).done(...)

It would make it more readable.

PVince81 · 2014-10-29T16:54:14Z

I'm not sure how to test this with openLDAP ?
When using master with your zombie script (batchCreateUsers) all users already have a "displayName" attribute and it seems to be detected on master already.

Should I try and create users that have "cn" as display name and see whether it's detected on this branch ?

blizzz · 2014-10-29T18:15:04Z

@PVince81 one way is to manually set the display name to something unexisting, and trigger a count action again ("Test Filter" button on User filter in experienced mode, or by changing the user list filter). Displayname should be found correctly again.

To test for cn, create a users which is not of inetOrgPerson objectclass to avoid display name and configure the user list filter to only find them.

blizzz · 2014-10-29T18:31:35Z

@PVince81 thanks for reviewing and your suggestions!

ghost · 2014-11-04T13:46:58Z

🚀 Test PASSed. 🚀
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/1944/
🚀 Test PASSed. 🚀

blizzz · 2014-11-04T16:02:27Z

@PVince81 satisfied?

others: More testers or reviewers? 💐

blizzz · 2014-11-11T12:35:03Z

cc @MorrisJobke

MorrisJobke · 2014-11-11T13:19:03Z

The display name is detected successfully. But I don't know how to properly review this.

I would give this a thumbs up, but I can't decide on the quality of the code. 👍

LukasReschke · 2014-11-19T12:14:51Z

As discussed with @blizzz via IRC I'll review this (in exchange for 🍻) ;-)

LukasReschke · 2014-11-19T12:16:35Z

apps/user_ldap/js/settings.js

+	/**
+	 * called after detectors have run
+	 * @callback runDetectorsCallback
+	 */


Seems somewhat lonely and without context to me that comment.

That's how JS doc works :) This defines the runDetectorsCallback used in the following method.

Okay. Good to know. Black magic that is. ;-)

blizzz · 2014-11-19T12:17:30Z

Best currency that is!

LukasReschke · 2014-11-19T12:20:43Z

apps/user_ldap/lib/access.php

@@ -1168,6 +1170,18 @@ private function getFilterPartForSearch($search, $searchAttributes, $fallbackAtt
 	}

 	/**
+	 * returns the filter used for counting users
+	 */


* @return string

LukasReschke · 2014-11-19T12:50:18Z

Works, but I wonder why it shows "0 users" found instead of my user count at the setup? - The users are shown correctly on the users page.

Recorded video of setup at https://s3.owncloud.com/owncloud/public.php?service=files&t=0a1b1a586e577884c49e25fa6710762f

If I access the page after the setup the correct numbers of users (7) is shown, but not before. This is kinda confusing.

blizzz · 2014-11-20T17:28:57Z

Hm, maybe it could make sense to trigger as user count instead of a group one in that case.

… its presence

…red from only 2 places. more reliable structure and flow, saves requests

…ching entries but limit it to 1 in order to make it faster

blizzz · 2014-11-21T15:25:49Z

Needed adjustments for current master and got a fix when paging size is set to 0.

@MorrisJobke @LukasReschke @PVince81 like to revisit?

LukasReschke · 2014-11-21T15:31:22Z

I actually don't really like to do it. But I'll do it anyways - you know the deal ;-)

blizzz · 2014-11-21T15:34:20Z

yeah, yes you break it anyway and even expect 🍻 for it. But that's 👌

LukasReschke · 2014-11-21T15:36:51Z

Port is a number and therefore not shown correctly:

LukasReschke · 2014-11-21T15:37:23Z

Rest works - port should get fixed though, might also exist on master. 👍

blizzz · 2014-11-21T15:41:00Z

@LukasReschke yes, port field needs to be addressed on master, but a different issue (and the same without this PR). Thx for your thumb!

…n with xp'ed mode (would be a regression otherwise)

scrutinizer-notifier · 2014-11-22T00:01:19Z

The inspection completed: 16 new issues, 5 updated code elements

LukasReschke · 2014-11-24T13:31:16Z

@blizzz This will most likely not work properly. Solution is to create a new branch, cherry-pick the commits to it, and create a new PR to have CI run over it.

MorrisJobke · 2014-11-24T13:47:09Z

@blizzz This will most likely not work properly. Solution is to create a new branch, cherry-pick the commits to it, and create a new PR to have CI run over it.

Checkout the branch and then directly create a new branch (no cherry-picking needed) just clone the branch ;)

MorrisJobke · 2014-11-24T13:47:38Z

I will take care of this.

MorrisJobke · 2014-11-24T13:49:03Z

Placeholder PR is in #12388

MorrisJobke · 2014-11-24T15:01:45Z

From #12388 (comment)
🚀 Test PASSed. 🚀
Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/3135/
🚀 Test PASSed. 🚀

blizzz · 2014-11-24T15:37:12Z

Now I need one more thumb :) Since, after @MorrisJobke 's one there were more changes, adapting to master. 🐫

MorrisJobke · 2014-11-24T15:38:36Z

@blizzz I know ... but I need time to recover from all the issues that rain on me 🌈

MorrisJobke · 2014-11-24T15:39:16Z

That's also the reason why I assigned it to myself ;)

MorrisJobke · 2014-11-24T17:13:00Z

I tested this. The user count on the filter page is updated correct. (I dropped the user name for one user and it got updated) 👍 as it works as expected (login for ldap users work and the login for the one without displayname doesn't work)

unify count filters and introduce display name attribute detection

blizzz · 2014-11-24T23:51:23Z

Thanks @MorrisJobke ! 🌟

@karlitschek backport? It fixes #11328 but also adds required functionality and thus it's a big PR. @jnfrmarks rated it with high severity.

karlitschek · 2014-11-25T12:38:35Z

Backport is fine in this case.

jnfrmarks · 2014-11-25T14:09:55Z

@blizzz @craigpg

What is the new functionality? Is there a lot of risk to LDAP with this change?

blizzz · 2014-11-25T18:56:15Z

Detecting display name attribute is new. There are not so small changes to the LDAP wizard code. I think we tested it good, but the amount of changes makes it potentially risky imho.

PVince81 reviewed Oct 29, 2014
View reviewed changes

This was referenced Nov 10, 2014

LDAP wizard user experience #12060

Closed

Can't login with LDAP users even if the wizard says, that he found users #12059

Closed

blizzz mentioned this pull request Nov 19, 2014

LDAP: Number of users on user filter tab does not match number of users showing up on oC user page #11328

Closed

LukasReschke added the 3 - To Review label Nov 19, 2014

LukasReschke reviewed Nov 19, 2014
View reviewed changes

LukasReschke added this to the 2014-sprint-08-current milestone Nov 19, 2014

blizzz added 5 commits November 20, 2014 18:31

detect user display name attribute and return user count depending on…

cb3af1d

… its presence

consolidate user count filter in wizard and user back end

f725cc6

detectors (email, displayname..) are now started in one place, trigge…

71944a5

…red from only 2 places. more reliable structure and flow, saves requests

to reassure that selected attributes still work, do not count all mat…

f9b4f5f

…ching entries but limit it to 1 in order to make it faster

a corrected email attribute needs to be saved, not only returned

4a3fe42

blizzz removed the 2 - Developing label Nov 21, 2014

make scrutinizer happier and always count users on assisted mode, eve…

9aef83b

…n with xp'ed mode (would be a regression otherwise)

MorrisJobke mentioned this pull request Nov 24, 2014

Jenkins fix 11328 #12388

Merged

MorrisJobke self-assigned this Nov 24, 2014

MorrisJobke added a commit that referenced this pull request Nov 24, 2014

Merge pull request #11837 from owncloud/fix-11328

02095d4

unify count filters and introduce display name attribute detection

MorrisJobke merged commit 02095d4 into master Nov 24, 2014

MorrisJobke deleted the fix-11328 branch November 24, 2014 17:13

blizzz mentioned this pull request Feb 17, 2015

Changing the user / group raw filter in the LDAP wizard doesn't update the user / group count #14161

Closed

MorrisJobke removed their assignment Oct 5, 2015

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

unify count filters and introduce display name attribute detection #11837

unify count filters and introduce display name attribute detection #11837

Conversation

blizzz commented Oct 29, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Oct 29, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Oct 29, 2014

blizzz commented Oct 29, 2014

blizzz commented Oct 29, 2014

ghost commented Nov 4, 2014

blizzz commented Nov 4, 2014

blizzz commented Nov 11, 2014

MorrisJobke commented Nov 11, 2014

LukasReschke commented Nov 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blizzz commented Nov 19, 2014

Choose a reason for hiding this comment

LukasReschke commented Nov 19, 2014

blizzz commented Nov 20, 2014

blizzz commented Nov 21, 2014

LukasReschke commented Nov 21, 2014

blizzz commented Nov 21, 2014

LukasReschke commented Nov 21, 2014

LukasReschke commented Nov 21, 2014

blizzz commented Nov 21, 2014

scrutinizer-notifier commented Nov 22, 2014

LukasReschke commented Nov 24, 2014

MorrisJobke commented Nov 24, 2014

MorrisJobke commented Nov 24, 2014

MorrisJobke commented Nov 24, 2014

MorrisJobke commented Nov 24, 2014

blizzz commented Nov 24, 2014

MorrisJobke commented Nov 24, 2014

MorrisJobke commented Nov 24, 2014

MorrisJobke commented Nov 24, 2014

blizzz commented Nov 24, 2014

karlitschek commented Nov 25, 2014

jnfrmarks commented Nov 25, 2014

blizzz commented Nov 25, 2014