Opened 6 years ago

Closed 6 years ago

#1718 closed new feature (fixed)

Remember Me function

Reported by: jsfan Owned by: jsfan
Priority: major Milestone: 1.1
Component: BW General Keywords: Remember Me, Login
Cc: crumbking

Description (last modified by jsfan)

When logging in, a user is offered to make the session persistent beyond normal session expiry.

When checking if user is logged in, expired sessions are resumed if user is set to persistent log in.

Change History (29)

comment:1 Changed 6 years ago by jsfan

  • Description modified (diff)
  • Milestone Future deleted
  • Owner set to jsfan
  • Reporter changed from TimLoal to jsfan
  • Status changed from new to assigned

comment:2 Changed 6 years ago by jsfan

This requires a cookie with a shared secret. It could be just the user name and some kind of secret salt but maybe it would be better to have something that changes with time.

A possible approach would be http://jaspan.com/improved_persistent_login_cookie_best_practice

Last edited 6 years ago by jsfan (previous) (diff)

comment:3 Changed 6 years ago by jsfan

  • Milestone set to 1.1

We will also need some kind of DB update mechanism because this will introduce new fields. Currently, DB updates are handled via htdocs/bw/lib/dbupdate.php but that one is quite ugly. I'll have a look at improving that. :)

Last edited 6 years ago by jsfan (previous) (diff)

comment:4 Changed 6 years ago by jsfan

  • Milestone changed from 1.1 to 1.0-sec

comment:5 Changed 6 years ago by planetcruiser

  • Milestone changed from 1.1 to unassigned

jsfan will look into this later, possibly for 1.2, milestone 1.1 should be a quick interim release before osm

comment:6 Changed 6 years ago by jsfan

  • Milestone changed from unassigned to 1.1

I have pushed a patch for this. Not deploying on alpha just, yet. Maybe someone wants to test locally?

https://gitorious.org/bewelcome/rox/commit/bc7026898cc40200271ba01f59fffb28cd6b5865/diffs/821ca78e3cb7a5dcd0f296a41e15986f9fa47418

You will have to enable "stay_logged_in" in the "env" section of your rox_local.ini. You will also have to run

ALTER TABLE members ADD `SeriesToken` CHAR(32);
ALTER TABLE members ADD `AuthToken` CHAR(32);
Last edited 6 years ago by jsfan (previous) (diff)

comment:7 Changed 6 years ago by jsfan

There is definitely one thing that isn't working which is the warning for hijacked sessions.

comment:9 Changed 6 years ago by jsfan

There is now a warning if the session was hijacked AND the first page accessed in the original session is a "members only" page.

https://gitorious.org/bewelcome/rox/commit/5270008ee5d2ff68520830c0dd2ab8a2d753975d/diffs/16cbc5c288d2ceaa464291549a1fb56a9732037c

comment:10 Changed 6 years ago by planetcruiser

just tested. logins are not remembered if different devices or browsers are used. the last login will always win. i would be ok with leaving this as it is for now, but a proper solution would be to remember sessions on each device/browser of course.

session hijacking still needs testing. instructions (copied from chat):

(13:43:02) jsfan: sniff a request and copy it to a text file
(13:43:06) jsfan: delete the session file
(13:43:20) jsfan: replay the last request with netcat and the file
(13:43:29) jsfan: delete the session file again
(13:43:36) jsfan: access a page in your browser again
(13:44:15) jsfan: the second time you use the browser, the auth token will have been renewed by the replay without session. as your browser sends a wrong one, it is recognised as a hijacking.
(15:39:20) jsfan: nc "<server>" 80 < bla.txt

comment:11 Changed 6 years ago by jsfan

The patch

https://gitorious.org/bewelcome/rox/commit/16cbc5c288d2ceaa464291549a1fb56a9732037c/diffs/3de5c7dc670de83d50c81f40e00e701ea08913e7

should now allow for several concurrent sessions for one user (login from more than one device).

It also improves the warning procedure when session is hijacked.

Instead of adding two columns (SQL in earlier comment), you will now need a new table.

CREATE TABLE `member_sessions` (
        `tstamp` int(11) unsigned DEFAULT '0' NOT NULL,
        `MemberId` int(11) DEFAULT NULL,
        `SeriesToken` char(32) DEFAULT NULL,
        `AuthToken` char(32) DEFAULT NULL
) ENGINE=MyISAM DEFAULT CHARSET=utf8;

