Bug 29957 - Cookies not removed after logout
Summary: Cookies not removed after logout
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on: 29914
Blocks: 30305 28786 30525 30532
  Show dependency treegraph
 
Reported: 2022-01-27 10:36 UTC by Jonathan Druart
Modified: 2022-04-25 21:13 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
This patch adds a new config variable to koha-conf.xml called do_not_remove_cookie. By default, all cookies are cleared now. But you could uncomment the KohaOpacLanguage entry to preserve it.
Version(s) released in:
22.05.00,21.11.05


Attachments
Bug 29957: Clear cookies on logout (6.54 KB, patch)
2022-01-27 11:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29957: Set path for bibs_selected (1016 bytes, patch)
2022-01-27 11:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29957: Clear cookies on logout (6.60 KB, patch)
2022-01-31 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
[STABLE] Bug 29957: Set path for bibs_selected (1.05 KB, patch)
2022-01-31 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29957: Skip KohaOpacLanguage (727 bytes, patch)
2022-03-07 08:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29957: Clear cookies on logout (4.34 KB, patch)
2022-03-15 15:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Introduce Koha::CookieManager (8.01 KB, patch)
2022-03-15 15:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Replace clear_all_cookies in Auth.pm (2.12 KB, patch)
2022-03-15 15:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Clear cookies on logout (4.99 KB, patch)
2022-03-16 15:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Introduce Koha::CookieManager (8.79 KB, patch)
2022-03-16 15:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Replace clear_all_cookies in Auth.pm (2.13 KB, patch)
2022-03-16 15:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Adjust push @$cookie statements in Auth (6.11 KB, patch)
2022-03-16 15:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Clear cookies on logout (4.99 KB, patch)
2022-03-17 09:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Introduce Koha::CookieManager (9.87 KB, patch)
2022-03-17 09:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Replace clear_all_cookies in Auth.pm (2.12 KB, patch)
2022-03-17 09:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Adjust push @$cookie statements in Auth (5.67 KB, patch)
2022-03-17 09:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Let check_api_auth return created cookie (1.28 KB, patch)
2022-03-17 09:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Introduce Koha::CookieManager (9.88 KB, patch)
2022-03-17 13:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Replace clear_all_cookies in Auth.pm (2.12 KB, patch)
2022-03-17 13:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Adjust push @$cookie statements in Auth (5.67 KB, patch)
2022-03-17 13:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Let check_api_auth return created cookie (1.28 KB, patch)
2022-03-17 13:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Add example lines in koha-conf.xml (1.53 KB, patch)
2022-03-18 07:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Clear cookies on logout (5.02 KB, patch)
2022-03-23 07:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Introduce Koha::CookieManager (9.88 KB, patch)
2022-03-23 08:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: (follow-up) Turn allow list into deny list (8.19 KB, patch)
2022-03-23 08:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Clear cookies on logout (5.02 KB, patch)
2022-03-24 09:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Introduce Koha::CookieManager (9.88 KB, patch)
2022-03-24 09:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: (follow-up) Turn allow list into deny list (8.19 KB, patch)
2022-03-24 09:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Replace clear_all_cookies in Auth.pm (2.12 KB, patch)
2022-03-24 09:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Adjust push @$cookie statements in Auth (5.34 KB, patch)
2022-03-24 09:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Let check_api_auth return created cookie (1.28 KB, patch)
2022-03-24 09:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Add example lines in koha-conf.xml (1.58 KB, patch)
2022-03-24 09:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Add example lines in koha-conf.xml (1.46 KB, patch)
2022-03-24 10:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Add --keep-cookie option to koha-create (5.11 KB, patch)
2022-03-25 08:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Add --keep-cookie option to koha-create (5.12 KB, patch)
2022-03-25 08:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29957: Clear cookies on logout (5.04 KB, patch)
2022-03-25 12:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29957: Introduce Koha::CookieManager (9.89 KB, patch)
2022-03-25 12:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29957: (follow-up) Turn allow list into deny list (8.21 KB, patch)
2022-03-25 12:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29957: Replace clear_all_cookies in Auth.pm (2.13 KB, patch)
2022-03-25 12:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29957: Adjust push @$cookie statements in Auth (5.36 KB, patch)
2022-03-25 12:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29957: Let check_api_auth return created cookie (1.30 KB, patch)
2022-03-25 12:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29957: Add example lines in koha-conf.xml (1.48 KB, patch)
2022-03-25 12:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29957: Add --keep-cookie option to koha-create (5.13 KB, patch)
2022-03-25 12:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29957: (follow-up) correct spelling QA complaint (875 bytes, patch)
2022-03-25 12:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29957: Clear cookies on logout (5.10 KB, patch)
2022-03-28 07:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29957: Introduce Koha::CookieManager (9.96 KB, patch)
2022-03-28 07:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29957: (follow-up) Turn allow list into deny list (8.27 KB, patch)
2022-03-28 07:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29957: Replace clear_all_cookies in Auth.pm (2.20 KB, patch)
2022-03-28 07:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29957: Adjust push @$cookie statements in Auth (5.42 KB, patch)
2022-03-28 07:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29957: Let check_api_auth return created cookie (1.36 KB, patch)
2022-03-28 07:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29957: Add example lines in koha-conf.xml (1.54 KB, patch)
2022-03-28 07:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29957: Add --keep-cookie option to koha-create (5.20 KB, patch)
2022-03-28 07:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29957: (follow-up) correct spelling QA complaint (940 bytes, patch)
2022-03-28 07:54 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2022-01-27 10:36:26 UTC
With bug 29956 combined, this is a bit scary...
Comment 1 Jonathan Druart 2022-01-27 11:34:14 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2022-01-27 11:41:40 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2022-01-27 12:40:23 UTC
Hmm
Comment 4 Marcel de Rooy 2022-01-27 12:45:03 UTC
At first glance we should not push this before the other things.
Comment 5 Jonathan Druart 2022-01-27 12:57:37 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2022-01-27 12:57:54 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2022-01-27 12:58:39 UTC
(lol still not)

