Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple Forgot Password requests invalidating previously requested Forgot Password links? #73

Open
SherriAlexander opened this issue Jan 27, 2015 · 14 comments
Assignees
Labels

Comments

@SherriAlexander
Copy link

Hey there! Hope everyone is staying warm and dry with this crazy blizzard! :)

We've been testing out the new Forgot Password functionality, and Mark noticed something interesting. If we send in a Forgot Password request, then we send in another Forgot Password request, and then we try to use the link from the first request to reset the password, we're getting an Ajax error when we submit the form.

According to the API_ENDPOINTS.md documentation, a 404 error for a /registrations PUT request should return a payload of an error message like "User not found". But instead, it seems to be a genuine URL not found type of error:

http://artbot-api.herokuapp.com/registrations 
Failed to load resource: the server responded with a status of 404 (Not Found)
Error: Not Found
jqXHR status: 404 Not Found

So it seems that the first password request token is invalidated when the second password request token is requested? Is this desired behavior? If so, how should we handle the error, and differentiate it from the other (more valid) 404 errors?

Thanks!

@heymarkreeves
Copy link
Collaborator

@mailbackwards & @desigonz We're just ironing out a few wrinkles I found in testing and then we'll be able to turn these enhancements over to you. Thanks!

@mailbackwards
Copy link
Collaborator

Hi Sherri and Mark,

I think this is the desired behavior...a user should only have one password reset token, and I think it would be best for it to be the most recently generated token. The error occurs because it can't find a user with the given reset token (I am getting a "user not found" error when testing-- see screenshot below). This is the error it's "supposed" to be giving right now, but if there's a more helpful error it could send, or if I'm misunderstanding the problem, let me know.

image

Thanks!

@heymarkreeves
Copy link
Collaborator

Hi, Liam! I guess the question is, if we get back that error due to an older or invalid token, do we automatically translate that to a "token invalid error - link expired" or do we display the authoritative "user does not exist" error, which would be inaccurate and misleading from the user's standpoint, as they clicked a link they received in an email generated by Artbot.

Should we look to the API to break out the two errors separately? Make our error handling more generic? Let us know your thoughts.

Thanks!

Mark

@mailbackwards
Copy link
Collaborator

Sure that makes sense, I can change the messaging in the API for the cases when it doesn't find a user with a matching password token. I can have it send a 404 with a "Token expired" message (or something similar) instead of a "User not found" message. Does that work, or is there a better response/message to send?

If no token is provided at all, it responds with a 422 error and "reset_password_token not set" error message.

@SherriAlexander
Copy link
Author

Hey there, Liam! That sounds great to me.

I'm curious as to how you were getting the "User not found" error though -- when I tried it from inside the app, the error returned was just "Not Found". Did you change something on your end, or is that doing a direct API call some other way? Thanks!

@mailbackwards
Copy link
Collaborator

Hi Sherri-- I was getting the error from the response to the /registrations API call. Now when I query it, I get the following:

image

Is the same happening for you? Thanks!

@SherriAlexander
Copy link
Author

Hey there, Liam!

Hm. When I go into that same panel in the Chrome dev tools (Network, click on the PUT request on the left, then select the Preview tab), I see the error. But if I use my error logging function...

logAjaxError: function (jqXHR, error, errorThrown, isErrorAjaxResponse) {
        console.log("Error: " + errorThrown);
        console.log("jqXHR status: " + jqXHR.status + " " + jqXHR.statusText);
        if (isErrorAjaxResponse) {
            console.log("jqXHR response: " + jqXHR.responseText);
        }
    }

...then it just returns the generic "404 Not Found".

@mailbackwards
Copy link
Collaborator

Hi Sherri,
Ah, I think I see. Hopefully this will be a better way to do it: now, it should now give you the same 422 Unprocessable Entity error as the other form problems (such as the password being too short). The error message is as follows:

image

Does that approach work? Let me know if I'm not following something! Thanks.

@SherriAlexander
Copy link
Author

Hey there, Liam!

I think this should work -- I'll give it a try soon and let you know what I find. Thanks!

@SherriAlexander
Copy link
Author

Hi Liam!

Hm. It looks like we have a couple different ways that we're formatting returned error messages.

For example, the JSON returned if you try to sign up with an already-existing email address and a too-short password is formatted like this:

{"email":["has already been taken"],"password":["is too short (minimum is 8 characters)"]}

The JSON returned if you try to sign in with an unknown email address is:

{"error":"User does not exist"}

I'm trying to tweak my error handling functionality to be more flexible, but at the same time I'm hoping to keep things standardized. :) Thoughts?

@mailbackwards
Copy link
Collaborator

I could change the latter response to {"email":["not found"]}, and an invalid password error to {"password":["is invalid"]}, both as 422 responses. That would make them more consistent formatting. Would that work?

@SherriAlexander
Copy link
Author

Hey there, Liam! That would be great -- if the error "keys" match up with the ID of a field, that will make it much easier to display the error messages in the appropriate places on the forms. I'm also trying to work out a fallback where if there isn't a field match, it creates a separate error message to be displayed in the page.

I think this means that we'd also want to change the password reset error to be "reset_password_token": ["is invalid"] instead of {error: {reset_password_token: ["is invalid"]}}?

Take care,

--Sherri

@mailbackwards
Copy link
Collaborator

correct-- that should be set now. Thanks!

@SherriAlexander
Copy link
Author

Awesome, thank you! I'll do a bit more testing to see what I can come up with. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants