Nice - lot of improvements since the first iteration.
test_forgot_password is still failing?
Should some of the nice commit message perhaps be moved into the
documentation?
One thought: Before, the automaticly assigned password would be random
and very "secure". This makes it more easy for users to change their
password to something insecure ... whatever that means. There is thus
perhaps an increased need for a way to configure password security
policy instead of just the existing hardcoded defaults in the templates.
Inline comments:
On 07/19/2015 03:35 PM, Andrew Shadura wrote:
# HG changeset patch
# User Andrew Shadura <[email protected]>
# Date 1431821238 -7200
# Sun May 17 02:07:18 2015 +0200
# Node ID 98cb64feddfb89f106f66763462061fd2ca3f412
# Parent f103b1a2383bc4fba5d28f9732ba832025e3bf00
secure password reset implementation
Please use a first line in the usual format that helps with the grouping
when creating release notes.
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.
The old implementation generated a new password and sent it
in clear text to whatever email assigned to the user currently,
invalidating the old password the very moment a new one is requested.
Apart from potential insecurity, this makes it possible for anyone
to disrupt users' workflow by repeatedly resetting their passwords.
The idea behind this proposed implementation is to generate
This will very soon no longer just be a proposal! ;-)
an authentication 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 to
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 hard
coded to 24h), 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.
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).
Is that still a TODO? Should it be done in this patch or later?
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
@@ -42,7 +42,8 @@ from kallithea.lib.base import BaseContr
from kallithea.lib.exceptions import UserCreationError
from kallithea.lib.utils2 import safe_str
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
@@ -197,12 +198,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:
This is probably a good idea to consistently do everywhere ... in a
separate changeset.
return htmlfill.render(
render('/password_reset.html'),
defaults=errors.value,
@@ -214,17 +215,45 @@ 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(
+ email = request.GET.get('email'),
+ timestamp = request.GET.get('timestamp'),
+ token = request.GET.get('token')
+ )
Please add a trailing comma to the last item (when each item has its own
line and the ending parens is on the next line) - that makes it a
one-liner diff to add a new line and it won't be forgotten. (And the
same below.)
+ if c.data['token']:
+ log.debug("data = %s" % c.data)
Perhaps add a bit extra text here to make the debug log entry more
useful for the admin debugging something?
Also, please use
log.debug("data = %s", c.data)
to avoid unnecessary string expansion when debugging is disabled.
+ 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'))
+ else:
+ return render('/password_reset_confirmation.html')
+ elif request.POST:
+ password_reset_confirmation_form =
PasswordResetConfirmationForm()()
+ c.data = dict(
+ email = request.POST.get('email'),
+ 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/lib/utils2.py b/kallithea/lib/utils2.py
--- a/kallithea/lib/utils2.py
+++ b/kallithea/lib/utils2.py
@@ -34,6 +34,7 @@ import uuid
import datetime
import urllib
import binascii
+import hashlib
import webob
import urlobject
@@ -170,6 +171,9 @@ def generate_api_key():
return binascii.hexlify(os.urandom(20))
+def hash_values(*args):
+ return hashlib.sha1('\0'.join([str(arg) for arg in args])).hexdigest()
+
Docstring?
+1 for using a 0 delimiter (which however also put some constraints on
which data this function will work 'fully correctly' on).
There is no need for creating the list for join - just use the generator
directly as
join(str(arg) for arg in args)
Perhaps even assume that the caller is aware of the types and already
converted all arguments to strings?
def safe_int(val, default=None):
"""
Returns int() of val if val is not convertable to int use default
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,13 +27,16 @@ Original author and date, and relevant c
import logging
+import time
import traceback
+from pylons import config
+from pylons import session
from pylons.i18n.translation import _
from sqlalchemy.exc import DatabaseError
from kallithea import EXTERN_TYPE_INTERNAL
-from kallithea.lib.utils2 import safe_unicode, generate_api_key,
get_current_authuser
+from kallithea.lib.utils2 import safe_unicode, generate_api_key,
get_current_authuser, hash_values
from kallithea.lib.caching_query import FromCache
from kallithea.model import BaseModel
from kallithea.model.db import User, UserToPerm, Notification, \
@@ -280,6 +283,13 @@ class UserModel(BaseModel):
from kallithea.lib.hooks import log_delete_user
log_delete_user(user.get_dict(), cur_user)
+ def reset_password_token(self, user, timestamp):
+ return hash_values(user.user_id,
+ timestamp,
+ user.password,
+ session.id,
+ config.get('app_instance_uuid'))
+
def reset_password_link(self, data):
from kallithea.lib.celerylib import tasks, run_task
from kallithea.model.notification import EmailNotificationModel
@@ -287,18 +297,24 @@ 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 = self.reset_password_token(user, timestamp)
link = h.canonical_url('reset_password_confirmation',
- key=user.api_key)
+ email=user_email,
+ timestamp=timestamp,
+ token=token)
reg_type = EmailNotificationModel.TYPE_PASSWORD_RESET
body = EmailNotificationModel().get_email_tmpl(
reg_type, 'txt',
user=user.short_contact,
+ reset_token=token,
reset_url=link)
html_body = EmailNotificationModel().get_email_tmpl(
reg_type, 'html',
user=user.short_contact,
+ reset_token=token,
reset_url=link)
log.debug('sending email')
run_task(tasks.send_email, [user_email],
@@ -307,27 +323,52 @@ class UserModel(BaseModel):
else:
log.debug("password reset email %s not found" % user_email)
- return True
+ return h.canonical_url('reset_password_confirmation',
+ email=user_email,
+ 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_email(data['email'])
+ if not user:
+ log.debug("user with email %s not found" % data['email'])
+ return False
+
+ now = int(time.time())
+ timestamp = int(data['timestamp'])
+
+ if (timestamp is None) or (timestamp > now):
+ log.debug('timestamp is None or from the future')
+ return False
+
+ # TODO for now, hardcode 24h, need to be a setting
+ if (now - timestamp) > 86400:
+ log.debug('token expired')
+ return False
+
+ token = self.reset_password_token(user, data['timestamp'])
+ 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_email(data['email'])
+ if not user:
+ log.debug("user with email %s not found" % data['email'])
+
+ 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)
Perhaps make it more clear where in the process we are:
'changing password for %s'
- 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 password has been changed'))
+ log.info('send password reset mail to %s' % user.email)
return True
diff --git a/kallithea/templates/email_templates/password_reset.html b/kallithea/templates/email_templates/password_reset.html
--- a/kallithea/templates/email_templates/password_reset.html
+++ b/kallithea/templates/email_templates/password_reset.html
@@ -3,8 +3,10 @@
<h4>${_('Hello %s') % user}</h4>
-<p>${_('We received a request to create a new password for your account.')}</p>
-<p>${_('You can generate it by clicking following URL')}:</p>
+<p>${_('We have received a request to reset the password for your
account.')}</p>
+<p>${_('To reset the password, click the following link')}:</p>
"reset" sounds a bit like the system is assigning a password. Perhaps
use wording like "To set a new password"? (But admittedly, setting a new
password is a way to reset it ...)
<p><a href="${reset_url}">${reset_url}</a></p>
-<p>${_("Please ignore this email if you did not request a new password .")}</p>
+<p>${_("Should you not be able to use the link above, please type the following code into the
password reset form")}: <code>${reset_token}</code></p>
+
+<p>${_("If it weren't you who requested the password reset, just disregard this
message.")}</p>
diff --git a/kallithea/templates/email_templates/password_reset.txt
b/kallithea/templates/email_templates/password_reset.txt
--- a/kallithea/templates/email_templates/password_reset.txt
+++ b/kallithea/templates/email_templates/password_reset.txt
@@ -3,9 +3,11 @@
${_('Hello %s') % user|n,unicode}
-${_('We received a request to create a new password for your account.')|n,unicode}
-${_('You can generate it by clicking following URL')|n,unicode}:
+${_('We have received a request to reset the password for your
account..')|n,unicode}
+${_('To reset the password, click the following link')|n,unicode}:
${reset_url|n,unicode}
-${_("Please ignore this email if you did not request a new password .")|n,unicode}
+${_("Should you not be able to use the link above, please type the following code
into the password reset form")|n,unicode}: ${reset_token|n,unicode}
+
+${_("If it weren't you who requested the password reset, just disregard this
message.")|n,unicode}
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
+ </div>
+ <div class="inner">
+ %if c.data['email']:
+ <p>${_('You are about to reset password for the email address %s') %
c.data['email']}</p>
Perhaps
... the password ...
+ ${h.form(h.canonical_url('reset_password_confirmation'), method='post'
if c.data['token'] else 'get')}
+ <div style="display: none;">
+ ${h.hidden('email',value=c.data['email'])}
+ %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>
+ </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
[email protected]
http://lists.sfconservancy.org/mailman/listinfo/kallithea-general