(In reply to Marcel de Rooy from comment #4)
> At first glance we should not push this before the other things.

I think it should be pushed after bug 29914 but before 29915. Would you agree with that?
Comment 8 Marcel de Rooy 2022-01-27 13:06:11 UTC
(In reply to Jonathan Druart from comment #7)
> (lol still not)
> 
> (In reply to Marcel de Rooy from comment #4)
> > At first glance we should not push this before the other things.
> 
> I think it should be pushed after bug 29914 but before 29915. Would you
> agree with that?

No, I wouldnt. Not so sure if this is crucial and should postpone other work too. Making sure that we do not include sensitive info in a cookie is more important. Like 29956.
Comment 9 Jonathan Druart 2022-01-27 13:21:43 UTC
With this patch we are making sure all cookies will be removed on logout. Doing it client-side is more fragile.

This patch is a security issue (data leak) when bug 29915 is not.
Comment 10 Marcel de Rooy 2022-01-27 13:26:23 UTC
(In reply to Jonathan Druart from comment #9)
> With this patch we are making sure all cookies will be removed on logout.
> Doing it client-side is more fragile.
> 
> This patch is a security issue (data leak) when bug 29915 is not.

Dont wanna repeat, but I think we should solve that at the start not at the end of the process.
Comment 11 Jonathan Druart 2022-01-27 13:39:44 UTC
To me bug 29915 is "cosmetic" only and shouldn't be backported.
Comment 12 Marcel de Rooy 2022-01-27 14:08:36 UTC
(In reply to Jonathan Druart from comment #11)
> To me bug 29915 is "cosmetic" only and shouldn't be backported.

Cosmetic is not the right word, but yes it is more an enhancement after 914 gets in. But reducing lots of session data.
Comment 13 Martin Renvoize 2022-01-27 16:20:22 UTC
I'm not 100% sure of this one.. whilst I agree in part, we should certainly be nuking at least some of the cookies.. I'm not sure we should nuke all of them.

The language one could be interesting.. I suppose the question is with regard to public computers vs personal computers.. on a public computer you clearly want to nuke all cookies.. but on a personal computer the user may actually want things like the language cookies to remain intact across login sessions... no?
Comment 14 Jonathan Druart 2022-01-27 16:24:16 UTC
We could skip language. It's easier to list the ones we want to keep than those to drop anyway :)
Comment 15 Martin Renvoize 2022-01-27 16:36:10 UTC
Totally agree there.. I like allow lists more than deny lists.. always fail-safe in my opinion.
Comment 16 Martin Renvoize 2022-01-27 16:38:05 UTC
We could even do the ol' 'Private computer' trick.. set a cookie at login that defines if we're on a private or public computer (defaulting to public).. if we get the 'private' checked then we set a cookie for that and that defines if we allow some cookies to remain across user sessions :)
Comment 17 David Cook 2022-01-27 22:45:16 UTC
Comment on attachment 129871 [details] [review]
Bug 29957: Clear cookies on logout

Review of attachment 129871 [details] [review]:
-----------------------------------------------------------------

::: C4/Auth.pm
@@ +788,4 @@
>      return $timeout;
>  }
>  
> +sub clear_all_cookies {

Could we name this something like "clear_all_cookies_cgi" since it has an implicit dependency on the CGI module? 

Trying to write code for Mojolicious is really challenging sometimes, because so many functions depend on CGI. In this case, the dependency is understandable since the CGI module is the backbone of the CGI controller (even when emulated and served over PSGI), but I think adding a "_cgi" suffix or "cgi_" prefix will help with future efforts to switch away from CGI...

Just my 2 cents.

@@ +791,5 @@
> +sub clear_all_cookies {
> +    my ( $query ) = shift;
> +    my @cookies;
> +    for my $cookie_name ( $query->cookie ) {
> +        push @cookies, $query->cookie( -name => $cookie_name, -value => '', -expires => '', -HttpOnly => 1 );

Do we need to consider the Secure flag? Since we're deleting cookies, I don't see harm in leaving it off, but flagging the flag anyway.
Comment 18 David Cook 2022-01-27 22:47:48 UTC
Oh another thing to consider... what about local storage? We've increasingly been using local storage. I noticed Owen was planning to replace a cookie with local storage, so we might want to be clearing that if possible...

(I know I'm just adding more headache but that's what I do.. heh)
Comment 19 Marcel de Rooy 2022-01-28 07:16:47 UTC
(In reply to Martin Renvoize from comment #16)
> We could even do the ol' 'Private computer' trick.. set a cookie at login
> that defines if we're on a private or public computer (defaulting to
> public).. if we get the 'private' checked then we set a cookie for that and
> that defines if we allow some cookies to remain across user sessions :)

Thats a point of interest.
We do not really need to worry much on the private computer front, I guess.
The term data leak is too heavy.
The public computer in the library is more interesting. On that computer you should perhaps not even store data in these (anonymous) session cookies. Two people behind that computer might see their searches etc.

Talking about searches, since it is the example mentioned already: Just open the browser history and you see all the search actions in the URL parameters. If we  really want to address that, we need a broader approach.
Comment 20 Marcel de Rooy 2022-01-28 07:20:23 UTC
(In reply to David Cook from comment #18)
> Oh another thing to consider... what about local storage? We've increasingly
> been using local storage. I noticed Owen was planning to replace a cookie
> with local storage, so we might want to be clearing that if possible...
> 
> (I know I'm just adding more headache but that's what I do.. heh)

Sure.
In view of code maintenance, I do not like to clear all cookies in Auth.
Auth is a module that already cries for refactoring.
I understand that Auth takes care of the most important cookie, the session id.
But let's not make it worse than it already is by moving all the cookie cruft inside.
Probably most cookies are quite specific and can be dealt with there.

Moving some cookies to sessionStorage might be an idea too. Like the session id?
Comment 21 Jonathan Druart 2022-01-28 08:13:03 UTC
We are moving some cookies to local storage already, but all cannot be moved there. Session ID should not be moved there. Only info that the server does not need to know should be moved.

Marcel, I really don't understand your point. I cannot think of another places where it could be efficient (based on an allow list approach). Please suggest an alternative idea then.
Comment 22 Marcel de Rooy 2022-01-28 08:18:06 UTC
(In reply to Jonathan Druart from comment #21)
> We are moving some cookies to local storage already, but all cannot be moved
> there. Session ID should not be moved there. Only info that the server does
> not need to know should be moved.
You are right for session id. Might be (quite) impossible to get that working.
Comment 23 Jonathan Druart 2022-01-28 08:22:20 UTC
(In reply to David Cook from comment #18)
> Oh another thing to consider... what about local storage? We've increasingly
> been using local storage.

This is done when the user logout (explicitly, so when they click the logout link).
Comment 24 Martin Renvoize 2022-01-31 14:06:17 UTC
I think this makes sense in Auth really.. our Auth module isn't really just about Authentication.. it's actually more of a session handler.. yes.. it does authentication, authorization and session creation and removal.. this is just cleaning up the 'session removal' part to work more thoroughly.  Whilst at some point I wouldn't mind these three distinct things being factored out into their own modules.. I do think the 'session creation/session removal' code does fit neatly together as a thing.. we don't have a 'logout' hook that gets passed around to controller or modules so clearing cookies in all these distinct places right now wouldn't be at all trivial I think.
Comment 25 Martin Renvoize 2022-01-31 14:08:43 UTC Comment hidden (obsolete)
Comment 26 Martin Renvoize 2022-01-31 14:08:48 UTC Comment hidden (obsolete)
Comment 27 Martin Renvoize 2022-01-31 14:10:10 UTC
This works as designed for me..  I'd really like to see the 'Public PC' checkbox added to the login screens sooner rather than later though as I think the change of functionality here could easily be seen as a regression by some.. but as a security piece, I think this covers the basic requirements.
Comment 28 Marcel de Rooy 2022-01-31 15:01:02 UTC
(In reply to Jonathan Druart from comment #21)
> Marcel, I really don't understand your point. I cannot think of another
> places where it could be efficient (based on an allow list approach). Please
> suggest an alternative idea then.

That takes time. I dont like to rush this. But thats just my opinion.
Comment 29 Fridolin Somers 2022-02-03 20:12:48 UTC
Hi,

I say security first, we must act ASAP.

We have released some security patches pointing to cookies so hackers may look there more.
The simplest way to secure is removing after logout.

We must indeed use more local storage.
We can always take time to propose alternatives than can be validated before being published.
Comment 30 Marcel de Rooy 2022-02-04 07:06:35 UTC
(In reply to Fridolin Somers from comment #29)
> I say security first, we must act ASAP.
> 
> We have released some security patches pointing to cookies so hackers may
> look there more.
> The simplest way to secure is removing after logout.
> 
> We must indeed use more local storage.
> We can always take time to propose alternatives than can be validated before
> being published.

The only critical thing I saw, was the password in the advanced search cookie which is an edge case. Most people will not login there. And it has been addressed. So please tell me what is your point?
Comment 31 Fridolin Somers 2022-02-08 06:37:28 UTC
(In reply to Marcel de Rooy from comment #30)
> 
> The only critical thing I saw, was the password in the advanced search
> cookie which is an edge case. Most people will not login there. And it has
> been addressed. So please tell me what is your point?

Indeed I have no proof that there is actually more security issues with cookies.

Maybe at least remove cookie with session ID ?
Comment 32 Jonathan Druart 2022-02-08 08:33:56 UTC
We don't really care about session ID if the session has been removed server-side.

But there are cookies that contain private data, like 'form_serialized' that has the last adv search info.
Comment 33 Jonathan Druart 2022-02-15 09:13:26 UTC
Comment on attachment 130006 [details] [review]
[STABLE] Bug 29957: Set path for bibs_selected

No longer needed in master since bug 29932.
Comment 34 Katrin Fischer 2022-03-07 06:43:29 UTC
I'd like to vote for keeping the language cookie. With bug 29932 taking care of the advanced search one, where does this leave us?
Comment 35 Jonathan Druart 2022-03-07 08:28:41 UTC Comment hidden (obsolete)
Comment 36 Martin Renvoize 2022-03-07 08:34:49 UTC
(In reply to Katrin Fischer from comment #34)
> I'd like to vote for keeping the language cookie. With bug 29932 taking care
> of the advanced search one, where does this leave us?

Why?

I can understand it from a 'this is a private computer' standpoint, but from a 'this is a public computer' standpoint, it feels like the language should be set at the browser level for the locality of the computer and once someone has logged out and walked away the site language should go back to the default for the locality of the machine, not the person who had just been using it.?
Comment 37 Jonathan Druart 2022-03-07 08:52:17 UTC
(In reply to Martin Renvoize from comment #36)
> (In reply to Katrin Fischer from comment #34)
> > I'd like to vote for keeping the language cookie. With bug 29932 taking care
> > of the advanced search one, where does this leave us?
> 
> Why?
> 
> I can understand it from a 'this is a private computer' standpoint, but from
> a 'this is a public computer' standpoint, it feels like the language should
> be set at the browser level for the locality of the computer and once
> someone has logged out and walked away the site language should go back to
> the default for the locality of the machine, not the person who had just
> been using it.?

Yes, same here. I don't think we should keep the language cookie.
Comment 38 Katrin Fischer 2022-03-07 08:56:31 UTC
Because I don't think the public computer is the most common use case anymore. People search on their phones and their laptops, the OPAC stations are still there, but not as important as they used to be.

But: I won't fight that. If anyone complains, I will file a new bug later.
Comment 39 Katrin Fischer 2022-03-07 20:05:43 UTC
Doesn't apply cleanly anymore:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 29957: Clear cookies on logout
Applying: Bug 29957: Set path for bibs_selected
Using index info to reconstruct a base tree...
M	koha-tmpl/intranet-tmpl/prog/js/staff-global.js
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/js/staff-global.js
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/js/staff-global.js
error: Failed to merge in the changes.
Patch failed at 0001 Bug 29957: Set path for bibs_selected
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/STABLE-Bug-29957-Set-path-for-bibsselected-R3fonE.patch
Comment 40 Martin Renvoize 2022-03-08 11:49:16 UTC
(In reply to Katrin Fischer from comment #38)
> Because I don't think the public computer is the most common use case
> anymore. People search on their phones and their laptops, the OPAC stations
> are still there, but not as important as they used to be.
> 
> But: I won't fight that. If anyone complains, I will file a new bug later.

This is the exact use case for a 'private computer' checkbox in my opinion.  Just because it's common to be using your own device doesn't mean we shouldn't account for public computers... and, if you're using your own device then in reality you will have localisation set already at the device level.. so we really don't need to keep the cookie.. we'll get the right thing from the browser environment already ;)
Comment 41 Jonathan Druart 2022-03-08 12:38:01 UTC
I obsoleted the patch.
Comment 42 Jonathan Druart 2022-03-08 12:39:14 UTC
(In reply to Katrin Fischer from comment #39)
> Doesn't apply cleanly anymore:

Second patch is for stable branches only.
Comment 43 Fridolin Somers 2022-03-10 04:40:16 UTC
(In reply to Martin Renvoize from comment #40)
> This is the exact use case for a 'private computer' checkbox in my opinion. 
> Just because it's common to be using your own device doesn't mean we
> shouldn't account for public computers... and, if you're using your own
> device then in reality you will have localisation set already at the device
> level.. so we really don't need to keep the cookie.. we'll get the right
> thing from the browser environment already ;)

OK, this would be like many software have a "Remember me on this computer" checkbox on authentication form.
Comment 44 Marcel de Rooy 2022-03-11 07:41:02 UTC
Revisiting this report now
Comment 45 Marcel de Rooy 2022-03-11 07:41:22 UTC
   FAIL   pod coverage
                POD is missing for clear_all_cookies
Comment 46 Marcel de Rooy 2022-03-11 10:58:45 UTC
With reference to above comments, I think we should start clearing cookies using an allow or deny list (coming from pref or koha-conf).

We should set the Expire date on a cookie that we want to delete.

I would recommend to remove duplicate cookies from the array and not just rely on the order of the array containing old and new entry for the same cookie name.

In order to keep the cookie 'clutter' out of Auth where possible, move it to a separate module ?
Will try to provide POC patch to help getting this further. Returning here Monday.
Comment 47 Marcel de Rooy 2022-03-15 13:35:04 UTC
(In reply to Jonathan Druart from comment #33)
> Comment on attachment 130006 [details] [review] [review]
> [STABLE] Bug 29957: Set path for bibs_selected
> 
> No longer needed in master since bug 29932.

No longer needed too for 21.11.x and 21.05.x

Moved to 30296
Comment 48 Marcel de Rooy 2022-03-15 13:37:45 UTC
In consultation with Joubu we move the dependency tree away from this report. The severity of this one can be reduced now. Lets start one level lower :) and see where the ship ends up ;)
Comment 49 Marcel de Rooy 2022-03-15 15:51:51 UTC Comment hidden (obsolete)
Comment 50 Marcel de Rooy 2022-03-15 15:51:57 UTC Comment hidden (obsolete)
Comment 51 Marcel de Rooy 2022-03-15 15:52:02 UTC Comment hidden (obsolete)
Comment 52 Marcel de Rooy 2022-03-15 15:52:42 UTC
This is just a preview. Few more patches coming.
Comment 53 Marcel de Rooy 2022-03-16 15:45:52 UTC Comment hidden (obsolete)
Comment 54 Marcel de Rooy 2022-03-16 15:45:56 UTC Comment hidden (obsolete)
Comment 55 Marcel de Rooy 2022-03-16 15:46:00 UTC Comment hidden (obsolete)
Comment 56 Marcel de Rooy 2022-03-16 15:46:04 UTC Comment hidden (obsolete)
Comment 57 Marcel de Rooy 2022-03-16 15:51:16 UTC
This can be moved out of security now.
Comment 58 Marcel de Rooy 2022-03-16 15:55:02 UTC
Jonathan or Martin, please have a look at the current stage.
What I now have, might be still possible to backport, although I still do not see a major reason to do so. We configure behavior now via <removable_cookie> lines in koha-conf.

Further development on this front should be master only. Could go to another report. The fourth commit message shows you an example of it: differentiate between HttpOnly yes or no.
What do you think?
Comment 59 Marcel de Rooy 2022-03-17 09:35:06 UTC Comment hidden (obsolete)
Comment 60 Marcel de Rooy 2022-03-17 09:35:10 UTC Comment hidden (obsolete)
Comment 61 Marcel de Rooy 2022-03-17 09:35:15 UTC Comment hidden (obsolete)
Comment 62 Marcel de Rooy 2022-03-17 09:35:19 UTC Comment hidden (obsolete)
Comment 63 Marcel de Rooy 2022-03-17 09:35:23 UTC Comment hidden (obsolete)
Comment 64 Marcel de Rooy 2022-03-17 10:10:59 UTC
(In reply to Marcel de Rooy from comment #58)
> 
> Further development on this front should be master only. Could go to another
> report. The fourth commit message shows you an example of it: differentiate
> between HttpOnly yes or no.
> What do you think?

I decided to fix the httponly issue when clearing cookies here by simply removing the flag for the temporary cleared cookie.
Comment 65 Marcel de Rooy 2022-03-17 10:12:59 UTC
The last patch deals with this funny code part in check_api_auth:

        my $cookie = $query->cookie(
            -name     => 'CGISESSID',
            -value    => $session->id,
            -HttpOnly => 1,
            -secure => ( C4::Context->https_enabled() ? 1 : 0 ),
        );
        return ( $return, undef, $session );

This is useless code. In this statement we can simply return the created $cookie instead.
Comment 66 Owen Leonard 2022-03-17 10:55:40 UTC
Does this affect Bug 26019 (besides creating understandable conflicts) ?
Comment 67 Marcel de Rooy 2022-03-17 12:33:08 UTC
(In reply to Owen Leonard from comment #66)
> Does this affect Bug 26019 (besides creating understandable conflicts) ?

Conflicts, sure. And we definitely dont like rebase trouble in Auth :) So they need some careful rebasing, especially since there is more Auth in the pipeline.
Not much more than that.
I would be tempted to move the SameSite attribute to CookieManager in the future and perhaps syspref it.
But thats just ideas.
Comment 68 Marcel de Rooy 2022-03-17 13:24:43 UTC
Created attachment 131840 [details] [review]
Bug 29957: Introduce Koha::CookieManager

Test plan:
Run t/CookieManager.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 69 Marcel de Rooy 2022-03-17 13:24:48 UTC
Created attachment 131841 [details] [review]
Bug 29957: Replace clear_all_cookies in Auth.pm

Test plan:
Enable language selection and have two languages.
Add removable_cookie line for KohaOpacLanguage in koha-conf.
Restart, flush.
Login, logout from opac or staff.
Check in browser dev console that value of this cookie is cleared,
noting that the httponly flag has been unset.
Remove the added line in koha-conf. Restart, flush.
Login. Change language.
Logout. Check if cookie is preserved.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 70 Marcel de Rooy 2022-03-17 13:24:52 UTC
Created attachment 131842 [details] [review]
Bug 29957: Adjust push @$cookie statements in Auth

We can now use $cookie_mgr->replace_in_list instead. This
effectively removes duplicates and keeps the newest cookie.

Test plan:
Run t/db_dependent/Auth.t
Add removable_cookie entries in koha-conf for:
    form_serialized, form_serialized_limits
(Note: these are js cookies, no httponly flag.)
Login at OPAC.
If you are using the same domain, check if you are logged in on the
staff client too.
Go back to OPAC, perform an Advanced search.
Check cookies on browser dev console. Look for the 2 form_ cookies.
Go to staff. Logout
Go to OPAC. Check cookies in dev console. Are you logged out?
Login at staff again. Hit some pages in the staff client and verify
that they respond as expected.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 71 Marcel de Rooy 2022-03-17 13:24:57 UTC
Created attachment 131843 [details] [review]
Bug 29957: Let check_api_auth return created cookie

The cookie created on L1496 is useless, since it is not returned.
We could either remove the cookie creation (unchanged behavior).
But since check_api_auth is expected to return a cookie when it is ok,
I opt for returning the cookie here (corrected behavior).

Test plan:
Logout in staff.
Check on staff: /cgi-bin/koha/svc/localization?id=1
You should have a 400 response.
Login with staff credentials (incl. manage_itemtypes)
Revisit same URL.
You should see a JSON response.
Check if you can hit other staff pages.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 72 Marcel de Rooy 2022-03-17 13:26:25 UTC
Just changed:

+    my $allowed = C4::Context->config(ALLOW_LIST_VAR) || []; # expecting scalar or arrayref

This prevents an empty entry in the hash later on.
Comment 73 Marcel de Rooy 2022-03-18 07:18:28 UTC
Created attachment 131856 [details] [review]
Bug 29957: Add example lines in koha-conf.xml

Test plan:
Just comments. Nothing to test.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 74 Jonathan Druart 2022-03-21 10:50:52 UTC
Those patches don't apply on top of bug 29915 and bug 28786. At this point I think they should go first.

You are deciding to list the cookie to remove whereas I think we decided the reverse, and clear them all by default. Even, as we don't want to keep the language, why should we let the possibility to keep some?

To me, cookies are tied to the session, on logout should just remove them all.
Comment 75 Marcel de Rooy 2022-03-21 11:10:17 UTC
(In reply to Jonathan Druart from comment #74)
> Those patches don't apply on top of bug 29915 and bug 28786. At this point I
> think they should go first.

I will rebase them when needed. 29915 is PQA, I would let the RM push it now.

> You are deciding to list the cookie to remove whereas I think we decided the
> reverse, and clear them all by default. Even, as we don't want to keep the
> language, why should we let the possibility to keep some?

I am not aware of a community decision here. The discussion showed me that it would be more acceptable to differentiate between cookies to keep and remove. And this patch set makes that possible via an allow list. 
The language cookie might be an excellent example to keep.
If there is concensus to use a deny list here, that is adjusted easily. Do you want me to ask on the dev ML? 

> To me, cookies are tied to the session, on logout should just remove them
> all.
Not all Koha cookies are session based. Some have a long expiry.
Logout will now clear all cookies that you want to clear. Session cookies are normally cleared when you close the browser.
Note that a considerable number of users may not logout, but closes the browser.
Comment 76 Jonathan Druart 2022-03-21 12:10:57 UTC
(In reply to Marcel de Rooy from comment #75)
> (In reply to Jonathan Druart from comment #74)
> > Those patches don't apply on top of bug 29915 and bug 28786. At this point I
> > think they should go first.
> 
> I will rebase them when needed. 29915 is PQA, I would let the RM push it now.
> 
> > You are deciding to list the cookie to remove whereas I think we decided the
> > reverse, and clear them all by default. Even, as we don't want to keep the
> > language, why should we let the possibility to keep some?
> 
> I am not aware of a community decision here. The discussion showed me that
> it would be more acceptable to differentiate between cookies to keep and
> remove. And this patch set makes that possible via an allow list. 
> The language cookie might be an excellent example to keep.
> If there is concensus to use a deny list here, that is adjusted easily. Do
> you want me to ask on the dev ML? 

Only Martin and me so far (comments #14 and #15)
The problem is: how do you maintain it? I am assuming that we want to clear a cookie by default, so how sysops will maintain this list up-to-date? They will have to know if a new cookie has been added to a given version. Seems a nightmare.

> > To me, cookies are tied to the session, on logout should just remove them
> > all.
> Not all Koha cookies are session based. Some have a long expiry.
> Logout will now clear all cookies that you want to clear. Session cookies
> are normally cleared when you close the browser.
> Note that a considerable number of users may not logout, but closes the
> browser.

I am not aware of cookies we want to keep in Koha after a logout. We proved earlier that even "language" was not a good pick.
Comment 77 Marcel de Rooy 2022-03-21 12:42:37 UTC
(In reply to Jonathan Druart from comment #76)
> Only Martin and me so far (comments #14 and #15)
> The problem is: how do you maintain it? I am assuming that we want to clear
> a cookie by default, so how sysops will maintain this list up-to-date? They
> will have to know if a new cookie has been added to a given version. Seems a
> nightmare.

It might indeed be easier to add KohaOpacLanguage as an entry on the deny list.

> I am not aware of cookies we want to keep in Koha after a logout. We proved
> earlier that even "language" was not a good pick.

I am not sure how solid that 'proof' actually is. It still seems to me that language is the best example for having differentiation. We should not assume that browser language e.g. is available in OPAC or automatically matches the user's preference. That is the main reason for having such a cookie.
Comment 78 Jonathan Druart 2022-03-21 13:41:50 UTC
(In reply to Marcel de Rooy from comment #77)
> (In reply to Jonathan Druart from comment #76)
> > I am not aware of cookies we want to keep in Koha after a logout. We proved
> > earlier that even "language" was not a good pick.
> 
> I am not sure how solid that 'proof' actually is. It still seems to me that
> language is the best example for having differentiation. We should not
> assume that browser language e.g. is available in OPAC or automatically
> matches the user's preference. That is the main reason for having such a
> cookie.

You are on a public computer in a university in UK, you switch the interface to Dutch, then logout.
Next user certainly wants the interface in English (locale of the browser), instead of Dutch.

What would be a use case for keeping the cookie?
Comment 79 Marcel de Rooy 2022-03-22 16:03:50 UTC
(In reply to Jonathan Druart from comment #78)
> You are on a public computer in a university in UK, you switch the interface
> to Dutch, then logout.
> Next user certainly wants the interface in English (locale of the browser),
> instead of Dutch.
> 
> What would be a use case for keeping the cookie?

You are on your own computer in whatever country.
You switch to a language that they do not speak, say English. Logout.
Every time you use Koha, you need to switch the language again UNLESS ...

You are on public computer in your library. Koha supports the language that you speak. Most people will not switch the language. Keeping or removing the cookie is not really relevant.

Out of scope here? But why not support passing the language too in the URL parameter of opac-main? Save as favorite, and you are done.
Comment 80 Jonathan Druart 2022-03-22 16:11:53 UTC
(In reply to Marcel de Rooy from comment #79)
> Out of scope here? But why not support passing the language too in the URL
> parameter of opac-main? Save as favorite, and you are done.

This is implemented already, you can pass &language=es-ES
Comment 81 Jonathan Druart 2022-03-22 16:12:29 UTC
(In reply to Jonathan Druart from comment #80)
> (In reply to Marcel de Rooy from comment #79)
> > Out of scope here? But why not support passing the language too in the URL
> > parameter of opac-main? Save as favorite, and you are done.
> 
> This is implemented already, you can pass &language=es-ES

Sorry, misread. Ignore that.
Comment 82 Jonathan Druart 2022-03-22 16:13:56 UTC
(In reply to Jonathan Druart from comment #81)
> (In reply to Jonathan Druart from comment #80)
> > (In reply to Marcel de Rooy from comment #79)
> > > Out of scope here? But why not support passing the language too in the URL
> > > parameter of opac-main? Save as favorite, and you are done.
> > 
> > This is implemented already, you can pass &language=es-ES
> 
> Sorry, misread. Ignore that.

Or maybe not, well, it's implemented if it's what you need.
Comment 83 Marcel de Rooy 2022-03-23 07:17:41 UTC
Rebasing, adjusting
Comment 84 Marcel de Rooy 2022-03-23 07:22:58 UTC
Created attachment 132045 [details] [review]
Bug 29957: Clear cookies on logout

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Reverted the change to $cookies to minimize changes.
Fixed Auth.t where checkauth is being mocked.
Comment 85 Marcel de Rooy 2022-03-23 08:12:33 UTC
Created attachment 132046 [details] [review]
Bug 29957: Introduce Koha::CookieManager

Test plan:
Run t/CookieManager.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 86 Marcel de Rooy 2022-03-23 08:12:38 UTC
Created attachment 132047 [details] [review]
Bug 29957: (follow-up) Turn allow list into deny list

See the discussion on the Bugzilla report.
It might be easier to work with a deny list.

Test plan:
Run t/CookieManager.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 87 Marcel de Rooy 2022-03-23 08:12:55 UTC
Not ready yet
Comment 88 Marcel de Rooy 2022-03-24 09:26:14 UTC
Created attachment 132105 [details] [review]
Bug 29957: Clear cookies on logout

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Reverted the change to $cookies to minimize changes.
Fixed Auth.t where checkauth is being mocked.
Comment 89 Marcel de Rooy 2022-03-24 09:26:20 UTC
Created attachment 132106 [details] [review]
Bug 29957: Introduce Koha::CookieManager

Test plan:
Run t/CookieManager.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 90 Marcel de Rooy 2022-03-24 09:26:25 UTC
Created attachment 132107 [details] [review]
Bug 29957: (follow-up) Turn allow list into deny list

See the discussion on the Bugzilla report.
It might be easier to work with a deny list.

Test plan:
Run t/CookieManager.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 91 Marcel de Rooy 2022-03-24 09:26:29 UTC
Created attachment 132108 [details] [review]
Bug 29957: Replace clear_all_cookies in Auth.pm

Test plan:
Enable language selection and have two languages.
Change language.
Check cookie value in browser for KohaOpacLanguage.
Logout.
Verify that cookie has been cleared in browser.

Add do_not_remove_cookie line for KohaOpacLanguage in koha-conf.
Restart, flush.
Login again.
Change language.
Check cookie value in browser for KohaOpacLanguage.
Logout.
Verify that cookie still contains the language.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 92 Marcel de Rooy 2022-03-24 09:26:36 UTC
Created attachment 132109 [details] [review]
Bug 29957: Adjust push @$cookie statements in Auth

We can now use $cookie_mgr->replace_in_list instead. This
effectively removes duplicates and keeps the newest cookie.

Note: In the test plan below we are just verifying if
this patch did not change behavior. The replace_in_list
routine has been tested already in a unit test.

Test plan:
Run t/db_dependent/Auth.t
Login at OPAC.
Hit some opac and staff pages.
Perform an Advanced search on OPAC.
Check cookies in browser.
Logout.
Check cookies again. Verify with your do_not_remove_cookie lines
in koha-conf.xml.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 93 Marcel de Rooy 2022-03-24 09:26:41 UTC
Created attachment 132110 [details] [review]
Bug 29957: Let check_api_auth return created cookie

The cookie created on L1496 is useless, since it is not returned.
We could either remove the cookie creation (unchanged behavior).
But since check_api_auth is expected to return a cookie when it is ok,
I opt for returning the cookie here (corrected behavior).

Test plan:
Logout in staff.
Check on staff: /cgi-bin/koha/svc/localization?id=1
You should have a 400 response.
Login with staff credentials (incl. manage_itemtypes)
Revisit same URL.
You should see a JSON response.
Check if you can hit other staff pages.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 94 Marcel de Rooy 2022-03-24 09:26:47 UTC
Created attachment 132111 [details] [review]
Bug 29957: Add example lines in koha-conf.xml

Test plan:
Just comments. Nothing to test.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: using new name for deny list.
Comment 95 Marcel de Rooy 2022-03-24 09:27:32 UTC
Tested now on top of last additional fix on bug 29915
Comment 96 Marcel de Rooy 2022-03-24 09:33:27 UTC
(In reply to Jonathan Druart from comment #82)
> (In reply to Jonathan Druart from comment #81)
> > (In reply to Jonathan Druart from comment #80)
> > > (In reply to Marcel de Rooy from comment #79)
> > > > Out of scope here? But why not support passing the language too in the URL
> > > > parameter of opac-main? Save as favorite, and you are done.
> > > 
> > > This is implemented already, you can pass &language=es-ES
> > 
> > Sorry, misread. Ignore that.
> 
> Or maybe not, well, it's implemented if it's what you need.

I now chose to add KohaOpacLanguage as a default exception to clearing at logout. But it will only apply to new installs.
Would it be useful in the release notes text too as a workaround?
Comment 97 Marcel de Rooy 2022-03-24 10:16:48 UTC
Created attachment 132115 [details] [review]
Bug 29957: Add example lines in koha-conf.xml

Test plan:
Just comments. Nothing to test.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: using new name for deny list.
Comment 98 Marcel de Rooy 2022-03-24 10:19:00 UTC
(In reply to Marcel de Rooy from comment #96)
> (In reply to Jonathan Druart from comment #82)
> > (In reply to Jonathan Druart from comment #81)
> > > (In reply to Jonathan Druart from comment #80)
> > > > (In reply to Marcel de Rooy from comment #79)
> > > > > Out of scope here? But why not support passing the language too in the URL
> > > > > parameter of opac-main? Save as favorite, and you are done.
> > > > 
> > > > This is implemented already, you can pass &language=es-ES
> > > 
> > > Sorry, misread. Ignore that.
> > 
> > Or maybe not, well, it's implemented if it's what you need.
> 
> I now chose to add KohaOpacLanguage as a default exception to clearing at
> logout. But it will only apply to new installs.
> Would it be useful in the release notes text too as a workaround?

Flexibility. Things have already changed.
Comment 99 Jonathan Druart 2022-03-24 14:01:54 UTC
New $KOHA_CONF entry needs to be added to debian/scripts/koha-create
Comment 100 Marcel de Rooy 2022-03-24 14:05:04 UTC
(In reply to Jonathan Druart from comment #99)
> New $KOHA_CONF entry needs to be added to debian/scripts/koha-create

Not sure what you mean.
Last patch adjusts the debian koha-conf ?
Comment 101 Marcel de Rooy 2022-03-25 07:10:01 UTC
(In reply to Marcel de Rooy from comment #100)
> (In reply to Jonathan Druart from comment #99)
> > New $KOHA_CONF entry needs to be added to debian/scripts/koha-create
> 
> Not sure what you mean.
> Last patch adjusts the debian koha-conf ?

Clarified on IRC: Add a do_not_remove_cookie line via a command line option of koha-create.
Note that this is not supported for several other config lines.
But I will have a look.
Comment 102 Marcel de Rooy 2022-03-25 08:37:49 UTC Comment hidden (obsolete)
Comment 103 Marcel de Rooy 2022-03-25 08:44:41 UTC
Created attachment 132211 [details] [review]
Bug 29957: Add --keep-cookie option to koha-create

As requested on comment99. Personally I do not feel that it
should be a requirement for adding a koha-conf entry though.
Note that koha-create does/did not even support adding multiple
line entries to koha-conf like we do here now.
But it might be a start for adding them?

Test plan:
The following tweak might help you to test without running the
complete koha-create, while still convincing you:

[1] cp debian/templates/koha-conf-site.xml.in /etc/koha/test.conf
[2] Add the following line in debian/scripts/koha-create. Append
    if after the BCRYPT_SETTINGS= line on L652:
    generate_config_file test.conf test.xml; exit
[3] Run debian/scripts/koha-create --keep-cookie A
[4] Check if you see <do_not_remove_cookie>A<..> in test.xml.
[5] Run debian/scripts/koha-create --keep-cookie A --keep-cookie B
[6] Check for two lines in test.xml
[7] Run debian/scripts/koha-create clone_name
[8] Check that the __KEEP_COOKIE__ line is gone in test.xml.
[9] Undo the tweak by removing test.xml and /etc/koha/test.conf,
    and removing the added line from koha-create.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 104 Nick Clemens 2022-03-25 12:21:42 UTC
Created attachment 132231 [details] [review]
Bug 29957: Clear cookies on logout

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Reverted the change to $cookies to minimize changes.
Fixed Auth.t where checkauth is being mocked.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 105 Nick Clemens 2022-03-25 12:21:46 UTC
Created attachment 132232 [details] [review]
Bug 29957: Introduce Koha::CookieManager

Test plan:
Run t/CookieManager.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 106 Nick Clemens 2022-03-25 12:21:50 UTC
Created attachment 132233 [details] [review]
Bug 29957: (follow-up) Turn allow list into deny list

See the discussion on the Bugzilla report.
It might be easier to work with a deny list.

Test plan:
Run t/CookieManager.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 107 Nick Clemens 2022-03-25 12:21:55 UTC
Created attachment 132234 [details] [review]
Bug 29957: Replace clear_all_cookies in Auth.pm

Test plan:
Enable language selection and have two languages.
Change language.
Check cookie value in browser for KohaOpacLanguage.
Logout.
Verify that cookie has been cleared in browser.

Add do_not_remove_cookie line for KohaOpacLanguage in koha-conf.
Restart, flush.
Login again.
Change language.
Check cookie value in browser for KohaOpacLanguage.
Logout.
Verify that cookie still contains the language.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 108 Nick Clemens 2022-03-25 12:21:59 UTC
Created attachment 132235 [details] [review]
Bug 29957: Adjust push @$cookie statements in Auth

We can now use $cookie_mgr->replace_in_list instead. This
effectively removes duplicates and keeps the newest cookie.

Note: In the test plan below we are just verifying if
this patch did not change behavior. The replace_in_list
routine has been tested already in a unit test.

Test plan:
Run t/db_dependent/Auth.t
Login at OPAC.
Hit some opac and staff pages.
Perform an Advanced search on OPAC.
Check cookies in browser.
Logout.
Check cookies again. Verify with your do_not_remove_cookie lines
in koha-conf.xml.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 109 Nick Clemens 2022-03-25 12:22:03 UTC
Created attachment 132236 [details] [review]
Bug 29957: Let check_api_auth return created cookie

The cookie created on L1496 is useless, since it is not returned.
We could either remove the cookie creation (unchanged behavior).
But since check_api_auth is expected to return a cookie when it is ok,
I opt for returning the cookie here (corrected behavior).

Test plan:
Logout in staff.
Check on staff: /cgi-bin/koha/svc/localization?id=1
You should have a 400 response.
Login with staff credentials (incl. manage_itemtypes)
Revisit same URL.
You should see a JSON response.
Check if you can hit other staff pages.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 110 Nick Clemens 2022-03-25 12:22:07 UTC
Created attachment 132237 [details] [review]
Bug 29957: Add example lines in koha-conf.xml

Test plan:
Just comments. Nothing to test.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: using new name for deny list.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 111 Nick Clemens 2022-03-25 12:22:11 UTC
Created attachment 132238 [details] [review]
Bug 29957: Add --keep-cookie option to koha-create

As requested on comment99. Personally I do not feel that it
should be a requirement for adding a koha-conf entry though.
Note that koha-create does/did not even support adding multiple
line entries to koha-conf like we do here now.
But it might be a start for adding them?

Test plan:
The following tweak might help you to test without running the
complete koha-create, while still convincing you:

[1] cp debian/templates/koha-conf-site.xml.in /etc/koha/test.conf
[2] Add the following line in debian/scripts/koha-create. Append
    if after the BCRYPT_SETTINGS= line on L652:
    generate_config_file test.conf test.xml; exit
[3] Run debian/scripts/koha-create --keep-cookie A
[4] Check if you see <do_not_remove_cookie>A<..> in test.xml.
[5] Run debian/scripts/koha-create --keep-cookie A --keep-cookie B
[6] Check for two lines in test.xml
[7] Run debian/scripts/koha-create clone_name
[8] Check that the __KEEP_COOKIE__ line is gone in test.xml.
[9] Undo the tweak by removing test.xml and /etc/koha/test.conf,
    and removing the added line from koha-create.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 112 Nick Clemens 2022-03-25 12:22:15 UTC
Created attachment 132239 [details] [review]
Bug 29957: (follow-up) correct spelling QA complaint

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 113 Marcel de Rooy 2022-03-28 06:34:46 UTC
(In reply to Nick Clemens from comment #112)

Thanks Nick for SO.
Comment 114 Martin Renvoize 2022-03-28 07:53:43 UTC
Created attachment 132288 [details] [review]
Bug 29957: Clear cookies on logout

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Reverted the change to $cookies to minimize changes.
Fixed Auth.t where checkauth is being mocked.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 115 Martin Renvoize 2022-03-28 07:53:47 UTC
Created attachment 132289 [details] [review]
Bug 29957: Introduce Koha::CookieManager

Test plan:
Run t/CookieManager.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 116 Martin Renvoize 2022-03-28 07:53:52 UTC
Created attachment 132290 [details] [review]
Bug 29957: (follow-up) Turn allow list into deny list

See the discussion on the Bugzilla report.
It might be easier to work with a deny list.

Test plan:
Run t/CookieManager.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 117 Martin Renvoize 2022-03-28 07:53:56 UTC
Created attachment 132291 [details] [review]
Bug 29957: Replace clear_all_cookies in Auth.pm

Test plan:
Enable language selection and have two languages.
Change language.
Check cookie value in browser for KohaOpacLanguage.
Logout.
Verify that cookie has been cleared in browser.

Add do_not_remove_cookie line for KohaOpacLanguage in koha-conf.
Restart, flush.
Login again.
Change language.
Check cookie value in browser for KohaOpacLanguage.
Logout.
Verify that cookie still contains the language.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 118 Martin Renvoize 2022-03-28 07:54:01 UTC
Created attachment 132292 [details] [review]
Bug 29957: Adjust push @$cookie statements in Auth

We can now use $cookie_mgr->replace_in_list instead. This
effectively removes duplicates and keeps the newest cookie.

Note: In the test plan below we are just verifying if
this patch did not change behavior. The replace_in_list
routine has been tested already in a unit test.

Test plan:
Run t/db_dependent/Auth.t
Login at OPAC.
Hit some opac and staff pages.
Perform an Advanced search on OPAC.
Check cookies in browser.
Logout.
Check cookies again. Verify with your do_not_remove_cookie lines
in koha-conf.xml.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 119 Martin Renvoize 2022-03-28 07:54:06 UTC
Created attachment 132293 [details] [review]
Bug 29957: Let check_api_auth return created cookie

The cookie created on L1496 is useless, since it is not returned.
We could either remove the cookie creation (unchanged behavior).
But since check_api_auth is expected to return a cookie when it is ok,
I opt for returning the cookie here (corrected behavior).

Test plan:
Logout in staff.
Check on staff: /cgi-bin/koha/svc/localization?id=1
You should have a 400 response.
Login with staff credentials (incl. manage_itemtypes)
Revisit same URL.
You should see a JSON response.
Check if you can hit other staff pages.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 120 Martin Renvoize 2022-03-28 07:54:11 UTC
Created attachment 132294 [details] [review]
Bug 29957: Add example lines in koha-conf.xml

Test plan:
Just comments. Nothing to test.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: using new name for deny list.

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 121 Martin Renvoize 2022-03-28 07:54:16 UTC
Created attachment 132295 [details] [review]
Bug 29957: Add --keep-cookie option to koha-create

As requested on comment99. Personally I do not feel that it
should be a requirement for adding a koha-conf entry though.
Note that koha-create does/did not even support adding multiple
line entries to koha-conf like we do here now.
But it might be a start for adding them?

Test plan:
The following tweak might help you to test without running the
complete koha-create, while still convincing you:

[1] cp debian/templates/koha-conf-site.xml.in /etc/koha/test.conf
[2] Add the following line in debian/scripts/koha-create. Append
    if after the BCRYPT_SETTINGS= line on L652:
    generate_config_file test.conf test.xml; exit
[3] Run debian/scripts/koha-create --keep-cookie A
[4] Check if you see <do_not_remove_cookie>A<..> in test.xml.
[5] Run debian/scripts/koha-create --keep-cookie A --keep-cookie B
[6] Check for two lines in test.xml
[7] Run debian/scripts/koha-create clone_name
[8] Check that the __KEEP_COOKIE__ line is gone in test.xml.
[9] Undo the tweak by removing test.xml and /etc/koha/test.conf,
    and removing the added line from koha-create.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 122 Martin Renvoize 2022-03-28 07:54:20 UTC
Created attachment 132296 [details] [review]
Bug 29957: (follow-up) correct spelling QA complaint

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 123 Martin Renvoize 2022-03-28 07:57:59 UTC
First QA of HackFest 22!

Whilst I'm still not sure about the need for such a list in config, it is great to see some of this stuff factored out into it's own module and it gives us a great foundation going forward. I'd love to see a 'Private computer' checkbox on login screens next and have a list of cookies to keep should that checkbox be ticked at login vs a completely public computer where I still think we should remove all by default.

So, given that statement, this is looking good. Tests are passing, QA scripts are happy and I can't find any regressions..

PQA, great teamwork.
Comment 124 Fridolin Somers 2022-04-12 09:41:37 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 125 Kyle M Hall 2022-04-15 11:16:17 UTC
Pushed to 21.11.x for 21.11.05
Comment 126 Andrew Fuerste-Henry 2022-04-25 19:28:44 UTC
I can't get this to apply to 21.05 for backport. Please rebase. Some of this code has been touched by bug 29915, which I was also unable to apply and backport.
Comment 127 Victor Grousset/tuxayo 2022-04-25 21:13:02 UTC
(In reply to Andrew Fuerste-Henry from comment #126)
> I can't get this to apply to 21.05 for backport. Please rebase. Some of this
> code has been touched by bug 29915, which I was also unable to apply and
> backport.

No need, it's actually the cause for bug 30525 and likely bug 30532. So backporting this isn't necessary to fix them. On the contrary, let's dodge them.