add empty function composeAndStoreDisplayName

[felixboehm]

@jvillafanez @PVince81

Adding an empty function to avoid "missing function" error, which is an issue in https://github.com/owncloud/enterprise/issues/1686

[mention-bot]

@felixboehm, thanks for your PR! By analyzing the history of the files in this pull request, we identified @blizzz, @owncloud-bot and @nickvergessen to be potential reviewers.

[CLAassistant]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

[PVince81]

@return

[PVince81]

I think the description should only say what the function does, so: "Returns the given display name and doesn't store anything as this user is deleted"

[PVince81]

👍 otherwise

@jvillafanez can you give a quick comment in case you know a bit more about this piece ?

[PVince81]

To be backported down to 8.2

[PVince81]

Hold on...

Steps to reproduce:

1. Setup LDAP from the test docker https://github.com/owncloud/administration/tree/master/ldap-testing
2. Populate it with zombies
3. Setup LDAP in OC
4. Login as "zombie300"
5. Create a folder "test"
6. Share "test" with "zombie301"
7. Login as "zombie301"
8. Check that the folder is there
9. Find the ownCloud user id for "zombie300"
10. Delete "zombie300" in LDAP
11. Wait for zombie300 to decompose into remnants occ ldap:show-remnants, might need running cron.php a few times and reducing the TTL for quicker results
12. delete from oc_preferences where appid='user_ldap' and configkey='lastFeatureRefresh';
13. occ files:scan $oc_ldap_id_of_zombie300

Cannot reproduce the issue.

A user marked as deleted cannot log in so the code will never reach processAttributes from checkPassword which is the code that calls composeAndStoreDisplayName. This is because the user is checked against LDAP and an exception makes it bail out.

However... what if the user is marked as deleted but at the same that this specific user was restored in LDAP ? Trying now...

[PVince81]

If I recreated "zombie400", the "isDeleted" flag automatically goes back to false.

Will try again with 8.2, maybe 8.2 behaves differently.

[PVince81]

Boah, restored the wrong zombie... Will retest anyway

[PVince81]

I was able to reproduce on 8.2.9 only, but 9.0.6 is fine:

Steps:

1. Setup LDAP from the test docker https://github.com/owncloud/administration/tree/master/ldap-testing
2. Populate it with zombies
3. Setup LDAP in OC v8.2.9
4. Login as "zombie300"
5. Create a folder "test"
6. Share "test" with zombie301
7. Login as "zombie301"
8. Log out
9. Mark "zombie300" as deleted in the DB but keep the user existing in LDAP: insert into oc_preferences values ($ldap_id_of_zombie300, 'user_ldap', 'isDeleted', 1);
10. occ files:scan $ldap_id_of_zombie301 => no output
11. Check logs:

{"reqId":"zaUHw3yKeiZ9FHdulttl","remoteAddr":"","app":"PHP","message":"Error: Call to undefined method OCA\\user_ldap\\lib\\user\\OfflineUser::composeAndStoreDisplayName() at \/srv\/www\/htdocs\/owncloud\/apps\/user_ldap\/user_ldap.php#385","level":3,"time":"2016-11-29T09:24:52+00:00","method":"--","url":"--"}

So this PR is not needed for master.

@felixboehm Resend for 8.2 only ?

[jvillafanez]

copy&pasting the implementation of the lib/User/User.php file might be a better option

[jvillafanez]

What bothers me a lot is that it seems we're traversing an array containing different classes (User and OfflineUser) without checking the class, which is obviously wrong.

Maybe the solution is already in 9.0.6? I think we should keep this on hold and check other solutions first.

[PVince81]

The solution is likely already on 9.0.6 as it works fine there.

I wasn't able to reverse bisect because "the merge base is bad" or whatever, the bisect process doesn't continue. So might require some manual digging into logs.

[jvillafanez]

Looks like this is what we want:

@@ -382,8 +383,14 @@ class USER_LDAP extends BackendUtility implements \OCP\IUserBackend, \OCP\UserIn
                        }
 
                        $user = $this->access->userManager->get($uid);
-                       $displayName = $user->composeAndStoreDisplayName($displayName, $displayName2);
-                       $this->access->connection->writeToCache($cacheKey, $displayName);
+                       if ($user instanceof User) {
+                               $displayName = $user->composeAndStoreDisplayName($displayName, $displayName2);
+                               $this->access->connection->writeToCache($cacheKey, $displayName);
+                       }
+                       if ($user instanceof OfflineUser) {
+                               /** @var OfflineUser $user*/
+                               $displayName = $user->getDisplayName();
+                       }
                        return $displayName;
                }

Looking for the commit id and PR now

[jvillafanez]

owncloud/core#25598

We'll need to backport that one to 8.2 I think.

[PVince81]

Ok, let's do that then. But I'm not sure we should release 8.2 in advance just for that one commit.