Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Show "Reset password token is invalid" error message immediately on /resource/password/edit #2183

Closed
wants to merge 1 commit into from

5 participants

@TylerRick

I think it would be less confusing and frustrating to users if we showed the "Reset password token
is invalid" error message immediately when they first get to the /resource/password/edit page
rather than waiting until after they've taken the time to enter their new password twice and
submit the form to tell them about the problem.

This behavior also seems more consistent with pull request #1902

Tyler Rick If user access /resource/password/edit?reset_password_token=token wit…
…h an invalid

reset_password_token, let them know immediately as soon they get there rather than waiting until
*after* they've taken the time to enter their new password twice and submit the form to tell them
about the problem.

Also redirect them to the "Forgot your password?" page so that they can request a new one instead of
leaving them wondering what to do next.
b149f42
@carlosantoniodasilva
Collaborator

To me this is unnecessary extra overhead. If the user is here with a reset password token, but it's not valid, it's highly likely the user is trying to do something he shouldn't be doing, otherwise a normal workflow would just work :tm:. Anyway, we can wait a bit for more feedback. Thanks @TylerRick.

@josevalim
Owner

I agree with @carlosantoniodasilva ! Thanks @TylerRick !

@josevalim josevalim closed this
@TylerRick

Hi, thanks for considering my pull request. Yes, it's possible they are trying to do something they shouldn't be doing (like brute forcing a reset password token), but it would still prevent them from doing anything, so I don't think it's really any less secure than before.

But it's also possible that it was an honest user error, in which case I would think the slight extra overhead is justified in order to prevent user confusion. For example, they may have copied and pasted the link incorrectly from the email, causing some of the last characters from the token to be missing. Or they may have already already used that reset password link to change their password once and then expected that the same link would work again the next time they needed to reset their password. The sooner we can detect an error condition and tell the user about it, the easier it is for them to understand what they did wrong and how to fix it.

Anyway, thanks again for considering this change and for your work on Devise.

@cliffrowley

I think I agree with @TylerRick - you can't really predict what users will do, even when they're doing what they're supposed to. Although I'd argue that most worthwhile email clients will present a clickable link which the user won't have to copy and paste, I've seen users do some pretty silly things and in this case it'd be relatively easy for someone to wander off the happy path. I don't think that the password reset feature will generate enough "overhead" to warrant sacrificing the user experience, even if it is minor.

I think what I'm trying to say is that in this case I don't think that:

If the user is here with a reset token, but it's not valid, it's highly likely the user is trying to do something he shouldn't be doing

Is a very safe assumption to make, since this is a re-entry point from an external application and not a tightly controlled part of the process. That said, it's not really critical because the user will find out pretty quickly.

Given the choice, since we're really talking about minimal overhead, I'd err on the side of telling the user immediately that something is wrong. At least, that's how I'd design my process.

@josevalim
Owner

Ok, let's do it then. Although I would prefer to set a flash message and redirect then mangling with full_messages. We already have a filter anyway:

https://github.com/plataformatec/devise/blob/master/app/controllers/devise/passwords_controller.rb#L51

@brycesenz

I altered my passwords controller for exactly this reason. My code is based off of this post: https://groups.google.com/forum/?fromgroups=#!topic/plataformatec-devise/GnDhPrZQD6M

# GET /resource/password/edit?reset_password_token=abcdef
def edit
  self.resource = resource_class.find_or_initialize_with_error_by(:reset_password_token, params[:reset_password_token])
  if resource.errors.empty?
    render_with_scope :edit
  else
    set_flash_message :notice, :invalid_reset_token
    redirect_to new_session_path(resource_name)
  end
end

It also makes controller testing a lot easier, since the logic for redirecting the user on a token mismatch is explicit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 18, 2012
  1. If user access /resource/password/edit?reset_password_token=token wit…

    Tyler Rick authored
    …h an invalid
    
    reset_password_token, let them know immediately as soon they get there rather than waiting until
    *after* they've taken the time to enter their new password twice and submit the form to tell them
    about the problem.
    
    Also redirect them to the "Forgot your password?" page so that they can request a new one instead of
    leaving them wondering what to do next.
This page is out of date. Refresh to see the latest.
View
7 app/controllers/devise/passwords_controller.rb
@@ -21,8 +21,11 @@ def create
# GET /resource/password/edit?reset_password_token=abcdef
def edit
- self.resource = resource_class.new
- resource.reset_password_token = params[:reset_password_token]
+ self.resource = resource_class.find_or_initialize_with_error_by(:reset_password_token, params[:reset_password_token])
+ if resource.errors[:reset_password_token].any?
+ flash[:error] = resource.errors.full_message(:reset_password_token, resource.errors[:reset_password_token].first)
+ redirect_to new_user_password_path
+ end
end
# PUT /resource/password
View
10 test/integration/recoverable_test.rb
@@ -132,6 +132,16 @@ def reset_password(options={}, &block)
assert_redirected_to "/users/sign_in"
end
+ test 'not authenticated user with an invalid reset password token should not be able to visit the edit_user_password page' do
+ get edit_user_password_path(:reset_password_token => 'something_invalid')
+ assert_response :redirect
+ assert_redirected_to "/users/password/new"
+
+ get "/users/password/new"
+ assert_have_selector '#flash_error'
+ assert_contain /Reset password token(.*)invalid/
+ end
+
test 'not authenticated user with invalid reset password token should not be able to change his password' do
user = create_user
reset_password :reset_password_token => 'invalid_reset_password'
Something went wrong with that request. Please try again.