Remember me feature and check session function

[GoogleCodeExporter]

Purpose of code changes on this branch:

Add "remember me" feature for IdP sessions (authsource part not yet 
implemented). Add configuration option for custom session check function. Add 
generic function for setting cookies.

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 29 Aug 2013 at 1:01

Attachments:

• remember.me+check.session.patch

[GoogleCodeExporter]

I'm sorry, but I don't really like this patch. It introduces too many unrelated 
features in one big change.

Some comments for each feature:
1. Custom session check function: I don't really see this one being used. 
Anyone in a position to grab the session cookies can also copy every other 
request header at the same time. The only entry in your example that looks like 
it can be of any use is the REMOTE_ADDR check, but that one is generally 
something you don't want, since clients tend to change IP address frequently. 
With IPv6 there is also the problem of the user randomly jumping between IPv6 
and IPv4 with some web browsers.

2. Generic function for cookie setting: What precicely does this function add 
over just calling setcookie()? The only thing I see is the few lines of error 
checking.

3. "Remember me" for IdP sessions: I am having difficulty seeing exactly what 
the changes for this one is. Do you intend to keep the IdP session alive for as 
long as the user is "remembered" for? Will this bypass the entire 
authentication source, or will the authentication source be expected to show 
the username and a password placeholder, allowing the user to just confirm the 
login? What lifetime will you give to SP sessions?

Original comment by olavmrk@gmail.com on 2 Sep 2013 at 11:56

[GoogleCodeExporter]

All changes are related to remember me feature. But yes, I could split it into 
multiple patches.

1. Mainly we are using REMOTE_ADDR with GeoIP ASN and coutry, which I just 
mention in example, but didn't put in the example because geoip php module 
should be installed.

2. Function replaces current SimpleSAML_SessionHandler::setCookie(), but it's 
static, with some useful common checks and definitions.

3. This is "remember me" feature for IdP which is common on popular services 
but under different names, e.g. google/gmail "Stay signed in", facebook "Keep 
me logged in", twitter "Remember me". Basically it just extends session 
duration on IdP if user checks "remember me" option when logging in.

Original comment by comel...@gmail.com on 2 Sep 2013 at 12:48

[GoogleCodeExporter]

But the changes are not, strictly speaking, part of the feature. Instead they 
do preparatory work, to make the feature easier to implement. Splitting them 
into separate patches make it easier to see what is done.

1. That makes more sense now. I think an example that doesn't promote bad 
practices would be better. It wouldn't need to be a complete example. A simple 
example could just be something like:

    $country = get_country_from_addr($_SERVER['REMOTE_ADDR']); //TODO: Replace get_country_from_addr() with a function to determine country.

2. I don't see a function being static as a goal in itself. As long as the 
function is only used from the session code, it may as well live in the session 
handler. The reason it is in the session handler is that cookie setting is tied 
to the session handler, e.g. because the PHP session handler has a separate 
session cookie configuration.

3. In that case I suggest a simpler solution:
  - Don't touch the session cookie at all (outside of setting it with a long lifetime by default). The AuthToken cookie is already set every time authentication changes, so simply extend the lifetime of that one when the user chooses to remember the login.

Original comment by olavmrk@gmail.com on 3 Sep 2013 at 9:33

[GoogleCodeExporter]

OK, I'll split it to multiple patches/issues.

1. OK.

2. I'we moved it to Utilities with intention to replace most direct calls to 
setcookie where e.g. secure flag is set but the HTTPS verification is not 
conducted.

3. Currently AuthToken cookie parameters are the same as for the session 
cookie. What would, in that case, then be the default value (with unchecked 
remember me) for the AuthToken cookie lifetime? Should we introduce another 
'session.authtoken.cookielifetime' configuration option? Or if 
'session.rememberme.enable' is enabled we should use 
'session.rememberme.cookielifetime' for session lifetime instead of 
'session.cookie.lifetime', and use 'session.cookie.lifetime' as default 
AuthToken cookie lifetime?

Original comment by comel...@gmail.com on 3 Sep 2013 at 11:29

[GoogleCodeExporter]

If rememberme is disabled, we should maintain the current behavior. If it is 
enabled, the AuthToken cookie should have a lifetime of 0 (i.e. current browser 
session) unless the user chooses to remember the login. If the user chooses to 
remember the login, the cookie should have a lifetime of 
session.rememberme.cookielifetime.

A warning or error should probably be logged if session.cookie.lifetime or 
session.duration is less that session.rememberme.cookielifetime.

(I would also consider renaming it to session.rememberme.lifetime.)

Original comment by olavmrk@gmail.com on 3 Sep 2013 at 1:25

[GoogleCodeExporter]

Regarding setcookie, if the plan is to move all the callers of setcookie to use 
the new function, that should probably be a part of the commit. (I.e. add the 
new function, and change all calls to setcookie to use that function.)

Original comment by olavmrk@gmail.com on 3 Sep 2013 at 1:27

[GoogleCodeExporter]

I'm closing this as invalid.

Original comment by comel...@gmail.com on 10 Sep 2013 at 12:20

• Changed state: Invalid