-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Fix null pointer exception in user_ldap #25062
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@butonic can you rerun it on the failed environment ? For which user did it return null ? Maybe it was a disabled/obsoleted/stray user ? |
|
It is a test environment. Will see what we can find out... |
|
So far I'm worried that we might be skipping legitimate users. But if those are broken/deleted users then your PR would be fine. |
|
@owncloud/ldap |
|
with the new patch the update completes fine. asked them to classify the users that are logged with this PR |
|
@michaelstingl mind posting the (anonymized) feedback here, if possible ? |
|
@butonic I've got the log from the latest upgrade: Could you check for disabled/obsoleted/stray users? |
|
This PR could also help with #24733 (non-update case) |
9c30d24 to
5134286
Compare
|
squashed and rebased |
|
👍 |
apps/user_ldap/lib/Access.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
returned
5134286 to
c0bbbd6
Compare
|
👍 I agree to merge. Better be more robust here and log errors in case they can help debugging. |
@butonic Have you seen something in their logs that needs to be fixed on their side? |
|
The log shows 144 occurrences where the ldap user manager returns null for a userid. @michaelstingl can they try to check if the uids exist in the oc_ldap_user_mapping table in the owncloud_name column, eg: |
|
Other than this I see a lot of @DeepDiver1975 this happens during the migration ... critical or can be ignored? |
|
also: {"reqId":"...","remoteAddr":"128.176.190.117","app":"PHP","message":"hash_file(\/var\/www\/html\/owncloud_9.0.2\/apps\/user_ldap\/lib\/access.php.orig): failed to open stream: Permission denied at \/var\/www\/html\/owncloud_9.0.2\/lib\/private\/integritycheck\/checker.php#224","level":3,"time":"2016-06-13T12:01:24+00:00","method":"GET","url":"\/settings\/integrity\/rescan?requesttoken=...","user":"adminUser"}indicates a patched 9.0.2 was used. the error will go away with 9.0.3. |
worth a look - I'll take care |
|
@butonic @michaelstingl which dbms is used there? |
|
@DeepDiver1975 they use a galera cluster |
hmmm .... this might explain why the card cannot be selected from the db after insert? |
|
@butonic @DeepDiver1975 Here is a investigation about the LDAP users that caused the trouble:
|
|
@michaelstingl do we have the results of #25062 (comment) |
|
@butonic No, but I will ask… |
Here is the output:
|
|
The mappings are all there ... looks fine. |
|
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. |
This will prevent upgrade experiences like this:
Leaves the qestion why do we get null from the ldap user manager ...
00005599