thoughtbot / clearance

Rails authentication with email & password.
https://thoughtbot.com
MIT License
3.71k stars 457 forks source link

Per-device remember tokens #675

Open derekprior opened 8 years ago

derekprior commented 8 years ago

Currently, each account has a single remember token. Signing out of your account on one device will sign you out on all devices because the single remember token is rotated. This is nice in its simplicity, but is a pain in many contexts with mobile users, shared computers, etc.

Here's a proposal for Clearance 2.0:

  1. User#remember_token is removed in favor of a remember_tokens table.
  2. Each new sign in adds a new entry for the user to the remember_tokens table.
  3. Signing out destroys that remember token only.

We would also add a sessions#index action which lists the current users' existing sessions and allows them to be destroyed remotely.

For the index view to be useful to users, each token would have to be associated to something which might give them some hope of identifying the session. How do you think we should handle this?

My first thought was that when we create a new remember token, we record the IP address and user agent string of the browser and present that in the index. I feel like UA strings are ugly and require some parsing to boil down to something useful and succinct and I'm heistant to do that myself and wary of taking a dependency to do that. Should I be? Similiarly, a raw IP address isn't nearly as helpful as geolocation but I'd need a dependency for that as well, and I think they'd require an external service call. So that's out immediately in my book.

Then I thought that perhaps we should also record a "last_seen_at" field, which would tell us when the token was last active. That alone might be sufficient. But then that's a database write on every request. Is that worth it?

What do you all think of this feature? Do you have any other intricacies you think I should consider? How do you feel about making the information meaningful to end users?

mjankowski commented 8 years ago

I like this feature idea.

I'm tempted to call it scope creep and just say to skip it and keep clearance simple ... but I think wanting to do handle sessions like this is a totally reasonable not-extravagant thing to want in our modern era of smart devices and IoT.

I share your same exact concerns on things like IP/Location tracking, UA-naming, etc ... might be worth looking for prior art in these areas so we can depend on something. I could also see core clearance doing the simplest version of this which might work (just UA strings and IPs?) and providing nice hooks for people to improve that experience ... maybe even provide a clearance-fancy-sessions package which does some of it?

mike-burns commented 8 years ago

To confirm, the use cases are:

?

I don't think we need to GeoIP anything -- just knowing the IP is sufficient for tracking. OTOH -- do we want IP tracking in a DB? Are there laws around that?

I'm on board with your original proposal: each sign in adds to the remember_tokens table; each sign out removes. I didn't follow the limitations around that.

jferris commented 8 years ago

I am in favor of this general idea.

Then I thought that perhaps we should also record a "last_seen_at" field, which would tell us when the token was last active. That alone might be sufficient. But then that's a database write on every request. Is that worth it?

If you use a date instead of a time, it will still provide useful context but only require one database update per day per user.

bdewater commented 8 years ago

I think updating once per day might not be precise enough to determine whether a session was recently active or not. How about writing an update if the last_seen_at field is > 1 minute ago? Or 5?

JoelQ commented 8 years ago

Conditionally updating based on "last_seen_at" means that on each request you are adding one SELECT query as well as a potential UPDATE query. Does it make sense to only persist the date that the session was created, not the last time it was used?

JoelQ commented 8 years ago

For the index view to be useful to users, each token would have to be associated to something which might give them some hope of identifying the session. How do you think we should handle this?

Is it possible to detect the device name? I want to say my bank shows me something fairly identifiable in it's session names.

Alternatively, thoughts on allowing users to name their session when the log in for the first time? (probably pre-filling it with a default)

Is a browser/OS combination good enough when it comes to naming sessions?

Ronak5 commented 8 years ago

Mutli device login is want in our modern era of smart devices is must to have feature.

Clearance can internally handle it using another model which should consists of :-

when i am logged in from android & web , want to logout from web then only that required would be archived.So remember_token for web wont work & give unauthorized

mike-burns commented 8 years ago

@Ronak5 what is stored in device_type? When is it stored?

Ronak5 commented 8 years ago

@mike-burns , we can pass new param at the time of login,its the device type like android , iOS or web.Can prove usefull when we want to send push notifications. Example like when update is using android app, we have to send push notification to iOS(if logged in) only to maintain consistency of data.And when update is done from web , we have to send push notification to both android & iOS.

mike-burns commented 8 years ago

@Ronak5 storing it as Web wouldn't solve two of the three use cases I listed here: https://github.com/thoughtbot/clearance/issues/675#issuecomment-216810109

bdewater commented 8 years ago

Conditionally updating based on "last_seen_at" means that on each request you are adding one SELECT query as well as a potential UPDATE query.

I am not sure I understand where that additional SELECT would come from - I assumed last_seen_at is a column on a remember_token row that you're already querying for every request, so that leaves only a potential UPDATE?

Ronak5 commented 8 years ago

@mike-burns it can sort if you pass different device-token something unique to browser, like combination for browser & ip address

derekprior commented 8 years ago

If we did this, I think what I would opt to do is whatever we can do without additional dependencies. So we'll probably log the ip and user agent and then if you want to make that more usable you can add UA parsing and IP geolocation yourself.

I'm on the fence about tracking last-seen at. I like the idea of tracking it only once/day but I could be convinced we should do it slightly more often or even not at all.

I'm leaning towards making this a feature of Clearance 2.0. My only remaining worry is that it's something else applications have to find a place in their UI to link to. But I think it's worth the tradeoff.

bdewater commented 8 years ago

Tracking last_seen_at once per day would not provide enough information for the smart device and IoT use cases mentioned in this issue IMO. I'd very much like precision down to the minute in the following cases:

Perhaps it could be configurable with a lambda like cookie_expiration currently is for those having performance concerns: have it return true if the field should be updated.

jjb commented 7 years ago

Perhaps features like last seen tracking, ip address tracking, and geolocation could be provided by sign-in guards, either first- or third-party.

So, they can be left out in the initial incarnation of the feature.