Skip to content

remove uselessly used parameter, read all user attributes also when l… - #20124

Merged
1 commit merged into
masterfrom
postscriptum-20093
Nov 10, 2015
Merged

remove uselessly used parameter, read all user attributes also when l…#20124
1 commit merged into
masterfrom
postscriptum-20093

Conversation

@blizzz

@blizzz blizzz commented Oct 28, 2015

Copy link
Copy Markdown
Contributor

…ooking up users in groups

Two improvements based on @eidota 's findings in #20093, see #20093 (comment) and #20093 (comment) for further explanation.

Basically, with the background job or when looking at an LDAP group with users on the Users page, there should not be any undefined index warnings anymore, but email address and co should be fetched this way just as well.

Since it was found after #20093 was merged this is kind of an P.S. to it, and needs to be backported alongside with that. fyi @karlitschek

Please test and review @davitol @eidota @MorrisJobke

@eidota

eidota commented Oct 30, 2015

Copy link
Copy Markdown

I implemented the changes. The 'missing index: displayname' error is gone now. But i get new errors when the background job is executed.

{"reqId":"...","remoteAddr":"","app":"PHP","message":"fopen(\.../avatar.jpg): failed to open stream: No such file or directory at \.../owncloud\/lib\/private\/files\/storage\/local.php#247","level":3,"time":"2015-10-30T14:30:12+01:00"} {"reqId":"...","remoteAddr":"","app":"PHP","message":"fwrite() expects parameter 1 to be resource, boolean given at \.../owncloud\/lib\/private\/files\/storage\/wrapper\/encryption.php#198","level":3,"time":"2015-10-30T14:30:12+01:00"} {"reqId":"...","remoteAddr":"","app":"PHP","message":"fclose() expects parameter 1 to be resource, boolean given at \.../owncloud\/lib\/private\/files\/storage\/wrapper\/encryption.php#199","level":3,"time":"2015-10-30T14:30:12+01:00"}

I get this error for about 4 people who never logged in (=files directory not existent). This is a bit odd, as i would expect to get this for much more users. Only three out of 66 LDAP-Users logged in until now.

E.g the following error is logged since the change to 8.2 and has an entry for all the users who have not logged in yet:
{"reqId":"...","remoteAddr":"","app":"PHP","message":"opendir(\.../data\/xxx\/files_trashbin\/files): failed to open dir: No such file or directory at \.../owncloud\/lib\/private\/files\/storage\/local.php#96","level":3,"time":"2015-10-30T14:30:04+01:00"}

@blizzz

blizzz commented Nov 6, 2015

Copy link
Copy Markdown
Contributor Author

Hm, avatars are supposed to be fetched only on login actions anyway. But about opening? Okay, we have encryption playing a part here… Anyway I have a look

@blizzz

blizzz commented Nov 6, 2015

Copy link
Copy Markdown
Contributor Author

Indeed, my bad, missed to request only minimal attributes and the avatars indeed were fetched…

@blizzz
blizzz force-pushed the postscriptum-20093 branch from 8b4181a to 133e3fe Compare November 6, 2015 14:22
@blizzz

blizzz commented Nov 6, 2015

Copy link
Copy Markdown
Contributor Author

@eidota ok, could you retry? :)

@eidota

eidota commented Nov 6, 2015

Copy link
Copy Markdown

Implemented the change (getAttributes(true);). Will check our logs on monday.

@blizzz

blizzz commented Nov 6, 2015

Copy link
Copy Markdown
Contributor Author

Awesome, thank you

@eidota

eidota commented Nov 9, 2015

Copy link
Copy Markdown

Everything is fine now. Thank you 👍

@blizzz

blizzz commented Nov 9, 2015

Copy link
Copy Markdown
Contributor Author

@eidota perfect, thank you!

Now, the second reviewer is needed… @PVince81 @davitol @Xenopathic or others?

@davitol

davitol commented Nov 9, 2015

Copy link
Copy Markdown
Contributor

@blizzz @eidota I cannot reproduce the "issue". Those logs do not appear to me. Could you please provide me the steps to reproduce it? 😁

@blizzz

blizzz commented Nov 9, 2015

Copy link
Copy Markdown
Contributor Author

The error messages appears, when the background job runs.

  1. To force it, set background jobs to be controlled by cron (Admin settings). Now you can run cron.php (in owncloud root) as web user, manually.
  2. However there is an interval that will only check every hour the group membership and thus call the method that produces the error. To change this you can edit apps/user_ldap/lib/jobs.php, change the 3600 in getRefreshInterval() to something lower, 10s or so. Then you don't need to wait an hour until you can test it again :D
  3. Now you can run this against the master branch and the fixed one while looking out for error in the log file.

@PVince81

PVince81 commented Nov 9, 2015

Copy link
Copy Markdown
Contributor

@karlitschek backport ? This is an additional fix for #20093 which will be backported too.

@karlitschek

Copy link
Copy Markdown
Contributor

yes. please backport

@davitol

davitol commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

@blizzz I still cannot reproduce it 😞 but, I got through this issue #19240 several times while trying to reproduce this one. If you maybe have some spare minutes to chat about it would be great.

@blizzz

blizzz commented Nov 10, 2015

Copy link
Copy Markdown
Contributor Author

To clarify: step 3 requires running cron.php as web user (sudo -u www-data php -f cron.php)

@davitol

davitol commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

@blizzz, yes i have already run it this way.

@blizzz

blizzz commented Nov 10, 2015

Copy link
Copy Markdown
Contributor Author

@davitol You cannot reproduce it, because it only occurs for setups where the user group association attribute is memberUid. In your case it's just member.

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
@blizzz

blizzz commented Nov 10, 2015

Copy link
Copy Markdown
Contributor Author

@PVince81 backport prepared here: #20446

ghost pushed a commit that referenced this pull request Nov 10, 2015
remove uselessly used parameter, read all user attributes also when l…
@ghost
ghost merged commit 98d3111 into master Nov 10, 2015
ghost pushed a commit that referenced this pull request Nov 10, 2015
@ghost
ghost deleted the postscriptum-20093 branch November 10, 2015 17:55
@davitol

davitol commented Nov 11, 2015

Copy link
Copy Markdown
Contributor

@blizzz Tested again creating a LDAP posixGroup with a user with his memberUid and I still can't reproduce it 👻

@lock lock Bot locked as resolved and limited conversation to collaborators Aug 9, 2019
This pull request was closed.
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.

6 participants