Created attachment 129644 [details] [review] Bug 29915: Don't generate a new session ID for anonymous navigation When a user is not logged in, a new session ID is generated every time a new page is hit.
Setting to NSO to get feedback but that will need to be strongly tested and reviewed.
NOTE: This depends on something in discussion still !
@@ -1706,6 +1703,8 @@ sub check_cookie_auth { if ($flags) { return ( "ok", $session ); } + } else { + return ( "anon", $session ); This change should go on its own ?
Created attachment 129653 [details] [review] Bug 29915: Don't generate a new session ID for anonymous navigation When a user is not logged in, a new session ID is generated every time a new page is hit.
Created attachment 129683 [details] [review] Bug 29915: Add tests
Busy here
Well this is quite a time consumer. If we are calling check_cookie_auth while our cookie expired or is no longer found, jt creates a new cookie thru get_session but at the same time returns an error code. So here we are duplicating already. And somehow the search history stuff still uses the former session ID and might create yet another one. This still needs more attention !
Created attachment 129748 [details] [review] Bug 29915: Don't generate a new session ID for anonymous navigation When a user is not logged in, a new session ID is generated every time a new page is hit.
Created attachment 129750 [details] [review] Bug 29915: Don't generate a new session ID for anonymous navigation When a user is not logged in, a new session ID is generated every time a new page is hit.
Created attachment 129751 [details] [review] Bug 29915: Changes for get_session and check_cookie_auth If we look for an existing session, do not create a new one.
Created attachment 129752 [details] [review] Bug 29915: Add tests Rebased 25-01-22 on changes for 29914.
Created attachment 129753 [details] [review] Bug 29915: Don't generate a new session ID for anonymous navigation When a user is not logged in, a new session ID is generated every time a new page is hit.
Created attachment 129754 [details] [review] Bug 29915: Changes for get_session and check_cookie_auth If we look for an existing session, do not create a new one.
Created attachment 129755 [details] [review] Bug 29915: Tiny session adjustments It may be that we need a few additional flushes. And checking the returned session before clearing busc.
Going out of scope, but this part of Auth is imo arguable: # Save anonymous search history in new session so it can be retrieved # by get_template_and_user to store it in user's search history after # a successful login. if ($anon_search_history) { $session->param( 'search_history', $anon_search_history ); } Why should we save history from an anonymous session into the session of a user? We are not sure if we have the same person in front of us.
Created attachment 129756 [details] [review] Bug 29915: (QA follow-up) Fix POD typo
Before finishing up here, I would like some feedback. We can still add a few tests, certainly :) An important change is made in get_session which really seems to bring down the number of sessions. Tested some opac, staff parts. Ran several tests. But it needs a bit more testing !
Why did you remove the _unset_userenv calls?
Created attachment 129822 [details] [review] Bug 29915: Add tests
Some of those tests are failing, when I think they should pass. I am trying to test that a logged in user access an unauthorized page, then reuse the cookie to access an authorized page. The code is calling The weird thing is that I don't recreate the problem using the interface: Patron has catalogue permission only, login, hit circulation.pl => you see the login form hit mainpage => you are still logged in
BTW why do we call haspermission in checkauth? It's called in check_cookie_auth already.
(In reply to Jonathan Druart from comment #19) > Why did you remove the _unset_userenv calls? Moved them. And removed the pattern create..remove directly after each other.
(In reply to Jonathan Druart from comment #21) > Some of those tests are failing, when I think they should pass. > > I am trying to test that a logged in user access an unauthorized page, then > reuse the cookie to access an authorized page. > > The code is calling > > The weird thing is that I don't recreate the problem using the interface: > Patron has catalogue permission only, login, hit circulation.pl => you see > the login form > hit mainpage => you are still logged in Having a look
(In reply to Marcel de Rooy from comment #23) > (In reply to Jonathan Druart from comment #19) > > Why did you remove the _unset_userenv calls? > > Moved them. And removed the pattern create..remove directly after each other. Are you sure? Where is _unset_userenv called for expired, restricted and failed? Do you mean we don't need to call _unset because we now do not longer call _new?
I am seeing things like ( $auth_status, $session) = C4::Auth::check_cookie_auth($cgi, 0, {catalogue => 1}); But you should not pass $cgi to this sub: sub check_cookie_auth { my $sessionID = shift; my $flagsrequired = shift; my $params = shift; You're mixing up both subroutines. Will fix, no worries.
(In reply to Jonathan Druart from comment #25) > (In reply to Marcel de Rooy from comment #23) > > (In reply to Jonathan Druart from comment #19) > > > Why did you remove the _unset_userenv calls? > > > > Moved them. And removed the pattern create..remove directly after each other. > > Are you sure? > Where is _unset_userenv called for expired, restricted and failed? > > Do you mean we don't need to call _unset because we now do not longer call > _new? Maybe I overlooked something? But normally when I do not create A I should not need to delete it either :)
Created attachment 129826 [details] [review] Bug 29915: Don't generate a new session ID for anonymous navigation When a user is not logged in, a new session ID is generated every time a new page is hit. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129827 [details] [review] Bug 29915: Changes for get_session and check_cookie_auth If we look for an existing session, do not create a new one. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129828 [details] [review] Bug 29915: Tiny session adjustments It may be that we need a few additional flushes. And checking the returned session before clearing busc. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129829 [details] [review] Bug 29915: (QA follow-up) Fix POD typo Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129830 [details] [review] Bug 29915: Add tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129831 [details] [review] Bug 29915: (experimental) Add skip_auth_template param in checkauth The skip_auth_template param allows you to skip printing the http cruft when running tests for C4::Auth::checkauth. It will not be used in regular calls. And does not affect them. Test plan: See next patch. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129832 [details] [review] Bug 29915: Changes to Auth.t Test plan: Run Auth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #33) > Created attachment 129831 [details] [review] [review] > Bug 29915: (experimental) Add skip_auth_template param in checkauth Marked it as experimental, but really unharmful.
Would not be bad to have more than one QAer to take a look.
If insisted upon, we could squash the last three patches.
Created attachment 129833 [details] [review] Bug 29915: (QA follow-up) Add a comment in checkauth on $flags This is quite a misleading call. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #27) > (In reply to Jonathan Druart from comment #25) > > (In reply to Marcel de Rooy from comment #23) > > > (In reply to Jonathan Druart from comment #19) > > > > Why did you remove the _unset_userenv calls? > > > > > > Moved them. And removed the pattern create..remove directly after each other. > > > > Are you sure? > > Where is _unset_userenv called for expired, restricted and failed? > > > > Do you mean we don't need to call _unset because we now do not longer call > > _new? > > Maybe I overlooked something? > But normally when I do not create A I should not need to delete it either :) Hmm. There might be a theoretical side-effect still. If there was some activeuser session already, we do not overwrite/delete it now in some cases? Will come back here tomorrow.
(In reply to Marcel de Rooy from comment #39) > (In reply to Marcel de Rooy from comment #27) > > (In reply to Jonathan Druart from comment #25) > > > (In reply to Marcel de Rooy from comment #23) > > > > (In reply to Jonathan Druart from comment #19) > > > > > Why did you remove the _unset_userenv calls? > > > > > > > > Moved them. And removed the pattern create..remove directly after each other. > > > > > > Are you sure? > > > Where is _unset_userenv called for expired, restricted and failed? > > > > > > Do you mean we don't need to call _unset because we now do not longer call > > > _new? > > > > Maybe I overlooked something? > > But normally when I do not create A I should not need to delete it either :) > > Hmm. There might be a theoretical side-effect still. If there was some > activeuser session already, we do not overwrite/delete it now in some cases? > Will come back here tomorrow. This should be sufficient: C4::Context->_unset_userenv($sessionID); # remove old userenv first my $session = get_session($sessionID); Will add some testing here.
Still struggling
It doesnt stop: =head2 _unset_userenv C4::Context->_unset_userenv; Destroys the hash for activeuser user environment variables. =cut sub _unset_userenv { my ($sessionID)= @_; THis is unbelievable. But _unset_userenv does NOT have a self or class parameter. WHile new_userenv DOES: =head2 _new_userenv C4::Context->_new_userenv($session); # FIXME: This calling style is wrong for what looks like an _internal function =cut sub _new_userenv { shift; # Useless except it compensates for bad calling style my ($sessionID)= @_;
I will workaround this here and open a new report for it elsewhere.
Bug 29954
So, actually this call did not work for years: - C4::Context->_unset_userenv($sessionID); The routine tried to unset 'C4::Context' LOL
Created attachment 129860 [details] [review] Bug 29915: Don't generate a new session ID for anonymous navigation When a user is not logged in, a new session ID is generated every time a new page is hit. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129861 [details] [review] Bug 29915: Changes for get_session and check_cookie_auth If we look for an existing session, do not create a new one. Found a bug in the unset_userenv calls. For this moment changing the calls in Auth here. Later fix goes to bug 29954. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129862 [details] [review] Bug 29915: Tiny session adjustments It may be that we need a few additional flushes. And checking the returned session before clearing busc. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129863 [details] [review] Bug 29915: (QA follow-up) Fix POD typo Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129864 [details] [review] Bug 29915: Add tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129865 [details] [review] Bug 29915: Changes to Auth.t Test plan: Run Auth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129866 [details] [review] Bug 29915: (QA follow-up) Add a comment in checkauth on $flags This is quite a misleading call. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Removed the skip_auth parameter and added a few STDOUT redirections. Following warns will be removed on 29954: ok 12 - HTTP_COOKIE not unset Use of uninitialized value in string eq at /usr/share/koha/C4/Context.pm line 784. ok 21 Use of uninitialized value in string eq at /usr/share/koha/C4/Context.pm line 784. ok 23 Use of uninitialized value in string eq at /usr/share/koha/C4/Context.pm line 784.
Created attachment 129878 [details] [review] Bug 29915: Add tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129879 [details] [review] Bug 29915: Changes to Auth.t Test plan: Run Auth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 129880 [details] [review] Bug 29915: (QA follow-up) Add a comment in checkauth on $flags This is quite a misleading call. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Last patches do *not* modify the patchset. Only mistakes from "Add tests" are fixed.
Created attachment 129882 [details] [review] Bug 29915: Add a note for tests
Created attachment 129883 [details] [review] Bug 29915: Changes for get_session and check_cookie_auth If we look for an existing session, do not create a new one. Found a bug in the unset_userenv calls. For this moment changing the calls in Auth here. Later fix goes to bug 29954. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 129884 [details] [review] Bug 29915: Tiny session adjustments It may be that we need a few additional flushes. And checking the returned session before clearing busc. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 129885 [details] [review] Bug 29915: (QA follow-up) Fix POD typo Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 129886 [details] [review] Bug 29915: Add tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 129887 [details] [review] Bug 29915: Changes to Auth.t Test plan: Run Auth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 129888 [details] [review] Bug 29915: (QA follow-up) Add a comment in checkauth on $flags This is quite a misleading call. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 129889 [details] [review] Bug 29915: Add a note for tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 129906 [details] [review] Bug 29915: Don't generate a new session ID for anonymous navigation When a user is not logged in, a new session ID is generated every time a new page is hit. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 129907 [details] [review] Bug 29915: Changes for get_session and check_cookie_auth If we look for an existing session, do not create a new one. Found a bug in the unset_userenv calls. For this moment changing the calls in Auth here. Later fix goes to bug 29954. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 129908 [details] [review] Bug 29915: Tiny session adjustments It may be that we need a few additional flushes. And checking the returned session before clearing busc. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 129909 [details] [review] Bug 29915: (QA follow-up) Fix POD typo Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 129910 [details] [review] Bug 29915: Add tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 129911 [details] [review] Bug 29915: Changes to Auth.t Test plan: Run Auth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 129912 [details] [review] Bug 29915: (QA follow-up) Add a comment in checkauth on $flags This is quite a misleading call. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 129913 [details] [review] Bug 29915: Add a note for tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to Nick Clemens from comment #73) > Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Thanks, Nick
+ # NOTE: It is not what the UI is doing. + # From the UI we are allowed to hit an unauthorized page then reuse the session to hit back authorized area. + # It is because check_cookie_auth is ALWAYS called from checkauth WITHOUT $flagsrequired + # It then return "ok", when the previous called got "failed" This note is not true
(In reply to Marcel de Rooy from comment #75) > + # NOTE: It is not what the UI is doing. > + # From the UI we are allowed to hit an unauthorized page then reuse > the session to hit back authorized area. > + # It is because check_cookie_auth is ALWAYS called from checkauth > WITHOUT $flagsrequired > + # It then return "ok", when the previous called got "failed" > > This note is not true This note is true! Not on master without 29914/5 but with these patches it is. So we solved a regression on the way. At least on staff side. I am only wondering how I should trigger the unauthorised page on opac and get to these lines: elsif ( $type eq "opac" ) { # anonymous sessions are created only for the OPAC # setting a couple of other session vars... $session->param( 'ip', $session->remote_addr() ); $session->param( 'lasttime', time() ); $session->param( 'sessiontype', 'anon' ); $session->param( 'interface', $type); This might have the effect of turning an logged in opac session into an anonymous one. But I cant find an example to reproduce on the interface. So might be no problem at all.
Note to RM: This could (imo) be handled as an enhancement after 29914 has been dealt with completely.
Created attachment 129951 [details] [review] Bug 29915: Add selenium tests
(In reply to Jonathan Druart from comment #78) > Created attachment 129951 [details] [review] [review] > Bug 29915: Add selenium tests Marcel, I think those tests are covering the different use cases we hit here. However I am struggling with the following FIXME: 251 # FIXME This new get should not be needed, but the cookie is not modified right after logout 252 # However it's not the behavour when testing the UI 253 $driver->get($mainpage); I don't understand why this new get is needed to have a new cookie. If you find some time to investigate, that would be useful!
(In reply to Jonathan Druart from comment #79) > (In reply to Jonathan Druart from comment #78) > > Created attachment 129951 [details] [review] [review] [review] > > Bug 29915: Add selenium tests > > Marcel, I think those tests are covering the different use cases we hit here. > However I am struggling with the following FIXME: > > 251 # FIXME This new get should not be needed, but the cookie is > not modified right after logout > 252 # However it's not the behavour when testing the UI > 253 $driver->get($mainpage); > > I don't understand why this new get is needed to have a new cookie. If you > find some time to investigate, that would be useful! Will have a look. Could we move it to a separate report ?
Hmm. It now even depends on 29957. I am setting the status back then. It needs another QA session after 29957.
(In reply to Jonathan Druart from comment #79) > (In reply to Jonathan Druart from comment #78) > > Created attachment 129951 [details] [review] [review] [review] > > Bug 29915: Add selenium tests > > Marcel, I think those tests are covering the different use cases we hit here. > However I am struggling with the following FIXME: > > 251 # FIXME This new get should not be needed, but the cookie is > not modified right after logout > 252 # However it's not the behavour when testing the UI > 253 $driver->get($mainpage); > > I don't understand why this new get is needed to have a new cookie. If you > find some time to investigate, that would be useful! I actually wanted to do that today, but 29957 is now standing in the way ;)
Created attachment 130596 [details] [review] Bug 29915: Don't generate a new session ID for anonymous navigation When a user is not logged in, a new session ID is generated every time a new page is hit. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 130597 [details] [review] Bug 29915: Changes for get_session and check_cookie_auth If we look for an existing session, do not create a new one. Found a bug in the unset_userenv calls. For this moment changing the calls in Auth here. Later fix goes to bug 29954. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 130598 [details] [review] Bug 29915: Tiny session adjustments It may be that we need a few additional flushes. And checking the returned session before clearing busc. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 130599 [details] [review] Bug 29915: (QA follow-up) Fix POD typo Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 130600 [details] [review] Bug 29915: Add tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 130601 [details] [review] Bug 29915: Changes to Auth.t Test plan: Run Auth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 130602 [details] [review] Bug 29915: (QA follow-up) Add a comment in checkauth on $flags This is quite a misleading call. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 130603 [details] [review] Bug 29915: Add a note for tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 130604 [details] [review] Bug 29915: Add selenium tests
Those patches are actually fixing bug 30083. So the original problem we reported here is a regression. 21.05.00 is not impacted 21.05.x neither (b71eebaf6ae) So it's certainly caused by bug 28785. For master we should push this, but what do we do for 21.11?
Removing dependency on bug 29957.
Created attachment 131708 [details] [review] Bug 29915: Don't generate a new session ID for anonymous navigation When a user is not logged in, a new session ID is generated every time a new page is hit. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 131709 [details] [review] Bug 29915: Changes for get_session and check_cookie_auth If we look for an existing session, do not create a new one. Found a bug in the unset_userenv calls. For this moment changing the calls in Auth here. Later fix goes to bug 29954. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 131710 [details] [review] Bug 29915: Tiny session adjustments It may be that we need a few additional flushes. And checking the returned session before clearing busc. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 131711 [details] [review] Bug 29915: (QA follow-up) Fix POD typo Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 131712 [details] [review] Bug 29915: Add tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 131713 [details] [review] Bug 29915: Changes to Auth.t Test plan: Run Auth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 131714 [details] [review] Bug 29915: (QA follow-up) Add a comment in checkauth on $flags This is quite a misleading call. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 131715 [details] [review] Bug 29915: Add a note for tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 131716 [details] [review] Bug 29915: Add selenium tests
Created attachment 131788 [details] [review] Bug 29915: Don't generate a new session ID for anonymous navigation When a user is not logged in, a new session ID is generated every time a new page is hit. 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>
Created attachment 131789 [details] [review] Bug 29915: Changes for get_session and check_cookie_auth If we look for an existing session, do not create a new one. Found a bug in the unset_userenv calls. For this moment changing the calls in Auth here. Later fix goes to bug 29954. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131790 [details] [review] Bug 29915: Tiny session adjustments It may be that we need a few additional flushes. And checking the returned session before clearing busc. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131791 [details] [review] Bug 29915: (QA follow-up) Fix POD typo Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131792 [details] [review] Bug 29915: Add tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131793 [details] [review] Bug 29915: Changes to Auth.t Test plan: Run Auth.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131794 [details] [review] Bug 29915: (QA follow-up) Add a comment in checkauth on $flags This is quite a misleading call. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131795 [details] [review] Bug 29915: Add a note for tests Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131796 [details] [review] Bug 29915: Add selenium tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131797 [details] [review] Bug 29915: (QA follow-up) Spelling
Not finding any regressions, it's a pretty large patchset, but fairly readable and clear. Auth is always hard to work though, but I'm confident, well done all. Passing QA
(In reply to Jonathan Druart from comment #92) > Those patches are actually fixing bug 30083. So the original problem we > reported here is a regression. > > 21.05.00 is not impacted > 21.05.x neither (b71eebaf6ae) > > So it's certainly caused by bug 28785. > > For master we should push this, but what do we do for 21.11? Hmm, I think this is still an open question :(
(In reply to Martin Renvoize from comment #114) > (In reply to Jonathan Druart from comment #92) > > Those patches are actually fixing bug 30083. So the original problem we > > reported here is a regression. > > > > 21.05.00 is not impacted > > 21.05.x neither (b71eebaf6ae) > > > > So it's certainly caused by bug 28785. > > > > For master we should push this, but what do we do for 21.11? > > Hmm, I think this is still an open question :( With the tests we can be confident and push it to 21.11 as well.
Can we move this one out of security btw ?
(In reply to Marcel de Rooy from comment #116) > Can we move this one out of security btw ? Yes, done.
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
I am having the impression that there is a problem with the session object still. It causes a bad cookie containing a session id. The session has a claimed id, but not an effective id ? Will be looking bit further..
Created attachment 132101 [details] [review] Bug 29915: Prevent bad cookie from corrupted session If there is deleted session info but no session->id, a wrong cookie with empty name could be generated containing expired session id. Test plan: Login. Check cookies in browser. Logout. Check cookies in browser. Without this patch, you should see an invalid cookie. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 132104 [details] [review] Bug 29915: Prevent bad cookie from corrupted session If there is deleted session info but no session->id, a wrong cookie with empty name could be generated containing expired session id. Test plan: Run t/db_dependent/Auth.t Login. Check cookies in browser. Logout. Check cookies in browser. Without this patch, you should see an invalid cookie. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 132114 [details] [review] Bug 29915: Prevent bad cookie from corrupted session If there is deleted session info but no session->id, a wrong cookie with empty name could be generated containing expired session id. Test plan: Run t/db_dependent/Auth.t Login. Check cookies in browser. Logout. Check cookies in browser. Without this patch, you should see an invalid cookie. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 132158 [details] [review] Bug 29915: Prevent bad cookie from corrupted session If there is deleted session info but no session->id, a wrong cookie with empty name could be generated containing expired session id. Test plan: Run t/db_dependent/Auth.t Login. Check cookies in browser. Logout. Check cookies in browser. Without this patch, you should see an invalid cookie. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 132159 [details] [review] Bug 29915: Prevent bad cookie from corrupted session If there is deleted session info but no session->id, a wrong cookie with empty name could be generated containing expired session id. Test plan: Run t/db_dependent/Auth.t Login. Check cookies in browser. Logout. Check cookies in browser. Without this patch, you should see an invalid cookie. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Nice work guys!
> Bug 29915: Prevent bad cookie from corrupted session Pushed to master
(In reply to Fridolin Somers from comment #126) > > Bug 29915: Prevent bad cookie from corrupted session > Pushed to master @Kyle : backport this in 21.11.x
I've been unable to get this to apply to 21.05. Please rebase for backport. Thanks!
(In reply to Andrew Fuerste-Henry from comment #128) > I've been unable to get this to apply to 21.05. Please rebase for backport. > Thanks! Fortunately this bug has been caused by bug 29914 and is not affecting 21.05.x!
*** Bug 30083 has been marked as a duplicate of this bug. ***