Add force option to delete user home folder

[sharidas]

Add a force option to the user delete command
in order to delete the users home folder if
exists.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

Add force option for the user:delete command, to delete the home folder of the user if available.

Related Issue

https://github.com/owncloud/enterprise/issues/2969

Motivation and Context

Its been noticed that when users upgrade from 9.1.5 to 10 of core, the accounts table does not have the user details and neither the home entry added to it. Now if the backend had deleted a user before upgrade then oC admin will not be able to delete the oc user mapped to the ldap backend. Because the entry is missing in the accounts table. Under such condition it would be handy to delete the home folder of those users (i.e, for missing backend users).

How Has This Been Tested?

• Install oC 9.1.5
• Create LDAP users
• Now enable user_ldap app
• Configure it with the oC as admin
• Make sure the LDAP users are synced. You can see the result in the Users page.
• Now try to login to few users who are mapped with the LDAP.
• Delete the users in the LDAP.
• Upgrade to oC 10.4.0
• Now try to run user:delete with uid and --force, where uid is the user unavailable in the LDAP or say the deleted one.
• The home folder ( i.e, the remains ) should be deleted.

Screenshots (if appropriate):

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Database schema changes (next release will require increase of minor version instead of patch)
• Breaking change (fix or feature that would cause existing functionality to change)
• Technical debt
• Tests only (no source changes)

Checklist:

• Code changes
• Unit tests added
• Acceptance tests added
• Documentation ticket raised:
• Changelog item, see TEMPLATE

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[jvillafanez]

I don't think the approach is good.

• The --force option is meant to bypass the safety checks there could be to prevent the command from running. Removing the home folder is a consequence of removing the user, but not the goal of the --force option
• The usual approach is more streamlined: you have actions A, B and C, and B (or A) could fail, preventing C from running. In this case, the --force option would need to ensure that A, B and C actions runs always, even though it could be unsafe

From our case, there is only one action, which is the $user->delete(), with a safety check, which is that the user must exists.
The user->delete() should have several actions which should involve deleting the home directory, deleting the shares, deleting user preferences, etc, These actions should be replicated in order to ensure the user is properly deleted, as if the $user->delete() has been called. If there is some event being triggered, the same event should be triggered in this case.

[codecov]

Codecov Report

Merging #37103 into master will increase coverage by 0.02%.
The diff coverage is 79.27%.

@@             Coverage Diff              @@
##             master   #37103      +/-   ##
============================================
+ Coverage     64.52%   64.54%   +0.02%     
- Complexity    19166    19207      +41     
============================================
  Files          1266     1267       +1     
  Lines         74938    75051     +113     
  Branches       1331     1331              
============================================
+ Hits          48356    48445      +89     
- Misses        26190    26214      +24     
  Partials        392      392              

Flag	Coverage Δ	Complexity Δ
#javascript	54.14% <ø> (ø)	0.00 <ø> (ø)
#phpunit	65.70% <79.27%> (+0.02%)	19207.00 <45.00> (+41.00)

Impacted Files	Coverage Δ	Complexity Δ
apps/dav/lib/HookManager.php	53.33% <0.00%> (-0.91%)	17.00 <0.00> (ø)
lib/private/User/DeletedUser.php	73.07% <73.07%> (ø)	33.00 <33.00> (?)
lib/private/User/Manager.php	86.16% <87.50%> (-0.21%)	57.00 <9.00> (+3.00)	⬇️
apps/dav/lib/CardDAV/SyncService.php	52.41% <100.00%> (+6.05%)	28.00 <3.00> (+3.00)
core/Command/User/Delete.php	100.00% <100.00%> (ø)	6.00 <0.00> (+1.00)
lib/private/Files/Storage/Common.php	84.70% <0.00%> (ø)	138.00% <0.00%> (ø%)
...ederatedfilesharing/lib/FederatedShareProvider.php	62.60% <0.00%> (+0.18%)	89.00% <0.00%> (+1.00%)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a29b3d...f093886. Read the comment docs.

[micbar]

@jvillafanez give it a new try with a fake user

[jvillafanez]

Apart from the unit tests, there are a couple of problems to be fixed:

• user ldap mappings won't be cleaned. We require a "postDelete" listener in the user_ldap app to do the job. Note that the app will likely need to remove that mapping regardless of where the uid comes from (we can't rely on any information about the user backend)
• cardDav sync table will still hold some user information. It tries to get the user object from the user manager, but the user is missing (https://github.com/owncloud/core/blob/master/apps/dav/lib/HookManager.php#L95). We might need to include a "delete user info regardless of the backend if it's unique" method in order to try to attempt removing that information

[jvillafanez]

It seems we have a huge mess with the hooks.

Current solution:
DeletedUser requires the User\Manager implementation, which extends the PublicEmitter class for whatever reason. This allows us to trigger the same events the User implementation is triggering, plus we ensure the listeners are the expected ones (we aren't notifying to a different set of listeners by accident)
However, this breaks the tests because the mocked class is IUserManager, without any relation to the User\Manager

Alternative 1

Include a getPublicEmitter() function in the IUserManager interface. This would effectively allow us to require a PublicEmitter in the DeletedUser class. The Delete command would get that PublicEmitter from the interface and inject it in the DeletedUser class.
Ideally, there is no big change in the User\Manager implementation: the method could just return the same instance (assuming it implements the same interface)

The problem is that anyone could trigger events anywhere (which, in fact, is what we're doing). This is potentially dangerous because any app could trigger an "user deleted" event even though such event never happened,

Alternative 2

Change the IUserManager::get(...) interface to include an option to return a DeletedUser instance if the user is missing. The default behaviour won't change.

I'll try this second approach

[jvillafanez]

We need to decide what to do with this. The AccountMapper should be called only once because the missing user should be cached (and stored as null) accordingly to what the UserManager does, but the cache implementation ignore these missing users when asked if they're in the cache (hasKey method returns false even if there is a key with a null value). This means that a second hit to the AccountMapper will happen as shown by this test.
Either there is a bug in the CappedMemoryCache class, or the UserManager's behaviour is wrong. It isn't clear if the missing users should be cached or not by the UserManager.