hubzero / hubzero-cms

Platform for Scientific Collaboration
https://hubzero.org
GNU General Public License v2.0
46 stars 57 forks source link

[NCN-341] DeIdentify Members From Admin #1636

Closed jessewoo closed 1 year ago

jessewoo commented 1 year ago

Reference

Summary of the Issue

Summary of the Development

Testing Strategy

How to Rollout

Assigned?

pascal-meunier commented 1 year ago

In the file deidentify.php, the new code is clean and easily read, and should be fairly easy to maintain, thank you. However, the queries in the plugin do not match the queries in the playbook.

Please do not delete in the tables jos_kb_articles and jos_content; either remove the SQL queries below or comment them out:

    $delete_KbArticles_Query = "DELETE from `#__kb_articles` where created_by='" . $user_id . "'";
    $delete_Content_Query = "DELETE from `#__content` where created_by='" . $user_id . "'";

There are missing SQL queries, such as the ones that delete rows in the tables jos_users_quotas_log and jos_users_log_auth. Please add them and verify that there aren't any other missing ones.

At the end, there are "update" SQL queries which are duplicated by userID and by email. However, only one of the 2 ways of doing it needs to be used. The playbook did it by email, but it would be simpler and consistent to do it by userID -- thank you for providing that. Please remove the SQL queries that update by email. The lines 204 (update_XProfilesByEmail_Query) and 210 (update_UsersByEmail_Query) are not needed.

jessewoo commented 1 year ago
    $delete_UsersQuotasLogByUserName_Query = "DELETE from `#__users_quotas_log` where name=" . $db->quote($userName);
    $this->runUpdateOrDeleteQuery($delete_UsersQuotasLogByUserName_Query);
jessewoo commented 1 year ago
    $delete_UsersLogAuth_Query = "DELETE from `#__users_log_auth` where user_id=" . $db->quote($userId);
    $this->runUpdateOrDeleteQuery($delete_UsersLogAuth_Query);
pascal-meunier commented 1 year ago

This looks great Jesse! However, I found a bug in the Ansible playbook, so I apologize for requesting a change even though your code meets the initial request. It's possible for a user to have many jos_auth_link entries; I found that some users have up to 5 authentication methods. Would you be able to loop through all the returned userLinkId to delete from the tables jos_auth_link and jos_auth_link_data please? What I mean is on line 64:

$userLinkId = $authLinkJsonObj[0]['id'];

instead of getting only the first id, we need to loop through the ids, with the lines 137-141 inside the loop. Again, sorry about this last minute change from the initial specification.

In the Ansible playbook, we delete the home directory. There's an easy way to do this from PHP, because at least in our hub deployments, home directories are available to /webdav/home. Could you please delete the user's home directory? The path to delete is /webdav/home/$userName. This should be close to working code: if ($userName) { $cmd = "/bin/rm -rf /webdav/home/" . escapeshellarg($userName); system($cmd, $retval); }

After this let's accept your pull request. Thank you very much!

pascal-meunier commented 1 year ago

After testing the deletion operation, I had to change the proposed code to:

if ($userName) {
    // as user Apache, we can't delete the directory so we need to delete all files.
    $cmd1 = "/bin/rm -rf /webdav/home/" . escapeshellarg($userName) . "/*";
    system($cmd1, $retval);

    // delete the files starting with a dot; ignore messages about '.' and '..'
    $cmd2 = "/bin/rm -rf /webdav/home/" . escapeshellarg($userName) . "/.*";
    system($cmd2, $retval);
}
jessewoo commented 1 year ago

@pascal-meunier - updated the code to make sure it loops thru auth links and delete home directories

pascal-meunier commented 1 year ago

Thank you Jesse. I believe that it should be approved. However, I'm not a reviewer so I can't approve it.

dbenham commented 1 year ago

Lets review this PR at the dev meeting tomorrow

monaw commented 1 year ago

i would recommend adding a tooltip on the button that will point the user to the location of the documentation that describes what will happen when they press the de-identifcation button

jessewoo commented 1 year ago

Worked with @pascal-meunier, think we can finalize it, and we would like @dbenham to merge it so we can view on dev.nanohub.org to get some eyes on it, test it out.