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

fix: delete all files from object store when user is deleted #40959

Merged
merged 1 commit into from Sep 19, 2023

Conversation

DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Aug 29, 2023

Description

As soon as a user is deleted files will also be removed from object storage (s3).

Motivation and Context

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

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
Copy link

update-docs bot commented Aug 29, 2023

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.

@DeepDiver1975 DeepDiver1975 marked this pull request as draft August 29, 2023 12:13
@DeepDiver1975 DeepDiver1975 force-pushed the fix/delete-files-s3-on-user-delete branch 6 times, most recently from 5beff20 to e28588c Compare September 5, 2023 10:50
@DeepDiver1975
Copy link
Member Author

@phil-davis because the full user deletion is moved to cron/command bus acceptance tests are having issues. https://drone.owncloud.com/owncloud/core/39034/37/14

I suggest to run cron after deleting a user via API or use the occ command user:delete because this will not delete via cron.

THX

@phil-davis
Copy link
Contributor

phil-davis commented Sep 7, 2023

For end-of-scenario cleanup, I can sort out the code so that it does a proper real delete of the user. For example, change the test code so that it does occ user:delete

But there are also scenarios that test the provisioning API DELETE behavior.
When /^the administrator deletes user "Alice" using the provisioning API

That Provisioning API endpoint now has different behavior - do I understand correctly?
It will return an HTTP status indicating success. But then if I do an API GET to try and get data about the user, I will not get a 404 (not found).
So tests will have to do more for steps like that, they will also have to run whatever "background" cron is needed to really delete the user.

@DeepDiver1975
Copy link
Member Author

That Provisioning API endpoint now has different behavior - do I understand correctly?

That is a good point. Depending on the cron setup it can take minutes until a user is deleted.
Add a flag?
Skip the cron approach?

@phil-davis
Copy link
Contributor

Add a flag? Skip the cron approach?

After the Provisioning API DELETE request is processed, what is the external status of the user:

  1. can they still login and see their data?
  2. can sharees still see shares received from the user?
  3. can sharers still see the username in the list of users that they can share with, and "accidentally" share with a user that is about to be really cleaned up?

There will be situations when an admin is deleting a user for "immediate" security reasons. I suppose for that case, the admin can first disable the user, preventing access. Then the user delete can be more async.

Maybe a delete can be done by doing a disable synchronously, and leaving the actual user deletion/file-deletion to later. That should preserve security, and still allow the "bigger" file deletion to be done later in the background.

@DeepDiver1975 DeepDiver1975 force-pushed the fix/delete-files-s3-on-user-delete branch from e28588c to b074a88 Compare September 15, 2023 13:46
@DeepDiver1975
Copy link
Member Author

I decided to skip the command bus. All user delete operations are now done synchronous.
In case the operations failed/times out it can still be re-run as the user is deleted last.

@DeepDiver1975 DeepDiver1975 marked this pull request as ready for review September 17, 2023 16:52
lib/private/User/User.php Outdated Show resolved Hide resolved
@pako81
Copy link
Contributor

pako81 commented Sep 18, 2023

Small remark about a variable's name. Apart from this, code looks ok and manual testing confirms the correct behaviour: files are removed from the object store as soon as the user gets deleted.

@pako81
Copy link
Contributor

pako81 commented Sep 18, 2023

CI fail is not related.

@DeepDiver1975 DeepDiver1975 force-pushed the fix/delete-files-s3-on-user-delete branch from b074a88 to 20e30cc Compare September 18, 2023 09:40
@jnweiger
Copy link
Contributor

CI fail is not related.

CI can now access the WND server again!

@sonarcloud
Copy link

sonarcloud bot commented Sep 19, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

60.0% 60.0% Coverage
0.0% 0.0% Duplication

@pako81
Copy link
Contributor

pako81 commented Sep 19, 2023

@DeepDiver1975 merge this or additional review needed?

@DeepDiver1975 DeepDiver1975 merged commit 9fe4c08 into master Sep 19, 2023
3 checks passed
@delete-merged-branch delete-merged-branch bot deleted the fix/delete-files-s3-on-user-delete branch September 19, 2023 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants