Opened 10 years ago
Closed 10 years ago
#555 closed defect (fixed)
ForgotPassword view needs improvement
|Reported by:||spaetz||Owned by:|
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
Change History (9)
comment:1 by , 10 years ago
comment:2 by , 10 years ago
THe above branch solves now most issues:
Restructure ForgotPassword view
1) Remove mongo limitations (no 'or' when querying for either username
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
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.
comment:3 by , 10 years ago
After an Elrond review, this branch was now updated. Review and comments and merging would be appreciated:
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.
comment:4 by , 10 years ago
comment:5 by , 10 years ago
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.
comment:6 by , 10 years ago
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.
comment:7 by , 10 years ago
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.
comment:8 by , 10 years ago
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.
comment:9 by , 10 years ago
|Status:||new → closed|
The originally filed bugs have been fixed by the push that happened right now.
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.