Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rememberme: Set cookie lifetime iso expire fixing the remember me fea… #1088

Conversation

@fingerspitziluijks
Copy link
Contributor

@fingerspitziluijks fingerspitziluijks commented Apr 1, 2019

rememberme: Set cookie lifetime iso expire fixing the remember me feature and respective errors.

Currently the remember me functionality does not work correctly and using it results in severable undefined index errors as the expire cookie parameter is passed along the SessionHandlerPHP which does not accept this one. Using lifetime instead of expire, effectively doing the same thing, this can be fixed in a pretty simple way. Next to that the params given to the session handler are merged with the current ones before given to the session handler instead of after.

…ture and respective errors.

Currently the remember me functionality does not work correctly and using it results in severable undefined index errors as the expire cookie parameter is passed along the SessionHandlerPHP which does not accept this one. Using lifetime instead of expire, effectively doing the same thing, this can be fixed in a pretty simple way. Next to that the params given to the session handler are merged with the current ones before given to the session handler instead of after.
@tvdijen
tvdijen approved these changes Apr 1, 2019
Copy link
Member

@tvdijen tvdijen left a comment

Good catch!

@tvdijen tvdijen added this to the 2.0 milestone Apr 1, 2019
@tvdijen
Copy link
Member

@tvdijen tvdijen commented Apr 1, 2019

Since this will break the API, it will have to wait until 2.0.. Until then we can fix the description in config.php

@fingerspitziluijks
Copy link
Contributor Author

@fingerspitziluijks fingerspitziluijks commented Apr 3, 2019

Thanks and excellent, love to see this feature working (again)!

@tvdijen tvdijen added this to In progress in 2.0 release via automation Nov 22, 2019
@codecov
Copy link

@codecov codecov bot commented Jun 6, 2020

Codecov Report

Merging #1088 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master    #1088      +/-   ##
============================================
- Coverage     38.36%   38.36%   -0.01%     
- Complexity     3385     3386       +1     
============================================
  Files           128      128              
  Lines          9633     9634       +1     
============================================
  Hits           3696     3696              
- Misses         5937     5938       +1     
@tvdijen tvdijen force-pushed the simplesamlphp:master branch 3 times, most recently from 1c686ab to eb20457 Aug 17, 2020
@thijskh
Copy link
Member

@thijskh thijskh commented Aug 23, 2020

We can probably make a good heuristic of whether someone is calling the method with expire or with lifetime.

if($lifetime > 1500000000) {
   // expire time was passed

However we already have too much legacy and compatibility to deal with. So I propose to do a hard changeover for 2.0 (document it in upgrade notes). We can add the if statement above and hve it trigger a warning in the logs: you are likely calling this wrong. To be helpful. But not fall back to old/broken behaviour.

We can merge this now to master?

@tvdijen
Copy link
Member

@tvdijen tvdijen commented Aug 23, 2020

Perhaps we could use a unit test, but other than that, all lights are green!

Copy link
Member

@thijskh thijskh left a comment

I've reviewed it and use outside core SSP that would break seems not so likely or would be quickly fixed. So I've opted not to add more 'bc' code lines. Did add some unit tests though, and merged. Thanks!

@thijskh thijskh closed this Aug 25, 2020
2.0 release automation moved this from In progress to Done Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
2.0 release
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants