From 92dd958866e021b05a828b0d8c86e613af23dc99 Mon Sep 17 00:00:00 2001 From: rr- Date: Wed, 6 Apr 2016 17:56:34 +0200 Subject: [PATCH] client+server: finish password reminders --- client/html/login.hbs | 2 +- client/html/password_reset.hbs | 20 +++++ client/js/controllers/auth_controller.js | 39 ++++++++++ client/js/main.js | 5 ++ client/js/views/password_reset_view.js | 37 +++++++++ server/szurubooru/api/__init__.py | 2 +- ..._reminder_api.py => password_reset_api.py} | 10 +-- server/szurubooru/api/user_api.py | 1 - server/szurubooru/app.py | 4 +- .../tests/api/test_password_reset_api.py | 77 +++++++++++++++++++ server/szurubooru/tests/api/test_user_api.py | 69 ++++++----------- server/szurubooru/tests/api/util.py | 26 +++++++ server/szurubooru/util/auth.py | 2 +- server/szurubooru/util/users.py | 8 ++ 14 files changed, 246 insertions(+), 56 deletions(-) create mode 100644 client/html/password_reset.hbs create mode 100644 client/js/views/password_reset_view.js rename server/szurubooru/api/{password_reminder_api.py => password_reset_api.py} (85%) create mode 100644 server/szurubooru/tests/api/test_password_reset_api.py create mode 100644 server/szurubooru/tests/api/util.py diff --git a/client/html/login.hbs b/client/html/login.hbs index 651ac92..d6dd262 100644 --- a/client/html/login.hbs +++ b/client/html/login.hbs @@ -20,7 +20,7 @@
- Forgot the password? + Forgot the password?
diff --git a/client/html/password_reset.hbs b/client/html/password_reset.hbs new file mode 100644 index 0000000..0866317 --- /dev/null +++ b/client/html/password_reset.hbs @@ -0,0 +1,20 @@ +
+

Password reset

+
+
+
    +
  • + + +
    • +
+
+

Proceeding will send an e-mail that contains a password reset + link. Clicking it is going to generate a new password for your account. + It is recommended to change that password to something else.

