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

Add expiry for tokens #25

Closed
wants to merge 3 commits into from

Conversation

lauraannwilliams
Copy link

adds a configuration value and check for expired tokens, to create auth tokens that expire at a custom time

@baschtl
Copy link
Owner

baschtl commented Oct 28, 2015

Hey @lauraannwilliams,

thanks a lot for providing this PR! Great!

Could you have a look at the failing specs on Travis and add some information to the readme about the new configuration option? After this I will have a detailed look at your changes.

@baschtl
Copy link
Owner

baschtl commented Mar 11, 2016

@lauraannwilliams Isn't this the same functionality as the devise timeoutable module (http://www.rubydoc.info/github/plataformatec/devise/master/Devise/Models/Timeoutable) gives us?

@baschtl baschtl force-pushed the master branch 3 times, most recently from 4eb2d93 to 7c4b9fb Compare April 15, 2016 12:22
@mikwat
Copy link
Contributor

mikwat commented Apr 20, 2016

I actually think this would be a good improvement. The devise timeoutable module doesn't quite accomplish the same thing. Timeoutable will invalidate a session after a period of inactivity, but if you include the same auth token in every request, a new session will be created each time and thus the token will never be reset.

@mikwat
Copy link
Contributor

mikwat commented Apr 21, 2016

@baschtl I've resurrected this PR (and fixed the specs) v0.4.6...TetrationAnalytics:v0.4.7 but since there isn't a branch for v0.4.6 I can't submit a PR.

@baschtl
Copy link
Owner

baschtl commented May 24, 2016

Changes were incorporated in version 0.4.9. Thanks @lauraannwilliams !

@baschtl baschtl closed this May 24, 2016
@leonelgalan
Copy link

Does the introduction of this PR requires a authentication_token_created_at on the users table? I'm haven't change any setting, other than upgrading the gem and I'm getting:

NoMethodError: undefined methodauthentication_token_created_at=' for #User:0x007f4eff0fb388`

…ctivemodel-4.2.6/lib/active_model/attribute_methods.rb: 433:in `method_missing'
…atable-0.5.0/lib/devise/token_authenticatable/model.rb:  62:in `reset_authentication_token'
…atable-0.5.0/lib/devise/token_authenticatable/model.rb:  73:in `ensure_authentication_token'
/app/app/controllers/api/v2/registrations_controller.rb:  17:in `create'

@mikwat
Copy link
Contributor

mikwat commented May 27, 2016

Yes @leonelgalan See here:
https://github.com/baschtl/devise-token_authenticatable/releases/tag/v0.4.9

Should we make it so this field isn't required, @baschtl ?

@leonelgalan
Copy link

leonelgalan commented May 27, 2016

I missed there. Specially being a 0.0.x increase, I didn't expect breaking changes. My gut says yes, as long as the setting is optional and off by default the field should too. I'll take a look.

EDIT: Backtracking, it might also be good to improve the README. Currently it doesn't mention the fields needed on the DB. Most of us come from Devise having this feature and this gem allowing us to keep that behavior, but for newcomers.

@baschtl
Copy link
Owner

baschtl commented Jun 1, 2016

@mikwat Yes, optional would be good. I just did not have the time to reason about it. It should only be optional if token_expires_in is not set IMO.

@leonelgalan Thanks for the input. You are also welcome to improve the readme via a pull request. :-)

@mikwat
Copy link
Contributor

mikwat commented Jun 16, 2016

@baschtl Here's a fix (if you create a branch for 0.4.9 I can send a proper PR): v0.4.9...TetrationAnalytics:optional-token-expires-in

CC: @leonelgalan

@mikwat
Copy link
Contributor

mikwat commented Jun 16, 2016

@baschtl
Copy link
Owner

baschtl commented Jun 21, 2016

@mikwat See https://github.com/baschtl/devise-token_authenticatable/tree/v0.4.9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants