"Lost password?" functionality ("Change password" functionality possibly embedded in case)

[joar]

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

[joar]

*Hacky formatting*


-  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

[Elrond]

(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."

[Christopher Allan Webber]

-  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 :(

[Elrond]

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.

[Sebastian Spaeth]

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?

[Elrond]

Joar is a bit too busy. :-)

[Alejandro Villanueva]

Im Already working on this, on the next days i'll send the patch
:)

[Alejandro Villanueva]

0005-Adding-forgot-password-functionality.patch.tar.gz

[Alejandro Villanueva]

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

[Elrond]

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!

[Alejandro Villanueva]

-  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.

[Christopher Allan Webber]

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.

[Christopher Allan Webber]

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)

[Elrond]

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.

[Christopher Allan Webber]

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.

[Alejandro Villanueva]

0001-Adding-fotgot-password-functionality.patch.tar.gz

[Alejandro Villanueva]

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.

[Christopher Allan Webber]

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

[Christopher Allan Webber]

Ping! Any update on this?

[Alejandro Villanueva]

cant work on this until the next week, ... pong!!!

[Alejandro Villanueva]

0001-Adding-fotgot-password-functionality_fixed.patch.tar.gz

[Alejandro Villanueva]

-  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 =)

[Will Kahn-Greene]

We release 0.0.4, so I'm bumping this to 0.0.5.

[Christopher Allan Webber]

Oh man! I forgot to review this. Reviewing today!

[Caleb Davis]

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)

[Christopher Allan Webber]

Sounding and looking great!

[Caleb Davis]

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?

[Caleb Davis]

claimed for refactoring

[Caleb Davis]

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.

[Caleb Davis]

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)

[Christopher Allan Webber]

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 :))

[Caleb Davis]

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 :)

[Christopher Allan Webber]

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!

[Christopher Allan Webber]

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

[Will Kahn-Greene]

The original url for this bug was ​http://bugs.foocorp.net/issues/357 .
Relations:
#107: blocks