Bug 17427 - Replace CGI::Session with Data::Session
Summary: Replace CGI::Session with Data::Session
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Bugs List
QA Contact: Marcel de Rooy
URL:
Keywords: rel_23_11_candidate
: 29942 (view as bug list)
Depends on: 28519
Blocks:
  Show dependency treegraph
 
Reported: 2016-10-11 13:34 UTC by Jonathan Druart
Modified: 2024-02-15 02:19 UTC (History)
9 users (show)

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


Attachments
Bug 17427: Replace CGI::Session with Data::Session (11.25 KB, patch)
2016-10-11 13:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Replace CGI::Session with Data::Session (14.12 KB, patch)
2016-10-14 10:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Replace CGI::Session with Data::Session (14.19 KB, patch)
2016-10-16 12:49 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 17427: Replace CGI::Session with Data::Session (14.17 KB, patch)
2016-11-02 09:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Replace CGI::Session with Data::Session (14.16 KB, patch)
2017-02-16 14:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Replace CGI::Session with Data::Session (11.27 KB, patch)
2021-05-24 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Use JSON serialization (2.13 KB, patch)
2021-06-04 15:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Replace CGI::Session with Data::Session (11.44 KB, patch)
2021-06-07 12:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Use JSON serialization (2.16 KB, patch)
2021-06-07 12:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Use our own YAML::XS serializer (2.33 KB, patch)
2021-06-07 12:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Replace CGI::Session with Data::Session (11.69 KB, patch)
2023-05-03 10:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Use JSON serialization (2.16 KB, patch)
2023-05-03 10:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Use our own YAML::XS serializer (2.32 KB, patch)
2023-05-03 10:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Replace CGI::Session with Data::Session (11.73 KB, patch)
2023-05-03 16:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17427: Use JSON serialization (2.21 KB, patch)
2023-05-03 16:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17427: Use our own YAML::XS serializer (2.37 KB, patch)
2023-05-03 16:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17427: Remove use lines for CGI::Session from Auth.pm (654 bytes, patch)
2023-05-03 16:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17427: Remove more occurrences of CGI::Session (1.56 KB, patch)
2023-05-04 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Remove SessionStorage mock from tests (5.59 KB, patch)
2023-05-04 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Embed Data::Session (203.07 KB, patch)
2023-05-04 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Remove confusion between session id and param id (7.24 KB, patch)
2023-05-04 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: HACKY - force new session (1.46 KB, patch)
2023-05-04 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Retrieve sessionID from HTTP_COOKIE (1.07 KB, patch)
2023-05-04 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Fix id vs userid in tests (3.30 KB, patch)
2023-05-04 10:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Remove SessionStorage mock from tests (5.60 KB, patch)
2023-05-10 06:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Embed Data::Session (203.07 KB, patch)
2023-05-10 06:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Remove confusion between session id and param id (7.24 KB, patch)
2023-05-10 06:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: HACKY - force new session (1.46 KB, patch)
2023-05-10 06:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Retrieve sessionID from HTTP_COOKIE (1.34 KB, patch)
2023-05-10 06:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Fix id vs userid in tests (3.30 KB, patch)
2023-05-10 06:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Replace CGI::Session with Data::Session (11.31 KB, patch)
2023-07-05 08:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Use JSON serialization (2.21 KB, patch)
2023-07-05 08:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Use our own YAML::XS serializer (2.38 KB, patch)
2023-07-05 08:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Remove use lines for CGI::Session from Auth.pm (656 bytes, patch)
2023-07-05 08:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Remove more occurrences of CGI::Session (1.56 KB, patch)
2023-07-05 08:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Remove SessionStorage mock from tests (5.60 KB, patch)
2023-07-05 08:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Embed Data::Session (203.07 KB, patch)
2023-07-05 08:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Remove confusion between session id and param id (7.24 KB, patch)
2023-07-05 08:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: HACKY - force new session (1.46 KB, patch)
2023-07-05 08:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Retrieve sessionID from HTTP_COOKIE (1.34 KB, patch)
2023-07-05 08:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17427: Fix id vs userid in tests (3.30 KB, patch)
2023-07-05 08:04 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 2016-10-11 13:34:37 UTC
CGI::Session is not maintained for ages and seems quite buggy.
It would be the culprit of random logout problems.

Another issue is the following crazy code in CGI::Session::Driver::DBI

sub DESTROY {
    my $self = shift;

    unless ( defined $self->{Handle} && $self->{Handle} -> ping ) {
        $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away');
        return;
        }

    unless ( $self->{Handle}->{AutoCommit} ) {
        $self->{Handle}->commit;
    }
    if ( $self->{_disconnect} ) {
        $self->{Handle}->disconnect;
    }
}


If AutoCommit is off, it will commit anyway...

It causes some tests to fail (api) if SessionStorage is set to MySQL
Comment 1 Jonathan Druart 2016-10-11 13:37:56 UTC
Created attachment 56185 [details] [review]
Bug 17427: Replace CGI::Session with Data::Session

CGI::Session is not maintained for ages and seems quite buggy.
It would be the culprit of random logout problems.

Another issue is the following crazy code in CGI::Session::Driver::DBI

sub DESTROY {
    my $self = shift;

    unless ( defined $self->{Handle} && $self->{Handle} -> ping ) {
        $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away');
        return;
        }

    unless ( $self->{Handle}->{AutoCommit} ) {
        $self->{Handle}->commit;
    }
    if ( $self->{_disconnect} ) {
        $self->{Handle}->disconnect;
    }
}

If AutoCommit is off, it will commit anyway...

It causes some tests to fail (api) if SessionStorage is set to MySQL

This is just a POC and will only works with SessionStorage set to MySQL.

Please test it and tell me if you find bug. If it looks bug free I will submit
a follow-up to cover the other combinations of SessionStorage (temporary storage
and Memcached)
Comment 2 Benjamin Rokseth 2016-10-13 22:00:22 UTC
This is great detective work, and it makes real sense to deprecate a deprecated session handler! I have tested with api and finally tests with autocommit set to null actually rolls back, so I no longer am left with a skewed db.

please bring this on and I will happily test and sign off!
Comment 3 Jonathan Druart 2016-10-14 10:21:39 UTC
Created attachment 56504 [details] [review]
Bug 17427: Replace CGI::Session with Data::Session

CGI::Session is not maintained for ages and seems quite buggy.
It would be the culprit of random logout problems.

Another issue is the following crazy code in CGI::Session::Driver::DBI

