LDAP: do not attempt to process user records without display name, fi… #20804

Merged
merged 1 commit into from Jan 20, 2016

Projects

None yet
@blizzz
Contributor
blizzz commented Nov 27, 2015

…xes fatal error

Steps to reproduce

  1. Have an LDAP server configured in a way, that a user without displayname would get fetched
  2. Go to Users page

Expected behaviour

All valid users appear

Actual behaviour

Page appears to be loading but nothing happens, Log says Call to a member function processAttributes() on a non-object at \/var\/www\/owncloud\/apps\/user_ldap\/lib\/access.php#717

Fix comes with integration test.

8.2 is affected and requires a backport. Please confirm @karlitschek

Please test and review @owncloud/ldap @GreenArchon @pierrejochem @phil-davis @dirkahrnke

@blizzz blizzz added this to the 8.2.2-current-maintenance milestone Nov 27, 2015
@mention-bot

By analyzing the blame information on this pull request, we identified @LukasReschke, @esakol and @MorrisJobke to be potential reviewers

@LukasReschke LukasReschke commented on the diff Nov 28, 2015
...ation/lib/integrationtestbatchapplyuserattributes.php
+ /**
+ * indirectly tests whether batchApplyUserAttributes does it job properly,
+ * when a user without display name is included in the result set from LDAP.
+ *
+ * @return bool
+ */
+ protected function case1() {
+ $result = $this->access->fetchListOfUsers('objectclass=person', 'dn');
+ // on the original issue, PHP would emit a fatal error
+ // – cannot catch it here, but will render the test as unsuccessful
+ return is_array($result) && !empty($result);
+ }
+
+}
+
+require_once(__DIR__ . '/../setup-scripts/config.php');
@LukasReschke
LukasReschke Nov 28, 2015 Member

Such tests with side effects make me shiver so much as users can also request them via web 🙊

@blizzz
blizzz Nov 28, 2015 Contributor

Should not be packed.

@LukasReschke
LukasReschke Nov 28, 2015 Member

Unfortunately. We do. Everything that you put into the tests folder will be packaged. One exception being the one in the root folder 👯 🚀 🙈

@blizzz
blizzz Nov 28, 2015 Contributor

Let's put it this way: at least Apache users should be safe, right. With the .htaccess in front. Anyway, I see this rather as issue in the packaging script than here.

@PVince81
Collaborator

Have an LDAP server configured in a way, that a user without displayname would get fetched

@blizzz if you want me to help testing this I'll need more details how to configure this 😄

@PVince81
Collaborator
@blizzz
Contributor
blizzz commented Nov 30, 2015

Have an LDAP server configured in a way, that a user without displayname would get fetched

@blizzz if you want me to help testing this I'll need more details how to configure this 😄

Have at least one user in your LDAP where the "displayname" attribute is not set. Use an auto-created user filter.

@PVince81
Collaborator
PVince81 commented Dec 4, 2015

My steps:

  1. Run the docker LDAP from https://github.com/owncloud/administration/tree/master/ldap-testing
  2. Populate with zombies with batchCreateUsers.php
  3. Setup OC on this branch
  4. Setup LDAP server, all with defaults (clicked continue)
  5. Using phpldapadmin docker, remove the display name of "zombie1", save (the attribute is deleted)
  6. Go to the users page in ownCloud
  7. Search for the user: the user is not there

It seems to work as expected.

HOWEVER, I'm not able to reproduce the original issue with these steps. I'm not getting the processAttributes() issue, so I probably set it up wrong.

@blizzz you mentioned auto-created filter, are you sure I shouldn't change it to make sure it fetches the users ? Which filter value should I use ? (LDAP noob)

@PVince81
Collaborator
PVince81 commented Dec 4, 2015

@blizzz in the original post you said "configured in a way, that a user without displayname would get fetched", this seems to imply additional steps in the wizard. Can you give me some details, what value should I enter where ?

Thanks.

@blizzz
Contributor
blizzz commented Dec 4, 2015

It should work exactly like this. Maybe you have had some results from LDAP cache? defaults to 10min.

@sebcworks

I was affected by this bug.
I think that if @PVince81 is not able to reproduce the fatal error, it's maybe because the displayName field is not the "User Display Name Field"?