comment:12 Changed 6 years ago by jsfan

Deployed on alpha.

comment:13 Changed 6 years ago by planetcruiser

a couple of things on naming conventions for mysql:

  • could we rename "tstamp" to "created" (or "updated", not sure what its function is) and have it type "timestamp" (maybe even auto-update) to be consistent with other tables and to be able to use some mysql goodies in ts calculation if we need to?
  • we usually use "IdMember", which is the wrong way around i know, but a de-facto convention for rox
  • the table should be members_ (with an "s"). again, it's unusual and i wouldn't do that when setting up a database, but it's the convention currently in place

comment:14 Changed 6 years ago by planetcruiser

code review:

comment:15 follow-up: Changed 6 years ago by planetcruiser

added translations for StayLoggedIn? and StayLoggedIn_SecurityHint in EN and DE. StayLoggedIn_SecurityHint needs the number of days passed to it now.

the hint looks a little bit long, and everyone knows this function from other websites, so i suggest to unclutter by making it only visible as a tool tip on StayLoggedIn? (title attribute for span or div) and maybe the checkbox itself.

you might want to give the checkbox a "vertical-align: bottom" to make it align nicely with the text afterwards.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 6 years ago by jsfan

  • Cc crumbking added

Replying to planetcruiser:

added translations for StayLoggedIn? and StayLoggedIn_SecurityHint in EN and DE. StayLoggedIn_SecurityHint needs the number of days passed to it now.

I kind of left that out intentionally... ;) This is hardly ever mentioned on other sites and most users don't care much. Basically, all that matters if 2 weeks is long enough or if there will be plenty of users coming back after 2 months expecting to still be logged in and complain when they find that they aren't.

the hint looks a little bit long, and everyone knows this function from other websites, so i suggest to unclutter by making it only visible as a tool tip on StayLoggedIn? (title attribute for span or div) and maybe the checkbox itself.

I only put in the hint after an IRC discussion with crumbking and beatnickgr. I think the people who need this kind of hint are also the people who don't read it. This will be even more of a problem when the hint is moved into the tool tip. I'd make it no more than a "don't tick on shared computers" and leave it where it is.

you might want to give the checkbox a "vertical-align: bottom" to make it align nicely with the text afterwards.

That's a good idea. Changed locally now.

comment:17 in reply to: ↑ 16 Changed 6 years ago by planetcruiser

Replying to jsfan:

I kind of left that out intentionally... ;) This is hardly ever mentioned on other sites and most users don't care much. Basically, all that matters if 2 weeks is long enough or if there will be plenty of users coming back after 2 months expecting to still be logged in and complain when they find that they aren't.

fair enough. although, we have that information, so why not display it. and i'm sure some of our privacy sensitive users will ask. ;)

I only put in the hint after an IRC discussion with crumbking and beatnickgr. I think the people who need this kind of hint are also the people who don't read it. This will be even more of a problem when the hint is moved into the tool tip. I'd make it no more than a "don't tick on shared computers" and leave it where it is.

if you look around the web, no other site has a warning there any more. i +1 less clutter in the interface, so leaving it out completely or putting it into a tooltip (but then ideally with more details, like the 14 days)

comment:18 Changed 6 years ago by jsfan

I've now moved the hint into a tool tip on the wrapping td. It displays the number of days. While I have chucked out the stay_logged_in config setting, I don't like that there is a hard coded number of days in the tool tip now which does not change if the expiry is changed.

Shouldn't we have a config option for the cookie expiry, so that we can also make it longer if e.g. people complain that it's too short? I could then use that setting to generate hints that are always consistent with the actual expiry.

comment:19 follow-up: Changed 6 years ago by jsfan

I have now introduced a setting in "env" called "rememberme_expiry". It is defaulted to 14 days in the code if missing in config but I've also added it to rox_default.ini.

I'm not pushing this, yet, because it breaks backward compatibility (session table now required!). However, the latest changes are trivial.

comment:20 in reply to: ↑ 19 Changed 6 years ago by planetcruiser

Replying to jsfan:

I have now introduced a setting in "env" called "rememberme_expiry". It is defaulted to 14 days in the code if missing in config but I've also added it to rox_default.ini.

yes, it makes total sense to have this in the config of course.

