Bug 29915 - Anonymous session generates 1 new session ID per hit
Summary: Anonymous session generates 1 new session ID per hit
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 29914
Blocks: 29954 28786
  Show dependency treegraph
 
Reported: 2022-01-20 10:59 UTC by Jonathan Druart
Modified: 2022-04-28 09:21 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.04


Attachments
Bug 29915: Don't generate a new session ID for anonymous navigation (2.17 KB, patch)
2022-01-20 11:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Don't generate a new session ID for anonymous navigation (1.91 KB, patch)
2022-01-20 14:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Add tests (1.99 KB, patch)
2022-01-21 08:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Don't generate a new session ID for anonymous navigation (1.95 KB, patch)
2022-01-25 12:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: Don't generate a new session ID for anonymous navigation (1.95 KB, patch)
2022-01-25 13:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: Changes for get_session and check_cookie_auth (5.00 KB, patch)
2022-01-25 13:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: Add tests (2.34 KB, patch)
2022-01-25 13:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: Don't generate a new session ID for anonymous navigation (1.95 KB, patch)
2022-01-25 13:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: Changes for get_session and check_cookie_auth (5.00 KB, patch)
2022-01-25 13:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: Tiny session adjustments (2.50 KB, patch)
2022-01-25 13:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: (QA follow-up) Fix POD typo (795 bytes, patch)
2022-01-25 14:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: Add tests (5.96 KB, patch)
2022-01-26 10:58 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Don't generate a new session ID for anonymous navigation (2.00 KB, patch)
2022-01-26 15:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: Changes for get_session and check_cookie_auth (5.05 KB, patch)
2022-01-26 15:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: Tiny session adjustments (2.56 KB, patch)
2022-01-26 15:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: (QA follow-up) Fix POD typo (852 bytes, patch)
2022-01-26 15:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: Add tests (6.05 KB, patch)
2022-01-26 15:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: (experimental) Add skip_auth_template param in checkauth (1.20 KB, patch)
2022-01-26 15:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: Changes to Auth.t (5.33 KB, patch)
2022-01-26 15:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: (QA follow-up) Add a comment in checkauth on $flags (950 bytes, patch)
2022-01-26 15:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: Don't generate a new session ID for anonymous navigation (2.00 KB, patch)
2022-01-27 10:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: Changes for get_session and check_cookie_auth (6.83 KB, patch)
2022-01-27 10:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: Tiny session adjustments (2.56 KB, patch)
2022-01-27 10:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: (QA follow-up) Fix POD typo (852 bytes, patch)
2022-01-27 10:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: Add tests (6.05 KB, patch)
2022-01-27 10:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: Changes to Auth.t (7.73 KB, patch)
2022-01-27 10:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: (QA follow-up) Add a comment in checkauth on $flags (950 bytes, patch)
2022-01-27 10:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: Add tests (6.02 KB, patch)
2022-01-27 13:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Changes to Auth.t (7.64 KB, patch)
2022-01-27 13:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: (QA follow-up) Add a comment in checkauth on $flags (912 bytes, patch)
2022-01-27 13:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Add a note for tests (1.31 KB, patch)
2022-01-27 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Changes for get_session and check_cookie_auth (6.87 KB, patch)
2022-01-27 14:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Tiny session adjustments (2.60 KB, patch)
2022-01-27 14:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: (QA follow-up) Fix POD typo (888 bytes, patch)
2022-01-27 14:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Add tests (6.09 KB, patch)
2022-01-27 14:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Changes to Auth.t (7.72 KB, patch)
2022-01-27 14:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: (QA follow-up) Add a comment in checkauth on $flags (986 bytes, patch)
2022-01-27 14:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Add a note for tests (1.38 KB, patch)
2022-01-27 14:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Don't generate a new session ID for anonymous navigation (2.02 KB, patch)
2022-01-27 16:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29915: Changes for get_session and check_cookie_auth (6.92 KB, patch)
2022-01-27 16:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29915: Tiny session adjustments (2.65 KB, patch)
2022-01-27 16:38 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29915: (QA follow-up) Fix POD typo (943 bytes, patch)
2022-01-27 16:38 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29915: Add tests (6.15 KB, patch)
2022-01-27 16:38 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29915: Changes to Auth.t (7.77 KB, patch)
2022-01-27 16:38 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29915: (QA follow-up) Add a comment in checkauth on $flags (1.02 KB, patch)
2022-01-27 16:38 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29915: Add a note for tests (1.44 KB, patch)
2022-01-27 16:38 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29915: Add selenium tests (9.42 KB, patch)
2022-01-28 16:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Don't generate a new session ID for anonymous navigation (2.02 KB, patch)
2022-02-15 09:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Changes for get_session and check_cookie_auth (6.91 KB, patch)
2022-02-15 09:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Tiny session adjustments (2.67 KB, patch)
2022-02-15 09:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: (QA follow-up) Fix POD typo (945 bytes, patch)
2022-02-15 09:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Add tests (6.15 KB, patch)
2022-02-15 09:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Changes to Auth.t (7.77 KB, patch)
2022-02-15 09:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: (QA follow-up) Add a comment in checkauth on $flags (1.02 KB, patch)
2022-02-15 09:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Add a note for tests (1.44 KB, patch)
2022-02-15 09:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Add selenium tests (9.42 KB, patch)
2022-02-15 09:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Don't generate a new session ID for anonymous navigation (2.02 KB, patch)
2022-03-15 13:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Changes for get_session and check_cookie_auth (6.92 KB, patch)
2022-03-15 13:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Tiny session adjustments (2.65 KB, patch)
2022-03-15 13:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: (QA follow-up) Fix POD typo (945 bytes, patch)
2022-03-15 13:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Add tests (6.15 KB, patch)
2022-03-15 13:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Changes to Auth.t (7.77 KB, patch)
2022-03-15 13:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: (QA follow-up) Add a comment in checkauth on $flags (1.02 KB, patch)
2022-03-15 13:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Add a note for tests (1.44 KB, patch)
2022-03-15 13:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Add selenium tests (9.42 KB, patch)
2022-03-15 13:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Don't generate a new session ID for anonymous navigation (2.08 KB, patch)
2022-03-16 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29915: Changes for get_session and check_cookie_auth (6.98 KB, patch)
2022-03-16 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29915: Tiny session adjustments (2.71 KB, patch)
2022-03-16 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29915: (QA follow-up) Fix POD typo (1006 bytes, patch)
2022-03-16 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29915: Add tests (6.21 KB, patch)
2022-03-16 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29915: Changes to Auth.t (7.83 KB, patch)
2022-03-16 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29915: (QA follow-up) Add a comment in checkauth on $flags (1.08 KB, patch)
2022-03-16 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29915: Add a note for tests (1.50 KB, patch)
2022-03-16 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29915: Add selenium tests (9.48 KB, patch)
2022-03-16 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29915: (QA follow-up) Spelling (1011 bytes, patch)
2022-03-16 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29915: Prevent bad cookie from corrupted session (1.66 KB, patch)
2022-03-24 07:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: Prevent bad cookie from corrupted session (2.56 KB, patch)
2022-03-24 08:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: Prevent bad cookie from corrupted session (2.57 KB, patch)
2022-03-24 10:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 29915: Prevent bad cookie from corrupted session (2.60 KB, patch)
2022-03-24 13:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29915: Prevent bad cookie from corrupted session (2.61 KB, patch)
2022-03-24 13:49 UTC, Jonathan Druart
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-20 10:59:11 UTC

    
Comment 1 Jonathan Druart 2022-01-20 11:11:23 UTC
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.
Comment 2 Jonathan Druart 2022-01-20 11:12:59 UTC
Setting to NSO to get feedback but that will need to be strongly tested and reviewed.
Comment 3 Marcel de Rooy 2022-01-20 12:43:03 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2022-01-20 12:44:05 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2022-01-20 14:01:03 UTC
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.
Comment 6 Jonathan Druart 2022-01-21 08:35:48 UTC
Created attachment 129683 [details] [review]
Bug 29915: Add tests
Comment 7 Marcel de Rooy 2022-01-24 12:43:02 UTC
Busy here
Comment 8 Marcel de Rooy 2022-01-24 15:34:45 UTC
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 !
Comment 9 Marcel de Rooy 2022-01-25 12:52:03 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2022-01-25 13:29:45 UTC Comment hidden (obsolete)
Comment 11 Marcel de Rooy 2022-01-25 13:29:49 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2022-01-25 13:30:48 UTC
Created attachment 129752 [details] [review]
Bug 29915: Add tests