Anyway, I was able to fix it but I think a lot of people may wait for it. This bug leads to some big issues (like disabling a share created from the admin to a LDAP group, having the shared folder removed from all the users' devices).

To tell more about my problem:
My case is special, my first user (the admin) has the same name as the one user in the LDAP database. This LDAP used doesn't have displayName attribute, but before 8.2, I had no problem only the "Database" user was retrieved, not the "LDAP" one.
Starting with 8.2, I had this problem since the function call to retrieve the name from the DN return false (and thus, later, a function is called on false, triggering the error).
For my case, the fix was "easy", I just had to add displayName to my "LDAP" user.

@PVince81
Collaborator
PVince81 commented Jan 8, 2016

@blizzz I set the TTL to 60 seconds.
When checking the users list I see a specific user "userX".
Then in the LDAP server I removed the display name for "userX".
After a minute or so, refreshed the users page.
The user disappeared. But I don't see any `processAttributes" error in the log.

This is on master 88c4cba in my attempt to reproduce the original issue.

@blizzz @DeepDiver1975 blizzz LDAP: do not attempt to process user records without display name, fi…
…xes fatal error
1ed6132
@lckarssen

Just to inform you that this indeed fixes my issue on OC 8.2.2 on Ubuntu 14.04.

@musaa32
musaa32 commented Jan 12, 2016

This fix should be in 8.2.2! It fixes my issue but now I got a Error in my log.

Exception: {"Exception":"OC\\User\\NoUserException","Message":"Backends provided no user object for <username>","Code":0,"Trace":"#0\/owncloud\/apps\/files_sharing\/lib\/cache.php(69): OC\\Files\\Filesystem::initMountPoints('<username>')\n#1\/owncloud\/apps\/files_sharing\/lib\/cache.php(107): OC\\Files\\Cache\\Shared_Cache->getSourceCache('')\n#2\/owncloud\/lib\/private\/files\/view.php(1234): OC\\Files\\Cache\\Shared_Cache->get('')\n#3\/owncloud\/lib\/private\/files\/filesystem.php(861): OC\\Files\\View->getFileInfo('\/', true)\n#4\/owncloud\/apps\/files\/ajax\/list.php(34): OC\\Files\\Filesystem::getFileInfo('\/')\n#5\/owncloud\/lib\/private\/route\/route.php(154) : runtime-created function(1): require_once('\/var\/www\/...')\n#6 [internal function]: __lambda_func(Array)\n#7\/owncloud\/lib\/private\/route\/router.php(282): call_user_func('\\x00lambda_6', Array)\n#8\/owncloud\/lib\/base.php(851): OC\\Route\\Router->match('\/apps\/files\/aja...')\n#9\/owncloud\/index.php(39): OC::handleRequest()\n#10 {main}","File":"\/owncloud\/lib\/private\/files\/filesystem.php","Line":385} 
@blizzz
Contributor
blizzz commented Jan 12, 2016

@musaa32 can you elaborate? It's don't really understand your statement, sorry. This fix here is not merged yet.

@lckarssen thanks for testing :)

@blizzz
Contributor
blizzz commented Jan 12, 2016

@scollin I understand you applied this patch as well and it works for you? Then we'd have two successful tests 🚀

@blizzz
Contributor
blizzz commented Jan 12, 2016

@PVince81

Using phpldapadmin docker, remove the display name of "zombie1", save (the attribute is deleted)

Maybe that does not really delete the attribute but just sets it to an empty value? Never tried with that, but I might remember it was not so easypeasy to fundamentally kill it. But I may mix something up.

@sebcworks

@blizzz No sorry, I didn't test the patch as I don't have a real test version (only the production one), I "fixed" the problem by adding the displayName field to my problematic LDAP user.

But if it is required for the fix to be merged, I can create a test version.

@blizzz
Contributor
blizzz commented Jan 18, 2016

@scollin

But if it is required for the fix to be merged, I can create a test version.

This would be wonderful :)

@Uzzi
Uzzi commented Jan 19, 2016

https://patch-diff.githubusercontent.com/raw/owncloud/core/pull/20804.patch
sudo patch -p1 -i 20804.patch
Now on my ubuntu 14.03 owncloud 8.2 works fine

@blizzz blizzz added 4 - To release and removed 3 - To Review labels Jan 19, 2016
@PVince81
Collaborator

Two people tested it, counting as 👍

The code looks good 👍

@DeepDiver1975 DeepDiver1975 merged commit dd733d8 into master Jan 20, 2016

