Bug 17427 - Replace CGI::Session with Data::Session
Summary: Replace CGI::Session with Data::Session
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords: dependency
Depends on:
Blocks:
 
Reported: 2016-10-11 13:34 UTC by Jonathan Druart
Modified: 2017-07-26 17:34 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
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

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.