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

[Evan-M]

NOTE: Much of this has been referenced before in several different Issues and PRs, which I will attempt to link to. If I've left any out, I apologize in advance.

---

Although it appears that this was first noted in either #410 or #519, and eventually merged in #1001, the current behavior is still not great.

After confirming a user's email address, I can continue to click the confirmation link (i.e. make GET requests to the /auth/confirmations endpoint, triggering the devise_token_auth/confirmations#show action). It seems that the current behavior will generate a new auth token for the user, and redirect to the successful confirmation URL (either from the params if present, or the DeviseTokenAuth.default_confirm_success_url).

As pointed out in #1064, #1067, and #1075, when the new token is created, it conditionally adds a expiration if the user has signed in (the :sign_in_count attribute), which creates a dependency on :trackable. And while #1064 + #1061 effectively uncouple the dependency, the result is that if :trackable is not included, then no expiration is set on the token! What-the-huh!!?

The confirmation process should happen in a consistent manner, regardless of the presence of the :trackable module. A review of the Devise internals shows that checking if a user has signed in (i.e. @resource.has_attribute?(:sign_in_count) && @resource.sign_in_count > 0) is not an indicator of the user having been confirmed! Rather, it is the presence of a timestamp in the :confirmed_at attribute that indicates if a user has confirmed their account. (See: Devise::Models::Confirmable#confirm and Devise::Models::Confirmable#confirmed?)

Furthermore, the Devise::ConfirmationsController#show action simply and concisely checks for the presence of validation errors on the user resource, as it handles checking for already confirmed users at the model level. (The Devise::Models::Confirmable#confirm method sets errors on the user's :email attribute, if that user has already been confirmed).

We're already using the same Devise::Models::Confirmable::ClassMethods#confirm_by_token method in:

devise_token_auth/app/controllers/devise_token_auth/confirmations_controller.rb

Lines 3 to 4 in 38d27cf

	def show
	@resource = resource_class.confirm_by_token(params[:confirmation_token])

Therefore, it shouldn't really be necessary to check for specific attributes (or their values) on the user instance (e.g. :sign_in_count, :confirmed_at, etc...); just check for errors on the returned user instance like the Devise::ConfirmationsController#show action does. The question is, what does a failure response from the controller action look like? It currently is just a 404 Not Found, but presumably it would call DeviseTokenAuth::ApplicationController#render_error, passing in the relevant validation errors where appropriate.

Lastly, it seems like the tests in the confirmations_controller_test.rb not actually test most model level attributes; instead the tests should assert that various requests (each in known states) lead to the correct set of controller responses given those states.

Specifically, the tests for :trackable attributes should not exist at all here:

devise_token_auth/test/controllers/devise_token_auth/confirmations_controller_test.rb

Lines 55 to 65 in 38d27cf

	test 'the sign_in_count should be 1' do
	assert @resource.sign_in_count == 1
	end
	test 'User shoud have the signed in info filled' do
	assert @resource.current_sign_in_at?
	end
	test 'User shoud have the Last checkin filled' do
	assert @resource.last_sign_in_at?
	end

And additional tests are needed to check the various possible validation errors on the user that could happen when calling Devise::Models::Confirmable::ClassMethods#confirm_by_token.

---

I do intend on making a PR for all this, but I'm hoping to collect some feedback in the meantime.

[Evan-M]

@lynndylanhurley, @MaicolBen, @zachfeldman, @dks17, @KelseyDH, thoughts? ☝️
(Any feedback appreciated from all!)

[MaicolBen]

the result is that if :trackable is not included, then no expiration is set on the token!

Agreed with that, it's awful.

And agreed with your planned changes. If with resource.errors.empty? is enough to check if the user already confirmed, go ahead with the change.

I prefer e2e tests like rspec request tests because they don't depend on internals, but it's hard to migrate the current test suite.

[dks17]

Question. Is the life cycle of the current version coming at the end and to go further new version of this gem should be released with reconsidering workflow, rspec, resolved issues (and, of course new ones) and etc?

[MaicolBen]

We aren't planning to change the current workflow, the next big milestone is #990 and it should work with the current workflow. If you have in mind a different workflow, open an issue and we discuss it.

[dikond]

To maintainers - thank you all for the gem and efforts to keep it up!
@Evan-M Thanks for bringing it all together 👍 And a special thank you for discovering race condition which you described here. It saved a lot of debugging time ❤️

I really like how simple devise controller looks using #errors hope to see it one day here also :)

I just came here to say that currently if the user was already confirmed we are returning an expired token with account_confirmation_success set to true, but it should be set to false I believe. Ideally, we need to give some error message also, e.g. confirmation link is expired or email already confirmed or it just an invalid link.

[mathportillo]

I am dealing with confirmation in my projects right now, and have two comments:

• clicking on the confirmation link a second time, results in generating a token that does not work. It should work or do not generate at all if the confirmation link is "used". When my rails server redirects to my web app, the web app has no way of knowing if the confirmation link was used first or second time, neither if the headers provided will work or not (prior to using it and failing/succeeding).

• I feel bad about auth headers showing auth headers as query params on the confirmation landing page. I think the proper way to confirm an email is to redirect to the front-end with a token and from there make an AJAX request to confirm the token. This is slightly more work but makes more sense to me.

The first item may be an unrelated bug and if so can be disconsidered.
The second item may be a behaviour to be discussed in another issue, and if so should be disconsidered.
Regardless, I found it wouldn't hurt to describe both here because maybe it can help Evan in his implementation

[experimatt]

+1 I just encountered this issue. I feel like more gracefully handling when a confirmed user clicks the confirm email link a second time would be a significant improvement.

[gamesover]

meet this issue.

I have a feeling that both devise and devise_token_auth are no longer actively maintained.
The two are wonderful gems for rails.

[MaicolBen]

Resolved by #1557