20 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
core-ci-linux-jsunit/database=sqlite,label=SLAVE Build #56383 succeeded in 3 min 38 sec
Details
core-ci-linux-swift-primary-storage/database=mysql,label=SLAVE Build #49966 succeeded in 14 min
Details
core-ci-linux/database=mysql,label=SLAVE Build #24937 succeeded in 12 min
Details
core-ci-linux/database=oci,label=SLAVE Build #24937 succeeded in 34 min
Details
core-ci-linux/database=pgsql,label=SLAVE Build #24937 succeeded in 12 min
Details
core-ci-linux/database=sqlite,label=SLAVE Build #24937 succeeded in 6 min 5 sec
Details
ocs-api-integration-tests-ci Build #5819 succeeded in 4 min 52 sec
Details
server-master-linux-externals-ci/database=sqlite,external=smb-silvershell,label=SLAVE Build #5680 succeeded in 3 min 4 sec
Details
server-master-linux-externals-ci/database=sqlite,external=swift-ceph,label=SLAVE Build #5680 succeeded in 5 min 5 sec
Details
server-master-linux-externals-ci/database=sqlite,external=webdav-ownCloud,label=SLAVE Build #5680 succeeded in 5 min 17 sec
Details
server-master-linux-externals-smb-windows-ext-ci/database=sqlite,external=smb-windows,label=master Build #5854 succeeded in 4 min 50 sec
Details
server-master-linux-php7-ci/database=sqlite,label=SLAVE Build #32764 succeeded in 1 min 53 sec
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=litmus,mirallBranch=v2.0.2,slave=SMASH Build #10183 succeeded in 4 min 42 sec
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_basicSync@0,mirallBranch=v2.0.2,slave=SMASH Build #10183 succeeded in 13 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_basicSync@1,mirallBranch=v2.0.2,slave=SMASH Build #10183 succeeded in 11 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_shareLink,mirallBranch=v2.0.2,slave=SMASH Build #10183 succeeded in 9 min 57 sec
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePermissions,mirallBranch=v2.0.2,slave=SMASH Build #10183 succeeded in 29 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePropagationGroups,mirallBranch=v2.0.2,slave=SMASH Build #10183 succeeded in 15 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePropagationInsideGroups,mirallBranch=v2.0.2,slave=SMASH Build #10183 succeeded in 18 min
Details
@DeepDiver1975 DeepDiver1975 deleted the fix-ldap-process-user-wo-displayname branch Jan 20, 2016
@blizzz
Contributor
blizzz commented May 9, 2016

Backport does not exist, label info is wrong. Ref #22541 (comment)

@blizzz blizzz added backport-request and removed 8.2.3 labels May 9, 2016
@blizzz
Contributor
blizzz commented May 9, 2016

Backport to stable8.2: #24498

@cccdemon
cccdemon commented May 19, 2016 edited

9.0.2 is also affected ...

i try to enable the LDAP Plugin, and the connection works like a charm, but
if a user tries to login he got an 500 error.

Server configuration
Operating system: Debian/wheezy
Web server: Apache/2.2.22
Database: Mysql
PHP version: PHP 5.4.45-0+deb7u2
ownCloud version (see ownCloud admin page): 9.0.2
Updated from an older ownCloud or fresh install: fresh
ownCloud log (data/owncloud.log):

{"reqId":"pyWOMjO18YY986CW+L1B","remoteAddr":"5.147.253.110","app":"PHP","message":"Call to a member function processAttributes() on a non-object at /srv/webapps/rb-owncloud/production/releases/1/public/apps/user_ldap/lib/access.php#733","level":3,"time":"2016-05-19T15:57:31+00:00","method":"POST","url":"/","user":"--"}

Just user_ldap

mysql oc_ldap_user_mapping
mysql> select * from oc_ldap_user_mapping;
| ldap_dn | owncloud_name | directory_uuid |
+-----------------------------------------------+-------------------+-------------------+
| cn=firstname lastname,cn=users,dc=xxx,dc=xxxx | firstname.lastname | firstname.lastname |
+-----------------------------------------------+-------------------+-------------------+
1 row in set (0.00 sec)

xxx = our domain stuff, firstname and lastname are your anon data

BaseDN Test works like a charm and retrives 70 entries
Groups are shown and usertest is also working

But login crashes with the error message in the log show above.

Any ideas?

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