Retrieve user home folder before deleting a user#16830
Conversation
|
@MorrisJobke the correct / used home folder is supposed to be saved in the user preferences table as "homePath" with appid "user_ldap" and this is what is supposed to be returned by getHome for deleted users. |
Then this is somehow broken :( |
|
The value is set when getHome() was called on this user, but not before OC 7.0.5. This does work, but the folder is not being delete - i just reproduced with master. https://github.com/owncloud/core/blob/master/apps/user_ldap/user_ldap.php#L239 has the correct value. The problem is that https://github.com/owncloud/core/blob/master/apps/user_ldap/user_ldap.php#L253 does not return the deleted user any more, which is needed to clean up that stuff. https://github.com/owncloud/core/blob/master/apps/user_ldap/user_ldap.php#L200 returns a OCA\user_ldap\USER_LDAP instance the second time ?! Probably because user values are deleted and thus also the deleted flag. Moving this block https://github.com/owncloud/core/blob/master/apps/user_ldap/user_ldap.php#L257 in front of the userExists check would fix the issue and delete the folder correctly. |
|
Actually, that was also the reason to keep this in an instance variable… |
|
@MorrisJobke I opened a new PR FWIW: #16890 |
|
@blizzz Nevertheless it feels wrong to delete a user first and afterwards fetch the home dir of that user. Considering other backends that doesn't handle this in a way like the LDAP backend does. |
|
@MorrisJobke there you have a point, and how user management works right now is somewhat special. The remove from group actions are also already special. |
There was a problem hiding this comment.
this takes the uid as parameter wrong getHome() i had in mind
There was a problem hiding this comment.
I just was confused ;)
|
@nickvergessen @PVince81 @blizzz Reviewers needed. |
|
@MorrisJobke could you rebase so we can easily test with #16890 which was merged inbetween? |
c3a3518 to
1fe2536
Compare
|
A new inspection was created. |
|
@blizzz Done |
|
Doesn't work with LDAP, the default ~ would be returned. LDAP's deleteUser and getHome needs to be modified as well, because they rely on how this is done here. getHome() will just return false because the user does not exist and $this->homesToKill was not set at the time. Want me to change it accordingly? |
I would say yes, because this is a "feature" of the LDAP implementation because it expects the core to behave wrong. 🙈 🙊 🙉 |
|
@blizzz Ping |
|
@blizzz @nickvergessen @Xenopathic Can I get some reviews for this? Thanks :) |
And opinions ;) |
It's merged :) Do you have time to update the mentioned methods? |
|
moving this to 9.0 |
1fe2536 to
bf429ae
Compare
|
Rebased just because ... |
|
Ha, yeah, sorry, facing deadline. 🙊 |
|
I will give it another review and test. |
|
No, this does not work without adjusting user_ldap either. The deleteUser() method adds the user folder to an array, so that getHome() will succeed and return the correct folder. Currently, getHome() would find out that the user is not existant and throw NoUserException. I.e. user_ldap needs to be touched as well to have this working. |
|
Any update ? How to move this forward ? |
|
Wanted to start with a test, needs to be an integration test, want to have #23832 finished first |
|
Moving to 9.2. Also, there are conflicts. |
bf429ae to
b2c21ba
Compare
|
Funny thing, I fixed the same issue from a completely different context: #26848 |
|
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. |
5 participants
A first try to fix #16588
@blizzz Somehow this doesn't retrieve the correct home folders if a different folder name is used by LDAP. I always get the user home dir with UUID instead of the specified attribute.
cn