plp050452 / simplesamlphp

Automatically exported from code.google.com/p/simplesamlphp
Other
0 stars 0 forks source link

Remember me feature #571

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
Purpose of code changes on this branch:

Remember me feature for IdP sessions. I've created three patches for this:

- 1-get-session-cookie-name.patch
Adds getSessionCookieName() function to SimpleSAML_SessionHandler class, used 
in patch 2. I've decided to update session cookie also, for consistency with 
AuthToken cookie, and for features like session extending introduced in patch 3.

- 2-rememberme.patch
Actual remember me feature. I'm not logging warning if 
session.rememberme.lifetime is less than session.duration because it's actually 
meant to have session.rememberme.lifetime set to value greater or equal to 
session.duration, to be able to control actual session duration for already 
authenticated users after remember me feature is enabled.

- 3-extend-idp-session.patch
Extend IdP session duration authentication processor. Some already asked for 
similar feature on mailing list 
(https://groups.google.com/d/msg/simplesamlphp/m1z8NUfo45s/iqOrnFh7ic4J), and I 
thought it could be useful.

When reviewing my code changes, please focus on:

After the review, I'll merge this branch into:
/trunk

Original issue reported on code.google.com by comel...@gmail.com on 10 Sep 2013 at 12:40

Attachments:

GoogleCodeExporter commented 8 years ago
In the second patch, you have several calls like 
«self::updateSessionCookies». Since this function isn't static, it should be 
$this->updateSessionCookies instead.

In the second patch, I think setRememberMeExpire() should also update the 
session cookie lifetime. That would make the third patch less dependant on the 
internals of the session object.

In the third patch, you are also calling setSessionDuration(). That function 
shouldn't be used any more. It is only present because of 
backwardscompatibility. (I guess I should mark it as deprecated somehow).

Instead, you should update the 'Expire' value for the authentication source the 
current authproc filter is using as base data. The problem is identifying that 
authentication source. It looks like you will have to add it to the processing 
state from the IdP.

For the third patch, you should probably include a bit of documentation 
somewhere. Actually, you may want to write a document showing how to take 
advantage of this entire feature :)

Btw.: If you aren't using it to bould your patch series already, you should 
have a look at «git format-patch».

Original comment by olavmrk@gmail.com on 12 Sep 2013 at 9:29

GoogleCodeExporter commented 8 years ago
Here are updated patches.

1. Updated for trunk.

2. Updated updateSessionCookies() and added setAuthorityExpire().

3. Updated for new updateSessionCookies() and setAuthorityExpire().

We are using somehow specific workflow for this project, so we are not using 
git format-patch. Thanks for the tip.

Original comment by comel...@gmail.com on 12 Sep 2013 at 3:43

Attachments:

GoogleCodeExporter commented 8 years ago
Only a couple of remarks against the last patch:

- You refer to sspmod_common_Util::getGlobalConfig() , but that one doesn't 
exist.
- Maybe this patch is more of an example? In that case it probably belongs in 
exampleauth?

Other than that, these patches look good.

Original comment by olavmrk@gmail.com on 13 Sep 2013 at 10:55

GoogleCodeExporter commented 8 years ago
Authentication processor was at first in custom module which is modified for 
patch 3 and sspmod_common_Util::getGlobalConfig is leftover. It's fixed now. 
Patches are committed as r3275, r3276 and r3277. Thanks!

Original comment by comel...@gmail.com on 13 Sep 2013 at 11:16