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

User Opt-in consent #2504

Merged
merged 78 commits into from
Oct 25, 2018
Merged

Conversation

drshawnkwang
Copy link
Contributor

Resolves GDPR User Opt-in Consent #2413.

This is a work in progress (WIP) merge. I wish to allow people to review what I have done and comment on it. As of this initial PR the following features are present.

  1. New consent and consent_type tables which record a user's consent.
  2. Web form now has terms of use (terms_of_use.txt) file which forces users to opt-in.
  3. create_account and am_set_info RPCs modified to record consent.
  4. HTML Ops page added for admin to manage consent_type table.

Shawn Kwang added 6 commits May 4, 2018 18:40
Add tables consent and consent_type. Projects automatically created with a pre-defined consent_type. When projects update database, pre-defined consent_type will be added as well.
create_account RPC modified to record a user's consent.
am_set_info RPC modified to allow for modification of user's consent.
Account registration on Web site modified to show terms_of_use.txt file and record user's consent.
Admin may add addiational consent types to the table.
When a user logs in, and they have not yet consented, the user is presented with a form to consent to the Terms of Use. This adds or modifies records in the consent table.
Supported logins are: web form, authenticator, email_link, and ldap.
Shawn Kwang added 4 commits May 17, 2018 10:29
…ort' option.

Added new preferences sub-class for consent table modification.
Added privacy preference form item to record user giving consent to data exports.
User's consent is recorded in the consent table.
@drshawnkwang drshawnkwang changed the title WIP: User Opt-in consent User Opt-in consent May 17, 2018
@drshawnkwang
Copy link
Contributor Author

drshawnkwang commented May 17, 2018

@TheAspens , @davidpanderson

I am going to ask that people start reviewing this code for merging. We especially wanted someone from outside E@H to look at this code.

I've done some (alpha) testing on my own. But this was using an 'empty project' with zero users and no Web content. If there are additional unit tests that could be done, please let me know.