sub DESTROY {
    my $self = shift;

    unless ( defined $self->{Handle} && $self->{Handle} -> ping ) {
        $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away');
        return;
        }

    unless ( $self->{Handle}->{AutoCommit} ) {
        $self->{Handle}->commit;
    }
    if ( $self->{_disconnect} ) {
        $self->{Handle}->disconnect;
    }
}

If AutoCommit is off, it will commit anyway...

It causes some tests to fail (api) if SessionStorage is set to MySQL

Note that PostgreSQL is not supported yet so no need to implement it.

Test plan:
0/ The dependencies are not packaged for debian so far, so install it
via cpan
1/ Test the 3 different values for SessionStorage
2/ Using different browsers (or sessions) login into Koha with different
users. Naviguate => you should not be logged out
3/ Set SessionStorage to Memcached, stop memcached make sure that it
defaults to file.
Comment 4 Jonathan Druart 2016-10-14 10:22:18 UTC
(In reply to Benjamin Rokseth from comment #2)
> This is great detective work, and it makes real sense to deprecate a
> deprecated session handler! I have tested with api and finally tests with
> autocommit set to null actually rolls back, so I no longer am left with a
> skewed db.
> 
> please bring this on and I will happily test and sign off!

It's your turn to play ;)
Comment 5 Jonathan Druart 2016-10-14 10:22:52 UTC
I should note: The first patch did not work as expected, it defaulted to File (instead of mysql).
Comment 6 Benjamin Rokseth 2016-10-16 12:49:28 UTC
Created attachment 56572 [details] [review]
Bug 17427: Replace CGI::Session with Data::Session

CGI::Session is not maintained for ages and seems quite buggy.
It would be the culprit of random logout problems.

Another issue is the following crazy code in CGI::Session::Driver::DBI

sub DESTROY {
    my $self = shift;

    unless ( defined $self->{Handle} && $self->{Handle} -> ping ) {
        $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away');
        return;
        }

    unless ( $self->{Handle}->{AutoCommit} ) {
        $self->{Handle}->commit;
    }
    if ( $self->{_disconnect} ) {
        $self->{Handle}->disconnect;
    }
}

If AutoCommit is off, it will commit anyway...

It causes some tests to fail (api) if SessionStorage is set to MySQL

Note that PostgreSQL is not supported yet so no need to implement it.

Test plan:
0/ The dependencies are not packaged for debian so far, so install it
via cpan
1/ Test the 3 different values for SessionStorage
2/ Using different browsers (or sessions) login into Koha with different
users. Naviguate => you should not be logged out
3/ Set SessionStorage to Memcached, stop memcached make sure that it
defaults to file.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 7 Benjamin Rokseth 2016-10-16 12:51:37 UTC
Works perfectly! Corrected a small typo in the PerlDependencies.

No sessions lost in tempfiles, mysql or memcached, and they survived a plack restart.
Comment 8 Martin Renvoize 2016-10-21 14:28:19 UTC
Comment on attachment 56572 [details] [review]
Bug 17427: Replace CGI::Session with Data::Session

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

Generally looks good.. minor question about using ENV though.. not sure about that part of the change.

::: C4/Auth.pm
@@ +1111,4 @@
>                      $session->param( 'branchname',   $branchname );
>                      $session->param( 'flags',        $userflags );
>                      $session->param( 'emailaddress', $emailaddress );
> +                    $session->param( 'ip',           $ENV{REMOTE_ADDR} );

Did we test this against plack?  Can we really rely on ENV for remote_addr... and in fact.. do we not compare the session ip to the env remote_addr in places as a security check?
Comment 9 Tomás Cohen Arazi 2016-10-21 15:50:32 UTC
(In reply to Martin Renvoize from comment #8)
> Comment on attachment 56572 [details] [review] [review]
> Bug 17427: Replace CGI::Session with Data::Session
> 
> Review of attachment 56572 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Generally looks good.. minor question about using ENV though.. not sure
> about that part of the change.
> 
> ::: C4/Auth.pm
> @@ +1111,4 @@
> >                      $session->param( 'branchname',   $branchname );
> >                      $session->param( 'flags',        $userflags );
> >                      $session->param( 'emailaddress', $emailaddress );
> > +                    $session->param( 'ip',           $ENV{REMOTE_ADDR} );
> 
> Did we test this against plack?  Can we really rely on ENV for
> remote_addr... and in fact.. do we not compare the session ip to the env
> remote_addr in places as a security check?

$ENV{REMOTE_ADDR} is correctly filled on each request by the ReverseProxy middleware as far as I recall (I debugged plack+ENV several days before making the packages plack integration official).
Comment 10 Tomás Cohen Arazi 2016-11-01 12:47:50 UTC
Please rebase. I'd say this one depends only on Mirko's answer about packaging Data::Session.
Comment 11 Jonathan Druart 2016-11-02 09:02:42 UTC
Created attachment 57067 [details] [review]
Bug 17427: Replace CGI::Session with Data::Session

CGI::Session is not maintained for ages and seems quite buggy.
It would be the culprit of random logout problems.

Another issue is the following crazy code in CGI::Session::Driver::DBI

sub DESTROY {
    my $self = shift;

    unless ( defined $self->{Handle} && $self->{Handle} -> ping ) {
        $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away');
        return;
        }

    unless ( $self->{Handle}->{AutoCommit} ) {
        $self->{Handle}->commit;
    }
    if ( $self->{_disconnect} ) {
        $self->{Handle}->disconnect;
    }
}

If AutoCommit is off, it will commit anyway...

It causes some tests to fail (api) if SessionStorage is set to MySQL

Note that PostgreSQL is not supported yet so no need to implement it.

Test plan:
0/ The dependencies are not packaged for debian so far, so install it
via cpan
1/ Test the 3 different values for SessionStorage
2/ Using different browsers (or sessions) login into Koha with different
users. Naviguate => you should not be logged out
3/ Set SessionStorage to Memcached, stop memcached make sure that it
defaults to file.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 12 Jonathan Druart 2016-11-02 09:04:05 UTC
I do not recommend to push this patch into the next stable release (at least not at the beginning).
Comment 13 Jonathan Druart 2017-02-16 14:42:40 UTC
Created attachment 60355 [details] [review]
Bug 17427: Replace CGI::Session with Data::Session

CGI::Session is not maintained for ages and seems quite buggy.
It would be the culprit of random logout problems.

Another issue is the following crazy code in CGI::Session::Driver::DBI

sub DESTROY {
    my $self = shift;

    unless ( defined $self->{Handle} && $self->{Handle} -> ping ) {
        $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away');
        return;
        }

    unless ( $self->{Handle}->{AutoCommit} ) {
        $self->{Handle}->commit;
    }
    if ( $self->{_disconnect} ) {
        $self->{Handle}->disconnect;
    }
}

If AutoCommit is off, it will commit anyway...

It causes some tests to fail (api) if SessionStorage is set to MySQL

Note that PostgreSQL is not supported yet so no need to implement it.

Test plan:
0/ The dependencies are not packaged for debian so far, so install it
via cpan
1/ Test the 3 different values for SessionStorage
2/ Using different browsers (or sessions) login into Koha with different
users. Naviguate => you should not be logged out
3/ Set SessionStorage to Memcached, stop memcached make sure that it
defaults to file.

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 14 Jonathan Druart 2017-02-16 14:43:03 UTC
Just a rebase...
Comment 15 Marcel de Rooy 2017-02-17 08:05:21 UTC
QA: Looking here now
Comment 16 Marcel de Rooy 2017-02-17 08:12:32 UTC
(In reply to Benjamin Rokseth from comment #7)
> Works perfectly! Corrected a small typo in the PerlDependencies.
> 
> No sessions lost in tempfiles, mysql or memcached, and they survived a plack
> restart.

Benjamin: What browsers did you test?
Comment 17 Marcel de Rooy 2017-02-17 09:03:54 UTC
This seems to break scripts like stage-marc-import and tools/upload.pl.
I am still testing a bit but after successfully uploading one file, I am not able to upload anymore.
Also saw this error:
stage-marc-import.pl: YAML::Tiny does not support C4::BackgroundJob references at /usr/local/share/perl/5.20.2/Data/Session/Serialize/YAML.pm line 18., 

Suspect that all scripts using BackgroungJob have issues now. Note that these jobs append information to the session.
Comment 18 Marcel de Rooy 2017-02-17 10:24:04 UTC
Data::Session creates a CGI object.
And we do not like that in scripts like upload-file.pl that call check_cookie_auth (calling get_session).
The created CGI object in this case makes the upload disappear.
Other scripts may have issues too going the similar cookie_auth way..
Comment 19 Jonathan Druart 2017-02-17 13:39:34 UTC
(In reply to Marcel de Rooy from comment #17)
> This seems to break scripts like stage-marc-import and tools/upload.pl.
> I am still testing a bit but after successfully uploading one file, I am not
> able to upload anymore.
> Also saw this error:
> stage-marc-import.pl: YAML::Tiny does not support C4::BackgroundJob
> references at /usr/local/share/perl/5.20.2/Data/Session/Serialize/YAML.pm
> line 18., 

That can be fixed using DataDumper or FreezeThaw instead of YAML as serializer.

(In reply to Marcel de Rooy from comment #18)
> Data::Session creates a CGI object.
> And we do not like that in scripts like upload-file.pl that call
> check_cookie_auth (calling get_session).
> The created CGI object in this case makes the upload disappear.
> Other scripts may have issues too going the similar cookie_auth way..

I do not know how to fix that.
Comment 20 Jonathan Druart 2021-05-24 14:14:44 UTC
Created attachment 121337 [details] [review]
Bug 17427: Replace CGI::Session with Data::Session

CGI::Session is not maintained for ages and seems quite buggy.
It would be the culprit of random logout problems.

Another issue is the following crazy code in CGI::Session::Driver::DBI

sub DESTROY {
    my $self = shift;

    unless ( defined $self->{Handle} && $self->{Handle} -> ping ) {
        $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away');
        return;
        }

    unless ( $self->{Handle}->{AutoCommit} ) {
        $self->{Handle}->commit;
    }
    if ( $self->{_disconnect} ) {
        $self->{Handle}->disconnect;
    }
}

If AutoCommit is off, it will commit anyway...

It causes some tests to fail (api) if SessionStorage is set to MySQL

Note that PostgreSQL is not supported yet so no need to implement it.

Test plan:
0/ The dependencies are not packaged for debian so far, so install it
via cpan
1/ Test the 3 different values for SessionStorage
2/ Using different browsers (or sessions) login into Koha with different
users. Naviguate => you should not be logged out
3/ Set SessionStorage to Memcached, stop memcached make sure that it
defaults to file.
Comment 21 Jonathan Druart 2021-05-24 14:15:11 UTC
(patch rebased)
Comment 22 David Cook 2021-05-25 01:09:01 UTC
(In reply to Jonathan Druart from comment #21)
> (patch rebased)

Should this be "Needs Signoff" now?
Comment 23 Jonathan Druart 2021-05-25 15:08:58 UTC
(In reply to David Cook from comment #22)
> (In reply to Jonathan Druart from comment #21)
> > (patch rebased)
> 
> Should this be "Needs Signoff" now?

I rebased it to see if it fixed bug 28158, it doesn't.
This patch can be tested, but I haven't much so far.

In my opinion that's still one we need, maybe a good candidate for early 21.11 adoption?

Feel free to test it!
Comment 24 Jonathan Druart 2021-06-04 15:35:46 UTC
Created attachment 121633 [details] [review]
Bug 17427: Use JSON serialization
Comment 25 Jonathan Druart 2021-06-04 15:41:24 UTC
We are now aiming to provide a long-term solution for the encoding issue found on bug 28489.

Using Data::Session using the JSON serializer seems to be the more robust solution.

However there is still an issue if SessionStorage=tmp file

Note that sessions.a_session must be longtext.
Comment 26 Jonathan Druart 2021-06-04 15:42:34 UTC
(In reply to Jonathan Druart from comment #25)
> However there is still an issue if SessionStorage=tmp file


root@kohadevbox$ more /var/lib/koha/kohadev/tmp/cgisess_koha_kohadev/cgisess_6967e16ccb3dddff1c32735b7930e967
{"_SESSION_CTIME":1622821220,"_SESSION_ETIME":0,"id":"koha","interface":"intranet","number":51,"ip":"172.20.0.1","lasttime":"1622821226","_SESSION_PTIME":{},"_SESSION_ATIME":1622821226,"flags":1,"surname":"koha"
,"cardnumber":"42","_SESSION_ID":"6967e16ccb3dddff1c32735b7930e967","branchname":"✔️ ❤️ ★","branch":"✔️❤️★"}
root@kohadevbox$ file /var/lib/koha/kohadev/tmp/cgisess_koha_kohadev/cgisess_6967e16ccb3dddff1c32735b7930e967
/var/lib/koha/kohadev/tmp/cgisess_koha_kohadev/cgisess_6967e16ccb3dddff1c32735b7930e967: JSON data
Comment 27 Katrin Fischer 2021-06-04 15:54:00 UTC
Bug 28517 might be related to this?
Comment 28 Jonathan Druart 2021-06-07 11:35:52 UTC
JSON serialization does not work (when SessionStorage=tmp), selenium tests from bug 28489 are failing.

With longblob:
    #   Failed test 'logged-in-branch-name set - SessionStorage=mysql, branchname=Test2❤️'                
    #   at t/db_dependent/selenium/regressions.t line 306.                      
    #          got: 'Test2â¤ï¸Â'                                                                  
    #     expected: 'Test2❤️'      

With longtext:
    #   Failed test 'logged-in-branch-name set - SessionStorage=tmp, branchname=Test2❤️'
    #   at t/db_dependent/selenium/regressions.t line 306.
    #          got: 'Test2â¤ï¸'
    #     expected: 'Test2❤️'
Comment 29 Jonathan Druart 2021-06-07 11:45:19 UTC
And it's not possible to add a serializer that is not in the same dir as others, because of this code in Data::Session:

 318 sub get_my_serializers
 319 {
 320     my($self)   = @_;
 321     my($path)   = $self -> _get_pm_path('Serialize');
 322 
 323     # Warning: Use sort map{} read_dir, not map{} sort read_dir. But, why?
 324 
 325     my(@serializer) = sort map{s/.pm//; $_} read_dir($path);
 326 
 327     ($#serializer < 0) && die __PACKAGE__ . '. No serializers available';
 328 
 329     ($self -> verbose > 1) && $self -> log('Serializers: ' . join(', ', @serializer) );
 330 
 331     $self -> my_serializers(\@serializer);
 332 
 333 } # End of get_my_serializers.
Comment 30 Jonathan Druart 2021-06-07 12:03:04 UTC
(In reply to Jonathan Druart from comment #28)

This comment was confusing, with JSON serialization we have:
With longtext it's working with SessionStorage=mysql
With longblob it's failing with mysql.

But it does not work with SessionStorage=tmp.
Comment 31 Jonathan Druart 2021-06-07 12:14:17 UTC
Created attachment 121657 [details] [review]
Bug 17427: Replace CGI::Session with Data::Session

CGI::Session is not maintained for ages and seems quite buggy.
It would be the culprit of random logout problems.

Another issue is the following crazy code in CGI::Session::Driver::DBI

sub DESTROY {
    my $self = shift;

    unless ( defined $self->{Handle} && $self->{Handle} -> ping ) {
        $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away');
        return;
        }

    unless ( $self->{Handle}->{AutoCommit} ) {
        $self->{Handle}->commit;
    }
    if ( $self->{_disconnect} ) {
        $self->{Handle}->disconnect;
    }
}

If AutoCommit is off, it will commit anyway...

It causes some tests to fail (api) if SessionStorage is set to MySQL

Note that PostgreSQL is not supported yet so no need to implement it.

Test plan:
0/ The dependencies are not packaged for debian so far, so install it
via cpan
1/ Test the 3 different values for SessionStorage
2/ Using different browsers (or sessions) login into Koha with different
users. Naviguate => you should not be logged out
3/ Set SessionStorage to Memcached, stop memcached make sure that it
defaults to file.
Comment 32 Jonathan Druart 2021-06-07 12:14:22 UTC
Created attachment 121658 [details] [review]
Bug 17427: Use JSON serialization
Comment 33 Jonathan Druart 2021-06-07 12:14:27 UTC
Created attachment 121659 [details] [review]
Bug 17427: Use our own YAML::XS serializer
Comment 34 Jonathan Druart 2021-06-07 12:15:21 UTC
And, FWIW, if I move the YAML::XS serializer (see last patch) in the Data/Session/Serialize directory, all tests pass (longblob).
Comment 35 David Cook 2021-06-07 23:16:14 UTC
(In reply to Jonathan Druart from comment #34)
> And, FWIW, if I move the YAML::XS serializer (see last patch) in the
> Data/Session/Serialize directory, all tests pass (longblob).

Does this have an unlisted dependency on Bug 28519? I'm just curious since the serializers are in a "lib" directory.
Comment 36 Jonathan Druart 2021-06-08 07:39:02 UTC
Yes David.
Comment 37 Mason James 2021-06-12 03:07:23 UTC
hi joubu
i've uploaded a libdata-session-perl pkg to koha-staging/dev, for testing

root@kohadevbox:/kohadevbox# apt install libdata-session-perl
Reading package lists... Done
Building dependency tree
Reading state information... Done
The following additional packages will be installed:
  libautovivification-perl libdbd-sqlite3-perl libdbix-admin-createtable-perl
  libfile-slurper-perl libhash-fieldhash-perl libperlio-utf8-strict-perl
The following NEW packages will be installed:
  libautovivification-perl libdata-session-perl libdbd-sqlite3-perl
  libdbix-admin-createtable-perl libfile-slurper-perl libhash-fieldhash-perl
  libperlio-utf8-strict-perl
0 upgraded, 7 newly installed, 0 to remove and 2 not upgraded.
Need to get 376 kB of archives.

root@kohadevbox:/kohadevbox# apt-cache policy libdata-session-perl
libdata-session-perl:
  Installed: 1.18-1
  Candidate: 1.18-1
  Version table:
 *** 1.18-1 500
        500 http://debian.koha-community.org/koha-staging dev/main amd64 Packages
        100 /var/lib/dpkg/status
Comment 38 David Cook 2021-06-25 04:43:19 UTC
(In reply to Jonathan Druart from comment #34)
> And, FWIW, if I move the YAML::XS serializer (see last patch) in the
> Data/Session/Serialize directory, all tests pass (longblob).

Wait this works despite https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17427#c29 ?
Comment 39 David Cook 2021-06-25 04:45:33 UTC
(In reply to David Cook from comment #38)
> (In reply to Jonathan Druart from comment #34)
> > And, FWIW, if I move the YAML::XS serializer (see last patch) in the
> > Data/Session/Serialize directory, all tests pass (longblob).
> 
> Wait this works despite
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17427#c29 ?

Oh maybe I misunderstand what you're saying... you meant in the vendor perl not the Koha perl lib... right?
Comment 40 Jonathan Druart 2021-06-25 06:30:40 UTC
(In reply to David Cook from comment #39)
> (In reply to David Cook from comment #38)
> > (In reply to Jonathan Druart from comment #34)
> > > And, FWIW, if I move the YAML::XS serializer (see last patch) in the
> > > Data/Session/Serialize directory, all tests pass (longblob).
> > 
> > Wait this works despite
> > https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17427#c29 ?
> 
> Oh maybe I misunderstand what you're saying... you meant in the vendor perl
> not the Koha perl lib... right?

Yes, I copied it to /path/Data/Session/Serialize/yamlxs.pm where /path/Data/Session/ is `pmpath Data::Session`.
Comment 41 David Cook 2021-06-25 07:11:36 UTC
(In reply to Jonathan Druart from comment #40)
> > Oh maybe I misunderstand what you're saying... you meant in the vendor perl
> > not the Koha perl lib... right?
> 
> Yes, I copied it to /path/Data/Session/Serialize/yamlxs.pm where
> /path/Data/Session/ is `pmpath Data::Session`.

But that's not a real solution for production. Shouldn't "Needs Signoff" be changed back to 'Assigned' or "In Discussion"?
Comment 42 Jonathan Druart 2021-06-25 08:31:00 UTC
Ok, let's discuss then. What do we do?

I am ok with having our module where Data::Session is expecting it.
Comment 43 David Cook 2021-06-28 01:31:54 UTC
(In reply to Jonathan Druart from comment #42)
> Ok, let's discuss then. What do we do?
>
> I am ok with having our module where Data::Session is expecting it.

How do you propose to do that in production though? 

The only reasonable option I see would be creating our Data::Session::Serializer::yamlxs as a standalone library (with its own git repository), and then packaging that and storing it in the Koha Community APT repo, and then adding it as a Koha dependency.
Comment 44 Tomás Cohen Arazi 2022-08-13 23:29:34 UTC
What about https://metacpan.org/pod/MojoX::Session
Comment 45 David Cook 2022-08-15 23:24:24 UTC
(In reply to Tomás Cohen Arazi from comment #44)
> What about https://metacpan.org/pod/MojoX::Session

It hasn't been updated in over 7 years and the developer says he doesn't want to develop it anymore (although he said he was open to pull requests): https://github.com/vti/mojox-session/issues/19

But then there's been a pull request sitting for over 8 years too https://github.com/vti/mojox-session/pull/14
Comment 46 David Cook 2022-08-15 23:32:23 UTC
Marcel suggests writing Koha::Session at https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29942#c2 

Maybe we should just do that. Jonathan opened this issue nearly 6 years ago and we still haven't resolved it...
Comment 47 Jonathan Druart 2023-05-03 10:38:22 UTC
(In reply to David Cook from comment #43)
> (In reply to Jonathan Druart from comment #42)
> > Ok, let's discuss then. What do we do?
> >
> > I am ok with having our module where Data::Session is expecting it.
> 
> How do you propose to do that in production though? 
> 
> The only reasonable option I see would be creating our
> Data::Session::Serializer::yamlxs as a standalone library (with its own git
> repository), and then packaging that and storing it in the Koha Community
> APT repo, and then adding it as a Koha dependency.

I don't understand. Why isn't this ok for production? It's what we do already for CGI::Session
Comment 48 Jonathan Druart 2023-05-03 10:41:08 UTC
Created attachment 150569 [details] [review]
Bug 17427: Replace CGI::Session with Data::Session

CGI::Session is not maintained for ages and seems quite buggy.
It would be the culprit of random logout problems.

Another issue is the following crazy code in CGI::Session::Driver::DBI

sub DESTROY {
    my $self = shift;

    unless ( defined $self->{Handle} && $self->{Handle} -> ping ) {
        $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away');
        return;
        }

    unless ( $self->{Handle}->{AutoCommit} ) {
        $self->{Handle}->commit;
    }
    if ( $self->{_disconnect} ) {
        $self->{Handle}->disconnect;
    }
}

If AutoCommit is off, it will commit anyway...

It causes some tests to fail (api) if SessionStorage is set to MySQL

Note that PostgreSQL is not supported yet so no need to implement it.

Test plan:
0/ The dependencies are not packaged for debian so far, so install it
via cpan
1/ Test the 3 different values for SessionStorage
2/ Using different browsers (or sessions) login into Koha with different
users. Naviguate => you should not be logged out
3/ Set SessionStorage to Memcached, stop memcached make sure that it
defaults to file.
Comment 49 Jonathan Druart 2023-05-03 10:41:12 UTC
Created attachment 150570 [details] [review]
Bug 17427: Use JSON serialization
Comment 50 Jonathan Druart 2023-05-03 10:41:16 UTC
Created attachment 150571 [details] [review]
Bug 17427: Use our own YAML::XS serializer
Comment 51 Jonathan Druart 2023-05-03 10:41:41 UTC
Patches rebased, status back to NSO. Why isn't this ready? What am I missing?

Everything is working correctly in my tests.
Comment 52 Jonathan Druart 2023-05-03 10:42:04 UTC
*** Bug 29942 has been marked as a duplicate of this bug. ***
Comment 53 Jonathan Druart 2023-05-03 10:46:43 UTC
This needs more work to be ready for push:
* Adjust deps (cpanfile)
* `git grep CGI::Session` occurrences to be adjusted
* Remove FIXMEs from api tests

But please test and tell me if it's worth spending time to continue here.
Comment 54 Kyle M Hall 2023-05-03 10:57:41 UTC
(In reply to Jonathan Druart from comment #53)
> This needs more work to be ready for push:
> * Adjust deps (cpanfile)
> * `git grep CGI::Session` occurrences to be adjusted
> * Remove FIXMEs from api tests
> 
> But please test and tell me if it's worth spending time to continue here.

I seems worthwhile to me if only because CGI::Session is unmaintained!
Comment 55 Tomás Cohen Arazi 2023-05-03 11:58:16 UTC
(In reply to Jonathan Druart from comment #53)
> This needs more work to be ready for push:
> * Adjust deps (cpanfile)
> * `git grep CGI::Session` occurrences to be adjusted
> * Remove FIXMEs from api tests
> 
> But please test and tell me if it's worth spending time to continue here.

+1
Comment 56 Kyle M Hall 2023-05-03 14:16:59 UTC
I'm getting the error "Data::Session. Unknown serialize 'yamlxs' at /usr/local/share/perl/5.32.1/Data/Session.pm line 752".

I installed Data::Session, are there other deps I missed?
Comment 57 Jonathan Druart 2023-05-03 15:05:09 UTC
(In reply to Kyle M Hall from comment #56)
> I'm getting the error "Data::Session. Unknown serialize 'yamlxs' at
> /usr/local/share/perl/5.32.1/Data/Session.pm line 752".
> 
> I installed Data::Session, are there other deps I missed?

Yes, you need to copy the lib file
cp lib/Data/Session/Serialize/yamlxs.pm /usr/share/perl5/Data/Session/Serialize/yamlxs.pm
Comment 58 Tomás Cohen Arazi 2023-05-03 15:07:31 UTC
Can we (In reply to Jonathan Druart from comment #57)
> (In reply to Kyle M Hall from comment #56)
> > I'm getting the error "Data::Session. Unknown serialize 'yamlxs' at
> > /usr/local/share/perl/5.32.1/Data/Session.pm line 752".
> > 
> > I installed Data::Session, are there other deps I missed?
> 
> Yes, you need to copy the lib file
> cp lib/Data/Session/Serialize/yamlxs.pm
> /usr/share/perl5/Data/Session/Serialize/yamlxs.pm

Can we ship Data::Session inside the Koha project instead, and get rid of the need for that?
Comment 59 Jonathan Druart 2023-05-03 15:10:48 UTC
No, please read the previous comments.
Comment 60 Jonathan Druart 2023-05-03 15:11:46 UTC Comment hidden (obsolete)
Comment 61 Jonathan Druart 2023-05-03 15:12:12 UTC
(In reply to Jonathan Druart from comment #59)
> No, please read the previous comments.

Well, yes, we could, if we embed Data::Session and modify it. But why would
we do that?

Isn't the "lib trick" already in place anyway?
Comment 62 Kyle M Hall 2023-05-03 16:20:40 UTC
Created attachment 150603 [details] [review]
Bug 17427: Replace CGI::Session with Data::Session

CGI::Session is not maintained for ages and seems quite buggy.
It would be the culprit of random logout problems.

Another issue is the following crazy code in CGI::Session::Driver::DBI

sub DESTROY {
    my $self = shift;

    unless ( defined $self->{Handle} && $self->{Handle} -> ping ) {
        $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away');
        return;
        }

    unless ( $self->{Handle}->{AutoCommit} ) {
        $self->{Handle}->commit;
    }
    if ( $self->{_disconnect} ) {
        $self->{Handle}->disconnect;
    }
}

If AutoCommit is off, it will commit anyway...

It causes some tests to fail (api) if SessionStorage is set to MySQL

Note that PostgreSQL is not supported yet so no need to implement it.

Test plan:
0/ The dependencies are not packaged for debian so far, so install it
via cpan
1/ Test the 3 different values for SessionStorage
2/ Using different browsers (or sessions) login into Koha with different
users. Naviguate => you should not be logged out
3/ Set SessionStorage to Memcached, stop memcached make sure that it
defaults to file.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 63 Kyle M Hall 2023-05-03 16:20:49 UTC
Created attachment 150604 [details] [review]
Bug 17427: Use JSON serialization

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 64 Kyle M Hall 2023-05-03 16:20:52 UTC
Created attachment 150605 [details] [review]
Bug 17427: Use our own YAML::XS serializer

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 65 Kyle M Hall 2023-05-03 16:20:54 UTC
Created attachment 150606 [details] [review]
Bug 17427: Remove use lines for CGI::Session from Auth.pm

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 66 Kyle M Hall 2023-05-03 16:25:39 UTC
The big blocker is replacing the use of CGI::Session::Find (https://metacpan.org/pod/CGI::Session#find(-%5C&code-)) in C4::Auth_with_cas
Comment 67 Jonathan Druart 2023-05-04 07:36:48 UTC
(In reply to Jonathan Druart from comment #61)
> (In reply to Jonathan Druart from comment #59)
> > No, please read the previous comments.
> 
> Well, yes, we could, if we embed Data::Session and modify it. But why would
> we do that?
> 
> Isn't the "lib trick" already in place anyway?

I am wrong.
The existing "lib trick" is copying our 'lib' dir content to /usr/share/koha/lib

but we need yamlxs.pm in /usr/share/perl5/Data/Session/Serialize/
Comment 68 Jonathan Druart 2023-05-04 10:08:12 UTC
Created attachment 150633 [details] [review]
Bug 17427: Remove more occurrences of CGI::Session
Comment 69 Jonathan Druart 2023-05-04 10:08:16 UTC
Created attachment 150634 [details] [review]
Bug 17427: Remove SessionStorage mock from tests
Comment 70 Jonathan Druart 2023-05-04 10:08:20 UTC
Created attachment 150635 [details] [review]
Bug 17427: Embed Data::Session

Test plan:
Install libdata-session-perl and make sure our lib has priority
over the installed version
Comment 71 Jonathan Druart 2023-05-04 10:08:24 UTC
Created attachment 150636 [details] [review]
Bug 17427: Remove confusion between session id and param id

FIXME Certainly more work needed here
Comment 72 Jonathan Druart 2023-05-04 10:08:28 UTC
Created attachment 150637 [details] [review]
Bug 17427: HACKY - force new session

Look at Data::Session->new, it's calling $self -> load_session;
Then look at load_session then user_id
=> we are retrieving the ID from the cookie

But in our Auth.t tests we are mocking CGI->cookie to always return the
previous CGISESSID

We should certainly adjust the test here, and remove this patch.

194         # Note: We can test return values from checkauth here since we mocked the safe_exit after the Redirect 303
195         is( $return[0], $patron2->userid, 'Login of patron2 approved' );
196         isnt( $return[2], $sessionID, 'Did not return previous session ID' );
197         ok( $return[2], 'New session ID not empty' );
Comment 73 Jonathan Druart 2023-05-04 10:08:31 UTC
Created attachment 150638 [details] [review]
Bug 17427: Retrieve sessionID from HTTP_COOKIE

To make some tests pass (t/db_dependent/Auth.t) and mimick what did
CGI::Session, but that is certainly useless and the tests should be
adjusted.
Comment 74 Jonathan Druart 2023-05-04 10:08:35 UTC
Created attachment 150639 [details] [review]
Bug 17427: Fix id vs userid in tests
Comment 75 Jonathan Druart 2023-05-04 10:08:49 UTC
still 2 tests failing

/kohadevbox/koha/t/db_dependent/Auth.t .. 12/17                                                                                                                                                                      
    #   Failed test at /kohadevbox/koha/t/db_dependent/Auth.t line 840.                                   
    #          got: 'anon'                                                                                
    #     expected: 'expired'                                                                             
                                                                                                          
    #   Failed test at /kohadevbox/koha/t/db_dependent/Auth.t line 841.
    #          got: 'Data::Session=HASH(0x564c72143498)'                                                  
    #     expected: undef                                                                                 
    # Looks like you failed 2 tests of 31.                                                                
/kohadevbox/koha/t/db_dependent/Auth.t .. 15/17                                                           
#   Failed test 'checkauth & check_cookie_auth'                                                           
#   at /kohadevbox/koha/t/db_dependent/Auth.t line 909.
Comment 76 Jonathan Druart 2023-05-04 10:26:46 UTC
(In reply to Kyle M Hall from comment #66)
> The big blocker is replacing the use of CGI::Session::Find
> (https://metacpan.org/pod/CGI::Session#find(-%5C&code-)) in C4::Auth_with_cas

Yes, that's a tricky one. And really ugly. First the method is experimental, and then we are looping over all the sessions to retrieve the corresponding one... outch!

We could eventually generate the session ID from the CAS ticket (using Koha::Encryption for instance?). Would that work? I am not familiar with CAS!
Comment 77 Jonathan Druart 2023-05-04 10:27:40 UTC
I am done for now, please help (review, feedback, test, see last commit messages)!
Comment 78 Kyle M Hall 2023-05-04 13:53:19 UTC
After applying the patches and running reset_all in ktd, I get the following error when I try to log in:

Data::Session::Driver::mysql::store(): DBI Exception: DBD::mysql::db do failed: Data too long for column 'id' at row 1 at /kohadevbox/koha/lib/Data/Session.pm line 262
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77
Comment 79 Jonathan Druart 2023-05-10 06:42:12 UTC
Created attachment 150926 [details] [review]
Bug 17427: Remove SessionStorage mock from tests
Comment 80 Jonathan Druart 2023-05-10 06:42:17 UTC
Created attachment 150927 [details] [review]
Bug 17427: Embed Data::Session

Test plan:
Install libdata-session-perl and make sure our lib has priority
over the installed version
Comment 81 Jonathan Druart 2023-05-10 06:42:20 UTC
Created attachment 150928 [details] [review]
Bug 17427: Remove confusion between session id and param id

FIXME Certainly more work needed here
Comment 82 Jonathan Druart 2023-05-10 06:42:24 UTC
Created attachment 150929 [details] [review]
Bug 17427: HACKY - force new session

Look at Data::Session->new, it's calling $self -> load_session;
Then look at load_session then user_id
=> we are retrieving the ID from the cookie

But in our Auth.t tests we are mocking CGI->cookie to always return the
previous CGISESSID

We should certainly adjust the test here, and remove this patch.

194         # Note: We can test return values from checkauth here since we mocked the safe_exit after the Redirect 303
195         is( $return[0], $patron2->userid, 'Login of patron2 approved' );
196         isnt( $return[2], $sessionID, 'Did not return previous session ID' );
197         ok( $return[2], 'New session ID not empty' );
Comment 83 Jonathan Druart 2023-05-10 06:42:28 UTC
Created attachment 150930 [details] [review]
Bug 17427: Retrieve sessionID from HTTP_COOKIE

To make some tests pass (t/db_dependent/Auth.t) and mimick what did
CGI::Session, but that is certainly useless and the tests should be
adjusted.

Commit modified, added ';' see comment 78
-        if ( $http_cookie && $http_cookie =~ m{CGISESSID=([^,:]*)} ) {
+        if ( $http_cookie && $http_cookie =~ m{CGISESSID=([^,:;]*)} ) {

sessionID was "a035bf508448b8b3ecd1ca23609b90b3; bib_list=; KohaOpacLanguage=; JWT= "
Comment 84 Jonathan Druart 2023-05-10 06:42:31 UTC
Created attachment 150931 [details] [review]
Bug 17427: Fix id vs userid in tests
Comment 85 Jonathan Druart 2023-05-10 06:42:46 UTC
(In reply to Kyle M Hall from comment #78)
> After applying the patches and running reset_all in ktd, I get the following
> error when I try to log in:
> 
> Data::Session::Driver::mysql::store(): DBI Exception: DBD::mysql::db do
> failed: Data too long for column 'id' at row 1 at
> /kohadevbox/koha/lib/Data/Session.pm line 262
>  at /usr/share/perl5/DBIx/Class/Exception.pm line 77

Fixed in "Retrieve sessionID from HTTP_COOKIE"

Patches rebased against master.
Comment 86 Jonathan Druart 2023-05-10 06:58:54 UTC
(In reply to Jonathan Druart from comment #73)
> Created attachment 150638 [details] [review] [review]
> Bug 17427: Retrieve sessionID from HTTP_COOKIE
> 
> To make some tests pass (t/db_dependent/Auth.t) and mimick what did
> CGI::Session, but that is certainly useless and the tests should be
> adjusted.

This patch fixes some tests but make the sessionID handling very wrong. We always end up with the same sessionID (even after logout, change user, etc.)

I think we should revert it and remove the related tests.
Comment 87 Jonathan Druart 2023-05-10 07:00:00 UTC
(In reply to Mason James from comment #37)
> hi joubu
> i've uploaded a libdata-session-perl pkg to koha-staging/dev, for testing

If we are going with the approach we have now we won't need it.
Comment 88 David Cook 2023-06-21 23:45:46 UTC
(In reply to Jonathan Druart from comment #80)
> Created attachment 150927 [details] [review] [review]
> Bug 17427: Embed Data::Session
> 
> Test plan:
> Install libdata-session-perl and make sure our lib has priority
> over the installed version

An alternative could be to contribute Data::Session::Serialize::yamlxs. Ron seems pretty responsive on Github, and it looks like Mason has contributed a couple patches in Feb 2023.
Comment 89 David Cook 2023-06-21 23:47:37 UTC
What's outstanding on this one?
Comment 90 Jonathan Druart 2023-06-22 04:45:46 UTC
(In reply to David Cook from comment #89)
> What's outstanding on this one?

Read the commit messages.
Comment 91 David Cook 2023-06-22 05:24:02 UTC
(In reply to Jonathan Druart from comment #90)
> (In reply to David Cook from comment #89)
> > What's outstanding on this one?
> 
> Read the commit messages.

I did and I couldn't make sense of them.
Comment 92 Jonathan Druart 2023-07-04 15:21:30 UTC
(In reply to David Cook from comment #91)
> (In reply to Jonathan Druart from comment #90)
> > (In reply to David Cook from comment #89)
> > > What's outstanding on this one?
> > 
> > Read the commit messages.
> 
> I did and I couldn't make sense of them.

Ok, trying to be more verbose then.

1. For discussion, waiting for review and feedback:
(In reply to Jonathan Druart from comment #82)
> Created attachment 150929 [details] [review] [review]
> Bug 17427: HACKY - force new session
> 
> Look at Data::Session->new, it's calling $self -> load_session;
> Then look at load_session then user_id
> => we are retrieving the ID from the cookie
> 
> But in our Auth.t tests we are mocking CGI->cookie to always return the
> previous CGISESSID
> 
> We should certainly adjust the test here, and remove this patch.
> 
> 194         # Note: We can test return values from checkauth here since we
> mocked the safe_exit after the Redirect 303
> 195         is( $return[0], $patron2->userid, 'Login of patron2 approved' );
> 196         isnt( $return[2], $sessionID, 'Did not return previous session
> ID' );
> 197         ok( $return[2], 'New session ID not empty' );

What do you think about this, should we adjust the test?

2. The behaviour is buggy, see the following comment:

(In reply to Jonathan Druart from comment #86)
> (In reply to Jonathan Druart from comment #73)
> > Created attachment 150638 [details] [review] [review] [review]
> > Bug 17427: Retrieve sessionID from HTTP_COOKIE
> > 
> > To make some tests pass (t/db_dependent/Auth.t) and mimick what did
> > CGI::Session, but that is certainly useless and the tests should be
> > adjusted.
> 
> This patch fixes some tests but make the sessionID handling very wrong. We
> always end up with the same sessionID (even after logout, change user, etc.)
> 
> I think we should revert it and remove the related tests.

Do you agree that we should remove the related tests?

3. This needs to be fully tested, but first we need to fix the failures and bugs highlighted in 1 and 2.
Comment 93 David Cook 2023-07-05 06:00:44 UTC
Thanks, Jonathan. I appreciate that.

I think I'll have to look at the code as it's running, but it won't apply at the moment.

I have some holidays this week and next, so I won't be around heaps, but if you get that rebased then I can take a look.
Comment 94 Jonathan Druart 2023-07-05 08:03:58 UTC
Created attachment 153030 [details] [review]
Bug 17427: Replace CGI::Session with Data::Session

CGI::Session is not maintained for ages and seems quite buggy.
It would be the culprit of random logout problems.

Another issue is the following crazy code in CGI::Session::Driver::DBI

sub DESTROY {
    my $self = shift;

    unless ( defined $self->{Handle} && $self->{Handle} -> ping ) {
        $self->set_error(__PACKAGE__ . '::DESTROY(). Database handle has gone away');
        return;
        }

    unless ( $self->{Handle}->{AutoCommit} ) {
        $self->{Handle}->commit;
    }
    if ( $self->{_disconnect} ) {
        $self->{Handle}->disconnect;
    }
}

If AutoCommit is off, it will commit anyway...

It causes some tests to fail (api) if SessionStorage is set to MySQL

Note that PostgreSQL is not supported yet so no need to implement it.

Test plan:
0/ The dependencies are not packaged for debian so far, so install it
via cpan
1/ Test the 3 different values for SessionStorage
2/ Using different browsers (or sessions) login into Koha with different
users. Naviguate => you should not be logged out
3/ Set SessionStorage to Memcached, stop memcached make sure that it
defaults to file.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 95 Jonathan Druart 2023-07-05 08:04:03 UTC
Created attachment 153031 [details] [review]
Bug 17427: Use JSON serialization

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 96 Jonathan Druart 2023-07-05 08:04:07 UTC
Created attachment 153032 [details] [review]
Bug 17427: Use our own YAML::XS serializer

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 97 Jonathan Druart 2023-07-05 08:04:11 UTC
Created attachment 153033 [details] [review]
Bug 17427: Remove use lines for CGI::Session from Auth.pm

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 98 Jonathan Druart 2023-07-05 08:04:15 UTC
Created attachment 153034 [details] [review]
Bug 17427: Remove more occurrences of CGI::Session
Comment 99 Jonathan Druart 2023-07-05 08:04:19 UTC
Created attachment 153035 [details] [review]
Bug 17427: Remove SessionStorage mock from tests
Comment 100 Jonathan Druart 2023-07-05 08:04:24 UTC
Created attachment 153036 [details] [review]
Bug 17427: Embed Data::Session

Test plan:
Install libdata-session-perl and make sure our lib has priority
over the installed version
Comment 101 Jonathan Druart 2023-07-05 08:04:28 UTC
Created attachment 153037 [details] [review]
Bug 17427: Remove confusion between session id and param id

FIXME Certainly more work needed here
Comment 102 Jonathan Druart 2023-07-05 08:04:32 UTC
Created attachment 153038 [details] [review]
Bug 17427: HACKY - force new session

Look at Data::Session->new, it's calling $self -> load_session;
Then look at load_session then user_id
=> we are retrieving the ID from the cookie

But in our Auth.t tests we are mocking CGI->cookie to always return the
previous CGISESSID

We should certainly adjust the test here, and remove this patch.

194         # Note: We can test return values from checkauth here since we mocked the safe_exit after the Redirect 303
195         is( $return[0], $patron2->userid, 'Login of patron2 approved' );
196         isnt( $return[2], $sessionID, 'Did not return previous session ID' );
197         ok( $return[2], 'New session ID not empty' );
Comment 103 Jonathan Druart 2023-07-05 08:04:36 UTC
Created attachment 153039 [details] [review]
Bug 17427: Retrieve sessionID from HTTP_COOKIE

To make some tests pass (t/db_dependent/Auth.t) and mimick what did
CGI::Session, but that is certainly useless and the tests should be
adjusted.

Commit modified, added ';' see comment 78
-        if ( $http_cookie && $http_cookie =~ m{CGISESSID=([^,:]*)} ) {
+        if ( $http_cookie && $http_cookie =~ m{CGISESSID=([^,:;]*)} ) {

sessionID was "a035bf508448b8b3ecd1ca23609b90b3; bib_list=; KohaOpacLanguage=; JWT= "
Comment 104 Jonathan Druart 2023-07-05 08:04:40 UTC
Created attachment 153040 [details] [review]
Bug 17427: Fix id vs userid in tests
Comment 105 Jonathan Druart 2023-07-05 08:11:26 UTC
(In reply to Jonathan Druart from comment #76)
> (In reply to Kyle M Hall from comment #66)
> > The big blocker is replacing the use of CGI::Session::Find
> > (https://metacpan.org/pod/CGI::Session#find(-%5C&code-)) in C4::Auth_with_cas
> 
> Yes, that's a tricky one. And really ugly. First the method is experimental,
> and then we are looping over all the sessions to retrieve the corresponding
> one... outch!
> 
> We could eventually generate the session ID from the CAS ticket (using
> Koha::Encryption for instance?). Would that work? I am not familiar with CAS!

And I forgot this one, it is a blocker!
Comment 106 Jonathan Druart 2023-07-05 08:31:03 UTC
Hum, I had to install the following packages:
  apt install libautovivification-perl libhash-fieldhash-perl libfile-slurper-perl
Comment 107 Jonathan Druart 2023-07-05 08:31:55 UTC
(In reply to Jonathan Druart from comment #106)
> Hum, I had to install the following packages:
>   apt install libautovivification-perl libhash-fieldhash-perl
> libfile-slurper-perl

Those are libdata-session-perl deps.