+
+
+ +
+
+
diff --git a/client/js/controllers/auth_controller.js b/client/js/controllers/auth_controller.js index 607a21c..1a4565d 100644 --- a/client/js/controllers/auth_controller.js +++ b/client/js/controllers/auth_controller.js @@ -5,10 +5,12 @@ const page = require('page'); const api = require('../api.js'); const topNavController = require('../controllers/top_nav_controller.js'); const LoginView = require('../views/login_view.js'); +const PasswordResetView = require('../views/password_reset_view.js'); class AuthController { constructor() { this.loginView = new LoginView(); + this.passwordResetView = new PasswordResetView(); const auth = cookies.getJSON('auth'); if (auth && auth.user && auth.password) { @@ -51,6 +53,43 @@ class AuthController { page('/'); this.loginView.notifySuccess('Logged out'); } + + passwordResetRoute() { + topNavController.activate('login'); + this.passwordResetView.render({ + proceed: nameOrEmail => { + api.logout(); + cookies.remove('auth'); + return new Promise((resolve, reject) => { + api.get('/password-reset/' + nameOrEmail) + .then(() => { resolve(); }) + .catch(errorMessage => { reject(errorMessage); }); + }); + }}); + } + + passwordResetFinishRoute(name, token) { + api.logout(); + cookies.remove('auth'); + api.post('/password-reset/' + name, {token: token}) + .then(response => { + const password = response.password; + api.login(name, password) + .then(() => { + cookies.set( + 'auth', {'user': name, 'password': password}, {}); + page('/'); + this.passwordResetView.notifySuccess( + 'New password: ' + password); + }).catch(errorMessage => { + page('/'); + this.passwordResetView.notifyError(errorMessage); + }); + }).catch(response => { + page('/'); + this.passwordResetView.notifyError(response.description); + }); + } } module.exports = new AuthController(); diff --git a/client/js/main.js b/client/js/main.js index 6f386ff..c221d3d 100644 --- a/client/js/main.js +++ b/client/js/main.js @@ -44,6 +44,11 @@ page( page('/history', () => { historyController.showHistoryRoute(); }); page('/tags', () => { tagsController.listTagsRoute(); }); page('/comments', () => { commentsController.listCommentsRoute(); }); +page(/\/password-reset\/([^:]+):([^:]+)$/, + (ctx, next) => { + authController.passwordResetFinishRoute(ctx.params[0], ctx.params[1]); + }); +page('/password-reset', () => { authController.passwordResetRoute(); }); page('/login', () => { authController.loginRoute(); }); page('/logout', () => { authController.logoutRoute(); }); diff --git a/client/js/views/password_reset_view.js b/client/js/views/password_reset_view.js new file mode 100644 index 0000000..23b17ee --- /dev/null +++ b/client/js/views/password_reset_view.js @@ -0,0 +1,37 @@ +'use strict'; + +const BaseView = require('./base_view.js'); + +class PasswordResetView extends BaseView { + constructor() { + super(); + this.template = this.getTemplate('password-reset-template'); + } + + render(options) { + this.showView(this.template()); + const form = this.contentHolder.querySelector('form'); + this.decorateValidator(form); + + const userNameOrEmailField = document.getElementById('user-name'); + + form.addEventListener('submit', e => { + e.preventDefault(); + this.clearMessages(); + this.disableForm(form); + options + .proceed(userNameOrEmailField.value) + .then(() => { + this.notifySuccess( + 'E-mail has been sent. To finish the procedure, ' + + 'please click the link it contains.'); + }) + .catch(errorMessage => { + this.enableForm(form); + this.notifyError(errorMessage); + }); + }); + } +} + +module.exports = PasswordResetView; diff --git a/server/szurubooru/api/__init__.py b/server/szurubooru/api/__init__.py index 291b2ac..4f79c4b 100644 --- a/server/szurubooru/api/__init__.py +++ b/server/szurubooru/api/__init__.py @@ -1,4 +1,4 @@ ''' Falcon-compatible API facades. ''' -from szurubooru.api.password_reminder_api import PasswordReminderApi +from szurubooru.api.password_reset_api import PasswordResetApi from szurubooru.api.user_api import UserListApi, UserDetailApi diff --git a/server/szurubooru/api/password_reminder_api.py b/server/szurubooru/api/password_reset_api.py similarity index 85% rename from server/szurubooru/api/password_reminder_api.py rename to server/szurubooru/api/password_reset_api.py index da2870e..45ef042 100644 --- a/server/szurubooru/api/password_reminder_api.py +++ b/server/szurubooru/api/password_reset_api.py @@ -8,18 +8,18 @@ MAIL_BODY = \ 'If you wish to proceed, click this link: {url}\n' \ 'Otherwise, please ignore this email.' -class PasswordReminderApi(BaseApi): +class PasswordResetApi(BaseApi): def get(self, context, user_name): ''' Send a mail with secure token to the correlated user. ''' - user = users.get_by_name(context.session, user_name) + user = users.get_by_name_or_email(context.session, user_name) if not user: raise errors.NotFoundError('User %r not found.' % user_name) if not user.email: raise errors.ValidationError( 'User %r hasn\'t supplied email. Cannot reset password.' % user_name) token = auth.generate_authentication_token(user) - url = '%s/password-reset/%s' % ( - config.config['basic']['base_url'].rstrip('/'), token) + url = '%s/password-reset/%s:%s' % ( + config.config['basic']['base_url'].rstrip('/'), user.name, token) mailer.send_mail( 'noreply@%s' % config.config['basic']['name'], user.email, @@ -29,7 +29,7 @@ class PasswordReminderApi(BaseApi): def post(self, context, user_name): ''' Verify token from mail, generate a new password and return it. ''' - user = users.get_by_name(context.session, user_name) + user = users.get_by_name_or_email(context.session, user_name) if not user: raise errors.NotFoundError('User %r not found.' % user_name) good_token = auth.generate_authentication_token(user) diff --git a/server/szurubooru/api/user_api.py b/server/szurubooru/api/user_api.py index b0971e3..a36110b 100644 --- a/server/szurubooru/api/user_api.py +++ b/server/szurubooru/api/user_api.py @@ -1,4 +1,3 @@ -import sqlalchemy from szurubooru import errors, search from szurubooru.util import auth, users from szurubooru.api.base_api import BaseApi diff --git a/server/szurubooru/app.py b/server/szurubooru/app.py index ede59c5..1d33e08 100644 --- a/server/szurubooru/app.py +++ b/server/szurubooru/app.py @@ -64,7 +64,7 @@ def create_app(): user_list_api = api.UserListApi() user_detail_api = api.UserDetailApi() - password_reminder_api = api.PasswordReminderApi() + password_reset_api = api.PasswordResetApi() app.add_error_handler(errors.AuthError, _on_auth_error) app.add_error_handler(errors.IntegrityError, _on_integrity_error) @@ -74,6 +74,6 @@ def create_app(): app.add_route('/users/', user_list_api) app.add_route('/user/{user_name}', user_detail_api) - app.add_route('/password_reminder/{user_name}', password_reminder_api) + app.add_route('/password-reset/{user_name}', password_reset_api) return app diff --git a/server/szurubooru/tests/api/test_password_reset_api.py b/server/szurubooru/tests/api/test_password_reset_api.py new file mode 100644 index 0000000..1e7a43d --- /dev/null +++ b/server/szurubooru/tests/api/test_password_reset_api.py @@ -0,0 +1,77 @@ +from datetime import datetime +from unittest import mock +from szurubooru import api, db, errors, config +from szurubooru.util import auth, misc, mailer +from szurubooru.tests.database_test_case import DatabaseTestCase +from szurubooru.tests.api import util + +class TestPasswordReset(DatabaseTestCase): + def setUp(self): + super().setUp() + config_mock = { + 'basic': { + 'secret': 'x', + 'base_url': 'http://example.com/', + 'name': 'Test instance', + }, + } + self.old_config = config.config + config.config = config_mock + self.context = misc.dotdict() + self.context.session = self.session + self.context.request = {} + self.context.user = db.User() + self.api = api.PasswordResetApi() + + def test_reset_non_existing(self): + self.assertRaises(errors.NotFoundError, self.api.get, self.context, 'u1') + + def test_reset_without_email(self): + user = util.mock_user('u1', 'regular_user') + user.email = None + self.session.add(user) + self.assertRaises(errors.ValidationError, self.api.get, self.context, 'u1') + + def test_reset_sending_email(self): + user = util.mock_user('u1', 'regular_user') + user.email = 'user@example.com' + self.session.add(user) + for getter in ['u1', 'user@example.com']: + with mock.MagicMock() as mock_method: + mailer.send_mail = mock_method + self.assertEqual({}, self.api.get(self.context, getter)) + mock_method.assert_called_once_with( + 'noreply@Test instance', + 'user@example.com', + 'Password reset for Test instance', + 'You (or someone else) requested to reset your password ' + + 'on Test instance.\nIf you wish to proceed, click this l' + + 'ink: http://example.com/password-reset/u1:4ac0be176fb36' + + '4f13ee6b634c43220e2\nOtherwise, please ignore this email.') + + def test_confirmation_non_existing(self): + self.assertRaises(errors.NotFoundError, self.api.post, self.context, 'u1') + + def test_confirmation_no_token(self): + user = util.mock_user('u1', 'regular_user') + user.email = 'user@example.com' + self.session.add(user) + self.context.request = {} + self.assertRaises(errors.ValidationError, self.api.post, self.context, 'u1') + + def test_confirmation_bad_token(self): + user = util.mock_user('u1', 'regular_user') + user.email = 'user@example.com' + self.session.add(user) + self.context.request = {'token': 'bad'} + self.assertRaises(errors.ValidationError, self.api.post, self.context, 'u1') + + def test_confirmation_good_token(self): + user = util.mock_user('u1', 'regular_user') + user.email = 'user@example.com' + old_hash = user.password_hash + self.session.add(user) + self.context.request = {'token': '4ac0be176fb364f13ee6b634c43220e2'} + result = self.api.post(self.context, 'u1') + self.assertNotEqual(user.password_hash, old_hash) + self.assertTrue(auth.is_valid_password(user, result['password'])) diff --git a/server/szurubooru/tests/api/test_user_api.py b/server/szurubooru/tests/api/test_user_api.py index 362a0ad..5d106c2 100644 --- a/server/szurubooru/tests/api/test_user_api.py +++ b/server/szurubooru/tests/api/test_user_api.py @@ -2,30 +2,7 @@ from datetime import datetime from szurubooru import api, db, errors, config from szurubooru.util import auth, misc from szurubooru.tests.database_test_case import DatabaseTestCase - -def _create_user(name, rank='admin'): - user = db.User() - user.name = name - user.password = 'dummy' - user.password_salt = 'dummy' - user.password_hash = 'dummy' - user.email = 'dummy' - user.access_rank = rank - user.creation_time = datetime(1997, 1, 1) - user.avatar_style = db.User.AVATAR_GRAVATAR - return user - -def _mock_params(context, params): - def get_param_as_string(key, default=None): - if key not in params: - return default - return params[key] - def get_param_as_int(key, default=None): - if key not in params: - return default - return int(params[key]) - context.get_param_as_string = get_param_as_string - context.get_param_as_int = get_param_as_int +from szurubooru.tests.api import util class TestRetrievingUsers(DatabaseTestCase): def setUp(self): @@ -48,10 +25,10 @@ class TestRetrievingUsers(DatabaseTestCase): self.context.user = db.User() def test_retrieving_multiple(self): - user1 = _create_user('u1', 'mod') - user2 = _create_user('u2', 'mod') + user1 = util.mock_user('u1', 'mod') + user2 = util.mock_user('u2', 'mod') self.session.add_all([user1, user2]) - _mock_params(self.context, {'query': '', 'page': 1}) + util.mock_params(self.context, {'query': '', 'page': 1}) self.context.user.access_rank = 'regular_user' api_ = api.UserListApi() result = api_.get(self.context) @@ -63,15 +40,15 @@ class TestRetrievingUsers(DatabaseTestCase): def test_retrieving_multiple_without_privileges(self): self.context.user.access_rank = 'anonymous' - _mock_params(self.context, {'query': '', 'page': 1}) + util.mock_params(self.context, {'query': '', 'page': 1}) api_ = api.UserListApi() self.assertRaises(errors.AuthError, api_.get, self.context) def test_retrieving_single(self): - user = _create_user('u1', 'regular_user') + user = util.mock_user('u1', 'regular_user') self.session.add(user) self.context.user.access_rank = 'regular_user' - _mock_params(self.context, {'query': '', 'page': 1}) + util.mock_params(self.context, {'query': '', 'page': 1}) api_ = api.UserDetailApi() result = api_.get(self.context, 'u1') self.assertEqual(result['user']['id'], user.user_id) @@ -83,13 +60,13 @@ class TestRetrievingUsers(DatabaseTestCase): def test_retrieving_non_existing(self): self.context.user.access_rank = 'regular_user' - _mock_params(self.context, {'query': '', 'page': 1}) + util.mock_params(self.context, {'query': '', 'page': 1}) api_ = api.UserDetailApi() self.assertRaises(errors.NotFoundError, api_.get, self.context, '-') def test_retrieving_single_without_privileges(self): self.context.user.access_rank = 'anonymous' - _mock_params(self.context, {'query': '', 'page': 1}) + util.mock_params(self.context, {'query': '', 'page': 1}) api_ = api.UserDetailApi() self.assertRaises(errors.AuthError, api_.get, self.context, '-') @@ -200,7 +177,7 @@ class TestUpdatingUser(DatabaseTestCase): config.config = self.old_config def test_update_changing_nothing(self): - admin_user = _create_user('u1', 'admin') + admin_user = util.mock_user('u1', 'admin') self.session.add(admin_user) self.context.user = admin_user self.api.put(self.context, 'u1') @@ -210,13 +187,13 @@ class TestUpdatingUser(DatabaseTestCase): self.assertEqual(admin_user.access_rank, 'admin') def test_updating_non_existing_user(self): - admin_user = _create_user('u1', 'admin') + admin_user = util.mock_user('u1', 'admin') self.session.add(admin_user) self.context.user = admin_user self.assertRaises(errors.NotFoundError, self.api.put, self.context, 'u2') def test_admin_updating_everything_for_themselves(self): - admin_user = _create_user('u1', 'admin') + admin_user = util.mock_user('u1', 'admin') self.session.add(admin_user) self.context.user = admin_user self.context.request = { @@ -234,7 +211,7 @@ class TestUpdatingUser(DatabaseTestCase): self.assertFalse(auth.is_valid_password(admin_user, 'invalid')) def test_removing_email(self): - admin_user = _create_user('u1', 'admin') + admin_user = util.mock_user('u1', 'admin') self.session.add(admin_user) self.context.user = admin_user self.context.request = {'email': ''} @@ -243,7 +220,7 @@ class TestUpdatingUser(DatabaseTestCase): self.assertEqual(admin_user.email, None) def test_invalid_inputs(self): - admin_user = _create_user('u1', 'admin') + admin_user = util.mock_user('u1', 'admin') self.session.add(admin_user) self.context.user = admin_user self.context.request = {'name': '.'} @@ -260,8 +237,8 @@ class TestUpdatingUser(DatabaseTestCase): errors.ValidationError, self.api.put, self.context, 'u1') def test_user_trying_to_update_someone_else(self): - user1 = _create_user('u1', 'regular_user') - user2 = _create_user('u2', 'regular_user') + user1 = util.mock_user('u1', 'regular_user') + user2 = util.mock_user('u2', 'regular_user') self.session.add_all([user1, user2]) self.context.user = user1 for request in [ @@ -274,26 +251,28 @@ class TestUpdatingUser(DatabaseTestCase): errors.AuthError, self.api.put, self.context, user2.name) def test_user_trying_to_become_someone_else(self): - user1 = _create_user('me', 'regular_user') - user2 = _create_user('her', 'regular_user') + user1 = util.mock_user('me', 'regular_user') + user2 = util.mock_user('her', 'regular_user') self.session.add_all([user1, user2]) self.context.user = user1 self.context.request = {'name': 'her'} self.assertRaises( errors.IntegrityError, self.api.put, self.context, 'me') + self.session.rollback() def test_user_trying_to_become_someone_else_insensitive(self): - user1 = _create_user('me', 'regular_user') - user2 = _create_user('her', 'regular_user') + user1 = util.mock_user('me', 'regular_user') + user2 = util.mock_user('her', 'regular_user') self.session.add_all([user1, user2]) self.context.user = user1 self.context.request = {'name': 'HER'} self.assertRaises( errors.IntegrityError, self.api.put, self.context, 'me') + self.session.rollback() def test_mods_trying_to_become_admin(self): - user1 = _create_user('u1', 'mod') - user2 = _create_user('u2', 'mod') + user1 = util.mock_user('u1', 'mod') + user2 = util.mock_user('u2', 'mod') self.session.add_all([user1, user2]) self.context.user = user1 self.context.request = {'accessRank': 'admin'} diff --git a/server/szurubooru/tests/api/util.py b/server/szurubooru/tests/api/util.py new file mode 100644 index 0000000..31e1d20 --- /dev/null +++ b/server/szurubooru/tests/api/util.py @@ -0,0 +1,26 @@ +from datetime import datetime +from szurubooru import db + +def mock_user(name, rank='admin'): + user = db.User() + user.name = name + user.password = 'dummy' + user.password_salt = 'dummy' + user.password_hash = 'dummy' + user.email = 'dummy' + user.access_rank = rank + user.creation_time = datetime(1997, 1, 1) + user.avatar_style = db.User.AVATAR_GRAVATAR + return user + +def mock_params(context, params): + def get_param_as_string(key, default=None): + if key not in params: + return default + return params[key] + def get_param_as_int(key, default=None): + if key not in params: + return default + return int(params[key]) + context.get_param_as_string = get_param_as_string + context.get_param_as_int = get_param_as_int diff --git a/server/szurubooru/util/auth.py b/server/szurubooru/util/auth.py index 741e58b..1283301 100644 --- a/server/szurubooru/util/auth.py +++ b/server/szurubooru/util/auth.py @@ -53,7 +53,7 @@ def verify_privilege(user, privilege_name): def generate_authentication_token(user): ''' Generate nonguessable challenge (e.g. links in password reminder). ''' - digest = hashlib.sha256() + digest = hashlib.md5() digest.update(config.config['basic']['secret'].encode('utf8')) digest.update(user.password_salt.encode('utf8')) return digest.hexdigest() diff --git a/server/szurubooru/util/users.py b/server/szurubooru/util/users.py index edaa3ce..0455eb7 100644 --- a/server/szurubooru/util/users.py +++ b/server/szurubooru/util/users.py @@ -68,3 +68,11 @@ def get_by_name(session, name): return session.query(db.User) \ .filter(func.lower(db.User.name) == func.lower(name)) \ .first() + +def get_by_name_or_email(session, name_or_email): + ''' Retrieve an user by its name or email. ''' + return session.query(db.User) \ + .filter( + (func.lower(db.User.name) == func.lower(name_or_email)) + | (func.lower(db.User.email) == func.lower(name_or_email))) \ + .first()