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

Add cookie consent option #3624

Open
jmacgreg opened this Issue Apr 20, 2018 · 27 comments

Comments

4 participants
@jmacgreg
Copy link
Member

jmacgreg commented Apr 20, 2018

OJS/OMP need a cookie consent option. This should:

  • provide a pop-up note describing the application's cookie policy;
  • stop any cookie creation until this consent statement has been agreed to.

I am not sure if this should be an "option", configurable by JMs, but if it is the option should probably go under Site Access Options.

@jmacgreg jmacgreg added this to To Do in GDPR Compliance Enhancements via automation Apr 20, 2018

@jmacgreg

This comment has been minimized.

Copy link
Member

jmacgreg commented Apr 30, 2018

Notes from @ajnyga, pulled over from other docs:

  1. I think cookies are only used to store a session ID (cookie called OJSSID). But that session ID is then connected to the sessions database table that has user IP and browser details strored. So the cookies themselves are not personal data, but they are linked to such data. I am actually not 100% sure if OJS needs a session if the user does not log in? At the moment it does create the OJSSID cookie immediately when the user enters a OJS page.

While the OJSSID cookie itself does not contain personal data, I think the most important thing is to erase the old sessions from the sessions database table as fast as possible.

What I am thinking here is that would it be possible to get the cookie consent as part of the user registration. Alec of course knows if sessions are needed also for users that are not logged in.

  1. Note that OJS does create a cookie also for visitors. But after writing the comments above, I realized that OJS does use a csrf tokens also in search form that are stored as part of sessions. But not sure if that is actually needed.

But if OJS can be easily used without cookies when user is not logged in, then maybe add the cookie consent to the login form? Just a simple checkbox, "I accept cookies".

@ajnyga

This comment has been minimized.

Copy link
Contributor

ajnyga commented May 2, 2018

An overview on how the above suggestion could work (ping @jmacgreg @asmecher @bozana):

  1. Only start new user sessions (and create OJSSID cookies) when user is logged in (check that the login, register and search forms work without csrf tokens for users who are not logged in?)
  2. In the login form, add a cookie consent checkbox. The user has to check it in order to login.
  3. When user gives consent while logging in, save that consent to the database (user_settings) and add a new cookie "gdpr_consent". This cookie will check the consent checkbox automatically when user opens the login form the next time (not actually needed, but would make things smoother).

The alternative of course is that you ask for the consent from all users visiting an OJS site, also those that are not logged in. But I can not figure out where/how you would save the user consent for users who do not accept cookies because I have understood that you need to have the consents before creating cookies and OJS now creates one immediately when you enter an OJS site.

Happy to help here if you just point to way you want to go.

@asmecher

This comment has been minimized.

Copy link
Member

asmecher commented May 2, 2018

@ajnyga, sounds good, though of course you could use the existing OJSSID cookie to store the GDPR consent status rather than introducing an entirely new one. Unless you think it would be useful to give the two cookies different characteristics.

There are a few things that use sessions before a user is logged in (e.g. the language), so this would be a good optional behavior rather than something OJS always did (at least until GDPR-type requirements become more common, which is possible too).

@ajnyga

This comment has been minimized.

Copy link
Contributor

ajnyga commented May 3, 2018

thanks @asmecher
You are right, you could just store the OJSSID and then check if that exists when the user returns. No reason to add new cookies.

I believe that cookies that store things like language selections are not regarded as cookies that need consent under GDPR because they do not contain or lead to personal data. So one option would be to add selections like language to another cookie. See https://www.automated-intelligence.com/news-and-events/blog/cookies-gdpr-need-know/ ("if you use cookies to uniquely identify a device or the person using that device, that’s now treated as personal data under GDPR"). But this is something @jmacgreg could confirm.

Also, I am not sure how it is enforced, but theoretically GDPR also applies to journals that have users coming from the EU, which probably means the majority of journals in Canada (http://www.canadianlawyermag.com/article/getting-ready-for-gdpr-3607/). Of course the main target here are companies, not journals.

@ajnyga

This comment has been minimized.

Copy link
Contributor

ajnyga commented May 3, 2018

Another question @asmecher how does OJS empty the sessions table at the moment? Thinking if we could have an automated task that does that for inactive sessions once a day.

@asmecher

This comment has been minimized.

Copy link
Member

asmecher commented May 7, 2018

@ajnyga, that's part of PHP's session manager support; see session_set_save_handler, which lib/pkp/classes/session/SessionManager.inc.php calls to map PHP's session management into the sessions database table. The gc (garbage collection) function is documented at the link above, including some php.ini parameters that can fine-tune its behavior.

@ajnyga

This comment has been minimized.

Copy link
Contributor

ajnyga commented May 11, 2018

@jmacgreg @asmecher (and @NateWr hoping to see your comments as well, you are not out of the European union yet!)

Round-up of the things I am planning to do:

  • New setting to config.inc.php require_cookie_consent on/off (@asmecher requested above that this would be optional and I think this is the best place to control that)
  • In SessionManager around the line https://github.com/pkp/pkp-lib/blob/master/classes/session/SessionManager.inc.php#L33 I will add a if clause that checks if require_cookie_consent is enabled. if it is AND there is no cookie consent cookie available, do not create a new session. If require_cookie_consent is disabled just continue normally.
  • If a cookie consent cookie does not exist, show the cookie consent banner in the footer. It will say something like "The site uses cookies. Some functionalities like changing language, registering or logging in to the system will not work without them. See the Privacy Policy for a description on how cookies are used. [Allow Cookies]"
  • Clicking on the [Allow Cookies] button will use Javascript to create the cookie consent cookie. The cookie only stores the domain name.
  • If cookie consent cookie is not available and require_cookie_consent is enabled, disable the registration form and login form submit buttons and add a message notifying that you need to allow cookies to do this.

Why use a new cookie? Because, if I have understood correctly, the OJSSID has a very short lifespan and I think we want to prevent the situation where users have to click on the cookie consent all the time. The cookie consent cookie does not have any personal data, so we can give it a long lifespan.

Problems:

  • it would make sense that the cookie consent would be site wide,right? But here again we have the problem that 3.1.1.1 will not a have site wide privacy policy setting. So while the cookie consent text has to have a link to the privacy policy text, where does that link lead to?
  • Blocking the creation of cookies will create these errors:
PHP Fatal error:  Call to a member function getUserId() on null in lib/pkp/classes/security/Validation.inc.php on line 369
PHP Fatal error:  Call to a member function getSessionVar() on null in classes/i18n/AppLocale.inc.php on line 81
PHP Fatal error:  Call to a member function getUser() on null in lib/pkp/classes/core/PKPRequest.inc.php on line 581
PHP Fatal error:  Call to a member function getSessionVar() on null in lib/pkp/classes/security/Validation.inc.php on line 384
PHP Fatal error:  Call to a member function getCSRFToken() on null in lib/pkp/classes/template/PKPTemplateManager.inc.php on line 1453

But most of these can be prevented by first checking if $session has a value. With the CSRF token you can just set to CSRF token to null if there is no session. I can add these changes to a pr, but hope that especially with the last one @asmecher evaluates if there are security implications.

What do you think about the overall plan here? This should not be too much work actually.

The other option would be to change OJS so that no cookies are ever created if the user does not log in and the cookie consent is asked when the user logs in. But I could not figure out how to do as a feature you can enable/disable like Alec requested. I think it requires a lot more changes to the code.

@NateWr

This comment has been minimized.

Copy link
Contributor

NateWr commented May 14, 2018

I am probably not the best person to comment on a cookie notice, because I think they're often used when not needed. For that reason, I like the use of a config option here. I think it's ok to have any specific request link to that context's privacy policy. If you'd like, you could add a site-wide privacy policy field to use for requests to the site-wide context.

@ajnyga

This comment has been minimized.

Copy link
Contributor

ajnyga commented May 14, 2018

In GDPR case cookie notice is needed if the cookie has personal data. I know that OJSSID does not have, but it is connected to some pretty detailed data in the sessions table. So to be honest, I am not 100% sure the OJSSID cookies actually requires a cookie notice according GDPR.

See for example https://webmasters.stackexchange.com/questions/114973/are-session-cookies-exempt-from-consent-under-gdpr

What do you think, @jmacgreg @bozana ?

@jmacgreg

This comment has been minimized.

Copy link
Member

jmacgreg commented May 14, 2018

Agreed with the config option. My reading of the above link, @ajnyga, is that session cookies like OJS' probably don't need a notice, and we can advise people of that, but others will likely want the option.

@ajnyga

This comment has been minimized.

Copy link
Contributor

ajnyga commented May 15, 2018

Thanks for the feedback.

@asmecher can you as well look at my suggestion here: #3624 (comment)

If it sounds ok, I can do a pr based on that. I think it has a big impact on the way the system works so just to be sure I am missing something. The code changes are actually not that big I think.

@asmecher

This comment has been minimized.

Copy link
Member

asmecher commented May 17, 2018

@ajnyga, sorry for the delay.

Broadly speaking, without cookies (and without a replacement session mechanism), the $session object will either not exist or will be re-created with each request. Of those two options, obviously it makes sense not to create one. So yes, we'll need to go through for code that expects $session to always be available and ensure that it can handle a null.

I suspect there will not be many of these. The $session object is primarily used to map to the $user object and that is aleady nullable.

Another option -- just mentioning it for the sake of completeness -- is to use URL-based SIDs until the user has agreed to the cookie policy. I see a lot of potential downsides (e.g. security) and no real need to go there.

@ajnyga

This comment has been minimized.

Copy link
Contributor

ajnyga commented May 18, 2018

thanks! when do you plan to release 3.1.1.1?

@asmecher

This comment has been minimized.

Copy link
Member

asmecher commented May 18, 2018

We were hoping to have it out already, but it looks like it'll still be 1-2 weeks.

@ajnyga

This comment has been minimized.

Copy link
Contributor

ajnyga commented May 18, 2018

Ok! (maybe this should be mentioned in the forum GDPR thread?)

@ajnyga

This comment has been minimized.

Copy link
Contributor

ajnyga commented May 22, 2018

@NateWr @asmecher @jmacgreg

Hoping for one more comment to this issue...

Cookies that do not have personal data do not require consent. The session cookie itself does not have personal data, but the sessions table does have. However, I began to think whether we could just minimize the personal data in the session.

Before the user is logged in, the session table only has these:

  • session_id
  • ip_address
  • user_agent
  • csrf_token
  • some timestamps

I believe we could just encrypt the IP address and the user_agent string before we save it to the sessions table and always compare encrypted values.

For the login form, we could add a consent checkbox regarding the privacy policy, which would then mention what is saved to the cookie and to the sessions table.

What do you think? Is there a reason why we could not encrypt the IP and the user_agent?

@ajnyga

This comment has been minimized.

Copy link
Contributor

ajnyga commented May 22, 2018

The above would as simple as adding

$ip = md5($request->getRemoteAddr());
$userAgent = md5($request->getUserAgent());

here: https://github.com/pkp/pkp-lib/blob/master/classes/session/SessionManager.inc.php#L61-L62
(not suggesting that we use md5, but you get the picture)

@asmecher

This comment has been minimized.

Copy link
Member

asmecher commented May 30, 2018

Hmm, I'm not sure, but I have a feeling that's starting to circumvent the purpose of the GDPR regulations -- we're storing a key that can uniquely track the user without their consent. I'd lean towards the options that don't generate cookies at all until the consent was given; did you hit a blocker there?

@ajnyga

This comment has been minimized.

Copy link
Contributor

ajnyga commented May 31, 2018

In general GDPR does not care about cookies at all. It only cares about personal data. So if a cookie stores just some simple settings etc. you do not need consent.

IP address is regarded as personal data, but encrypted IP is clearly a different thing. At least I can not see how it would be personal data while it can not be used to identify a person.

But with the other approach I did not hit a blocker, just figured that this would be an easier solution and wanted to hear your thoughts. I am on a vacation right now (greeting from Mallorca), but will do a PR next Wednesday. I think only 4-5 files need minor changes.

Another thing with GDPR is that it encourages to limit the collection of personal data. So encrypting the IP address, if it is only used to compare existing sessions, would make OJS more GDPR compliant in any case. I have not found any other purpose for IP in the sessions table.

@ajnyga

This comment has been minimized.

Copy link
Contributor

ajnyga commented May 31, 2018

@asmecher

This comment has been minimized.

Copy link
Member

asmecher commented Jun 6, 2018

There's a potentially overlapping request at #1179 to exempt bot user agents from session management.

ajnyga added a commit to ajnyga/ojs that referenced this issue Jun 9, 2018

ajnyga added a commit to ajnyga/pkp-lib that referenced this issue Jun 9, 2018

ajnyga added a commit to ajnyga/ojs that referenced this issue Jun 9, 2018

ajnyga added a commit to ajnyga/ojs that referenced this issue Jun 9, 2018

ajnyga added a commit to ajnyga/pkp-lib that referenced this issue Jun 9, 2018

ajnyga added a commit to ajnyga/pkp-lib that referenced this issue Jun 9, 2018

ajnyga added a commit to ajnyga/pkp-lib that referenced this issue Jun 9, 2018

ajnyga added a commit to ajnyga/pkp-lib that referenced this issue Jun 9, 2018

@ajnyga

This comment has been minimized.

Copy link
Contributor

ajnyga commented Jun 9, 2018

edit: nevermind, figured it out

ajnyga added a commit to ajnyga/ojs that referenced this issue Jun 10, 2018

ajnyga added a commit to ajnyga/pkp-lib that referenced this issue Jun 10, 2018

ajnyga added a commit to ajnyga/pkp-lib that referenced this issue Jun 10, 2018

ajnyga added a commit to ajnyga/pkp-lib that referenced this issue Jun 10, 2018

@ajnyga

This comment has been minimized.

Copy link
Contributor

ajnyga commented Jun 10, 2018

Initial pull request for cookie consent pkp/ojs#2010

ajnyga added a commit to ajnyga/ojs that referenced this issue Jul 5, 2018

ajnyga added a commit to ajnyga/pkp-lib that referenced this issue Jul 5, 2018

ajnyga added a commit to ajnyga/pkp-lib that referenced this issue Jul 5, 2018

ajnyga added a commit to ajnyga/pkp-lib that referenced this issue Jul 5, 2018

ajnyga added a commit to ajnyga/pkp-lib that referenced this issue Jul 5, 2018

ajnyga added a commit to ajnyga/ojs that referenced this issue Jul 5, 2018

ajnyga added a commit to ajnyga/ojs that referenced this issue Jul 5, 2018

ajnyga added a commit to ajnyga/pkp-lib that referenced this issue Jul 6, 2018

ajnyga added a commit to ajnyga/pkp-lib that referenced this issue Jul 6, 2018

ajnyga added a commit to ajnyga/ojs that referenced this issue Jul 6, 2018

ajnyga added a commit to ajnyga/pkp-lib that referenced this issue Jul 6, 2018

ajnyga added a commit to ajnyga/ojs that referenced this issue Jul 6, 2018

@asmecher

This comment has been minimized.

Copy link
Member

asmecher commented Aug 8, 2018

With apologies for the delay, @ajnyga -- this is still waiting for review, correct?

@ajnyga

This comment has been minimized.

Copy link
Contributor

ajnyga commented Aug 10, 2018

Sorry for the delay, this new github UI does not seem to show new messages the way the old one did...

This is not waiting for review yet, because it is not passing tests. I will maybe try another test run today. Have been busy with other things.

@asmecher

This comment has been minimized.

Copy link
Member

asmecher commented Aug 10, 2018

Not a problem, just making sure I haven't forgotten something.

ajnyga added a commit to ajnyga/pkp-lib that referenced this issue Aug 25, 2018

ajnyga added a commit to ajnyga/ojs that referenced this issue Aug 25, 2018

ajnyga added a commit to ajnyga/ojs that referenced this issue Aug 25, 2018

@ajnyga

This comment has been minimized.

Copy link
Contributor

ajnyga commented Aug 25, 2018

A new try on this (tests running): pkp/ojs#2106

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment