Skip to content

Remove alternate keys storage during user delete#29144

Merged
DeepDiver1975 merged 1 commit into
masterfrom
remove-alternate-key-storage-user-delete
Oct 6, 2017
Merged

Remove alternate keys storage during user delete#29144
DeepDiver1975 merged 1 commit into
masterfrom
remove-alternate-key-storage-user-delete

Conversation

@sharidas

@sharidas sharidas commented Oct 4, 2017

Copy link
Copy Markdown
Contributor

When user is deleted the alternate key storage location
does have folder with username. So while deleting user
this should also be cleaned up.

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

Description

Remove alternate key storage location during user delete process.

Related Issue

#26936

Motivation and Context

When user is deleted, the alternate key storage location which has folder name with username should
also be deleted.

How Has This Been Tested?

  • enable encryption with user keys
  • Using change-key-storage-root command move the keys to folder enckeys
  • Create another user user1 apart from user admin
  • Login as user1
  • Logout from user1 and login as admin
  • Delete the user1, there shouldn't be any traces of user1 inside the folder enckeys

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.

@sharidas sharidas self-assigned this Oct 4, 2017
@sharidas sharidas requested a review from DeepDiver1975 October 4, 2017 10:56
@sharidas sharidas added this to the development milestone Oct 4, 2017
Comment thread lib/private/User/User.php Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can't we do this in a event? This belongs to encryption - this should not live in the user class

@sharidas sharidas force-pushed the remove-alternate-key-storage-user-delete branch from e6ec3c6 to c4e7132 Compare October 4, 2017 12:56
@sharidas

sharidas commented Oct 4, 2017

Copy link
Copy Markdown
Contributor Author

Updated the PR with the help of event dispatcher. And hence there is another bit to be reviewed which is associated with this PR: owncloud/encryption#16

Comment thread lib/private/User/User.php Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

no - this way we have a specific event in here. Please use a generic event name which allows any code to integrate in here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do not we use just preDelete hooks in here? If we are planning to remove hooks, imho we should use more generic event like oc.user.postdelete.
Also, symfony has some naming conventions on the EventDispatcher, It may be beneficial for us to adhere to these rules:
-Use only lowercase letters, numbers, dots (.) and underscores (_);
-Prefix names with a namespace followed by a dot (e.g. order., user.*);
-End names with a verb that indicates what action has been taken (e.g. order.placed).
https://symfony.com/doc/current/components/event_dispatcher.html#naming-conventions

When user is deleted the alternate key storage location
does have folder with username. So while deleting user
this should also be cleaned up.

Signed-off-by: Sujith H <sharidasan@owncloud.com>
@sharidas sharidas force-pushed the remove-alternate-key-storage-user-delete branch from c4e7132 to 32a1177 Compare October 5, 2017 06:58
@sharidas

sharidas commented Oct 5, 2017

Copy link
Copy Markdown
Contributor Author

Got another idea, why not call https://github.com/owncloud/encryption/pull/16/files#diff-ef2f85587bb2300b22909edff73c7440L198. We already have post delete user hook in the encryption app. Let me know if this looks ok?

@DeepDiver1975

Copy link
Copy Markdown
Member

Got another idea, why not call https://github.com/owncloud/encryption/pull/16/files#diff-ef2f85587bb2300b22909edff73c7440L198. We already have post delete user hook in the encryption app. Let me know if this looks ok?

nice - thats the way to go - we can close this pr then - right?

@sharidas

sharidas commented Oct 6, 2017

Copy link
Copy Markdown
Contributor Author

Yes we can merge this change.

@DeepDiver1975 DeepDiver1975 merged commit 17691e4 into master Oct 6, 2017
@DeepDiver1975 DeepDiver1975 deleted the remove-alternate-key-storage-user-delete branch October 6, 2017 06:36
@sharidas

sharidas commented Oct 6, 2017

Copy link
Copy Markdown
Contributor Author

Created backport : #29155

@lock

lock Bot commented Aug 2, 2019

Copy link
Copy Markdown

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.

@lock lock Bot locked as resolved and limited conversation to collaborators Aug 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants