Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Sep 27, 2018

Conversation

sharidas
Copy link
Contributor

@sharidas sharidas commented Sep 13, 2018

When a user is deleted the storage added by the user
should be removed from the db. This change helps to
remove the user from the applicable with the help
of DBConfigService.

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

Description

When a user is deleted, the storages created by user should be cleaned from the db. That is the applicable array should not have any more residues about the user.

Related Issue

Motivation and Context

When a user is deleted, the storages created by user should be cleaned from the db. That is the applicable array should not have any more residues about the user.

How Has This Been Tested?

Following the procedure from steps to produce in #32637 (comment):

  • In the newly installed machine before deleting user4 the tables were like this:
sqlite> select * from oc_mounts;
id          storage_id  root_id     user_id     mount_point
----------  ----------  ----------  ----------  -----------
1           1           1           admin       /admin/    
2           4           23          user4       /user4/    
3           3           28          user4       /user4/file
sqlite> select * from oc_external_applicable;
applicable_id  mount_id    type        value     
-------------  ----------  ----------  ----------
1              1           1                     
3              2           3           user1     
4              2           3           user2     
5              2           3           user4     
6              2           3           user3     
7              3           3           user4     
sqlite> select * from oc_external_mounts;
mount_id    mount_point  storage_backend  auth_backend        priority    type      
----------  -----------  ---------------  ------------------  ----------  ----------
1           /sftp        sftp             password::password  100         1         
2           /sftpuser4   sftp             password::password  100         1         
3           /sftpdirect  sftp             password::password  100         2         
sqlite> select * from oc_external_config;
config_id   mount_id    key         value     
----------  ----------  ----------  ----------
1           1           host        localhost 
2           1           root        /tmp/a    
3           1           user        sujith    
4           1           password    9f16a17347
5           2           host        localhost 
6           2           root        /tmp/a    
7           2           user        sujith    
8           2           password    aaafba9dc9
9           3           host        localhost 
10          3           root        /tmp/a    
11          3           user        sujith    
12          3           password    6c9d98c910
sqlite>
  • After user4 is deleted table is as shown below:
sqlite> select * from oc_mounts;
id          storage_id  root_id     user_id     mount_point
----------  ----------  ----------  ----------  -----------
1           1           1           admin       /admin/    
4           3           28          admin       /admin/file
sqlite> select * from oc_external_applicable;
applicable_id  mount_id    type        value     
-------------  ----------  ----------  ----------
1              1           1                     
3              2           3           user1     
4              2           3           user2     
6              2           3           user3     
sqlite> select * from oc_external_mounts;
mount_id    mount_point  storage_backend  auth_backend        priority    type      
----------  -----------  ---------------  ------------------  ----------  ----------
1           /sftp        sftp             password::password  100         1         
2           /sftpuser4   sftp             password::password  100         1         
sqlite> select * from oc_external_config;
config_id   mount_id    key         value     
----------  ----------  ----------  ----------
1           1           host        localhost 
2           1           root        /tmp/a    
3           1           user        sujith    
4           1           password    9f16a17347
5           2           host        localhost 
6           2           root        /tmp/a    
7           2           user        sujith    
8           2           password    aaafba9dc9
sqlite>
  • Create user4 again and the table is as shown:
sqlite> select * from oc_mounts;
id          storage_id  root_id     user_id     mount_point
----------  ----------  ----------  ----------  -----------
1           1           1           admin       /admin/    
4           3           28          admin       /admin/file
5           3           28          user4       /user4/file
6           5           33          user4       /user4/    
sqlite> select * from oc_external_applicable;
applicable_id  mount_id    type        value     
-------------  ----------  ----------  ----------
1              1           1                     
3              2           3           user1     
4              2           3           user2     
6              2           3           user3     
8              2           3           user4     
9              4           3           user4     
sqlite> select * from oc_external_mounts;
mount_id    mount_point  storage_backend  auth_backend        priority    type      
----------  -----------  ---------------  ------------------  ----------  ----------
1           /sftp        sftp             password::password  100         1         
2           /sftpuser4   sftp             password::password  100         1         
4           /sftpdirect  sftp             password::password  100         2         
sqlite> select * from oc_external_config;
config_id   mount_id    key         value     
----------  ----------  ----------  ----------
1           1           host        localhost 
2           1           root        /tmp/a    
3           1           user        sujith    
4           1           password    9f16a17347
5           2           host        localhost 
6           2           root        /tmp/a    
7           2           user        sujith    
8           2           password    aaafba9dc9
13          4           host        localhost 
14          4           root        /tmp/a    
15          4           user        sujith    
16          4           password    7e35de472a
sqlite>
  • Again delete user4 and the table is as shown below:
sqlite> select * from oc_mounts;
id          storage_id  root_id     user_id     mount_point
----------  ----------  ----------  ----------  -----------
1           1           1           admin       /admin/    
4           3           28          admin       /admin/file
sqlite> select * from oc_external_applicable;
applicable_id  mount_id    type        value     
-------------  ----------  ----------  ----------
1              1           1                     
3              2           3           user1     
4              2           3           user2     
6              2           3           user3     
sqlite> select * from oc_external_mounts;
mount_id    mount_point  storage_backend  auth_backend        priority    type      
----------  -----------  ---------------  ------------------  ----------  ----------
1           /sftp        sftp             password::password  100         1         
2           /sftpuser4   sftp             password::password  100         1         
sqlite> select * from oc_external_config;
config_id   mount_id    key         value     
----------  ----------  ----------  ----------
1           1           host        localhost 
2           1           root        /tmp/a    
3           1           user        sujith    
4           1           password    9f16a17347
5           2           host        localhost 
6           2           root        /tmp/a    
7           2           user        sujith    
8           2           password    aaafba9dc9
sqlite>
  • Also verified after the deletion the admin settings never showed user4 for the storage sftpuser4.

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)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@sharidas sharidas added this to the QA milestone Sep 13, 2018
@sharidas sharidas self-assigned this Sep 13, 2018
@mmattel
Copy link
Contributor

mmattel commented Sep 13, 2018

For the record, [stable10] already merged: #32069

When a user is deleted the storage added by the user
should be removed from the db. This change helps to
remove the user from the applicable with the help
of DBConfigService.

Signed-off-by: Sujith H <sharidasan@owncloud.com>
@sharidas sharidas force-pushed the fix-deletion-of-applicable-users branch from 22267c1 to 35bb075 Compare September 14, 2018 06:09
@PVince81
Copy link
Contributor

@mmattel that one didn't work as expected, reported here: #32637

the PR here brings in some missing bits

@mmattel
Copy link
Contributor

mmattel commented Sep 14, 2018

The referenced PR is stable10 and as far as I have seen there is no corresponding master PR (even it would not work properly).
Pls correct me, if this PR does its job, master is fixed, but needs backporting to stable10 to be clean

@PVince81
Copy link
Contributor

@sharidas please double check if there are missing references

@sharidas
Copy link
Contributor Author

Let me add my results here:

  1. Create four users "user1", "user2", "user3", "user4"
  2. Allow users mounting external storages and limit it to only "SFTP"
  3. Create an external storage "/sftp" applicable to all
  4. Create a global external storage "/sftpuser4" applicable to "user1", "user2", "user4", "user3"
  5. Create a global external storage with "owncloud" for "user4"
  6. Login as "user4"
  7. Create a personal storage "/ownclouddirect"
  8. Check that all four storages are accessible
  9. Login as admin
  10. Delete user4
  11. Result from database query to prove it worked:
sqlite> select * from oc_external_mounts;
mount_id    mount_point  storage_backend  auth_backend        priority    type      
----------  -----------  ---------------  ------------------  ----------  ----------
1           /sftp        sftp             password::password  100         1         
2           /sftpuser4   sftp             password::password  100         1         
sqlite> select * from oc_mounts;
id          storage_id  root_id     user_id     mount_point
----------  ----------  ----------  ----------  -----------
1           1           1           admin       /admin/    
4           3           34          admin       /admin/file
sqlite> 
sqlite> 
sqlite> 
sqlite> 
sqlite> select * from oc_mounts;
id          storage_id  root_id     user_id     mount_point
----------  ----------  ----------  ----------  -----------
1           1           1           admin       /admin/    
4           3           34          admin       /admin/file
sqlite> select * from oc_external_applicable;
applicable_id  mount_id    type        value     
-------------  ----------  ----------  ----------
1              1           1                     
3              2           3           user1     
4              2           3           user2     
5              2           3           user3     
sqlite> select * from oc_external_mounts;
mount_id    mount_point  storage_backend  auth_backend        priority    type      
----------  -----------  ---------------  ------------------  ----------  ----------
1           /sftp        sftp             password::password  100         1         
2           /sftpuser4   sftp             password::password  100         1         
sqlite>

There is no reference of "user4" here.

Another test to have 2 personal storages for user4:

  1. Create "user4"
  2. Login as "user4"
  3. Allow user4 to create 2 personal storages "/ownclouddirect" and "/sftpdirect"
  4. Now delete user4.
  5. There should be no traces of "user4" in the db.
  6. Traces of "user4" not found in db:
sqlite> select * from oc_mounts;
id          storage_id  root_id     user_id     mount_point
----------  ----------  ----------  ----------  -----------
1           1           1           admin       /admin/    
sqlite> select * from oc_external_mounts;
sqlite> select * from oc_external_applicable;
sqlite> select * from oc_external_config;
sqlite> 

Another test to have global storages for user4 and no personal storages:

  1. Create "user4"
  2. Create "/owncloud" and "/sftp", which are available for users like "user1", "user2", "user3" and "user4" only
  3. Login as "user4"
  4. Now delete user4.
  5. There should be no traces of "user4" in the db.
  6. Db table results are as shown below:
sqlite> select * from oc_mounts;
id          storage_id  root_id     user_id     mount_point
----------  ----------  ----------  ----------  -----------
1           1           1           admin       /admin/    
7           5           33          user1       /user1/file
8           4           32          user1       /user1/file
9           5           33          user2       /user2/file
10          4           32          user2       /user2/file
11          5           33          user3       /user3/file
12          4           32          user3       /user3/file
sqlite> select * from oc_external_mounts;
mount_id    mount_point  storage_backend  auth_backend        priority    type      
----------  -----------  ---------------  ------------------  ----------  ----------
3           /owncloud    owncloud         password::password  100         1         
4           /sftp        sftp             password::password  100         1         
sqlite> select * from oc_external_config;
config_id   mount_id    key         value                    
----------  ----------  ----------  -------------------------
10          3           host        http://localhost/testing2
11          3           root                                 
12          3           secure                               
13          3           user        admin                    
14          3           password    65f3f06859c35cffc0c3fadc5
15          4           host        localhost                
16          4           root        /tmp/a                   
17          4           user        sujith                   
18          4           password    e1d93e49a590b170156e0955d
sqlite> select * from oc_external_applicable;
applicable_id  mount_id    type        value     
-------------  ----------  ----------  ----------
5              4           3           user1     
6              4           3           user2     
9              4           3           user3     
10             3           3           user1     
12             3           3           user2     
13             3           3           user3     
sqlite>

The above tests do cover global storages only, personal storages only and both global + personal storages. Which in my humble opinion covers most of the cases.

@PVince81 PVince81 modified the milestones: QA, development Sep 25, 2018
* Remove any mounts which are applicable to only this user
* Specifically targeted to, mounts created by the user.
*/
if (\count($userMount['applicable']) === 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so are we removing the user from the applicable list in a different location in the code ? (when applicable is more than one user). from your test results it seems to work so properly different existing code ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GlobalStoragesService does remove user from applicable list, here https://github.com/owncloud/core/blob/master/lib/private/Files/External/Service/GlobalStoragesService.php#L199-L214. So as per the test #32637 (comment) -> the global storages does remove user4 from the applicable list.

Let me know if this was the information, that you are looking for. If not I would need your helping hand in understanding the query.

@sharidas
Copy link
Contributor Author

The referenced PR is stable10 and as far as I have seen there is no corresponding master PR (even it would not work properly).
Pls correct me, if this PR does its job, master is fixed, but needs backporting to stable10 to be clean

The not working pr merged to master branch -> #30712 and stable10 branch pr -> #32069 ( not working ).
So we do have a master branch pr here. Let me know if I answered your query?

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@PVince81 PVince81 merged commit 4dad523 into master Sep 27, 2018
@PVince81 PVince81 deleted the fix-deletion-of-applicable-users branch September 27, 2018 11:47
@PVince81
Copy link
Contributor

@sharidas please backport

@sharidas
Copy link
Contributor Author

Backport PR: #32906

@lock lock bot locked as resolved and limited conversation to collaborators Sep 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[10.0.10RC1] Deleting user doesn't delete them from external storages
3 participants