Set an expiration for password reset tokens #532

mxie · 2015-02-06T18:41:06Z

Overview:
Password reset tokens will now have an expiration date/time. By default, it
will be in 15 minutes. This is configurable via the password_reset_time_limit
option.

Users can request a reset as many times as they wish. Once they "claim" a reset
with one of their tokens, all others will be invalidated (set as expired).

Details:

User-token data in a separate table called password_resets.
Password reset logic has been extracted from the User model. It's now
handled at the controller level, referring to the PasswordReset and
PasswordResetDeactivator classes as needed.
Routes and views have stayed the same for transparency.
- To consider: renaming PasswordsController to PasswordResetsController
  later.
Introduced Clearance::Generators::Migration to abstract common migration-related methods

This has been tested on a repo that's currently using Clearance, and it seems to work fine. I plan on adding some info on the Wiki for anyone who's adding the expiration feature to their existing Clearance'd project.

Also, this is a huge PR. Suggestions on how to break this up would be great appreciated.

Fixes #465

houndci-bot · 2015-02-06T18:41:12Z

app/controllers/clearance/passwords_controller.rb

-    unless find_user_by_id_and_confirmation_token
+    unless find_user_from_password_reset
+      flash_failure_when_forbidden
+      render template: 'passwords/new'


Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

houndci-bot · 2015-02-06T19:31:14Z

app/models/password_reset.rb

+  def time_limit
+    Clearance.configuration.password_reset_time_limit
+  end
+


Extra empty line detected at body end.

mxie · 2015-02-06T19:39:36Z

app/controllers/clearance/passwords_controller.rb

-      deliver_email(user)
+      password_reset = PasswordReset.new
+      password_reset.user = user
+      password_reset.save


It's been a while since I've Rails'ed, but there's a better way to do this, right? It's a one-way association, i.e. there's only a belongs_to association in PasswordReset, no has_many from User.

Maybe:

password_reset = PasswordReset.create!(user: user)

Yeah, I had something like that originally, but Rails 3.2 doesn't seem to like it because of mass-assignments. =\ I guess I could at least extract the reset-building as a private method in here.

Did something like this instead: f2ee26c.

mike-burns · 2015-02-06T20:09:27Z

Since this changes the interface for documented template methods (the documentation is about as broad as possible: "See app/controllers/clearance for the default behavior"), this means that this will be part of the major version bump. Right?

mxie · 2015-02-06T20:13:13Z

@mike-burns Yes, you are correct.

derekprior · 2015-02-10T02:07:20Z

db/migrate/20150122204353_create_password_resets.rb

+      t.timestamps null: false
+    end
+
+    add_index :password_resets, :user_id


should we index the expires_at column as well? Maybe change this to a compound index on [:user_id, :expires_at]? Might be too much...

Sounds good to me. Done in 8f0f9a8.

derekprior · 2015-02-10T02:13:40Z

Took a quick spin through everything. This is great, thanks so much.

I do think we can do this in a manner that lets us ship this in opt-in fashion for 1.x. Either via a separate controller and/or via service objects like monban. We can look at it Friday.

mxie · 2015-02-12T19:36:38Z

app/models/password_reset.rb

@@ -0,0 +1,36 @@
+class PasswordReset < ActiveRecord::Base


Should this be a template instead of in app/models, like https://github.com/thoughtbot/clearance/blob/master/lib/generators/clearance/install/templates/user.rb?

No. Clearance models are internal to clearance and not generate. User is a template because we expect you already have a user class of some sort.

houndci-bot · 2015-03-02T22:08:42Z

db/migrate/20150127232904_remove_confirmation_token_from_users.rb

+  def up
+    expiration_timestamp = Clearance.configuration.
+      password_reset_time_limit.
+      from_now


Align the operands of an expression in an assignment spanning multiple lines.

houndci-bot · 2015-07-17T19:46:21Z

spec/generators/clearance/install/install_generator_spec.rb

+    provide_existing_application_controller
+    allow(password_reset_migration_generator).to receive(:new).
+      and_return(
+        instance_double(password_reset_migration_generator).as_null_object


Put a comma after the last parameter of a multiline method call.

houndci-bot · 2015-07-17T19:51:39Z

app/models/password_reset.rb

+    where(
+      "#{Clearance.configuration.user_id_parameter} = ? AND expires_at > ?",
+      user_id,
+      Time.zone.now


Put a comma after the last parameter of a multiline method call.

mxie · 2015-07-24T15:26:31Z

@derekprior Whenever you get a chance, could you take a look at this again? I think it's pretty much all set to go. The only thing I might add is an "Upgrade Guide" of sorts. That could be done separately though.

derekprior · 2016-05-15T01:01:49Z

With Clearance 2.0 progressing, I was finally able to return to this. I made some major changes that are afforded by dropping support for older versions of Rails. We now no longer need the database for password reset tokens, so the resulting PR is simpler, I think.

See #682.

houndci-bot reviewed Feb 6, 2015
View reviewed changes

mxie force-pushed the mx-expiring-resets branch from 840d69b to c686c74 Compare February 6, 2015 19:03

houndci-bot reviewed Feb 6, 2015
View reviewed changes

app/models/password_reset.rb

def time_limit

Clearance.configuration.password_reset_time_limit

end

Copy link

houndci-bot Feb 6, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra empty line detected at body end.

mxie force-pushed the mx-expiring-resets branch from b011ae8 to 5690e51 Compare February 6, 2015 19:31

mxie reviewed Feb 6, 2015
View reviewed changes

derekprior reviewed Feb 10, 2015
View reviewed changes

mxie reviewed Feb 12, 2015
View reviewed changes

mxie force-pushed the mx-expiring-resets branch from 832f668 to b919253 Compare March 2, 2015 22:08

houndci-bot reviewed Mar 2, 2015
View reviewed changes

mxie added 19 commits July 17, 2015 14:21

Clean up instructions about customizable settings for emails

2c2a317

delivers->sends email and add "Existing users" as a prefix

42153c0

Simplify associating a user to new password reset

91b0bab

shorten password reset finding method

dbf0acb

Bump limit to 20 minutes

caccebf

Rename deactivator to be plural

c41a02a

rename deactivate! and add complete

498488e

move time limit test

84ddd55

Change pw reset URL in text formatted email and mailer spec

d2cfb56

Remove character limit on token

cc0858b

wip

cf0c985

remove deactivation check

8492fcc

remove leftover controller callback

2962f39

find active resets

70bce4a

change update failure message to be more generic

0ec2fa5

clean up password reset spec

d49f107

handle completed reset

d5ad597

just have #complete return true or false

a3c73c7

forgot to remove :with_forgotten_password trait in spec

e8358da

mxie force-pushed the mx-expiring-resets branch from 0aa95df to e8358da Compare July 17, 2015 19:46

houndci-bot reviewed Jul 17, 2015
View reviewed changes

long lines

3bd868e

houndci-bot reviewed Jul 17, 2015
View reviewed changes

derekprior closed this May 15, 2016

mjankowski deleted the mx-expiring-resets branch February 25, 2019 19:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set an expiration for password reset tokens #532

Set an expiration for password reset tokens #532

mxie commented Feb 6, 2015

houndci-bot Feb 6, 2015

houndci-bot Feb 6, 2015

mxie Feb 6, 2015

derekprior Feb 10, 2015

mxie Feb 10, 2015

mxie Feb 10, 2015

mike-burns commented Feb 6, 2015

mxie commented Feb 6, 2015

derekprior Feb 10, 2015

mxie Feb 12, 2015

derekprior commented Feb 10, 2015

mxie Feb 12, 2015

derekprior Jun 5, 2015

houndci-bot Mar 2, 2015

houndci-bot Jul 17, 2015

houndci-bot Jul 17, 2015

mxie commented Jul 24, 2015

derekprior commented May 15, 2016

Set an expiration for password reset tokens #532

Set an expiration for password reset tokens #532

Conversation

mxie commented Feb 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-burns commented Feb 6, 2015

mxie commented Feb 6, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekprior commented Feb 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxie commented Jul 24, 2015

derekprior commented May 15, 2016