Shawn Kwang added 3 commits May 17, 2018 14:30
html/inc/prefs_util.inc Outdated Show resolved Hide resolved
html/inc/prefs_util.inc Outdated Show resolved Hide resolved
if (!$rc) {
xml_error(-1, "database error, please contact site administrators");
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If consent is required but whoever is creating the account knows nothing of consent then what? I think the code that checks that consent was given should go before make_user call.

Copy link
Contributor Author

@drshawnkwang drshawnkwang May 22, 2018

Choose a reason for hiding this comment

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

Right now there are multiple ways this Web RPC is called, including simply typing in https://project_url/[email protected]&passwd_hash=ahash&username=name in to a browser URL bar.

This means anyone can create an account without knowledge of a project's terms of use/opt-in consent. The boinc-command line client is another place where you can create an account without ever seeing the terms of use and consenting to it.

Ultimately, projects which really care about GDPR, i.e., located in Europe, should turn off this create_account RPC and only allow users to register via their Web site, which guarantees the user sees the terms of use.

Another way of putting this: It is assumed that the user has already consented to the project's terms of use before this RPC is called.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking that Manager and boinccmd would be later enhanced to pass optin and source parameters. A user could be using some third party manager that doesn't show TOU and the user might not know he needs to give consent.

I was under the impression that new (non-anonymous) accounts can't be created unless the user consents to TOU. But that raises the question, if user has not given or revoked consent to TOU at all, then does that make the account automatically anonymous?

html/user/user_optin_action.php Outdated Show resolved Hide resolved
html/inc/consent.inc Outdated Show resolved Hide resolved
html/inc/prefs_project.inc Outdated Show resolved Hide resolved
html/ops/manage_consent_types.php Outdated Show resolved Hide resolved
$cquery .= "consent_id=$consent_id";
if ($consent_settime==1) {
$cquery .= ",consent_time=$now";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better if consent_time is set any time consent is given or changed and account managers have no say in the matter.

As for the rest of the consent parameters I would check that either they are all set or none of them are set. I don't think setting only some of them would work well.

Copy link
Contributor Author

@drshawnkwang drshawnkwang May 22, 2018

Choose a reason for hiding this comment

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

I agree with you in principle, but I received very little feedback from AM developers on what would be best for them to be able to change, hence a very flexible/open "API" for interaction.

@JuhaSointusalo
Copy link
Contributor

About revoking consent. I don't think it's possible to just record the fact and then carry on. Revoking consent requires some more actions.

If we require third parties to remove any data about a user after the user deletes their account then I would expect that we need to do the same when the user revokes permission to export data about his account. The user_deleted and host_deleted that were added for deleting accounts would work for this too I think, even though nothing was really deleted. This needs to be handled in both am_set_info RPC and when saving project preferences.

As for revoking general terms of use, I don't think it's possible to handle that in am_set_info RPC at all. Revoking consent to TOU is the same as deleting account. Quite a bit of effort went into making delete account feature secure and if it were possible to revoke the consent with am_set_info then it would bypass the new security additions. I think revoking consent to TOU needs to be blocked entirely and user redirected to delete account feature.

@drshawnkwang
Copy link
Contributor Author

drshawnkwang commented May 22, 2018

@JuhaSointusalo , could you copy and paste your comment into the Issue ticket #2413. Let's keep discussion about the PR here, and other (good) ideas about the user consent feature in the Issue ticket.

This is because I think you have some good points that require discussion.

@drshawnkwang
Copy link
Contributor Author

Shouldn't this update consent_time and maybe source too?

@JuhaSointusalo , you asked this a few times and the answer is: it's complicated. The assumption is you are updating a row in the consent table, where consent_time and source are already set. Should these be updated from their original time/source? Is the record static and the user allowed to give and then revoke consent?

Part of the answer is related to your longer comment. It depends on the design of the user opt-in functionality.

Changed name of consent function to be more generic.
Added translation of ops page text.
@drshawnkwang drshawnkwang changed the title User Opt-in consent WIP: User Opt-in consent May 24, 2018
@drshawnkwang drshawnkwang changed the title WIP: User Opt-in consent User Opt-in consent May 24, 2018
@drshawnkwang drshawnkwang changed the title User Opt-in consent WIP: User Opt-in consent Oct 5, 2018
@JuhaSointusalo
Copy link
Contributor

One more thing. When the create account process is started by BOINC Manager (and maybe other GUIs), the account is created with user name that is the same as the user's account on their computer. I would imagine this is private information. Can you store the user name without having the user agree to it? So can you store the user name and then wait for the user to log in on the website to agree to terms?

@JuhaSointusalo
Copy link
Contributor

I think the follow TheAspen's idea is a good one: adding a user_agreed_tou parameter to create_account RPC.

There is that consent_flag. I think it would work like this:

  • old Manager or third party GUI that doesn't fully support this consent stuff
    -> consent_flag not set
  • new GUI, terms showed and user agreed
    -> consent_flag=1
  • new or older GUI, terms showed and user disagreed
    -> no create account RPC at all

This is only when using GUIs. Account managers can still have consent_flag=0.

@drshawnkwang
Copy link
Contributor Author

Juha is correct, the additional parameters user_agreed_tou is not required. The consent_flag states should have all the necessary information. I will ret-con my earlier long comment.

Re: the username, #1929, there is an issue I created. I think this is a client side issue because the username is passed to create_account RPC. No one has worked on it though.

@drshawnkwang drshawnkwang changed the title WIP: User Opt-in consent User Opt-in consent Oct 15, 2018
}

BoincConsentType::insert(
"(shortname, description, project_specific) VALUES ('$shortname', '$description', 1)"
Copy link
Member

Choose a reason for hiding this comment

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

enabled and privacypref are not-null but do not have default values. Therefore this insert fails. Either give them default values in the db definition or add them with values in this section of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 7d7c187

@TheAspens
Copy link
Member

While testing I noticed that db_dump works correctly in these cases:

  • If STATSEXPORT consent type enabled, then users and hosts that have not consented are not exported
  • If STATSEXPORT consent type is disabled, then all users and hosts are exported correctly

However, the file tables.xml should reflect the same value without regards to the STATSEXPORT consent type. Since this file is an aggregate field, GDPR restrictions are not a concern. This file will be very important going forward since it will be the only way for sites like BOINCStats to be able to present comparisons of number of users, hosts and total credit between projects. If data vanishes from this aggregate level file then there will be no way to see that total level at projects.

@TheAspens
Copy link
Member

Those are the only two items I found. Once they are resolved, I'll be ready to merge.

Shawn Kwang added 3 commits October 22, 2018 08:49
…export consent.

If the project does have stats exports consent, and a user does NOT consent to stats. exports: then the total user and host in table.xml should still include the user. This is an aggregate total of the number of users and hosts that use the project.

Introduces an additional count query to count the total number of users and hosts regardless of the stats-export consent functionality.
Fixed conflicts and prepare for merge into master.
@drshawnkwang
Copy link
Contributor Author

I changed db_dump to have an independent count of the number of users and hosts for a project.

I also merged in master into this branch to resolve merge-conflicts.

@TheAspens
Copy link
Member

@drshawnkwang - two items on the db_dump. 1) If the user or host is deleted, it should not be included (the logic in the db_dump is for the "scrub" option of the delete user code). 2) total credit needs to include all users without regard to the STATSEXPORT setting.

