On 05/17/2015 02:07 AM, Andrew Shadura wrote:
# HG changeset patch
# User Andrew Shadura <and...@shadura.me>
# Date 1431821238 -7200
# Sun May 17 02:07:18 2015 +0200
# Node ID 8d43a8174c960779437c2d8de7a0906a8cd14128
# Parent cb911e90e205bdb18fc2e2bd66549ea388d00413
secure password reset implementation
This is a better implementation of password reset function, which
doesn't involve sending a new password to the user's email address
in clear text, and at the same time doesn't require state storing.
Very cool. It would be nice if you stated more clearly what is wrong
about the old implementation.
The idea behind is to generate a token which is dependent on the
user state at the time before the password change takes place,
so the token is one-time and can't be reused, and also to bind the
token the the session.
The token is calculated as SHA1 hash of the following:
* user's identifier (number, not a name)
* timestamp
* hashed user's password
* session identifier
* per-application secret
We use numeric user's identifier, as it's fixed and doesn't change,
so renaming users doesn't affect the mechanism. Timestamp is added
to make it possible to limit the token's validness (currently not
implemented),
Please just hardcode something like 24 hours.
and we don't want users to be able to fake that field
easily. Hashed user's password is needed to prevent using the token
again once the password has been changed. Session identifier is
an additional security measure to ensure someone else stealing the
token can't use it. Finally, per-application secret is just another
way to make it harder for an attacker to guess all values in an
attempt to generate a valid token.
Are you or others aware of "standard" ways of handling password resets?
Something that indicates we are doing it correctly or that we can reuse?
When the token is generated, an anonymous user is directed to a
confirmation page where the timestamp and the usernames are already
preloaded, so the user needs to specify the token. User can either
click the link in the email if it's really them reading it, or to type
the token manually (note: email template needs to be improved).
Yes - please fix the email template ;-) This change without a change of
the template would be wrong.
Using the right token in the same session as it was requested directs
the user to a password change form, where the user is supposed to
specify a new password (twice, of course). Upon completing the form
(which is POSTed) the password change happens and a notification
mail is sent.
diff --git a/kallithea/controllers/login.py b/kallithea/controllers/login.py
--- a/kallithea/controllers/login.py
+++ b/kallithea/controllers/login.py
@@ -43,7 +43,8 @@ from kallithea.lib.auth_modules import i
from kallithea.lib.base import BaseController, render
from kallithea.lib.exceptions import UserCreationError
from kallithea.model.db import User, Setting
-from kallithea.model.forms import LoginForm, RegisterForm, PasswordResetForm
+from kallithea.model.forms import \
+ LoginForm, RegisterForm, PasswordResetForm, PasswordResetConfirmationForm
from kallithea.model.user import UserModel
from kallithea.model.meta import Session
@@ -241,12 +242,12 @@ class LoginController(BaseController):
error_dict = {'recaptcha_field': _msg}
raise formencode.Invalid(_msg, _value, None,
error_dict=error_dict)
- UserModel().reset_password_link(form_result)
+ redirect_link = UserModel().reset_password_link(form_result)
h.flash(_('Your password reset link was sent'),
category='success')
- return redirect(url('login_home'))
+ return redirect(redirect_link)
- except formencode.Invalid, errors:
+ except formencode.Invalid as errors:
return htmlfill.render(
render('/password_reset.html'),
defaults=errors.value,
@@ -258,17 +259,49 @@ class LoginController(BaseController):
return render('/password_reset.html')
def password_reset_confirmation(self):
- if request.GET and request.GET.get('key'):
+ if request.GET:
+ c.data = dict(
+ username = request.GET.get('username'),
+ timestamp = request.GET.get('timestamp'),
+ token = request.GET.get('token')
+ )
+ if c.data['token']:
+ try:
+ log.debug("data = %s" % c.data)
+ if UserModel().reset_password_confirm(c.data):
+ return render('/password_reset_confirmation.html')
+ else:
+ h.flash(_('Invalid password reset token'),
+ category='error')
+ return redirect(url('reset_password'))
+ except Exception as e:
+ log.error(e)
+ return redirect(url('reset_password'))
I don't like the pattern of redirecting on serious internal errors and
have been trying to get rid of it in other places. I think it generally
is much better to just show an error message so the error can be fixed
... and the user easily can use back to go back and retry.
+ else:
+ return render('/password_reset_confirmation.html')
+ elif request.POST:
+ password_reset_confirmation_form =
PasswordResetConfirmationForm()()
+ c.data = dict(
+ username = request.POST.get('username'),
+ password = request.POST.get('password'),
+ password_confirm = request.POST.get('password_confirm'),
+ timestamp = request.POST.get('timestamp'),
+ token = request.POST.get('token')
+ )
try:
- user = User.get_by_api_key(request.GET.get('key'))
- data = dict(email=user.email)
- UserModel().reset_password(data)
- h.flash(_('Your password reset was successful, '
- 'new password has been sent to your email'),
- category='success')
- except Exception, e:
- log.error(e)
- return redirect(url('reset_password'))
+ log.debug("data = %s" % c.data)
+ form_result =
password_reset_confirmation_form.to_python(dict(request.POST))
+ UserModel().reset_password(c.data)
+ h.flash(_('Successfully updated password'),
+ category='success')
+ except formencode.Invalid as errors:
+ return htmlfill.render(
+ render('/password_reset_confirmation.html'),
+ defaults=errors.value,
+ errors=errors.error_dict or {},
+ prefix_error=False,
+ encoding="UTF-8",
+ force_defaults=False)
return redirect(url('login_home'))
diff --git a/kallithea/model/forms.py b/kallithea/model/forms.py
--- a/kallithea/model/forms.py
+++ b/kallithea/model/forms.py
@@ -205,6 +205,17 @@ def PasswordResetForm():
email = v.Email(not_empty=True)
return _PasswordResetForm
+def PasswordResetConfirmationForm():
+ class _PasswordResetConfirmationForm(formencode.Schema):
+ allow_extra_fields = True
+ filter_extra_fields = True
+
+ password = All(v.ValidPassword(), v.UnicodeString(strip=False, min=6))
+ password_confirm = All(v.ValidPassword(), v.UnicodeString(strip=False,
min=6))
+
+ chained_validators = [v.ValidPasswordsMatch('password',
+ 'password_confirm')]
+ return _PasswordResetConfirmationForm
def RepoForm(edit=False, old_data={}, supported_backends=BACKENDS.keys(),
repo_groups=[], landing_revs=[]):
diff --git a/kallithea/model/user.py b/kallithea/model/user.py
--- a/kallithea/model/user.py
+++ b/kallithea/model/user.py
@@ -27,7 +27,11 @@ Original author and date, and relevant c
import logging
+import hashlib
+import time
import traceback
+from pylons import config
+from pylons import session
from pylons.i18n.translation import _
from sqlalchemy.exc import DatabaseError
@@ -296,10 +300,19 @@ class UserModel(BaseModel):
user_email = data['email']
user = User.get_by_email(user_email)
+ timestamp = int(time.time())
if user:
log.debug('password reset user found %s' % user)
+ token = hashlib.sha1('%s%s%s%s%s' % (user.user_id,
+ timestamp,
+ user.password,
+ session.id,
+ config.get('app_instance_uuid'))
+ ).hexdigest()
Now we are reinventing crypto. That is dangerous. One thing: One thing
is that hashes could give conflicts in the hash but we should make sure
that we never got "collisions" in the input string - such collisions
could perhaps be used as attacks. Perhaps use \n as delimiter of the fields?
The computation should probably be moved to a function so it can be reused.
link = h.canonical_url('reset_password_confirmation',
- key=user.api_key)
+ username=user.username,
+ timestamp=timestamp,
+ token=token)
reg_type = EmailNotificationModel.TYPE_PASSWORD_RESET
body = EmailNotificationModel().get_email_tmpl(
reg_type, 'txt',
@@ -316,27 +329,45 @@ class UserModel(BaseModel):
else:
log.debug("password reset email %s not found" % user_email)
- return True
+ return h.canonical_url('reset_password_confirmation',
+ username=user.username,
+ timestamp=timestamp)
+
+ def reset_password_confirm(self, data):
+ from kallithea.lib.celerylib import tasks, run_task
+ from kallithea.lib import auth
+ user = User.get_by_username(data['username'])
+ if not user:
+ log.debug("user %s not found" % data['username'])
+ return False
+
+ token = hashlib.sha1('%s%s%s%s%s' % (user.user_id,
+ data['timestamp'],
+ user.password,
+ session.id,
+ config.get('app_instance_uuid'))
+ ).hexdigest()
+ log.debug('computed token: %s' % token)
+ log.debug('received token: %s' % data['token'])
+ return token == data['token']
def reset_password(self, data):
from kallithea.lib.celerylib import tasks, run_task
from kallithea.lib import auth
- user_email = data['email']
- user = User.get_by_email(user_email)
- new_passwd = auth.PasswordGenerator().gen_password(
- 8, auth.PasswordGenerator.ALPHABETS_BIG_SMALL)
- if user:
- user.password = auth.get_crypt_password(new_passwd)
+ user = User.get_by_username(data['username'])
+ if not user:
+ log.debug("user %s not found" % data['username'])
+
+ if data['password']:
+ user.password = auth.get_crypt_password(data['password'])
Session().add(user)
Session().commit()
- log.info('change password for %s' % user_email)
- if new_passwd is None:
- raise Exception('unable to generate new password')
+ log.info('change password for %s' % user.username)
- run_task(tasks.send_email, [user_email],
- _('Your new password'),
- _('Your new Kallithea password:%s') % (new_passwd,))
- log.info('send new password mail to %s' % user_email)
+ run_task(tasks.send_email, [user.email],
+ _('Password change notification'),
+ _('Your Kallithea has been changed'))
Your Kallithea _password_ has been changed
+ log.info('send password reset mail to %s' % user.email)
return True
diff --git a/kallithea/templates/password_reset_confirmation.html b/kallithea/templates/password_reset_confirmation.html
--- a/kallithea/templates/password_reset_confirmation.html
+++ b/kallithea/templates/password_reset_confirmation.html
@@ -0,0 +1,73 @@
+## -*- coding: utf-8 -*-
+<%inherit file="base/root.html"/>
+
+<%block name="title">
+ ${_('Password Reset Confirmation')}
+</%block>
+
+<div id="register">
+ <%include file="/base/flash_msg.html"/>
+ <div class="title withlogo">
+ %if c.site_name:
+ <h5>${_('Reset Your Password to %s') % c.site_name}</h5>
+ %else:
+ <h5>${_('Reset Your Password')}</h5>
+ %endif
We should probably also show the canonical url to make sure the user
recognizes which site we are talking about.
+ </div>
+ <div class="inner">
+ %if c.data['username']:
+ <p>${_('You are about to reset password for the user %s') %
c.data['username']}</p>
Reset _the_ password
We should however not reveal the username - at most the email address
the user provided. We should also make sure the same message is shown,
no matter if the address is recognized or not.
+ ${h.form(h.canonical_url('reset_password_confirmation'), method='post'
if c.data['token'] else 'get')}
+ <div style="display: none;">
+ ${h.hidden('username',value=c.data['username'])}
+ %if c.data['token']:
+ ${h.hidden('token',value=c.data['token'])}
+ %endif
+ %if c.data['timestamp']:
+ ${h.hidden('timestamp',value=c.data['timestamp'])}
+ %endif
+ </div>
+ %endif
+ <div class="form">
+ <!-- fields -->
+ <div class="fields">
+ %if c.data['token']:
+ <div class="field">
+ <div class="label">
+ <label for="password">${_('New Password')}:</label>
+ </div>
+ <div class="input">
+ ${h.password('password',class_='focus')}
+ </div>
+ </div>
+
+ <div class="field">
+ <div class="label">
+ <label for="password_confirm">${_('Confirm New
Password')}:</label>
+ </div>
+ <div class="input">
+ ${h.password('password_confirm',class_='focus')}
+ </div>
+ </div>
+ %else:
+ <div class="field">
+ <div class="label">
+ <label for="token">${_('Code you received in the
email')}:</label>
But there is no information about any code in the email - there is only
a URL?
We should either explicitly show the code in the mail or just ask the
user to click the link in the mail and not show the "code" input field.
+ </div>
+ <div class="input">
+ ${h.text('token',class_='focus')}
+ </div>
+ </div>
+
+ %endif
+
+ <div class="buttons">
+ <div class="nohighlight">
+ ${h.submit('send',_('Confirm'),class_="btn")}
+ </div>
+ </div>
+ </div>
+ </div>
+ ${h.end_form()}
+ </div>
+ </div>
/Mads
_______________________________________________
kallithea-general mailing list
kallithea-general@sfconservancy.org
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
