Delete Account #2472

TheAspens · 2018-04-17T23:25:50Z

This pull request resolves issue #2447

This will be a work in progress for a bit, so please don't merge. However, since it is a large change I wanted to push incremental updates for review if people want to look.

<enable_delete_account/> is set in config.xml

create token and send email)

Ageless93 · 2018-04-18T16:56:15Z

The one thing I notice in all the code is the blurb at the top that says:

// This file is part of BOINC.
// http://boinc.berkeley.edu
// Copyright (C) 2018 University of California

It would be nicer if the URL was immediately made https, so that you don't have to do that again in the future.

Conflicts: html/inc/token.inc html/ops/test_token.php

single php file

TheAspens · 2018-04-18T22:44:32Z

@Ageless93 - thanks for the feedback. I've updated them to https.

Uplinger · 2018-04-19T18:34:13Z

html/inc/token.inc

+// Constants for token durations
+define("TOKEN_DURATION_ONE_DAY", "86400");
+
+function create_confirm_delete_account_token($user) {


Can this function and the is_valid_delete_account_token be generalized. I think having them generalized will help decrease the amount of code as well as the maintenance of it going forward. Having functions that call say create_token($userid, $type, $expire_time = null). This way anywhere that needs to use a new token can easily call it. At the moment we have 2 types needed, one for delete account and email change, but this can be used for many more types going forward.

is_valid_token($userid, $token, $type) is for the other generalized function

I'm going to implement this on #2474 and then merge into this branch.

performed

erased code. This is the implementation that will be used for the 'obfuscate' account mechanism.

project config.xml

TheAspens · 2018-05-01T19:51:11Z

@davidpanderson - left you a message. Give me a ring or I'll call you again tomorrow.

davidpanderson · 2018-05-01T20:23:22Z

Kevin: I'm in DC at a mandatory NSF meeting. I should be home around 1 PM tomorrow; will try you. -- D

…

On Tue, May 1, 2018 at 12:51 PM, Kevin Reed ***@***.***> wrote: @davidpanderson <https://github.com/davidpanderson> - left you a message. Give me a ring or I'll call you again tomorrow. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2472 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA8KgY3KdtZRt2Bl7vJESk1ShyIEC6EPks5tuLywgaJpZM4TZMfx> .

deleting a user

is removed from the system. add task to remove entries from those tables after 60 days.

id instead of userid and hostid

Also modify db_dump to exclude user records whose authenticator starts with 'deleted' or host domain names that equal 'deleted'. Those values are set by the obfuscate delete method.

host_deleted

TheAspens · 2018-05-16T21:10:40Z

@davidpanderson - if you could review also and if both you and @brevilo are satisfied, then please merge as well. Thanks!

davidpanderson · 2018-05-17T00:45:23Z

Fine with me. Oliver, merge if OK with you.

brevilo

While it almost feels like nitpicking, I'm not really happy with the abbreviated index names. I mean, they're almost never to be typed anyway so I don't see the justification to reduce their readability (contextual meaning) in favor of length. Opinions?

brevilo · 2018-05-17T09:06:26Z

However, to reduce the chance (of something that is already a very small chance), I will take the step to invalidate the authenticator and add a short delay of 2 seconds. That is more time than most of our scheduler requests take and this would make the scenario you describe vanishingly unlikely.

@bema-ligo are you happy with this or does it (still) mean a no-merge for you? Please review Kevin's changes regarding this.

TheAspens · 2018-05-17T13:48:11Z

While it almost feels like nitpicking, I'm not really happy with the abbreviated index names. I mean, they're almost never to be typed anyway so I don't see the justification to reduce their readability (contextual meaning) in favor of length. Opinions?

I've made the names long. However, many indexes previously had shortened names so I had been following that convention.

can be customized by projects in the project.inc file

TheAspens · 2018-05-17T14:00:55Z

@bema-ligo and @brevilo,
I've added commit: ae72004 which make the length of the delay something that projects can configure in the project.inc file. Since the timeouts and reasonable values depend on different conditions on different projects, this allows projects to choose a setting that works for them with a default that will work properly in the majority of cases.

brevilo · 2018-05-17T14:11:16Z

Thanks Kevin, sounds good to me. I'll merge as soon as I here from @bema-ligo .

brevilo · 2018-05-17T14:14:25Z

How about your review @Uplinger? Are you satisfied? If so, please acknowledge/accept the review changes.

Ideally @davidpanderson would do the same such that all reviews are visibly done (upper right).

brevilo · 2018-05-17T14:16:59Z

Duh!

TheAspens · 2018-05-17T14:18:44Z

I've completed my regression testing. Once I review the scrutinizer feedback it should be good for merge.

TheAspens · 2018-05-17T14:59:27Z

I've incorproated the scruitinizer feedback as pertains to my code. Note that it has a false bug report that I've submitted to them for review.

TheAspens · 2018-05-17T14:59:50Z

I'm done with the code. Please merge when you are satisfied.

TheAspens · 2018-05-17T15:31:06Z

@brevilo - Can you go over to Bernd's office and discuss with him? All other reviewers have approved (either inline in the discussion or formally via the review feature). Sorry for pushing, but since WCG is not up to current code, we have some work to do to integrate this into our code and @Uplinger is blocked getting started on that until this is in master.

brevilo · 2018-05-17T15:36:26Z

Sorry for pushing, but since WCG is not up to current code, we have some work to do to integrate this into our code

Yep, same here. I reckon we can still optimize things later should Bernd have further input. So let's go...

brevilo · 2018-05-17T15:37:32Z

Thanks Kevin for all the effort you put into this! Thanks also to all the others involved in various ways 👍

bema-aei · 2018-05-18T07:25:58Z

Sorry for the delay at my end. FTR: I'm fine with the solution (configurable time).

Ageless93 · 2018-05-23T11:49:42Z

Since this was merged 6 days ago, can it be tested someplace for real? I haven't seen this code be available yet at for instance Seti@Home. Not that I want to test it there, but wouldn't mind testing it at a test project. The 25th is less than two days away.

brevilo · 2018-05-23T11:58:42Z

We already deployed a custom solution at Einstein@Home as well as our test instance Albert@Home. However, we're currently waiting for #2524 to be merged which will then make use of the features added by this PR.

Kevin Reed added 6 commits April 10, 2018 09:53

web: add php pages for support of right to erasure

3689cda

web: add feature flag to only show delete account if

27b1274

<enable_delete_account/> is set in config.xml

web: Implement remove_account_request page

5ed98ba

Merge branch 'master' into knr_right_to_erase

2e857de

web: Implement delete_account_request_action.php (verify password,

3cac897

create token and send email)

web: complete UI for delete account

a726238

This was referenced Apr 18, 2018

MySQL doesn't allow integers to have default values of functions #2473

Closed

web: Fix bug creating token table with MySQL and MariaDB < 10.2 #2474

Merged

Kevin Reed added 3 commits April 18, 2018 17:18

web: refactor delete account pages to reduce duplicate code

00440ad

Merge branch 'knr_token2' into knr_right_to_erase

059d4e0

Conflicts: html/inc/token.inc html/ops/test_token.php

web: At request of David, consolidate form and action pages into a

501c9a5

single php file

Uplinger requested changes Apr 19, 2018

View reviewed changes

Kevin Reed added 2 commits April 30, 2018 14:14

Merge branch 'master' into knr_right_to_erase

0485c36

web: Update calls to get and validate tokens to match changed code

da81c49

TheAspens mentioned this pull request Apr 30, 2018

Add a mechanism allowing project admins to "delete" a user #2445

Merged

Kevin Reed added 5 commits April 30, 2018 15:08

web: add switch to choose which type of account delete should be

65c7cfd

performed

Merge branch 'master' into knr_right_to_erase

fb85bd3

web: integrate changes from PR 2445 into new delete account/right to be

2e360d6

erased code. This is the implementation that will be used for the 'obfuscate' account mechanism.

web: add table definitions for user_deleted and host_deleted

fd06682

web: add <enable_delete_account>0</enable_delete_account> to default

d67a8c4

project config.xml

Kevin Reed added 6 commits May 1, 2018 17:08

web: add logic to insert into user_deleted and host_deleted when

e821de2

deleting a user

web: add logic to insert into user_deleted and host_deleted when account

4002f6d

is removed from the system. add task to remove entries from those tables after 60 days.

web: change primary key for user_deleted and host_deleted to simply be

b7bb437

id instead of userid and hostid

server: add user_deleted and host_deleted output files for db_dump.

ae17dfb

Also modify db_dump to exclude user records whose authenticator starts with 'deleted' or host domain names that equal 'deleted'. Those values are set by the obfuscate delete method.

server: db_dump shouldn't export create_time for user_deleted or

b4f0bae

host_deleted

web: implementation of wipe account

bcd853b

web: fix issues identified by scruitinizer

f799c9c

brevilo reviewed May 17, 2018

View reviewed changes

db: change index names to not be shortened

7ce2370

web: Make the delay after invalidating the autheniticator something that

ae72004

can be customized by projects in the project.inc file

brevilo approved these changes May 17, 2018

View reviewed changes

web: add mising closing parenthesis

b6804ab

Kevin Reed added 2 commits May 17, 2018 09:46

web: remove excess space

0c2b0bc

web: fixing spacing identified by scruitinizer

83ea2bc

Uplinger approved these changes May 17, 2018

View reviewed changes

brevilo merged commit 16bbbea into master May 17, 2018

TheAspens mentioned this pull request May 17, 2018

EU-GDPR - Right to Erasure #2447

Closed

TheAspens deleted the knr_right_to_erase branch May 17, 2018 20:24

darkdragon-001 mentioned this pull request Jun 27, 2020

Android client FoldingAtHome/fah-issues#1321

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete Account #2472

Delete Account #2472

TheAspens commented Apr 17, 2018

Ageless93 commented Apr 18, 2018 •

edited

Loading

TheAspens commented Apr 18, 2018

Uplinger Apr 19, 2018

TheAspens Apr 20, 2018

TheAspens commented May 1, 2018

davidpanderson commented May 1, 2018 via email

TheAspens commented May 16, 2018

davidpanderson commented May 17, 2018

brevilo left a comment •

edited

Loading

brevilo commented May 17, 2018 •

edited

Loading

TheAspens commented May 17, 2018

TheAspens commented May 17, 2018

brevilo commented May 17, 2018

brevilo commented May 17, 2018

brevilo commented May 17, 2018

TheAspens commented May 17, 2018

TheAspens commented May 17, 2018

TheAspens commented May 17, 2018

TheAspens commented May 17, 2018

brevilo commented May 17, 2018

brevilo commented May 17, 2018

bema-aei commented May 18, 2018

Ageless93 commented May 23, 2018

brevilo commented May 23, 2018 •

edited

Loading

Delete Account #2472

Delete Account #2472

Conversation

TheAspens commented Apr 17, 2018

Ageless93 commented Apr 18, 2018 • edited Loading

TheAspens commented Apr 18, 2018

Uplinger Apr 19, 2018

Choose a reason for hiding this comment

TheAspens Apr 20, 2018

Choose a reason for hiding this comment

TheAspens commented May 1, 2018

davidpanderson commented May 1, 2018 via email

TheAspens commented May 16, 2018

davidpanderson commented May 17, 2018

brevilo left a comment • edited Loading

Choose a reason for hiding this comment

brevilo commented May 17, 2018 • edited Loading

TheAspens commented May 17, 2018

TheAspens commented May 17, 2018

brevilo commented May 17, 2018

brevilo commented May 17, 2018

brevilo commented May 17, 2018

TheAspens commented May 17, 2018

TheAspens commented May 17, 2018

TheAspens commented May 17, 2018

TheAspens commented May 17, 2018

brevilo commented May 17, 2018

brevilo commented May 17, 2018

bema-aei commented May 18, 2018

Ageless93 commented May 23, 2018

brevilo commented May 23, 2018 • edited Loading

Ageless93 commented Apr 18, 2018 •

edited

Loading

brevilo left a comment •

edited

Loading

brevilo commented May 17, 2018 •

edited

Loading

brevilo commented May 23, 2018 •

edited

Loading