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

Unexpected reconfirmable behaviors especially trying to re-send confirmation email #2060

Closed
mcbsys opened this Issue Sep 12, 2012 · 7 comments

Comments

Projects
None yet
4 participants

mcbsys commented Sep 12, 2012

I'm testing the reconfirmable feature with Rails 3.2.8 with Devise 2.1.2, upgraded from 1.5.3. I've come across a few unexpected behaviors:

  1. When the user changes his email, the confirmation email is sent to the unconfirmed_email, but the email body greets the user with the old email: Welcome <%= @resource.email %>!. I can fix that in views/devise/mailer/confirmation_instructions (see below).
  2. If the user doesn't get the instructions, he may try the "Resend confirmation instructions" link with his new email address. He will get the message, "[new email] not found". The WHERE clause here should be something like WHERE "users"."email" = [form_email] OR "users"."unconfirmed_email" = [form_email]. Assuming that will work that way someday, I guess that means I should index unconfirmed_email in my migration.
  3. On the Resend confirmation page, maybe the user will think to enter his old email address to get it to send confirmation instructions to his new email address. He will get the message, "[old email] was already confirmed. Please try signing in." Maybe confirmed_at should be set to nil when a (re-)confirmation is pending?
  4. So the user finally tries signing in using his old email address. This works. In my app, he can now go to the Edit User screen, where he will see his old email address. "What? I thought I changed that!" But if he changes it again, he will get another confirmation email.
  5. Some of this wouldn't be so confusing if the user was informed of the flow. Is there a built-in message I should be displaying? If not, I will to add some code to the Users controller so that when the user changes his email, the flash message tells him, "A confirmation email has been sent to [new email]. Until you confirm the new email, you can continue to log in as [old email]." And maybe I'll #show the user unconfirmed_email as "New email (pending confirmation)".

Here is my updated confirmation_instructions.html.erb:

<% if @resource.unconfirmed_email %>
  <p>Welcome <%= @resource.unconfirmed_email %>!</p>
  <p>Please confirm your new email address by clicking on the link below:</p>
<% else %>
  <p>Welcome <%= @resource.email %>!</p>
  <p>Please confirm your account by clicking on the link below:</p>
<% end %>

<p><%= link_to 'Confirm my account', confirmation_url(@resource, :confirmation_token => @resource.confirmation_token) %></p>

mcbsys commented Sep 13, 2012

Okay re. point 5 above, displaying an update message, it looks like this is already handled if you use the default Devise RegistrationsController. My workaround using my own Users controller:

Add this to config/locales/devise.en.yml (note that update_needs_confirmation is not mentioned in the upgrade instructions):

devise:
  registrations:
    update_needs_confirmation: "You updated your account successfully, but we need to verify your new email address. Please check your email and click on the confirm link to finalize confirming your new email address."

app/controllers/users_controller.rb could look something like this:

class UsersController < ApplicationController
  def update
    @user = User.find(params[:id])
    changing_email = params[:user][:email] != @user.email
    if @user.update_attributes(params[:user])
      flash_msg = (changing_email && @user.pending_reconfirmation?) ?
          t("devise.registrations.update_needs_confirmation") : 
          t("devise.registrations.updated")
      flash[:success] = flash_msg
      redirect_to @user
    else
      render 'edit'
    end
  end
end

I'd be glad to know if there is a better way!

Owner

josevalim commented Sep 13, 2012

All the five points are valid with a small notice on 3). We should not clear up confirmed_at because the user then may no longer be able to access the app. Although we could be smarter and show a better message (this e-mail is confirmed, did you mean to send a confirmation to X?).

mcbsys commented Sep 13, 2012

That sounds fine for 3). IMO the user is less likely to get to 3) if he can re-send instructions using his new email (point 2).

Just starting to use this feature. I was surprised to see that the confirmation email is sent to the new email address. I was expecting to get an email at my old address to confirm the change. What if someone gets my password and changes the email to his/her address, then I don't even know what happened and my account is lost forever.

To summarize:
Change email->send confirmation to old email->confirm->current email = unconfirmed_email->log user out->optional(get success email to your new email address)

Am I totally misunderstanding what the feature is supposed to do?

mcbsys commented Sep 27, 2012

@GeorgeCrecoukias, my understanding is that the purpose of email confirmation, whether at initial signup or when changing the email address, is to make sure that the user typed a valid email address that is under the user's control before allowing it to be used for sign-in.

I think I understand your concern, but IMO, confirming with the old address first could create more problems than it solves. What if your old email account has expired, or the provider is defunct, or just down for a while? What if the person who got your app password also controls your old email account--he or she unlikely to confirm the change, and you will never get back into the app.

I suppose one could argue for sending an email to the old address AFTER the change is confirmed saying, "Your email address has been changed to x@y.com. If you did not request this change, please log in to your account, change it back, and change your password," but to me that seems to go beyond what this function is supposed to do.

yeah you're right. It would probably require more to make it a complete solution. I just remember that this is how it worked somewhere else so I thought it would work the same.

thanks for the 2 cents

@josevalim josevalim closed this in 839e8fc Dec 13, 2012

how could i do that using clearance gem.

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