ForgotPassword view needs improvement

[spaetz]

1: Our Lost password function reveals if a user name or

an email address is registered, which can be considered a data leak.
Should we simply respond and say: "If you have an account here, we
have send you your email"?

2: username and email search is case sensitive. Is that

intended? Should we provide helper functions or how do we deal with
that?

[spaetz]

In any case, here is a branch that restructures the function, simplifying the code (removing indention) and removing some mongo-limitations. It does NOT fix above limitations though.

branch formerge/555_forgot_password_improvement
repo: ​git://gitorious.org/~spaetz/mediagoblin/spaetz-mediagoblin.git

[spaetz]

THe above branch solves now most issues:

Restructure ForgotPassword view

1) Remove mongo limitations (no 'or' when querying for either username
or email).

2) Lost password function revealed if an user name or email address

is registered, which can be considered a data leak.
Leaking user names is OK, they are public anyway, but don't reveal
lookup success in case the lookup happened by email address.
Simply respond: "If you have an account here, we have send you your email"?

3) username and email search was case sensitive. Made username search

case insensitive (they are always stored lowercase in the db).
Keep email-address search case sensitive for now. This might need
further discussion

4) Remove a whole bunch of indention in the style of:

      if no error:
            ...
            if no error:
                ...
                if no error:
                    actually do something in the regular case

by restructuring the function.

[spaetz]

After an Elrond review, this branch was now updated. Review and comments and merging would be appreciated:
formerge/555_forgot_password_improvement.

The big change since the last update is that the casing is normalized as part of the form field validator,
so the view code has not to be bothered with it.

5) Outsource the sanity checking for username and email fields into the
   validator function. This way, we get automatic case sanity checking
   and sanitizing for all required fields.

[Christopher Allan Webber]

Reading this now. Wow, this line is really interesting:

    found_by_email = '@' in request.form['username']

I guess it's totally logical. The only case in which it might not make sense is if we end up building a flexible login system, but then again, lots of this will need to change.

This looks really sensible to me; just doing one more pass and then I'm merging probably.

[Christopher Allan Webber]

Oh, erk. I merged this foolishly without running the tests or actually testing. That'll show me... it broke like half the tests! I rolled it back quickly.

[Christopher Allan Webber]

As far as I can tell, there's something wrong with the way authentication is working in this branch, and logging in is not working right somehow, which means a whole bunch of tests broke.

[spaetz]

Aye, that was a fun one. The test suite uses a 5 char password by default. Previously the "change password" form required a password between 6-30 chars. My commit made it consistent for all forms (e.g. login) requiring >=6 chars. This broke login of the test user in the test suite.

Rebased, fixed some other issues that had crept in and fixed tests too. I decided to require 5-30 chars for the password now. If we bump it to 6 chars again, we need to fix too short passwords in the test suite.

made sure that all tests pass this time.

[spaetz]

The originally filed bugs have been fixed by the push that happened right now.