Opened 12 years ago
Last modified 12 years ago
#72 closed defect (FIXED)
"Lost password?" functionality ("Change password" functionality possibly embedded in case)
|Reported by:||joar||Owned by:||Caleb Davis|
A user needs to be able to reset and recover his/her password. When doing so, my initial plan is to - Generate a new password \* Generate a password hash for the newly generated password \* Overwrite the old hash with the new hash \* Set some kind of "user\_has\_forgotten\_password" or "requested\_to\_change\_password" value so that the user is pushed towards changing his/her password, thus avoiding any security issues related to the transfer of the password in plain text to the user. - Email the password to the user \* Ask the user to change his/her password \* Reset the "requested\_to\_change\_password" flag \* Update the generated password with the new password
Change History (37)
comment:1 by , 12 years ago
comment:2 by , 12 years ago
(setting target to 0.0.3 as per meeting.) Have only quickly read the description... Just make sure, that the feature can't be abused to annoy users. "Oh heh, I know someones user/email, let's send him a new password for fun."
comment:3 by , 12 years ago
- If someone else says "user forgot their password" and enters the username, if we override the real password, doesn't that lock the user out? - Maybe at this point since we're emailing the user the password we should just email the user some kind of **separate password field** you can recover the password from (that then gets nullified later)? - Should we provide some sort of crappy question and answer thing in addition for the question and answer thing? Fun fact though, I hate those things :(
comment:4 by , 12 years ago
paroneayea - Other sites send you a token url, which allows you to set a new password. :) And PLEASE no stupid questions, which just are alike another password. I tend to choose a stupid question and retype my password. ;o) There is already a field for a token in the db, and somewhere some code to generate it. The field should probably be cleared when it has been used. Possibly clear it after some timeout also.
comment:5 by , 12 years ago
Let's NOT overwrote the password when clicking the "forgot password" link. Anyone else could make my current password invalid, very annoying. Ideally, if someone else clicks it, I shouldn't need to do anything (except ignoring an email) to just keep on going. I propose: 1) User clicks "Forgot password link" and enters username or email. If such user exists, this will create a random token akin to the account activation token, maybe we can even use that mechanism for it? Do NOT revel to the "clicker" if the request was successfull or not (to not reveal registered email addresses). 2) System sends out email to registered email address with activation token. Either user ignores it (it will have to expire and be deleted at some point) and we are done, or the user actually clicks the link. This should allow the user to set a new password. I am thinking we should be sharing lots of functionality with the account registration procedure here. E.g. account registration could not require an initial password (making the signup process very painless), and ask for the password when a user clicks the account activation link. The same activation mechanism could then be used for forgotten passwords. We only would need to store some expiration date with each activation token and occasionally clean up the table. Am I making sense?
comment:6 by , 12 years ago
Joar is a bit too busy. :-)
comment:7 by , 12 years ago
Im Already working on this, on the next days i'll send the patch :)
by , 12 years ago
comment:8 by , 12 years ago
|Status:||New → In Progress|
Finally here is the patch, the process is described below: - the user clics the forgot password link in the login templates - enter his username/email and an email is sent to their email whit an url to change their password - the users clics on the url (it contains a token) the forms to change the password is shown - finally if the token is correct and passwords match we change their password and show a success page
comment:9 by , 12 years ago
Hi, a few comments on your patch: - First: Looks quite good generally - Please post the "do not allow duplicate email adress" part of your patch on the relevant bug (`#320 </issues/320>`_) not here. - Why do you add another fp\_verification\_key to the db model instead of reusing the existent one? Are there any abuse cases that I don't see? - Can you try to indent the continuation lines on the render\_to\_response lines? - I don't see error handling on the verify\_forgot\_password/GET/user\_or\_verifykey\_wrong case? - The POST case doesn't seem to handle the "user does not exist" case, it seems? Just my quick observations. If I'm wrong, please correct me!
comment:10 by , 12 years ago
- i will remove the part of bug `#320 </issues/320>`_ - fp\_verification\_key was added because is another process so i like to keep things separate, there will be only problems if the user hasn't verified their email and try to change their password via "forgot password" since the vertification\_key will be overwritten and the url sended to verified their email will no longer match, but if you think we can use verification\_key field i will modified the patch - will indent lines, cwebber suggested to be pep8 compilant so i will :) - will fix when the user provides an id but its wrong send 404 or user does not exist Working on that.
comment:11 by , 12 years ago
I actually think it's a good idea to have separate values for the forgot password verification key and the email verification key. It's not a real harm to create a second value in the database. What I think is that the verification\_key should be renamed to email\_verification\_key via a migration.
comment:12 by , 12 years ago
Elrond has pointed out that we should probably have this field expire. It would be good if we could add a datetime field like: fp\_verification\_expires You can use a timedelta to set this 14 days ahead of the present day or so: :: >>> import datetime >>> datetime.datetime.now() + datetime.timedelta(days=10) datetime.datetime(2011, 7, 4, 8, 41, 8, 502139)
comment:13 by , 12 years ago
Note, that the normal verification\_token also should expire (meaning the whole account expires). And IMHO the expiration is a "new feature" and probably should have its own ticket.
comment:14 by , 12 years ago
That's a good point. Done, new ticket here: `http://bugs.foocorp.net/issues/394 <http://bugs.foocorp.net/issues/394>`_ Adding the expiration aspect is not a requirement for completing this ticket.
by , 12 years ago
comment:15 by , 12 years ago
Here is the patch fixed, it adds more cheking as Elrond pointed, i also added the fp\_token\_expire field when the user request an url to change their password the datetime.datetime.now() + datetime.timedelta(days=10) is added and the code from bug `#320 </issues/320>`_ was removed. later we should change rendering simple templates whit a string and use the messaging framework to avoid having templates like this.
comment:16 by , 12 years ago
Hm... so it's looking good! But a few comments: - No reason to have a print statement in send\_fp\_verification\_email - u'Sorry, the username doesn't exists' -> u"Sorry, that username doesn't exist" - Could you do comments like: :: # comment instead of :: #comment - I'm not sure what's going on with the GET vs POST stuff in verify\_forgot\_password, what's the difference here? They mostly look like the same thing? - Please write a migration for the new fields - Check your spacing around dictionaries for PEP-8 compliance. EG: :: 'fp_verification_key':unicode, should look like: :: 'fp_verification_key': unicode, Otherwise else, looking great, as far as I can tell! I look forward to merging this soon :) EDITED: formatting :P
comment:17 by , 12 years ago
|Milestone:||0.0.3 → 0.0.4|
comment:17 by , 12 years ago
Ping! Any update on this?
comment:18 by , 12 years ago
cant work on this until the next week, ... pong!!!
by , 12 years ago
comment:19 by , 12 years ago
- print statement removed - String fixed - Comments fixed - We check the same in GET and POST to make sure anything has changed when the form is submmited - I wrote a migration but i dont know if it's good enough could you help on this :D - Dictionary spacing fixed I havent update my repo for a while and is a completely mess if there anything else wrong let me know it this week i do have some extra free time =)
comment:20 by , 12 years ago
|Milestone:||0.0.4 → 0.0.5|
We release 0.0.4, so I'm bumping this to 0.0.5.
comment:21 by , 12 years ago
Oh man! I forgot to review this. Reviewing today!
comment:22 by , 12 years ago
Alejandro, really great stuff here! I took your ball and ran with it, hoping to integrate this feature into the next release, but I don't know if there is still time. I added a few things - translations - css grid stuff so the forms look like our newer ones - I think there was a change in how the globals were managed since you made your patches - now it's gmg\_globals.app\_config['global\_foo'] - Some spelling and grammar for the UI bits - your migrations were spot on! All I had to do was change the number from 3 to 6 :) - made the error model more generic, meaning instead of saying things like, 'this user doesn't exist' we either pretend everything was successful (sending the password email) or give a 404 error (incorrect usernames, ids, or tokens). - added checks for expired tokens I wasn't sure what the post/get differentiation in verify\_forgot\_password was about, but then I saw you re-use the function by calling it from the password changing form. All we need now are unit tests. Here's what I've got so far: [https://gitorious.org/\ :sub:`cfdv/mediagoblin/cfdvs-mediagoblin/commits/f357\_lost\_password\_functionality](https://gitorious.org/`\ cfdv/mediagoblin/cfdvs-mediagoblin/commits/f357\_lost\_password\_functionality)
comment:23 by , 12 years ago
Sounding and looking great!
comment:24 by , 12 years ago
I sat down tonight to write unit tests but I got cranky because I didn't want to duplicate more of the email verification code -- in this case the testing code. So, I see two options: 1. the tests get written, we close this ticket, and refactoring goes on a new ticket ... OR 2. we impose a refactoring requirement on this ticket. On the one hand, a few tests is all that stands between this feature and the next release -- we could pay down the debt later, but on the other hand we strengthen and simplify security with some auth code re-use. My vote is for the latter. What say you?
comment:25 by , 12 years ago
claimed for refactoring
comment:26 by , 12 years ago
|Milestone:||0.0.5 → 0.1.0|
comment:26 by , 12 years ago
refactoring successful! No more code duplication due to GET/POST conditionals. Now verify\_forgot\_password proceeds mostly method-agnostically, and hands off the GET/POST conditional responsibility to another function. [https://gitorious.org/\ :sub:`cfdv/mediagoblin/cfdvs-mediagoblin/commits/f357\_lost\_password\_functionality](https://gitorious.org/`\ cfdv/mediagoblin/cfdvs-mediagoblin/commits/f357\_lost\_password\_functionality) Now, for tests.
comment:27 by , 12 years ago
tests added. Merge requested! [https://gitorious.org/\ :sub:`cfdv/mediagoblin/cfdvs-mediagoblin/commits/f357\_lost\_password\_functionality](https://gitorious.org/`\ cfdv/mediagoblin/cfdvs-mediagoblin/commits/f357\_lost\_password\_functionality)
comment:28 by , 12 years ago
That's a lot of code and I wasn't fully able to review tonight. I leave a question here though. :) Is have-email-forgot-password a good addition here? Do we need it? Could it be problematic? Not sure, but that crossed my mind, thought I'd dump it here. Maybe it is good. (Not a helpful comment tonight :))
comment:29 by , 12 years ago
Sure, what a blur of activity this weekend! Okay, so potential problems include: - annoying emails if someone puts in your email or username by accident or maliciously - security risk? If the email is sent without encryption, a malicious agent could intercept the email and take over the account. - a bunch of extra code to maintain We should weigh these concerns against the benefits of change-password, and potential problems if we do not allow change-password. I think it's a great addition, but I am clearly biased here :)
comment:30 by , 12 years ago
It probably is a worthwhile addition. I see facebook does something similar? Anyway, I guess I can't think of how it really would be a security risk. You've compromised a user's email account... well they're already pretty screwed. Okay, thanks for helping me think that through. I'll try to review this branch shortly!
comment:31 by , 12 years ago
|Status:||In Progress → Closed|
I made a few tweaks, but almost all of them are minor. The only one I want to note here is that there were some variables referring to the request.GET/request.POST as "formdata", and that's a bit inaccurate. Oh also, when making an empty field in the database as being done in these migrations, better to mark them as None than empty strings. Anyway, clarified that. Merged and pushed! **GREAT** work on this bug, both to Alejandro and Caleb here! :D
comment:32 by , 11 years ago
The original url for this bug was http://bugs.foocorp.net/issues/357 .