Skip to content

LDAP fix quota and case, fixes #20043 - #20093

Merged
DeepDiver1975 merged 2 commits into
masterfrom
ldap-fix-quota-and-case
Oct 28, 2015
Merged

LDAP fix quota and case, fixes #20043#20093
DeepDiver1975 merged 2 commits into
masterfrom
ldap-fix-quota-and-case

Conversation

@blizzz

@blizzz blizzz commented Oct 27, 2015

Copy link
Copy Markdown
Contributor

This is a forward port of two issues found only when backporting #18469 to stable8.1 (#19427, #19427 (comment)). This also fixes #20043.

It needs to be backported to 8.2. Please confirm @karlitschek

Please test and review @davitol @eidota @MorrisJobke

@blizzz blizzz added this to the 9.0-current milestone Oct 27, 2015
@ghost ghost added the in progress label Oct 27, 2015
@karlitschek

Copy link
Copy Markdown
Contributor

makes sense. please backport 👍

@eidota

eidota commented Oct 28, 2015

Copy link
Copy Markdown

Working as expected - Quota is delivered and even the mass 'displayName' errors in our logfile are gone 👍

@MorrisJobke

Copy link
Copy Markdown
Contributor

Tested and still works 👍

DeepDiver1975 added a commit that referenced this pull request Oct 28, 2015
@DeepDiver1975
DeepDiver1975 merged commit 0eeee8f into master Oct 28, 2015
@DeepDiver1975
DeepDiver1975 deleted the ldap-fix-quota-and-case branch October 28, 2015 08:31
@ghost ghost removed the in progress label Oct 28, 2015
@MorrisJobke

Copy link
Copy Markdown
Contributor

@blizzz Can you please prepare the backport? Thanks

@eidota

eidota commented Oct 28, 2015

Copy link
Copy Markdown

Contrary to my last comment my logs still show the displayname error:
{"reqId":"aKzMlvmDUGzMFNVLD3lX","remoteAddr":"","app":"PHP","message":"Undefined index: displayname at \/disk1\/webserver\/owncloud\/apps\/user_ldap\/lib\/access.php#696","level":3,"time":"2015-10-28T12:15:08+01:00"}
Before i edited the code with your changes the log showed the same message but with the index 'displayName'.
Is this error a separate issue?

@blizzz

blizzz commented Oct 28, 2015

Copy link
Copy Markdown
Contributor Author

@eidota in that case it seems that the attribute was either not read or not available at all for the record. Can you give me reproduction steps?

@eidota

eidota commented Oct 28, 2015

Copy link
Copy Markdown

The error doesn't occur on demand, e.g. when i reload the users page. I get blocks of 66 entries of this error message about once per hour. Seems to happen when checking the ldap data in the background.
The 66 entries match our 66 users in our testing environment. I checked the ldap-records - the attribute displayName is filled for all of them.

Is this attribute used to fill the 'Full Name' field on the users page? Because they are shown correctly there.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$userRecord[$displayNameAttribute] must be [0]. Thanks to @eidota 's hint, i rechecked that part. I does not cause any trouble, because in dn2ocname this is used just for mapping a group. With that inside, i could cleanup dn2ocname() a bit… So, actually we can just leave the part displayNameAttribute part away, which will save any undefined index warning (which is probably a user record without display name).

@blizzz

blizzz commented Oct 28, 2015

Copy link
Copy Markdown
Contributor Author

I get blocks of 66 entries of this error message about once per hour. Seems to happen when checking the ldap data in the background.

Yes, it's the background job, calling usersInGroup() which only looks for the DN. This can also happen when viewing selecting groups on the Users page. It makes sense to read the other attributes in this case as well. There are more places in group_ldap where we are fetching users like that. But in countUsersInGroup() and inGroup() it is good keep it simple and don't read and do any more than necessary.

So, Todos for me:

  • don't provide the displayname (and asssume it is there) when passing it to dn2ocname
  • usersInGroup() should also read additional meta data about users
  • I will do ^this^ for master in a new PR and backport both together.
  • additonaly i clean up dn2ocname a bit, but only for 9.0 → LDAP: cleanup Access::dn2ocname() #20369

@blizzz

blizzz commented Nov 6, 2015

Copy link
Copy Markdown
Contributor Author

Please see #20124 which fixes remaining tasks

blizzz added a commit that referenced this pull request Nov 10, 2015
 #20093 LDAP fix quota and case
 #20124 remove uselessly used parameter, read all user attributes also when looking up users in groups

fix update quota with known value

fix attribute casing to ensure array keys work

remove uselessly used parameter, read all user attributes also when looking up users in groups
ghost pushed a commit that referenced this pull request Nov 10, 2015
@lock lock Bot locked as resolved and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[8.2] LDAP - Quota not delivered

5 participants