I'm not pushing this, yet, because it breaks backward compatibility (session table now required!). However, the latest changes are trivial.

why not, let's break things! :) it's develop. i think once we finalised the table schema, we can push and send out a mail to the dev list to let devs fix their dbs

comment:21 Changed 6 years ago by jsfan

The database schema of the sessions table should now be consistent with the Rox quasi standard. It now reads

CREATE TABLE `members_sessions` (
        `IdMember` int(11) DEFAULT NULL,
        `SeriesToken` char(32) DEFAULT NULL,
        `AuthToken` char(32) DEFAULT NULL,
        `modified` timestamp DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP
) ENGINE=MyISAM DEFAULT CHARSET=utf8

Also, "stay logged in" functionality is switched on by default with an expiry time of 14 days.

comment:22 follow-up: Changed 6 years ago by planetcruiser

the vertical-align:bottom for the checkbox is still missing

comment:23 Changed 6 years ago by planetcruiser

execute this to get rid of the previously suggested db changes:

ALTER TABLE `members` DROP `SeriesToken`, DROP `AuthToken`;
DROP TABLE IF EXISTS `member_sessions`;

comment:24 Changed 6 years ago by planetcruiser

deployed to alpha.

shouldn't clicking "log out" delete a row in members_sessions? at the moment it just leaves the old session and creates a new one when logging in again with "keep logged in".

bug or feature?

comment:25 Changed 6 years ago by planetcruiser

interesting.. this is what is left in the database after a few logins with different browsers:

mysql> select * from members_sessions;
+----------+----------------------------------+----------------------------------+---------------------+
| IdMember | SeriesToken                      | AuthToken                        | modified            |
+----------+----------------------------------+----------------------------------+---------------------+
|      202 | f50e1c224913d885753564832911ddc8 | 548f866e4aff20cc8b2d5103c30cb6b0 | 2012-09-29 14:46:21 |
|      202 | f50e1c224913d885753564832911ddc8 | 548f866e4aff20cc8b2d5103c30cb6b0 | 2012-09-29 14:46:21 |
|      202 | f50e1c224913d885753564832911ddc8 | 548f866e4aff20cc8b2d5103c30cb6b0 | 2012-09-29 14:46:21 |
|      202 | f50e1c224913d885753564832911ddc8 | 548f866e4aff20cc8b2d5103c30cb6b0 | 2012-09-29 14:46:21 |
|      202 | f50e1c224913d885753564832911ddc8 | 548f866e4aff20cc8b2d5103c30cb6b0 | 2012-09-29 14:46:21 |
|      202 | f50e1c224913d885753564832911ddc8 | 548f866e4aff20cc8b2d5103c30cb6b0 | 2012-09-29 14:46:21 |
|      202 | f50e1c224913d885753564832911ddc8 | 548f866e4aff20cc8b2d5103c30cb6b0 | 2012-09-29 14:46:21 |
|      202 | f50e1c224913d885753564832911ddc8 | 548f866e4aff20cc8b2d5103c30cb6b0 | 2012-09-29 14:46:21 |
|      202 | 9b1a803b81bb740170a29602b485c503 | ece180a465ee796414c5a1548bf74249 | 2012-09-29 14:46:49 |
+----------+----------------------------------+----------------------------------+---------------------+
9 rows in set (0.00 sec)

the tokens and times are the same. i think this was triggered when i clicked "log out".

this is probably also why a new login still cancels the last session in another browser ;)

comment:26 Changed 6 years ago by jsfan

Looks like the cookie was unset too late in the log out process when the member ID had already become unavailable. Fixed now.

Concurrent sessions should not cancel each other. Please test again with this revision.

https://gitorious.org/bewelcome/rox/commit/6d9171a5a0b4f6c07f0599ea10291f2b4ece38b3/diffs/06449ba4d74263d5c3557afbb5e466b7caef6ca5

comment:27 Changed 6 years ago by jsfan

All seems ok to me now.

comment:29 in reply to: ↑ 22 Changed 6 years ago by planetcruiser

  • Resolution set to fixed
  • Status changed from assigned to closed

Replying to planetcruiser:

the vertical-align:bottom for the checkbox is still missing

fixed via https://gitorious.org/bewelcome/rox/commit/ebbf1b228c64d0314d09451b70f6469775304afc ;)

i think we can close this one now.

Note: See TracTickets for help on using tickets.