Rebased 25-01-22 on changes for 29914.
Comment 13 Marcel de Rooy 2022-01-25 13:30:52 UTC
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.
Comment 14 Marcel de Rooy 2022-01-25 13:30:56 UTC
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.
Comment 15 Marcel de Rooy 2022-01-25 13:57:52 UTC
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.
Comment 16 Marcel de Rooy 2022-01-25 14:48:01 UTC
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.
Comment 17 Marcel de Rooy 2022-01-25 14:54:05 UTC
Created attachment 129756 [details] [review]
Bug 29915: (QA follow-up) Fix POD typo
Comment 18 Marcel de Rooy 2022-01-25 14:59:05 UTC
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 !
Comment 19 Jonathan Druart 2022-01-26 08:38:06 UTC
Why did you remove the _unset_userenv calls?
Comment 20 Jonathan Druart 2022-01-26 10:58:51 UTC
Created attachment 129822 [details] [review]
Bug 29915: Add tests
Comment 21 Jonathan Druart 2022-01-26 11:03:27 UTC
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
Comment 22 Jonathan Druart 2022-01-26 11:04:07 UTC
BTW why do we call haspermission in checkauth? It's called in check_cookie_auth already.
Comment 23 Marcel de Rooy 2022-01-26 11:34:21 UTC
(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.
Comment 24 Marcel de Rooy 2022-01-26 12:16:03 UTC
(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
Comment 25 Jonathan Druart 2022-01-26 13:05:37 UTC
(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?
Comment 26 Marcel de Rooy 2022-01-26 13:09:02 UTC
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.
Comment 27 Marcel de Rooy 2022-01-26 13:10:01 UTC
(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 :)
Comment 28 Marcel de Rooy 2022-01-26 15:45:39 UTC
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>
Comment 29 Marcel de Rooy 2022-01-26 15:45:44 UTC
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>
Comment 30 Marcel de Rooy 2022-01-26 15:45:48 UTC
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>
Comment 31 Marcel de Rooy 2022-01-26 15:45:52 UTC
Created attachment 129829 [details] [review]
Bug 29915: (QA follow-up) Fix POD typo

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2022-01-26 15:45:56 UTC
Created attachment 129830 [details] [review]
Bug 29915: Add tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 33 Marcel de Rooy 2022-01-26 15:46:00 UTC Comment hidden (obsolete)
Comment 34 Marcel de Rooy 2022-01-26 15:46:04 UTC
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>
Comment 35 Marcel de Rooy 2022-01-26 15:47:09 UTC Comment hidden (obsolete)
Comment 36 Marcel de Rooy 2022-01-26 15:47:51 UTC
Would not be bad to have more than one QAer to take a look.
Comment 37 Marcel de Rooy 2022-01-26 15:48:33 UTC
If insisted upon, we could squash the last three patches.
Comment 38 Marcel de Rooy 2022-01-26 15:55:54 UTC
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>
Comment 39 Marcel de Rooy 2022-01-26 16:04:14 UTC
(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.
Comment 40 Marcel de Rooy 2022-01-27 08:08:01 UTC
(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.
Comment 41 Marcel de Rooy 2022-01-27 09:45:23 UTC Comment hidden (obsolete)
Comment 42 Marcel de Rooy 2022-01-27 09:48:07 UTC
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)= @_;
Comment 43 Marcel de Rooy 2022-01-27 09:48:54 UTC
I will workaround this here and open a new report for it elsewhere.
Comment 44 Marcel de Rooy 2022-01-27 09:55:08 UTC
Bug 29954
Comment 45 Marcel de Rooy 2022-01-27 10:01:17 UTC
So, actually this call did not work for years:
-            C4::Context->_unset_userenv($sessionID);
The routine tried to unset 'C4::Context' LOL
Comment 46 Marcel de Rooy 2022-01-27 10:32:12 UTC
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>
Comment 47 Marcel de Rooy 2022-01-27 10:32:20 UTC
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>
Comment 48 Marcel de Rooy 2022-01-27 10:32:26 UTC
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>
Comment 49 Marcel de Rooy 2022-01-27 10:32:32 UTC
Created attachment 129863 [details] [review]
Bug 29915: (QA follow-up) Fix POD typo

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 50 Marcel de Rooy 2022-01-27 10:32:37 UTC
Created attachment 129864 [details] [review]
Bug 29915: Add tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 51 Marcel de Rooy 2022-01-27 10:32:42 UTC
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>
Comment 52 Marcel de Rooy 2022-01-27 10:32:46 UTC
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>
Comment 53 Marcel de Rooy 2022-01-27 10:37:48 UTC
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.
Comment 54 Jonathan Druart 2022-01-27 13:33:16 UTC
Created attachment 129878 [details] [review]
Bug 29915: Add tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 55 Jonathan Druart 2022-01-27 13:33:21 UTC
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>
Comment 56 Jonathan Druart 2022-01-27 13:33:26 UTC
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>
Comment 57 Jonathan Druart 2022-01-27 13:34:22 UTC
Last patches do *not* modify the patchset. Only mistakes from "Add tests" are fixed.
Comment 58 Jonathan Druart 2022-01-27 14:13:07 UTC
Created attachment 129882 [details] [review]
Bug 29915: Add a note for tests
Comment 59 Jonathan Druart 2022-01-27 14:17:40 UTC
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>
Comment 60 Jonathan Druart 2022-01-27 14:17:46 UTC
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>
Comment 61 Jonathan Druart 2022-01-27 14:17:51 UTC
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>
Comment 62 Jonathan Druart 2022-01-27 14:17:56 UTC
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>
Comment 63 Jonathan Druart 2022-01-27 14:18:01 UTC
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>
Comment 64 Jonathan Druart 2022-01-27 14:18:07 UTC
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>
Comment 65 Jonathan Druart 2022-01-27 14:18:12 UTC
Created attachment 129889 [details] [review]
Bug 29915: Add a note for tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 66 Nick Clemens 2022-01-27 16:37:54 UTC
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>
Comment 67 Nick Clemens 2022-01-27 16:37:58 UTC
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>
Comment 68 Nick Clemens 2022-01-27 16:38:03 UTC
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>
Comment 69 Nick Clemens 2022-01-27 16:38:10 UTC
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>
Comment 70 Nick Clemens 2022-01-27 16:38:15 UTC
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>
Comment 71 Nick Clemens 2022-01-27 16:38:20 UTC
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>
Comment 72 Nick Clemens 2022-01-27 16:38:24 UTC
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>
Comment 73 Nick Clemens 2022-01-27 16:38:28 UTC
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>
Comment 74 Marcel de Rooy 2022-01-28 07:22:46 UTC
(In reply to Nick Clemens from comment #73)
> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Thanks, Nick
Comment 75 Marcel de Rooy 2022-01-28 08:52:15 UTC Comment hidden (obsolete)
Comment 76 Marcel de Rooy 2022-01-28 09:42:02 UTC
(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.
Comment 77 Marcel de Rooy 2022-01-28 09:50:05 UTC
Note to RM: This could (imo) be handled as an enhancement after 29914 has been dealt with completely.
Comment 78 Jonathan Druart 2022-01-28 16:12:47 UTC
Created attachment 129951 [details] [review]
Bug 29915: Add selenium tests
Comment 79 Jonathan Druart 2022-01-28 16:14:09 UTC
(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!
Comment 80 Marcel de Rooy 2022-01-31 07:23:09 UTC
(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 ?
Comment 81 Marcel de Rooy 2022-02-04 07:50:50 UTC
Hmm. It now even depends on 29957. I am setting the status back then. It needs another QA session after 29957.
Comment 82 Marcel de Rooy 2022-02-04 10:42:40 UTC
(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 ;)
Comment 83 Jonathan Druart 2022-02-15 09:15:26 UTC
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>
Comment 84 Jonathan Druart 2022-02-15 09:15:34 UTC
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>
Comment 85 Jonathan Druart 2022-02-15 09:15:40 UTC
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>
Comment 86 Jonathan Druart 2022-02-15 09:15:45 UTC
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>
Comment 87 Jonathan Druart 2022-02-15 09:15:51 UTC
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>
Comment 88 Jonathan Druart 2022-02-15 09:15:56 UTC
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>
Comment 89 Jonathan Druart 2022-02-15 09:16:01 UTC
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>
Comment 90 Jonathan Druart 2022-02-15 09:16:07 UTC
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>
Comment 91 Jonathan Druart 2022-02-15 09:16:13 UTC
Created attachment 130604 [details] [review]
Bug 29915: Add selenium tests
Comment 92 Jonathan Druart 2022-02-15 09:37:26 UTC
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?
Comment 93 Jonathan Druart 2022-03-15 13:19:14 UTC
Removing dependency on bug 29957.
Comment 94 Jonathan Druart 2022-03-15 13:21:16 UTC
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>
Comment 95 Jonathan Druart 2022-03-15 13:21:22 UTC
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>
Comment 96 Jonathan Druart 2022-03-15 13:21:29 UTC
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>
Comment 97 Jonathan Druart 2022-03-15 13:21:35 UTC
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>
Comment 98 Jonathan Druart 2022-03-15 13:21:41 UTC
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>
Comment 99 Jonathan Druart 2022-03-15 13:21:47 UTC
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>
Comment 100 Jonathan Druart 2022-03-15 13:21:53 UTC
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>
Comment 101 Jonathan Druart 2022-03-15 13:22:02 UTC
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>
Comment 102 Jonathan Druart 2022-03-15 13:22:09 UTC
Created attachment 131716 [details] [review]
Bug 29915: Add selenium tests
Comment 103 Martin Renvoize 2022-03-16 14:08:15 UTC
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>
Comment 104 Martin Renvoize 2022-03-16 14:08:20 UTC
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>
Comment 105 Martin Renvoize 2022-03-16 14:08:25 UTC
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>
Comment 106 Martin Renvoize 2022-03-16 14:08:30 UTC
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>
Comment 107 Martin Renvoize 2022-03-16 14:08:34 UTC
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>
Comment 108 Martin Renvoize 2022-03-16 14:08:39 UTC
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>
Comment 109 Martin Renvoize 2022-03-16 14:08:44 UTC
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>
Comment 110 Martin Renvoize 2022-03-16 14:08:49 UTC
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>
Comment 111 Martin Renvoize 2022-03-16 14:08:54 UTC
Created attachment 131796 [details] [review]
Bug 29915: Add selenium tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 112 Martin Renvoize 2022-03-16 14:08:58 UTC
Created attachment 131797 [details] [review]
Bug 29915: (QA follow-up) Spelling
Comment 113 Martin Renvoize 2022-03-16 14:10:10 UTC
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
Comment 114 Martin Renvoize 2022-03-16 14:11:26 UTC
(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 :(
Comment 115 Jonathan Druart 2022-03-16 14:21:30 UTC
(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.
Comment 116 Marcel de Rooy 2022-03-18 07:21:36 UTC
Can we move this one out of security btw ?
Comment 117 Jonathan Druart 2022-03-18 08:26:55 UTC
(In reply to Marcel de Rooy from comment #116)
> Can we move this one out of security btw ?

Yes, done.
Comment 118 Fridolin Somers 2022-03-22 20:18:55 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 119 Marcel de Rooy 2022-03-23 16:16:56 UTC
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..
Comment 120 Marcel de Rooy 2022-03-24 07:38:49 UTC
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>
Comment 121 Marcel de Rooy 2022-03-24 08:43:18 UTC
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>
Comment 122 Marcel de Rooy 2022-03-24 10:10:44 UTC
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>
Comment 123 Jonathan Druart 2022-03-24 13:37:41 UTC
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>
Comment 124 Jonathan Druart 2022-03-24 13:49:27 UTC
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>
Comment 125 Martin Renvoize 2022-03-24 16:26:09 UTC
Nice work guys!
Comment 126 Fridolin Somers 2022-03-24 19:19:50 UTC
> Bug 29915: Prevent bad cookie from corrupted session
Pushed to master
Comment 127 Fridolin Somers 2022-04-05 15:12:42 UTC
(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
Comment 128 Andrew Fuerste-Henry 2022-04-25 19:23:25 UTC
I've been unable to get this to apply to 21.05. Please rebase for backport. Thanks!
Comment 129 Jonathan Druart 2022-04-28 09:21:04 UTC
(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!