Bring user deletion account by tcitworld · Pull Request #1893 · nextcloud/server

tcitworld · 2016-10-24T16:49:56Z

Bring the possibility for an user to delete it's own account. Administrators can enable/disable this functionality.

Security issues
Redirect to login with js after deleting account
Tests

mention-bot · 2016-10-24T16:49:58Z

@tcitworld, thanks for your PR! By analyzing the history of the files in this pull request, we identified @LukasReschke, @MorrisJobke and @icewind1991 to be potential reviewers.

MorrisJobke · 2016-10-24T18:58:43Z

 	public function destroy($id) {
 		$userId = $this->userSession->getUser()->getUID();
 		$user = $this->userManager->get($id);
+		$userOwnDeletion = $this->config->getSystemValue('user_own_account_deletion', false);


Please no config.php switch. This should be stored in the database.

Okay. Where should it be activated by the administrator. Additional settings ?

MorrisJobke · 2016-10-24T19:00:16Z

 	}
 }

+function deleteAccount () {


Global namespace 🙈 Yes I know, the other methods are also in the global namespace - but at some point we need to clean up this mess 😉

MorrisJobke · 2016-10-24T19:00:30Z

 	}
 }

+function deleteAccount () {


Global namespace 🙈 Yes I know, the other methods are also in the global namespace - but at some point we need to clean up this mess 😉

rullzer · 2016-10-24T19:10:20Z

Can't we just do this as an app?

tcitworld · 2016-10-24T19:43:46Z

I thought it was a quite small feature to create an app. Also it's configurable by the admin.

LukasReschke

I'd also prefer this to be in it's own app. But that may just be personal preference.

In any case, there are some problems with this:

Missing unit tests for the UsersController
The "delete your account" should only appear in the settings when it is possible (e.g. the user backend of the currently logged-in user supports it)
Some remarks that Morris had as well.

tcitworld · 2016-10-25T10:58:55Z

Most of it is done. I just don't know where the admin setting should show up. Also, how to test if backend supports deletion (I've just put Database for now) ?

nickvergessen · 2016-10-26T10:11:44Z

 	</div>
 </div>

+<?php if ($_['accountDeletionEnabled']) { ?>


Should be at the bottom of the site in my opinion

LukasReschke · 2016-10-26T10:15:36Z

 }

+// show account deletion button only if it's enabled and if the backend supports it
+$accountDeletionEnabled = $config->getAppValue('core', 'user_own_account_deletion', false) && $user->getBackendClassName() === 'Database';


Most of it is done. I just don't know where the admin setting should show up. Also, how to test if backend supports deletion (I've just put Database for now) ?

\OCP\UserInterface::implementsActions + add an action constant + adjust the backends.

No hard-coded hackery here 😉

tcitworld · 2016-10-28T20:08:42Z

+	 * @return bool
+	 */
+	public function canDeleteAccount() {
+		// TODO : Change this


For some reason. Any help here ?

(bool)$this->config->getAppValue('core', 'user_own_account_deletion', 0) and store an integer would have been my approach.

tcitworld · 2016-10-28T20:08:59Z

+	/**
+	 * check if the backend supports deleting user
+	 *
+	 * @return bool


This or ownCloud notation ?

I'm fine with this.

We used 9.2 so far, I guess we should "unify" them all before the release and do one or the other.

tcitworld · 2016-10-28T20:09:40Z


 $formsAndMore = array_merge($formsAndMore, $formsMap);

+if ($user->canDeleteAccount()) {


Better way ?

Signed-off-by: Thomas Citharel <[email protected]>

* Use OCP instead of OC js namespace * Rewrite config entry condition * Rebase Signed-off-by: Thomas Citharel <[email protected]>

tcitworld · 2016-11-05T12:43:18Z

Rebased. Please review. :-)

rullzer · 2016-11-05T13:27:31Z

I still really dislike this being in core. In 99.99% of the cases this is not required. For me the only real usecase is if a service provider wants to allow people to delete their accounts. Which is a fair use case. But, again, please do it as an app.

MariusBluem · 2016-11-05T13:55:33Z

For me the only real usecase is if a service provider wants to allow people to delete their accounts. Which is a fair use case. But, again, please do it as an app.

Agreed. And Service Providers may want to use their own CRM to have better control over it. So the use-case is as small as this is something for a separate app, I think 😁 ...However: Good work 👍

tcitworld · 2016-11-05T13:57:00Z

Okay ! (I will delete the branch later)

nickvergessen · 2016-11-07T11:25:01Z

+	 * @return bool
+	 */
+	public function canDeleteAccount() {
+		// TODO : Change this


no need for the bool casting here

nickvergessen

Needs changes

nickvergessen · 2016-11-07T11:26:05Z

 	public function destroy($id) {
 		$userId = $this->userSession->getUser()->getUID();
 		$user = $this->userManager->get($id);
+		$userOwnDeletion = $this->config->getAppValue('core', 'user_own_account_deletion', false);


default value should be 0

nickvergessen · 2016-11-07T11:26:50Z


+OC.Settings.Personal = OC.Settings.Personal || {
+	deleteAccount: function() {
+		OC.dialogs.confirm('Do you really want to delete your account ? All your data will be lost !', 'Delete your account ?', function (res) {


Please remove the spaces before ? and !

tcitworld added 2. developing Work in progress feature: users and groups labels Oct 24, 2016

MorrisJobke reviewed Oct 24, 2016

View reviewed changes

LukasReschke requested changes Oct 25, 2016

View reviewed changes

tcitworld force-pushed the user-delete-account branch from f7392cf to 9a81ff8 Compare October 25, 2016 11:52

nickvergessen reviewed Oct 26, 2016

View reviewed changes

LukasReschke requested changes Oct 26, 2016

View reviewed changes

tcitworld commented Oct 28, 2016

View reviewed changes

tcitworld added 3 commits November 5, 2016 13:34

Bring user deletion account

f959db5

Signed-off-by: Thomas Citharel <[email protected]>

Use implementsActions and display a checkbox in users panel

27bd22c

Signed-off-by: Thomas Citharel <[email protected]>

Fix reviews

a3902b5

* Use OCP instead of OC js namespace * Rewrite config entry condition * Rebase Signed-off-by: Thomas Citharel <[email protected]>

tcitworld force-pushed the user-delete-account branch from e3aeb52 to a3902b5 Compare November 5, 2016 12:42

tcitworld added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 5, 2016

tcitworld closed this Nov 5, 2016

nickvergessen reviewed Nov 7, 2016

View reviewed changes

tcitworld mentioned this pull request Nov 14, 2016

Handle session closure properly for redirecting to a controller in a post_login hook #1303

Closed

tcitworld deleted the user-delete-account branch November 14, 2016 18:03


		$formsAndMore = array_merge($formsAndMore, $formsMap);

		if ($user->canDeleteAccount()) {

Conversation

tcitworld commented Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Oct 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rullzer commented Oct 24, 2016

Uh oh!

tcitworld commented Oct 24, 2016

Uh oh!

LukasReschke left a comment

Choose a reason for hiding this comment

Uh oh!

tcitworld commented Oct 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcitworld commented Nov 5, 2016

Uh oh!

rullzer commented Nov 5, 2016

Uh oh!

MariusBluem commented Nov 5, 2016

Uh oh!

tcitworld commented Nov 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickvergessen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tcitworld commented Oct 24, 2016 •

edited

Loading