Simplifying ConfirmationsController show behavior #1075

dks17 · 2018-01-31T09:12:20Z

Is this snippet is really needed there?

expiry = nil
if defined?(@resource.sign_in_count) && @resource.sign_in_count > 0
  expiry = (Time.now + 1.second).to_i
end

module DeviseTokenAuth
  class ConfirmationsController < DeviseTokenAuth::ApplicationController
    def show
      @resource = resource_class.confirm_by_token(params[:confirmation_token])

      if @resource && @resource.id
        expiry = nil
        if defined?(@resource.sign_in_count) && @resource.sign_in_count > 0
          expiry = (Time.now + 1.second).to_i
        end

        client_id, token = @resource.create_token expiry: expiry

        sign_in(@resource)
        @resource.save!

        yield @resource if block_given?

        redirect_header_options = {account_confirmation_success: true}
        redirect_headers = build_redirect_headers(token,
                                                  client_id,
                                                  redirect_header_options)
        redirect_to(@resource.build_auth_url(params[:redirect_url],
                                             redirect_headers))
      else
        raise ActionController::RoutingError.new('Not Found')
      end
    end
  end
end

Should behavior of show action depends on including Trackable module or not?
What if Trackable module is off?
Should that behavior be covered with some tests?

UPDATE

-Trackable module extract and removed totally. Now :trackable is optional and functionality does not depend on it.

@resource and signed_in_resource are not the same. signed_in_resource is current session user, and @resource just user which email is confirmed.
If user is not signed in and follows the confirmation link - will be redirected without new token generation. If user is signed in - will be redirected with new token as expected.

Signed in user:

> response.body
=> "<html><body>You are being <a href=\"http://windler.com/chuck_tromp?access-token=BldiEDtjqLcfvArwB3c1cA&amp;account_confirmation_success=true&amp;client=g4hPjIW505PjYqWjREHwkw&amp;client_id=g4hPjIW505PjYqWjREHwkw&amp;config=&amp;expiry=&amp;token=BldiEDtjqLcfvArwB3c1cA&amp;uid=junko%40example.org\">redirected</a>.</body></html>"
> response.headers
=> {"X-Frame-Options"=>"SAMEORIGIN",
 "X-XSS-Protection"=>"1; mode=block",
 "X-Content-Type-Options"=>"nosniff",
 "X-Download-Options"=>"noopen",
 "X-Permitted-Cross-Domain-Policies"=>"none",
 "Referrer-Policy"=>"strict-origin-when-cross-origin",
 "Location"=>
  "http://windler.com/chuck_tromp?access-token=BldiEDtjqLcfvArwB3c1cA&account_confirmation_success=true&client=g4hPjIW505PjYqWjREHwkw&client_id=g4hPjIW505PjYqWjREHwkw&config=&expiry=&token=BldiEDtjqLcfvArwB3c1cA&uid=junko%40example.org",
 "Content-Type"=>"text/html; charset=utf-8"}

Signed out user:

> response.body
=> "<html><body>You are being <a href=\"http://kutchbradtke.info/omer.littel?account_confirmation_success=true\">redirected</a>.</body></html>"
> response.headers
=> {"X-Frame-Options"=>"SAMEORIGIN",
 "X-XSS-Protection"=>"1; mode=block",
 "X-Content-Type-Options"=>"nosniff",
 "X-Download-Options"=>"noopen",
 "X-Permitted-Cross-Domain-Policies"=>"none",
 "Referrer-Policy"=>"strict-origin-when-cross-origin",
 "Location"=>"http://kutchbradtke.info/omer.littel?account_confirmation_success=true",
 "Content-Type"=>"text/html; charset=utf-8"}

When signed in user tries to confirm the same email more than once - raise routing error. I think that the show action should return json with errors (it brings small api changes). It looks like an issue for another PR.
No more small expiry for a token. This caused the error:

ERROR["test_yield_resource_to_block_on_show_success", #<Minitest::Reporters::Suite:0x0000557f27959fb0 @name="Custom::ConfirmationsController">, 124.99346235499979]
 test_yield_resource_to_block_on_show_success#Custom::ConfirmationsController (124.99s)
NoMethodError:         NoMethodError: undefined method `[]' for nil:NilClass
            app/controllers/custom/confirmations_controller.rb:5:in `show'

zachfeldman · 2018-02-01T19:56:39Z

Hey @dks17 , perhaps you're right that we shouldn't depend on trackable here. In that case, are you suggesting we set no expiry on the sign in token instead?

dks17 · 2018-02-02T07:16:28Z

@zachfeldman I am suggesting do less than more.
Is it necessary to compute conditional block if possible and acceptable doesn't do it?
If expiry is not provided it is set by default

expiry ||= (Time.now + token_lifespan).to_i

I just slimplified it much as were possible. Of course we could leave only

expiry = (Time.now + 1.second).to_i
client_id, token = @resource.create_token expiry: expiry

I think conditional block should be cleaned up anyway.

zachfeldman · 2018-02-02T15:42:16Z

Ok, I think this change makes sense then. If another maintainer approves this pull request, I will merge it.

dks17 · 2018-02-05T19:24:33Z

I would like implement TokenFactory to generate token in one place with opportunity of tuning token and I have almost done it.
I would like that this pr will be considered soon. If this pr will be rejected I will add to my next pr a code to which we will come. Or I will just close this pr and forget about it.
I think this code should be refactored and simplified:

expiry = nil
if defined?(@resource.sign_in_count) && @resource.sign_in_count > 0
 expiry = (Time.now + 1.second).to_i
 end

client_id, token = @resource.create_token expiry: expiry

@lynndylanhurley, @MaicolBen
what do you think about changes?

MaicolBen · 2018-02-05T19:59:37Z

I don't quite understand why you're removing the code, it was added by #519 (I remade the PR in #1001).
I don't know why the tests are still working without the expiration change, but it doesn't make sense your argument "Should behavior of show action depends on including Trackable module or not?". We try to support all the device modules, and if there are a specific module code, it should ask for the module presence (in this case asks for the presence of sign_in_count definition, so it's the same).

MaicolBen · 2018-02-05T20:04:13Z

The idea of #519 is to allow only one confirmation, it expires the token with expiry = (Time.now + 1.second).to_i

dks17 · 2018-02-05T21:05:04Z

Where is place I could turn trackable off in the gem? Is it there?
I would like check this statement:

You may not want all of these features enabled in your app. That's OK! You can mix and match to suit your own unique style.

Devise implementation doesn't use trackable sign_in_count.

Of course I can be mistaken.

dks17 · 2018-02-05T21:09:47Z

I have removed :trackable from here

included do
    # Hack to check if devise is already enabled
    unless self.method_defined?(:devise_modules)
      devise :database_authenticatable, :registerable,
          :recoverable, :validatable, :confirmable
else

and got bunch of 9 failures:

FAIL["test_User_shoud_have_the_Last_checkin_filled", Minitest::Result, 16.495720947001246]
 test_User_shoud_have_the_Last_checkin_filled#Minitest::Result (16.50s)
        Expected false to be truthy.

 FAIL["test_User_shoud_have_the_signed_in_info_filled", Minitest::Result, 16.618372673998238]
 test_User_shoud_have_the_signed_in_info_filled#Minitest::Result (16.62s)
        Expected false to be truthy.

 FAIL["test_user_already_confirmed", Minitest::Result, 16.850464181996358]
 test_user_already_confirmed#Minitest::Result (16.85s)
        Expected false to be truthy.

 FAIL["test_the_sign_in_count_should_be_1", Minitest::Result, 17.10246518799977]
 test_the_sign_in_count_should_be_1#Minitest::Result (17.10s)
        Expected false to be truthy.

 FAIL["test_sign_in_count_incrementns", Minitest::Result, 29.07351241700235]
 test_sign_in_count_incrementns#Minitest::Result (29.07s)
        Expected: 1
          Actual: 0

 FAIL["test_current_sign_in_at_is_updated", Minitest::Result, 29.27919944599853]
 test_current_sign_in_at_is_updated#Minitest::Result (29.28s)
        Expected nil to be truthy.

 FAIL["test_last_sign_in_ip_is_updated", Minitest::Result, 29.481255300001067]
 test_last_sign_in_ip_is_updated#Minitest::Result (29.48s)
        Expected: "0.0.0.0"
          Actual: nil

 FAIL["test_sign_in_ip_is_updated", Minitest::Result, 29.64863184800197]
 test_sign_in_ip_is_updated#Minitest::Result (29.65s)
        Expected: "0.0.0.0"
          Actual: nil

 FAIL["test_last_sign_in_at_is_updated", Minitest::Result, 29.857282397002564]
 test_last_sign_in_at_is_updated#Minitest::Result (29.86s)
        Expected nil to be truthy.

409 tests, 562 assertions, 9 failures, 0 errors, 0 skips

MaicolBen · 2018-02-06T14:08:52Z

Devise implementation doesn't use trackable sign_in_count.

I don't know why you pointed confirmations controller, of course, devise doesn't use it there, but we use it here to avoid double confirmation. So we can't turn off the module, because we use it.

You have to turn it off in your code, but seems you have sign_in_count defined, so you should remove that field if you aren't interested in tracking the sign in.

dks17 · 2018-02-06T20:14:00Z

DeviseTokenAuth includes modules if devise macro(?) with modules is not defined.
As I understand you can't use a module by default for DeviseTokenAuth if user doesn't include specific module into user model.
You can reproduce the same failures another way. Put it in https://github.com/lynndylanhurley/devise_token_auth/blob/master/test/dummy/app/models/user.rb

class User < ActiveRecord::Base
  devise :database_authenticatable, :registerable,
          :recoverable, :validatable, :confirmable
  include DeviseTokenAuth::Concerns::User

This is the same I have done above. I think we should pay attention to the failures.

I think Trackable module is should be optional, for statistics gathering only. And implementation should be independent of the module.

What will happen if Trackable was been off? Should 'If' statement computing thousand/million times even if user didn't include Trackable module?
I think implementation should be the same in the both cases (Trackable off/on).

MaicolBen · 2018-02-06T20:23:24Z

I think Trackable module is should be optional, for statistics gathering only. And implementation should be independent of the module.

Sorry if I wasn't clear but in this case defined?(@resource.sign_in_count) is the same as resource_class.devise_modules.include?(:trackable). So if you don't have enabled trackable in your project, it won't enter to that condition.

What will happen if Trackable was been off? Should 'If' statement computing thousand/million times even if user didn't include Trackable module?

I think defined?(@resource.sign_in_count) is way faster than resource_class.devise_modules.include?(:trackable). In case you don't want that code at all in your project because it's slow (I doubt it), you can override the show action in your project and remove the if

I think implementation should be the same in the both cases (Trackable off/on).

In this part I can agree, how would you achieve that?

dks17 · 2018-02-07T07:16:28Z

In this part I can agree, how would you achieve that?

Devise confirmed? method?

# expiry = nil
# if defined?(@resource.sign_in_count) && @resource.sign_in_count > 0
#   expiry = (Time.now + 1.second).to_i
# end
expiry = @resource.confirmed? ? (Time.now + 1.second).to_i : nil

client_id, token = @resource.create_token expiry: expiry

MaicolBen · 2018-02-07T13:25:02Z

@dks17 Sounds good! Can you change that in the PR? It shouldn't break any test

dks17 · 2018-02-07T13:58:39Z

I have caught:

ERROR["test_yield_resource_to_block_on_show_success", Minitest::Result, 26.821582222997677]
 test_yield_resource_to_block_on_show_success#Minitest::Result (26.82s)
NoMethodError:         NoMethodError: undefined method `[]' for nil:NilClass
            app/controllers/custom/confirmations_controller.rb:4:in `show'

ERROR["test_should_redirect_to_success_url", Minitest::Result, 34.8219258479985]
 test_should_redirect_to_success_url#Minitest::Result (34.82s)
NoMethodError:         NoMethodError: undefined method `[]' for nil:NilClass
            

ERROR["test_user_should_now_be_confirmed", Minitest::Result, 38.842425470997114]
 test_user_should_now_be_confirmed#Minitest::Result (38.84s)
NoMethodError:         NoMethodError: undefined method `[]' for nil:NilClass

Tests is running randomly. You can catch more errors.
I think new pr is needed for this changes (controller + tests). I would not like including this in TokenFactory pr.

Evan-M · 2018-04-04T23:48:52Z

@dks17: I've run into this same issue while working on a PR for #1123. I have rewritten /test/controllers/devise_token_auth/confirmations_controller_test.rb nearly entirely, but I'm still "randomly" getting the NoMethodError: undefined method []' for nil:NilClass` errors that you mentioned above.

These errors appear to be the result of a race-condition!

Using the Timecop gem, I can manually replicate the two branches of the race condition if I freeze Time.zone.now in the tests, and manually advance Time.zone.now to either the same second as the auth_token expiry or the second after the auth_token expiry. To see the race condition, the manual advance in time must happen after generating a new auth_token, but before the resource is saved (with that auth_token).

Essentially, since the expiry is hard-coded to 1.second.from_now, if either the subsequent calls to sign_in(@resource) or @resource.save! happen after the expiry, then the newly generated token is deleted by the DeviseTokenAuth::Concerns::User#destroy_expired_tokens before_save callback.

I've reviewed the changes added in #519 (and #1001), and noticed two things:

the call to sign_in(@resource) seems to exist to increment the sign_in_count (which shouldn't really be used in determining confirmations anyway). Commit #15ee641 seems to imply this as well, but notes that successful confirmation will automatically sign in the user (so the columns should be updated to reflect that)
since the sign_in(@resource) call updates the :trackable columns on the @resource, it saves the resource which in turn triggers the before_save :destroy_expired_tokens callback. Calling @resource.save! will also trigger the before_save :destroy_expired_tokens callback. This means there are two opportunities for enough time to pass that a newly created auth_token will have expired (and get destroyed) prior to the success redirect happening.

@MaicolBen: What is not clear to me is: if the short expiration is conditionally set when a @resource has already been confirmed, what is the purpose of setting the expiration to Time.zone.now + 1.second, as opposed to Time.zone.now? I'm guessing that was an attempt to prevent the before_save :destroy_expired_tokens callback from removing the new auth_token prior to redirect?

As I've mentioned in #1123, the confirmations controller should check for the confirmed_at attribute to determine if the resource has already been confirmed (instead of sign_in_count). However, even an explicit check of confirmed_at (or its predicate wrapper-methodconfirmed?) is unnecessary; Devise::Models::Confirmable::ClassMethods#confirm_by_token checks the confirmed_at attribute internally, and sets errors on the resource if either not found or already confirmed.

MaicolBen · 2018-04-05T16:09:25Z

@Evan-M I thought we agreed on #1123 to drop sign_in_count check, can you make a PR? or do you want me to make one?

Evan-M · 2018-04-05T17:39:20Z

@MaicolBen: I'm already on it. 👍

However, I've run into a few side effects of that change, and I wanted to ascertain the specifics of the intended behavior when a user clicks on an email confirmation link for a user with an already confirmed email.

jacferreira · 2018-11-06T16:21:41Z

Just wanted to add one more thing regarding show#confirmations:

if defined?(@resource.sign_in_count) && @resource.sign_in_count > 0
  expiry = (Time.zone.now + 1.second).to_i
end

In my case I have the default token lifespan (2 weeks) and max number of devices (10). If a user reaches the max number of devices in less than 2 weeks (not at all impossible), the token that is generated on show#confirmations will be removed by clean_old_tokens since it will be the oldest among the existing tokens. This makes it impossible to be successfully redirected to an authenticated route.

dks17 · 2018-12-10T09:46:01Z

Updated. Please see top message.

MaicolBen

I see you are using signed_in? (I had to remind the history of this PR), good work and thanks for bringing back this PR!

zachfeldman self-requested a review February 2, 2018 15:42

zachfeldman approved these changes Feb 2, 2018

View reviewed changes

MaicolBen added the on hold label Feb 5, 2018

dks17 closed this Feb 14, 2018

dks17 deleted the confirmation_controller branch February 14, 2018 18:43

dks17 restored the confirmation_controller branch February 14, 2018 20:32

dks17 reopened this Feb 14, 2018

Evan-M mentioned this pull request Mar 22, 2018

Confirming an already confirmed user -- still not quite working. #1123

Closed

dks17 mentioned this pull request Nov 6, 2018

new TokenFactory module #1085

Merged

Drop Devise Trackable module

52a0b0e

dks17 force-pushed the confirmation_controller branch from def9671 to 52a0b0e Compare December 10, 2018 08:48

MaicolBen approved these changes Jan 19, 2019

View reviewed changes

MaicolBen removed the on hold label Jan 19, 2019

MaicolBen merged commit 7eeed86 into lynndylanhurley:master Jan 19, 2019

dks17 deleted the confirmation_controller branch January 29, 2019 13:13

MaicolBen mentioned this pull request Aug 18, 2019

Missing user credential in confirmation redirect url querystring #1292

Closed

SugiKent mentioned this pull request Nov 16, 2019

Why is Trackable option still active? #1356

Closed

Simplifying ConfirmationsController show behavior #1075

Simplifying ConfirmationsController show behavior #1075

Uh oh!

Conversation

dks17 commented Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zachfeldman commented Feb 1, 2018

Uh oh!

dks17 commented Feb 2, 2018

Uh oh!

zachfeldman commented Feb 2, 2018

Uh oh!

dks17 commented Feb 5, 2018

Uh oh!

MaicolBen commented Feb 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaicolBen commented Feb 5, 2018

Uh oh!

dks17 commented Feb 5, 2018

Uh oh!

dks17 commented Feb 5, 2018

Uh oh!

MaicolBen commented Feb 6, 2018

Uh oh!

dks17 commented Feb 6, 2018

Uh oh!

MaicolBen commented Feb 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dks17 commented Feb 7, 2018

Uh oh!

MaicolBen commented Feb 7, 2018

Uh oh!

dks17 commented Feb 7, 2018

Uh oh!

Evan-M commented Apr 4, 2018

Uh oh!

MaicolBen commented Apr 5, 2018

Uh oh!

Evan-M commented Apr 5, 2018

Uh oh!

jacferreira commented Nov 6, 2018

Uh oh!

dks17 commented Dec 10, 2018

Uh oh!

MaicolBen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dks17 commented Jan 31, 2018 •

edited

Loading

MaicolBen commented Feb 5, 2018 •

edited

Loading

MaicolBen commented Feb 6, 2018 •

edited

Loading