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

[new feature] - allow users self-delete their accounts #19023

Closed
wants to merge 58 commits into from

Conversation

@alikon
Copy link
Contributor

commented Dec 8, 2017

Allow users to delete their account by themselves

Summary of Changes

  • Added a new com_user menu item User Delete Request

screenshot from 2017-12-08 14-01-06

  • Added a new plugin group (userdelete)
  • Added a new plugin for check content existence before account deletion
  • Added a new plugin for check contact existence before account deletion

Testing Instructions

apply PR
discover the new plugins and install
screenshot from 2017-12-17 11-40-15
and enable them
screenshot from 2017-12-17 12-00-14

create a dummy user
Add a new com_user menu item and set to Registered
login with that dummy user
click on the delete account menu item
fill the account email

screenshot from 2017-12-08 14-10-27
after click on submit and after a 2nd confirmation account is deleted
screenshot from 2017-12-09 08-58-54

The delete of the user is denied if he/she still have content and the relative userdelete plugin is enabled
backend
screenshot from 2017-12-17 11-50-44
frontend
screenshot from 2017-12-17 11-53-24

Expected result

a user is able to delete his account from frontend
but if a userdelete plugin is enabled a check is made to verify that the user don't have "content", "contact", etc..

Actual result

an user is unable to delete his account from frontend

Documentation Changes Required

maybe

alikon added 17 commits Dec 8, 2017
@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

commented Dec 8, 2017

@alikon is this related to #18160?

@C-Lodder

This comment has been minimized.

Copy link
Member

commented Dec 8, 2017

I assume super users should not be allowed to have their account deleted?

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented Dec 8, 2017

If we require double confirmation to create an account (confirming the email) then we should do the same for deleting an account

@@ -48,3 +48,6 @@ COM_USER_RESET_VIEW_DEFAULT_DESC="Displays a request to reset password."
COM_USER_RESET_VIEW_DEFAULT_OPTION="Default"
COM_USER_RESET_VIEW_DEFAULT_TITLE="Password Reset"
COM_USERS_XML_DESCRIPTION="Component for managing users."
COM_USER_DELETE_VIEW_DEFAULT_DESC="Displays a user delete request."

This comment has been minimized.

Copy link
@Quy

Quy Dec 8, 2017

Contributor

Sort keys in alpha order.

@@ -163,3 +163,7 @@ COM_USERS_USER_FIELD_TIMEZONE_DESC="Choose your time zone."
COM_USERS_USER_FIELD_TIMEZONE_LABEL="Time Zone"
COM_USERS_USER_NOT_FOUND="User not found."
COM_USERS_USER_SAVE_FAILED="Failed to save user: %s"
COM_USERS_DELETE="Delete"

This comment has been minimized.

Copy link
@Quy

Quy Dec 8, 2017

Contributor

Sort keys in alpha order.

{
// The request succeeded.
// Proceed to step two.
$message = JText::_('COM_USERS_DELETE_REQUEST_SUCCESS');

This comment has been minimized.

Copy link
@Quy

Quy Dec 8, 2017

Contributor

Add language string.

{
// The request failed.
// Go back to the request form.
$message = JText::sprintf('COM_USERS_DELETE_REQUEST_FAILED', $model->getError());

This comment has been minimized.

Copy link
@Quy

Quy Dec 8, 2017

Contributor

Add language string.

if (JUserHelper::checkSuperUserInUsers(ArrayHelper::toInteger($user->id)))
{
$this->setError(JText::_('COM_USERS_ERROR_CANNOT_DELETE_SUPERUSER'));

This comment has been minimized.

Copy link
@Quy

Quy Dec 8, 2017

Contributor

Add language string.

<metadata>
<layout title="COM_USER_DELETE_VIEW_DEFAULT_TITLE" option="COM_USER_DELETE_VIEW_DEFAULT_OPTION">
<help
key = "JHELP_MENUS_MENU_ITEM_USER_DELETE"

This comment has been minimized.

Copy link
@Quy

Quy Dec 8, 2017

Contributor

Add language string.

This comment has been minimized.

Copy link
@alikon

alikon Dec 17, 2017

Author Contributor

done

{
$app = JFactory::getApplication();
$menus = $app->getMenu();
$title = null;

This comment has been minimized.

Copy link
@Quy

Quy Dec 8, 2017

Contributor

Remove line 76 since it being reassigned in line 91.

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

commented Dec 8, 2017

@Quy can you please e-mail me at franz.wohlkoenig@community.joomla.org?

@tonypartridge

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2017

@joomdonation

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2017

That hooks is available out of the box, it is onUserBeforeDelete. The question is do we want to allow plugin to prevent deleting user? And what's the best way to allow plugin give the reason (message) to end user if it doesn't allow user account is deleted

Right now, we just trigger the event https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_users/models/user.php#L379 , and ignore the result returned by plugins https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_users/models/user.php#L379

Look like you are suggesting that user account should be deleted anyways, and the jobs of plugins are deleting the data associated to that user?

@tonypartridge

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2017

@Webdongle

This comment has been minimized.

Copy link
Contributor

commented Dec 22, 2017

@joomdonation

I actually asked about technical questions about the best ways to implement these check. I guess you are saying that because of the new law, no check is needed?

No ... I think because of @mbabker saying about copywrite. It could be (due to T&C) that the user does not have the legal right to delete the content. #19023 (comment)

@ggppdk

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2018

What about if the user deletion happens like this

  • name, email, login name etc data in the users DB table are cleared,

... but the record in users table is kept as a "ghosted" record, maintaining the user id

  • any profile data deleted

  • usergroup mappings deleted

  • user is blocked but also without any assignments to usergroups, user cannot login, (but you may take some extra step here)

  • all content is kept

@Webdongle

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2018

@ggppdk

but the record in users table is kept as a "ghosted" record, maintaining the user id

But that would prevent the user deleting all reference to themselves.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19023.

@tonypartridge

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2018

@Webdongle

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2018

But the user ID is used to display the author of the content. So Either the users name will be displayed in the article or it will show an error of not found.

@Bakual

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2018

You have the UserID already as foreign key in the content table. There is no point keeping an "empty" row for it in the user table. At least as long as we don't use referential integrity in our CMS.

@tonypartridge

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2018

@Bakual

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2018

It should return, sorry user not found. Or user unavailable or similar.

You can do that regardless of a row present or not.

@mbabker

This comment has been minimized.

Copy link
Member

commented Apr 2, 2018

But the user ID is used to display the author of the content. So Either the users name will be displayed in the article or it will show an error of not found.

The user ID is needed to query the users table to look up information, what is shown on the frontend is the result of the query. So, the database table's primary key (the user ID) is NOT personally identifiable information and should NOT be treated as such. Hence the reason the account should be pseudo anonymized, not deleted in full (basically run an UPDATE query on the users table row versus a DELETE query). You end up with something like @ghost here on GitHub in that state in that there is still a valid cross-table reference in the stored database information to a user record, but that user record is not identifiable to whatever individual for whom the account was originally created.

We already have enough problems with administrators deleting users in the backend then having all sorts of "user not found" errors show up on the site because, as core does not use proper relations and keys for cross-table references, records in other tables still have the deleted user ID in their fields (commonly created by and modified by). Let's not create more.

@ggppdk

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2018

@mbabker

Hence the reason the account should be pseudo anonymized, not deleted in full (basically run an UPDATE query on the users table row versus a DELETE query). You end up with something like @ghost here on GitHub in that state in that there is still a valid cross-table reference in the stored database information to a user record, but that user record is not identifiable to whatever individual for whom the account was originally created.

We already have enough problems with administrators deleting users in the backend ...

So you support the idea of "anonymizing" the user record, and saying that deleting it fully is problematic, right ?

@mbabker

This comment has been minimized.

Copy link
Member

commented Apr 2, 2018

Giving a non-administrative user a button which can trigger a DELETE FROM users query should not happen, it carries too much risk (imagine if Joomla did have proper relational schema and there were CASCADE DELETE actions set on those relations); that delete query should ever only be triggered by an authorized administrator who fully understands the platform and the risk of said action.

So yes, the account should be anonymized.

@Webdongle

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2018

KISS just have it delete user details regardless of content ... change the user name to 'Unknown user. No point in deleting the user record because that ID will not be used for new users anyway.

@alikon

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2018

i'm strongly against ghost userid
if a user wich sumbit content or whatever ask for deletion he is asking for deleting all his account related stuff, i'm not a GDPR expert but.....

@mbabker

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

You have to ghost the account (completely anonymize it). Joomla lacks proper relational structure, and we have no idea how extensions are referencing the users table. A blind DELETE FROM users WHERE id = 42 query is going to cause problems for someone somewhere in ways that we cannot predict or fix. Whereas UPDATE users SET name = 'adlsfhklasdf', username = 'iuwherofasdnf', email = 'asdfjkn@whoue.com'... keeps the data record in place but has anonymized out the user information.

Sure, we can do the DELETE query. It's going to have consequences. Core's not in a state to deal with those consequences, extensions are probably even less so.

@alikon

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2018

at some point in time we should really care about referential integrity
if extension developers are serious they can even write their onUserBeforeDelete code
the same as can do and should do the joomla core but maybe i'm too much optimistic, so let me fall down to the planet earth, i'll reconsider the ghost...

@mbabker

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

I know we need to get there (having referential integrity), but using what we have now I don't think it's practical to try and start down that path with this feature (especially as this particular item is more against a calendar deadline than core getting its schema together).

@alikon

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2018

you're damn right as usual 😄
what should be the last available window/branch if i'll find the time to switch to the ghost "thing" ?

@Webdongle

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2018

Whereas UPDATE users SET name = 'adlsfhklasdf', username = 'iuwherofasdnf', email = 'asdfjkn@whoue.com'... keeps the data record in place but has anonymized out the user information.

Sounds good but would it cause a problem when 'name'/'username'/'email' get duplicated when a second user deletes their accounts ?

@ggppdk

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2018

Other than the primary key (that is unique by its nature)

  • the other columns do not need to be unique

https://github.com/joomla/joomla-cms/blob/staging/installation/sql/mysql/joomla.sql#L1984-L2010

So you can set them to empty string

  • i understand that some extensions will expect non-empty data and have some issues like email sending failures, just let these extensions deal with this

since the above is much less common problem than having almost all extensions assuming that a record in users table exist, causing a lot of problems if the row would get deleted

Also other topics

  1. How to detect that a rows users table has been "ghosted"
  • i say be empty username ?
  1. Allow new user accounts having usernames and/or emails of ghosted account to be created ?
  • just yes, no need to check or keep the old account name / emails, because of keep such information (even if we would keep a hash of them ...) then the promise that account data were deleted would be a lie
  1. About extensions duplicating data from users table it will be their issue to deal with this
    but at least 1 (or 2 ?) new event(s) should be triggered to indicate the "ghosting" of a user record
@mbabker

This comment has been minimized.

Copy link
Member

commented Apr 4, 2018

How to detect that a rows users table has been "ghosted"

Why does there need to be detection of this? Why does a ghosted account need to be treated any differently than other accounts once it has been pseudo-anonymized?

Allow new user accounts having usernames and/or emails of ghosted account to be created ?

What remains in the database should still be valid data, even if pseudo-anonymized random stuff, and any validation rules should still apply. If today it is not allowed to duplicate usernames, a condition for "no duplicate usernames unless account is 'ghosted'" should not be introduced. These types of conditional rules are very complex to actually implement and much of the system would not be suited to account for it.

new event(s) should be triggered to indicate the "ghosting" of a user record

What is the need for new events that cannot be accounted for in the existing before/after save events?

@ggppdk

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2018

Ok things are getting clearer on how this should be implemented

What remains in the database should still be valid data, even if pseudo-anonymized random stuff, and any validation rules should still apply. If today it is not allowed to duplicate usernames, a condition for "no duplicate usernames unless account is 'ghosted'" should not be introduced. These types of conditional rules are very complex to actually implement and much of the system would not be suited to account for it.

  1. so we agree that since such conditionals should be avoided and anyway there will be no memory of ghosted usernames and emails,
    both of them will allowed for new acounts

  1. about what data should be added in the ghosted user record (DB), it is not clear to me
    do we really want a random name ? and a random email ?

a. the displayed name should be "Ghost" or "Deleted user" or similar and not a random one because we do not want people to be able to distiguish between ghosted user content, it should not be possible to a web-sit's visitors to see that ghostAAA wrote these 3 comments, and ghostBBB wrote those 4 comments

b. the email should not be usable so that to no attempts should be made to send emails to it


  1. about having a way to identify ghosted records, i think it is needed

e.g. easily exclude them from actions like promotional (newsletters) emails, etc)
and with a later PR be able to filter them in users manager too
and be possible in any extensions to create (if they want / need) a "users" like filter that can filter their views based on ghosted accounts)
a way to distiguish them can be ?


  1. about events yes it can be done with existing events, no need for new events
@alikon

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2018

please test/comment #20173

@alikon alikon closed this Apr 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.