Ugh. On further investigation, the forgot password feature doesn't
work even if there aren't multiple users with the same email address.
I found that exception I alluded to. Try this:
1) Create a new user
2) Log out and choose Forgot Password
3) Type in the user's email
4) Paste the url and choose a new password
5) New password works. Repeat from step 2.
6) MultipleObjectsReturned at /account/password_reset_key/
ae4667a26a1c040454eb2f0b68be2b7c196f0055/
get() returned more than one PasswordReset -- it returned 2! Lookup
parameters were {'temp_key__exact':
u'ae4667a26a1c040454eb2f0b68be2b7c196f0055'}
This is the real consequence of my issue #2 above. The problem is
given away by the differences between these lines:
293: if not PasswordReset.objects.filter(temp_key=temp_key,
reset=False).count() == 1:
306: password_reset = PasswordReset.objects.get
(temp_key__exact=temp_key)
293 is done in the validation step, 306 in the save step. The form
passes validation because only one password reset key exists such that
reset=False. Then, when the form is saved, it breaks because an old
password reset still exists but does not meet the reset=False
requirement.
The naive fix would be to just add reset=False to this query as well.
But that only serves to mask the real problem, which is that these old
password confirmations are still there. The validation step should be
removed anyway, since this is not something the user has any control
over.
Oho, and I just discovered a way to break the app the way I broke it
in issue #2, except this time I only need one user. It's quite
simple.
1) Type your email into the forgotten password form
2) Go back and do it again
3) follow either email's link.
4) Experience the same unresponsive form from issue #3.
Why? Because it's failing validation on that hidden field, because
there are two identical password reset keys in the database.
Well, since I've done all this research, I might as well make a
patch. I'll have to patch django-email-confirmation and the pinax
account app simultaneously. Lets see if I can hit all these issues.
On Oct 26, 7:17 pm, Nick Retallack <[email protected]> wrote:
> I posted a task about this, but it got all jumbled up, so I'll
> rephrase.http://code.pinaxproject.com/tasks/task/497/
>
> There are a bunch of issues here. #3 is the most important one, but
> I'd really like to see #1 and #2 done too.
>
> 1) Wouldn't it be nice if verifying your email address logged you in
> automatically?
>
> Let assume you have enabled the ACCOUNT_EMAIL_VERIFICATION setting.
> That means that when a new user tries to sign up, they are asked to
> check their email for the confirmation code / link. When they click
> the link, it would be nice if that logged them in, instead of just
> dropping them on an "Email Address Confirmed" page and hoping they
> think to click Login in the top right at this point.
>
> Of course, you don't have their password. You'd have to do a custom
> authentication backend trick to make django.contrib.auth happy.
> Currently, the account/urls.py routes this url to a view in the
> emailconfirmation app, and leaves it at that.
>
> url(r'^confirm_email/(\w+)/$',
> 'emailconfirmation.views.confirm_email', name="acct_confirm_email"),
>
> This then calls EmailConfirmation.objects.confirm_email
> (confirmation_key), which brings me to the next thing.
>
> 2) If #1, then all email confirmation keys for this email should be
> consumed when used.
>
> If you do go ahead and make the email confirmation log you in with the
> associated account, the confirmations become dangerous. You can't
> leave stale ones around. Right now, I can keep refreshing this email
> confirmed page with the same confirmation key and it just confirms it
> repeatedly. I can even pull out old confirmation keys and they work
> too. I suppose the keys expire eventually, but I think it would be
> best if they were just deleted when they are no longer useful. When
> an email is successfully confirmed, it should delete all the
> associated confirmation keys.
>
> 3) Two users shouldn't be allowed to verify the same email address.
>
> Why not? Because it breaks the forgotten password feature. The
> forgotten password form only asks for an email address. If two users
> have the same verified email address, there is no way to decide which
> one is the right one. This used to cause a straight up exception when
> you entered an email address that two people have, but it seems to
> have been fixed in a strange way:
>
> Now, it loops through the users (line 260 of account/forms.py) and
> creates a password reset key for each user, then emails them all out.
> They all end up the same though, since they are all generated by
> hashing the same data, and they are all sent to the same email address
> too, so the result is a bunch of identical emails sent to the same
> person. Silly. At least it doesn't send anything to people who have
> added but not verified an email address.
>
> However, this behavior just shoots itself in the foot because as soon
> as you get to the ResetPasswordKeyForm, we have this on line 291 of
> account/forms.py:
>
> def clean_temp_key(self):
> temp_key = self.cleaned_data.get("temp_key")
> if not PasswordReset.objects.filter(temp_key=temp_key,
> reset=False).count() == 1:
> raise forms.ValidationError(_("Temporary key is
> invalid."))
> return temp_key
>
> That's right, it marks the key invalid because more than one of them
> exist in the database. To make matters worse, this error doesn't even
> show up in the form for some reason. Maybe because it's erroring on a
> hidden field? They just get presented with the same empty form again
> with no indication of what went wrong. In my opinion, the user
> shouldn't even be presented with a form that is impossible to
> validate, so this problem should have been caught on the GET, not the
> POST.
>
> I can't see how this forgotten password feature is going to work
> unless we prevent users from verifying the same email address. Here's
> how I'd do it:
>
> It's ok for two people to try to verify the same email address.
> Otherwise, someone could just 'attempt' to verify a lot of peoples'
> email addresses and then those people would not be able to sign up
> with their own email addresses or add them to their existing account.
> If they clicked the link in the confirmation email, all they would do
> is confirm their email address as belonging to someone else, and then
> they would get that person's messages. Currently there is no downside
> for the exploiter. However, if my feature #1 was implemented, this
> behavior would be like giving out free keys to enter your account.
>
> However, once someone verifies an email address, it should be
> impossible for anyone else to verify the same email address. All
> existing attempts to verify that email address should be deleted, and
> future attempts to even add that email address should be met with
> "That email address is taken". The SignupForm should include a
> similar check, as should the OpenIDSignupForm.
>
> 4) You shouldn't be allowed to just jam on that resend-verification
> link forever.
>
> Currently, I could add someone else's email address and then click
> resend-verification a million times to flood their inbox with spam.
> Or if they're using gmail which collapses these all into one message,
> I could just hit it once a day to be sure the message never leaves
> their sight. Eventually, messages from the origin website might get
> marked as spam if people get annoyed at this. So, maybe there should
> be some kind of timeout or cap on this behavior. Or maybe it should
> notify the administrators if it catches you using this feature an
> unrealistically large number of times.
>
> Fortunately, if you implement #3, any email address that is already
> verified by a user on the site will be immune from this sort of abuse.
>
> 5) The notification that a confirmation was sent is kind of dumb.
>
> Once you log in after confirming your email, you see a message
> indicating that the email was sent. No duh, I just confirmed it.
> However, since I wasn't logged in at the time, it couldn't show me
> this user message before. I think what we need here is a session-
> based flash message, like the kind in rails, so we can display the
> message when it is generated. There are a bunch of django snippets
> out there that do this. We should use one.
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Pinax Core Development" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, visit this group at
http://groups.google.com/group/pinax-core-dev?hl=en
-~----------~----~----~----~------~----~------~--~---
