Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Email token expiration #1975

Merged
merged 6 commits into from Jul 22, 2012

Conversation

Projects
None yet
3 participants
Contributor

promisedlandt commented Jul 12, 2012

As discussed in issue #1965, this patch adds the ability to let email confirmation tokens only be valid for a limited time period.

promisedlandt added some commits Jul 9, 2012

Add options to expire confirmation tokens
With this patch, functionality is added to expire the confirmation
tokens that are being sent by email.
For example, if a token is valid for 3 days only, it cannot be used for
confirmation on the 4th day.
Fix tests for email token expiration
The tests work now, but are a bit wonky because User.create does things
I don't understand.

@rodrigoflores rodrigoflores commented on an outdated diff Jul 13, 2012

lib/devise/models/confirmable.rb
@@ -28,6 +30,8 @@ module Models
#
module Confirmable
extend ActiveSupport::Concern
+ # TODO: is this a good idea?
+ include ActionView::Helpers::DateHelper
@rodrigoflores

rodrigoflores Jul 13, 2012

Contributor

I think this isn't necessary. I removed it and the tests still pass.

@rodrigoflores

rodrigoflores Jul 15, 2012

Contributor

Nevermind, it is being used because of time_ago_in_words

@rodrigoflores rodrigoflores commented on an outdated diff Jul 13, 2012

lib/devise/models/confirmable.rb
# Checks whether the record requires any confirmation.
def pending_any_confirmation
- if !confirmed? || pending_reconfirmation?
+ unless confirmation_period_expired? || (confirmed? && !pending_reconfirmation?)
@rodrigoflores

rodrigoflores Jul 13, 2012

Contributor

Sorry, I think I wasn't clear when I said to invert the condition :)

I would rewrite this line this way:

if (!confirmed? || pending_reconfirmation?) && !confirmation_period_expired?
Contributor

rodrigoflores commented Jul 13, 2012

Hi

I didn't finish taking a look at your pull request. But I should finish it until the end of the day :)

Contributor

promisedlandt commented Jul 13, 2012

Take your time, I can't work on it until sunday I think

@rodrigoflores rodrigoflores and 1 other commented on an outdated diff Jul 15, 2012

lib/devise/models/confirmable.rb
@@ -156,12 +159,34 @@ def confirmation_period_valid?
confirmation_sent_at && confirmation_sent_at.utc >= self.class.allow_unconfirmed_access_for.ago
end
+ # Checks if the user confirmation happens before the token becomes invalid
+ #
+ # Examples:
+ #
+ # # expire_confirmation_token_after = 3.days and confirmation_sent_at = 2.days.ago
+ # confirmation_period_expired? # returns false
+ #
+ # # expire_confirmation_token_after = 3.days and confirmation_sent_at = 4.days.ago
+ # confirmation_period_expired? # returns true
+ #
+ # # expire_confirmation_token_after = nil
+ # confirmation_period_expired? # will always return false
+ #
+ def confirmation_period_expired?
+ self.class.expire_confirmation_token_after && (Time.now > self.confirmation_sent_at + self.class.expire_confirmation_token_after)
@rodrigoflores

rodrigoflores Jul 15, 2012

Contributor

Maybe we can "cache" it this way:

@confirmation_period_expired ||= self.class.expire_confirmation_token_after && (Time.now > self.confirmation_sent_at + self.class.expire_confirmation_token_after)
@promisedlandt

promisedlandt Jul 15, 2012

Contributor

I don't think it's great to cache boolean variables this way (because false can never be cached), it would have to be more like

@confirmation_period_expired = @confirmation_period_expired.nil? ? self.class.expire_confirmation_token_after && (Time.now > self.confirmation_sent_at + self.class.expire_confirmation_token_after : @confirmation_period_expired

and that doesn't help readability enough, right?

@rodrigoflores

rodrigoflores Jul 15, 2012

Contributor

Yeah, that's right.

I'll try to think about a way to do that.

@rodrigoflores rodrigoflores commented on an outdated diff Jul 15, 2012

test/integration/confirmable_test.rb
+ # TODO: User.create! always sets confirmation_sent_at to Time.now
+ user = create_user(:confirm => false, :confirmation_sent_at => 4.days.ago)
+ user.confirmation_sent_at = 4.days.ago
+ user.save
+ assert_not user.confirmed?
+ visit_user_confirmation_with_token(user.confirmation_token)
+
+ assert_have_selector '#error_explanation'
+ assert_contain /needs to be confirmed within/
+ assert_not user.reload.confirmed?
+ end
+ end
+
+ test 'user with valid confirmation token should be able to confirm an account before the token has expired' do
+ swap Devise, :expire_confirmation_token_after => 3.days do
+ # TODO: why is confirmation_sent_at not set correctly when passed to create_user?
@rodrigoflores

rodrigoflores Jul 15, 2012

Contributor

This happens because of the callback which overrides this value.

As we want to enforce the date to be 3 days ago, I don't see any problem in what you did. Maybe we should use instead update_attribute to clearly show that we're enforcing it.

@rodrigoflores

rodrigoflores Jul 15, 2012

Contributor

I've commented on the "factory" about this.

@rodrigoflores rodrigoflores commented on an outdated diff Jul 15, 2012

test/support/integration.rb
@@ -12,7 +12,8 @@ def create_user(options={})
:email => options[:email] || 'user@test.com',
:password => options[:password] || '12345678',
:password_confirmation => options[:password] || '12345678',
- :created_at => Time.now.utc
+ :created_at => Time.now.utc,
+ :confirmation_sent_at => options[:confirmation_sent_at]
@rodrigoflores

rodrigoflores Jul 15, 2012

Contributor

I think that we may remove this line and add this:

user.update_attribute(:confirmation_sent_at, options[:confirmation_sent_at]) if options[:confirmation_sent_at]

@rodrigoflores rodrigoflores commented on an outdated diff Jul 15, 2012

test/support/helpers.rb
@@ -26,7 +26,8 @@ def valid_attributes(attributes={})
{ :username => "usertest",
:email => generate_unique_email,
:password => '12345678',
- :password_confirmation => '12345678' }.update(attributes)
+ :password_confirmation => '12345678',
+ :confirmation_sent_at => nil }.update(attributes)
@rodrigoflores

rodrigoflores Jul 15, 2012

Contributor

Same as I mentioned on test/support/integration.rb line

promisedlandt added some commits Jul 16, 2012

Refactor according to rodrigoflores
- Favor using update_attribute instead of constructor parameters in user
  factory for tests
- Test for accurate error message when confirmation token is expired
- Don't check twice whether the confirmation period is expired
Contributor

rodrigoflores commented Jul 19, 2012

I would refactor this way:

        def confirmation_period_expired?
          if @confirmation_period_expired.nil?
            @confirmation_period_expired = self.class.expire_confirmation_token_after && (Time.now > self.confirmation_sent_at + self.class.expire_confirmation_token_after )
            @confirmation_period_expired
          else
            @confirmation_period_expired
          end
        end

        # Checks whether the record requires any confirmation.
        def pending_any_confirmation
          @confirmation_period_expired = confirmation_period_expired?

          if (!confirmed? || pending_reconfirmation?) && !@confirmation_period_expired
            yield
          else
            if @confirmation_period_expired
              self.errors.add(:email, :confirmation_period_expired, period: time_ago_in_words(self.class.expire_confirmation_token_after.ago))
            else
              self.errors.add(:email, :already_confirmed)
            end
            false
          end
        end
Contributor

rodrigoflores commented Jul 19, 2012

@josevalim @carlosantoniodasilva @rafaelfranca Any remarks about this pull request ?

@josevalim josevalim commented on an outdated diff Jul 19, 2012

lib/devise/models/confirmable.rb
yield
else
- self.errors.add(:email, :already_confirmed)
+ if @confirmation_period_expired
+ self.errors.add(:email, :confirmation_period_expired, period: time_ago_in_words(self.class.expire_confirmation_token_after.ago))
@josevalim

josevalim Jul 19, 2012

Owner

Please use hashrocket syntax for hashes.

@josevalim josevalim commented on an outdated diff Jul 19, 2012

lib/devise/models/confirmable.rb
@@ -158,10 +160,20 @@ def confirmation_period_valid?
# Checks whether the record requires any confirmation.
def pending_any_confirmation
- if !confirmed? || pending_reconfirmation?
+ @confirmation_period_expired = if @confirmation_period_expired.nil?
@josevalim

josevalim Jul 19, 2012

Owner

Please encapsulate this logic in another method.

@josevalim josevalim commented on an outdated diff Jul 19, 2012

lib/generators/templates/devise.rb
@@ -92,6 +92,14 @@
# the user cannot access the website without confirming his account.
# config.allow_unconfirmed_access_for = 2.days
+ # A period that the user is allowed to confirm their account before their token
+ # becomes invalid. For example, if set to 3.days, the user can confirm their account
+ # within 3 days after the mail was sent, but on the fourth day their account can't be
+ # confirmed with the token any more
+ # Default is nil, meaning there is no restriction on how long a user can take before
+ # confirming their account.
+ # config.expire_confirmation_token_after = 3.days
@josevalim

josevalim Jul 19, 2012

Owner

Can we please name the configuration confirm_within ? It will be similar to reset_password_within.

Owner

josevalim commented Jul 19, 2012

This looks great functionality wise, thanks! I have just added some small comments about code organization and naming. Could you please take a look?

Refactor according to line notes from josevalim
- rename reset_password_within to confirm_within
- confirmation_period_valid? is back and memoized
- fix hash syntax to hashrocket
Contributor

promisedlandt commented Jul 22, 2012

Sorry for the delay, I'm swamped atm. Refactored, tests pass.

rodrigoflores added a commit that referenced this pull request Jul 22, 2012

@rodrigoflores rodrigoflores merged commit 6a37945 into plataformatec:master Jul 22, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment