#548 Add a change password link for local auth
Merged 8 years ago by pingou. Opened 8 years ago by farhaan.
farhaan/pagure change-password  into  master

@@ -0,0 +1,28 @@ 

+ """versioning_passwords

+ 

+ Revision ID: 1b6d7dc5600a

+ Revises: 3b441ef4e928

+ Create Date: 2016-01-13 07:57:23.465676

+ 

+ """

+ 

+ # revision identifiers, used by Alembic.

+ revision = '1b6d7dc5600a'

+ down_revision = '3b441ef4e928'

+ 

+ from alembic import op

+ import sqlalchemy as sa

+ import sqlalchemy.orm

+ from pagure.lib import model

+ 

+ 

+ def upgrade():

+     engine = op.get_bind().engine

+     session = sa.orm.scoped_session(sa.orm.sessionmaker(bind=engine))

+     session.query(model.User).update(

+         {model.User.password: '$1$' + model.User.password}, synchronize_session=False)

+     session.commit()

+ 

+ 

+ def downgrade():

+     raise ValueError("Password can not be downgraded")

file modified
+1
@@ -309,6 +309,7 @@ 

              FAS.logout()

              flask.flash("You are no longer logged-in")

      elif APP.config.get('PAGURE_AUTH', None) == 'local':

+         import pagure.ui.login as login

          login.logout()

      return flask.redirect(return_point)

  

file modified
+39 -1
@@ -1,17 +1,23 @@ 

  # -*- coding: utf-8 -*-

  

  """

-  (c) 2015 - Copyright Red Hat Inc

+  (c) 2015-2016 - Copyright Red Hat Inc

  

   Authors:

     Pierre-Yves Chibon <pingou@pingoured.fr>

+    Farhaan Bukhsh <farhaan.bukhsh@gmail.com>

  

  """

  

  import random

  import string

+ import bcrypt

  

+ import hashlib

+ import pagure

  from pagure.lib import model

+ from kitchen.text.converters import to_unicode, to_bytes

+ from cryptography.hazmat.primitives import constant_time

  

  

  def id_generator(size=15, chars=string.ascii_uppercase + string.digits):
@@ -61,3 +67,35 @@ 

      )

  

      return query.all()

+ 

+ 

+ def generate_hashed_value(password):

+     """ Generate hash value for password

+     """

+     return '$2$' + bcrypt.hashpw(to_unicode(password), bcrypt.gensalt())

+ 

+ 

+ def check_password(entered_password, user_password, seed=None):

+     """ Version checking and returning the password

+     """

+     if not user_password.count('$') >= 2:

+         raise pagure.exceptions.PagureException(

+             'Password of unknown version found in the database'

+         )

+ 

+     _, version, user_password = user_password.split('$', 2)

+ 

+     if version == '2':

+         password = bcrypt.hashpw(to_unicode(entered_password), user_password)

+ 

+     elif version == '1':

+         password = '%s%s' % (to_unicode(entered_password), seed)

+         password = hashlib.sha512(password).hexdigest()

+ 

+     else:

+         raise pagure.exceptions.PagureException(

+             'Password of unknown version found in the database'

+         )

+ 

+     return constant_time.bytes_eq(

+         to_bytes(password), to_bytes(user_password))

file modified
+15
@@ -86,3 +86,18 @@ 

          'Confirm password  <span class="error">*</span>',

          [wtforms.validators.Required(), same_password]

      )

+ 

+ class ChangePasswordForm(wtf.Form):

+     """ Form to reset one's password in the local database. """

+     old_password = wtforms.PasswordField(

+         'Old Password  <span class="error">*</span>',

+         [wtforms.validators.Required()]

+     )

+     password = wtforms.PasswordField(

I believe same_password expect this name

+         'Password  <span class="error">*</span>',

+         [wtforms.validators.Required()]

+     )

+     confirm_password = wtforms.PasswordField(

+         'Confirm password  <span class="error">*</span>',

+         [wtforms.validators.Required(), same_password]

+     )

@@ -0,0 +1,31 @@ 

+ {% extends "master.html" %}

+ {% from "_formhelper.html" import render_bootstrap_field %}

+ 

+ {% block title %}Change password{% endblock %}

+ {%block tag %}home{% endblock %}

+ 

+ {% block content %}

+ 

+ <div class="container">

+   <div class="row">

+     <div class="col-md-8 col-md-offset-2">

+       <div class="card m-t-3">

+         <div class="card-header">

+           <strong>Change password</strong>

+         </div>

+         <form action="{{ url_for('change_password')}}" method="post">

+           {{ render_bootstrap_field(form.old_password) }}

+           {{ render_bootstrap_field(form.password) }}

+           {{ render_bootstrap_field(form.confirm_password) }}

+           {{ form.csrf_token }}

+           <input class="btn btn-primary" type="submit" value="Update"

+             title="Update description">

+           <input type="button" value="Cancel" class="btn btn-default"

+             onclick="history.back();">

+         </form>

+       </div>

+     </div>

+   </div>

+ </div>

+ 

+ {% endblock %}

@@ -1,5 +1,5 @@ 

  {% extends "master.html" %}

- {% from "_formhelper.html" import render_field_in_row %}

+ {% from "_formhelper.html" import render_bootstrap_field %}

  

  {% block title %}Change password{% endblock %}

  {% set tag = "home" %}
@@ -16,7 +16,7 @@ 

            <form action="{{ url_for('.reset_password', token=token)

                  }}" method="post">

              {{ render_bootstrap_field(form.password) }}

-             {{ render_field_in_row(form.confirm_password) }}

+             {{ render_bootstrap_field(form.confirm_password) }}

              <input class="btn btn-primary" type="submit" value="Update">

              <input type="button" value="Cancel" class="btn btn-default" onclick="history.back();">

              {{ form.csrf_token }}

file modified
+1 -1
@@ -78,7 +78,7 @@ 

                </a>

              </li>

              {% endif %}

-           {% endif %}

+          {% endif %}

          </ul>

        </div>

      </nav>

@@ -88,7 +88,12 @@ 

          </form>

        </div>

      </div>

- 

+     <div class="card m-b-3">

+         {% if config.get('PAGURE_AUTH')=='local' %}

+             <a href="{{ url_for('change_password', username=g.fas_user.username) }}">Change password</a>

+         {% endif %}

+     </div>

    </div>

  </div>

+ 

  {% endblock %}

file modified
+95 -26
@@ -1,10 +1,11 @@ 

  # -*- coding: utf-8 -*-

  

  """

-  (c) 2014, 2015 - Copyright Red Hat Inc

+  (c) 2014-2016 - Copyright Red Hat Inc

  

   Authors:

     Pierre-Yves Chibon <pingou@pingoured.fr>

+    Farhaan Bukhsh <farhaan.bukhsh@gmail.com>

  

  """

  
@@ -12,6 +13,7 @@ 

  import hashlib

  import datetime

  import urlparse

+ import bcrypt

  

  import flask

  from sqlalchemy.exc import SQLAlchemyError
@@ -21,7 +23,8 @@ 

  import pagure.lib.login

  import pagure.lib.model as model

  import pagure.lib.notify

- from pagure import APP, SESSION

+ from pagure import APP, SESSION, cla_required

+ from pagure.lib.login import generate_hashed_value, check_password

  

  # pylint: disable=E1101

  
@@ -44,9 +47,7 @@ 

              flask.flash('Email address already taken.', 'error')

              return flask.redirect(flask.request.url)

  

-         password = '%s%s' % (

-             form.password.data, APP.config.get('PASSWORD_SEED', None))

-         form.password.data = hashlib.sha512(password).hexdigest()

+         form.password.data = generate_hashed_value(form.password.data)

  

          token = pagure.lib.login.id_generator(40)

  
@@ -66,19 +67,17 @@ 

              SESSION.flush()

  

          try:

-             SESSION.flush()

+             SESSION.commit()

              send_confirmation_email(user)

              flask.flash(

                  'User created, please check your email to activate the '

                  'account')

-         except SQLAlchemyError as err:

+         except SQLAlchemyError as err:  # pragma: no cover

              SESSION.rollback()

              flask.flash('Could not create user.')

              APP.logger.debug('Could not create user.')

              APP.logger.exception(err)

  

-         SESSION.commit()

- 

          return flask.redirect(flask.url_for('auth_login'))

  

      return flask.render_template(
@@ -89,7 +88,7 @@ 

  

  @APP.route('/dologin', methods=['POST'])

  def do_login():

-     """ Lo the user in user.

+     """ Log the user in user.

      """

      form = forms.LoginForm()

      next_url = flask.request.args.get('next_url')
@@ -98,20 +97,36 @@ 

  

      if form.validate_on_submit():

          username = form.username.data

-         password = '%s%s' % (

-             form.password.data, APP.config.get('PASSWORD_SEED', None))

-         password = hashlib.sha512(password).hexdigest()

- 

          user_obj = pagure.lib.search_user(SESSION, username=username)

-         if not user_obj or user_obj.password != password:

+         if not user_obj:

+             flask.flash('Username or password invalid.', 'error')

+             return flask.redirect(flask.url_for('auth_login'))

+ 

+         try:

+             password_checks = check_password(

+                 form.password.data, user_obj.password,

+                 seed=APP.config.get('PASSWORD_SEED', None))

+         except pagure.exceptions.PagureException as err:

+             APP.logger.exception(err)

+             flask.flash('Username or password of invalid format.', 'error')

+             return flask.redirect(flask.url_for('auth_login'))

+ 

+         if not password_checks:

              flask.flash('Username or password invalid.', 'error')

              return flask.redirect(flask.url_for('auth_login'))

+ 

          elif user_obj.token:

              flask.flash(

                  'Invalid user, did you confirm the creation with the url '

                  'provided by email?', 'error')

              return flask.redirect(flask.url_for('auth_login'))

+ 

          else:

+ 

+             if not user_obj.password.startswith('$2$'):

+                 user_obj.password = generate_hashed_value(form.password.data)

+                 SESSION.add(user_obj)

+ 

              visit_key = pagure.lib.login.id_generator(40)

              now = datetime.datetime.utcnow()

              expiry = now + datetime.timedelta(days=30)
@@ -180,12 +195,13 @@ 

              flask.flash('Username invalid.', 'error')

              return flask.redirect(flask.url_for('auth_login'))

          elif user_obj.token:

-             current_time = datetime.datetime.now()

-             invalid_period = user_obj.updated_on + datetime.timedelta(minutes=3)

+             current_time = datetime.datetime.utcnow()

+             invalid_period = user_obj.updated_on + \

+                 datetime.timedelta(minutes=3)

              if current_time < invalid_period:

                  flask.flash('An email was sent to you less than 3 minutes ago, '

-                     'did you check your spam folder? Otherwise, '

-                     'try again after some time.', 'error')

+                             'did you check your spam folder? Otherwise, '

+                             'try again after some time.', 'error')

                  return flask.redirect(flask.url_for('auth_login'))

  

          token = pagure.lib.login.id_generator(40)
@@ -197,7 +213,7 @@ 

              send_lostpassword_email(user_obj)

              flask.flash(

                  'Check your email to finish changing your password')

-         except SQLAlchemyError as err:

+         except SQLAlchemyError as err:  # pragma: no cover

              SESSION.rollback()

              flask.flash(

                  'Could not set the token allowing changing a password.',
@@ -232,9 +248,8 @@ 

  

      if form.validate_on_submit():

  

-         password = '%s%s' % (

-             form.password.data, APP.config.get('PASSWORD_SEED', None))

-         user_obj.password = hashlib.sha512(password).hexdigest()

+         user_obj.password = generate_hashed_value(form.password.data)

+ 

          user_obj.token = None

          SESSION.add(user_obj)

  
@@ -242,7 +257,7 @@ 

              SESSION.commit()

              flask.flash(

                  'Password changed')

-         except SQLAlchemyError as err:

+         except SQLAlchemyError as err:  # pragma: no cover

              SESSION.rollback()

              flask.flash('Could not set the new password.', 'error')

              APP.logger.debug(
@@ -256,13 +271,67 @@ 

          form=form,

          token=token,

      )

- 

- 

  #

  # Methods specific to local login.

  #

  

  

+ @APP.route('/password/change/', methods=['GET', 'POST'])

+ @APP.route('/password/change', methods=['GET', 'POST'])

+ @cla_required

+ def change_password():

+     """ Method to change the password for local auth users.

+     """

+ 

+     form = forms.ChangePasswordForm()

+     user_obj = pagure.lib.search_user(

+         SESSION, username=flask.g.fas_user.username)

+ 

+     if not user_obj:

+         flask.abort(404, 'User not found')

+ 

+     if form.validate_on_submit():

+ 

+         try:

+             password_checks = check_password(

+                 form.old_password.data, user_obj.password,

+                 seed=APP.config.get('PASSWORD_SEED', None))

+         except pagure.exceptions.PagureException as err:

+             APP.logger.exception(err)

+             flask.flash(

+                 'Could not update your password, either user or password '

+                 'could not be checked', 'error')

+             return flask.redirect(flask.url_for('auth_login'))

+ 

+         if password_checks:

+             user_obj.password = generate_hashed_value(form.password.data)

+             SESSION.add(user_obj)

+ 

+         else:

+             flask.flash(

+                 'Could not update your password, either user or password '

+                 'could not be checked', 'error')

+             return flask.redirect(flask.url_for('auth_login'))

+ 

+         try:

+             SESSION.commit()

+             flask.flash(

+                 'Password changed')

+         except SQLAlchemyError as err:  # pragma: no cover

+             SESSION.rollback()

+             flask.flash('Could not set the new password.', 'error')

+             APP.logger.debug(

+                 'Password change  - Error setting new password.')

+             APP.logger.exception(err)

+ 

+         return flask.redirect(flask.url_for('auth_login'))

+ 

+     return flask.render_template(

+         'login/password_recover.html',

+         form=form,

+     )

+ 

+ 

  def send_confirmation_email(user):

      """ Sends the confirmation email asking the user to confirm its email

      address.

file modified
+1
@@ -26,3 +26,4 @@ 

  straight.plugin==1.4.0-post-1

  trollius-redis

  wtforms

+ py-bcrypt

@@ -0,0 +1,553 @@ 

+ # -*- coding: utf-8 -*-

+ 

+ """

+  (c) 2016 - Copyright Red Hat Inc

+ 

+  Authors:

+    Pierre-Yves Chibon <pingou@pingoured.fr>

+    Farhaan Bukhsh <farhaan.bukhsh@gmail.com>

+ 

+ """

+ 

+ __requires__ = ['SQLAlchemy >= 0.8']

+ import pkg_resources

+ 

+ import datetime

+ import hashlib

+ import json

+ import unittest

+ import shutil

+ import sys

+ import tempfile

+ import os

+ 

+ import pygit2

+ from mock import patch

+ 

+ sys.path.insert(0, os.path.join(os.path.dirname(

+     os.path.abspath(__file__)), '..'))

+ 

+ 

+ import pagure.lib

+ import tests

+ from pagure.lib.repo import PagureRepo

+ 

+ import pagure.ui.login

+ 

+ 

+ class PagureFlaskLogintests(tests.Modeltests):

+     """ Tests for flask app controller of pagure """

+ 

+     def setUp(self):

+         """ Set up the environnment, ran before every tests. """

+         super(PagureFlaskLogintests, self).setUp()

+ 

+         pagure.APP.config['TESTING'] = True

+         pagure.APP.config['EMAIL_SEND'] = False

+         pagure.APP.config['PAGURE_AUTH'] = 'local'

+         pagure.SESSION = self.session

+         pagure.ui.SESSION = self.session

+         pagure.ui.app.SESSION = self.session

+         pagure.ui.login.SESSION = self.session

+ 

+         self.app = pagure.APP.test_client()

+ 

+     def test_new_user(self):

+         """ Test the new_user endpoint. """

+ 

+         # Check before:

+         items = pagure.lib.search_user(self.session)

+         self.assertEqual(2, len(items))

+ 

+         # First access the new user page

+         output = self.app.get('/user/new')

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>New user - Pagure</title>', output.data)

+         self.assertIn(

+             '<form action="/user/new" method="post">', output.data)

+ 

+         # Create the form to send there

+ 

+         # This has all the data needed

+         data = {

+             'user': 'foo',

+             'fullname': 'user foo',

+             'email_address': 'foo@bar.com',

+             'password': 'barpass',

+             'confirm_password': 'barpass',

+         }

+ 

+         # Submit this form  -  Doesn't work since there is no csrf token

+         output = self.app.post('/user/new', data=data)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>New user - Pagure</title>', output.data)

+         self.assertIn(

+             '<form action="/user/new" method="post">', output.data)

+ 

+         csrf_token = output.data.split(

+             'name="csrf_token" type="hidden" value="')[1].split('">')[0]

+ 

+         # Submit the form with the csrf token

+         data['csrf_token'] = csrf_token

+         output = self.app.post('/user/new', data=data, follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>New user - Pagure</title>', output.data)

+         self.assertIn(

+             '<form action="/user/new" method="post">', output.data)

+         self.assertIn('Username already taken.', output.data)

+ 

+         # Submit the form with another username

+         data['user'] = 'foouser'

+         output = self.app.post('/user/new', data=data, follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>New user - Pagure</title>', output.data)

+         self.assertIn('Email address already taken.', output.data)

+ 

+         # Submit the form with proper data

+         data['email_address'] = 'foo@example.com'

+         output = self.app.post('/user/new', data=data, follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Login - Pagure</title>', output.data)

+         self.assertIn(

+             'User created, please check your email to activate the account',

+             output.data)

+ 

+         # Check after:

+         items = pagure.lib.search_user(self.session)

+         self.assertEqual(3, len(items))

+ 

+     def test_do_login(self):

+         """ Test the do_login endpoint. """

+ 

+         output = self.app.get('/login/')

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Login - Pagure</title>', output.data)

+         self.assertIn(

+             '<form action="/dologin" method="post">', output.data)

+ 

+         # This has all the data needed

+         data = {

+             'username': 'foouser',

+             'password': 'barpass',

+         }

+ 

+         # Submit this form  -  Doesn't work since there is no csrf token

+         output = self.app.post('/dologin', data=data, follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Login - Pagure</title>', output.data)

+         self.assertIn(

+             '<form action="/dologin" method="post">', output.data)

+         self.assertIn('Insufficient information provided', output.data)

+ 

+         csrf_token = output.data.split(

+             'name="csrf_token" type="hidden" value="')[1].split('">')[0]

+ 

+         # Submit the form with the csrf token  -  but invalid user

+         data['csrf_token'] = csrf_token

+         output = self.app.post('/dologin', data=data, follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Login - Pagure</title>', output.data)

+         self.assertIn(

+             '<form action="/dologin" method="post">', output.data)

+         self.assertIn('Username or password invalid.', output.data)

+ 

+         # Create a local user

+         self.test_new_user()

+ 

+         items = pagure.lib.search_user(self.session)

+         self.assertEqual(3, len(items))

+ 

+         # Submit the form with the csrf token  -  but user not confirmed

+         data['csrf_token'] = csrf_token

+         output = self.app.post('/dologin', data=data, follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Login - Pagure</title>', output.data)

+         self.assertIn(

+             '<form action="/dologin" method="post">', output.data)

+         self.assertIn(

+             'Invalid user, did you confirm the creation with the url '

+             'provided by email?', output.data)

+ 

+         # User in the DB, csrf provided  -  but wrong password submitted

+         data['password'] = 'password'

+         output = self.app.post('/dologin', data=data, follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Login - Pagure</title>', output.data)

+         self.assertIn(

+             '<form action="/dologin" method="post">', output.data)

+         self.assertIn('Username or password invalid.', output.data)

+ 

+         # When account is not confirmed i.e user_obj != None

+         data['password'] = 'barpass'

+         output = self.app.post('/dologin', data=data, follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Login - Pagure</title>', output.data)

+         self.assertIn(

+             '<form action="/dologin" method="post">', output.data)

+         self.assertIn(

+             'Invalid user, did you confirm the creation with the url '

+             'provided by email?', output.data)

+ 

+         # Wrong password submitted

+         data['password'] = 'password'

+         output = self.app.post('/dologin', data=data, follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Login - Pagure</title>', output.data)

+         self.assertIn(

+             '<form action="/dologin" method="post">', output.data)

+         self.assertIn('Username or password invalid.', output.data)

+ 

+         # When account is not confirmed i.e user_obj != None

+         data['password'] = 'barpass'

+         output = self.app.post('/dologin', data=data, follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Login - Pagure</title>', output.data)

+         self.assertIn(

+             '<form action="/dologin" method="post">', output.data)

+         self.assertIn(

+             'Invalid user, did you confirm the creation with the url '

+             'provided by email?', output.data)

+ 

+         # Confirm the user so that we can log in

+         item = pagure.lib.search_user(self.session, username='foouser')

+         self.assertEqual(item.user, 'foouser')

+         self.assertNotEqual(item.token, None)

+ 

+         # Remove the token

+         item.token = None

+         self.session.add(item)

+         self.session.commit

+ 

+         # Check the user

+         item = pagure.lib.search_user(self.session, username='foouser')

+         self.assertEqual(item.user, 'foouser')

+         self.assertEqual(item.token, None)

+ 

+         # Login but cannot save the session to the DB due to the missing IP

+         # address in the flask request

+         data['password'] = 'barpass'

+         output = self.app.post('/dologin', data=data, follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Home - Pagure</title>', output.data)

+         self.assertIn(

+             '<a class="nav-link" href="/login/?next=http://localhost/">',

+             output.data)

+         self.assertIn(

+             'Could not set the session in the db, please report this error '

+             'to an admin', output.data)

+ 

+         # Make the password invalid

+         item = pagure.lib.search_user(self.session, username='foouser')

+         self.assertEqual(item.user, 'foouser')

+         self.assertTrue(item.password.startswith('$2$'))

+ 

+         # Remove the $2$

+         item.password = item.password[3:]

+         self.session.add(item)

+         self.session.commit

+ 

+         # Check the password

+         item = pagure.lib.search_user(self.session, username='foouser')

+         self.assertEqual(item.user, 'foouser')

+         self.assertFalse(item.password.startswith('$2$'))

+ 

+         # Try login again

+         output = self.app.post('/dologin', data=data, follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Login - Pagure</title>', output.data)

+         self.assertIn(

+             '<form action="/dologin" method="post">', output.data)

+         self.assertIn('Username or password of invalid format.', output.data)

+ 

+         # Make the password be version 1

+         item = pagure.lib.search_user(self.session, username='foouser')

+         self.assertEqual(item.user, 'foouser')

+         self.assertTrue(item.password.startswith('$2$'))

+ 

+         # V1 password

+         password = '%s%s' % ('barpass', None)

+         password = hashlib.sha512(password).hexdigest()

+         item.token = None

+         item.password = '$1$%s' % password

+         self.session.add(item)

+         self.session.commit

+ 

+         # Check the password

+         item = pagure.lib.search_user(self.session, username='foouser')

+         self.assertEqual(item.user, 'foouser')

+         self.assertTrue(item.password.startswith('$1$'))

+ 

+         # Log in with a v1 password

+         output = self.app.post('/dologin', data=data, follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Home - Pagure</title>', output.data)

+         self.assertIn(

+             '<a class="nav-link" href="/login/?next=http://localhost/">',

+             output.data)

+         self.assertIn(

+             'Could not set the session in the db, please report this error '

+             'to an admin', output.data)

+ 

+     def test_confirm_user(self):

+         """ Test the confirm_user endpoint. """

+ 

+         output = self.app.get('/confirm/foo', follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Home - Pagure</title>', output.data)

+         self.assertIn(

+             'No user associated with this token.', output.data)

+ 

+         # Create a local user

+         self.test_new_user()

+ 

+         items = pagure.lib.search_user(self.session)

+         self.assertEqual(3, len(items))

+         item = pagure.lib.search_user(self.session, username='foouser')

+         self.assertEqual(item.user, 'foouser')

+         self.assertTrue(item.password.startswith('$2$'))

+         self.assertNotEqual(item.token, None)

+ 

+         output = self.app.get(

+             '/confirm/%s' % item.token, follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Login - Pagure</title>', output.data)

+         self.assertIn(

+             'Email confirmed, account activated', output.data)

+ 

+     def test_lost_password(self):

+         """ Test the lost_password endpoint. """

+ 

+         output = self.app.get('/password/lost')

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Lost password - Pagure</title>', output.data)

+         self.assertIn(

+             '<form action="/password/lost" method="post">', output.data)

+ 

+         # Prepare the data to send

+         data = {

+             'username': 'foouser',

+         }

+ 

+         # Missing CSRF

+         output = self.app.post('/password/lost', data=data)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Lost password - Pagure</title>', output.data)

+         self.assertIn(

+             '<form action="/password/lost" method="post">', output.data)

+ 

+         csrf_token = output.data.split(

+             'name="csrf_token" type="hidden" value="')[1].split('">')[0]

+ 

+         # With the CSRF  -  But invalid user

+         data['csrf_token'] = csrf_token

+         output = self.app.post(

+             '/password/lost', data=data, follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Login - Pagure</title>', output.data)

+         self.assertIn('Username invalid.', output.data)

+ 

+         # With the CSRF and a valid user

+         data['username'] = 'foo'

+         output = self.app.post(

+             '/password/lost', data=data, follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Login - Pagure</title>', output.data)

+         self.assertIn(

+             'Check your email to finish changing your password', output.data)

+ 

+         # With the CSRF and a valid user  -  but too quick after the last one

+         data['username'] = 'foo'

+         output = self.app.post(

+             '/password/lost', data=data, follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Login - Pagure</title>', output.data)

+         self.assertIn(

+             'An email was sent to you less than 3 minutes ago, did you '

+             'check your spam folder? Otherwise, try again after some time.',

+             output.data)

+ 

+     def test_reset_password(self):

+         """ Test the reset_password endpoint. """

+ 

+         output = self.app.get('/password/reset/foo', follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Login - Pagure</title>', output.data)

+         self.assertIn('No user associated with this token.', output.data)

+         self.assertIn('<form action="/dologin" method="post">', output.data)

+ 

+         self.test_lost_password()

+         self.test_new_user()

+ 

+         # Check the password

+         item = pagure.lib.search_user(self.session, username='foouser')

+         self.assertEqual(item.user, 'foouser')

+         self.assertNotEqual(item.token, None)

+         self.assertTrue(item.password.startswith('$2$'))

+ 

+         old_password = item.password

+         token = item.token

+ 

+         output = self.app.get(

+             '/password/reset/%s' % token, follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Change password - Pagure</title>', output.data)

+         self.assertIn(

+             '<form action="/password/reset/', output.data)

+ 

+         data = {

+             'password': 'passwd',

+             'confirm_password': 'passwd',

+         }

+ 

+         # Missing CSRF

+         output = self.app.post(

+             '/password/reset/%s' % token, data=data, follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Change password - Pagure</title>', output.data)

+         self.assertIn(

+             '<form action="/password/reset/', output.data)

+ 

+         csrf_token = output.data.split(

+             'name="csrf_token" type="hidden" value="')[1].split('">')[0]

+ 

+         # With CSRF

+         data['csrf_token'] = csrf_token

+         output = self.app.post(

+             '/password/reset/%s' % token, data=data, follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Login - Pagure</title>', output.data)

+         self.assertIn('Password changed', output.data)

+ 

+     def test_change_password(self):

+         """ Test the change_password endpoint. """

+ 

+         # Not logged in, redirects

+         output = self.app.get('/password/change', follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Login - Pagure</title>', output.data)

+         self.assertIn('<form action="/dologin" method="post">', output.data)

+ 

+         user = tests.FakeUser()

+         with tests.user_set(pagure.APP, user):

+             output = self.app.get('/password/change')

+             self.assertEqual(output.status_code, 404)

+             self.assertIn('User not found', output.data)

+ 

+         user = tests.FakeUser(username='foo')

+         with tests.user_set(pagure.APP, user):

+             output = self.app.get('/password/change')

+             self.assertEqual(output.status_code, 200)

+             self.assertIn(

+                 '<title>Change password - Pagure</title>', output.data)

+             self.assertIn(

+                 '<form action="/password/change" method="post">', output.data)

+ 

+             data = {

+                 'old_password': 'foo',

+                 'password': 'foo',

+                 'confirm_password': 'foo',

+             }

+ 

+             # No CSRF token

+             output = self.app.post('/password/change', data=data)

+             self.assertEqual(output.status_code, 200)

+             self.assertIn(

+                 '<title>Change password - Pagure</title>', output.data)

+             self.assertIn(

+                 '<form action="/password/change" method="post">', output.data)

+ 

+             csrf_token = output.data.split(

+                 'name="csrf_token" type="hidden" value="')[1].split('">')[0]

+ 

+             # With CSRF  -  Invalid password format

+             data['csrf_token'] = csrf_token

+             output = self.app.post(

+                 '/password/change', data=data, follow_redirects=True)

+             self.assertEqual(output.status_code, 200)

+             self.assertIn('<title>Home - Pagure</title>', output.data)

+             self.assertIn(

+                 'Could not update your password, either user or password '

+                 'could not be checked', output.data)

+ 

+         self.test_new_user()

+ 

+         # Remove token of foouser

+         item = pagure.lib.search_user(self.session, username='foouser')

+         self.assertEqual(item.user, 'foouser')

+         self.assertNotEqual(item.token, None)

+         self.assertTrue(item.password.startswith('$2$'))

+         item.token = None

+         self.session.add(item)

+         self.session.commit()

+ 

+         user = tests.FakeUser(username='foouser')

+         with tests.user_set(pagure.APP, user):

+             output = self.app.get('/password/change')

+             self.assertEqual(output.status_code, 200)

+             self.assertIn(

+                 '<title>Change password - Pagure</title>', output.data)

+             self.assertIn(

+                 '<form action="/password/change" method="post">', output.data)

+ 

+             data = {

+                 'old_password': 'foo',

+                 'password': 'foo',

+                 'confirm_password': 'foo',

+             }

+ 

+             # No CSRF token

+             output = self.app.post('/password/change', data=data)

+             self.assertEqual(output.status_code, 200)

+             self.assertIn(

+                 '<title>Change password - Pagure</title>', output.data)

+             self.assertIn(

+                 '<form action="/password/change" method="post">', output.data)

+ 

+             csrf_token = output.data.split(

+                 'name="csrf_token" type="hidden" value="')[1].split('">')[0]

+ 

+             # With CSRF  -  Incorrect password

+             data['csrf_token'] = csrf_token

+             output = self.app.post(

+                 '/password/change', data=data, follow_redirects=True)

+             self.assertEqual(output.status_code, 200)

+             self.assertIn('<title>Home - Pagure</title>', output.data)

+             self.assertIn(

+                 'Could not update your password, either user or password '

+                 'could not be checked', output.data)

+ 

+             # With CSRF  -  Correct password

+             data['old_password'] = 'barpass'

+             output = self.app.post(

+                 '/password/change', data=data, follow_redirects=True)

+             self.assertEqual(output.status_code, 200)

+             self.assertIn('<title>Home - Pagure</title>', output.data)

+             self.assertIn('Password changed', output.data)

+ 

+     def test_logout(self):

+         """ Test the auth_logout endpoint for local login. """

+ 

+         output = self.app.get('/logout/', follow_redirects=True)

+         self.assertEqual(output.status_code, 200)

+         self.assertIn('<title>Home - Pagure</title>', output.data)

+         self.assertNotIn('You have been logged out', output.data)

+         self.assertIn(

+             '<a class="nav-link" href="/login/?next=http://localhost/">',

+             output.data)

+ 

+         user = tests.FakeUser(username='foo')

+         with tests.user_set(pagure.APP, user):

+             output = self.app.get('/logout/', follow_redirects=True)

+             self.assertEqual(output.status_code, 200)

+             self.assertIn('<title>Home - Pagure</title>', output.data)

+             self.assertIn('You have been logged out', output.data)

+             # Due to the way the tests are running we do not actually

+             # log out

+             self.assertIn(

+                 '<a href="/logout/?next=http://localhost/">log out</a>',

+                 output.data)

+ 

+ 

+ if __name__ == '__main__':

+     SUITE = unittest.TestLoader().loadTestsFromTestCase(PagureFlaskLogintests)

+     unittest.TextTestRunner(verbosity=2).run(SUITE)

@@ -0,0 +1,113 @@ 

+ # -*- coding: utf-8 -*-

+ 

+ """

+  (c) 2016 - Copyright Red Hat Inc

+ 

+  Authors:

+    Pierre-Yves Chibon <pingou@pingoured.fr>

+    Farhaan Bukhsh <farhaan.bukhsh@gmail.com>

+ 

+ """

+ 

+ __requires__ = ['SQLAlchemy >= 0.8']

+ import pkg_resources

+ 

+ import unittest

+ import shutil

+ import sys

+ import os

+ 

+ from mock import patch

+ 

+ sys.path.insert(0, os.path.join(os.path.dirname(

+     os.path.abspath(__file__)), '..'))

+ 

+ import pagure.lib

+ from pagure.exceptions import PagureException

+ import pagure.lib.login

+ import hashlib

+ from pagure import APP

+ import tests

+ 

+ 

+ class PagureLibLogintests(tests.Modeltests):

+     """ Tests for pagure.lib.login """

+ 

+     def test_id_generator(self):

+         ''' Test pagure.lib.login.id_generator. '''

+         self.assertEqual(

+             pagure.lib.login.id_generator(size=3, chars=['a']),

+             'aaa'

+         )

+ 

+     def test_get_users_by_group(self):

+         ''' Test pagure.lib.login.get_users_by_group. '''

+ 

+         users = pagure.lib.login.get_users_by_group(self.session, 'foo')

+         self.assertEqual(users, [])

+ 

+     def test_get_session_by_visitkey(self):

+         ''' Test pagure.lib.login.get_session_by_visitkey. '''

+ 

+         session = pagure.lib.login.get_session_by_visitkey(self.session, 'foo')

+         self.assertEqual(session, None)

+ 

+     def test_generate_hashed_value(self):

+         ''' Test pagure.lib.login.generate_hashed_value. '''

+         password = pagure.lib.login.generate_hashed_value('foo')

+         self.assertTrue(password.startswith('$2$'))

+         self.assertEqual(len(password), 63)

+ 

+     def test_check_password(self):

+         ''' Test pagure.lib.login.check_password. '''

+ 

+         # Version 2

+         password = pagure.lib.login.generate_hashed_value('foo')

+         self.assertTrue(

+             pagure.lib.login.check_password('foo', password))

+         self.assertFalse(

+             pagure.lib.login.check_password('bar', password))

+ 

+         # Version 1

+         password = '%s%s' % ('foo', APP.config.get('PASSWORD_SEED', None))

+         password = '$1$' + hashlib.sha512(password).hexdigest()

+         self.assertTrue(pagure.lib.login.check_password('foo', password))

+         self.assertFalse(pagure.lib.login.check_password('bar', password))

+ 

+         # Invalid password  -  No version

+         password = '%s%s' % ('foo', APP.config.get('PASSWORD_SEED', None))

+         password = hashlib.sha512(password).hexdigest()

+         self.assertRaises(

+             PagureException,

+             pagure.lib.login.check_password,

+             'foo', password

+         )

+ 

+         # Invalid password  -  Invalid version

+         password = '$3$' + password

+         self.assertRaises(

+             PagureException,

+             pagure.lib.login.check_password,

+             'foo',

+             password

+         )

+         password = '%s%s' % ('foo', APP.config.get('PASSWORD_SEED', None))

+         password = hashlib.sha512(password).hexdigest()

+         self.assertRaises(

+             PagureException,

+             pagure.lib.login.check_password,

+             'foo', password

+         )

+ 

+         # Invalid password  -  Invalid version

+         password = '$3$' + password

+         self.assertRaises(

+             PagureException,

+             pagure.lib.login.check_password,

+             'foo',

+             password

+         )

+ 

+ if __name__ == '__main__':

+     SUITE = unittest.TestLoader().loadTestsFromTestCase(PagureLibLogintests)

+     unittest.TextTestRunner(verbosity=2).run(SUITE)

no initial comment

This is a HORRIBLE way of checking the old password, since this is completely open to a timing side-channel attack.
Please, PLEASE, use a constant time comparison function!

For example, use: https://cryptography.readthedocs.org/en/latest/hazmat/primitives/constant-time/

This is very bad way of storing a password!
Please consider using bcrypt for password storage.
The problem is that the SHA family of functions is created in such a way that they are as fast as possible, which you do NOT want with password storage: you want to make it as hard as possible for an offline attacker to try lots of possibilities to check if they match (dictionary and/or brute-force attack).

How is password recovery working then?

Sounds like a good idea, def :thumbsup: for me

In order to improve the security, we may want to move all the code related to securing a password into 1 single function, allowing us adjust this one method to improve things

Using a setup-wide password "seed" (in the industry often called a "salt") is a very bad idea, as it doesn't deter attackers much: they just have to create a rainbow table for your setup instead of reusing one for all instances.

You should use a per-user salt so that they will have to brute-force every user by him-/herself.

Sorry, I meant "using ONLY a setup-wide password" is bad idea.
Using one in combination with a per-user salt is fine (but doesn't add much if the salt is generated securely).

After discussing with @puiterwijk, it seems that using https://pypi.python.org/pypi/bcrypt/ might solve most/all of the mentioned issues.

https://pypi.python.org/pypi/py-bcrypt is packaged for Fedora and EPEL7, so might be a good start

Left over I suppose :)

This will work but will break backward compatibility with all the instances currently deployed.

I see no changes in the requirements, which module is being used here?

We'll need unit-tests for this, especially to make sure the form works properly on checking that the password and confirm_password are indeed always equals

This also means we can drop the PASSWORD_SEED no? Check it (git grep should help) and if so please go ahead :)

Name this new_password perhaps?

As Toshio mentioned, this will fail if non-ASCII characters are inserted.
Please make sure to test that case as well.

(If you need test data: you can copy and paste "当たり")

Actually, it doesn't crash, and should work fine.

Perhaps a bit more sanity check? This is a very broad exception, and can mean a lot of things.

I would suggest to instead change the password into something like $1$<old-system> and $2$<bcrypt-version> so you can actually know which version of password storage is in use.

This will require an alembic change during the upgrade to make sure all current passwords are prefixed with $1$.

add intending to pass pep8.

When you get in this case, you have both the old style password hash and the unhashed pasword.

This means that you can now update the password to the new hash system!

NOTE: You are still using == for password comparison!
This is NOT constant-time, and as such a side channel data leakage!

I believe same_password expect this name

Or check if password[0] and password[2] are != $ no?

Well, sure, but then you're required to always support the version without version-indicator.
Just porting the current values on upgrade makes the most sense to me, as that would make the code neater.

Why this try-catch? You can just always use the to_unicode function.

Why do you have a separate if for this? Just store an password_version=1 and then after checking the password just below this code, convert it.

Why hash it yet again? bcrypt is slow (that's the exact reason you should use it), so this should be done as little as possible.
If you first check (as suggested before), you don't need this.

This is still not constant time.

Maybe extract away the prefixing and hashing of the password into a helper function, so it's easier to switch to $3$ in the future without having to change it at multiple places?

How about instead doing something like:
_, password_version, password = user_obj.password.split('$')

That way you can also handle possible versions like $2,loadfactor=50$ if we ever want to make the load factor variable.

This error message sounds quite strange.

How about instead doing something like:
_, password_version, password = user_obj.password.split('$')

That way you can also handle possible versions like $2,loadfactor=50$ if we ever want to make the load factor variable.

You may want to limit the split, in case the hash itself ever contains a '$'

Right, that's a good call.
_, password_version, password_hash = user_obj.password.split('$', 2)

pep8 would ask you for spaces around the + but this is a detail

what is version going to be on existing sha512 passwords?

In other words, will version ever gonna == '1'?

  • This is longer than 80 chars
  • How is this going to work for passwords in existing databases?

Similar to above

  • This is longer than 80 chars
  • Is version ever going to == '1'?

So if the if above is not true, we still say Password changed?

Probably best to do version == '1': and add an else: that just says "Something is wrong with your account". No uncertainties wanted with security.

As said, probably you want a function generate_hashed_password for the storage and updating functions.
That way, if the default every changes from bcrypt to X, you only need to update one place to generate the new schema instead of digging for all places.

Perhaps it's useful to add test cases, since this introduces some pretty interesting code.

Are we ever going to have a version == 1?

Probably best to do version == '1': and add an else: that just says "Something is wrong with your account". No uncertainties wanted with security.

I was more thinking along the lines of:
if there is no version -> check the current approach (sha512)

This allows to keep the app backward compatible w/o any changes to the DB ( I'm
not entirely sure I like the idea of touching the passwords stored in an alembic
migration)

Why not comparing here directly?

Should these two method remain in the controller?

you do string comparison using is not? Oo

I wonder if this part, that checks the validity of the old password shouldn't be using the same path/functions as we have upon login.

I was more thinking along the lines of:
if there is no version -> check the current approach (sha512)
This allows to keep the app backward compatible w/o any changes to the DB ( I'm
not entirely sure I like the idea of touching the passwords stored in an alembic
migration)

Well, the problem here is: are you 100% sure that the current approach will never create hashes that start with $X$?
Also, in that case you can't just do "_, version, user_password = user_obj.password.split('$', 2)" since that would crash horribly if there were no $'s in the password field.

Ok since bcrypt may use '$', then alembic migration it is, but then, it's still missing :)

Not comparing here directly because I am checking version and the performing the required conversion , comparing here will lead to one additional if in login and change password! Should I do it here ?

Should we move this code to the back-end as it seems to be redundant with what is in the login process?

Moreoever, this would make testing the code much easier.

We will need unit-tests before we can merge this, so after the alembic migration script this is the next big step/requirement.

Could you split this line? place the dict and the synchronize_session in another line (or two).

Two empty lines between functions

No need for two returns if we store the value in a variable

Just do a global 'return' at the end

Also: What happens if version is neither 1 nor 2? Shouldn't we say something?

Is this a 3 or 4 spaces indentation?

really sorry for that been writing JS for a while :rabbit:

How do you flask an error message from the backend library?

err, let's try again: how do you flash an error message? :)

I actually kinda wish this was in the backend as well, we have this line present twice at the moment, having it on the backend would allow having it only once.

And that would allow to do the split in the backend as well which is also something that I like :)

Looking at the code I think we may want to drop this, it's used in only one place and doesn't bring much

You are right ! If we are checking in check_password there is no need of this.

The imports here needs to be adjusted as well

While working on the tests I realized:

a) This method does not enforce being logged in?

b) Why requiring the username in the URL? Are we allowing to change someone else's password?

This is the place where I commented about the fact that user are not required to be logged in and that we can basically change anyone's password.

I think this is a bit vague, since if passsword_checks is True it first run this code and then also what comes after the end of the else block.

It would feel more readable if you just have an "if not password_checks" which aborts, and otherwise just falls through, just like in the login code.

Except for the remark with the if-blocks, this code looks good to me.

How can user_obj be None since we check if line 80?

Didn't use py-bcrypt finally?

I think you also touched this file :)

Btw, this is meant to be below the routes, I had replaced it there but looks like this got lost.

Add your name to the author list at the top

Let's adjust the © year to 2015-2016 while at it

Official :thumbsup: for me! Congrats @farhaan

@puiterwijk, are you ok as well?

The error message is incorrect, that's not a password lost error

That's the redundant if check

Only partially.
It should still check for password_checks.

Please note that this means that if someone requests a password reset for my user, I need to go to my email and reset my password before I can log back in: it won't accept my current password anymore.

This might be a Denial of Service if I can't reach my old email anymore (and was just going to update it), or otherwise just a major annoyance.

On the other side, it means if your password gets public that anyone can log in with it even during the time you're busy resetting it.

Well, I don't like using the email token as "security feature" in this way: it just doesn't make sense. I doubt that if someone accidentally publishes their password (in which case they're hosed anyway), they won't use the lost password feature: they'll use change password.

And again :thumbsup: for me :)