Resend confirmation is allowing unconfirmed users to login

[luismfonseca]

If we set config.confirm_within = 1.days and config.allow_unconfirmed_access_for = 1.days, then a user can just trigger a resend_confirmation_instructions which will cause the update field confirmed_sent_at to be updated.

And since that gets set to Time.now.utc, the user is now able to login for the next 24h, without confirming the email address.

The natural response to this would be to bump confirm_within, but that does not solve the actual problem, it only defers it.

PS: I've searched for similar tickets and I haven't found any, but feel free to close this if a ticket already existed.

[domangi]

Hi @luismfonseca , actually devise uses the same field, confirmation_sent_at timestamp, to check if a confirmation token is still valid (confirm_within) and to check if a user can authenticate without confirmation (allow_unconfirmed_access_for).

Just not updating the confirmation_sent_at timestamp when a new confirmation email is requested would not work because a new confirmation token would be considered always invalid.

The only way I see to limit the unconfirmed access to the period after the first confirmation is sent, is to add another timestamp field first_confirmation_sent_at, to be used just to check how many days passed after the first confirmation was sent.

I have implemented this solution here https://github.com/domangi/devise/tree/allow-unconfirmed-access-only-for-one-period

Test Code

test 'should be not active when first confirmation sent is overpast' do
    swap Devise, allow_unconfirmed_access_for: 5.days, confirm_within: 5.days do
      Devise.allow_unconfirmed_access_for = 5.days

      user = create_user
      user.update_attribute(:confirmation_sent_at, 7.days.ago)
      old_confirmation_sent_at = user.confirmation_sent_at
      old_token = user.confirmation_token
      refute user.active_for_authentication?

      user = User.find(user.id)
      user.resend_confirmation_instructions

      assert_equal old_confirmation_sent_at, user.confirmation_sent_at
      assert_not_equal old_token, user.confirmation_token

      refute user.active_for_authentication?
    end
  end

The method that checks if unconfirmed access is allowed becomes

def confirmation_period_valid?
    self.class.allow_unconfirmed_access_for.nil? || 
    (first_confirmation_sent_at && 
     first_confirmation_sent_at.utc >= self.class.allow_unconfirmed_access_for.ago)
end

Where first_confirmation_sent_at is set only the first time a confirmation is send.

I don't know if it is the best solution, for sure it requires some work to be backwards compatible, since it ads a new column to the database, perhaps there could be a fallback on confirmation_sent_at if first_confirmation_sent_at is not present.

@luismfonseca what do you think about this solution?

[luismfonseca]

Right, I think that makes sense. It's unfortunate that it can't be done without adding a new field though. :/

Can libraries in Rails add migrations so that the default value is copied over from confirmation_sent_at?

[domangi]

@luismfonseca I don't know if there is a way to copy the default value from confirmation_sent_at during migration, but probably it is not needed.

If you have to correct version and columns from beginning there is nothing to be done. If you add the first_confirmation_sent_at column afterwards, then I would suggest the following migration procedure:

1) Run migration to add the missing column, e.g. if devise is configured on User rails g migration AddFirstConfirmationSentAtToUser first_confirmation_sent_at:datetime

2) If you want to set the first_confirmation_sent_at value immediately run: User.all.each{|u| u.set_default_for_first_confirmation_sent_at }

But step 2 is not necessary, because I added a fallback in case first_confirmation_sent_at is nil:

The first time the value of first_confirmation_sent_at is checked, if it is nil then it is set equal to confirmation_sent_at.

Test case

  test 'should set first_confirmation_sent_at equal to confirmation_sent_at on the first check if it is nil' do
    user = create_user

    user.first_confirmation_sent_at = nil
    user.confirmation_sent_at = 4.days.ago
    user.active_for_authentication?
    assert_equal user.first_confirmation_sent_at, user.confirmation_sent_at
  end

Implementation

def confirmation_period_valid?
  # If first_confirmation_sent_at field was added after the user was created it's first_confirmation_sent_at could be not set.
  # Set it equal to confirmation_sent_at the first time it is needed
  self.set_default_for_first_confirmation_sent_at
  self.class.allow_unconfirmed_access_for.nil? || (first_confirmation_sent_at && first_confirmation_sent_at.utc >= self.class.allow_unconfirmed_access_for.ago)
end

def set_default_for_first_confirmation_sent_at
  self.update(first_confirmation_sent_at: self.confirmation_sent_at) if self.first_confirmation_sent_at.nil?
end

I pushed the "fallback" to https://github.com/domangi/devise/tree/allow-unconfirmed-access-only-for-one-period

[tegon]

Hi everyone, thanks for the report.

I'm not sure I fully understood the problem, but we have a pull request open (#4792) which seems related to this issue. Can you take a look at it to see if it solves this issue too?

[luismfonseca]

Yes, that PR will solve this issue. 👍

[tannakartikey]

Hi, @tegon. Any plans of merging it anytime soon?

[tegon]

@tannakartikey the PR is still missing a test case for the new behavior.

[tegon]

Is anyone interested in tackling this issue? #4792 seems to fix it, but it's lacking tests and now it seems like the author deleted its GitHub account.

[tannakartikey]

@tegon I'd be happy to tackle it.