@TheAspens
Copy link
Member

One additional question that I noticed this time testing. If the terms of use file is present, then the manager will show the terms of use without regard to whether the consent_type 'ENROLL' is enabled or not. However, the website only shows the terms of use if the consent_type 'ENROLL' is enabled. I think that it is odd for it to behave differently in different places.

I would suggest that either:

A) Terms of use on a call to get_project_config.php should only be shown if both terms_of_use.txt exists and the ENROLL consent type is enabled
or
B) If terms_of_use.txt exists, show it on the manager and on the website always without regard to the ENROLL consent type.

I think A is more consent with the design of the consent_type system - but I don't have a strong opinion.

Shawn Kwang added 2 commits October 23, 2018 11:06
RPC checks if consent_type 'ENROLL' is enabled by the project. If this is true and if the terms-of-use file exists, then show the contents of the file in the XML output.
Summary of total_credit, number of users, and number of hosts now are summed first. Users with authenticator 'deleted*' are ignored. Hosts with userid=0 and domain_name 'deleted' are ignored. total_credit summation only uses non-deleted users.
@drshawnkwang
Copy link
Contributor Author

Latest changes: db_dump correctly takes into account delete users and hosts.

From Kevin's above comment, 'Option A' implemented. - the get_project_config ignores the terms-of-use file if the ENROLL consent type is disabled

db_dump now builds a SQL statement out of different clauses: JOIN, WHERE, and ORDER BY are separated into different c-strings. Additionally, different clauses are used to build different queries for different tables: host, user, team.
@drshawnkwang
Copy link
Contributor Author

Latest changes: db_dump correctly takes into account delete users and hosts.

The changes were buggy: this should be fixed with the latest commit: which revamps how the SQL clause is generated.

@JuhaSointusalo
Copy link
Contributor

Maybe it would be good to document that after creating an account with old Manager, enable_login_mustagree_termsofuse=1 takes effect and records are created only after logging out and back in. After adding a project and creating an account at the same the Manager opens account_finish.php which logs the user in with authenticator.

Shawn Kwang added 2 commits October 25, 2018 09:40
Admin may not delete consent types after creation. They may still be disabled.
@drshawnkwang
Copy link
Contributor Author

Juha, You are correct, the old manager behavior is a bit confusing because it looks like you consented to the terms-of-use, when the database is not changed. I will try to document this in the BOINC wiki.

Latest commits: we removed the option for admins to delete a consent_type from the consent_type table in the HTML OPS pages. This is because of a foreign key constraint in the consent table. And there may be legal reasons to keep all these records in the consent table. Admins may still disable a consent_type if there is no need for its use.

@TheAspens
Copy link
Member

I've tested and reviewed and I believe that open items have been resolved (with 156 comments, it is hard to be sure so if I missed something, I apologize). Thanks for the contribution @drshawnkwang and everyone for their work reviewing and testing this piece of code!

@TheAspens TheAspens merged commit 4198df0 into BOINC:master Oct 25, 2018
@brevilo
Copy link
Contributor

brevilo commented Oct 26, 2018

Well done everyone and a big THANK YOU to Shawn and Kevin for taking on most of the work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants