Remove user from the storage setting or storage when user is deleted #30712

sharidas · 2018-03-06T14:07:37Z

Remove user from the storage if there are multiple
users associated with the storage during deletion
of the user. Else remove the storage when user is
deleted, since the storage is only associated with
the user being deleted.

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

Description

Remove user from the external storage list when there are multiple users applicable users for the storage, during deletion of user. Else remove the storage as the user which is being deleted is the only storage associated with the user.

Related Issue

#30694

Motivation and Context

This change would help to clean up the external storage when a user is being deleted.

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

PVince81 · 2018-03-06T14:16:43Z

lib/private/User/User.php

@@ -227,6 +226,24 @@ public function delete() {
 		// Delete the user's keys in preferences
 		\OC::$server->getConfig()->deleteAllUserValues($this->getUID());

+		//Delete external storage or remove user from applicableUsers list
+		foreach (\OC::$server->getUserGlobalStoragesService()->getStorageForAllUsers() as $storageForAllUser) {


did you try with getUserStoragesService() instead ? this would be more direct and give you the whole list without the need to filter.
However one problem is that this service works with the user from the session, might need to refactor it in a way that allows creating/querying it for a given user.

PVince81 · 2018-03-06T14:17:14Z

lib/private/User/User.php

+			if (in_array($this->getUID(), $applicableUsers, true)) {
+				if (count($applicableUsers) === 1) {
+					//As this storage is associated only with this user.
+					\OC::$server->getGlobalStoragesService()->removeStorage($id);


but actually you might be right, this removes storages created by an admin and applied only for a user, so maybe this approach is ok

PVince81 · 2018-03-06T14:20:40Z

I wonder if this logic could be put in a method of one of the services. But it seems the services don't know from each other yet. Does deleting a storage not work from the "UserGlobalStoragesService" ?

codecov · 2018-03-06T14:27:20Z

Codecov Report

Merging #30712 into master will increase coverage by 0.13%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #30712      +/-   ##
============================================
+ Coverage     63.59%   63.72%   +0.13%     
- Complexity    18556    18563       +7     
============================================
  Files          1169     1169              
  Lines         69606    69631      +25     
  Branches       1264     1264              
============================================
+ Hits          44267    44374     +107     
+ Misses        24970    24888      -82     
  Partials        369      369

Flag	Coverage Δ	Complexity Δ
#javascript	`52.59% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`65% <100%> (+0.14%)`	`18563 <7> (+7)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
...ate/Files/External/Service/UserStoragesService.php	`100% <100%> (ø)`	`14 <3> (+3)`	⬆️
lib/private/User/User.php	`87.93% <100%> (+0.14%)`	`67 <0> (ø)`	⬇️
...e/Files/External/Service/GlobalStoragesService.php	`89.69% <100%> (+1.22%)`	`18 <4> (+4)`	⬆️
lib/private/Security/CertificateManager.php	`93.33% <0%> (-1.12%)`	`36% <0%> (ø)`
lib/private/Files/Storage/DAV.php	`81.09% <0%> (+0.42%)`	`0% <0%> (ø)`	⬇️
lib/private/legacy/app.php	`64.75% <0%> (+0.44%)`	`180% <0%> (ø)`	⬇️
...private/Files/External/Service/StoragesService.php	`93.68% <0%> (+0.48%)`	`62% <0%> (ø)`	⬇️
lib/private/Server.php	`86.36% <0%> (+1.07%)`	`253% <0%> (ø)`	⬇️
.../private/Files/External/StoragesBackendService.php	`91.78% <0%> (+2.73%)`	`34% <0%> (ø)`	⬇️
lib/public/Files/External/DefinitionParameter.php	`83.01% <0%> (+3.77%)`	`23% <0%> (ø)`	⬇️
... and 9 more

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 00402f3...3f576a5. Read the comment docs.

sharidas · 2018-03-06T15:09:44Z

Does deleting a storage not work from the "UserGlobalStoragesService" ?

If we use "getUserGlobalStoragesService", then the problem happens when ./occ user:delete is executed. The deletion of storage doesn't work and causes exception. The exception message is UserGlobalStoragesService writing is disallowed

PVince81 · 2018-03-06T15:42:05Z

Yeah, UserGlobalStoragesService is designed for read-only use in the web UI and storage level, to find out all storages the current user has access to, whether configured globally or locally.

Another approach would be to simply have two calls: one for the local storages, maybe UserStoragesService->deleteAll() and another one GlobalStoragesService->deleteAllForUser($userId).

It would also make it easier to write unit tests as you'd mostly write tests for these two methods.

sharidas · 2018-03-12T06:32:38Z

lib/private/User/User.php

@@ -227,6 +226,9 @@ public function delete() {
 		// Delete the user's keys in preferences
 		\OC::$server->getConfig()->deleteAllUserValues($this->getUID());

+		//Delete external storage or remove user from applicableUsers list
+		\OC::$server->getGlobalStoragesService()->deleteAllForUser($this->getUID());


Doesn't local storage gets deleted here. I mean https://github.com/owncloud/core/pull/30712/files#diff-3030ca4989f892c50dea24eaa3f1ae38R236

yeah, the home storage gets deleted later as part of user deletion

however home storage is untouched by the any of the storagesservice which only operate on external storage currently

mmattel · 2018-06-11T17:41:46Z

@sharidas any update on this ?

PVince81 · 2018-06-12T08:43:17Z

tests/lib/Files/External/Service/GlobalStoragesServiceTest.php

+			$storage = $this->makeStorageConfig($storageParams);
+			$this->service->addStorage($storage);
+			if (isset($storageParams['applicableUsers'])) {
+				$this->assertTrue($this->service->deleteAllForUser($storageParams['applicableUsers'][0]));


please also verify that the "applicable" value was properly set after deleteAllForUser and possibly other values. Checking for the return value is not enough to ensure the function is doing its job

PVince81 · 2018-06-19T15:17:30Z

lib/private/Files/External/Service/GlobalStoragesService.php

+	 */
+	public function deleteAllForUser($userId) {
+		$result = false;
+		$mounts = $this->getStorageForAllUsers();


here I'm wondering about scalability: if there are 10000 users and each have a mount, we're querying all possible mounts, even the personal mounts of irrelevant users

I wonder if we should split this in two parts:

get all personal mounts of said user, then remove said storages
and

get all system mounts (no personal mounts) and then filter applicable users as you did below

@butonic does that make sense or is the current solution sufficient for real-life scenarios ?

@sharidas let's go with the proposed approach to play it safe. Here I'm assuming that we already have the necessary API calls to retrieve this info.

Two API's are found to get mounts for user:

getMountsForUser() - https://github.com/owncloud/core/blob/master/lib/private/Files/Config/UserMountCache.php#L198 . When a regular user is mounted with SFTP storage, it shows the /user and SFTP storage folder as mount points.

getMountsForUser() - https://github.com/owncloud/core/blob/master/lib/private/Files/Config/MountProviderCollection.php#L74 . When tested a regular user with SFTP storage mounted, this shows only the SFTP folder.

Guess we can opt for the first one.

PVince81 · 2018-06-19T15:19:06Z

tests/lib/Files/External/Service/GlobalStoragesServiceTest.php

+					\array_shift($storageParams['applicableUsers']);
+					$this->assertEquals([], \array_diff($storageParams['applicableUsers'], $finalApplicableUsers));
+				} else {
+					$storages = \count($this->service->getAllStorages());


you should also make sure that non-relevant storages were not deleted and are still present
for this, create a few unrelated storages and check for these

you could use the "store their id" technique after creation then requery them to see if they still exist and confirm that the correct ones are gone

PVince81 · 2018-06-25T07:57:18Z

lib/public/Files/External/Service/IGlobalStoragesService.php

+	 * Deletes the external storages mounted to the user
+	 * @param $userId
+	 * @return bool
+	 * @since 10.0.8


set to 10.0.10

PVince81 · 2018-06-25T07:58:49Z

lib/private/User/User.php

+		// Delete all mount points for user
+		\OC::$server->getUserStoragesService()->deleteAllMountsForUser($this);
+		//Delete external storage or remove user from applicableUsers list
+		\OC::$server->getGlobalStoragesService()->deleteAllForUser($this->getUID());


be consistent: use either UID or user object

Done. Prefer to use user object. Considering future usage.

sharidas · 2018-06-25T12:50:11Z

I have created a new file in this PR, https://github.com/owncloud/core/pull/30712/files#diff-31407757e5ad52b7e7c4a81530bb4113. The reason why I created a new file is while playing with storageId, I found that GlobalStoragesServiceTest extens StoragesServiceTest and that was causing issues while testing. So I thought its better to separate this test in a separate file.

sharidas · 2018-07-06T13:50:08Z

@PVince81 review required.

PVince81

Looks good, there's only one additional adjustment needed to avoid performance issue by reading too many configs that aren't needed

PVince81 · 2018-07-09T10:28:50Z

lib/private/Files/External/Service/GlobalStoragesService.php

+	public function deleteAllForUser($user) {
+		$userId = $user->getUID();
+		$result = false;
+		$mounts = $this->getStorageForAllUsers();


It seems this method returns not only admin mounts but also personal mounts as it uses DBConfigService::getAllMounts() instead of getAdminMounts().

From reading the code further, it seems you'd need to call getAllStorages() or getStorages() which do call DBConfigService::getAdminMounts() through StorageService::readConfig()

Modified the PR, and updated the change to use getStorages() https://github.com/owncloud/core/pull/30712/files#diff-16d6579da5930e07b8c14d76623559e8R197

Remove user from the storage if there are multiple users associated with the storage during deletion of the user. Else remove the storage when user is deleted, since the storage is only associated with the user being deleted. Signed-off-by: Sujith H <sharidasan@owncloud.com>

PVince81 · 2018-07-16T12:11:49Z

@sharidas please backport

phil-davis · 2018-08-28T15:58:40Z

@sharidas has this been backported?

sharidas · 2018-09-04T08:21:16Z

@PVince81 @phil-davis

This has been backported. The commit in stable10 b6d79b3

The PR for stable10 branch : #32069

sharidas added the 2 - Developing label Mar 6, 2018

sharidas self-assigned this Mar 6, 2018

PVince81 reviewed Mar 6, 2018

View reviewed changes

sharidas force-pushed the user-delete-storage-issue branch 2 times, most recently from 0743cee to f69b77c Compare March 7, 2018 13:29

sharidas commented Mar 12, 2018

View reviewed changes

sharidas added 3 - To Review and removed 2 - Developing labels Mar 12, 2018

sharidas changed the title ~~[WIP] Remove user from the storage setting or storage when user is deleted~~ Remove user from the storage setting or storage when user is deleted Mar 12, 2018

ownclouders added the status/STALE label Mar 20, 2018

sharidas removed the status/STALE label Mar 20, 2018

ownclouders added the status/STALE label Mar 28, 2018

sharidas removed the status/STALE label Mar 28, 2018

ownclouders added the status/STALE label Apr 5, 2018

sharidas removed the status/STALE label May 23, 2018

ownclouders added the status/STALE label May 31, 2018

sharidas force-pushed the user-delete-storage-issue branch from f69b77c to cb58a3b Compare June 6, 2018 12:54

ownclouders removed the status/STALE label Jun 6, 2018

sharidas force-pushed the user-delete-storage-issue branch from cb58a3b to d811617 Compare June 6, 2018 13:25

PVince81 reviewed Jun 12, 2018

View reviewed changes

PVince81 added this to the development milestone Jun 12, 2018

sharidas force-pushed the user-delete-storage-issue branch from d811617 to e4edb74 Compare June 12, 2018 10:18

PVince81 reviewed Jun 19, 2018

View reviewed changes

sharidas force-pushed the user-delete-storage-issue branch from 0b9fd38 to a228e95 Compare June 22, 2018 13:27

PVince81 reviewed Jun 25, 2018

View reviewed changes

sharidas force-pushed the user-delete-storage-issue branch 2 times, most recently from 4e584e1 to 7dd879f Compare June 25, 2018 12:44

sharidas force-pushed the user-delete-storage-issue branch 6 times, most recently from 86a229a to 795a085 Compare June 26, 2018 10:45

ownclouders added the status/STALE label Jul 4, 2018

sharidas force-pushed the user-delete-storage-issue branch from 795a085 to 8e8ddf3 Compare July 4, 2018 07:12

ownclouders removed the status/STALE label Jul 4, 2018

sharidas force-pushed the user-delete-storage-issue branch from 8e8ddf3 to b50540a Compare July 4, 2018 07:57

PVince81 suggested changes Jul 9, 2018

View reviewed changes

sharidas force-pushed the user-delete-storage-issue branch from b50540a to 3f576a5 Compare July 12, 2018 10:36

PVince81 approved these changes Jul 16, 2018

View reviewed changes

PVince81 merged commit a4b2858 into master Jul 16, 2018

PVince81 added the backport-request label Jul 16, 2018

PVince81 deleted the user-delete-storage-issue branch July 16, 2018 12:11

sharidas removed the backport-request label Sep 4, 2018

sharidas mentioned this pull request Sep 27, 2018

Deletion of user should also delete applicable users for the storage #32696

Merged

10 tasks

lock bot locked as resolved and limited conversation to collaborators Sep 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove user from the storage setting or storage when user is deleted #30712

Remove user from the storage setting or storage when user is deleted #30712

sharidas commented Mar 6, 2018

PVince81 Mar 6, 2018

PVince81 Mar 6, 2018

PVince81 commented Mar 6, 2018

codecov bot commented Mar 6, 2018 •

edited

Loading

sharidas commented Mar 6, 2018

PVince81 commented Mar 6, 2018

sharidas Mar 12, 2018

PVince81 Mar 12, 2018

mmattel commented Jun 11, 2018

PVince81 Jun 12, 2018

sharidas Jun 19, 2018

PVince81 Jun 19, 2018

PVince81 Jun 21, 2018

sharidas Jun 21, 2018 •

edited

Loading

PVince81 Jun 19, 2018

PVince81 Jun 25, 2018

sharidas Jun 25, 2018

PVince81 Jun 25, 2018

sharidas Jun 25, 2018

sharidas commented Jun 25, 2018

sharidas commented Jul 6, 2018

PVince81 left a comment

PVince81 Jul 9, 2018

sharidas Jul 12, 2018

PVince81 commented Jul 16, 2018

phil-davis commented Aug 28, 2018

sharidas commented Sep 4, 2018 •

edited

Loading

Remove user from the storage setting or storage when user is deleted #30712

Remove user from the storage setting or storage when user is deleted #30712

Conversation

sharidas commented Mar 6, 2018

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Mar 6, 2018

codecov bot commented Mar 6, 2018 • edited Loading

Codecov Report

sharidas commented Mar 6, 2018

PVince81 commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmattel commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharidas Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharidas commented Jun 25, 2018

sharidas commented Jul 6, 2018

PVince81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Jul 16, 2018

phil-davis commented Aug 28, 2018

sharidas commented Sep 4, 2018 • edited Loading

codecov bot commented Mar 6, 2018 •

edited

Loading

sharidas Jun 21, 2018 •

edited

Loading

sharidas commented Sep 4, 2018 •

edited

Loading