Bug #75896 for Plack-Middleware-Auth-Form: Cookie Expiry Date not set for "remember" session

Tue Mar 20 02:24:22 2012 cpan [...] sourcentral.org - Ticket created

Subject:

Cookie Expiry Date not set for "remember" session

Only the first time the plack_session cookie is returned to the browser the expiry date is set. Every subsequent cookie returned is a session cookie (without expiry date set). Since the "remember" marker is removed from the session data there is no chance to set the expiry date again on the cookie if it is sent again. My proposed fix is attached: - remove deletion of remember attribute after setting expiry date on cookie - setting the remember attribute on every valid login to the actual state of the checkbox - remove remember attribute on logout

Subject:

auth-form-expiry.patch

--- /usr/share/perl5/vendor_perl/Plack/Middleware/Auth/Form.pm 2011-08-04 21:59:06.000000000 +0200 +++ /usr/share/perl5/vendor_perl/Plack/Middleware/Auth/Form.pm.new 2012-03-20 06:50:23.974449572 +0100 @@ -31,7 +31,6 @@ if( $path ne '/logout' ){ $env->{'psgix.session.options'}{expires} = time + 60 * 60 * 24 * 30; } - delete $env->{'psgix.session'}{remember}; } if( $path eq '/login' ){ @@ -75,7 +74,7 @@ } if( !$login_error ){ $env->{'psgix.session'}{user_id} = $user_id; - $env->{'psgix.session'}{remember} = 1 if $params->get( 'remember' ); + $env->{'psgix.session'}{remember} = ($params->get( 'remember' ) ? 1 : 0); my $redir_to = delete $env->{'psgix.session'}{redir_to}; $redir_to = '/' if URI->new( $redir_to )->path eq $env->{PATH_INFO}; @@ -130,6 +129,7 @@ my($self, $env) = @_; if( $env->{REQUEST_METHOD} eq 'POST' ){ delete $env->{'psgix.session'}{user_id}; + delete $env->{'psgix.session'}{remember}; } return [ 303,

Tue Mar 20 03:32:01 2012 cpan [...] sourcentral.org - Status changed from 'new' to 'open'

Tue Mar 20 03:46:16 2012 cpan [...] sourcentral.org - Correspondence added

While fixing the tests I found out that I changed the remember attribute correctly within the module but not with the tests. For this I fixed the tests. This time the proposed patch includes the fixes for the changes with login on logged in account as well.

Subject:

auth-form-test.patch

diff -Nur Plack-Middleware-Auth-Form-0.010.orig/t/unit.t Plack-Middleware-Auth-Form-0.010/t/unit.t --- Plack-Middleware-Auth-Form-0.010.orig/t/unit.t 2011-08-04 21:59:06.000000000 +0200 +++ Plack-Middleware-Auth-Form-0.010/t/unit.t 2012-03-20 08:29:01.140358155 +0100 @@ -34,13 +34,23 @@ is( $res->[1][1], '/landing_page', 'Redirection after login' ) or warn Dumper($res); is( $post_req->{'psgix.session'}{user_id}, 'joe', 'Username saved in the session' ); is( $post_req->{'psgix.session'}{redir_to}, undef, 'redir_to removed after usage' ); -ok( !exists $post_req->{'psgix.session'}{remember} ); +is( $post_req->{'psgix.session'}{remember}, 0, 'remember not set' ); +$post_req->{PATH_INFO} = '/logout'; +$res = $middleware->call( $post_req ); +ok( !exists( $post_req->{'psgix.session'}{user_id} ), 'User logged out' ); + +$post_req->{PATH_INFO} = '/login'; $post_req->{'psgix.session'}{redir_to} = '/new_landing_page'; $middleware = Plack::Middleware::Auth::Form->new( authenticator => sub { { user_id => 1 } } ); $res = $middleware->call( $post_req ); is( $post_req->{'psgix.session'}{user_id}, '1', 'User id saved in the session' ); +$post_req->{PATH_INFO} = '/logout'; +$res = $middleware->call( $post_req ); +ok( !exists( $post_req->{'psgix.session'}{user_id} ), 'User logged out' ); + +$post_req->{PATH_INFO} = '/login'; $middleware = Plack::Middleware::Auth::Form->new( authenticator => sub { 0 } ); $res = $middleware->call( $post_req ); like( join( '', @{ $res->[2] } ), qr/error.*form id="login_form"/, 'login form for login error' ); diff -Nur Plack-Middleware-Auth-Form-0.010.orig/t/wrap_body_override.t Plack-Middleware-Auth-Form-0.010/t/wrap_body_override.t --- Plack-Middleware-Auth-Form-0.010.orig/t/wrap_body_override.t 2011-08-04 21:59:06.000000000 +0200 +++ Plack-Middleware-Auth-Form-0.010/t/wrap_body_override.t 2012-03-20 08:30:16.960372929 +0100 @@ -48,9 +48,13 @@ is( $res->[1][1], '/landing_page', 'Redirection after login' ) or warn Dumper($res); is( $post_req->{'psgix.session'}{user_id}, 'joe', 'Username saved in the session' ); is( $post_req->{'psgix.session'}{redir_to}, undef, 'redir_to removed after usage' ); -ok( !exists $post_req->{'psgix.session'}{remember} ); +is( $post_req->{'psgix.session'}{remember}, 0, 'remember not set' ); +$post_req->{PATH_INFO} = '/logout'; +$res = $middleware->call( $post_req ); +ok( !exists( $post_req->{'psgix.session'}{user_id} ), 'User logged out' ); +$post_req->{PATH_INFO} = '/login'; $middleware = $class->new( authenticator => sub { 0 } ); $res = $middleware->call( $post_req ); $html = join '', @{ $res->[2] };

Tue Mar 20 03:46:18 2012 cpan [...] sourcentral.org - Status changed from 'open' to 'new'

Thu Mar 22 01:59:11 2012 zby [...] cpan.org - Correspondence added

Thanks for the report! It got me thinking - somehow I expected the Session middleware not to send the cookie when there were no changes. I think this is how other systems work. Do you happen to know why it does that?

Thu Mar 22 01:59:12 2012 The RT System itself - Status changed from 'new' to 'open'

Thu Mar 22 05:29:40 2012 cpan [...] sourcentral.org - Correspondence added

I see two aspects: 1) An expiring cookie should be updated from time to time to extend it's duration even if the token has not changed. Would be bad if an expiring cookie including the token is dropped during an active session. Even if something is changed in update algorithm of P::M::Session, this requires my fix, even if the hardcoded expiry date might not be hit. 2) An session cookie does not need do be updated until the session token changes. Maybe you are right and we should discuss this with Plack::Middleware::Session Maintainers. They have interesting undocumented features like $options->{no_store} and $options->{change_id}. Any maybe there is a better way to hand of handling of remember to the State::Cookie class itself, including a configurable expiry date. Maybe you can switch the remember logic: delete the expiry date if remember is not set. If remeber is set, the predefined State::Cookie expiry date is used.

Sat Mar 31 03:07:17 2012 zby [...] cpan.org - Correspondence added

I committed your changes in: https://github.com/zby/Plack-Middleware- Auth-Form/commit/650190fef5bea91851617b91cffb9cd02a3149cf and I added one test for this in: https://github.com/zby/Plack-Middleware-Auth- Form/commit/35191ba77013d639c0bf9c1a03ebebc9bb2eba47 I really think that the Session middleware should not reset Expire without request - but since they do we'll need to work around that. I think I'll make the release next week - there are already many changes there. I am thinking about changing the name to WebPrototypes::LoginForm.

Fri Apr 06 17:13:16 2012 zby [...] cpan.org - Correspondence added

The fix above is published in 0.011

Fri Apr 06 17:13:17 2012 zby [...] cpan.org - Status changed from 'open' to 'resolved'